Closed Bug 1319274 Opened 8 years ago Closed 7 years ago

[AS] [Pinned] Add support for pinned sites, as part of A-S Top Sites

Categories

(Firefox for Android Graveyard :: General, defect, P1)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect

Tracking

(firefox53 fixed)

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: Grisha, Assigned: Grisha)

References

Details

(Whiteboard: [MobileAS])

Attachments

(3 files)

Bug 1319274 - Part 1: Use regular getTopSites method for A-S top sites
58 bytes, text/x-review-board-request
ahunt
: review+
Details
Bug 1319274 - Part 2: Display a pin icon besides the Top Site title
58 bytes, text/x-review-board-request
ahunt
: review+
Details
Bug 1319274 - Part 3: Allow pinning and unpinning of both Top Sites and Highlights
58 bytes, text/x-review-board-request
ahunt
: review+
Details 
Currently we display pinned sites as part of regular Top Sites.

This work is waiting on UX, but it seems feasible to do the same for AS Top Sites.

We already have a Top Sites query which includes Pinned Sites in its results. Currently AS Top Sites is using a "plain top sites query", which does not. It might be possible to just switch which query we're using.

Additionally, we need to indicate that an item shown is pinned. This is also waiting for UX.
Blocks: 1314725
(In reply to :Grisha Kruglov from comment #0)
> We already have a Top Sites query which includes Pinned Sites in its
> results. Currently AS Top Sites is using a "plain top sites query", which
> does not. It might be possible to just switch which query we're using.

Yup. IIRC the AS topsites query is just a copy of the usual/old topsites query, with all the pinning logic removed - it *should* have all the same columns in place (best to test this though). And we can hopefully remove the non-pinned query as part of this bug too.
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Comment on attachment 8816391 [details]
Bug 1319274 - Part 1: Use regular getTopSites method for A-S top sites

https://reviewboard.mozilla.org/r/97142/#review97574
Attachment #8816391 - Flags: review?(ahunt) → review+
Comment on attachment 8816392 [details]
Bug 1319274 - Part 2: Display a pin icon besides the Top Site title

https://reviewboard.mozilla.org/r/97144/#review97580
Attachment #8816392 - Flags: review?(ahunt) → review+
Comment on attachment 8816393 [details]
Bug 1319274 - Part 3: Allow pinning and unpinning of both Top Sites and Highlights

https://reviewboard.mozilla.org/r/97146/#review97584

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDB.java:168
(Diff revision 2)
>  
> +    // Used by regular top sites, which observe pinning position.
>      public abstract void pinSite(ContentResolver cr, String url, String title, int position);
>      public abstract void unpinSite(ContentResolver cr, int position);
>  
> +    // Used by activity stream top sites, which ignore position - it's always 0.

One issue here is that unpinning/pin management will be broken if you revert to old topsites. Old TopSites is written with the assumption that pinned positions are correct, and starts acting weirdly in other cases (unpinning is broken, I don't think we'll crash anymore though).

I'm not sure what a good solution would be. Maybe we can only select pinned sites with position >= 0 for old topsites, and use position == -1 (or some other magic value) for AS topsites? (IIRC the position indeces start with 0 for old topsites)?

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/topsites/TopSitesPageAdapter.java:89
(Diff revision 2)
>              final int type = cursor.getInt(cursor.getColumnIndexOrThrow(BrowserContract.TopSites.TYPE));
>  
> -            topSites.add(new TopSite(id, url, title, type));
> +            final int bookmarkIdColumn = cursor.getColumnIndexOrThrow(BrowserContract.Combined.BOOKMARK_ID);
> +            // Top Sites query already takes care of excluding "special" bookmarks (pinned sites, etc),
> +            // so it should be safe to simply check for presence of a bookmark_id.
> +            final boolean isBookmarked = !cursor.isNull(bookmarkIdColumn);

In getTopSites, suggested sites receive bookmark_id = -1. I'm guessing that changing that to NULL should make us not treat them as bookmarked. We'd need to make sure old topsites doesn't break, but IIRC it doesn't care too much whether bookmark_id is -1 or NULL.

However this also results in pinned sites being considered bookmarked. I'm not sure how best to untangle this. I think we'll have to query whether a pinned item is also bookmarked somewhere - maybe it's easiest to keep the bookmark state query in the context menu to ensure that it's always correct?

::: mobile/android/base/resources/menu/activitystream_contextmenu.xml:9
(Diff revision 2)
>      <group android:id="@+id/group0">
>          <item
>              android:id="@+id/bookmark"
>              android:icon="@drawable/as_bookmark"
>              android:title="@string/bookmark"/>
>          <item

On an N9 the pin looks a bit blurry (not noticeable on older devices though), I'm wondering whether it's worth filing a followup to replace it with a VectorDrawable?
Comment on attachment 8816393 [details]
Bug 1319274 - Part 3: Allow pinning and unpinning of both Top Sites and Highlights

https://reviewboard.mozilla.org/r/97146/#review97590

I've got some concerns around compatibility with old topsites, and also how we determine bookmark state for pinned sites - I'm not really sure what a good solution would be yet though. It might be worth discussing whether we want to simplify old topsites to not require pinning positions (especially if it's likely to be scrapped soon), which should make things simpler?

Since we already have the positioning logic in place though, do we definitely want to scrap it?
I think the simplest solution which I'll investigate is using a different fake parent ID for A-S top sites. We should be able to separate two types of pinned sites entirely this way.
Distribution bookmarks support "pinned" flag, which creates a "pinned bookmark". If we're splitting pins into two types, we have two options:
1) create both types of pins
2) create just the regular top sites pins, and add AS support later on once it's necessary.

