Closed Bug 800899 Opened 12 years ago Closed 11 years ago

Reader Mode:Toolbar remove button not updated when removing a Reading List entry via Context Menu

Categories

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

19 Branch
ARM
Android
defect

Tracking

(firefox16 affected, firefox17 affected, firefox18 affected, firefox19 affected)

RESOLVED FIXED
Firefox 23
Tracking Status
firefox16 --- affected
firefox17 --- affected
firefox18 --- affected
firefox19 --- affected

People

(Reporter: paul.feher, Assigned: capella)

References

Details

Attachments

(1 file, 4 obsolete files)

Firefox 18.0a2 (2012-10-11)
Device: Samsung Galaxy Tab
OS: Android 3.1

Steps to reproduce:
1. Go to news.google.com and tap on any news from the list.
2. Tap on screen to invoke the Reader Mode toolbar.
3. Add the page to Reading List by tapping the button from the down left corner.
4. Go to the reading list by tapping on the reading list icon.
5. Long tap the page added in the reading list for the context menu to appear.
6. Tap remove from the context menu.
7. Double tap the Backspace button to go back to the initial page.
8. Check the add/remove button from the Reader Mode toolbar.

Expected result:
The button should be updated in the "add to reading list" button having the specific icon attached.

Actual result:
The "remove from the reading list" button is still present. If you click the button the "Page removed from your Reading List" message is displayed.
The toolbar would need to listen for any changes. Also closing and re-opening the toolbar did not swap the button stat; only via a reload of the page did it.
Priority: -- → P2
Assignee: nobody → markcapella
Attached patch Patch (v1) (obsolete) — Splinter Review
I noticed this bug myself, saw the outstanding request and thought I'd give it a stab.

This patch seems to work pretty well, handles status changes across multiple tabs, private or public, and propagates changes (add to readerlist / remove from readerlist) made from either the reader mode screen or from the readerlist context menu for the remove.

One question I'm still looking at is removal of the Observers I'm adding. 

Also, this points out another existing bug in the process. Since the readerlist status for the tab is poplated going in with "readerList=#" in the url, if a user changes the status (add/remove from reader list) while the changes are propagated, internal status is changed, but the URL isnt updated and any user refreshing of the screen (briefly) mucks up the apparant status of that single tab/page.
Attachment #732448 - Flags: feedback?(bnicholson)
Comment on attachment 732448 [details] [diff] [review]
Patch (v1)

I think Lucas would be a better reviewer for this since he implemented the reader toolbar, so passing review to him.
Attachment #732448 - Flags: feedback?(bnicholson) → feedback?(lucasr.at.mozilla)
Status: NEW → ASSIGNED
Just noticed Bug 814587 ... might have to take that into account depending on timing of the landings
Comment on attachment 732448 [details] [diff] [review]
Patch (v1)

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

We already have things like Reader:Added and Reader:Removed messages. You should probably use them instead of creating new message types.
Attachment #732448 - Flags: feedback?(lucasr.at.mozilla) → feedback-
I don't think that works. I had to create new messages so aboutreader.js could signal to itself that all tabs need to udpate their reader mode menu icon status ... these is a subtly discrete / differently timed message. I was thinking especially with multiple tabs open to the same page public and private ... and the fact that:

/Reader:Add/ and /Reader:Remove/ pair goes to JS and //Reader:Added// and //Reader:Removed// pair goes to JAVA

Normally for the ADD cases:

1)   ||tab.java||         -> /Reader:Add/   -> |browser.js| -> //Reader:Added//   -> ||geckoapp.java||
2)   |adoutreader.js| -------------------------------------------^

Normally for the REMOVE cases:

3)   ||awesomebar.java||  -> /Reader:Remove/ -> |browser.js| -> //Reader:Removed// -> ||geckoapp.java||
4)   |aboutreader.js| -------------------------------------------^


(Note that case 1) isn't tied into the context menu / doesn't exist yet technically)


|aboutreader.js| can't listen for //Reader:Added// or //Reader:Removed// as you suggest, since they're going to JAVA. 

Also, to complete the loop, |aboutreader.js| needs to send a JS message to itself to update open tabs in readermode when it changes one of them.

At this point I decided for |aboutreader.js| to issue a new pair of JS messages /Reader:Toggle:On/ and /Reader:Toggle:Off/ to itself.

