889612 - BookmarksPage fragment should handle the adapters for its views

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

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.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: BookmarksListAdapter

Moved BookmarksListAdapter to a separate file. BookmarksPage will hold and handle things for it. (aah.. sooo cleaaaan)

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: 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.

Comment 3

[Sriram Ramasubramanian [:sriram]]

All cleanups done with Eclipse. So, no "unwanted imports" or "unused methods" anywhere. :D

Comment 4

[Brian Nicholson (:bnicholson)]

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

Comment 6

[Brian Nicholson (:bnicholson)]

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 "("

Comment 7

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/22104d00bbdc
http://hg.mozilla.org/projects/fig/rev/d9832319249b

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/22104d00bbdc
https://hg.mozilla.org/mozilla-central/rev/d9832319249b