Closed Bug 1234316 Opened 4 years ago Closed 4 years ago

Remove reading list panel (and inform users of the change)

Categories

(Firefox for Android Graveyard :: Reading List, defect)

35 Branch
defect
Not set

Tracking

(firefox48 verified)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- verified

People

(Reporter: Margaret, Assigned: ahunt)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 3 obsolete files)

If a user doesn't have any reading list items, let's just remove the panel from the default config (and potentially their saved customized config).

However, if they have saved reading list items, let's update the reading list panel to tell them about this move, then remove the panel after they hit some sort of "Got it" button.

NI to antlam for what this message should look like.
Flags: needinfo?(alam)
Do we want to require they hit a button to do this change? I think we could follow the same migration model we used for our Synced tabs panel here. Leave a message informing users of the change for 1 release, then execute/migrate things in the subsequent release.

We can still leave a "Got it" button to dismiss the prompt but if they never click it then, we still migrate everyone over in the next release. Thoughts?
Flags: needinfo?(alam)
Attached image prev_readinglist_migrate.png (obsolete) —
Attaching designs.

As with our RL efforts before, I'm reusing the empty state specs (in bug 1091826) except the icon is now 60dp rather than 80dp.

The "learn more" link would be to a SUMO article of sorts if we have one but if not, we can just exclude it from the final version.
notes from meeting:

when button is clicked -> hide RL panel (switch to bookmarks panel)
Attached image prev_readinglist_migrate.png (obsolete) —
Reading this over again, I don't think we need a "got it" button to perform the change. What happens if they never press it? We could be getting into a lot of edge cases down the line.

Instead, the button could lead the user to where things have moved, Bookmarks.

Here's what I'm thinking... 

In the same release,
1) Migrate content into a Bookmarks folder (bug 1234315)
2) Display this message in the RL panel

In the release after,
1) Remove RL panel (with message) completely

That said, I don't see a clear 100% winner here so I'm happy to get other perspectives. 

NI Barbara for product input!
Attachment #8709220 - Attachment is obsolete: true
Flags: needinfo?(bbermes)
Data could help us here!
Note: Copy TBD, talking to Matej
Thinking about this again, I vote for same release, keeps it compact in one release.

If we are unsure, maybe we should figure out a way to get % of users who actually use the RL (have more than 1 RL item)? In my mind, this http://mzl.la/1nSK6Y8 only shows the distribution of # of items but doesn't give us any idea of how many of our users are actually using it.
Flags: needinfo?(bbermes)
Attached image prev_readinglist_migrate.png (obsolete) —
Copy finalized.
Attachment #8716403 - Attachment is obsolete: true
Updated the image here to be more informative than just a star. Nothing else has changed.
Attachment #8718391 - Attachment is obsolete: true
Attached file img_migration.zip
Assets! These are the same height as the star in the original design, just wider. So the spec should still work.
Assignee: nobody → ahunt
We'll kill this panel in a few weeks, hence we don't really care about
duplication here.

Review commit: https://reviewboard.mozilla.org/r/37929/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37929/
Attachment #8726336 - Flags: review?(liuche)
Attachment #8726336 - Flags: review?(liuche)
Comment on attachment 8726336 [details]
MozReview Request: Bug 1234316 - Show migration message in Reading List panel r?liuche

https://reviewboard.mozilla.org/r/37929/#review34501

Good start - I'd like to see more thorough removal of the code that used to be used from this file that is no longer relevant.

I think there are a few more places that we should remove code from.
Off the top of my head:
- Mark read/unread (context menu item, as well as the strings) unless we're handling that in the Bookmarks smartfolder
- Any other files or classes (is ReadingListRow being used in the Bookmarks panel, or is it unused now?)
- Telemetry that is no longer relevant
And probably other things as well (xml layouts, strings, colors, images/resources, classes).

