1206627 - Offer to pin homepage to topsites when set

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

When setting a homepage, we should offer to add it to top sites.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: MozReview Request: Bug 1206627 - Offer to pin homepage to topsites when set; r?margaret

Bug 1206627 - Offer to pin homepage to topsites when set; r?margaret

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

https://reviewboard.mozilla.org/r/19851/#review18021

::: mobile/android/base/TelemetryContract.java:59
(Diff revision 1)
> +        HOMEPAGE("homepage.1"),

Can we reuse "edit.1" for this? It is a setting and we normally capture simple settings using ("edit.1", "setting", prefName)

::: mobile/android/base/TelemetryContract.java:62
(Diff revision 1)
> +        HOMEPAGE_TOP_SITES_PIN("homepage.pin.1"),

Can we just reuse "pin.1" ?

::: mobile/android/base/preferences/SetHomepagePreference.java:134
(Diff revision 1)
> +            Telemetry.sendUIEvent(TelemetryContract.Event.HOMEPAGE);

Note, you are missing the Method (we always send) and Extra (which we sometimes send).

But I would use:
Telemetry.sendUIEvent(TelemetryContract.Event.EDIT, TelemetryContract.Method.SETTING, "homepage");

::: mobile/android/base/preferences/SetHomepagePreference.java:149
(Diff revision 1)
> +                            // We'll pin the homepage to the first position in top sites, but first we have to shuffle the other

Ugh, but I guess we need to do it

::: mobile/android/base/preferences/SetHomepagePreference.java:191
(Diff revision 1)
> +                        Telemetry.sendUIEvent(TelemetryContract.Event.HOMEPAGE_TOP_SITES_PIN);

Note, you are missing the Method (we always send) and Extra (which we sometimes send).

But I would use:
Telemetry.sendUIEvent(TelemetryContract.Event.PIN, TelemetryContract.Method.SETTING, "homepage");

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I wonder how this affects or tackles bug 1206155. 

Since this bug is a rather specific solution (works best if the user has set their default as "Top Sites", for example), I'd like to take a swing at bug 1206155 before heading too far down this path. I feel like they tackle a similar use case.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #3)
> I wonder how this affects or tackles bug 1206155. 
> 
> Since this bug is a rather specific solution (works best if the user has set
> their default as "Top Sites", for example), I'd like to take a swing at bug
> 1206155 before heading too far down this path. I feel like they tackle a
> similar use case.

I am making this block bug 1206155, since it is one of the ways to satisfy that bug. Note that Top Sites _is_ the default panel. The vast majority of our users leave it set that way.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8664142 [details]
MozReview Request: Bug 1206627 - Offer to pin homepage to topsites when set; r?margaret

https://reviewboard.mozilla.org/r/19851/#review18097

This looks like it's the right approach, but I want us to double check the UX with Anthony, and I think it would be good to get feedback from rnewman on the DB operations.

::: mobile/android/base/db/LocalBrowserDB.java:1604
(Diff revision 1)
> +                appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true").build();

A lot of this seems redundant with the `pinSite` method above. Is there any way to share the combined logic?

::: mobile/android/base/db/LocalBrowserDB.java:1613
(Diff revision 1)
> +        operations.add(builder.build());

I'm not familiar with this ContentProviderOperation API, it may be good to get a once-over from a second set of eyes, like rnewman.

