Last Comment Bug 784387 - Long press Reader Mode icon to add article to Reading List
: Long press Reader Mode icon to add article to Reading List
Status: VERIFIED FIXED
[mentor=lucasr][lang=java][lang=js]
:
Product: Firefox for Android
Classification: Client Software
Component: Reader View (show other bugs)
: unspecified
: ARM Android
: P3 normal (vote)
: Firefox 23
Assigned To: aditya2204
:
Mentors:
Depends on:
Blocks: 866766
  Show dependency treegraph
 
Reported: 2012-08-21 09:36 PDT by Ian Barlow (:ibarlow)
Modified: 2013-05-21 11:19 PDT (History)
14 users (show)
teodora.vermesan: in‑moztrap+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
23+


Attachments
I have added the onLongClick Listener here.. (51.20 KB, patch)
2013-02-14 04:58 PST, aditya2204
lucasr.at.mozilla: review-
Details | Diff | Splinter Review
Working patch (1.42 KB, patch)
2013-02-17 08:53 PST, aditya2204
no flags Details | Diff | Splinter Review
Patch in better format (1.59 KB, patch)
2013-02-18 03:50 PST, aditya2204
lucasr.at.mozilla: review-
Details | Diff | Splinter Review
Final patch !! (1.41 KB, patch)
2013-02-18 08:39 PST, aditya2204
lucasr.at.mozilla: review-
Details | Diff | Splinter Review
Final patch !! (1.40 KB, patch)
2013-02-18 08:53 PST, aditya2204
no flags Details | Diff | Splinter Review
Final patch !! (1.40 KB, patch)
2013-02-18 09:02 PST, aditya2204
lucasr.at.mozilla: review+
Details | Diff | Splinter Review
Absolute final patch !! (1.40 KB, patch)
2013-02-19 05:51 PST, aditya2204
no flags Details | Diff | Splinter Review
Absolute final patch !! (1.40 KB, patch)
2013-02-19 05:55 PST, aditya2204
no flags Details | Diff | Splinter Review
Handle duplicate reading list item case (8.14 KB, patch)
2013-04-16 07:42 PDT, Lucas Rocha (:lucasr)
bnicholson: review+
Details | Diff | Splinter Review

Description Ian Barlow (:ibarlow) 2012-08-21 09:36:08 PDT
Let's try offering a quicker way to add stuff to your Reading List: by long pressing the reader icon in the title bar. 

That way, you can add an item from your list both in the Reader Mode toolbar, and also do it more quickly from the regular content view.
Comment 1 aditya2204 2013-02-07 07:11:21 PST
Hey..

I am willing to take up this bug.
Comment 2 Lucas Rocha (:lucasr) 2013-02-07 07:16:34 PST
Nice! Assigning to you.
Comment 3 aditya2204 2013-02-14 04:58:17 PST
Created attachment 713874 [details] [diff] [review]
I have added the onLongClick Listener here..
Comment 4 aditya2204 2013-02-14 05:04:23 PST
Hey guys.. This is my first patch submission (and the first one that I've created..!!)..

Please point out any errors and improvements wherever possible..

Thanks..:)
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2013-02-14 05:41:52 PST
aditya - Thanks for the code. One thing to do is create a "patch" in a specific format. Here is a doc for how to make a patch:
https://developer.mozilla.org/en-US/docs/Creating_a_patch

The patch format helps us review only the changes you make to files. It makes it a lot easier for us to see what you modified.
Comment 6 aditya2204 2013-02-15 03:29:41 PST
(In reply to Mark Finkle (:mfinkle) from comment #5)
> aditya - Thanks for the code. One thing to do is create a "patch" in a
> specific format. Here is a doc for how to make a patch:
> https://developer.mozilla.org/en-US/docs/Creating_a_patch
> 
> The patch format helps us review only the changes you make to files. It
> makes it a lot easier for us to see what you modified.

Well, it's my pleasure..!!

Anyways, I made the above 'patch' by using hg queues. Will it be favourable to do that against using the 'hg diff' output..?
Comment 7 Lucas Rocha (:lucasr) 2013-02-15 05:21:01 PST
Comment on attachment 713874 [details] [diff] [review]
I have added the onLongClick Listener here..

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

Thanks for the patch! Needs work but the necessary changes in the patch are simple.

::: mobile/android/base/BrowserToolbar.java
@@ +267,5 @@
>          });
>  
> +// Added for the LongClick operation
> +
> +         mStop.setOnLongClickListener( new Button.OnLongClickListener() {

You have to *add* the long click listener instead replacing the current click listener. You should also keep the check for tab != null.
Comment 8 aditya2204 2013-02-17 08:53:12 PST
Created attachment 714920 [details] [diff] [review]
Working patch

Here's a working patch !!
Comment 9 aditya2204 2013-02-18 03:50:04 PST
Created attachment 715108 [details] [diff] [review]
Patch in better format

Issue with using MQ seems to be fixed.
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2013-02-18 05:25:34 PST
Comment on attachment 715108 [details] [diff] [review]
Patch in better format

Lucas' review will be enough. I would remove the comments though.
Comment 11 Lucas Rocha (:lucasr) 2013-02-18 06:30:14 PST
Comment on attachment 715108 [details] [diff] [review]
Patch in better format

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

Almost there. Just remove the bits as suggested and this patch is good to go :-)

::: mobile/android/base/BrowserToolbar.java
@@ +274,5 @@
>                  if (tab != null)
>                      tab.readerMode();
>              }
>          });
> +// Added OnLongClickListener here

