BookmarksPage fragment should handle the adapters for its views

RESOLVED FIXED in Firefox 26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 26
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Currently the views are holding the adapters that back them. Instead someone else (like a fragment) should be controlling these adapters. Given that the fragment is taking care of cursor loaders, it would be logical if the fragment is aware and takes care of the adapters as well.
(Assignee)

Comment 1

5 years ago
Created attachment 770464 [details] [diff] [review]
Part 1: BookmarksListAdapter

Moved BookmarksListAdapter to a separate file. BookmarksPage will hold and handle things for it. (aah.. sooo cleaaaan)
Attachment #770464 - Flags: review?(bnicholson)
(Assignee)

Comment 2

5 years ago
Created attachment 770467 [details] [diff] [review]
Part 2: TopBookmarksAdapter

Moved TopBookmarksAdapter to a separate file. BookmarksPage will take care of it. Additionally BookmarksPage will be responsible for triggerring LoadThumbnailsTask, as it holds the cursor and the adapter.
Attachment #770467 - Flags: review?(bnicholson)
(Assignee)

Comment 3

5 years ago
All cleanups done with Eclipse. So, no "unwanted imports" or "unused methods" anywhere. :D
Comment on attachment 770464 [details] [diff] [review]
Part 1: BookmarksListAdapter

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

::: mobile/android/base/home/BookmarksListAdapter.java
@@ +33,5 @@
> +    public static interface OnRefreshFolderListener {
> +
> +        // The folder id to refresh the list with.
> +        public void onRefreshFolder(int folderId);
> +

Nit: remove both newlines

@@ +95,5 @@
> +    }
> +
> +    /**
> +     * {@inheritDoc}
> +     */

This is redundant; JavaDoc will automatically inherit from the extended interface.

@@ +134,5 @@
> +    }
> +
> +    /**
> +     * {@inheritDoc}
> +     */

Redundant
Attachment #770464 - Flags: review?(bnicholson) → review+
Duplicate of this bug: 889588
Comment on attachment 770467 [details] [diff] [review]
Part 2: TopBookmarksAdapter

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

::: mobile/android/base/home/BookmarksPage.java
@@ +258,5 @@
> +
> +    /**
> +     * An AsyncTask to load the thumbnails from a cursor.
> +     */
> +    private class LoadThumbnailsTask extends UiAsyncTask<Cursor, Void, Map<String, Thumbnail>> {

If the only reason this class is non-static is because it references mTopBookmarks, I'd suggest passing mTopBookmarks to the constructor.

@@ +278,5 @@
> +            final List<String> urls = new ArrayList<String>();
> +            do {
> +                final String url = adapterCursor.getString(adapterCursor.getColumnIndexOrThrow(URLColumns.URL));
> +                urls.add(url);
> +            } while(adapterCursor.moveToNext());

Nit: space before "("
Attachment #770467 - Flags: review?(bnicholson) → review+
(Assignee)

Updated

5 years ago
Depends on: 891098
(Assignee)

Updated

5 years ago
Depends on: 891105

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/22104d00bbdc
https://hg.mozilla.org/mozilla-central/rev/d9832319249b
Assignee: nobody → sriram
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.