784387 - Long press Reader Mode icon to add article to Reading List

Status: VERIFIED FIXED

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

Description

[Ian Barlow (:ibarlow)]

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]

Hey..

I am willing to take up this bug.

Comment 2

[Lucas Rocha (:lucasr)]

Nice! Assigning to you.

Comment 3

[aditya2204]

Attachment: I have added the onLongClick Listener here..

Comment 4

[aditya2204]

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?)]

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]

(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)]

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]

Attachment: Working patch

Here's a working patch !!

Comment 9

[aditya2204]

Attachment: Patch in better format

Issue with using MQ seems to be fixed.

Comment 10

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

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

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]

Attachment: Final patch !!

Should do the job.

Comment 13

[aditya2204]

(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)]

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]

Attachment: Final patch !!

Should be the final one !!

Comment 16

[aditya2204]

Attachment: Final patch !!

Should be it !!

Comment 17

[Lucas Rocha (:lucasr)]

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]

Attachment: Absolute final patch !!

Comment 19

[aditya2204]

Attachment: Absolute final patch !!

Did one more nitpick.

Comment 20

[aditya2204]

(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)]

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]

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

Attachment: Handle duplicate reading list item case

Comment 24

[Lucas Rocha (:lucasr)]

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

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

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

(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)]

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

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

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7568a8b8c96
https://hg.mozilla.org/integration/mozilla-inbound/rev/91255369ead5

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7568a8b8c96
https://hg.mozilla.org/mozilla-central/rev/91255369ead5

Comment 32

[Aaron Train [:aaronmt]]

Accidental find (wasn't tracking this issue). Seems to work for me.

Comment 33

[Teodora Vermesan (:TeoVermesan)]

Test Case added in moztrap:
https://moztrap.mozilla.org/manage/case/8077/

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

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.