Closed
Bug 898197
Opened 12 years ago
Closed 11 years ago
[fig] Use CursorAdapter instead of SimpleCursorAdapter
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: Margaret, Assigned: sriram)
References
Details
(Whiteboard: abouthome-hackathon, fixed-fig)
Attachments
(6 files)
4.23 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
2.14 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
14.53 KB,
patch
|
Margaret
:
review+
lucasr
:
review+
|
Details | Diff | Splinter Review |
5.40 KB,
patch
|
lucasr
:
review-
|
Details | Diff | Splinter Review |
6.32 KB,
patch
|
Margaret
:
review+
lucasr
:
review-
|
Details | Diff | Splinter Review |
9.06 KB,
patch
|
lucasr
:
feedback+
|
Details | Diff | Splinter Review |
This is similar to bug 885590, but for all other places we use SimpleCursorAdapter. Those places include BrowserSearch, HistoryPage, and LastTabsPage.
Basically, we don't take advantage of the benefits of SimpleCursorAdapter, so it makes more sense for use to just use CursorAdapter.
Assignee | ||
Comment 1•12 years ago
|
||
Replaced SimpleCursorAdapter to CursorAdapter. Now that adapter is super simple.
More changes:
1. The time to load a view by storing a reference to LayoutInflater, and not storing a reference is --- 3ms! So, there is no point in storing a LayoutInflater anywhere, ever!
2. Whenever possible it is better to make an inner class static.
http://stackoverflow.com/questions/3106912/why-does-android-prefer-static-classes
Attachment #781493 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #781494 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 3•12 years ago
|
||
HistoryAdapter is the model for the HistoryListView. There was no reason for the HistoryListView to have methods related to its model outside the adapter. This patches moves all the "history section" based methods inside the adapter.
Voila! The adapter is now static and :bnicholson is happy!
Attachment #781504 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 781505 [details] [diff] [review]
Part 4: Use CursorAdapter in HistoryPage
Ideally we need to override only bindView() and newView(). However, the cursor doesn't have any information about the "history sections". Hence the getView() is overriden to take care of this scenario. This logic is same as in BookmarksPage.
Attachment #781505 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 6•12 years ago
|
||
Comment on attachment 781505 [details] [diff] [review]
Part 4: Use CursorAdapter in HistoryPage
Review of attachment 781505 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/home/HistoryPage.java
@@ +248,4 @@
>
> + @Override
> + public View newView(Context context, Cursor cursor, ViewGroup parent) {
> + return LayoutInflater.from(mContext).inflate(R.layout.home_item_row, parent, false);
This could be just "context". I'll make that change.
Assignee | ||
Comment 7•12 years ago
|
||
I tried making SearchAdapter a static class. That's a bigger mess touching many things outside. I don't want to touch it yet. I've made the CursorLoader changes.
Attachment #781510 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 8•12 years ago
|
||
I am motivated to move the Adapters out. This way, they'll always remain independent of the Views they back and force anyone not to make them dependent. I made it for TopBookmarksAdapter and BookmarksListAdapter. These two adapters can follow suit.
Reporter | ||
Comment 9•12 years ago
|
||
Comment on attachment 781493 [details] [diff] [review]
Part 1: CusorAdapter in LastTabsPage
Review of attachment 781493 [details] [diff] [review]:
-----------------------------------------------------------------
Nice.
Attachment #781493 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #781494 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 781504 [details] [diff] [review]
Part 3: Make HistoryAdapter static
I still want to think about this one a bit, and I'd like to know what lucasr's opinion is as well.
Attachment #781504 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 11•12 years ago
|
||
Reporter | ||
Comment 12•12 years ago
|
||
Comment on attachment 781510 [details] [diff] [review]
Part 5: Use CusorLoader with BrowserSearch
Review of attachment 781510 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/home/BrowserSearch.java
@@ +640,3 @@
>
> @Override
> public View getView(int position, View convertView, ViewGroup parent) {
Can you add a comment about why we need to override getView? It isn't always necessary for implementing a CursorAdapter, so adding a comment that this is special might prevent some bad copypasta in the future :)
@@ +663,5 @@
> mAnimateSuggestions = false;
> }
>
> return row;
> } else {
I know it's not your code, but we don't need an else statement, since we're returning in the if statement.
Attachment #781510 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to :Margaret Leibovic from comment #12)
> Comment on attachment 781510 [details] [diff] [review]
> Part 5: Use CusorLoader with BrowserSearch
>
> Review of attachment 781510 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: mobile/android/base/home/BrowserSearch.java
> @@ +640,3 @@
> >
> > @Override
> > public View getView(int position, View convertView, ViewGroup parent) {
>
> Can you add a comment about why we need to override getView? It isn't always
> necessary for implementing a CursorAdapter, so adding a comment that this is
> special might prevent some bad copypasta in the future :)
>
> @@ +663,5 @@
> > mAnimateSuggestions = false;
> > }
> >
> > return row;
> > } else {
>
> I know it's not your code, but we don't need an else statement, since we're
> returning in the if statement.
The else statement does a return too. It calls a super.getView(). Also, I would like to have the else statement here as it makes the code flow logically.
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Whiteboard: abouthome-hackathon
Reporter | ||
Comment 15•12 years ago
|
||
(In reply to Sriram Ramasubramanian [:sriram] from comment #13)
> > @@ +663,5 @@
> > > mAnimateSuggestions = false;
> > > }
> > >
> > > return row;
> > > } else {
> >
> > I know it's not your code, but we don't need an else statement, since we're
> > returning in the if statement.
>
> The else statement does a return too. It calls a super.getView(). Also, I
> would like to have the else statement here as it makes the code flow
> logically.
Well, yes, the method does return at the end. It's just not necessary to have an else because the only time we end up below the if is if we weren't in it. It's not a big deal, I just prefer to avoid unnecessary indentation.
Comment 16•12 years ago
|
||
Comment on attachment 781504 [details] [diff] [review]
Part 3: Make HistoryAdapter static
Review of attachment 781504 [details] [diff] [review]:
-----------------------------------------------------------------
Looks fine.
::: mobile/android/base/home/HistoryPage.java
@@ +287,5 @@
> +
> + return sectionsBefore;
> + }
> +
> + private HistorySection getHistorySectionForTime(long from, long time) {
This method can be made static.
Attachment #781504 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 17•12 years ago
|
||
Comment on attachment 781510 [details] [diff] [review]
Part 5: Use CusorLoader with BrowserSearch
Review of attachment 781510 [details] [diff] [review]:
-----------------------------------------------------------------
This is a case where I think using CursorLoader actually makes code harder to read. Handling everything in getView() makes things simpler. Now you're splitting the responsibility of inflating and binding to separate methods for only one of the view types, which is harder to understand imo.
Attachment #781510 -
Flags: feedback-
Comment 18•12 years ago
|
||
Comment on attachment 781505 [details] [diff] [review]
Part 4: Use CursorAdapter in HistoryPage
Review of attachment 781505 [details] [diff] [review]:
-----------------------------------------------------------------
Same here. I don't think this patch improves readability. It just makes splits responsibility of creating/binding views into different methods for only one type of view. I'd prefer to keep everything in getView() instead.
Attachment #781505 -
Flags: feedback-
Assignee | ||
Comment 19•12 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #17)
> Comment on attachment 781510 [details] [diff] [review]
> Part 5: Use CusorLoader with BrowserSearch
>
> Review of attachment 781510 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This is a case where I think using CursorLoader actually makes code harder
> to read. Handling everything in getView() makes things simpler. Now you're
> splitting the responsibility of inflating and binding to separate methods
> for only one of the view types, which is harder to understand imo.
I agree that CursorAdapter isn't the best in this case. That's because the CursorAdapter is used to use it only with cursors. But then we store our data in other places too, along with a cursor, and the list rows are depending on both these kinds of data. So, just a cursor-based adapter cannot help us here.
But, using a SimpleCursorAdapter is more than what we need. We don't use the layout passed to it. We don't allow it to do stuff that it is intended to do.
Assignee | ||
Comment 20•12 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #18)
> Comment on attachment 781505 [details] [diff] [review]
> Part 4: Use CursorAdapter in HistoryPage
>
> Review of attachment 781505 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Same here. I don't think this patch improves readability. It just makes
> splits responsibility of creating/binding views into different methods for
> only one type of view. I'd prefer to keep everything in getView() instead.
I pushed the patch as I got an r+ already ;)
Reporter | ||
Comment 21•12 years ago
|
||
(In reply to Sriram Ramasubramanian [:sriram] from comment #20)
> (In reply to Lucas Rocha (:lucasr) from comment #18)
> > Comment on attachment 781505 [details] [diff] [review]
> > Part 4: Use CursorAdapter in HistoryPage
> >
> > Review of attachment 781505 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > Same here. I don't think this patch improves readability. It just makes
> > splits responsibility of creating/binding views into different methods for
> > only one type of view. I'd prefer to keep everything in getView() instead.
>
> I pushed the patch as I got an r+ already ;)
Lucas still has the right to dislike it :) I don't have a big opinion either way, but we can always back this out or file a follow-up to change it if there's a better solution.
Assignee | ||
Comment 22•12 years ago
|
||
(In reply to :Margaret Leibovic from comment #21)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #20)
> > (In reply to Lucas Rocha (:lucasr) from comment #18)
> > > Comment on attachment 781505 [details] [diff] [review]
> > > Part 4: Use CursorAdapter in HistoryPage
> > >
> > > Review of attachment 781505 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > >
> > > Same here. I don't think this patch improves readability. It just makes
> > > splits responsibility of creating/binding views into different methods for
> > > only one type of view. I'd prefer to keep everything in getView() instead.
> >
> > I pushed the patch as I got an r+ already ;)
>
> Lucas still has the right to dislike it :) I don't have a big opinion either
> way, but we can always back this out or file a follow-up to change it if
> there's a better solution.
If a better solution is recommended, I can push it.
Comment 23•12 years ago
|
||
Comment on attachment 781505 [details] [diff] [review]
Part 4: Use CursorAdapter in HistoryPage
Giving r- just to make it more clear that I don't agree that this patch is an improvement over the current code. Keeping everything in getView() is easier to understand for adapters with multiple view types. You can still inherit from CursorAdapter if you want.
Attachment #781505 -
Flags: feedback- → review-
Comment 24•12 years ago
|
||
Comment on attachment 781510 [details] [diff] [review]
Part 5: Use CusorLoader with BrowserSearch
Review of attachment 781510 [details] [diff] [review]:
-----------------------------------------------------------------
Ditto.
Attachment #781510 -
Flags: review-
Updated•12 years ago
|
Attachment #781510 -
Flags: feedback-
Assignee | ||
Comment 25•12 years ago
|
||
Backed Part 5 out as suggested:
http://hg.mozilla.org/projects/fig/rev/ce8fb2c1f618
Part 4 was not pushed.
Assignee | ||
Comment 26•12 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #23)
> Comment on attachment 781505 [details] [diff] [review]
> Part 4: Use CursorAdapter in HistoryPage
>
> Giving r- just to make it more clear that I don't agree that this patch is
> an improvement over the current code. Keeping everything in getView() is
> easier to understand for adapters with multiple view types. You can still
> inherit from CursorAdapter if you want.
The reason for overriding bindView() and newView() was because they are abstract methods and have to be implemented. So, if this class inherits CursorAdapter, we need to override those two methods -- that was the reason for the more complicated approach.
Anyways, backed out.
Assignee | ||
Comment 27•12 years ago
|
||
Backout for wrong bug number: http://hg.mozilla.org/projects/fig/rev/58523c4666e8
Original backout: http://hg.mozilla.org/projects/fig/rev/a960b4cb3378
Assignee | ||
Updated•12 years ago
|
Whiteboard: abouthome-hackathon → abouthome-hackathon, fixed-fig
Assignee | ||
Comment 28•12 years ago
|
||
Not yet fixed-fig, part 3 remains :P
Whiteboard: abouthome-hackathon, fixed-fig → abouthome-hackathon
Assignee | ||
Comment 29•12 years ago
|
||
SimpleCursorAdapter is clearly not for our needs (or I hate a bunch of -1, new int[] { }).
How about a ComplexCursorAdapter? Just like the Simple version, this takes in a bunch of view types and resource ids. newView()s are created based on view-types. The classes extending this will just have to bindViews(). This cleans up a bunch of (convertView != null) cases.
Attachment #782777 -
Flags: feedback?(lucasr.at.mozilla)
Reporter | ||
Updated•12 years ago
|
Attachment #781504 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #781505 -
Flags: review?(margaret.leibovic)
Comment 30•12 years ago
|
||
Comment on attachment 782777 [details] [diff] [review]
Part 6: WIP: ComplexCursorAdapter
Review of attachment 782777 [details] [diff] [review]:
-----------------------------------------------------------------
Looks like a handy thing to have. Just needs some API cleanups. Maybe move this to a separate bug so that you can close this bug?
::: mobile/android/base/home/ComplexCursorAdapter.java
@@ +17,5 @@
> + * SimpleCursorAdapter's help is unusable for our needs.
> + * ComplexCursorAdapter can take a set of view-types and map them to a set of layouts.
> + * Any extending class will need to bindView().
> + */
> +abstract class ComplexCursorAdapter extends CursorAdapter {
Maybe MultiTypeCursorAdapter is more clear?
@@ +29,5 @@
> + mViewTypes = viewTypes;
> + mLayouts = layouts;
> +
> + if (mViewTypes.length != mLayouts.length) {
> + throw new IllegalStateException("Y U NO GIMME SAME SIZE ARRAYS??");
A better exception message please :-)
@@ +45,5 @@
> + return convertView;
> + }
> +
> + @Override
> + public void bindView(View view, Context context, Cursor cursor) {
Make it final so that subclasses can't mistakenly override it?
@@ +50,5 @@
> + // Do nothing.
> + }
> +
> + @Override
> + public View newView(Context context, Cursor cursor, ViewGroup parent) {
Same here: make it final so that subclasses can't mistakenly override it?
@@ +54,5 @@
> + public View newView(Context context, Cursor cursor, ViewGroup parent) {
> + return null;
> + }
> +
> + public View newView(Context context, int position, ViewGroup parent) {
And here: make it final so that subclasses can't mistakenly override it?
::: mobile/android/base/home/HistoryPage.java
@@ +197,5 @@
> return ROW_STANDARD;
> }
>
> @Override
> public int getViewTypeCount() {
This should be a final method in ComplexCursorAdapter() that always returns mViewTypes.length. More solid, less error-prone.
Attachment #782777 -
Flags: feedback?(lucasr.at.mozilla) → feedback+
Assignee | ||
Comment 31•12 years ago
|
||
Assignee | ||
Comment 32•12 years ago
|
||
Closing this bug. Part 6 will be handled separately.
Whiteboard: abouthome-hackathon → abouthome-hackathon, fixed-fig
Comment 33•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0e590de383a8
https://hg.mozilla.org/mozilla-central/rev/1b246e4cd75a
https://hg.mozilla.org/mozilla-central/rev/4c3421eb1d43
Status: NEW → RESOLVED
Closed: 11 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
•