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

VERIFIED FIXED in Firefox 24

Status

()

P3
normal
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: paul.feher, Assigned: capella)

Tracking

19 Branch
Firefox 24
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox24 verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

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

Updated

6 years ago
Priority: -- → P3
(Assignee)

Comment 1

5 years ago
Created attachment 760141 [details] [diff] [review]
Patch (v1)

Take a look at this for me? I've been testing it and it came out pretty well  :)
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #760141 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 2

5 years ago
Created attachment 760142 [details]
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 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.
Attachment #760141 - Flags: review?(lucasr.at.mozilla) → review-
Ian, happy about the screenshots?
Flags: needinfo?(ibarlow)
Looks fine to me
Flags: needinfo?(ibarlow)
(Assignee)

Comment 7

5 years ago
Created attachment 760438 [details] [diff] [review]
Patch (v2)
Attachment #760438 - Flags: review?(lucasr.at.mozilla)

Updated

5 years ago
Attachment #760438 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/5e011badb20e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox24: --- → verified
You need to log in before you can comment on or make changes to this bug.