766942 - Reader Mode: Unable to add an article to the reading list from within Reader Mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Aaron Train [:aaronmt]]

Currently when one opens an article; there is no way to 'Add to Reading List'. I open the menu and 'Reading List' is greyed out.

Comment 1

[Lucas Rocha (:lucasr)]

(In reply to Aaron Train [:aaronmt] from comment #0)
> Currently when one opens an article; there is no way to 'Add to Reading
> List'. I open the menu and 'Reading List' is greyed out.

The Reading List menu item should only be enabled when you see the reader icon in the toolbar. If it's greyed out, this probably means the current page is not convertible to reader mode.

If you think the specific page where this happens should to be convertible but reader mode is not available, please file a separate bug for that. I'm sure we'll have to refine the way we do the readability check on pages.

Comment 2

[Aaron Train [:aaronmt]]

This bug is exactly in reference to articles viewed from within reading-mode (i.e, I am sitting in reader mode, and I can't find a way to 'Add to reading list'). Perhaps not all the patches landed, as I recall this working on your personal build a few days ago?

URL I am using: http://www.thestar.com/iphone

Comment 3

[Lucas Rocha (:lucasr)]

This bug has two parts:
1. The menu item for toggling the page on and off reading list is always disabled while in reader mode.
2. There's no way directly in the Reader UI to add/remove the page to/from the reading list. This part slightly overlaps with bug 750678 (reading list navigation in Reader) but it's more about how to add/remove reading list items in Reader Mode.

Madhava, Ian, any decision on the part 2 here?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Madhava, Ian: Ping!

Comment 5

[Ian Barlow (:ibarlow)]

Attachment: toolbar mockup

Attached is a revised cut of the Reader Mode toolbar, that now supports a couple of cases that should make interacting with the Reading List a little more comfortable.

1. Support for Adding / Removing items from your Reading List (as Lucas mentioned above)

* If an article is not in your Reading List, the toolbar should show a "Add to Reading List" icon. When tapping the icon, a toast that reads "Added to Reading List" should appear.
* If an article is in your Reading List, the toolbar should show a "Remove" icon. When tapping the icon, a toast that reads "Removed from Reading List" should appear.


2. A quick way to open your Reading List

Tapping the List icon should display your full Reading List. Currently this exists as a folder in the Bookmarks tab in the Awesomebar, so this is what it should open. We are currently exploring other possible placements for this list, so if it changes, this icon will have to be mapped to whichever new location we determine to be more appropriate.

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #5)
> Created attachment 647270 [details]
> toolbar mockup
> 
> Attached is a revised cut of the Reader Mode toolbar, that now supports a
> couple of cases that should make interacting with the Reading List a little
> more comfortable.
> 
> 1. Support for Adding / Removing items from your Reading List (as Lucas
> mentioned above)
> 
> * If an article is not in your Reading List, the toolbar should show a "Add
> to Reading List" icon. When tapping the icon, a toast that reads "Added to
> Reading List" should appear.
> * If an article is in your Reading List, the toolbar should show a "Remove"
> icon. When tapping the icon, a toast that reads "Removed from Reading List"
> should appear.
> 
> 
> 2. A quick way to open your Reading List
> 
> Tapping the List icon should display your full Reading List. Currently this
> exists as a folder in the Bookmarks tab in the Awesomebar, so this is what
> it should open. We are currently exploring other possible placements for
> this list, so if it changes, this icon will have to be mapped to whichever
> new location we determine to be more appropriate.

Are we going to remove the reading list item from the app menu? If not, are you ok with having different icons for the reading list toggling on reader toolbar and app menu?

How should those button align on bigger screens (i.e. tablets)? Align left (or right) I guess?

What is the background colour when the toolbar buttons are pressed? Orange? It's a shade of grey/blue right now.

Also, assets, please :-)

Comment 7

[Ian Barlow (:ibarlow)]

(In reply to Lucas Rocha (:lucasr) from comment #6)

> Are we going to remove the reading list item from the app menu? If not, are
> you ok with having different icons for the reading list toggling on reader
> toolbar and app menu?

It would be great if, before we ship "add to reading list", we resolve bug 769239 (manage down our menu list), which groups the Add to Reading List option into a bookmarking submenu. In that case, we probably wouldn't show icons in that submenu. 


> How should those button align on bigger screens (i.e. tablets)? Align left
> (or right) I guess?

Align right, please.


> What is the background colour when the toolbar buttons are pressed? Orange?
> It's a shade of grey/blue right now.

Orange



> Also, assets, please :-)

Coming soon :)

