Is this custom CursorAdapter for a ListView properly coded for Android?

Question

I have never been happy with the code on my custom CursorAdapter until today I decided to review it and fix a little problem that was bothering me for a long time (interestingly enough, none of the users of my app ever reported such a problem).

Here's a small description of my question:

My custom CursorAdapter overrides newView() and bindView() instead of getView() as most examples I see. I use the ViewHolder pattern between these 2 methods. But my main issue was with the custom layout I'm using for each list item, it contains a ToggleButton.

The problem was that the button state was not kept when a list item view scrolled out of view and then scrollbed back into view. This problem existed because the cursor was never aware that the database data changed when the ToggleButton was pressed and it was always pulling the same data. I tried to requery the cursor when clicking the ToggleButton and that solved the problem, but it was very slow.

I have solved this issue and I'm posting the whole class here for review. I've commented the code thoroughly for this specific question to better explain my coding decisions.

Does this code look good to you? Would you improve/optimize or change it somehow?

P.S: I know the CursorLoader is an obvious improvement but I don't have time to deal with such big code rewrites for the time being. It's something I have in the roadmap though.

Here's the code:

public class NotesListAdapter extends CursorAdapter implements OnClickListener {

    private static class ViewHolder {
        ImageView icon;
        TextView title;
        TextView description;
        ToggleButton visibility;
    }

    private static class NoteData {
        long id;
        int iconId;
        String title;
        String description;
        int position;
    }

    private LayoutInflater mInflater;

    private NotificationHelper mNotificationHelper;
    private AgendaNotesAdapter mAgendaAdapter;

    /*
     * This is used to store the state of the toggle buttons for each item in the list
     */
    private List<Boolean> mToggleState;

    private int mColumnRowId;
    private int mColumnTitle;
    private int mColumnDescription;
    private int mColumnIconName;
    private int mColumnVisibility;

    public NotesListAdapter(Context context, Cursor cursor, NotificationHelper helper, AgendaNotesAdapter adapter) {
        super(context, cursor);

        mInflater = LayoutInflater.from(context);

        /*
         * Helper class to post notifications to the status bar and database adapter class to update
         * the database data when the user presses the toggle button in any of items in the list
         */
        mNotificationHelper = helper;
        mAgendaAdapter = adapter;

        /*
         * There's no need to keep getting the column indexes every time in bindView() (as I see in
         * a few examples) so I do it once and save the indexes in instance variables
         */
        findColumnIndexes(cursor);

        /*
         * Populate the toggle button states for each item in the list with the corresponding value
         * from each record in the database, but isn't this a slow operation?
         */
        for(mToggleState = new ArrayList<Boolean>(); !cursor.isAfterLast(); cursor.moveToNext()) {
            mToggleState.add(cursor.getInt(mColumnVisibility) != 0);
        }
    }

    @Override
    public View newView(Context context, Cursor cursor, ViewGroup parent) {
        View view = mInflater.inflate(R.layout.list_item_note, null);

        /*
         * The ViewHolder pattern is here only used to prevent calling findViewById() all the time
         * in bindView(), we only need to find all the views once
         */
        ViewHolder viewHolder = new ViewHolder();

        viewHolder.icon = (ImageView)view.findViewById(R.id.imageview_icon);
        viewHolder.title = (TextView)view.findViewById(R.id.textview_title);
        viewHolder.description = (TextView)view.findViewById(R.id.textview_description);
        viewHolder.visibility = (ToggleButton)view.findViewById(R.id.togglebutton_visibility);

        /*
         * I also use newView() to set the toggle button click listener for each item in the list
         */
        viewHolder.visibility.setOnClickListener(this);

        view.setTag(viewHolder);

        return view;
    }

