Closed Bug 1257345 Opened 4 years ago Closed 4 years ago

Implement Readermode/Reading-list smart folder

Categories

(Firefox for Android :: Awesomescreen, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: ahunt, Assigned: ahunt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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).
Oops, wrong link in the comment 0, that should be Bug 1234315.
Assignee: nobody → ahunt
Depends on: migrate-RL, 1234315
No longer depends on: migrate-RL
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 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/
Attachment #8734492 - Attachment description: MozReview Request: Bug 1257345 - Implement reading list smartfolder → MozReview Request: Bug 1257345 - Implement reading list smartfolder r?mcomella
Attachment #8734492 - Flags: review?(michael.l.comella)
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?
Attachment #8734492 - Flags: review?(michael.l.comella) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/d5064e39467c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.