Comment 8

[Ian Barlow (:ibarlow)]

Attachment: Icon assets for toolbar

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Change bookmarks tab to support reading list as initial folder

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: Support showing reading list directly on awesomebar startup

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: Add Reader:GoToReadingList bits to GeckoApp

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: Set query argument with initial reading list state

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: Fix GeckoApp's showToast() to actually show the notification

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: Change Tabs to handle Reader:Removed message from Gecko

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Stop prograpation on clicks in toolbar buttons

Comment 16

[Lucas Rocha (:lucasr)]

Attachment: Implement new reader toolbar

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 648812 [details] [diff] [review]
Change Tabs to handle Reader:Removed message from Gecko

>+    void handleReaderRemoved(final String url) {
>+        GeckoAppShell.getHandler().post(new Runnable() {
>+            public void run() {
>+                BrowserDB.removeReadingListItemWithURL(mResolver, url);
>+                mActivity.showToast(R.string.reading_list_removed, Toast.LENGTH_SHORT);
>+            }
>+        });
>+    }

Looks good and we don't need to fire any TabChanged events for this (just like we don't for handleReaderAdded)

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 648815 [details] [diff] [review]
Implement new reader toolbar

More images :(

Oh well.

Comment 19

[:Margaret Leibovic]

Comment on attachment 648807 [details] [diff] [review]
Change bookmarks tab to support reading list as initial folder

This looks okay to me, but I'm not as familiar with this code after it got refactored into BookmarksTab, so maybe Wes should take a look as well, since I see his name all over that file.

Comment 20

[:Margaret Leibovic]

Comment on attachment 648807 [details] [diff] [review]
Change bookmarks tab to support reading list as initial folder

Actually, thinking about this more, does this mean that you won't be able to navigate outside of the reading list as long as mShowReadingList is true? I'm not confident about how all this code works, so I think Wes should take a look.

Does pressing the back button exit the AwesomeBar activity? Also, what happens if you switch to a different AwesomeBarTab and then back to Bookmarks?

Comment 21

[Wesley Johnston (:wesj)]

Comment on attachment 648807 [details] [diff] [review]
Change bookmarks tab to support reading list as initial folder

Review of attachment 648807 [details] [diff] [review]:
-----------------------------------------------------------------

Heh. I didn't write this either. Just copy-pasted it, but I'm gonna comment anyway. Tell me if I'm nuts.

::: mobile/android/base/awesomebar/BookmarksTab.java
@@ +38,5 @@
>      private int mFolderId;
>      private String mFolderTitle;
>      private BookmarksListAdapter mCursorAdapter = null;
>      private BookmarksQueryTask mQueryTask = null;
> +    private boolean mShowReadingList = false;

I don't really like this name, but then I don't really like this variable existing at all. We already have mInReadingList which seems kinda silly too. At the very least, I'd rather store some info than have a boolean for just this one folder. i.e.

BookmarksTab bookmarksTab = mAwesomeTabs.getBookmarksTab();
bookmarksTab.moveToChildFolder(Bookmarks.FIXED_READING_LIST_ID, ...)
  // the same code below can run except slightly more generic? or
  // refreshCurrentFolder can just bail if there's no mView. The getQueryTask()
  // call below can be made more generic and could automatically get a queryTask
  // for the current folder if nothing was passed to it and mQueryTask == null.
  // Not sure if that will work or not, but in my head it feels a little cleaner?
mAwesomeTabs.setCurrentTabByTag(bookmarksTab.getTag());

That may give issues if the folder was a few levels deep.... I'd rather put those off and come back to them later.

@@ +79,5 @@
>              list.setAdapter(null);
>              list.setAdapter(getCursorAdapter());
>  
> +            if (mShowReadingList) {
> +                String title = getResources().getString(R.string.bookmarks_folder_reading_list);

We should put a helper method in Bookmarks for this stuff. Get Name for Folder or something? I don't like having it in here...

Comment 22

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #21)
> Comment on attachment 648807 [details] [diff] [review]
> Change bookmarks tab to support reading list as initial folder
> 
> Review of attachment 648807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Heh. I didn't write this either. Just copy-pasted it, but I'm gonna comment
> anyway. Tell me if I'm nuts.
> 
> ::: mobile/android/base/awesomebar/BookmarksTab.java
> @@ +38,5 @@
> >      private int mFolderId;
> >      private String mFolderTitle;
> >      private BookmarksListAdapter mCursorAdapter = null;
> >      private BookmarksQueryTask mQueryTask = null;
> > +    private boolean mShowReadingList = false;
> 
> I don't really like this name, but then I don't really like this variable
> existing at all. We already have mInReadingList which seems kinda silly too.
> At the very least, I'd rather store some info than have a boolean for just
> this one folder. i.e.

You're right, the mInReadingList property is not strictly needed anymore. I'm posting a patch now to remove it.

> BookmarksTab bookmarksTab = mAwesomeTabs.getBookmarksTab();
> bookmarksTab.moveToChildFolder(Bookmarks.FIXED_READING_LIST_ID, ...)
>   // the same code below can run except slightly more generic? or
>   // refreshCurrentFolder can just bail if there's no mView. The
> getQueryTask()
>   // call below can be made more generic and could automatically get a
> queryTask
>   // for the current folder if nothing was passed to it and mQueryTask ==
> null.
>   // Not sure if that will work or not, but in my head it feels a little
> cleaner?
> mAwesomeTabs.setCurrentTabByTag(bookmarksTab.getTag());
> 
> That may give issues if the folder was a few levels deep.... I'd rather put
> those off and come back to them later.

I explicitly decided to not make this look too generic exactly because supporting awesomescreen startups for any sub-folder requires handling multiple number of navigation levels, which is a) a non-trivial change b) not needed for any of our real use cases. We can worry about this if/once we need it.

