715274 - Update styling of History and bookmark folder subheadings for pre-Honeycomb

Status: VERIFIED FIXED

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

Description

[Ian Barlow (:ibarlow)]

Let's tweak the styling of the History subheadings (Today, Yesterday, etc) to look more consistent with rest of UI

Mocks and specs to follow soon.

Comment 1

[Peter Retzer (:pretzer)]

Attachment: Screenshot on Gingerbread

Since bug 732104 landed the same styling is also used for the new bookmark folder header. Based on the screenshot on the other bug the styling seems to look good for Honeycomb and ICS. For Gingerbread and below this really looks bad though (see screenshot attached here).

Comment 2

[Peter Retzer (:pretzer)]

Ian, do you have specs for this?

Comment 3

[Ian Barlow (:ibarlow)]

Attachment: mockup of tweaks

Changes:
* Make background white #FFFFFF
* Text colour should be #222222
* Header divider should be 3DIP, other row dividers should be 1DIP

Comment 4

[Peter Retzer (:pretzer)]

Thanks for the quick mockup. Just one question:
Wouldn't that be too subtle for the history tab? IMO the sub-headers ('Today', etc.) will hardly be noticeable when they are not at the very top but somewhere between the history items.

Comment 5

[Ian Barlow (:ibarlow)]

Pretzer, we haven't seen any issue on newer, ICS based phones where the headings are white, too. The indenting of the text over the favicons is generally enough to provide adequate visual separation.

Comment 6

[Peter Retzer (:pretzer)]

Just from looking at the mockup I somehow didn't realize that they have different indentation, but that's clear now. Thanks for the clarification!

Comment 7

[:Margaret Leibovic]

Attachment: WIP

Trying to follow our new color separation pattern!

Comment 8

[:Margaret Leibovic]

Attachment: screenshot of patch (gingerbread)

Comment 9

[:Margaret Leibovic]

Comment on attachment 607379 [details] [diff] [review]
WIP

Actually, this breaks the border underneath the subheader on ICS... I'm going to have to work on this more.

Comment 10

[:Margaret Leibovic]

Attachment: patch

This patch creates a different layout for pre-honeycomb. This does what we want, but I'm not sure if there's a cost associated with creating more layout files like this. Asking Sriram for review, since it seems like he touches this stuff more than anyone else :)

Comment 11

[:Margaret Leibovic]

Comment on attachment 607385 [details] [diff] [review]
patch

Clearing review. Sriram and I talked about this yesterday, and I'm going to take a different approach using styles.xml.

Comment 12

[Sriram Ramasubramanian [:sriram]]

What's the color for the divider?

Comment 13

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch uses the colors as specified.

Comment 14

[:Margaret Leibovic]

Comment on attachment 608798 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java

