802093 - Reader Mode:Update reading list button state if the list is empty

Status: VERIFIED FIXED

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

Description

[Paul Feher]

Firefox 19.0a1 (2012-10-16)
Device: Samsung Galaxy R
OS: Android 2.3.4

Steps to reproduce:
1. Go to news.google.com and tap on any news from the list
2. Switch to reader mode.
3. Tap the page content to access the reading mode toolbar.
4. Try to access the reading list by tapping on the corresponding button.

Expected result:
If the reading list is empty the button should be deactivated (grayed-out) and you should not be able to access the reading list.

Actual result:
You are able to access the reading list even if the list is empty.The button is active.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Take a look at this for me? I've been testing it and it came out pretty well  :)

Comment 2

[Mark Capella [:capella]]

Attachment: Example Screens

1) Page not in readerlist, readerlist is empty
2) Page not in readerlist, readerlist has entries
3) Page in readerlist, readerlist has it as an entry and/or others

I should have mentioned this dynamically handles state changes while navigating forward / back, adding / removing readerlist entries from other tabs / private tabs, context menu readerlist item deletions, etc.

Comment 3

[Mark Capella [:capella]]

And for testing ...
https://tbpl.mozilla.org/?tree=Try&rev=61ffc8f8cdd1

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 760141 [details] [diff] [review]
Patch (v1)

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

Looks pretty good. Just need the tweaks on the event names.

::: mobile/android/base/BrowserApp.java
@@ +338,5 @@
>              public void run() {
>                  BrowserDB.addReadingListItem(getContentResolver(), title, url);
>                  showToast(R.string.reading_list_added, Toast.LENGTH_SHORT);
> +
> +                int count = BrowserDB.getReadingListCount(getContentResolver());

nit: add final to this count variable.

@@ +351,5 @@
>              public void run() {
>                  BrowserDB.removeReadingListItemWithURL(getContentResolver(), url);
>                  showToast(R.string.reading_list_removed, Toast.LENGTH_SHORT);
> +
> +                int count = BrowserDB.getReadingListCount(getContentResolver());

ditto.

::: mobile/android/chrome/content/aboutReader.js
@@ +30,5 @@
>  
>    Services.obs.addObserver(this, "Reader:FaviconReturn", false);
>    Services.obs.addObserver(this, "Reader:Add", false);
>    Services.obs.addObserver(this, "Reader:Remove", false);
> +  Services.obs.addObserver(this, "Reader:ListCount", false);

Name it ListCountReturn to be consistent with FaviconReturn?

You're sending a Reader:ListCount from java to gecko in different situations (when you request list count and when new items are added/removed from reading list). I'd prefer to have Reader:ListCountReturn and a Reader:ListCountUpdated as separate events being handled the same way. Just for correcteness and clarity.

@@ +207,5 @@
>            }
>          }
>          break;
>        }
> +

Just add: case "Reader:ListCountUpdated" here.

@@ +208,5 @@
>          }
>          break;
>        }
> +
> +      case "Reader:ListCount": {

Rename this to Reader:ListCountReturn

@@ +210,5 @@
>        }
> +
> +      case "Reader:ListCount": {
> +        if (this._readingListCount != aData) {
> +          this._readingListCount = aData;

Isn't aData a string here? Might be worth casting this explicitly to an int, just in case.

Comment 5

[Lucas Rocha (:lucasr)]

Ian, happy about the screenshots?

Comment 6

[Ian Barlow (:ibarlow)]

Looks fine to me

Comment 7

[Mark Capella [:capella]]

Attachment: Patch (v2)

Comment 8

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e011badb20e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e011badb20e