902038 - Use a MetaCursorAdapter instead of SimpleCursorAdapter

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

It is better to create a meta cursor adapter (that works on a cursor and associated meta data) than use SimpleCursorAdapter.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: MetaCursorAdapter

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.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: Replace all

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.

Comment 3

[Lucas Rocha (:lucasr)]

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.

Comment 4

[Lucas Rocha (:lucasr)]

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.

Comment 5

[Sriram Ramasubramanian [:sriram]]

(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.

Comment 6

[Sriram Ramasubramanian [:sriram]]

(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

[Lucas Rocha (:lucasr)]

(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.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Oops. I thought the comment was for getView(). I will change this method to have private access.

Comment 9

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/5ac4c656ec55
http://hg.mozilla.org/projects/fig/rev/04d3d68298d0

Comment 10

[Sriram Ramasubramanian [:sriram]]

Changed the access: http://hg.mozilla.org/projects/fig/rev/f7264772c802

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5ac4c656ec55
https://hg.mozilla.org/mozilla-central/rev/04d3d68298d0
https://hg.mozilla.org/mozilla-central/rev/f7264772c802