887982 - BookmarksListView should use cursor loaders

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

BookmarksListView should use cursor loaders.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This adds the cursor loader logic to BookmarksPage (as the BookmarksListView has no access to LoaderManager). The AsyncTask code is moved into the loader.

Note: I've removed the cursor closing logic in onDetachedFromWindow() on the hopes that the loader will take care of closing (as it recommends swapCursor() and not changeCursor()). In case it won't I can add that piece of code back.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Minor correction to avoid NPE while loading a page.

Comment 3

[Lucas Rocha (:lucasr)]

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

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

Looks good (with suggested changes)

::: mobile/android/base/home/BookmarksListView.java
@@ +164,5 @@
>              }
>          }
>      }
>  
> +    public void refreshListWithCursor(Cursor cursor) {

I'd rename this to refreshFromCursor() for simplicity and consistency (with TwoLinePageRow).

::: mobile/android/base/home/BookmarksPage.java
@@ +28,5 @@
>  public class BookmarksPage extends HomeFragment {
>      public static final String LOGTAG = "GeckoBookmarksPage";
>  
> +    // Cursor loader ID for list of bookmarks.
> +    private static final int LOADER_ID_BOOKMARKS_LIST = 0;

BOOKMARKS_LIST_LOADER_ID for consistency with the other about:home pages.

@@ +153,5 @@
> +                    } else {
> +                        return new BookmarksLoader(getActivity(), args.getInt(BOOKMARKS_FOLDER_KEY));
> +                    }
> +                }
> +            }

nit: add empty line here.

Comment 4

[Sriram Ramasubramanian [:sriram]]

I would like to use support library's CursorAdapter instead of the default one due to bug 888039. There wasn't a need for this earlier as the code used changeCursor() method.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #4)
> I would like to use support library's CursorAdapter instead of the default
> one due to bug 888039. There wasn't a need for this earlier as the code used
> changeCursor() method.

File a follow-up bug?

Comment 6

[Sriram Ramasubramanian [:sriram]]

I'll post patches in bug 888039, once Bug 887985 is also reviewed? Let bug 888039 track this issue in one place.

Comment 7

[Sriram Ramasubramanian [:sriram]]

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

Comment 8

[Ed Morley [:emorley]]

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