I'd rather not pretend we're doing the generic solution while we're not. And, as I said, we don't need to be generic here.

> @@ +79,5 @@
> >              list.setAdapter(null);
> >              list.setAdapter(getCursorAdapter());
> >  
> > +            if (mShowReadingList) {
> > +                String title = getResources().getString(R.string.bookmarks_folder_reading_list);
> 
> We should put a helper method in Bookmarks for this stuff. Get Name for
> Folder or something? I don't like having it in here...

Again, I feel like this would be generalizing too much for a use case that is very specific. We only need to handle reading list because it's a very special case.

Comment 23

[Lucas Rocha (:lucasr)]

Attachment: Get rid of mInReadingList property in BookmarksTab

Comment 24

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #20)
> Comment on attachment 648807 [details] [diff] [review]
> Change bookmarks tab to support reading list as initial folder
> 
> Actually, thinking about this more, does this mean that you won't be able to
> navigate outside of the reading list as long as mShowReadingList is true?
> I'm not confident about how all this code works, so I think Wes should take
> a look.

mShowReadingList only affects the initial folder to be shown on startup.
 
> Does pressing the back button exit the AwesomeBar activity? Also, what
> happens if you switch to a different AwesomeBarTab and then back to
> Bookmarks?

Folder navigation will work exactly as it usually works: pressing back will go up to the bookmarks root folder. Tab switching behavior will remain unchanged. The bookmarks tab is only created once and mShowReadingList only defines the initial folder in the tab.

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 648807 [details] [diff] [review]
Change bookmarks tab to support reading list as initial folder

Review of attachment 648807 [details] [diff] [review]:
-----------------------------------------------------------------

I get the point about the subfolder thing. I still think I'd be willing to overlook that in favour of a more general solution and a documentation note/todo. I don't think this code should know anything about ReadingList.

BUT, I don't want to hold this entire feature up waiting for it either. Can you file a bug about getting rid of it at least?

Comment 26

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #25)
> Comment on attachment 648807 [details] [diff] [review]
> Change bookmarks tab to support reading list as initial folder
> 
> Review of attachment 648807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I get the point about the subfolder thing. I still think I'd be willing to
> overlook that in favour of a more general solution and a documentation
> note/todo. I don't think this code should know anything about ReadingList.
> 
> BUT, I don't want to hold this entire feature up waiting for it either. Can
> you file a bug about getting rid of it at least?

Fair enough. I filed bug 780865 to track this.

Comment 27

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8700c238ad0
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bff363a148f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9a377baf074
https://hg.mozilla.org/integration/mozilla-inbound/rev/21aab12ef0ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/49dc584b5275
https://hg.mozilla.org/integration/mozilla-inbound/rev/22529b2d0e4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed360f992f9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d9cbf4373f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/49b3e00465ed

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/49b3e00465ed
https://hg.mozilla.org/mozilla-central/rev/0d9cbf4373f7
https://hg.mozilla.org/mozilla-central/rev/ed360f992f9c
https://hg.mozilla.org/mozilla-central/rev/22529b2d0e4c
https://hg.mozilla.org/mozilla-central/rev/49dc584b5275
https://hg.mozilla.org/mozilla-central/rev/21aab12ef0ba
https://hg.mozilla.org/mozilla-central/rev/c9a377baf074
https://hg.mozilla.org/mozilla-central/rev/9bff363a148f
https://hg.mozilla.org/mozilla-central/rev/a8700c238ad0

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 648807 [details] [diff] [review]
Change bookmarks tab to support reading list as initial folder

Reader mode final UI implementation. No strings added or changed. Reader mode is on the Firefox 16 roadmap.

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 648808 [details] [diff] [review]
Support showing reading list directly on awesomebar startup

Reader mode final UI implementation. No strings added or changed. Reader mode is on the Firefox 16 roadmap.

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 648809 [details] [diff] [review]
Add Reader:GoToReadingList bits to GeckoApp

Reader mode final UI implementation. No strings added or changed. Reader mode is on the Firefox 16 roadmap.

Comment 32

[Lucas Rocha (:lucasr)]

Comment on attachment 648810 [details] [diff] [review]
Set query argument with initial reading list state

Reader mode final UI implementation. No strings added or changed. Reader mode is on the Firefox 16 roadmap.

Comment 33

[Lucas Rocha (:lucasr)]

Comment on attachment 648811 [details] [diff] [review]
Fix GeckoApp's showToast() to actually show the notification

Reader mode final UI implementation. No strings added or changed. Reader mode is on the Firefox 16 roadmap.

Comment 34

[Lucas Rocha (:lucasr)]

Comment on attachment 648812 [details] [diff] [review]
Change Tabs to handle Reader:Removed message from Gecko

Reader mode final UI implementation. No strings added or changed. Reader mode is on the Firefox 16 roadmap.

Comment 35

[Lucas Rocha (:lucasr)]

Comment on attachment 648813 [details] [diff] [review]
Stop prograpation on clicks in toolbar buttons

Reader mode final UI implementation. No strings added or changed. Reader mode is on the Firefox 16 roadmap.

Comment 36

[Lucas Rocha (:lucasr)]

Comment on attachment 648815 [details] [diff] [review]
Implement new reader toolbar

Reader mode final UI implementation. No strings added or changed. Reader mode is on the Firefox 16 roadmap.

Comment 37

[Lucas Rocha (:lucasr)]

Comment on attachment 649229 [details] [diff] [review]
Get rid of mInReadingList property in BookmarksTab

Reader mode final UI implementation. No strings added or changed. Reader mode is on the Firefox 16 roadmap.

Comment 38

[Thomas Stache]

The 4 icons in the new toolbar arrangement are very blurry on my 800x480 ICS device. Does it use the right resources, are additional ones needed (followup bug)?

Comment 39

[Matt Brubeck (:mbrubeck)]

(In reply to Thomas Stache from comment #38)
> The 4 icons in the new toolbar arrangement are very blurry on my 800x480 ICS
> device.

See bug 776903.

Comment 40

[Cristian Nicolae (:xti)]

This issue is fixed on the latest Nightly. I will leave this bug open until the patches will land on m-a.

--
Firefox 17.a1 (2012-08-12)
Device: Galaxy Nexus
OS: Android 4.1.1

Comment 41

[Lucas Rocha (:lucasr)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/7ae2826a5873
https://hg.mozilla.org/releases/mozilla-aurora/rev/d760ef085a73
https://hg.mozilla.org/releases/mozilla-aurora/rev/aff96f868e6c
https://hg.mozilla.org/releases/mozilla-aurora/rev/be4405c0406c
https://hg.mozilla.org/releases/mozilla-aurora/rev/9ec746a4982f
https://hg.mozilla.org/releases/mozilla-aurora/rev/ed5f474d5f72
https://hg.mozilla.org/releases/mozilla-aurora/rev/cff53f283a80
https://hg.mozilla.org/releases/mozilla-aurora/rev/d25e95cee40d

Comment 42

[Adrian Tamas (:AdrianT)]

Verified fix on: Firefox Mobile 16 beta 2, Nightly 18.0a1 2012-09-04 and Aurora 17.0a2 2012-09-05 on Samsung Galaxy Tab 2 7.0 (Android 4.0.4)

Any article opened in Reading Mode can be added to the Reading List. Also if Reader Mode is available the article can be added to the reading list from the Custom Menu.