OnClickListener in ArrayAdapter is taking action on wrong rows

Question

I have an ArrayAdapter for a list view that has multiple buttons in it. For one toggle button, I want to have a default state based on a condition, and let users toggle the button as well.

However, when users click button on row 1, the button for row 3 actually gets selected. I'm not sure why this is happening. Below is snippet of relevant code from my getView method with comments.

layout of my toggle button

    <ToggleButton android:id="@+id/color_toggle"
        android:layout_width="50px"
        android:layout_height="50px"
        android:focusable="false"
        android:textOn="" android:textOff="" android:layout_alignParentLeft="true"
        android:layout_marginRight="10dp"
        />

class Color {
   int id;
   int something;
}
List<Color> colorsList;

class ColorHolder {
   TextView colorNameText;
   ToggleButton toggleButton;
}


public View getView(final int position, final View convertView, final ViewGroup parent) {
  View rowView = convertView;
  Color c = colorsList.get(position);
  if (null == rowView) {
      rowView = this.inflater.inflate(R.layout.list_item_color, parent, false);
      holder = new ColorHolder();
      holder.colorNameText = (TextView) rowView.findViewById(R.id.color_name);
      holder.toggleButton = (ToggleButton) rowView.findViewById(R.id.color_toggle);


      rowView.setTag(holder);
  }
  else { 
      holder = (ColorHolder)rowView.getTag();
  }
  holder.toggleButton.setTag(c.getId());
  final ColorHolder thisRowHolder = holder;
  holder.toggleButton.setOnClickListener(new View.OnClickListener() {
    @Override
    public void onClick(View v) {
        if (thisRowHolder.toggleButton.isChecked()) {
            thisRowHolder.toggleButton.setBackgroundDrawable(//normal button);
            thisRowHolder.toggleButton.setChecked(false);
            for (int i = 0; i < colorList.size(); i++) {
               if (colorList.get(i) == (Integer)v.getTag()) {
                   colorList.get(i).setSomething(0);
                   break;
               }
            }
            adapter.notifyDataSetChanged();
        }
        else {
            thisRowHolder.toggleButton.setBackgroundDrawable(//enabled button);
            thisRowHolder.toggleButton.setChecked(true);
            for (int i = 0; i < colorList.size(); i++) {
               if (colorList.get(i) == (Integer)v.getTag()) {
                   colorList.get(i).setSomething(1);
                   break;
               }
            }
            adapter.notifyDataSetChanged();
        }
    }
});

if (c.getSomething()>0) {
   holder.toggleButton.setBackgroundDrawable(//enabled button);
   holder.toggleButton.setChecked(true);
}
else {
   holder.toggleButton.setBackgroundDrawable(//normal button);
   holder.toggleButton.setChecked(false);
}

return rowView;
}

Question

What am I doing wrong? why are other buttons in third row toggling even though i'm toggling buttons in row one.

I read that this happens because the listView recycles, is there no way to fix it? Some strategies i've tried, to no avail, based on similar questions: 1) put onClickListener in the if clause. 2) instead of setting int in setTag instead set the holder and use that holder in onClickListener

update

I've updated all the code in the question with suggestions I received.

Answer 1

Hope This Helps.

Activity Code

public class DemoActivity extends Activity {
    /** Called when the activity is first created. */

    @Override
    public void onCreate(Bundle savedInstanceState) 
    {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.main);

        ColorInfo[] clr= new ColorInfo[20];

        for(int i=0;i<20;i++){
            clr[i] = new ColorInfo();
        }

        ((ListView)findViewById(R.id.list)).setAdapter(new MyAdapter(this, 0, clr));

    }

    private static class MyAdapter extends ArrayAdapter<ColorInfo> implements OnClickListener{

        LayoutInflater inflater;
        public MyAdapter(Context context, int textViewResourceId,
                ColorInfo[] objects) {
            super(context, textViewResourceId, objects);
            inflater = (LayoutInflater) context.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
        }

        @Override
        public View getView(int position, View convertView, ViewGroup parent) { 

            ViewHolder holder;

            if(convertView == null){
                convertView = inflater.inflate(R.layout.row, null);
                holder = new ViewHolder();
                holder.tgl = (ToggleButton) convertView.findViewById(R.id.toggle);
                convertView.setTag(holder);
            }
            holder = (ViewHolder) convertView.getTag();
            holder.tgl.setTag(position);
            holder.tgl.setOnClickListener(this);
            holder.tgl.setChecked(getItem(position).isChecked);
            return convertView;
        }


        private static class ViewHolder{
            ToggleButton tgl;
        }


        public void onClick(View v) {

            int pos = (Integer) v.getTag();

            ColorInfo cinfo = getItem(pos);

            cinfo.isChecked = !cinfo.isChecked;

        }
    }

    private static class ColorInfo{
        boolean isChecked=false;
    }

}

main.xml

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="fill_parent"
    android:layout_height="fill_parent"
    android:orientation="vertical" >

    <ListView
        android:id="@+id/list"
        android:layout_width="fill_parent"
        android:layout_height="wrap_content" >
    </ListView>

</LinearLayout>

row.xml

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:orientation="vertical" >

<ToggleButton android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:id="@+id/toggle"
    />
</LinearLayout>

Answer 2

You are using a member variable for your ViewHolder, rather than a final local variable. So your OnClickListener is referencing whatever the latest holder instance is, which will correspond with the most recently created or recycled list item.

Do this instead:

  //Lock in this reference for the OnClickListener
  final ColorHolder thisRowHolder = holder; 

  holder.favButton.setOnClickListener(new View.OnClickListener() {
    @Override
    public void onClick(View v) {
        if (thisRowHolder.toggleButton.isChecked()) {
            thisRowHolder.toggleButton.setBackgroundDrawable(getResources().getDrawable(...);
            thisRowHolder.toggleButton.setChecked(false);
        }
        else {
            thisRowHolder.toggleButton.setBackgroundDrawable(getResources().getDrawable(...));
            thisRowHolder.toggleButton.setChecked(true);
        }
    }
});

...

---

Edit:

Also noticed this. In these two lines:

holder.colorNameText = (TextView) itemView.findViewById(R.id.color_name);
holder.toggleButton = (ToggleButton) itemView.findViewById(R.id.color_toggle);

You are finding the views in some member variable itemView, but you need to be finding them in rowView so you are getting the instances for this specific row. All your view holders are looking at the same ToggleButton instance, which may not even be on screen.

Edit 2:

One more thing you're missing. You need to store the state of the toggle buttons and reapply them. So in your OnClickListener, when you call setChecked() you must also update the backing data in colorsList. Looks like you already cached a reference to the proper list element in your ToggleButton's ID, so should be easy. Then move this block of code out of your if/else block and put it afterwards, so the toggle button is always updated to the latest data:

if (c.getSomething()>0) {
     holder.toggleButton.setBackgroundDrawable(getResource().getDrawable(...)));
     holder.setChecked(false);
  }
  else {
     holder.toggleButton.setBackgroundDrawable(getResource().getDrawable(...)));
     holder.setChecked(true);
  }

Answer 3

Your problem is listview recycling views

You have to store state of toggle button for each row of listview. Eg.Create class which stores information about each row,suppose ColorInfo which contains color and isChecked boolean. so instead of

Color c = colorsList.get(position);

it will be

ColorInfo colorInfo = colorsList.get(position);

and in getview

togglebutton.setCheck(colorInfo.isCheck)

and in onClick listener of toggle buttons you change state of object of ColorInfo for that position to toggleChecked true or false and notifyDatasetChanged,this will solve your problem.