Remove this comment.

@@ +280,5 @@
> +        mReader.setOnLongClickListener(new Button.OnLongClickListener() {
> +            public boolean onLongClick(View v) {
> +                Tab tab = Tabs.getInstance().getSelectedTab();
> +                if (tab != null) {
> +                    tab.readerMode();

Remove the readerMode() call here. The long press is only supposed to add the current page to reading list.

@@ +284,5 @@
> +                    tab.readerMode();
> +                    tab.addToReadingList();
> +                    return true;
> +                   }
> +                    else

Remove this "else". Not necessary.

@@ +289,5 @@
> +                    return false;
> +             }
> +         });
> +
> +// Till here

Remove this comment.
Comment 12 aditya2204 2013-02-18 08:39:52 PST
Created attachment 715161 [details] [diff] [review]
Final patch !!

Should do the job.
Comment 13 aditya2204 2013-02-18 08:41:24 PST
(In reply to Mark Finkle (:mfinkle) from comment #10)
> Comment on attachment 715108 [details] [diff] [review]
> Patch in better format
> 
> Lucas' review will be enough. I would remove the comments though.

Surely..

Thanks..!!
Comment 14 Lucas Rocha (:lucasr) 2013-02-18 08:41:53 PST
Comment on attachment 715161 [details] [diff] [review]
Final patch !!

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

Fix these nits and the patch is ready to do :-)

::: mobile/android/base/BrowserToolbar.java
@@ +281,5 @@
> +                Tab tab = Tabs.getInstance().getSelectedTab();
> +                if (tab != null) {
> +                    tab.addToReadingList();
> +                    return true;
> +                   }

Add empty line here. Fix indentation of the '}' here.

@@ +282,5 @@
> +                if (tab != null) {
> +                    tab.addToReadingList();
> +                    return true;
> +                   }
> +                    return false;

Indent this line correctly.
Comment 15 aditya2204 2013-02-18 08:53:59 PST
Created attachment 715170 [details] [diff] [review]
Final patch !!

Should be the final one !!
Comment 16 aditya2204 2013-02-18 09:02:51 PST
Created attachment 715173 [details] [diff] [review]
Final patch !!

Should be it !!
Comment 17 Lucas Rocha (:lucasr) 2013-02-18 09:06:16 PST
Comment on attachment 715173 [details] [diff] [review]
Final patch !!

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

Looks good. I assume you tested the patch locally?
Comment 18 aditya2204 2013-02-19 05:51:33 PST
Created attachment 715467 [details] [diff] [review]
Absolute final patch !!
Comment 19 aditya2204 2013-02-19 05:55:09 PST
Created attachment 715468 [details] [diff] [review]
Absolute final patch !!

Did one more nitpick.
Comment 20 aditya2204 2013-02-19 05:56:28 PST
(In reply to Lucas Rocha (:lucasr) from comment #17)
> Comment on attachment 715173 [details] [diff] [review]
> Final patch !!
> 
> Review of attachment 715173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I assume you tested the patch locally?

Yes..!!
Comment 21 Lucas Rocha (:lucasr) 2013-02-20 10:25:16 PST
I've found one issue in this patch after trying it locally. We're showing an error message when you long tap on the reader mode and the current is already in the reading list.

We should just show a message stating that the page is already in the reading list. Have a look at the code in browser.js:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7674

Maybe send success if the page is already there but with an extra flag to tell the Java side what message to show? Maybe a cleaner approach would be to return a "result code" (0 for added, 1 for already-in-reading-list, and 2 for failure) instead of the current boolean.
Comment 22 :Margaret Leibovic 2013-04-09 16:23:29 PDT
Aditya, have you managed to make any progress on this since Lucas's last comment? Let us know if you need help!
Comment 23 Lucas Rocha (:lucasr) 2013-04-16 07:42:44 PDT
Created attachment 737996 [details] [diff] [review]
Handle duplicate reading list item case
Comment 24 Lucas Rocha (:lucasr) 2013-04-16 07:43:30 PDT
(In reply to Lucas Rocha (:lucasr) from comment #23)
> Created attachment 737996 [details] [diff] [review]
> Handle duplicate reading list item case

FYI: this patch covers the issue described in comment #21.
Comment 25 :Margaret Leibovic 2013-04-16 10:24:16 PDT
Comment on attachment 737996 [details] [diff] [review]
Handle duplicate reading list item case

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

::: mobile/android/chrome/content/browser.js
@@ +6532,5 @@
> +              let result = (success ? this.READER_ADD_SUCCESS :
> +                  this.READER_ADD_FAILED);
> +              sendResult(result, article.title);
> +            }.bind(this));
> +          }.bind(this));