    @Override
    public void bindView(View view, Context context, Cursor cursor) {
        Resources resources = context.getResources();

        int iconId = resources.getIdentifier(cursor.getString(mColumnIconName),
                "drawable", context.getPackageName());

        String title = cursor.getString(mColumnTitle);
        String description = cursor.getString(mColumnDescription);

        /*
         * This is similar to the ViewHolder pattern and it's need to access the note data when the
         * onClick() method is fired
         */
        NoteData noteData = new NoteData();

        /*
         * This data is needed to post a notification when the onClick() method is fired
         */
        noteData.id = cursor.getLong(mColumnRowId);
        noteData.iconId = iconId;
        noteData.title = title;
        noteData.description = description;

        /*
         * This data is needed to update mToggleState[POS] when the onClick() method is fired
         */
        noteData.position = cursor.getPosition();

        /*
         * Get our ViewHolder with all the view IDs found in newView()
         */
        ViewHolder viewHolder = (ViewHolder)view.getTag();

        /*
         * The Html.fromHtml is needed but the code relevant to that was stripped
         */
        viewHolder.icon.setImageResource(iconId);
        viewHolder.title.setText(Html.fromHtml(title));
        viewHolder.description.setText(Html.fromHtml(description));

        /*
         * Set the toggle button state for this list item from the value in mToggleState[POS]
         * instead of getting it from the database with 'cursor.getInt(mColumnVisibility) != 0'
         * otherwise the state will be incorrect if it was changed between the item view scrolling
         * out of view and scrolling back into view
         */
        viewHolder.visibility.setChecked(mToggleState.get(noteData.position));

        /*
         * Again, save the note data to be accessed when onClick() gets fired
         */
        viewHolder.visibility.setTag(noteData);
    }

    @Override
    public void onClick(View view) {
        /*
         * Get the new state directly from the toggle button state 
         */
        boolean visibility = ((ToggleButton)view).isChecked();

        /*
         * Get all our note data needed to post (or remove) a notification 
         */
        NoteData noteData = (NoteData)view.getTag();

        /*
         * The toggle button state changed, update mToggleState[POS] to reflect that new change
         */
        mToggleState.set(noteData.position, visibility);

        /*
         * Post the notification or remove it from the status bar depending on toggle button state
         */
        if(visibility) {
            mNotificationHelper.postNotification(
                    noteData.id, noteData.iconId, noteData.title, noteData.description);
        } else {
            mNotificationHelper.cancelNotification(noteData.id);
        }

        /*
         * Update the database note item with the new toggle button state, without the need to
         * requery the cursor (which is slow, I've tested it) to reflect the new toggle button state
         * in the list because the value was saved in mToggleState[POS] a few lines above
         */
        mAgendaAdapter.updateNote(noteData.id, null, null, null, null, visibility);
    }

    private void findColumnIndexes(Cursor cursor) {
        mColumnRowId = cursor.getColumnIndex(AgendaNotesAdapter.KEY_ROW_ID);
        mColumnTitle = cursor.getColumnIndex(AgendaNotesAdapter.KEY_TITLE);
        mColumnDescription = cursor.getColumnIndex(AgendaNotesAdapter.KEY_DESCRIPTION);
        mColumnIconName = cursor.getColumnIndex(AgendaNotesAdapter.KEY_ICON_NAME);
        mColumnVisibility = cursor.getColumnIndex(AgendaNotesAdapter.KEY_VISIBILITY);
    }

}

Answer 1

Your solution is optimal an I will add it to my weapons :) Maybe, I'll try to bring a little optimization for the calls to database.

Indeed, because of conditions of the task, there are only three solutions:

1. Update only one row, requery cursor and redraw all items. (Straight-forward, brute force).
2. Update the row, cache the results and use cache for drawing items.
3. Cache the results, use cache for drawing items. And when you leave this activity/fragment then commit the results to database.

For 3rd solution you can use SparseArray for looking for the changes.

private SparseArray<NoteData> mArrayViewHolders;

public void onClick(View view) {
     //here your logic with NoteData. 
     //start of my improve
     if (mArrayViewHolders.get(selectedPosition) == null) {
        // put the change into array
        mArrayViewHolders.put(selectedPosition, noteData);
     } else {
        // rollback the change
        mArrayViewHolders.delete(selectedPosition);
     }
     //end of my improve
     //we don't commit the changes to database.
}

Once again: from the start this array is empty. When you toggle the button first time (there is a change), you add NoteData to array. When you toggle the button second time (there is a rollback), you remove NoteData from array. And so on.

When you're finishing, just request the array and push the changes into database.

Answer 2

What you are seeing is the View re use of Android. I don't think that you are doing something wrong by querying the cursor again. Just dont use the cursor.requery() function.

Instead, set the toggle to false at first always and then ask the cursor and switch it on if you have to.

Maybe you were doing that and I misunderstood something, however I don't think that you should have slow results doing it.

Pseudo-code:

getView(){
setToggleFalse();
boolean value = Boolean.valueOf(cursor.getString('my column'));
if (value){
   setToggleTrue();
}
}

Answer 3

I would wait before going to CursorLoader. As it seems CursorLoader derivatives do not worl with CursorLoader.