Closed Bug 885590 Opened 11 years ago Closed 11 years ago

BookmarksListView should use a CursorAdapter

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)

BookmarksListView currently uses a SimpleCursorAdapter. This is not needed. Instead it should use a "simple" CursorAdapter ;)
Attached patch PatchSplinter Review
Minor cleanup.
Attachment #765693 - Flags: review?(margaret.leibovic)
I forgot to refresh after removing "import android.widget.SimpleCursorAdapter;". That will be one change to the uploaded patch.
Assignee: nobody → sriram
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.
Attachment #765693 - Flags: review?(margaret.leibovic) → review+
(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?
(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.
Whiteboard: fixed-fig
https://hg.mozilla.org/mozilla-central/rev/78e379237afb
Status: NEW → RESOLVED
Closed: 11 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: