BookmarksListView should use a CursorAdapter

RESOLVED FIXED in Firefox 26

Status

()

Firefox for Android
Theme and Visual Design
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 26
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-fig)

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
BookmarksListView currently uses a SimpleCursorAdapter. This is not needed. Instead it should use a "simple" CursorAdapter ;)
(Assignee)

Comment 1

5 years ago
Created attachment 765693 [details] [diff] [review]
Patch

Minor cleanup.
Attachment #765693 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 2

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

Updated

5 years ago
Assignee: nobody → sriram
Blocks: 862793

Comment 3

5 years ago
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+
(Assignee)

Comment 4

5 years ago
(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

5 years ago
(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.

Updated

5 years ago
Whiteboard: fixed-fig

Updated

5 years ago
Duplicate of this bug: 878134

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/78e379237afb
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.