Closed Bug 757776 Opened 10 years ago Closed 10 years ago

Reader Mode: Show a special icon on awesome bar search entries that are in the reading list

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox16- verified, firefox17 verified, firefox18 verified)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox16 - verified
firefox17 --- verified
firefox18 --- verified

People

(Reporter: lucasr, Unassigned)

References

Details

Attachments

(3 files, 5 obsolete files)

For bookmarks we show a star, for reading list I guess we'd show a little book icon?
Blocks: reader
These patches are pretty much ready. Just need the real assets for the reader icon.
Here are some book icons.

Now that we have more than one kind of icon we show in the Awesomebar list, they should probably all be the same colour, so I've also included new, grey bookmark stars that should replace the current yellow ones.

http://cl.ly/3u1a0P221M2g2D333u3H
Attachment #629380 - Flags: review?(margaret.leibovic)
Attachment #627814 - Attachment is obsolete: true
Attachment #629381 - Flags: review?(margaret.leibovic)
Attachment #627816 - Attachment is obsolete: true
Attachment #629382 - Flags: review?(margaret.leibovic)
Attachment #629380 - Flags: review?(margaret.leibovic) → review+
Attachment #629381 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 629382 [details] [diff] [review]
Show reading list icon on awesomebar search

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

Just be sure to add back the |display| local variable, or this won't build now :)

::: mobile/android/base/AwesomeBarTabs.java
@@ +158,2 @@
>              viewHolder.bookmarkIconView.setVisibility(visibility);
> +            viewHolder.bookmarkIconView.setImageResource(R.drawable.ic_awesomebar_star);

This was already an existing issue, but it feels bad that we're duplicating code between here and updateBookmarkIcon, especially now that it's doing a bit more. It would be really nice if we could use updateBookmarkIcon here, but I suppose that would involve dealing with the fact that updateBookmarkIcon takes a Cursor. Perhaps we could move that cursor logic to AwesomeBarCursorAdapter's getView, similarly to how we deal with the historyItem logic here, and then updateBookmarkIcon could just take an id and a display (although then we'd need to special-case the fact that we don't want to show DISPLAY_READER icons in the history tab). I'm just thinking aloud here in case you can see a way to make this nicer :) This can be work for a clean-up patch.

@@ +500,5 @@
>                  historyItem.put(URLColumns.FAVICON, favicon);
>  
>              historyItem.put(Combined.BOOKMARK_ID, bookmarkId);
>              historyItem.put(Combined.HISTORY_ID, historyId);
> +            historyItem.put(Combined.DISPLAY, display);

Sigh, you'll need to add back that change to declare this |display| local variable.
Attachment #629382 - Flags: review?(margaret.leibovic) → review+
Rebased.
Attachment #629380 - Attachment is obsolete: true
Attachment #629381 - Attachment is obsolete: true
Attachment #630624 - Flags: review+
Rebased.
Attachment #629382 - Attachment is obsolete: true
Attachment #630625 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e5ca73e593b
https://hg.mozilla.org/mozilla-central/rev/d1b6fee75013
https://hg.mozilla.org/mozilla-central/rev/cdc5af7eb6bf

(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Depends on: 763184
Flags: in-testsuite-
Keywords: checkin-needed
No longer depends on: 763184
(In reply to Aaron Train [:aaronmt] from comment #13)
> Should Aurora get the new ic_awesomebar_star.png drawable?
> 
> http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/resources/
> drawable/ic_awesomebar_star.png

My impression is that the tablet and corresponding Awesome screen changes assume the new icon is in place. So I guess the icon should be uplifted to aurora (if it's not there yet). I changed the icon in a separate patch with that in mind:

https://hg.mozilla.org/mozilla-central/rev/d1b6fee75013
Comment on attachment 630624 [details] [diff] [review]
Update bookmark star icon for awesomebar items

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: We will show golden stars in awesomescren.
Testing completed (on m-c, etc.): Landed in m-c on 06/08
Risk to taking this patch (and alternatives if risky): None. Just image replacements.
String or UUID changes made by this patch: None.
Attachment #630624 - Flags: approval-mozilla-beta?
Attachment #630624 - Flags: approval-mozilla-aurora?
Comment on attachment 630624 [details] [diff] [review]
Update bookmark star icon for awesomebar items

[Triage Comment]
Reading mode isn't in 15 - only 16.
Attachment #630624 - Flags: approval-mozilla-beta?
Attachment #630624 - Flags: approval-mozilla-beta-
Attachment #630624 - Flags: approval-mozilla-aurora?
Attachment #630624 - Flags: approval-mozilla-aurora+
This doesn't appear to have landed on FF16 yet. Anything holding us up?
Comment on attachment 630623 [details] [diff] [review]
Rename starView to bookmarkIconView in AwesomeBarTabs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

[Approval Request Comment]
User impact if declined: Users might get confused of why certain pages in awesomescreen go straight to reader mode. We need show clear indication when a page is in reading list.
Testing completed (on m-c, etc.): It's been a long time in m-c, no problems.
Risk to taking this patch (and alternatives if risky): Low, well tested features for a long time in nightly.
String or UUID changes made by this patch: none.
Attachment #630623 - Flags: approval-mozilla-beta?
Comment on attachment 630625 [details] [diff] [review]
Show reading list icon on awesomebar search

[Approval Request Comment]
User impact if declined: Users might get confused of why certain pages in awesomescreen go straight to reader mode. We need show clear indication when a page is in reading list.
Testing completed (on m-c, etc.): It's been a long time in m-c, no problems.
Risk to taking this patch (and alternatives if risky): Low, well tested features for a long time in nightly.
String or UUID changes made by this patch: none.
Attachment #630625 - Flags: approval-mozilla-beta?
(In reply to Alex Keybl [:akeybl] from comment #17)
> This doesn't appear to have landed on FF16 yet. Anything holding us up?

Missed this somehow. Requested approval for Beta now.
Comment on attachment 630623 [details] [diff] [review]
Rename starView to bookmarkIconView in AwesomeBarTabs

Actually, this patch indirectly landed in Beta as part of bug 759041.
Attachment #630623 - Flags: approval-mozilla-beta?
Comment on attachment 630625 [details] [diff] [review]
Show reading list icon on awesomebar search

Actually, this patch indirectly landed in Beta as part of bug 759041.
Attachment #630625 - Flags: approval-mozilla-beta?
The Reader icon is displayed in Awesomescreen as expected on the latest Nightly, Aurora and Beta builds. Closing bug as verified fixed.

--
Device: Galaxy Note
OS: Android 4.0.4
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.