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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

For bookmarks we show a star, for reading list I guess we'd show a little book icon?

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Rename starView to bookmarkIconView in AwesomeBarTabs

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Show reading list icon on awesomebar search

These patches are pretty much ready. Just need the real assets for the reader icon.

Comment 3

[Ian Barlow (:ibarlow)]

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

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Rename starView to bookmarkIconView in AwesomeBarTabs

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Update bookmark star icon for awesomebar items

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Show reading list icon on awesomebar search

Comment 7

[:Margaret Leibovic]

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.

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Rename starView to bookmarkIconView in AwesomeBarTabs

Rebased.

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Update bookmark star icon for awesomebar items

Rebased.

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: Show reading list icon on awesomebar search

Rebased.

Comment 11

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2e5ca73e593b
http://hg.mozilla.org/integration/mozilla-inbound/rev/d1b6fee75013
http://hg.mozilla.org/integration/mozilla-inbound/rev/cdc5af7eb6bf

Comment 12

[Graeme McCutcheon [:graememcc]]

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)

Comment 13

[Aaron Train [:aaronmt]]

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

Comment 14

[Lucas Rocha (:lucasr)]

(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 15

[Sriram Ramasubramanian [:sriram]]

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.

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 630624 [details] [diff] [review]
Update bookmark star icon for awesomebar items

[Triage Comment]
Reading mode isn't in 15 - only 16.

Comment 17

[Alex Keybl [:akeybl]]

This doesn't appear to have landed on FF16 yet. Anything holding us up?

Comment 18

[Lucas Rocha (:lucasr)]

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.

Comment 19

[Lucas Rocha (:lucasr)]

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.

Comment 20

[Lucas Rocha (:lucasr)]

(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 21

[Lucas Rocha (:lucasr)]

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.

Comment 22

[Lucas Rocha (:lucasr)]

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.

Comment 23

[Cristian Nicolae (:xti)]

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