>             protected void onPostExecute(Cursor cursor) {
>                 mRefreshTask = null;
>                 mBookmarksAdapter.changeCursor(cursor);
>+ 
>+                ListView bookmarksList = (ListView) findViewById(R.id.bookmarks_list);
> 
>                 // Hide the header text if we're at the root folder
>                 if (mFolderId == Bookmarks.FIXED_ROOT_ID) {
>-                    mBookmarksTitleView.setVisibility(View.GONE);
>+                    if (bookmarksList.getHeaderViewsCount() == 1)
>+                        bookmarksList.removeHeaderView(mBookmarksTitleView);
>                 } else {
>-                    mBookmarksTitleView.setText(mFolderTitle);
>-                    mBookmarksTitleView.setVisibility(View.VISIBLE);
>+                    if (bookmarksList.getHeaderViewsCount() == 0)
>+                        bookmarksList.addHeaderView(mBookmarksTitleView, null, true);
>+
>+                    ((TextView) mBookmarksTitleView.findViewById(R.id.title)).setText(mFolderTitle);

If you're going to just add/remove the header views, you don't need to set the visibility on them, and you could just always keep them as visible, right? And then you can also probably get rid of the LinearLayout that's wrapped around them, since that was just added to help collapse them.

>             // We need to add the header before we set the adapter
>             LayoutInflater inflater =
>                     (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
>-            View headerView = inflater.inflate(R.layout.awesomebar_folder_header_row, null);
>-
>-            // Hide the header title view to begin with
>-            TextView titleView = (TextView) headerView.findViewById(R.id.title);
>-            titleView.setVisibility(View.GONE);
>-            mBookmarksAdapter.setBookmarksTitleView(titleView);
>-
>-            bookmarksList.addHeaderView(headerView, null, true);
>+            LinearLayout headerView = (LinearLayout) inflater.inflate(R.layout.awesomebar_header_row, null);
>+            mBookmarksAdapter.setBookmarksTitleView(headerView);

Following the changes to add/remove the header in mBookmarksAdapter, you could also inflate the view the first time you need to add it, since we don't want to show it in this root view anyway. Also, BookmarksListAdapter already caches the inflater. Then you could get rid of setBookmarksTitleView all together.

>diff --git a/mobile/android/base/resources/layout/awesomebar_header_row.xml b/mobile/android/base/resources/layout/awesomebar_header_row.xml

>+    <TextView android:id="@+id/title"
>+              android:layout_width="fill_parent"
>+              android:layout_height="wrap_content"
>+              android:background="#FFFFFF"
>+              android:textColor="#222222"

We probably want to define these colors in colors.xml, right?

>diff --git a/mobile/android/base/resources/layout/awesomebar_tabs.xml b/mobile/android/base/resources/layout/awesomebar_tabs.xml

>             <ExpandableListView android:id="@+id/history_list"
>                                 style="@style/AwesomeBarList"
>+                                android:childDivider="#E5E5E5"

Same thing for this color.


>diff --git a/mobile/android/base/resources/values-v11/styles.xml b/mobile/android/base/resources/values-v11/styles.xml

>     <!-- Lists in AwesomeBar -->
>     <style name="AwesomeBarList" parent="android:style/Widget.Holo.ListView">
>         <item name="android:layout_width">fill_parent</item>
>         <item name="android:layout_height">fill_parent</item>
>         <item name="android:layout_weight">1</item>
>+        <item name="android:divider">#E5E5E5</item>

And here.

>diff --git a/mobile/android/base/resources/values/styles.xml b/mobile/android/base/resources/values/styles.xml

>     <!-- Lists in AwesomeBar -->
>     <style name="AwesomeBarList" parent="android:style/Widget.ListView.White">
>         <item name="android:layout_width">fill_parent</item>
>         <item name="android:layout_height">fill_parent</item>
>         <item name="android:layout_weight">1</item>
>+        <item name="android:divider">#E5E5E5</item>

And here.

Comment 15

[Sriram Ramasubramanian [:sriram]]

I didn't take a close look into the inflater and the "need for linearLayout". I just reused the header view used in Expandable List View. And... LinearLayout "is needed", because we need a "3dp" divider for header -- where 2dp comes from LinearLayout's background.

As much as I agree with you on moving #E5E5E5 to colors.xml, I don't want to do it now. This is going to change next week with new tablet UI. I checked with Ian on whether it will be a 1dp or 2dp drawable, and he is yet to decide. Hence I thought of having the color values, so that we can change it based on need -- instead of adding it in colors.xml and failing to remove it -- like many color resources sticking out there.

  <color name="splash_background">#000000</color>
  <color name="splash_msgfont">#ffffff</color>
  <color name="splash_urlfont">#000000</color>
  <color name="splash_content">#ffffff</color>

I don't find any of them being used today.

I'll post a patch with cleaning up inflation, based on need.

Comment 16

[:Margaret Leibovic]

Oh, also, if you do this dynamic add/remove, you'll have to update the logic in handleBookmarkItemClick, since it assumes a header row exists:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBarTabs.java#873

Same thing with testBookmark:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testBookmark.java.in#33

Maybe we'd be better off splitting that dynamic add/remove to a different bug, since it seems to impact a bunch of things.

Comment 17

[Sriram Ramasubramanian [:sriram]]

>diff --git a/mobile/android/base/resources/layout/awesomebar_header_row.xml b/mobile/android/base/resources/layout/awesomebar_header_row.xml

>+    <TextView android:id="@+id/title"
>+              android:layout_width="fill_parent"
>+              android:layout_height="wrap_content"
>+              android:background="#FFFFFF"
>+              android:textColor="#222222"

We probably want to define these colors in colors.xml, right?

It's an overkill it to move it to colors.xml and then reference here when you won't "reuse it anywhere else".

Also, the background is going to change to textured blue very soon, and I don't feel like moving anything to colors.xml as of now.

Comment 18

[Sriram Ramasubramanian [:sriram]]

(In reply to Margaret Leibovic [:margaret] from comment #16)
> Oh, also, if you do this dynamic add/remove, you'll have to update the logic
> in handleBookmarkItemClick, since it assumes a header row exists:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> AwesomeBarTabs.java#873
> 
> Same thing with testBookmark:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/
> testBookmark.java.in#33
> 
> Maybe we'd be better off splitting that dynamic add/remove to a different
> bug, since it seems to impact a bunch of things.

How did things work when you were "hiding" the header? Ideally that code should have been taken | list.getHeaderViewsCount() | into consideration.

And... Why are handle click events not within the adapter but somewhere else?

https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/TabsTray.java#l218 <-- Everything is better self contained like this.

Comment 19

[Sriram Ramasubramanian [:sriram]]

Attachment: Patc

This patch fixes the header part.

Comment 20

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

That! finally works fine! :O :O :O
We need to be adding and removing headers here (as 2dp border below the title comes from the background LinearLayout.. and hiding the headerView still shows empty space). To add/remove headers, the adapter should be made null, headers should be add/removed and then reverted back to old adapter (this time cursor changed). This works perfectly fine with/without sync/root folders.

Comment 21

[:Margaret Leibovic]

Comment on attachment 608914 [details] [diff] [review]
Patch

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

This looks good. I just want to see if we can improve these two bits:

::: mobile/android/base/AwesomeBarTabs.java
@@ +329,5 @@
>                  return BrowserDB.getBookmarksInFolder(mContentResolver, mFolderId);
>              }
>  
>              protected void onPostExecute(Cursor cursor) {
> +                ListView list = (ListView) findViewById(R.id.bookmarks_list);

Cache this ListView? Even better, you could create a setBookmarksList(...) method and call it to cahce bookmarksList from BookmarksQueryTask.onPostExecute(...).

@@ +392,5 @@
> +            if (mBookmarksAdapter.getBookmarksTitleView() == null) {
> +                // We need to add the header before we set the adapter
> +                LinearLayout headerView = (LinearLayout) LayoutInflater.from(mContext).inflate(R.layout.awesomebar_header_row, null);
> +                mBookmarksAdapter.setBookmarksTitleView(headerView);
> +            }

Since you got the dynamic addHeaderView/removeHeaderView to work (nice!), I think we should just inflate this view in BookmarksListAdapter, especially since we're not doing anything with it in here anymore.

This would allow you to get rid of getBookmarksTitleView()/setBookmarksTitleView().

Comment 22

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Cleanup

This is the proposed cleanup part. I've removed "Refresh..QueryTask" as it was redundant. Everything is now handled by BookmarksQueryTask itself. I've tested it with/without sync and multiple levels of folders. Everything works fine. This should be applied on top of previous patch.

Comment 23

[:Margaret Leibovic]

Also, as I mentioned in comment 16, you'll need to update testBookmark to deal with the fact that there won't be a header view in the root bookmark list anymore (this should be pretty straightforward, since we don't actually have any tests right now for going into/out of folders).

Comment 24

[Sriram Ramasubramanian [:sriram]]

(In reply to Margaret Leibovic [:margaret] from comment #21)
> Comment on attachment 608914 [details] [diff] [review]
> Patch
> 
> Review of attachment 608914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. I just want to see if we can improve these two bits:
> 
> ::: mobile/android/base/AwesomeBarTabs.java
> @@ +329,5 @@
> >                  return BrowserDB.getBookmarksInFolder(mContentResolver, mFolderId);
> >              }
> >  
> >              protected void onPostExecute(Cursor cursor) {
> > +                ListView list = (ListView) findViewById(R.id.bookmarks_list);
> 
> Cache this ListView? Even better, you could create a setBookmarksList(...)
> method and call it to cahce bookmarksList from
> BookmarksQueryTask.onPostExecute(...).

Who should cache the ListView? Adapter is bound to a ListView and not the other way round. If you want to cache the list view, it should be done at Activity level (which is my next proposal :D to move 3 all lists out into separate files.. so that they are easy to manage).

> 
> @@ +392,5 @@
> > +            if (mBookmarksAdapter.getBookmarksTitleView() == null) {
> > +                // We need to add the header before we set the adapter
> > +                LinearLayout headerView = (LinearLayout) LayoutInflater.from(mContext).inflate(R.layout.awesomebar_header_row, null);
> > +                mBookmarksAdapter.setBookmarksTitleView(headerView);
> > +            }
> 
> Since you got the dynamic addHeaderView/removeHeaderView to work (nice!), I
> think we should just inflate this view in BookmarksListAdapter, especially
> since we're not doing anything with it in here anymore.
> 
> This would allow you to get rid of
> getBookmarksTitleView()/setBookmarksTitleView().

The header/footer for a list should be added "before" setting an adapter. Hence, we cannot move this code into that. Since we don't have a "custom" list view, I am just caching there. Ideally, we should be caching at Activity level. But, that gets too cumbersome with so many lists in that file. When we move the lists out, we can cache it somewhere else.
So, this cannot be inflated from an adapter, but has to be done outside of it.

Comment 25

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #22)
> Created attachment 608920 [details] [diff] [review]
> Patch: Cleanup
> 
> This is the proposed cleanup part. I've removed "Refresh..QueryTask" as it
> was redundant. Everything is now handled by BookmarksQueryTask itself. I've
> tested it with/without sync and multiple levels of folders. Everything works
> fine. This should be applied on top of previous patch.

Would you mind putting this patch in a separate bug? There are a decent number of changes, and they're not related to updating the styling of the header, so I think they belong in their own bug. It's looking good, but I want to take a closer look to make sure we aren't overlooking anything.

Comment 26

[:Margaret Leibovic]

Attachment: test fixes

Wes, in Sriram's patch, we're now removing the header instead of hiding it, so testBookmark doesn't need to account for it anymore. I think this patch takes care of it, but I haven't had the chance to build with Sriram's patch yet (Sriram, maybe you want to run the test with this patch applied to make sure it still passes).

Comment 27

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #24)
> (In reply to Margaret Leibovic [:margaret] from comment #21)
> > Comment on attachment 608914 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 608914 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good. I just want to see if we can improve these two bits:
> > 
> > ::: mobile/android/base/AwesomeBarTabs.java
> > @@ +329,5 @@
> > >                  return BrowserDB.getBookmarksInFolder(mContentResolver, mFolderId);
> > >              }
> > >  
> > >              protected void onPostExecute(Cursor cursor) {
> > > +                ListView list = (ListView) findViewById(R.id.bookmarks_list);
> > 
> > Cache this ListView? Even better, you could create a setBookmarksList(...)
> > method and call it to cahce bookmarksList from
> > BookmarksQueryTask.onPostExecute(...).
> 
> Who should cache the ListView? Adapter is bound to a ListView and not the
> other way round.

But that ListView shouldn't be changing over the life of the adapter, right? When the activity dies, the adapter dies as well. This doesn't matter that much, though, if you don't want to do it.

> > 
> > @@ +392,5 @@
> > > +            if (mBookmarksAdapter.getBookmarksTitleView() == null) {
> > > +                // We need to add the header before we set the adapter
> > > +                LinearLayout headerView = (LinearLayout) LayoutInflater.from(mContext).inflate(R.layout.awesomebar_header_row, null);
> > > +                mBookmarksAdapter.setBookmarksTitleView(headerView);
> > > +            }
> > 
> > Since you got the dynamic addHeaderView/removeHeaderView to work (nice!), I
> > think we should just inflate this view in BookmarksListAdapter, especially
> > since we're not doing anything with it in here anymore.
> > 
> > This would allow you to get rid of
> > getBookmarksTitleView()/setBookmarksTitleView().
> 
> The header/footer for a list should be added "before" setting an adapter.
> Hence, we cannot move this code into that. Since we don't have a "custom"
> list view, I am just caching there.

My point was that you _are_ adding the header before setting the adapter when you do it in RefreshCursorAdapter. You're not actually adding the header here (which is correct, since we don't want a header in the initial root list view), so I was thinking we don't need to be inflating the layout until it's actually time to add the header. 

> Ideally, we should be caching at
> Activity level. But, that gets too cumbersome with so many lists in that
> file. When we move the lists out, we can cache it somewhere else.

We only ever create one BookmarksAdapter, so I feel that caching these there is fine, since that still means we're only going to inflate the views once.

> So, this cannot be inflated from an adapter, but has to be done outside of
> it.

Why is that? We're currently inflating other views inside the adapter.

Comment 28

[Sriram Ramasubramanian [:sriram]]

> But that ListView shouldn't be changing over the life of the adapter, right?
> When the activity dies, the adapter dies as well. This doesn't matter that
> much, though, if you don't want to do it.

I don't know. I don't like caching ListView inside an adapter. Adapter is a "resuable" component. We shouldn't be associating it to a particular list. Probably we can have a variable mListView, and we can always get it as | this.for() | which return the ListView to operate on. If that's what you would want, I can do that. (though, that's not how the mental model behind the adapters and listviews work). Ideally ListViews should be cached at activity level, as BookmarksQueryTask is the one that is in need of the listView.

> 
> We only ever create one BookmarksAdapter, so I feel that caching these there
> is fine, since that still means we're only going to inflate the views once.

We can do it in BookmarksAdapter's constructor. Again, that's not the way how adapter works. Adapter just stores the data and gives the views for the list. It doesn't care about the headers or the footers (and it shouldn't). Putting the header inside BookmarksAdapter feels conceptually wrong to me. Adapter is something that binds the data to the view and doesn't need to know about header/footer. Later when we move out to a custom listView, we can always have the header with the listview.

> 
> > So, this cannot be inflated from an adapter, but has to be done outside of
> > it.
> 
> Why is that? We're currently inflating other views inside the adapter.

getView() returns a view for the data held by the adapter. It's doesnt know anything about header or footer.

I am happy to move the list out to create a cleaner logical block. But since this is not the time to take risk in development cycle, I'll do it later. When that is done, we can move the headerView to the list, and leave the adapter sane.

Comment 29

[Sriram Ramasubramanian [:sriram]]

More cleaner way of doing this would be:

AwesomeBarTabs.java:
* should hold the listviews in private variables.
* should have 3-4 AsyncTask taking care of getting the data and inform the list to change their adapters.

some-custom-ListView.java:
* should hold the headerview (if needed)
* have its own adapter that operates on the data, which doesn't care about AsyncTask's at all.

Comment 30

[:Margaret Leibovic]

Comment on attachment 608914 [details] [diff] [review]
Patch

Okay, you've convinced me that this patch it the way to go :) Just one small last comment:

>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java

>-            bookmarksList.addHeaderView(headerView, null, true);
>+            if (mBookmarksAdapter.getBookmarksTitleView() == null) {
>+                // We need to add the header before we set the adapter
>+                LinearLayout headerView = (LinearLayout) LayoutInflater.from(mContext).inflate(R.layout.awesomebar_header_row, null);
>+                mBookmarksAdapter.setBookmarksTitleView(headerView);

You should update this comment, since we're not adding the header in here anymore.

Comment 31

[:Margaret Leibovic]

Comment on attachment 608920 [details] [diff] [review]
Patch: Cleanup

Clearing review here, since this got moved to bug 739334.

Comment 32

[Sriram Ramasubramanian [:sriram]]

(In reply to Margaret Leibovic [:margaret] from comment #30)
> Comment on attachment 608914 [details] [diff] [review]
> Patch
> 
> Okay, you've convinced me that this patch it the way to go :) Just one small
> last comment:
> 
> >diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java
> 
> >-            bookmarksList.addHeaderView(headerView, null, true);
> >+            if (mBookmarksAdapter.getBookmarksTitleView() == null) {
> >+                // We need to add the header before we set the adapter
> >+                LinearLayout headerView = (LinearLayout) LayoutInflater.from(mContext).inflate(R.layout.awesomebar_header_row, null);
> >+                mBookmarksAdapter.setBookmarksTitleView(headerView);
> 
> You should update this comment, since we're not adding the header in here
> anymore.

I felt that. :) Will change it :)

Comment 33

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #26)
> Created attachment 609328 [details] [diff] [review]
> test fixes
> 
> Wes, in Sriram's patch, we're now removing the header instead of hiding it,
> so testBookmark doesn't need to account for it anymore. I think this patch
> takes care of it, but I haven't had the chance to build with Sriram's patch
> yet (Sriram, maybe you want to run the test with this patch applied to make
> sure it still passes).

I ran the updated test with the patch applied, and it passes.

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 608914 [details] [diff] [review]
Patch

Looks good. I agree with making the Adapters less coupled.

Comment 35

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b179fc3dc5d5

Comment 36

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bfa75f04b29e

Comment 37

[Ed Morley [:emorley]]

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

Comment 38

[Ed Morley [:emorley]]

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

Comment 39

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c730b2ecebec

Comment 40

[Adrian Tamas (:AdrianT)]

Verified fixed on:
Build: Nightly 15.0a1 2012-05-22/Aurora 14.0a2 2012-05-21
Device: HTC Desire
OS: Android 2.2.2

The labels and other UI tweaks are implemented.