885590 - BookmarksListView should use a CursorAdapter

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

BookmarksListView currently uses a SimpleCursorAdapter. This is not needed. Instead it should use a "simple" CursorAdapter ;)

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Minor cleanup.

Comment 2

[Sriram Ramasubramanian [:sriram]]

I forgot to refresh after removing "import android.widget.SimpleCursorAdapter;". That will be one change to the uploaded patch.

Comment 3

[:Margaret Leibovic]

Comment on attachment 765693 [details] [diff] [review]
Patch

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

This looks good, but I think we should get rid of the getFoo(position) methods like I mention in a comment down below.

::: mobile/android/base/home/BookmarksListView.java
@@ +257,5 @@
> +         * Returns the type of the item at the given position in the cursor.
> +         *
> +         * @param cursor A cursor moved to the required position.
> +         * @return The type of the item.
> +         */

Nice documentation.

@@ +324,3 @@
>          @Override
> +        public void bindView(View view, Context context, Cursor cursor) {
> +            final int viewType = getItemViewType(cursor);

It looks like we only use getItemViewType in this one place, so we can get rid of the getItemViewType(position) method.

We do, however, use getFolderTitle(position) somewhere else:
http://hg.mozilla.org/projects/fig/file/42c9ee1b0a15/mobile/android/base/home/BookmarksListView.java#l137

Although looking there, it looks like we could just replace that with a call to getFolderTitle(cursor), so we could get rid of the overloaded methods.

Comment 4

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #3)
> Comment on attachment 765693 [details] [diff] [review]
> Patch
> 
> Review of attachment 765693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but I think we should get rid of the getFoo(position)
> methods like I mention in a comment down below.
> @@ +324,3 @@
> >          @Override
> > +        public void bindView(View view, Context context, Cursor cursor) {
> > +            final int viewType = getItemViewType(cursor);
> 
> It looks like we only use getItemViewType in this one place, so we can get
> rid of the getItemViewType(position) method.

The only reason for me having this method is that, its an overriden method. I just wanted to have a logic here since we use a different method to get the values.

> 
> We do, however, use getFolderTitle(position) somewhere else:
> http://hg.mozilla.org/projects/fig/file/42c9ee1b0a15/mobile/android/base/
> home/BookmarksListView.java#l137
> 
> Although looking there, it looks like we could just replace that with a call
> to getFolderTitle(cursor), so we could get rid of the overloaded methods.

I can cleanup this part. Do you want me to post a new patch? Or shall I make the changes and push it?

Comment 5

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > Comment on attachment 765693 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 765693 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good, but I think we should get rid of the getFoo(position)
> > methods like I mention in a comment down below.
> > @@ +324,3 @@
> > >          @Override
> > > +        public void bindView(View view, Context context, Cursor cursor) {
> > > +            final int viewType = getItemViewType(cursor);
> > 
> > It looks like we only use getItemViewType in this one place, so we can get
> > rid of the getItemViewType(position) method.
> 
> The only reason for me having this method is that, its an overriden method.
> I just wanted to have a logic here since we use a different method to get
> the values.

Aha, I didn't notice the @Override. Makes sense to keep it, then.

> > 
> > We do, however, use getFolderTitle(position) somewhere else:
> > http://hg.mozilla.org/projects/fig/file/42c9ee1b0a15/mobile/android/base/
> > home/BookmarksListView.java#l137
> > 
> > Although looking there, it looks like we could just replace that with a call
> > to getFolderTitle(cursor), so we could get rid of the overloaded methods.
> 
> I can cleanup this part. Do you want me to post a new patch? Or shall I make
> the changes and push it?

You can just make the change and push it. Whichever you prefer.

Comment 6

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/78e379237afb

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/78e379237afb