::: mobile/android/base/preferences/SetHomepagePreference.java:136
(Diff revision 1)
> +            if (pinToTopSitesCheck.isChecked()) {

To avoid so many levels of indentation, it may be nice to move this into a helper method.

::: mobile/android/base/preferences/SetHomepagePreference.java:139
(Diff revision 1)
> +                final ContentResolver contentResolver = getContext().getContentResolver();

Nit: you could use the local `context` variable here, instead of calling `getContext()` again.

::: mobile/android/base/preferences/SetHomepagePreference.java:149
(Diff revision 1)
> +                            // We'll pin the homepage to the first position in top sites, but first we have to shuffle the other

Yeah, this is kinda icky, but I agree it's a bad experience to just pave over someone's original pinned site.

::: mobile/android/base/preferences/SetHomepagePreference.java:151
(Diff revision 1)
> +                            Cursor pinnedSitesCursor = db.getPinnedSites(contentResolver, 0);

Can you comment on what the contents of this cursor is? Are these sites guaranteed to be returned in any particiular order?

What happens if the user already has sites pinned in all the available spots? Do we run into problems trying to bump a site to the 7th spot (on phones)? We should double check any assumptions we make about consuming these pinned sites.

Is there any way to write an automated test for this? Maybe we should consider moving this pinned site shifting logic into a unit-testable method.

::: mobile/android/base/preferences/SetHomepagePreference.java:171
(Diff revision 1)
> +                                                pinnedSitesCursor.getLong(pinnedSitesCursor.getColumnIndex(BrowserContract.Bookmarks.DATE_MODIFIED)),

I think we should probably consider this modified right now, so we should update the DATE_MODIFIED to the current time (or we may have logic elsewhere for that to automatically happen).

::: mobile/android/base/preferences/SetHomepagePreference.java:173
(Diff revision 1)
> +                                                pinnedSitesCursor.getInt(pinnedSitesCursor.getColumnIndex(BrowserContract.Bookmarks.IS_DELETED)));

A visible pinned site should never be deleted, so I don't think we need to actually update this.

::: mobile/android/base/resources/layout/preference_set_homepage.xml:47
(Diff revision 1)
> +                  android:layout_marginTop="8dp"

Did you run the UX here by Anthony? Could be good to share a screenshot.

::: mobile/android/base/resources/layout/preference_set_homepage.xml:56
(Diff revision 1)
> +                  tools:text="Pin to top sites" />

Is this tools:text attribute necessary?

Comment 6

[Martyn Haigh (:mhaigh)]

> 
> What happens if the user already has sites pinned in all the available
> spots? Do we run into problems trying to bump a site to the 7th spot (on
> phones)? We should double check any assumptions we make about consuming
> these pinned sites.

This is an issue which I'm not sure what to do with.  If we offer to pin the homepage as a topsite then it makes sense to pin it as #1, but in doing this we may be forced to push one of their existing pinned sites in to obscurity, effectively deleting it (there's no way to see more than 6 top sites).    

Options would be:
- to not allow the user to pin if they already have a full set of pinned sites
- we could prompt the user that they need to remove one sites before we can pin 
- we could display a dialog so that they can choose a pinned site to remove

I think doing nothing to address this issue is the worst idea.

antlam: any ideas on this front?
 
> 
> Is there any way to write an automated test for this? Maybe we should
> consider moving this pinned site shifting logic into a unit-testable method.

Let's wait until we have a UX decision on direction, we may end up changing this code.


> Did you run the UX here by Anthony? Could be good to share a screenshot.

screenshot incoming!

> 
> ::: mobile/android/base/resources/layout/preference_set_homepage.xml:56
> (Diff revision 1)
> > +                  tools:text="Pin to top sites" />
> 
> Is this tools:text attribute necessary?

It's not necessary, but it's incredibly handy when developing in an IDE.  It allows you to see what the layout is going to look like without having to deploy and gets stripped out of the final layout without having any impact on the runtime - http://tools.android.com/tips/layout-designtime-attributes

Comment 7

[Martyn Haigh (:mhaigh)]

Attachment: Screenshot of settings with top site checkbox

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Just a heads up. Sounds like Anthony is also exploring the use of a Material design floating button as an easy way to access the Home page from the Home Panels. That approach, if suitable/acceptable, would be easier and more imsple than this approach. Less complexity, I think.

Comment 9

[Martyn Haigh (:mhaigh)]

There was possible talk of doing some A/B testing, so wanted to get this in a good state in case that option pans out.

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Since comment 8 is about a FAB button, I'll post the mocks in bug 1206155 to avoid confusion.

Comment 11

[:Margaret Leibovic]

We're not planning on doing this right now. We can repoen or file a new bug if we ever decide to do this.