The alternative was for it to both listen for and re-emit the JS /Reader:Add/ and /Reader:Remove/ pair, but that would run through |browser.js| and then ||geckoapp.java|| a second time and this seemed clean and faster. If you want I can review the code to see how that would be handled.

Well, think about it and I will too, maybe my memory fails me. I'm working on a few other things and this was a little confusing in the first place :D
Summary: Reader Mode:Toolbar remove button is not updated when removing a Reading List entry via Custom Menu → Reader Mode:Toolbar remove button not updated when removing a Reading List entry via Context Menu
Attachment #732448 - Flags: feedback- → feedback?(lucasr.at.mozilla)
Comment on attachment 732448 [details] [diff] [review]
Patch (v1)

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

I really don't like the fact that the new messages create a separate code path for handling UI-specific state. The goal here is to make reader aware of data changes made from somewhere else in the UI.

Simply watching for the existing messages for when an article is added/removed is less error-prone because if later we add new ways to add/remove items from the reading list, you don't need to remember to send this separate set of UI-specific messages. Comment #4 is a clear example of what I'm talking about.

So, let's use Reader:Add and Reader:Remove here and if you feel there's something about these messages that is not implemented correctly/consistently, let's just fix it.
Attachment #732448 - Flags: feedback?(lucasr.at.mozilla) → feedback-
Attached patch Patch (v2) (obsolete) — Splinter Review
Oh, well it seems I over-conceptualized this :)

This does seem to handle the situation gracefully and is much cleaner / tighter.
Attachment #732448 - Attachment is obsolete: true
Attachment #736811 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 736811 [details] [diff] [review]
Patch (v2)

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

Looks much better, thanks :-)

Just giving r- until I get an answer for the Reader:Add listener question.

