Closed Bug 902038 Opened 8 years ago Closed 8 years ago

Use a MetaCursorAdapter instead of SimpleCursorAdapter

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: sriram, Assigned: sriram)

References

Details

(Whiteboard: fixed-fig)

Attachments

(2 files)

It is better to create a meta cursor adapter (that works on a cursor and associated meta data) than use SimpleCursorAdapter.
Assignee: nobody → sriram
Blocks: 898197
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)
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 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 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+
(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.
(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.
(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.
Oops. I thought the comment was for getView(). I will change this method to have private access.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.