Closed Bug 887982 Opened 12 years ago Closed 12 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.
Assignee: nobody → sriram
Whiteboard: fixed-fig
Status: NEW → RESOLVED
Closed: 12 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.

Attachment

General

Created:
Updated:
Size: