Android, List Adapter returns wrong position in getView

Question

I have found a mysterious problem that may be a bug! I have a list in my fragment. Each row has a button. List shouldn't respond to click however buttons are clickable.

In order to get which button has clicked I have created a listener and implement it in my fragment. This is the code of my adapter.

public class AddFriendsAdapter extends BaseAdapter {

    public interface OnAddFriendsListener {
        public void OnAddUserClicked(MutualFriends user);
    }

    private final String TAG = "*** AddFriendsAdapter ***";

    private Context context;
    private OnAddFriendsListener listener;
    private LayoutInflater myInflater;
    private ImageDownloader imageDownloader;
    private List<MutualFriends> userList;

    public AddFriendsAdapter(Context context) {
        this.context = context;
        myInflater = LayoutInflater.from(context);

        imageDownloader = ImageDownloader.getInstance(context);
    }

    public void setData(List<MutualFriends> userList) {
        this.userList = userList;

        Log.i(TAG, "List passed to the adapter.");
    }

    @Override
    public int getCount() {
        try {
            return userList.size();
        } catch (Exception e) {
            e.printStackTrace();
            return 0;
        }
    }

    @Override
    public Object getItem(int position) {
        return null;
    }

    @Override
    public long getItemId(int position) {
        return position;
    }

    @Override
    public View getView(final int position, View convertView, ViewGroup parent) {
        ViewHolder holder;

        if (convertView == null) {
            convertView = myInflater.inflate(R.layout.list_add_friends_row, null);
            holder = new ViewHolder();

            Typeface font = Typeface.createFromAsset(context.getAssets(), "fonts/ITCAvantGardeStd-Demi.ttf");
            holder.tvUserName = (TextView) convertView.findViewById(R.id.tvUserName);
            holder.tvUserName.setTypeface(font);
            holder.ivPicture = (ImageView) convertView.findViewById(R.id.ivPicture);
            holder.btnAdd = (Button) convertView.findViewById(R.id.btnAdd);
            holder.btnAdd.setOnClickListener(new View.OnClickListener() {
                @Override
                public void onClick(View v) {
                    Log.e(TAG, "Item: " + position);
                    listener.OnAddUserClicked(userList.get(position));
                }
            });

            convertView.setTag(holder);
        } else {
            holder = (ViewHolder) convertView.getTag();
        }

        holder.tvUserName.setText(userList.get(position).getName());
        imageDownloader.displayImage(holder.ivPicture, userList.get(position).getPhotoUrl());

        return convertView;
    }

    public void setOnAddClickedListener(OnAddFriendsListener listener) {
        this.listener = listener;
    }

    static class ViewHolder {
        TextView tvUserName;
        ImageView ivPicture;
        Button btnAdd;
    }
}

When I run the app, I can see my rows however since my list is long and has over 200 items when i goto middle of list and click an item then returned position is wrong (it's something like 7, sometimes 4 and etc.).

Now what is the mystery? If I active on item listener of list from my fragment and click on row then correct row position will be displayed while on that row if I click on button then wrong position will be displayed.

listView.setOnItemClickListener(new AdapterView.OnItemClickListener() {
            @Override
            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
                Log.e(TAG, "item " + position + " clicked.");
            }
        });

Result in logcat:

