Closed
Bug 1257345
Opened 8 years ago
Closed 8 years ago
Implement Readermode/Reading-list smart folder
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Firefox for Android Graveyard
Awesomescreen
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: ahunt, Assigned: ahunt)
References
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).
Assignee | ||
Comment 1•8 years ago
|
||
Oops, wrong link in the comment 0, that should be Bug 1234315.
Assignee: nobody → ahunt
Depends on: migrate-RL, 1234315
Assignee | ||
Updated•8 years ago
|
No longer depends on: migrate-RL
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42281/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42281/
Assignee | ||
Comment 3•8 years ago
|
||
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/
Assignee | ||
Comment 4•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f21ae3b98f6c
Assignee | ||
Comment 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8999a54226e
Assignee | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9121c1254264
Assignee | ||
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d5064e39467cc518c50603cb5603caad8e9510e0 Bug 1257345 - Implement reading list smartfolder r=mcomella
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d5064e39467c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•