1257345 - Implement Readermode/Reading-list smart folder

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Andrzej Hunt :ahunt]

Bug 134315 takes care of the actual reading-list migration, however the smartfolder isn't a hard requirement for the reading-list migration to happen, hence I'm splitting this out into a separate bug.

There are still a few open questions around this:
* Do we show real bookmarks, which happen to be readermoded
** If so what does deletion do: de-readermode, or delete bookmark?
** Do we deduplicate bookmarks - if so, which bookmark do we use?
* Do we create a synthetic readermode list (similarly to screenshots)
** How does this interact with bookmarks when we try to remove an item?
** How do we ensure this is kept in sync with bookmarks? If we don't generate this list directly from bookmarks we could end up with readermode items that aren't a bookmark (it's possible to delete bookmarks on desktop, in which case we wouldn't remove the readercache entry automatically).

Comment 1

[Andrzej Hunt :ahunt]

Oops, wrong link in the comment 0, that should be Bug 1234315.

Comment 2

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1257345 - Implement reading list smartfolder r?mcomella

Review commit: https://reviewboard.mozilla.org/r/42281/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42281/

Comment 3

[Andrzej Hunt :ahunt]

Comment on attachment 8734492 [details]
MozReview Request: Bug 1257345 - Implement reading list smartfolder r?mcomella

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

Comment 4

[Andrzej Hunt :ahunt]

Comment on attachment 8734492 [details]
MozReview Request: Bug 1257345 - Implement reading list smartfolder r?mcomella

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

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8734492 [details]
MozReview Request: Bug 1257345 - Implement reading list smartfolder r?mcomella

https://reviewboard.mozilla.org/r/42281/#review40903

r+ since the `GROUP BY` issue is a query and is probably harmless – but it's probably not what you want.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:796
(Diff revision 3)
>  
>              // We'll add a fake "Desktop Bookmarks" folder to the root view if desktop
>              // bookmarks exist, so that the user can still access non-mobile bookmarks.
>              addDesktopFolder = desktopBookmarksExist(cr);
>              addScreenshotsFolder = AppConstants.SCREENSHOTS_IN_BOOKMARKS_ENABLED;
> +            addReadingListFolder = true;

Do we want to wrap this in a build flag (like screenshots)? Does that happen elsewhere?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:820
(Diff revision 3)
>                                          Bookmarks.MENU_FOLDER_GUID,
>                                          Bookmarks.UNFILED_FOLDER_GUID },
>                           null);
>          } else if (folderId == Bookmarks.FIXED_SCREENSHOT_FOLDER_ID) {
>              c = getUrlAnnotations().getScreenshots(cr);
> +        } else if (folderId == Bookmarks.FAKE_READINGLIST_SMARTFOLDER_ID) {

nit: The containing method is already pretty big – can we put this in a helper method?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:826
(Diff revision 3)
> +            // group by URL to avoid having duplicate bookmarks listed. It's possible to have multiple
> +            // bookmarks pointing to the same URL (this would most commonly happen by manually
> +            // copying bookmarks on desktop, followed by syncing with mobile), and we don't want
> +            // to show the same URL multiple times in the reading list folder.
> +            final Uri bookmarksGroupedByUri = mBookmarksUriWithProfile.buildUpon()
> +                    .appendQueryParameter(BrowserContract.PARAM_GROUP_BY, Bookmarks.URL)

With an aggregation function (e.g. `SUM`, `COUNT`), I don't think `GROUP BY` is what you want here. Would `DISTINCT` work for your use case?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:836
(Diff revision 3)
> +                         Bookmarks.ANNOTATION_KEY + " == ? AND " +
> +                         Bookmarks.ANNOTATION_VALUE + " == ? AND " +
> +                         "(" + Bookmarks.TYPE + " = ? AND " + Bookmarks.URL + " IS NOT NULL)",
> +                         new String[] {
> +                                 BrowserContract.UrlAnnotations.Key.READER_VIEW.getDbValue(),
> +                                 Boolean.toString(true),

To be clear, we only want to show things that are reader-mode-able? I just want to make sure I'm up-to-date on the reading list semantics here.

I think it'd be safer to store `Boolean.toString(true)` as a static constant somewhere rather than duplicating the method and potentially getting it wrong (e.g. it's also called in `LocalUrlAnnotations.insertReaderviewUrl`.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:865
(Diff revision 3)
>          }
>      }
>  
>      @CheckResult
> -    private ArrayList<Cursor> getSpecialFoldersCursorList(final boolean addDesktopFolder, final boolean addScreenshotsFolder) {
> +    private ArrayList<Cursor> getSpecialFoldersCursorList(final boolean addDesktopFolder,
> +            final boolean addScreenshotsFolder, final boolean addReadermodeFolder) {

nit: -> `addReadingListFolder` ?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:866
(Diff revision 3)
>      }
>  
>      @CheckResult
> -    private ArrayList<Cursor> getSpecialFoldersCursorList(final boolean addDesktopFolder, final boolean addScreenshotsFolder) {
> +    private ArrayList<Cursor> getSpecialFoldersCursorList(final boolean addDesktopFolder,
> +            final boolean addScreenshotsFolder, final boolean addReadermodeFolder) {
>          if (addDesktopFolder || addScreenshotsFolder) {

Add `addReadermodeFolder` here too, right?

Comment 6

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/42281/#review40903

> Do we want to wrap this in a build flag (like screenshots)? Does that happen elsewhere?

We're wanting to release this to everyone, it's the direct replacement of our previous reading-list (i.e. not an experimental feature).

> With an aggregation function (e.g. `SUM`, `COUNT`), I don't think `GROUP BY` is what you want here. Would `DISTINCT` work for your use case?

DISTINCT doesn't work since it applies to all rows returned in a query (all of which are distinct since each bookmark is distinct), and not an individual column. I don't think there's any alternative to grouping, and this seemed to be what :rnewman was suggesting on IRC.

Comment 7

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f21ae3b98f6c

Comment 8

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8999a54226e

Comment 9

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9121c1254264

Comment 10

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/d5064e39467cc518c50603cb5603caad8e9510e0
Bug 1257345 - Implement reading list smartfolder r=mcomella

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d5064e39467c