Android Click on listItem checks wrong checkbox

Question

I've created a custom ListView by extending SimpleCursorAdapter. The result is IMAGE + CheckedTextView (Text + Checkbox).

When I long click an Item, everything works fine - I get the right ID and details of the clicked Item.

The problem occurs when I try to mark an Item as checked but it checks the wrong checkbox.

For example: I have 9 items on my list, sorted 1-9. if I click on listItem 1, the checkbox on line 9 is being checked. if I click on item 4, the checkbox on line 6 is being checked and if I click on the middle line, it is being checked.

Clearly I'm missing something here :) Do remember when I long click the line (contextMenu opens), everything works great.

This is the listener:

lv.setOnItemClickListener(new OnItemClickListener() {
            @Override
            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
                CheckedTextView markedItem = (CheckedTextView) view.findViewById(R.id.btitle);

                if (!markedItem.isChecked()) {
                    markedItem.setChecked(true);
                } else {
                    markedItem.setChecked(false);
                }

            }
        });

Appreciate any help!

Let me know If you need me to post more code.

Thank you!

btw, If I click on more than one... the PARTY continues... no obvious order...

EDIT: the Adapter code

public class ImageCursorAdapter extends SimpleCursorAdapter {

    private Cursor c;
    private Context context;

    private String url;
    private TextView bUrl;

    public ImageCursorAdapter(Context context, int layout, Cursor c,
            String[] from, int[] to) {
        super(context, layout, c, from, to);
        this.c = c;
        this.context = context;
    }

    public View getView(int pos, View inView, ViewGroup parent) {
        View v = inView;
        if (v == null) {
            LayoutInflater inflater = (LayoutInflater) context.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
            v = inflater.inflate(R.layout.image_list, null);
        }

        this.c.moveToPosition(pos);

        final TextView bTitle = (TextView) v.findViewById(R.id.btitle);
        String bookmark = this.c.getString(this.c.getColumnIndex(Browser.BookmarkColumns.TITLE));


        byte[] favicon = this.c.getBlob(this.c.getColumnIndex(Browser.BookmarkColumns.FAVICON));

        if (favicon != null) {
            ImageView iv = (ImageView) v.findViewById(R.id.bimage);
            iv.setImageBitmap(BitmapFactory.decodeByteArray(favicon, 0, favicon.length));
        }
        bTitle.setText(bookmark);

        return (v);
    }
}

Answer 1

Mayra is right - the problem has to do with the way the ListView is reusing your views. It's not as if there are 9 instances of the CheckedTextView object, one per view. Instead, there's a single one that is reused in all the rows. Thus you can't rely on the CheckedTextView object to hold the state of whether an item is checked. You'll need some additional data structure to hold whether a given row is checked For instance,

ArrayList<Boolean> checkedStates = new ArrayList<Boolean>();

Where the ith element is true iff the ith row should be checked. Then within your itemClickListener:

lv.setOnItemClickListener(new OnItemClickListener() {
        @Override
        public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
            boolean currentlyChecked = checkedStates.get(position);
            checkedStates.set(position, !currentlyChecked);
            // Refresh the list
        }
    });

Then within your view code:

public View getView(int pos, View inView, ViewGroup parent) {
    View v = inView;
    if (v == null) {
        LayoutInflater inflater = (LayoutInflater) context.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
        v = inflater.inflate(R.layout.image_list, null);
    }

    this.c.moveToPosition(pos);

    final TextView bTitle = (TextView) v.findViewById(R.id.btitle);
    String bookmark = this.c.getString(this.c.getColumnIndex(Browser.BookmarkColumns.TITLE));


    byte[] favicon = this.c.getBlob(this.c.getColumnIndex(Browser.BookmarkColumns.FAVICON));

    if (favicon != null) {
        ImageView iv = (ImageView) v.findViewById(R.id.bimage);
        iv.setImageBitmap(BitmapFactory.decodeByteArray(favicon, 0, favicon.length));
    }
    bTitle.setText(bookmark);


    // Change the state of the checkbox to match that of the row's checked state.
    // This check box item is reused for every row, so we need to reset its state each
    // time the row is rendered.
    CheckedTextView markedItem = (CheckedTextView) view.findViewById(R.id.btitle);
    markedItem.setChecked(checkedStates.get(pos));


    return (v);
}

This should solve your problem. An alternative approach would be to move the logic of whether the row is checked or not into the domain object that the row represents. That would be my preference.