Closed Bug 887982 Opened 9 years ago Closed 8 years ago

BookmarksListView should use cursor loaders

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: sriram, Assigned: sriram)

References

Details

(Whiteboard: fixed-fig)

Attachments

(1 file, 1 obsolete file)

BookmarksListView should use cursor loaders.
Blocks: 862796
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #768575 - Flags: review?(lucasr.at.mozilla)
Attached patch PatchSplinter Review
Minor correction to avoid NPE while loading a page.
Attachment #768583 - Flags: review?(lucasr.at.mozilla)
Attachment #768575 - Attachment is obsolete: true
Attachment #768575 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #768583 - Flags: review?(lucasr.at.mozilla) → review+
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.
(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?
I'll post patches in bug 888039, once Bug 887985 is also reviewed? Let bug 888039 track this issue in one place.
http://hg.mozilla.org/projects/fig/rev/b3b119e93c07
Assignee: nobody → sriram
Whiteboard: fixed-fig
https://hg.mozilla.org/mozilla-central/rev/b3b119e93c07
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.