Also, what happens when someone adds a reading list item from the menu, does it go into bookmarks? (Is that functionality being removed in a different bug, or should we remove it all at once? If that's scope creep and we'll be doing that in another bug, that's fine, but we should explicitly be okay with whatever limbo state we have in the meantime.)

::: mobile/android/base/java/org/mozilla/gecko/home/ReadingListPanel.java:29
(Diff revision 1)
> -        mList.setTag(HomePager.LIST_TAG_READING_LIST);
> +        // release, hence there's little utility in optimising this code.

We don't need this constant anymore.

::: mobile/android/base/java/org/mozilla/gecko/home/ReadingListPanel.java:33
(Diff revision 1)
> -            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +                mUrlOpenListener.onUrlOpen("about:home?panel=" + HomeConfig.getIdForBuiltinPanelType(HomeConfig.PanelType.BOOKMARKS),

What happens if the user has a disabled bookmarks panel?

::: mobile/android/base/java/org/mozilla/gecko/home/ReadingListPanel.java
(Diff revision 1)
> -            emptyImage.setImageResource(R.drawable.icon_reading_list_empty);

These string and image resources aren't used anymore, so let's remove them.

::: mobile/android/base/java/org/mozilla/gecko/home/ReadingListPanel.java
(Diff revision 1)
> -            return LayoutInflater.from(parent.getContext()).inflate(R.layout.reading_list_item_row, parent, false);

Ditto.
(In reply to Chenxia Liu [:liuche] from comment #12)
> Comment on attachment 8726336 [details]
> MozReview Request: Bug 1234316 - Show migration message in Reading List
> panel r?liuche
> 
> https://reviewboard.mozilla.org/r/37929/#review34501
> 
> Good start - I'd like to see more thorough removal of the code that used to
> be used from this file that is no longer relevant.
> 
> I think there are a few more places that we should remove code from.
> Off the top of my head:
> - Mark read/unread (context menu item, as well as the strings) unless we're
> handling that in the Bookmarks smartfolder
> - Any other files or classes (is ReadingListRow being used in the Bookmarks
> panel, or is it unused now?)
> - Telemetry that is no longer relevant
> And probably other things as well (xml layouts, strings, colors,
> images/resources, classes).
> 
> Also, what happens when someone adds a reading list item from the menu, does
> it go into bookmarks? (Is that functionality being removed in a different
> bug, or should we remove it all at once? If that's scope creep and we'll be
> doing that in another bug, that's fine, but we should explicitly be okay
> with whatever limbo state we have in the meantime.)
> 

I'll add removal of the ReadingListRow / styles / etc in a followup commit (as part of this bug) - that will be pushed to review soon. Similarly for telemetry, probably a separate commit there too. We don't have any plan to support mark read/unread in bookmarks so I'll remove that for now too (we can always add that back later if we decide to implement that?).

I'm removing various other reading list menu items in other bugs, mainly Bug 1234319, I think the plan is that we only allow bookmarking, and if we happen to bookmark an already open readerview page it's also cached in readermode. I'm also planning to land most of the readinglist migration bugs simultaneously which should avoid a limbo state.
(In reply to Chenxia Liu [:liuche] from comment #12)
> (Diff revision 1)
> > -            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> > +                mUrlOpenListener.onUrlOpen("about:home?panel=" + HomeConfig.getIdForBuiltinPanelType(HomeConfig.PanelType.BOOKMARKS),
> 
> What happens if the user has a disabled bookmarks panel?
> 

I'm not sure what we'd actually want to do, some of the options might be:

1) Reenable the bookmarks panel when the user presses the "open bookmarks" button?
1A) Prompt the user to ask them whether they want to reenable the bookmarks panel? (This isn't ideal: more strings required, more complicated to implement.)
2) Disable the button when the bookmarks panel is disabled.
2A) Disable the button and show a string indicating that you need to reenable the bookmarks panel?

:antlam, any thoughts? I think (1) is probably simplest and most obvious, I feel like it's very much an edge case that someone would even be in this state?
Flags: needinfo?(alam)
Attachment #8726336 - Flags: review?(liuche)
Comment on attachment 8726336 [details]
MozReview Request: Bug 1234316 - Show migration message in Reading List panel r?liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37929/diff/1-2/
Let's use a snack bar for this type of feedback. I wouldn't even worry too much about teach users to enable it since they disabled it in the first place. 

A simple snackbar will do:

"Your Bookmarks panel has been hidden."
Flags: needinfo?(alam)
I agree with the snackbark, but probably a different string? "has been hidden" seems to imply to me that the action taken (clicking) caused this state (bookmarks panel has been hidden), so something more like, "Your Bookmarks panel is not enabled" or "Your Bookmarks panel is hidden", or "Enable your Bookmarks panel to access Reading List"
Flags: needinfo?(alam)
(In reply to Chenxia Liu [:liuche] from comment #20)
> I agree with the snackbark, but probably a different string? "has been
> hidden" seems to imply to me that the action taken (clicking) caused this
> state (bookmarks panel has been hidden), so something more like, "Your
> Bookmarks panel is not enabled" or "Your Bookmarks panel is hidden", or
> "Enable your Bookmarks panel to access Reading List"

I was going to go with "is disabled." But "Hide" is the verb we use when a user goes to do this in our Settings so we should keep consistent with that.
Flags: needinfo?(alam)
Comment on attachment 8726336 [details]
MozReview Request: Bug 1234316 - Show migration message in Reading List panel r?liuche

https://reviewboard.mozilla.org/r/37929/#review37145

::: mobile/android/base/java/org/mozilla/gecko/home/ReadingListPanel.java:42
(Diff revision 2)
>  
> -        mList.setContextMenuInfoFactory(new HomeContextMenuInfo.Factory() {
> +        root.findViewById(R.id.welcome_browse).setOnClickListener(new View.OnClickListener() {
>              @Override
> -            public HomeContextMenuInfo makeInfoForCursor(View view, int position, long id, Cursor cursor) {
> -                final HomeContextMenuInfo info = new HomeContextMenuInfo(view, position, id);
> -                info.url = cursor.getString(cursor.getColumnIndexOrThrow(ReadingListItems.URL));
> +            public void onClick(View v) {
> +                // TODO: this should be updated to the real URL in Bug 1248477
> +                mUrlOpenListener.onUrlOpen("https://support.mozilla.org/",

It looks like the SUMO link is finished, so you should try it out.
Attachment #8726336 - Flags: review?(liuche) → review+
Comment on attachment 8728035 [details]
MozReview Request: Bug 1234316 - Post: remove reading list read/unread items from homepanel context menu r=liuche

https://reviewboard.mozilla.org/r/38757/#review37155

::: mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java
(Diff revision 1)
>          if (!Restrictions.isAllowed(context, Restrictable.PRIVATE_BROWSING)) {
>              menu.findItem(R.id.home_open_private_tab).setVisible(false);
>          }
> -
> -        // We only show these menu items on the reading list panel:
> -        menu.findItem(R.id.mark_read).setVisible(false);

Are we stopping people from removing things from the reading list? It wasn't clear to me from the bug. (Relatedly, if we're still allowing adding reading list items from the 3-dot menu, that might be a problem.) What is the mechanism for removing a reading list item from the "smart folder" in Bookmarks?
Attachment #8728035 - Flags: review?(liuche)
Comment on attachment 8728035 [details]
MozReview Request: Bug 1234316 - Post: remove reading list read/unread items from homepanel context menu r=liuche

https://reviewboard.mozilla.org/r/38757/#review37159
Attachment #8728035 - Flags: review+
Comment on attachment 8728036 [details]
MozReview Request: Bug 1234316 - Post: remove unused ReadingListRow and related styles r=liuche

https://reviewboard.mozilla.org/r/38759/#review37161
Attachment #8728036 - Flags: review?(liuche) → review+
Attachment #8728037 - Flags: review?(liuche)
Comment on attachment 8728037 [details]
MozReview Request: Bug 1234316 - Post: cleanup unneeded reading list resources/strings r?liuche

https://reviewboard.mozilla.org/r/38761/#review37157

r- here - this will break with strings, and I think there are still more parts that may need to be looked at/removed. I don't see anything from HomeConfig/HomeConfigPrefsBackend (where the reading list is set as part of the default home panels). A gentle nudge to be more thorough about making sure to find all the parts of removing the reading list panel before sending this last part (or potential future parts) back for review.

::: mobile/android/base/strings.xml.in
(Diff revision 1)
>  
>    <string name="site_settings_title">&site_settings_title3;</string>
>    <string name="site_settings_cancel">&site_settings_cancel;</string>
>    <string name="site_settings_clear">&site_settings_clear;</string>
>  
> -  <string name="reading_list_added">&reading_list_added3;</string>

I think some of these strings are actually still used - see BrowserApp.showSwitchToReadingListSnackbar for reading_list_added, which I didn't see any changes to.
Barbara, just to clarify, in this new no-reading-list-world, we're still allowing adding reading list items (3-dot menu book icon, long-pressing book icon in the urlbar, etc, not from the share overlay) only they're going to the smartfolder in bookmarks, right? And you can remove items from the smart-folder readinglist via context menu.

And relevant to this bug, what are the dependencies for this bug (as well as the other bugs) - does this need to land after bug 1257345 and bug 1234315 (which add the reading list smart folder and actually migrate the reading list)? If so, can you update the dependency hierarchy for the metabug migrate-RL hierarchy? I'm sorry I'm confused and am not up to speed on what's going on with the stages of removing the reading list (and the new code that we need to add in), there seem to be a lot of pieces to remove and I'm not completely clear about the dependencies.
Flags: needinfo?(bbermes)
Comment on attachment 8726336 [details]
MozReview Request: Bug 1234316 - Show migration message in Reading List panel r?liuche

https://reviewboard.mozilla.org/r/37929/#review37167

::: mobile/android/base/java/org/mozilla/gecko/home/ReadingListPanel.java:33
(Diff revision 2)
> -        mList = (HomeListView) view.findViewById(R.id.list);
> -        mList.setTag(HomePager.LIST_TAG_READING_LIST);
> -
> -        mList.setOnItemClickListener(new AdapterView.OnItemClickListener() {
> -            @Override
> -            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +        // We could update the ID names - however this panel is only intended to be live for one
> +        // release, hence there's little utility in optimising this code.
> +        root.findViewById(R.id.welcome_account).setOnClickListener(new View.OnClickListener() {
> +            @Override
> +            public void onClick(View v) {
> +                mUrlOpenListener.onUrlOpen("about:home?panel=" + HomeConfig.getIdForBuiltinPanelType(HomeConfig.PanelType.BOOKMARKS),

Also - reminder to add the snackbar antlam mentioned in comment 19.

https://bugzilla.mozilla.org/show_bug.cgi?id=1234316#c19
Attachment #8726336 - Flags: review+
(In reply to Chenxia Liu [:liuche] from comment #27)
> Barbara, just to clarify, in this new no-reading-list-world, we're still
> allowing adding reading list items (3-dot menu book icon, long-pressing book
> icon in the urlbar, etc, not from the share overlay) only they're going to
> the smartfolder in bookmarks, right? And you can remove items from the
> smart-folder readinglist via context menu.
> 
> And relevant to this bug, what are the dependencies for this bug (as well as
> the other bugs) - does this need to land after bug 1257345 and bug 1234315
> (which add the reading list smart folder and actually migrate the reading
> list)? If so, can you update the dependency hierarchy for the metabug
> migrate-RL hierarchy? I'm sorry I'm confused and am not up to speed on
> what's going on with the stages of removing the reading list (and the new
> code that we need to add in), there seem to be a lot of pieces to remove and
> I'm not completely clear about the dependencies.

I should probably look into sorting out these dependencies - I have a long series of commits (with a mix of bug numbers) locally, and most of these issues are solved, but in potentially different bugs - which is confusing to me too.

But as a quick summary:
- All reading list related functionality is being removed (various context menu items, and the main menu add-to-reading-lsit).
- We only have bookmark/unbookmark for about:reader pages - doing this will add/remove a page from the readercache. (Similarly: entering/exiting readermode while viewing a bookmark will toggle that state.)
- The smart folder isn't a priority for now, we're probably going to try to land the migration first (including these UI changes that kill all RL functionality), and then add the smartfolder as a second stage.

I'm trying to push all my local commits to a user-repo which should make it more obvious what's happening. I've realised that I've removed some strings in commits in this bug, that are used by snackbars that I removed in a separate bug, I'll need to sort that out too.
Andrzej is correct in regards to the "quick summary" he gave except for the smart folder comment.

In my mind, adding the smart folder and migrating is one step and should be landed together for the user to benefit from it.

In addition, those should be dependencies as well.

+ Migration notice for existing users: https://bugzilla.mozilla.org/show_bug.cgi?id=1234316
+ Helper UI to inform the user they can save Reader View items offline: https://bugzilla.mozilla.org/show_bug.cgi?id=1246238


NI Anthony so he is in the loop as well.
Flags: needinfo?(bbermes) → needinfo?(alam)
https://reviewboard.mozilla.org/r/37929/#review37167

> Also - reminder to add the snackbar antlam mentioned in comment 19.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1234316#c19

I've filed this as https://bugzilla.mozilla.org/show_bug.cgi?id=1257636 - I'm trying to minimise the number of patches that we land when doing the actual migration, and therefore splitting out new features as much as possible. (The new panel has to land with the migration since the old panel would crash when accessing inexistant DB tables, but the snackbar can be added anytime before the next release. That said, I'll probably have all the separated bugs complete before the migration lands, and should be able to land them a day or two later.)
https://reviewboard.mozilla.org/r/38757/#review37155

> Are we stopping people from removing things from the reading list? It wasn't clear to me from the bug. (Relatedly, if we're still allowing adding reading list items from the 3-dot menu, that might be a problem.) What is the mechanism for removing a reading list item from the "smart folder" in Bookmarks?

I might have already mentioned this elsewhere, but _all_ reading list functionality is going, and we use normal bookmark management to add/remove any readermode bookmarks. The "smart folder" will still contain valid bookmarks that can be removed as needed. I've tried putting a better summary in:
https://docs.google.com/document/d/1kSo4wlIG41KSMMKxdfJZh-NFl3nusSSbKxesnaSVffI/edit#
https://reviewboard.mozilla.org/r/38761/#review37157

I think the plan is to keep the reading list panel (in this new migration message state) for at least one release cycle. Meaning that we can leave all the HomeConfig stuff for now, and remove it during the next release.

We could probably disable the panel for those users who don't have any items in their reading list, however that seems quite expensive given that those users are unlikely to even be opening the reading list panel.

> I think some of these strings are actually still used - see BrowserApp.showSwitchToReadingListSnackbar for reading_list_added, which I didn't see any changes to.

Sorry, this should've been part of Bug 1234319 where those snackbars are being removed.
(In reply to Barbara Bermes [:barbara] from comment #30)
> Andrzej is correct in regards to the "quick summary" he gave except for the
> smart folder comment.
> 
> In my mind, adding the smart folder and migrating is one step and should be
> landed together for the user to benefit from it.
> 
> In addition, those should be dependencies as well.
> 
> + Migration notice for existing users:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1234316
> + Helper UI to inform the user they can save Reader View items offline:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1246238
> 
> 
> NI Anthony so he is in the loop as well.

Barbara is right.org
Flags: needinfo?(alam)
(In reply to Chenxia Liu [:liuche] from comment #26)
> Comment on attachment 8728037 [details]
> MozReview Request: Bug 1234316 - Post: cleanup unneeded reading list
> resources/strings r?liuche
> 
> https://reviewboard.mozilla.org/r/38761/#review37157
> 
> r- here - this will break with strings, and I think there are still more
> parts that may need to be looked at/removed. I don't see anything from
> HomeConfig/HomeConfigPrefsBackend (where the reading list is set as part of
> the default home panels). A gentle nudge to be more thorough about making
> sure to find all the parts of removing the reading list panel before sending
> this last part (or potential future parts) back for review.

For the record: I filed Bug 1259078 to look at whether we want to hide the reading-list panel for new users / migrated users without reading-list items. For now we'll just show this panel with the migration to everyone, and we'll then kill it completely in the (probably) next release.
Comment on attachment 8726336 [details]
MozReview Request: Bug 1234316 - Show migration message in Reading List panel r?liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37929/diff/2-3/
Attachment #8726336 - Flags: review?(liuche)
Comment on attachment 8728035 [details]
MozReview Request: Bug 1234316 - Post: remove reading list read/unread items from homepanel context menu r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38757/diff/1-2/
Attachment #8728035 - Attachment description: MozReview Request: Bug 1234316 - Post: remove reading list read/unread items from homepanel context menu r?liuche → MozReview Request: Bug 1234316 - Post: remove reading list read/unread items from homepanel context menu r=liuche
Comment on attachment 8728036 [details]
MozReview Request: Bug 1234316 - Post: remove unused ReadingListRow and related styles r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38759/diff/1-2/
Attachment #8728036 - Attachment description: MozReview Request: Bug 1234316 - Post: remove unused ReadingListRow and related styles r?liuche → MozReview Request: Bug 1234316 - Post: remove unused ReadingListRow and related styles r=liuche
Comment on attachment 8728037 [details]
MozReview Request: Bug 1234316 - Post: cleanup unneeded reading list resources/strings r?liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38761/diff/1-2/
Attachment #8728037 - Flags: review?(liuche)
Comment on attachment 8726336 [details]
MozReview Request: Bug 1234316 - Show migration message in Reading List panel r?liuche

https://reviewboard.mozilla.org/r/37929/#review39365
Attachment #8726336 - Flags: review?(liuche) → review+
Comment on attachment 8728037 [details]
MozReview Request: Bug 1234316 - Post: cleanup unneeded reading list resources/strings r?liuche

https://reviewboard.mozilla.org/r/38761/#review39367

Okay, this looks good to me. And since this seems to be an especially hairy set of patch dependencies, it's better to land everything and start getting people testing on Nightly (and I don't see any migration stuff that will break anything either).
Attachment #8728037 - Flags: review?(liuche) → review+
https://hg.mozilla.org/integration/fx-team/rev/42f0de34a198132e2035d4548c23de6f3e647e67
Bug 1234316 - Show migration message in Reading List panel r=liuche

https://hg.mozilla.org/integration/fx-team/rev/a6a051cd01f1851526fcc1ea53c3f0a6028df31a
Bug 1234316 - Post: remove reading list read/unread items from homepanel context menu r=liuche

https://hg.mozilla.org/integration/fx-team/rev/6cff5892adc4905b27d75c3a07eeac1e1d209647
Bug 1234316 - Post: remove unused ReadingListRow and related styles r=liuche

https://hg.mozilla.org/integration/fx-team/rev/b5334b56c79dbff96761c13559d6fe5a7ef64dc6
Bug 1234316 - Post: cleanup unneeded reading list resources/strings r=liuche
Verified as fixed using:
Device: Nexus 6 (Android 6.0) and Nexus 7 (Android 5.1)
Build: Firefox for Android 48.0a1 (2016-04-10)
The background here is grey, but it should be white, like the other panels.
Flags: needinfo?(ahunt)
(In reply to Anthony Lam (:antlam) from comment #45)
> The background here is grey, but it should be white, like the other panels.

Fixed in Bug 1265087 !
Flags: needinfo?(ahunt)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.