885481 - Setting a new cursor everytime causes flickering of TopBookmarksView inside BookmarksListView

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

As a part of Bug 882365, the TopBookmarksView was added to BookmarksListView. Whenever the cursor on the BookmarksListView changes, the adapter had to be set to null and set back to a cursor based adapter -- to account for the header views. This causes flickering, as a "setAdapter(null)", removes all views inside the ListView and then re-adds them with the new cursor adapter.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Option 1: Wrapper adapter

(Taken from bug 882365 for reference).

This adds a HeaderViewListAdapter to start with. This way, changed the "wrapped adapter" on the fly wouldn't cause flickering issues. The header views will be shown still. However, the HeaderViewListAdapter wouldn't be aware of the header views added.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Option 2: Minor cleanup

This is option 2. This is same as previous approach except for setting the adapter again.

How this approach works?
1. Basically the GridView is added as a header even before setting any adapter. So, the ListView's mHeaderViewsInfo will have an entry for it.
2. Now by forcefully setting a HeaderViewListAdapter when this ListView attaches to the window, we make sure that we can add/remove headers on demand anytime after this.
This makes ListView set an adapter as:
mAdapter = new HeaderViewListAdapter( _the_grid_view_ , null, _this_forced_header_view_list_adapter_); [1]

So, the header view counts are good. The item counts are also good.

3. When the cursor is changed in the refreshListWithCursor(), we either add or remove the header view for the folder. This will not be reflected in the ListView's mAdapter (which is a new HeaderViewListAdapter). This will however be accounted for in its own mHeaderViewsInfo.

4. Now by calling setAdapter() inside refreshListWithCursor(),
a. The newly added/removed folder view (header) is taken care of, by wrapping itself in a new adapter internally. [1]
b. The mCursorAdapter will be the "wrapped adapter" now. This means, the empty wrapped cursor adapter we added in pt. 2 won't be used anymore.

[1] https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/ListView.java#L448

Now to the tricky part:
By doing a | mCursoAdapter.changeCursor(cursor); |. We already trigger a dataset change to the adapter, we starts the invalidation process. And by setting a new adapter, this is going to re-do everything again. This is something we cannot avoid.

Comment 3

[Sriram Ramasubramanian [:sriram]]