::: mobile/android/chrome/content/aboutReader.js
@@ +220,5 @@
>      if (this._isReadingListItem) {
>        gChromeWin.Reader.storeArticleInCache(this._article, function(success) {
>          dump("Reader:Add (in reader) success=" + success);
>  
> +        Services.obs.notifyObservers(null, "Reader:Add", this._article.url);

Won't his trigger the message listener in browser.js which will then send a Reader:Added message too? If so, maybe just emitting this Reader:Add message and removing all the cache-related bits from here would be better?

@@ +233,5 @@
>      } else {
>        gChromeWin.Reader.removeArticleFromCache(this._article.url , function(success) {
>          dump("Reader:Remove (in reader) success=" + success);
>  
> +        Services.obs.notifyObservers(null, "Reader:Remove", this._article.url);

Same here.
Attachment #736811 - Flags: review?(lucasr.at.mozilla) → review-
No, the Reader:Add/Remove messages I generate out of aboutreader.js work when fed back to itsef, but the aData isn't what browser.js expects and it fails there.
Specifically,

let tab = BrowserApp.getTabForId(aData);   // returns null
let currentURI = tab.browser.currentURI;   // This fails

For the reader:remove case, it doesn't try to call java either way.
Attached patch Patch (v3) (obsolete) — Splinter Review
I don't want to early exit on (tab returning null) in browser.js, as that obscures the inconsistent use of the parm ... how about a little expansion of the Reader:Add message?

push to TRY for testing https://tbpl.mozilla.org/?tree=Try&rev=cb6b4481fe45
Attachment #737264 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 737264 [details] [diff] [review]
Patch (v3)

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

::: mobile/android/base/Tab.java
@@ +390,5 @@
>      public void addToReadingList() {
>          if (!mReaderEnabled)
>              return;
>  
> +        JSONObject json = new JSONObject();

Make this variable a 'final'

::: mobile/android/chrome/content/browser.js
@@ +6497,5 @@
>      switch(aTopic) {
>        case "Reader:Add": {
> +        // Ignore adds initiated from aboutReader menu banner
> +        let info = JSON.parse(aData);
> +        if (info.url)

Relying on empty arguments to define the handler's behaviour here is a bit hacky. Add a more explicit argument that describes the intended behaviour more clearly. Maybe something like a boolean argument like "fromAboutReader"?

@@ +6499,5 @@
> +        // Ignore adds initiated from aboutReader menu banner
> +        let info = JSON.parse(aData);
> +        if (info.url)
> +          break;
> +

Micro-efficiency suggestion: create a local variable to hold the tabID here to avoid extra property searches on info. 

let tabID = info.tabID;
Attachment #737264 - Flags: feedback?(lucasr.at.mozilla) → feedback-
Attached patch Patch (v4) (obsolete) — Splinter Review
Ok, have another look ... your test suggestion (click link in readermode, then change readerlist status for same page on another tab, then click back in first tab) still has readerlist status wrong ...

caused I think by same problem with URL entry to page setting inReaderMode status ... then page refreshes mucks it up after status change ...
Attachment #736811 - Attachment is obsolete: true
Attachment #737264 - Attachment is obsolete: true
Attachment #738045 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 738045 [details] [diff] [review]
Patch (v4)

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

I've just landed a patch for bug 814587 that uses your Tab.java changes. I've also made changes to how Reader:Add is handled that is in line with what you're doing here (json object as argument instead of just the tabID). You'll have to rebase. On the plus side, this will probably simplify your patch a bit.

Still need to figure out a way to re-add the observers if the user goes back to reader.

::: mobile/android/base/Tab.java
@@ +392,5 @@
>              return;
>  
> +        final JSONObject json = new JSONObject();
> +        try {
> +            json.put("fromAboutReader", false);

No need to explicitly define 'fromAboutReader' here. Just check if the key is defined in the arguments in browser.js

::: mobile/android/chrome/content/aboutReader.js
@@ +203,5 @@
>          break;
> +
> +      case "unload":
> +        Services.obs.removeObserver(this, "Reader:Add");
> +        Services.obs.removeObserver(this, "Reader:Remove");

If you do this, you'll have to re-add the observers if the user goes back to reader. Please try the following steps to verify:

1. Go to any page where you can enter reader mode
2. Enter reader mode
3. Go to some other page either by clicking on a link in the article or typing a new URL
4. Use back button to go back to reader article

On step 3, I assume the 'unload' event will be triggered and you'll remove the observers. On step 4, the observers won't work anymore. Maybe you'll have to track 'pageshow' on the respective browser element or something. mfinkle will probably have better advice to give you here.

@@ +229,5 @@
>      if (this._isReadingListItem) {
>        gChromeWin.Reader.storeArticleInCache(this._article, function(success) {
>          dump("Reader:Add (in reader) success=" + success);
>  
> +        let json = JSON.stringify({ "fromAboutReader" : true, "url" : this._article.url });

No need to quote the key names here. I'd rename the json with args.

::: mobile/android/chrome/content/browser.js
@@ +6496,5 @@
>    observe: function(aMessage, aTopic, aData) {
>      switch(aTopic) {
>        case "Reader:Add": {
> +        // Ignore adds initiated from aboutReader menu banner
> +        let info = JSON.parse(aData);

Rename info with args.
Attachment #738045 - Flags: review?(lucasr.at.mozilla) → review-
Attached patch Patch (v5)Splinter Review
Ok, hoping this one is it :D

Regarding the unloading of the observers, we're actually all set with this patch.

Entering the readermode page by 1) pressing the awesomebar icon or 2) clicking a readerlist item adds the observers as expected.

Leaving the readermode page by 1) Clicking the awesomebar's close readermode icon, or 2) Pressing the back button, or 3) navigating forward via link removes the observers.

Returning to the readermode page from the forward navigated link by pressing the back button re-adds the observers properly!

I still need to follow up with a bug request to handle the initial readerlist status being determined at page entry time, versus being passed in through the ?readingList=# url parm as we currently do.
Attachment #738045 - Attachment is obsolete: true
Attachment #739529 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 739529 [details] [diff] [review]
Patch (v5)

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

Looks good (with nits fixed). I still suspect that the observers won't work if you follow a link while in reader mode then go back. This can be investigated in a follow-up though (please file a bug if you confirm this issue exists).

::: mobile/android/chrome/content/aboutReader.js
@@ +243,5 @@
>  
>          let result = (success ? gChromeWin.Reader.READER_ADD_SUCCESS :
>              gChromeWin.Reader.READER_ADD_FAILED);
>  
> +        let json = JSON.stringify({ fromAboutReader: true, url: this._article.url });

No need to pass a URL here as the fromAboutReader will make the handler for Reader:Add bail before handling any tabID or URL.

nit: rename json to args.

::: mobile/android/chrome/content/browser.js
@@ +6545,5 @@
>  
>          let tabID = null;
>          let url, urlWithoutRef;
>  
> +        if ('fromAboutReader' in args) {

I have a slight preference for having this 'if' even before declaring the variables above. No big deal though.
Attachment #739529 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/e07d7cf9649e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Depends on: 982019
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: