701330 - Show star on urls that are bookmarks in AwesomeBar screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Lucas Rocha (:lucasr)]

See specs. Maybe needs asset.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Update https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=35321&edit=1 when this bug is resolved.

Comment 3

[Ian Barlow (:ibarlow)]

Soft blocker nom?

A bookmark indicator would certainly make the awesomescreen a little more useful, and consistent with the desktop awesomebar (and xul fennec). I can provide assets / designs right away if we mark this as a blocker

Comment 4

[Lucas Rocha (:lucasr)]

FYI: I have a patch ready to submit. Just need assets and specs from Ian.

Comment 5

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> FYI: I have a patch ready to submit. Just need assets and specs from Ian.

If you don't get assets soon, would you mind posting a WIP? I imagine you're adding something to the query results to let us know if an item is a bookmark or not, and similar logic is also needed for bug 736272, so I can probably work on top of your patch here.

Comment 6

[Ian Barlow (:ibarlow)]

Attachment: Mockup of bookmark star location

As an aside, also included here is how we want to treat open local and remote tabs in the awesomebar results.

Assets are coming right up.

Comment 7

[Ian Barlow (:ibarlow)]

Attachment: graphic assets

Good to go!

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #6)
> Created attachment 614632 [details]
> Mockup of bookmark star location
> 
> As an aside, also included here is how we want to treat open local and
> remote tabs in the awesomebar results.
> 
> Assets are coming right up.

Thanks for assets, Ian. The design for the bookmark star has a caveat though: we don't show favicons for pages with no favicon. We used to show a default favicon but we don't do it anymore. So, if we have a bookmark without a favicon (which is not that rare) we're show the star on top of whitespace. Showing a default favicon just to show the star on top is not ideal I guess.

My suggestion is to move the star to the right side of the row. Maybe somewhere on the top-right corner? Let me know how you prefer to do it.

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #5)
> (In reply to Lucas Rocha (:lucasr) from comment #4)
> > FYI: I have a patch ready to submit. Just need assets and specs from Ian.
> 
> If you don't get assets soon, would you mind posting a WIP? I imagine you're
> adding something to the query results to let us know if an item is a
> bookmark or not, and similar logic is also needed for bug 736272, so I can
> probably work on top of your patch here.

I'm simply checking the value of bookmark_id. If it's greater than 0, the url is a bookmark. You can probably apply the same logic on bug 736272.

Comment 10

[Ian Barlow (:ibarlow)]

Lucas, moving the star to the top right is fine for now. In the longer intend to rework the look of empty favicons, but for now we can just move the star.

You can give it the same top and outside margin as the favicons have.

Comment 11

[Ian Barlow (:ibarlow)]

sorry, that was meant to say "In the longer term, I intend to rework..."

Comment 12

[Ian Barlow (:ibarlow)]

Attachment: Mockup of Bookmark on the right, for reference

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: (1/2) Show star on "Top Sites" tab rows that are bookmarks

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: (2/2) Show star on "History" tab rows that are bookmarks

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Screenshot

Ian, here's the screenshot for UI validation.

Comment 16

[Ian Barlow (:ibarlow)]

Looks great!

Comment 17

[:Margaret Leibovic]

Comment on attachment 616562 [details] [diff] [review]
(1/2) Show star on "Top Sites" tab rows that are bookmarks

This looks good. I have one concern about the bookmark id value, but that's just more about understanding why this works.

>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java
>@@ -943,16 +948,24 @@ public class AwesomeBarTabs extends TabH

>+    private void updateBookmarkStar(ImageView starView, Cursor cursor) {
>+        int bookmarkIdIndex = cursor.getColumnIndexOrThrow(Combined.BOOKMARK_ID);
>+        long id = cursor.getLong(bookmarkIdIndex);
>+
>+        int visibility = (id == 0 ? View.GONE : View.VISIBLE);

Following what I talked about in bug 736272 comment 7, I wonder why cursor.getLong(bookmarkIdIndex) is returning 0 for non-bookmarks. It seems this is probably just what the getLong implementation does for null values. The docs say this behavior is "implementation-defined": http://developer.android.com/reference/android/database/Cursor.html#getLong%28int%29. Is it possible for us to get back different Cursor implementations here? We should just make sure we are always going to get 0 like we're expecting.

There will never be an entry with bookmark_id = 0 because that is our fixed bookmarks root, so this check shouldn't cause problems, but I think you should add a comment about why we're checking for 0.

Comment 18

[:Margaret Leibovic]

Comment on attachment 616563 [details] [diff] [review]
(2/2) Show star on "History" tab rows that are bookmarks

This also looks good, with the same comment I had about the last patch. OOC, do we want to consider showing stars in the bookmarks tab as well? I know it's not that useful because they will all have stars, but it will be consistent with the other tabs.

>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java
>@@ -170,16 +171,20 @@ public class AwesomeBarTabs extends TabH

>+            Long bookmarkId = (Long) historyItem.get(Combined.BOOKMARK_ID);
>+            int visibility = (bookmarkId == 0 ? View.GONE : View.VISIBLE);
>+            viewHolder.starView.setVisibility(visibility);

My last comment also applies here.

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 616562 [details] [diff] [review]
(1/2) Show star on "Top Sites" tab rows that are bookmarks

[Approval Request Comment]

Soft blocker, fairly low risk. Nice UX improvement (you can see which entries from awesomebar are bookmarks).

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 616562 [details] [diff] [review]
(1/2) Show star on "Top Sites" tab rows that are bookmarks

Let me land this m-c before requesting aurora approval. Sorry for the spam.

Comment 21

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #18)
> Comment on attachment 616563 [details] [diff] [review]
> (2/2) Show star on "History" tab rows that are bookmarks
> 
> This also looks good, with the same comment I had about the last patch. OOC,
> do we want to consider showing stars in the bookmarks tab as well? I know
> it's not that useful because they will all have stars, but it will be
> consistent with the other tabs.

Good point. I'll check with Ian.

Comment 22

[Ian Barlow (:ibarlow)]

I think we can hide them in the Bookmarks tab. 

The stars are useful to differentiate bookmarked content from normal content, but it just adds visual clutter if everything has a star on it :)

Comment 23

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #17)
> Comment on attachment 616562 [details] [diff] [review]
> (1/2) Show star on "Top Sites" tab rows that are bookmarks
> 
> This looks good. I have one concern about the bookmark id value, but that's
> just more about understanding why this works.
> 
> >diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java
> >@@ -943,16 +948,24 @@ public class AwesomeBarTabs extends TabH
> 
> >+    private void updateBookmarkStar(ImageView starView, Cursor cursor) {
> >+        int bookmarkIdIndex = cursor.getColumnIndexOrThrow(Combined.BOOKMARK_ID);
> >+        long id = cursor.getLong(bookmarkIdIndex);
> >+
> >+        int visibility = (id == 0 ? View.GONE : View.VISIBLE);
> 
> Following what I talked about in bug 736272 comment 7, I wonder why
> cursor.getLong(bookmarkIdIndex) is returning 0 for non-bookmarks. It seems
> this is probably just what the getLong implementation does for null values.
> The docs say this behavior is "implementation-defined":
> http://developer.android.com/reference/android/database/Cursor.
> html#getLong%28int%29. Is it possible for us to get back different Cursor
> implementations here? We should just make sure we are always going to get 0
> like we're expecting.

Yeah, I actually expected it to throw if the value is null on database (this is what the docs say). But it's actually returning 0.

> There will never be an entry with bookmark_id = 0 because that is our fixed
> bookmarks root, so this check shouldn't cause problems, but I think you
> should add a comment about why we're checking for 0.

Ok, added a comment.

Comment 24

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ecb25f038e12
http://hg.mozilla.org/integration/mozilla-inbound/rev/e36768ec309b

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 616562 [details] [diff] [review]
(1/2) Show star on "Top Sites" tab rows that are bookmarks

Mobile-only, soft blocker. Important UX improvement: makes it visually clear which entries in AwesomeBar are bookmarks. Low risk patch.

Comment 26

[Lucas Rocha (:lucasr)]

Comment on attachment 616563 [details] [diff] [review]
(2/2) Show star on "History" tab rows that are bookmarks

Mobile-only, soft blocker. Important UX improvement: makes it visually clear which entries in AwesomeBar are bookmarks. Low risk patch.

Comment 27

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ecb25f038e12

Comment 28

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/e36768ec309b

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 616562 [details] [diff] [review]
(1/2) Show star on "Top Sites" tab rows that are bookmarks

Given that this feature will still involved some follow up fixes (bug 748583 and bug 748765), I'd like to cancel the aurora request. This is not beta or 1.0 material.

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 616563 [details] [diff] [review]
(2/2) Show star on "History" tab rows that are bookmarks

Given that this feature will still involved some follow up fixes (bug 748583 and bug 748765), I'd like to cancel the aurora request. This is not beta or 1.0 material.

Comment 31

[Cristian Nicolae (:xti)]

Stars are displayed both in Top Sites and History for visited bookmarked pages as it's expected.

Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-28)
Device: Galaxy Nexus
OS: Android 4.0.2

Comment 32

[Adrian Tamas (:AdrianT)]

A new test case was created to cover the feature in the BFTs run in the Bookmarks testsuite: https://moztrap.mozilla.org/manage/cases/_detail/6346/