I think I'll just file a follow-up bug for option 2.
Blocks: 1323105
Comment on attachment 8816393 [details]
Bug 1319274 - Part 3: Allow pinning and unpinning of both Top Sites and Highlights

https://reviewboard.mozilla.org/r/97146/#review97584

> One issue here is that unpinning/pin management will be broken if you revert to old topsites. Old TopSites is written with the assumption that pinned positions are correct, and starts acting weirdly in other cases (unpinning is broken, I don't think we'll crash anymore though).
> 
> I'm not sure what a good solution would be. Maybe we can only select pinned sites with position >= 0 for old topsites, and use position == -1 (or some other magic value) for AS topsites? (IIRC the position indeces start with 0 for old topsites)?

Introduced an "AS pin", which doesn't overlap with regular pins. Two sets of pins are independent, and do not affect each other.

> In getTopSites, suggested sites receive bookmark_id = -1. I'm guessing that changing that to NULL should make us not treat them as bookmarked. We'd need to make sure old topsites doesn't break, but IIRC it doesn't care too much whether bookmark_id is -1 or NULL.
> 
> However this also results in pinned sites being considered bookmarked. I'm not sure how best to untangle this. I think we'll have to query whether a pinned item is also bookmarked somewhere - maybe it's easiest to keep the bookmark state query in the context menu to ensure that it's always correct?

I've ended up keeping bookmark query in the context menu, and introducing a "is this pinned" query as well. Turns out you can only reliable tell as pinned/bookmarked state in a handful of cases, and most of the time you're looking up at least one of them when the menu opens. This is unfortunate, but necessary with the current underlying queries. See my commit comment for Part 3.

> On an N9 the pin looks a bit blurry (not noticeable on older devices though), I'm wondering whether it's worth filing a followup to replace it with a VectorDrawable?

Certainly. Filed Bug 1323105.
Priority: P2 → P1
Blocks: 1321368
Comment on attachment 8816393 [details]
Bug 1319274 - Part 3: Allow pinning and unpinning of both Top Sites and Highlights

https://reviewboard.mozilla.org/r/97146/#review98736

\o/
Attachment #8816393 - Flags: review?(ahunt) → review+
Iteration: --- → 1.11
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41be921ce6fb
Part 1: Use regular getTopSites method for A-S top sites r=ahunt
https://hg.mozilla.org/integration/autoland/rev/a972030923fe
Part 2: Display a pin icon besides the Top Site title r=ahunt
https://hg.mozilla.org/integration/autoland/rev/97a622e11a8c
Part 3: Allow pinning and unpinning of both Top Sites and Highlights r=ahunt
Summary: [AS] [Pinned] Display pinned sites, probably as part of Top Sites → [AS] [Pinned] Add support for pinned sites, as part of A-S Top Sites
https://hg.mozilla.org/mozilla-central/rev/41be921ce6fb
https://hg.mozilla.org/mozilla-central/rev/a972030923fe
https://hg.mozilla.org/mozilla-central/rev/97a622e11a8c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Tested on latest Nightly build 54.0a1 (2017-02-23) using following devices:
- Asus ZenPad 8 (Android 6.0.1);
- HTC 10 (Android 6.0.1).

Context menu displays and allows to Pin/Unpin websites for both Top sites and Highlights.
Also, the pin icon is displayed to the left side of the Top site title.

I'm marking this as Verified.
Status: RESOLVED → VERIFIED
See Also: → 1407217
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: