Closed Bug 1288127 Opened 6 years ago Closed 6 years ago

Implement revised Bookmarks panel layout


(Firefox for Android Graveyard :: Awesomescreen, defect, P3)



(Not tracked)



(Reporter: sebastian, Assigned: ahunt)



(Whiteboard: [MobileAS])


(7 files)

Component: General → Awesomescreen
Depends on: 1289242
One questions I have here is what happens with bookmark folders in this scenario. I think bookmark folders are rarely used on Android (especially given that you can't do folder management in Fennec), but we'd need to check this.

Note to self: look at the Fennec telemetry for folders.
FWIW it's not too hard to get a basic version of this working: we can reuse the History panel code, in fact I'm making the CombinedHistoryPanel configurable so we can enable/disable synced tabs, recently closed tabs, and switching between history and bookmarks as the main data source (some work will be needed so that we can pass DATE_CREATED through to the adapter that handles the bookmarks, since it currently assumes we have history with a last-visited time).
Assignee: nobody → ahunt
Whiteboard: [MobileAS s1.1]
I'm probably going to split this into two bugs:

1) Implement the time-based sorting / splitting (i.e. reusing the CombinedHistoryPanel to show bookmarks).
2) Changing the appearance of the items (i.e. TwoLinePageRow), which is shared with Bug 1288128. For now I'll track that work in the same bug, since it doesn't really require any significant changes apart from the appearance of the items.
This provides a sorted list of bookmarks suitable for feeding into the CombinedHistoryPanel
(which will in future be a switchable combined history and bookmarks panel). This panel
sections history (or bookmarks) based on when they were last visited (or created in case
of bookmarks).

Review commit:
See other reviews:
Attachment #8775677 - Flags: review?(gkruglov)
Attachment #8775680 - Flags: review?(gkruglov)
Attachment #8775681 - Flags: review?(gkruglov)
This class can handle both history and bookmarks, lets make the naming clear.

Review commit:
See other reviews:
We want to generalise the CombinedHistoryPanel to allow showing a list of
chronoligically sorted and separated bookmarks. The smartfolders aren't
relevant to bookmarks, hence we need to be able to disable them.

Review commit:
See other reviews:
This is necessary when we're using this fragment in a normal ViewPager. However
HomePager has some special logic for loading, hence this code might need to be
disabled for HomePager usage.

Review commit:
See other reviews:
Comment on attachment 8775677 [details]
Bug 1288127 - Pre: Implement getRecentBookmarks (equivalent to getRecentHistory)

::: mobile/android/base/java/org/mozilla/gecko/db/
(Diff revision 1)
>                          null,
>                          null);
>      }
>      @Override
> +    public Cursor getRecentBookmarks(ContentResolver cr, int limit) {

Attachment #8775677 - Flags: review?(gkruglov) → review+
Comment on attachment 8775680 [details]
Bug 1288127 - Handle pure bookmark cursors in CombinedHistoryAdapter too

::: mobile/android/base/java/org/mozilla/gecko/home/
(Diff revision 1)
> +                final long bookmarkId;
> -                final int bookmarkIdCol = historyCursor.getColumnIndexOrThrow(BrowserContract.Combined.BOOKMARK_ID);
> -                final long bookmarkId = historyCursor.getLong(bookmarkIdCol);
> +                if (bookmarkIdCol != -1) {
> +                    bookmarkId = historyCursor.getLong(bookmarkIdCol);
> +                } else {
> +                    bookmarkId = historyCursor.getLong(historyCursor.getColumnIndexOrThrow(BrowserContract.Bookmarks._ID));

Can add a comment describing the two types of bookmarks we might see here (one from the combined cursor, one from a recent bookmarks cursor).
Attachment #8775680 - Flags: review?(gkruglov) → review+
Comment on attachment 8775681 [details]
Bug 1288127 - Add Bookmarks mode to CombinedHistoryPanel
Attachment #8775681 - Flags: review?(gkruglov) → review+
As a p.s.: While this works for now, I'm not entirely sold on retrofitting this stuff into an existing adapter/panel - it feels a little brittle. On the other hand, now is probably not the right time to start refactoring this code either...
Whiteboard: [MobileAS s1.1] → [MobileAS s1.2]
Comment on attachment 8775679 [details]
Bug 1288127 - Allows disabling smartfolders mode in CombinedHistoryAdapter

I guess this will change after the patches in bug 1289242 changed.
Attachment #8775679 - Flags: review?(s.kaspari)
Attachment #8775682 - Flags: review?(s.kaspari)
Whiteboard: [MobileAS s1.2] → [MobileAS backlog]
Whiteboard: [MobileAS backlog] → [MobileAS]
Priority: -- → P3
Rank: 6
I'll close this for now: It is not part of the MVP and the current mocks for later stages have changed so that we won't have a bookmarks-only panel.
Closed: 6 years ago
Resolution: --- → INVALID
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.