Closed
Bug 885590
Opened 12 years ago
Closed 12 years ago
BookmarksListView should use a CursorAdapter
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: sriram, Assigned: sriram)
References
Details
(Whiteboard: fixed-fig)
Attachments
(1 file)
7.91 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
BookmarksListView currently uses a SimpleCursorAdapter. This is not needed. Instead it should use a "simple" CursorAdapter ;)
Assignee | ||
Comment 1•12 years ago
|
||
Minor cleanup.
Attachment #765693 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 2•12 years ago
|
||
I forgot to refresh after removing "import android.widget.SimpleCursorAdapter;". That will be one change to the uploaded patch.
Updated•12 years ago
|
Assignee: nobody → sriram
Blocks: new-about-home
Comment 3•12 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•12 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•12 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.
Assignee | ||
Comment 6•12 years ago
|
||
Updated•12 years ago
|
Whiteboard: fixed-fig
Comment 8•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•