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)

All
Android
defect
Not set
normal

Tracking

(firefox48 fixed, firefox49 verified)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- fixed
firefox49 --- verified

People

(Reporter: sebastian, Assigned: ahunt)

References

Details

Attachments

(5 files)

Attached image 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?)
(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?
(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.
(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)
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)
(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)
(Assigning to myself)
Assignee: nobody → ahunt
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)
(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+
https://hg.mozilla.org/mozilla-central/rev/e155723908e3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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?
Attached image 0qDJnEMZ.jpg
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)
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+
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)
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)
Flags: needinfo?(ahunt)
(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).
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.