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);
    }
}

Dwelling

are you logging the position value in the listener? Do you have any other listeners? – Liverwort

No :( and I have one more listener for a button but it's not related to the listView. but I added a toast just to see what position is being clicked and to my surprise, it's the right position. when I clicked on listItem 2 it showed position 2 and so on... weird. When you mentioned it I expected it to show the wrong position... – Dwelling

Any chance the checkboxes is somehow in reversed order to the list? – Dwelling

Maybe post the layout code for the row layouts (image_list.xml) as well? – Schiff

I don't see a particular problem, but I bet it has to do with ListView reusing views. Also, why are you inflating things yoruself in getView, instead of letting super handle the view creation? – Pitzer

Mayra thanks for replying once again! :) I'll check both of your suggestions. – Dwelling

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.

Deplete

Thank you for replying! Tried your suggestion and no matter what I did I got a Force Close for "IndexOutOfBoundsExeption". Any ideas? – Dwelling

You need to at some point add the initial entries to the ArrayList. When you know how many items there are, e.g., `for (int i = 0; i < numItems; i++) { checkedStates.add(false);} – Deplete

btw, shouldn't the checkedStates Array (located in main activity) be static? – Dwelling

Why do you think it should be static? – Deplete

uhh sorry for taking too long. I had to make it static so I can access it from my custom SimpleCursorAdapter which is on a different class file than the Main Activity. (btw, as you can see, I marked your answer as it worked perfectly! can't thank you enough!) – Dwelling

Glad it helped. I wrote a blog post describing the issue in more depth - developmentality.wordpress.com/2010/11/05/… . I was a little inaccurate in my statements earlier that there was a single checkbox reused across all rows - in fact there are N checkboxes, where N is the number of visible rows. But the basic point remains. – Deplete

Yeah. After your answer I wasn't sure it'll solve my problem so I continued exploring and asking and only after a week or two I return to your solution with better understanding the situation and solved it right away. Thanks again! :) – Dwelling