1288127 - Implement revised Bookmarks panel layout

Status: RESOLVED INVALID

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Screenshot 2016-07-20 10.49.06.png

WIP Invision mockups: https://mozilla.invisionapp.com/share/VW7ZBXV8Q#/screens/174481420

Trello card: https://trello.com/c/jnJ0qMOK/52-i-want-easy-access-to-my-most-interesting-recent-history

Comment 1

[Andrzej Hunt :ahunt]

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.

Comment 2

[Andrzej Hunt :ahunt]

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).

Comment 3

[Andrzej Hunt :ahunt]

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.

Comment 4

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 5

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 7

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 8

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 9

[Andrzej Hunt :ahunt]

Attachment: 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 10

[:Grisha Kruglov]

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

Comment 11

[:Grisha Kruglov]

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).

Comment 12

[:Grisha Kruglov]

Comment on attachment 8775681 [details]
Bug 1288127 - Add Bookmarks mode to CombinedHistoryPanel

https://reviewboard.mozilla.org/r/67828/#review66212

Comment 13

[:Grisha Kruglov]

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...

Comment 14

[Andrzej Hunt :ahunt]

Comment on attachment 8775677 [details]
Bug 1288127 - Pre: Implement getRecentBookmarks (equivalent to getRecentHistory)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67820/diff/1-2/

Comment 15

[Andrzej Hunt :ahunt]

Comment on attachment 8775678 [details]
Bug 1288127 - Pre: rename CombinedHistoryItem.HistoryItem to BookmarkHistoryItem

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67822/diff/1-2/

Comment 16

[Andrzej Hunt :ahunt]

Comment on attachment 8775679 [details]
Bug 1288127 - Allows disabling smartfolders mode in CombinedHistoryAdapter

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67824/diff/1-2/

Comment 17

[Andrzej Hunt :ahunt]

Comment on attachment 8775680 [details]
Bug 1288127 - Handle pure bookmark cursors in CombinedHistoryAdapter too

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67826/diff/1-2/

Comment 18

[Andrzej Hunt :ahunt]

Comment on attachment 8775681 [details]
Bug 1288127 - Add Bookmarks mode to CombinedHistoryPanel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67828/diff/1-2/

Comment 19

[Andrzej Hunt :ahunt]

Comment on attachment 8775682 [details]
Bug 1288127 - load() History/Bookmarks during onResume() to ensure data is loaded

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67830/diff/1-2/

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

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.