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)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: sriram, Assigned: sriram)
References
Details
(Whiteboard: fixed-fig)
Attachments
(1 file, 1 obsolete file)
13.25 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
BookmarksListView should use cursor loaders.
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
Minor correction to avoid NPE while loading a page.
Attachment #768583 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•12 years ago
|
Attachment #768575 -
Attachment is obsolete: true
Attachment #768575 -
Flags: review?(lucasr.at.mozilla)
Comment 3•12 years ago
|
||
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+
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
(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?
Assignee | ||
Comment 6•12 years ago
|
||
I'll post patches in bug 888039, once Bug 887985 is also reviewed? Let bug 888039 track this issue in one place.
Assignee | ||
Comment 7•12 years ago
|
||
Assignee: nobody → sriram
Whiteboard: fixed-fig
Comment 8•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•