750707 - Reader Mode: Implement "Add to Reading List" action

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Initial idea is to add a browser menu item for that. When pages are added to reading list, they should available for offline usage later (see bug 750702). This bug is about implementing the basic operation of adding a page to the reading list.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Factor out method to add a bookmark, generalize folder GUID argument

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Add method to add reading list items in BrowserDB

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Toggling bookmarks off should only apply to normal bookmarks

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Check for bookmarks should only consider normal bookmarks

Again, I'll probably squash all those db patches in one single change. I only split them to make them easier to review.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Pack URL bar buttons in same layout to allow showing multiple icons

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Add reader button to the browser toolbar

Ok, here is where the fun actually starts :-)

This patch adds the reader button in the browser toolbar. As you can see, I added all the plumbing for dynamically showing/hiding it but the actual code to check if the current page is "reader-friendly" is not there yet. It should be just about sending a "ReaderEnabled" message to Java. So, for now, the reader button will never show up. I'll implement this in bug 750683. This patch also adds a dummy "Read now" button that does nothing (it will be implemented in bug 750698).

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Reader button in browser toolbar

Here's how the reader icon looks in the browser toolbar

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Doorhanger for reader operations

I've proposed (on IRC) to concentrate all reader operations in a doorhanger activated from the reader button in the browser toolbar. This seems like a better approach than splitting it in different places. ibarlow and patryc seem to agree.

This is how the doorhanger looks right now. Patryk and Madhava, this needs proper design and assets.

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Reader and identity icons don't feel consistent

Just one observation: I think the icons for reader and identity are going well together (see attached image). Maybe the reader icon should be a bit more subtle (lighter grey or something?)

Comment 10

[Lucas Rocha (:lucasr)]

Forgot to mention: need feedback from UX team on the toast notification text for when you add/remove items to/from the Reading List. For now, I'm using:

"Page added to your Reading List"
"Page removed from your Reading List"

Not entirely happy about it, suggestions for simpler messages are welcome.

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: Factor out method to add a bookmark, generalize folder GUID argument

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: Add method to add reading list items in BrowserDB

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: Toggling bookmarks off should only apply to normal bookmarks

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: Check for bookmarks should only consider normal bookmarks

Comment 15

[Ian Barlow (:ibarlow)]

Attachment: mockup of reader mode menu

Here are some updated icons for the urlbar, to make the lock, stop/go/search, and reader mode icons feel more in line. Please replace all of these at the same time. http://cl.ly/3O42022J1H0N2E1Y3K11

Also, attached is a menu design we can pursue. I've added Sriram to this bug, as this design follows the same look as the custom overflow menu he has been implementing, so I imagine there is some code that can be shared here :)

Comment 16

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #5)
> Created attachment 627284 [details] [diff] [review]
> Pack URL bar buttons in same layout to allow showing multiple icons

You might need to change the layout in layout-land-v14/ also.
With the tablet UI code pushed in, there are newer browser_toolbar.xml in layout-xlarge/ and layout-sw600dp/. Please make similar changes there too.

Comment 17

[:Margaret Leibovic]

Comment on attachment 627809 [details] [diff] [review]
Factor out method to add a bookmark, generalize folder GUID argument

Looks good to me.

>diff --git a/mobile/android/base/db/LocalBrowserDB.java b/mobile/android/base/db/LocalBrowserDB.java
>@@ -441,52 +441,54 @@ public class LocalBrowserDB implements B

>         Uri contentUri = mBookmarksUriWithProfile;

Not part of your patch, but this local variable seems pretty useless. I feel like we could get rid of this and just be using mBookmarksUriWithProfile directly.

Comment 18

[:Margaret Leibovic]

Comment on attachment 627811 [details] [diff] [review]
Add method to add reading list items in BrowserDB

We really need to finish bug 723623.

Comment 19

[:Margaret Leibovic]

Comment on attachment 627813 [details] [diff] [review]
Check for bookmarks should only consider normal bookmarks

