Implement revised Bookmarks panel layout

RESOLVED INVALID

Status

()

Firefox for Android
Awesomescreen
P3
normal
Rank:
6
RESOLVED INVALID
2 years ago
2 years ago

People

(Reporter: sebastian, Assigned: ahunt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MobileAS])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Reporter)

Updated

2 years ago
Component: General → Awesomescreen
(Reporter)

Updated

2 years ago
Depends on: 1289242
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
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
(Assignee)

Updated

2 years ago
Whiteboard: [MobileAS s1.1]
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
Created attachment 8775677 [details]
Bug 1288127 - Pre: Implement getRecentBookmarks (equivalent to getRecentHistory)

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: https://reviewboard.mozilla.org/r/67820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67820/
Attachment #8775677 - Flags: review?(gkruglov)
Attachment #8775680 - Flags: review?(gkruglov)
Attachment #8775681 - Flags: review?(gkruglov)
(Assignee)

Comment 5

2 years ago
Created attachment 8775678 [details]
Bug 1288127 - Pre: rename CombinedHistoryItem.HistoryItem to BookmarkHistoryItem

This class can handle both history and bookmarks, lets make the naming clear.

Review commit: https://reviewboard.mozilla.org/r/67822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67822/
(Assignee)

Comment 6

2 years ago
Created attachment 8775679 [details]
Bug 1288127 - Allows disabling smartfolders mode in CombinedHistoryAdapter

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: https://reviewboard.mozilla.org/r/67824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67824/
(Assignee)

Comment 7

2 years ago
Created attachment 8775680 [details]
Bug 1288127 - Handle pure bookmark cursors in CombinedHistoryAdapter too

Review commit: https://reviewboard.mozilla.org/r/67826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67826/
(Assignee)

Comment 8

2 years ago
Created attachment 8775681 [details]
Bug 1288127 - Add Bookmarks mode to CombinedHistoryPanel

Review commit: https://reviewboard.mozilla.org/r/67828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67828/
(Assignee)

Comment 9

2 years ago
Created attachment 8775682 [details]
Bug 1288127 - load() History/Bookmarks during onResume() to ensure data is loaded

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: https://reviewboard.mozilla.org/r/67830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67830/
Comment on attachment 8775677 [details]
Bug 1288127 - Pre: Implement getRecentBookmarks (equivalent to getRecentHistory)

https://reviewboard.mozilla.org/r/67820/#review66204

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

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

https://reviewboard.mozilla.org/r/67826/#review66206

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:252
(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

https://reviewboard.mozilla.org/r/67828/#review66212
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...

Updated

2 years ago
Whiteboard: [MobileAS s1.1] → [MobileAS s1.2]

Updated

2 years ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 20

2 years ago
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)
(Reporter)

Updated

2 years ago
Attachment #8775682 - Flags: review?(s.kaspari)

Updated

2 years ago
Whiteboard: [MobileAS s1.2] → [MobileAS backlog]

Updated

2 years ago
Whiteboard: [MobileAS backlog] → [MobileAS]
(Reporter)

Updated

2 years ago
Priority: -- → P3

Updated

2 years ago
Rank: 6
(Reporter)

Comment 21

2 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.