ListView with ArrayAdapter and ViewHolder adding icons to the wrong item

Question

I have a dynamic ListView which uses an ArrayAdapter. When a name is selected from a spinner, the name together with an icon showing whether they are male or female gets added to the ListView.

Mostly everything is good (the name gets added to the list correctly, together with an icon). But the icon showing the sex gets added to the wrong item in the ListView. The name gets added to the bottom of the list, but the icon gets placed at the name at the top of the list. I don't know if it's the way I'm using ViewHolder but there is zero documentation on it in the Android website.

// Listview inflater
inflater = (LayoutInflater) (this).getSystemService(LAYOUT_INFLATER_SERVICE);

// List Array.
mAdapter = new ArrayAdapter<String>(this, R.layout.player_simple_list, 
                                                 R.id.label, mStrings) {

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

        Log.i("ANDY","View getView Called");
        // A ViewHolder keeps references to children views to 
        // avoid unneccessary calls to findViewById() on each row.
        ViewHolder holder;

        if (null == convertView) {
            Log.i("ANDY","Position not previously used, so inflating");
            convertView = inflater.inflate(R.layout.player_simple_list, null);
            // Creates a ViewHolder and store references to the
            // two children views we want to bind data to.
            holder = new ViewHolder();
            holder.text = (TextView) convertView.findViewById(R.id.label);
            holder.icon = (ImageView) convertView.findViewById(R.id.icon);
            if (sexmale == true) {
                holder.icon.setImageBitmap(maleicon);
            }
            else {
                holder.icon.setImageBitmap(femaleicon);
            }
            convertView.setTag(holder);
        } else {
            // Get the ViewHolder back to get fast access to the TextView
            // and the ImageView.
            holder = (ViewHolder) convertView.getTag();

        }
        // Bind the data efficiently with the holder.
        holder.text.setText(getItem(position));
        // Change icon depending is the sexmale variable is true or false.
        Log.i("ANDY","getCount = "+mAdapter.getCount());
        return convertView;
    }
};
setListAdapter(mAdapter);

Solution 1

Update: ViewHolder is only meant to hold references to the component views inside the item layout. This helps to avoid the overhead of calling findViewById for rendering each component inside complex item layouts with multiple components(Like the TextView, and ImageView in this case).

I fixed it by using a routine (called getSex) to retrieve the sex data and setting all the view data including icons outside the if-else blocks.

The working code now looks like this:

if (null == convertView) {
    Log.i("ANDY","Position not previously used, so inflating");
    convertView = inflater.inflate(R.layout.player_simple_list, null);

    // Creates a ViewHolder and store references to the two children views
    // we want to bind data to.
    holder = new ViewHolder();
    holder.text = (TextView) convertView.findViewById(R.id.label);
    holder.icon = (ImageView) convertView.findViewById(R.id.icon);
    convertView.setTag(holder);
} else {
    // Get the ViewHolder back to get fast access to the TextView
    // and the ImageView.
    holder = (ViewHolder) convertView.getTag();
}

// Bind the data efficiently with the holder.
holder.text.setText(getItem(position));
// Change icon depending is the sexmale variable is true or false.
if (getSex (getItem(position)) == true)  {
    holder.icon.setImageBitmap(maleicon);
}
else {
    holder.icon.setImageBitmap(femaleicon);
}
return convertView;

Solution 2

You have to set the icons after if-else-if for creating or binding a holder. Otherwise, the icons would be rightly displayed only in first few items in the list i.e until the ListView is not filled.

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

    Log.i("ANDY","View getView Called");
    // A ViewHolder keeps references to children views
    // to avoid unneccessary calls to findViewById() on each row.
    ViewHolder holder;

        if (null == convertView) {
            Log.i("ANDY","Position not previously used, so inflating");
            convertView = inflater.inflate(R.layout.player_simple_list, null);

            // Creates a ViewHolder and store references to
            // the two children views we want to bind data to.
            holder = new ViewHolder();
            holder.text = (TextView) convertView.findViewById(R.id.label);
            holder.icon = (ImageView) convertView.findViewById(R.id.icon);
            convertView.setTag(holder);
        } else {
            // Get the ViewHolder back to get fast access to the TextView
            // and the ImageView.
            holder = (ViewHolder) convertView.getTag();

        }
        // Bind the data efficiently with the holder.
        holder.text.setText(getItem(position));

        // Change icon depending is the sexmale variable is true or false.
        if (sexmale == true) {
            holder.icon.setImageBitmap(maleicon);
        }
        else {
            holder.icon.setImageBitmap(femaleicon);
        }
        Log.i("ANDY","getCount = "+mAdapter.getCount());
        return convertView;
}

Solution 3

You have to move from the if a few lines of data after the comment, as in this question is explained

// Bind the data efficiently with the holder.

so it will look like this

if (null == convertView) {
    Log.i("ANDY","Position not previously used, so inflating");
    convertView = inflater.inflate(R.layout.player_simple_list, null);
    // Creates a ViewHolder and store references to the two children views
    // we want to bind data to.
    holder = new ViewHolder();
    convertView.setTag(holder);
} else {
    // Get the ViewHolder back to get fast access to the TextView
    // and the ImageView.
    holder = (ViewHolder) convertView.getTag();
}

// Bind the data efficiently with the holder.
holder.text = (TextView) convertView.findViewById(R.id.label);
holder.icon = (ImageView) convertView.findViewById(R.id.icon);
if (sexmale == true) {
    holder.icon.setImageBitmap(maleicon);
}
else {
    holder.icon.setImageBitmap(femaleicon);
}
holder.text.setText(getItem(position));

Comments

[cristis]

I disagree, this will just make the ViewHolder useless since you are overwriting it every time. The correct solution should set holder.text and holder.icon within the if branch and set the contents (setText, setImageBitmap) outside the if block.

[Pentium10]

I think the overwrite needs to be there because if not, it will hold data related to some other record position. For example you display record 10, and if will reuse the cache view from record 3, if you don't overwrite the data set for record 3 will be visible for record 10.

[CommonsWare]

@Pentium10: no, cristis is correct. The ViewHolder is tied to the row, so which widgets it holds are not changing. What needs to change simply is the contents of those widgets.

[andy_spoo]

Thanks for the quick reply, much appreciated. If I understand you correctly, then I get the same result as my comment below. All the icons become the same sex all down the list.

[andy_spoo]

Hi. I tried this, but all the icons change to the same type, all down the list. i.e. If a male name selected, all the icons become male.

[Pentium10]

I don't see that you ever change sexmale variable, so that always is the same.

[andy_spoo]

The sexmale variable gets changed else where in the program. I think that the problem is that I don't know how to retrieve the stored image/label. I need the image equivalent of the text: holder.text.setText(getItem(position)); // set the text from the item located at the current position. The example given in List14.java is: holder.icon.setImageBitmap((position & 1) == 1 ? mIcon1 : mIcon2); I think I just need a relevant equivalent of this line.