Unless one of your patches in bug 750684 handles this case, I think we're also going to need another patch to make sure we don't show the bookmark star on items in the awesome screen, since AwesomeBarTabs's updateBookmarkStar method doesn't go through this method.

So many patches! Hard to keep track :)

Comment 20

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

Comment on attachment 627284 [details] [diff] [review]
Pack URL bar buttons in same layout to allow showing multiple icons

Note Sriram's comment about the additional browser_toolbar.xml files that now exist to support tablets.

Comment 21

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

Comment on attachment 627286 [details] [diff] [review]
Add reader button to the browser toolbar

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java
>     public ImageButton mSiteSecurity;
>+    public ImageButton mReader;

>+    private boolean mShowSiteSecurity;
>+    private boolean mShowReader;

Just an observation:
Along with my desire to create a PageActions API, which would allow add-ons to add images to the URLbar like you added the "reader", I see a code-scaling problem. We are creating a specific imageview and boolean for each actions. We'll need to get to a generic system when we add the next action, or the pageaction API itself.

>+        private ReaderPopup(Context context) {

>+            Button readingListButton = (Button) layout.findViewById(R.id.reading_list);
>+            readingListButton.setOnClickListener(new Button.OnClickListener() {

What happens if the page is already on the reading list? Should the popup be ignored and "read now" be the default action if I tap the "reader" action?

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>         tab.setContentType(contentType);
>         tab.updateFavicon(null);
>         tab.updateFaviconURL(null);
>         tab.updateIdentityData(null);
>+        tab.setReaderEnabled(false);
>         tab.removeTransientDoorHangers();
>         tab.setAllowZoom(true);
>         tab.setDefaultZoom(0);
>         tab.setMinZoom(0);
>         tab.setMaxZoom(0);
>         tab.setHasTouchListeners(false);
>         tab.setCheckerboardColor(Color.WHITE);

Well this block of code is screaming out for a helper method to be added to Tab. File a followup bug?

>                     mBrowserToolbar.setTitle(uri);
>                     mBrowserToolbar.setFavicon(null);
>                     mBrowserToolbar.setSecurityMode(tab.getSecurityMode());
>+                    mBrowserToolbar.setReaderVisibility(tab.getReaderEnabled());
>                     mDoorHangerPopup.updatePopup();
>                     mBrowserToolbar.setShadowVisibility(!(tab.getURL().startsWith("about:")));

BrowserToolbar.refresh() should be capable of doing this too. There was a patch somewhere to remove all of these patterns and just call mBrowserToolbar.refresh()

>     void handleDocumentStart(int tabId, final boolean showProgress, String uri) {

>         tab.setState("about:home".equals(uri) ? Tab.STATE_SUCCESS : Tab.STATE_LOADING);
>         tab.updateIdentityData(null);
>+        tab.setReaderEnabled(false);

Does this handle back/forward session updating too? I think it should, but just asking.

>diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java
>+    public void addToReadingList() {

>+                BrowserDB.addReadingListItem(mContentResolver, getTitle(), url);

I assume this code handles duplicates (i.e. we don't want dupes in the list)

>diff --git a/mobile/android/base/resources/layout/reader_popup.xml b/mobile/android/base/resources/layout/reader_popup.xml
>+</LinearLayout>
>\ No newline at end of file

Add a newline

Comment 22

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #19)
> Comment on attachment 627813 [details] [diff] [review]
> Check for bookmarks should only consider normal bookmarks
> 
> Unless one of your patches in bug 750684 handles this case, I think we're
> also going to need another patch to make sure we don't show the bookmark
> star on items in the awesome screen, since AwesomeBarTabs's
> updateBookmarkStar method doesn't go through this method.

I'm handling icons in awesome bar items in bug 757776 (yeah, more patches!).

Comment 23

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #17)
> Comment on attachment 627809 [details] [diff] [review]
> Factor out method to add a bookmark, generalize folder GUID argument
> 
> Looks good to me.
> 
> >diff --git a/mobile/android/base/db/LocalBrowserDB.java b/mobile/android/base/db/LocalBrowserDB.java
> >@@ -441,52 +441,54 @@ public class LocalBrowserDB implements B
> 
> >         Uri contentUri = mBookmarksUriWithProfile;
> 
> Not part of your patch, but this local variable seems pretty useless. I feel
> like we could get rid of this and just be using mBookmarksUriWithProfile
> directly.

Will submit a patch now.

Comment 24

[Lucas Rocha (:lucasr)]

Attachment: Remove unnecessary local variable in addBookmarkItem()

Comment 25

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #20)
> Comment on attachment 627284 [details] [diff] [review]
> Pack URL bar buttons in same layout to allow showing multiple icons
> 
> Note Sriram's comment about the additional browser_toolbar.xml files that
> now exist to support tablets.

Done. Change applied to all 4 (!) browser_toolbar.xml we have now.

Comment 26

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #21)
> Comment on attachment 627286 [details] [diff] [review]
> Add reader button to the browser toolbar
> 
> >diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java
> >     public ImageButton mSiteSecurity;
> >+    public ImageButton mReader;
> 
> >+    private boolean mShowSiteSecurity;
> >+    private boolean mShowReader;
> 
> Just an observation:
> Along with my desire to create a PageActions API, which would allow add-ons
> to add images to the URLbar like you added the "reader", I see a
> code-scaling problem. We are creating a specific imageview and boolean for
> each actions. We'll need to get to a generic system when we add the next
> action, or the pageaction API itself.

Totally agree. Just don't think I should block on this for now.

> >+        private ReaderPopup(Context context) {
> 
> >+            Button readingListButton = (Button) layout.findViewById(R.id.reading_list);
> >+            readingListButton.setOnClickListener(new Button.OnClickListener() {
> 
> What happens if the page is already on the reading list? Should the popup be
> ignored and "read now" be the default action if I tap the "reader" action?

I'd probably avoid changing the reader button behaviour dynamically but adapting the content of the popup (hiding/disabling "add to reading list" when url is already there) depending on the case is definitely something we should do. Need to check with UX team. Filed bug 760645 to track this.

> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >         tab.setContentType(contentType);
> >         tab.updateFavicon(null);
> >         tab.updateFaviconURL(null);
> >         tab.updateIdentityData(null);
> >+        tab.setReaderEnabled(false);
> >         tab.removeTransientDoorHangers();
> >         tab.setAllowZoom(true);
> >         tab.setDefaultZoom(0);
> >         tab.setMinZoom(0);
> >         tab.setMaxZoom(0);
> >         tab.setHasTouchListeners(false);
> >         tab.setCheckerboardColor(Color.WHITE);
> 
> Well this block of code is screaming out for a helper method to be added to
> Tab. File a followup bug?

This has been replaced by a refresh() call.

> >                     mBrowserToolbar.setTitle(uri);
> >                     mBrowserToolbar.setFavicon(null);
> >                     mBrowserToolbar.setSecurityMode(tab.getSecurityMode());
> >+                    mBrowserToolbar.setReaderVisibility(tab.getReaderEnabled());
> >                     mDoorHangerPopup.updatePopup();
> >                     mBrowserToolbar.setShadowVisibility(!(tab.getURL().startsWith("about:")));
> 
> BrowserToolbar.refresh() should be capable of doing this too. There was a
> patch somewhere to remove all of these patterns and just call
> mBrowserToolbar.refresh()

Yeah, updated the patch to use mBrowserToolbar.refresh() whenever applicable.

> >     void handleDocumentStart(int tabId, final boolean showProgress, String uri) {
> 
> >         tab.setState("about:home".equals(uri) ? Tab.STATE_SUCCESS : Tab.STATE_LOADING);
> >         tab.updateIdentityData(null);
> >+        tab.setReaderEnabled(false);
> 
> Does this handle back/forward session updating too? I think it should, but
> just asking.

Yep.

> >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java
> >+    public void addToReadingList() {
> 
> >+                BrowserDB.addReadingListItem(mContentResolver, getTitle(), url);
> 
> I assume this code handles duplicates (i.e. we don't want dupes in the list)

Yep. We first try to update an existing entry and insert only if there isn't.

> >diff --git a/mobile/android/base/resources/layout/reader_popup.xml b/mobile/android/base/resources/layout/reader_popup.xml
> >+</LinearLayout>
> >\ No newline at end of file
> 
> Add a newline

Done.

Comment 27

[:Margaret Leibovic]

Comment on attachment 629295 [details] [diff] [review]
Remove unnecessary local variable in addBookmarkItem()

Not to scope creep here or anything, but we have the same unnecessary local variable down below in removeBookmark and removeBookmarksWithURL, so you could get rid of those, too.

Comment 28

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #15)
> Created attachment 628001 [details]
> mockup of reader mode menu
> 
> Here are some updated icons for the urlbar, to make the lock,
> stop/go/search, and reader mode icons feel more in line. Please replace all
> of these at the same time. http://cl.ly/3O42022J1H0N2E1Y3K11

FYI: I decided to update all icons at once in a separate bug 760724, just to avoid blocking on this for now.

Comment 29

[Lucas Rocha (:lucasr)]

Attachment: Screenshot of reader menu

Here's how it looks now.

Comment 30

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4168c98dd747
http://hg.mozilla.org/integration/mozilla-inbound/rev/315b7cb0a7ef
http://hg.mozilla.org/integration/mozilla-inbound/rev/95006ffc47e1
http://hg.mozilla.org/integration/mozilla-inbound/rev/456839be8a72
http://hg.mozilla.org/integration/mozilla-inbound/rev/26a5bfa9733a
http://hg.mozilla.org/integration/mozilla-inbound/rev/1330924c44f2
http://hg.mozilla.org/integration/mozilla-inbound/rev/d4a962a62d63

Comment 31

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4168c98dd747
https://hg.mozilla.org/mozilla-central/rev/315b7cb0a7ef
https://hg.mozilla.org/mozilla-central/rev/95006ffc47e1
https://hg.mozilla.org/mozilla-central/rev/456839be8a72
https://hg.mozilla.org/mozilla-central/rev/26a5bfa9733a
https://hg.mozilla.org/mozilla-central/rev/1330924c44f2
https://hg.mozilla.org/mozilla-central/rev/d4a962a62d63

Comment 32

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

(In reply to Lucas Rocha (:lucasr) from comment #25)
> (In reply to Mark Finkle (:mfinkle) from comment #20)
> > Comment on attachment 627284 [details] [diff] [review]
> > Pack URL bar buttons in same layout to allow showing multiple icons
> > 
> > Note Sriram's comment about the additional browser_toolbar.xml files that
> > now exist to support tablets.
> 
> Done. Change applied to all 4 (!) browser_toolbar.xml we have now.

Based on what was checked in, this was not done. Bug 761131 is tracking crashes as a result.

Comment 33

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

(In reply to Mark Finkle (:mfinkle) from comment #32)
> (In reply to Lucas Rocha (:lucasr) from comment #25)
> > (In reply to Mark Finkle (:mfinkle) from comment #20)
> > > Comment on attachment 627284 [details] [diff] [review]
> > > Pack URL bar buttons in same layout to allow showing multiple icons
> > > 
> > > Note Sriram's comment about the additional browser_toolbar.xml files that
> > > now exist to support tablets.
> > 
> > Done. Change applied to all 4 (!) browser_toolbar.xml we have now.
> 
> Based on what was checked in, this was not done. Bug 761131 is tracking
> crashes as a result.

OK, maybe I assumed too much here. The "reader" button needed to be added to all 4 browser_toolbar files too. I think you only added the linear layout to all 4 files.