05-09 10:22:25.228: E/AddFriendsFragment(20296): item 109 clicked.
05-09 10:22:34.453: E/*** AddFriendsAdapter ***(20296): Item: 0

Any suggestion would be appreciated. Thanks

Biography

Where are you implementing OnAddUserClicked() ? Also if you are using a BaseAdapter and List<T> you could simplify your adapter by using ArrayAdapter instead since it essentially is a List and is built to solve the exact problem of loading an Array or List of data objects into an AdapterView – Cod

Answer 1

Because the convertView and holder will be recycled to use, move your setOnClickListener out of the if else statement:

    if (convertView == null) {
        convertView = myInflater.inflate(R.layout.list_add_friends_row, null);
        holder = new ViewHolder();

        Typeface font = Typeface.createFromAsset(context.getAssets(), "fonts/ITCAvantGardeStd-Demi.ttf");
        holder.tvUserName = (TextView) convertView.findViewById(R.id.tvUserName);
        holder.tvUserName.setTypeface(font);
        holder.ivPicture = (ImageView) convertView.findViewById(R.id.ivPicture);
        holder.btnAdd = (Button) convertView.findViewById(R.id.btnAdd);
        convertView.setTag(holder);
    } else {
        holder = (ViewHolder) convertView.getTag();
    }
    holder.btnAdd.setOnClickListener(new View.OnClickListener() {
         @Override
         public void onClick(View v) 
                Log.e(TAG, "Item: " + position);
                listener.OnAddUserClicked(userList.get(position));
            }
        });

It is not the best solution for that,because there will be some performance issue. I suggest you create a Map for your view and create a new view for your item then just use the relative view for each view.

I think it will be a better solution with best performance:

@Override
public View getView(final int position, View convertView, ViewGroup parent) {
    ViewHolder holder;

    if (convertView == null) {
        convertView = myInflater.inflate(R.layout.list_add_friends_row, null);
        holder = new ViewHolder();

        Typeface font = Typeface.createFromAsset(context.getAssets(), "fonts/ITCAvantGardeStd-Demi.ttf");
        holder.tvUserName = (TextView) convertView.findViewById(R.id.tvUserName);
        holder.tvUserName.setTypeface(font);
        holder.ivPicture = (ImageView) convertView.findViewById(R.id.ivPicture);
        holder.btnAdd = (Button) convertView.findViewById(R.id.btnAdd);
        holder.btnAdd.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                Integer pos = (Integer)v.getTag();
                Log.e(TAG, "Item: " + pos);
                listener.OnAddUserClicked(userList.get(pos));
            }
        });

        convertView.setTag(holder);
    } else {
        holder = (ViewHolder) convertView.getTag();
    }

    holder.tvUserName.setText(userList.get(position).getName());
    imageDownloader.displayImage(holder.ivPicture, userList.get(position).getPhotoUrl());
    holder.btnAdd.setTag(position);
    return convertView;
}

You can also manage your view by yourself. Create every unique view for your item, don't recycle view.

//member various
private Map<Integer, View> myViews = new HashMap<Integer, View>(); 

@Override
public View getView(final int position, View convertView, ViewGroup parent) {
    ViewHolder holder;
    View view = myViews.get(position);
    if (view == null) {
        view = myInflater.inflate(R.layout.list_add_friends_row, null);
        //don't need use the holder anymore.

        Typeface font = Typeface.createFromAsset(context.getAssets(), "fonts/ITCAvantGardeStd-Demi.ttf");
        holder.tvUserName = (TextView) convertView.findViewById(R.id.tvUserName);
        holder.tvUserName.setTypeface(font);
        holder.ivPicture = (ImageView) convertView.findViewById(R.id.ivPicture);
        holder.btnAdd = (Button) convertView.findViewById(R.id.btnAdd);
        holder.btnAdd.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                Integer pos = (Integer)v.getTag();
                Log.e(TAG, "Item: " + pos);
                listener.OnAddUserClicked(userList.get(pos));
            }
        });

       holder.tvUserName.setText(userList.get(position).getName());
       imageDownloader.displayImage(holder.ivPicture,  
                userList.get(position).getPhotoUrl());
       myViews.put(position, view);

    }
    return view;
}

Navarino

I don't think where you set the click listener will matter since it should get different values for position is subsequent calls. And thus it will be doing the same thing just passing along a different user object based on the position that was passed to getView(). – Cod

Exactly, buptcoder is right. When i moved it out then both of list item listener and my listener returned same position. Thanks again :) Please give me a sample if you have about your suggestion. – Biography

Because we write set position in the statements if (convertView == null) {, so the position will be not set if convertView not null. it will happened every time when listview recycle the convertview. At lest we need put the set Position out of the is else statement so that we can get the right position. – Navarino

Thanks buptcoder, regard to "I suggest you create a Map for your view...", since it's new to me if you know a reference please share it with me. Because you said this way (moving that part of code to outside of if/elss) has performance issue. I want to investigate what issue it might be have. Thanks again ;) – Biography

I mean you can manager the view by yourself instend of use the recycle mechanism. I will put the example code in my post. – Navarino

How would you populate the map? I have two views that are for ads and then the rest that are standard items. I need to keep track of where the ads are in the list so a Map solution works for me. Just don't know how you populated it in your solution. – Shipentine

Answer 2

Did you tried doing something like this:

holder.btnAdd.setTag(Integer.valueOf(position));

And then retrieve wich row was clicked in the callback for the button, like this:

public void btnAddClickListener(View view)
    {
        position = (Integer)view.getTag();
        Foo foo = (Foo)foos_adapter.getItem(position);  //get data of row(position)
        //do some
    }

Unaccompanied

Strange, that after three years and two similar decisions it is the first upvote for this answer. Also this is universal solution, as we can create one object onClickListener = new View.OnClickListener(...) and assign it to any button (if many). – Turco

Answer 3

Another approach I found useful (if you are using the ViewHolder pattern of course) is to set the index on a separate attribute whenever getView() is called, then inside your onClickListener you just have to reference your holder's position attribute, something like this:

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

    final ViewHolder holder;

    if(convertView == null){

        convertView = View.inflate(mContext, R.layout.contact_picker_row,null);

        holder = new ViewHolder();

        holder.body = (RelativeLayout)convertView.findViewById(R.id.numberBody);

        convertView.setTag(holder);

    }else{

        holder = (ViewHolder)convertView.getTag();

    }

    holder.position = position;

    holder.body.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {

            Toast.makeText(mContext,"Clicked on: "+holder.position,Toast.LENGTH_LONG).show();

        }
    });

    return convertView;
}

private class ViewHolder{

    RelativeLayout body;
    int position;

}

Aricaarick