This could be a cool opportunity to play around with fat arrow syntax, since that would let you avoid these bind(this) calls :)
Comment 26 Brian Nicholson (:bnicholson) (PTO through August 1) 2013-04-16 11:47:30 PDT
Comment on attachment 737996 [details] [diff] [review]
Handle duplicate reading list item case

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

The patch itself looks okay, but I don't understand why we'd want this behavior. The first patch adds to reading list on long press -- why not do the opposite and remove it if the icon is highlighted and the page is in the reading list? Long press to both add and remove seems like a natural parity to me.

::: mobile/android/base/BrowserApp.java
@@ +440,5 @@
> +        // 2 - Already in reading list
> +        if (result != 1) {
> +            if (result == 0) {
> +                showToast(R.string.reading_list_failed, Toast.LENGTH_SHORT);
> +            } else if (result == 2) {

Rather than having magic numbers inlined in the code, why not define these as constants like you did in browser.js? Also, it might be cleaner to have 0 be success, and non-zero values can indicate different error codes.
Comment 27 Lucas Rocha (:lucasr) 2013-04-16 12:33:52 PDT
(In reply to Brian Nicholson (:bnicholson) from comment #26)
> Comment on attachment 737996 [details] [diff] [review]
> Handle duplicate reading list item case
> 
> Review of attachment 737996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch itself looks okay, but I don't understand why we'd want this
> behavior. The first patch adds to reading list on long press -- why not do
> the opposite and remove it if the icon is highlighted and the page is in the
> reading list? Long press to both add and remove seems like a natural parity
> to me.

This is the behaviour ibarlow and I agreed last time we chatted about this bug. The problem I see with the toggling approach is that you'd probably need to change the reader icon to communicate the current reading list state. Otherwise long press will become a bit of a hit-and-miss i.e. you don't what it will do until you try it.

Always adding with long press makes it predictable. Also, keep in mind that this is meant to be just a power-user shortcut as an alternative to entering reader mode then tapping on the reading list button.

There are pros and cons for both approaches. I personally prefer the long-press-always-adds-to-reading-list behaviour. Ian, what do you think?

> ::: mobile/android/base/BrowserApp.java
> @@ +440,5 @@
> > +        // 2 - Already in reading list
> > +        if (result != 1) {
> > +            if (result == 0) {
> > +                showToast(R.string.reading_list_failed, Toast.LENGTH_SHORT);
> > +            } else if (result == 2) {
> 
> Rather than having magic numbers inlined in the code, why not define these
> as constants like you did in browser.js? Also, it might be cleaner to have 0
> be success, and non-zero values can indicate different error codes.

Good point. I'll do that.
Comment 28 Brian Nicholson (:bnicholson) (PTO through August 1) 2013-04-16 13:45:21 PDT
Comment on attachment 737996 [details] [diff] [review]
Handle duplicate reading list item case

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

I saw this screenshot awhile ago: https://bug784331.bugzilla.mozilla.org/attachment.cgi?id=704298. I had just assumed that the orange icon meant that the page was added to the reading list (similar to how a bookmark star gets filled in) since I never read the bug comments. But looking at Nightly, we don't actually have any indicator for whether a page is in the reading list like I thought we did.

So without any indicator for whether a page is in the reading list, I agree that long press to remove isn't a good option. One other option to consider, though, is replacing the saved reader page instead of bailing on duplicates. That would allow the user to update the saved article in case the page's content has changed for that URL.
Comment 29 Ian Barlow (:ibarlow) 2013-04-16 13:57:18 PDT
(In reply to Lucas Rocha (:lucasr) from comment #27)

> There are pros and cons for both approaches. I personally prefer the
> long-press-always-adds-to-reading-list behaviour. Ian, what do you think?


I agree, consistency is the better approach here. I doubt many users would remember if something was in their Reading List, much less think to long press the icon to remove it.

It does raise the question, though, is the book with the X through it the best icon to use when in Reader Mode. Long pressing something with an X in it does sort of imply that some kind of remove behaviour will take place. I'll think about that visual treatment a bit, but don't let that block progress on this bug.
Comment 32 Aaron Train [:aaronmt] 2013-04-22 08:21:08 PDT
Accidental find (wasn't tracking this issue). Seems to work for me.
Comment 33 Teodora Vermesan (:TeoVermesan) 2013-05-13 02:25:50 PDT
Test Case added in moztrap:
https://moztrap.mozilla.org/manage/case/8077/
Comment 34 Lukas Blakk [:lsblakk] use ?needinfo 2013-05-21 11:19:48 PDT
This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.

Note You need to log in before you can comment on or make changes to this bug.