The default bug view has changed. See this FAQ.

Long press Reader Mode icon to add article to Reading List

VERIFIED FIXED in Firefox 23

Status

()

Firefox for Android
Reader View
P3
normal
VERIFIED FIXED
5 years ago
8 months ago

People

(Reporter: ibarlow, Assigned: aditya2204)

Tracking

unspecified
Firefox 23
ARM
Android
Points:
---
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox23 verified, relnote-firefox 23+)

Details

(Whiteboard: [mentor=lucasr][lang=java][lang=js])

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Priority: -- → P3

Updated

4 years ago
Whiteboard: [mentor=lucasr][lang=java][lang=js]
(Assignee)

Comment 1

4 years ago
Hey..

I am willing to take up this bug.
Nice! Assigning to you.
Assignee: nobody → aditya2204
(Assignee)

Comment 3

4 years ago
Created attachment 713874 [details] [diff] [review]
I have added the onLongClick Listener here..
(Assignee)

Comment 4

4 years ago
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..:)
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.
(Assignee)

Comment 6

4 years ago
(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 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.
Attachment #713874 - Flags: review-
(Assignee)

Comment 8

4 years ago
Created attachment 714920 [details] [diff] [review]
Working patch

Here's a working patch !!
Attachment #713874 - Attachment is obsolete: true
Attachment #714920 - Flags: review?
(Assignee)

Comment 9

4 years ago
Created attachment 715108 [details] [diff] [review]
Patch in better format

Issue with using MQ seems to be fixed.
Attachment #714920 - Attachment is obsolete: true
Attachment #714920 - Flags: review?
Attachment #715108 - Flags: review?(mark.finkle)
Attachment #715108 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 715108 [details] [diff] [review]
Patch in better format

Lucas' review will be enough. I would remove the comments though.
Attachment #715108 - Flags: review?(mark.finkle)
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.
Attachment #715108 - Flags: review?(lucasr.at.mozilla) → review-
(Assignee)

Comment 12

4 years ago
Created attachment 715161 [details] [diff] [review]
Final patch !!

Should do the job.
Attachment #715108 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
(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 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.
Attachment #715161 - Flags: review-
(Assignee)

Comment 15

4 years ago
Created attachment 715170 [details] [diff] [review]
Final patch !!

Should be the final one !!
Attachment #715161 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Created attachment 715173 [details] [diff] [review]
Final patch !!

Should be it !!
Attachment #715170 - Attachment is obsolete: true
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?
Attachment #715173 - Flags: review+
(Assignee)

Comment 18

4 years ago
Created attachment 715467 [details] [diff] [review]
Absolute final patch !!
Attachment #715173 - Attachment is obsolete: true
(Assignee)

Comment 19

4 years ago
Created attachment 715468 [details] [diff] [review]
Absolute final patch !!

Did one more nitpick.
Attachment #715467 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
(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..!!
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

4 years ago
Aditya, have you managed to make any progress on this since Lucas's last comment? Let us know if you need help!
Flags: needinfo?(aditya2204)
Created attachment 737996 [details] [diff] [review]
Handle duplicate reading list item case
Attachment #737996 - Flags: review?(bnicholson)
(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

4 years ago
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 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.
Attachment #737996 - Flags: review?(bnicholson) → review-
(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.
Flags: needinfo?(aditya2204) → needinfo?(ibarlow)
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.
Attachment #737996 - Flags: review- → review+
(Reporter)

Comment 29

4 years ago
(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.
Flags: needinfo?(ibarlow)
Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7568a8b8c96
https://hg.mozilla.org/integration/mozilla-inbound/rev/91255369ead5
https://hg.mozilla.org/mozilla-central/rev/b7568a8b8c96
https://hg.mozilla.org/mozilla-central/rev/91255369ead5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Accidental find (wasn't tracking this issue). Seems to work for me.
Status: RESOLVED → VERIFIED
status-firefox23: --- → verified
Flags: in-moztrap?(fennec)
OS: Mac OS X → Android
Hardware: x86 → ARM

Updated

4 years ago
Blocks: 866766

Updated

4 years ago
relnote-firefox: --- → ?
relnote-firefox: ? → 23+
Test Case added in moztrap:
https://moztrap.mozilla.org/manage/case/8077/
Flags: in-moztrap?(fennec) → in-moztrap+
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.
You need to log in before you can comment on or make changes to this bug.