Closed
Bug 1267580
Opened 8 years ago
Closed 8 years ago
Empty reading list smart folder in bookmarks
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 fixed, firefox49 verified)
RESOLVED
FIXED
Firefox 49
People
(Reporter: sebastian, Assigned: ahunt)
References
Details
Attachments
(5 files)
51.40 KB,
image/png
|
Details | |
111.25 KB,
image/png
|
Details | |
58 bytes,
text/x-review-board-request
|
mcomella
:
review+
|
Details |
8.53 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
87.78 KB,
image/jpeg
|
Details |
After installing Firefox you have an empty reading list in the bookmarks panel - without any hint how to add something. We could hide the reading list smart folder but it would be nicer to have an empty state that explains what this is (or a default 'fake' article explaining the feature in there?)
Updated•8 years ago
|
Blocks: migrate-RL
Comment 1•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #0) > Created attachment 8745278 [details] > empty_reading_list.png > > After installing Firefox you have an empty reading list in the bookmarks > panel - without any hint how to add something. > > We could hide the reading list smart folder but it would be nicer to have an > empty state that explains what this is (or a default 'fake' article > explaining the feature in there?) I like the idea for a default "fake" article, which would be straightforward to implement as a new default bookmark, although we'd need some additional logic to also save an offline version of it. We could just ship the article JSON and store it directly in the cache when we create the default bookmark. How about using the SUMO article that we currently link to in the "Learn more" for the reading list migration panel?
Comment 2•8 years ago
|
||
(In reply to :Margaret Leibovic from comment #1) > (In reply to Sebastian Kaspari (:sebastian) from comment #0) > > We could hide the reading list smart folder but it would be nicer to have an > > empty state that explains what this is (or a default 'fake' article > > explaining the feature in there?) > > I like the idea for a default "fake" article, which would be straightforward > to implement as a new default bookmark, although we'd need some additional > logic to also save an offline version of it. We could just ship the article > JSON and store it directly in the cache when we create the default bookmark. Let's do this! we wanted to do this before IIRC. > How about using the SUMO article that we currently link to in the "Learn > more" for the reading list migration panel? https://bugzilla.mozilla.org/show_bug.cgi?id=1248477#c8 has the link from Joni.
Comment 3•8 years ago
|
||
Here's what it might look like
Comment 4•8 years ago
|
||
(In reply to :Margaret Leibovic from comment #1) > (In reply to Sebastian Kaspari (:sebastian) from comment #0) > > Created attachment 8745278 [details] > > empty_reading_list.png > > > > After installing Firefox you have an empty reading list in the bookmarks > > panel - without any hint how to add something. > > > > We could hide the reading list smart folder but it would be nicer to have an > > empty state that explains what this is (or a default 'fake' article > > explaining the feature in there?) > > I like the idea for a default "fake" article, which would be straightforward > to implement as a new default bookmark, although we'd need some additional > logic to also save an offline version of it. We could just ship the article > JSON and store it directly in the cache when we create the default bookmark. > > How about using the SUMO article that we currently link to in the "Learn > more" for the reading list migration panel? This is going to require string changes, and as ahunt pointed out to me, it's going to be hard to pre-cache the content in all locales. I think we should open a separate bug to investigate doing that, but in the meantime, we should figure out what we're doing for Fx48. Should we just hide this smart folder if there are no items in it?
Flags: needinfo?(alam)
Flags: needinfo?(ahunt)
Assignee | ||
Comment 5•8 years ago
|
||
I've tried putting together a proof-of-concept patch which downloads this article to the reading list. I doubt this is the best path to go down, but it's certainly doiable, and would avoid any string changes. Current issues 1: this will fail if we're not online. 2. We probably want to do this once for new, or newly migrated users (this patch tries to add the item on every startup). We might be able to fix these two together: try to download on startup until the download succeeds, never try again after that? 3: we can't use the usual URL since sumo redirects to the actual article (the app-internal URL is version/locale specific) and our reader view saving code gives up if there's a redirect while downloading. This could be fixed by following up to N redirects when directly downloading an article?
Flags: needinfo?(ahunt)
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50945/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50945/
Comment 7•8 years ago
|
||
(In reply to :Margaret Leibovic from comment #4) > I think we should open a separate bug to investigate doing that, but in the > meantime, we should figure out what we're doing for Fx48. Should we just > hide this smart folder if there are no items in it? Good point. For now, let's stick to the original plan: hide smart folder if there are no items.
Flags: needinfo?(alam)
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8749430 [details] MozReview Request: Bug 1267580 - Hide Reading List smartfolder if empty r?mcomella Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50945/diff/1-2/
Attachment #8749430 -
Attachment description: MozReview Request: Bug 1267580 - Proof of concept: download sumo article to reading list on app startup → MozReview Request: Bug 1267580 - Hide Reading List smartfolder if empty r?mcomella
Attachment #8749430 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to :Margaret Leibovic from comment #4) > (In reply to :Margaret Leibovic from comment #1) > > (In reply to Sebastian Kaspari (:sebastian) from comment #0) > > > Created attachment 8745278 [details] > > > empty_reading_list.png > > > > > > After installing Firefox you have an empty reading list in the bookmarks > > > panel - without any hint how to add something. > > > > > > We could hide the reading list smart folder but it would be nicer to have an > > > empty state that explains what this is (or a default 'fake' article > > > explaining the feature in there?) > > > > I like the idea for a default "fake" article, which would be straightforward > > to implement as a new default bookmark, although we'd need some additional > > logic to also save an offline version of it. We could just ship the article > > JSON and store it directly in the cache when we create the default bookmark. > > > > How about using the SUMO article that we currently link to in the "Learn > > more" for the reading list migration panel? > > This is going to require string changes, and as ahunt pointed out to me, > it's going to be hard to pre-cache the content in all locales. > > I think we should open a separate bug to investigate doing that, but in the > meantime, we should figure out what we're doing for Fx48. Should we just > hide this smart folder if there are no items in it? I've filed that as Bug 1273938. I already have a proof of concept patch, I'm guessing this isn't a huge priority right now though so I probably won't continue with that myself, but that patch should be useful for anyone else wanting to implement it.
Comment on attachment 8749430 [details] MozReview Request: Bug 1267580 - Hide Reading List smartfolder if empty r?mcomella https://reviewboard.mozilla.org/r/50945/#review50792
Attachment #8749430 -
Flags: review?(michael.l.comella) → review+
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e155723908e3c2aa129f12e7000ae416d91f5c70 Bug 1267580 - Hide Reading List smartfolder if empty r=mcomella
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e155723908e3
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 14•8 years ago
|
||
Uplifting this will require one part of Bug 1269001, I've created a combined patch which includes that commit along with this bug ( https://hg.mozilla.org/integration/fx-team/rev/95739305c4d0 ) Approval Request Comment [Feature/regressing bug #]: Bug 1257345 [User impact if declined]: Empty reading-list folder is shown in bookmarks without any explanation of how it can be used. [Describe test coverage new/current, TreeHerder]: manual testing. [Risks and why]: Low risk: we exclude an optional folder if its count is zero, this involves a simple count() DB query. [String/UUID change made/needed]: none.
Attachment #8756002 -
Flags: approval-mozilla-aurora?
Comment 15•8 years ago
|
||
On a clean install, Reader View folder is not displayed. After bookmarking a page in reader view, the folder will be displayed with 1 item. After deleting that item, "Reader View folder" will also be deleted. Tested using: Device: ONE A2001 (Android 5.1.1) Build: Firefox for Android 49.0a1(2016-05-24)
Updated•8 years ago
|
Updated•8 years ago
|
status-firefox48:
--- → affected
Comment 16•8 years ago
|
||
Comment on attachment 8756002 [details] [diff] [review] Hide RL folder aurora Bad UX, has been verified, taking it.
Attachment #8756002 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 17•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/69d24df550f7
Comment 18•8 years ago
|
||
On a clean install, Reader View folder is not displayed. After bookmarking a page in reader view, the folder will be displayed, but does not indicate the number of items inside. After deleting that item, "Reading List" folder is still displayed. Tapping the folder and exit after that, it won't be displayed anymore. Tested using: Device: ONE A2001 (Android 5.1.1) Build: Firefox for Android 48.0a2 (2016-05-29)
Updated•8 years ago
|
Flags: needinfo?(wkocher)
Redirecting the needinfo to ahunt since I only did the uplift and can't say what the state of things should be.
Flags: needinfo?(wkocher) → needinfo?(ahunt)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(ahunt)
Assignee | ||
Comment 20•8 years ago
|
||
(In reply to Teodora Vermesan (:TeoVermesan) from comment #18) > On a clean install, Reader View folder is not displayed. After bookmarking a > page in reader view, the folder will be displayed, but does not indicate the > number of items inside. After deleting that item, "Reading List" folder is > still displayed. Tapping the folder and exit after that, it won't be > displayed anymore. > > Tested using: > Device: ONE A2001 (Android 5.1.1) > Build: Firefox for Android 48.0a2 (2016-05-29) I'm not sure how much we need to worry about this: this seems to be an edge case, and is also a race condition (the behaviour is different on faster devices: the reading-list folder disappears immediately). I guess we'd need to figure out how common it is for users to delete their only reading-list item while having the root folder open (my guess is that either people don't use this feature at all, or they have a larger number of items in it).
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
•