Closed
Bug 902038
Opened 12 years ago
Closed 12 years ago
Use a MetaCursorAdapter instead of SimpleCursorAdapter
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: sriram, Assigned: sriram)
References
Details
(Whiteboard: fixed-fig)
Attachments
(2 files)
4.24 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
20.23 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
It is better to create a meta cursor adapter (that works on a cursor and associated meta data) than use SimpleCursorAdapter.
Assignee | ||
Comment 1•12 years ago
|
||
This adds a MetaCursorAdapter. I was thinking of names and came up with a weird explanation "this is a cursor based adapter, but has some meta data about the cursor" :P I can change the name if needed.
Attachment #786417 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 2•12 years ago
|
||
This replaces the other SimpleCursorAdapter and cleans up BookmarksListAdapter too. BrowserSearch's adapter is an inner class, but not a static inner class. So we can't create static VIEW_TYPES and LAYOUT_TYPES.
Attachment #786420 -
Flags: review?(lucasr.at.mozilla)
Comment 3•12 years ago
|
||
Comment on attachment 786417 [details] [diff] [review]
Part 1: MetaCursorAdapter
Review of attachment 786417 [details] [diff] [review]:
-----------------------------------------------------------------
Looks with the suggested changes. I still prefer MultiTypeCursorAdapter. "Meta" is just too vague :-)
::: mobile/android/base/home/MetaCursorAdapter.java
@@ +42,5 @@
> +
> + /**
> + * @return Cursor for the given position.
> + */
> + public Cursor getCursor(int position) {
Is this something you'd like to allow subclasses to override? If not, make this method final.
@@ +52,5 @@
> + return cursor;
> + }
> +
> + @Override
> + public View getView(int position, View convertView, ViewGroup parent) {
Make it final?
@@ +64,5 @@
> + }
> +
> + @Override
> + public final void bindView(View view, Context context, Cursor cursor) {
> + // Do nothing.
It would be probably useful if you threw an exception here as a clear sign that these methods should be used in any circumstance.
@@ +69,5 @@
> + }
> +
> + @Override
> + public final View newView(Context context, Cursor cursor, ViewGroup parent) {
> + return null;
Same here: throw an exception here as a clear sign that these methods should be used in any circumstance.
@@ +79,5 @@
> + * @param context Context for inflating the view.
> + * @param position Position of the view.
> + * @param parent Parent view group that will hold this view.
> + */
> + public View newView(Context context, int position, ViewGroup parent) {
Does this method actually need to be public?
@@ +81,5 @@
> + * @param parent Parent view group that will hold this view.
> + */
> + public View newView(Context context, int position, ViewGroup parent) {
> + final int type = getItemViewType(position);
> + final int count = mViewTypes.length;
nit: add empty line here.
@@ +86,5 @@
> + for (int i = 0; i < count; i++) {
> + if (mViewTypes[i] == type) {
> + return LayoutInflater.from(context).inflate(mLayouts[i], parent, false);
> + }
> + }
nit: add empty line here.
Attachment #786417 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 4•12 years ago
|
||
Comment on attachment 786420 [details] [diff] [review]
Part 2: Replace all
Review of attachment 786420 [details] [diff] [review]:
-----------------------------------------------------------------
Nice.
::: mobile/android/base/home/BrowserSearch.java
@@ +626,4 @@
> public SearchAdapter(Context context) {
> + super(context, null, new int[] { ROW_STANDARD,
> + ROW_SEARCH,
> + ROW_SUGGEST },
I'd follow the same pattern of declaring constants holding the lists of types and layouts.
Attachment #786420 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 786420 [details] [diff] [review]
> Part 2: Replace all
>
> Review of attachment 786420 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Nice.
>
> ::: mobile/android/base/home/BrowserSearch.java
> @@ +626,4 @@
> > public SearchAdapter(Context context) {
> > + super(context, null, new int[] { ROW_STANDARD,
> > + ROW_SEARCH,
> > + ROW_SUGGEST },
>
> I'd follow the same pattern of declaring constants holding the lists of
> types and layouts.
BrowserSearch's SearchAdapter is an inner class but not static. Hence it can't have static members. So the code will be like,
class SearchAdapter {
public final int[] VIEW_TYPES, LAYOUT_TYPES;
SearchAdapter() {
VIEW_TYPES = ...;
LAYOUT_TYPES = ...;
super(...);
}
}
But then, super() should be the first line in the constructor and so the variables would become useless. Hence I passed the values directly.
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #3)
> Comment on attachment 786417 [details] [diff] [review]
> Part 1: MetaCursorAdapter
>
> Review of attachment 786417 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> @@ +64,5 @@
> > + }
> > +
> > + @Override
> > + public final void bindView(View view, Context context, Cursor cursor) {
> > + // Do nothing.
>
> It would be probably useful if you threw an exception here as a clear sign
> that these methods should be used in any circumstance.
The final makes sure that anyone extending this Adapter cannot use this method. So this method would never be called. So we needn't throw an exception here.
>
> @@ +69,5 @@
> > + }
> > +
> > + @Override
> > + public final View newView(Context context, Cursor cursor, ViewGroup parent) {
> > + return null;
>
> Same here: throw an exception here as a clear sign that these methods should
> be used in any circumstance.
The final makes sure that anyone extending this Adapter cannot use this method. So this method would never be called.
>
> @@ +79,5 @@
> > + * @param context Context for inflating the view.
> > + * @param position Position of the view.
> > + * @param parent Parent view group that will hold this view.
> > + */
> > + public View newView(Context context, int position, ViewGroup parent) {
>
> Does this method actually need to be public?
The class has package access only. So, this could be public.
Comment 7•12 years ago
|
||
(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> (In reply to Lucas Rocha (:lucasr) from comment #3)
> > Comment on attachment 786417 [details] [diff] [review]
> > Part 1: MetaCursorAdapter
> >
> > Review of attachment 786417 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > @@ +64,5 @@
> > > + }
> > > +
> > > + @Override
> > > + public final void bindView(View view, Context context, Cursor cursor) {
> > > + // Do nothing.
> >
> > It would be probably useful if you threw an exception here as a clear sign
> > that these methods should be used in any circumstance.
>
>
> The final makes sure that anyone extending this Adapter cannot use this
> method. So this method would never be called. So we needn't throw an
> exception here.
>
> >
> > @@ +69,5 @@
> > > + }
> > > +
> > > + @Override
> > > + public final View newView(Context context, Cursor cursor, ViewGroup parent) {
> > > + return null;
> >
> > Same here: throw an exception here as a clear sign that these methods should
> > be used in any circumstance.
>
> The final makes sure that anyone extending this Adapter cannot use this
> method. So this method would never be called.
final ensures no one will be able to override it. The exception is for the cases a developer tries to call the method directly for some reason. It's not a big deal. Just an extra protection for more clarity on the API.
> >
> > @@ +79,5 @@
> > > + * @param context Context for inflating the view.
> > > + * @param position Position of the view.
> > > + * @param parent Parent view group that will hold this view.
> > > + */
> > > + public View newView(Context context, int position, ViewGroup parent) {
> >
> > Does this method actually need to be public?
>
> The class has package access only. So, this could be public.
But you still don't want other classes from the same package to have access to it anyway. For instance, Overriding this method is likely to break the expected behaviour of this class.
Assignee | ||
Comment 8•12 years ago
|
||
Oops. I thought the comment was for getView(). I will change this method to have private access.
Assignee | ||
Comment 9•12 years ago
|
||
http://hg.mozilla.org/projects/fig/rev/5ac4c656ec55
http://hg.mozilla.org/projects/fig/rev/04d3d68298d0
Whiteboard: fixed-fig
Assignee | ||
Comment 10•12 years ago
|
||
Changed the access: http://hg.mozilla.org/projects/fig/rev/f7264772c802
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5ac4c656ec55
https://hg.mozilla.org/mozilla-central/rev/04d3d68298d0
https://hg.mozilla.org/mozilla-central/rev/f7264772c802
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
•