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.

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?

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)

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!

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.

Copy finalized.

Updated the image here to be more informative than just a star. Nothing else has changed.

Assets! These are the same height as the star in the original design, just wider. So the spec should still work.

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/

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?

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/

Review commit: https://reviewboard.mozilla.org/r/38757/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38757/

Review commit: https://reviewboard.mozilla.org/r/38759/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38759/

Review commit: https://reviewboard.mozilla.org/r/38761/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38761/

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."

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"

(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.

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.

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?

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

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

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.

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

(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.

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

(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/

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/

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/

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/

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

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).

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

https://hg.mozilla.org/mozilla-central/rev/42f0de34a198 https://hg.mozilla.org/mozilla-central/rev/a6a051cd01f1 https://hg.mozilla.org/mozilla-central/rev/6cff5892adc4 https://hg.mozilla.org/mozilla-central/rev/b5334b56c79d

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.

(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 !