I tried the option of hiding the folder view. This doesn't work with listviews. It still tries to leave the space empty with a divider! :(

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> I tried the option of hiding the folder view. This doesn't work with
> listviews. It still tries to leave the space empty with a divider! :(

Maybe instead of having 2 header views, we could pack the grid and folder view into the same layout and then add this layout as a header view? That would also simplify the way we track header views I guess. Just thinking out loud here. I'd really like us to avoid working around HeaderViewListAdapter's (potentially unpredictable) behavior.

HeaderViewListAdapter shouldn't really be a public API in the platform. It's a ListView implementation detail that leaked out.

Comment 5

[Sriram Ramasubramanian [:sriram]]

I feel using HeaderViewListAdapter is not a problem. They haven't deprecated it since its introduction in API level 1. Also, the comment in their addFooterView() code says that there should have been a check but they missed adding one :P

Here's another approach.
class WrapperListAdapter extends HeaderViewListAdatper {
   private ArrayList<FixedViewInfo> mHeaderInfos;

   public void addHeaderViews(...);
}

This class will keep track of headers and allows us to "addHeaderViews()". Everything else will call super().

Also, when adding a header, our overridden "addHeaderView()" in ListView will ensure to add it to the adapter. These will ensure that the headers are tracked by the adapter, and the count stays right all the time.

The addition/removal of folder view (header) is always followed by a refresh on the cursor, which triggers refreshing the list. So, the data set change happens by default. <-- this is why we see the added/removed header instantly.

(But some magic is happening where the count is fine already with my first patch :P Some buggy ListView implementation :P ).

Comment 6

[Sriram Ramasubramanian [:sriram]]

My approach mentioned in Comment 5 won't work. Even though we can extend HeaderViewListAdapter and keep track of header views, we cannot get the FixedViewInfo that Android's ListView creates to add it to its own list. It's a private variable and we don't have any access to it. ListView depends on its own list rather than the HeaderViewListAdapter's list.

We cannot merge all the headers into one. The GridView may or may not be a header -- and its better to have it separate. The folder view may or may not be shown, depending on the which list it is showing. It's better to keep these independent.

I feel the only approach to solve this is use either of the two patches. I don't know how first one works, even though we don't reset the adapter. But the tap events are working fine as per the item. (Note: The internal HeaderViewListAdapter might not be reflecting folder view at all. But still it works). The second approach resets the adapter which keeps track of HeaderViewInfos properly.

In either ways, the only thing we enforce is: set a HeaderViewListAdapter to start with, so that Android wouldn't complain when we set a new adapter.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

(As discussed on IRC) This removes the need for a folder row as a header. Instead, it makes that a row in the ListView. The cursor is left untouched. Since getCount() returns the size with/without the folder row, the position is always taken after discounting for this.

Note: I want to cleanup refreshListWithCursor(). However, I don't like moving it to onPostExecute() on the BookmarkQueryTask. The code will read as:
mQueryTask = new BookmarkQueryTask() { ... mQueryTask = null; }. Somehow that doesn't feel like a good encapsulation. Probably this will go away with cursorloaders.

Comment 8

[Sriram Ramasubramanian [:sriram]]

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

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

::: mobile/android/base/home/BookmarksListView.java
@@ +326,5 @@
> +        }
> +
> +        @Override
> +        public int getCount() {
> +            return super.getCount() + (isShowingChildFolder() ? 0 : 1);

This should have been the other way round: "isShowingChildFolder() ? 1 : 0".
Taken care of in my local code.

Comment 9

[Sriram Ramasubramanian [:sriram]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #7) 
> Note: I want to cleanup refreshListWithCursor(). However, I don't like
> moving it to onPostExecute() on the BookmarkQueryTask. The code will read as:
> mQueryTask = new BookmarkQueryTask() { ... mQueryTask = null; }. Somehow
> that doesn't feel like a good encapsulation. Probably this will go away with
> cursorloaders.

This goes away with CursorLoaders.

Comment 10

[Lucas Rocha (:lucasr)]

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

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

Looks nice but the special handling for the top folder view inflation looks suspicious and the handling of position decrements needs to be better documented.

::: mobile/android/base/home/BookmarksListView.java
@@ +148,4 @@
>                  mCursorAdapter.moveToParentFolder();
>                  return;
> +            } else {
> +                position--;

This looks a bit cryptic. Maybe move this decrement call closer to the moveToPosition() and add a comment explaining the reason for it?

@@ +251,5 @@
> +            if (isShowingChildFolder()) {
> +                if (position == 0) {
> +                    return VIEW_TYPE_FOLDER;
> +                } else {
> +                    position--;

Same here. Move this decrement close to moveToPosition() to make it move prominent and add a comment explaning why this is necessary.

@@ +321,5 @@
> +        /**
> +         * @return true, if currently showing a child folder, false otherwise.
> +         */
> +        public boolean isShowingChildFolder() {
> +            return (mParentStack.peek().first != Bookmarks.FIXED_ROOT_ID);

Isn't it just about doing "mFolderId != Bookmarks.FIXED_ROOT_ID"?

@@ +335,5 @@
> +            // The position also reflects the opened child folder row.
> +            if (isShowingChildFolder()) {
> +                if (position == 0) {
> +                    BookmarkFolderView folder = (BookmarkFolderView) LayoutInflater.from(parent.getContext()).inflate(R.layout.bookmark_folder_row, null);
> +                    folder.setText(mParentStack.peek().second);

This shouldn't be necessary if you're inflating things correctly in newView(). Not sure I understand why you need to handle the top folderview differently here.

@@ +339,5 @@
> +                    folder.setText(mParentStack.peek().second);
> +                    folder.open();
> +                    return folder;
> +                } else {
> +                    position--;

Ditto.

Comment 11

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Moving the review to mfinkle as lucasr will be gone for few days. The comments from the previous review are addressed.

Comment 12

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> Comment on attachment 767976 [details] [diff] [review]
> Patch
> 
> Review of attachment 767976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks nice but the special handling for the top folder view inflation looks
> suspicious and the handling of position decrements needs to be better
> documented.
> 
> ::: mobile/android/base/home/BookmarksListView.java
> @@ +148,4 @@
> >                  mCursorAdapter.moveToParentFolder();
> >                  return;
> > +            } else {
> > +                position--;
> 
> This looks a bit cryptic. Maybe move this decrement call closer to the
> moveToPosition() and add a comment explaining the reason for it?

Added a comment "Accounting for the folder view". (And all other such position-- ).

> 
> @@ +321,5 @@
> > +        /**
> > +         * @return true, if currently showing a child folder, false otherwise.
> > +         */
> > +        public boolean isShowingChildFolder() {
> > +            return (mParentStack.peek().first != Bookmarks.FIXED_ROOT_ID);
> 
> Isn't it just about doing "mFolderId != Bookmarks.FIXED_ROOT_ID"?

A class level mFolderId was removed (as a part of cleanup). Hence taking it from mParentStack.

> 
> @@ +335,5 @@
> > +            // The position also reflects the opened child folder row.
> > +            if (isShowingChildFolder()) {
> > +                if (position == 0) {
> > +                    BookmarkFolderView folder = (BookmarkFolderView) LayoutInflater.from(parent.getContext()).inflate(R.layout.bookmark_folder_row, null);
> > +                    folder.setText(mParentStack.peek().second);
> 
> This shouldn't be necessary if you're inflating things correctly in
> newView(). Not sure I understand why you need to handle the top folderview
> differently here.

Ideally we don't need to override getView() of CursorAdapter. However, we are spoofing the count to be one more than the cursor (if we show a folder header). newView() and bindView() only take cursors. The default implementation of getView() knows about the change in the count and not about the fact that cursor doesn't have the folder view. Hence we are using the "position" in getView() to see if we want to show folder header (and show it), or use default getView() implementation after adjusting the position for the cursor.

> 
> @@ +339,5 @@
> > +                    folder.setText(mParentStack.peek().second);
> > +                    folder.open();
> > +                    return folder;
> > +                } else {
> > +                    position--;
> 
> Ditto.

Comment 13

[Brian Nicholson (:bnicholson)]

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

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

LGTM

Comment 14

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/db454c5472bf

Comment 15

[Ed Morley [:emorley]]

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