AsyncTask is not setting correct image in correct row in ListView

Question

I am trying to make a album ListView. In there I am using AsyncTask to load the album_art image to the list thumbnail. Everything is OK but the problem is those images are not getting attached with the correct rows. And if I scroll the listview the album images are changing there place randomly. Sometime the same image is showing in multiple row, sometime there is no image at all. I have no idea why this happening. It looks like kind of thread problem. How can I solve it?

This is my code:

public class PropertyOfAlbum extends BaseAdapter {


    public static ViewHolder holder;
    private Context mContext;
    Cursor cursor;
    private FakeImageLoader mFakeImageLoader;
    private LayoutInflater layoutInflater;
    Typeface tf, tf2, tf3;
    Bitmap coverBitmap;
    //private FakeImageLoader mFakeImageLoader;

    public PropertyOfAlbum(Context context, Cursor cur) {
        super();
        mContext = context;
        cursor = cur;
        tf = Typeface.createFromAsset(context.getAssets(),
                "fonts/gang_wolfik_blade.ttf");
        tf2 = Typeface.createFromAsset(context.getAssets(),
                "fonts/gang_wolfik_blade.ttf");
        tf3 = Typeface.createFromAsset(context.getAssets(),
                "fonts/gang_wolfik_blade.ttf");
        layoutInflater = LayoutInflater.from(context);
    }

    @TargetApi(Build.VERSION_CODES.GINGERBREAD)
    @SuppressLint("NewApi")
    public View getView(final int position, View view, ViewGroup parent) {
        cursor.moveToPosition(position);
        // Using an AsyncTask to load the slow images in a background thread

        if (view == null) {
            view = layoutInflater.inflate(R.layout.album_row, null);
            holder = new ViewHolder();
            holder.title = (TextView) view.findViewById(R.id.title);
            holder.duration = (TextView) view.findViewById(R.id.duration);
            holder.artist = (TextView) view.findViewById(R.id.artist);
            holder.iv = (ImageView) view.findViewById(R.id.list_image);
            holder.col = cursor.getColumnIndexOrThrow(MediaStore.Audio.Albums.ALBUM_ART);

            view.setTag(holder);        
        }else{
            holder = (ViewHolder) view.getTag();
        }
        //mFakeImageLoader = new FakeImageLoader(cursor,holder.col);
        holder.title.setTypeface(tf);
        holder.artist.setTypeface(tf2);
        holder.duration.setTypeface(tf3);
        holder.title.setTextSize(18);
        Log.d(null, "col " + holder.col);

        holder.title.setText(cursor.getString(1));
        holder.artist.setText(cursor.getString(2));
        holder.duration.setText(cursor.getString(4));
        //coverBitmap = BitmapFactory.decodeFile(cursor.getString(holder.col));
        if(coverBitmap == null){
            holder.iv.setImageDrawable(mContext.getResources().getDrawable(R.drawable.album));
        }else{
            holder.iv.setImageBitmap(coverBitmap);
        }
        holder.position = position;
        new AsyncTask<ViewHolder, Void, Bitmap>() {
            private ViewHolder v;

            @Override
            protected Bitmap doInBackground(ViewHolder... params) {
                v = params[0];
                coverBitmap = BitmapFactory.decodeFile(cursor.getString(holder.col));
                return coverBitmap;
            }

            @Override
            protected void onPostExecute(Bitmap result) {
                super.onPostExecute(result);
                if(v.position == position){
                    if (coverBitmap==null) {
                        holder.iv.setImageDrawable(mContext.getResources().getDrawable(R.drawable.album));
                    }else{
                        holder.iv.setImageBitmap(coverBitmap);
                    }
                }
            }
        }.execute(holder);
        return view;

    }

    @Override
    public int getCount() {
        // TODO Auto-generated method stub
        return cursor.getCount();
    }

    @Override
    public Object getItem(int arg0) {
        // TODO Auto-generated method stub
        return arg0;
    }

    @Override
    public long getItemId(int arg0) {
        // TODO Auto-generated method stub
        return arg0;
    }

    public static class ViewHolder {
        TextView title;
        TextView duration;
        TextView artist;
        ImageView iv;
        int col,position;
    }

}

Answer 1

It's not a good practice to link a AsyncTask instance with a View row on the Adapter, usually, when it is made necessary, it is placed outside the getView() in another flow, because views on getView() get recycled and you lose control over what is being displayed on that view at the time the AsyncTask finishes to load.

The best advice i could give you is to forget about this approach and apply Picasso on your project. Picasso is the simplest, but powerful image loading library for Android, I promise you won't regret: http://square.github.io/picasso/

Answer 2

You only have 1 reference to coverBitmap. Each time getView() is called (e.g. every time you scroll), it's reusing some of the views (ie. convertView != null), you're setting the new background to whatever coverBitmap currently is, and then starting a new AsyncTask that will then replace coverBitmap again. Thus, the cycle continues and you're experiencing your issue.

Now, a solution for you would be to have some kind of object map the bitmap to the specific album you're referencing.

Eg.

private Map<String, Bitmap> albumImages = new HashMap<String, Bitmap>();

Now that you have something to hold your stuff, you can delete your "coverBitmap" and add your Bitmaps to albumImages inside your AsyncTask:

        @Override
        protected Bitmap doInBackground(ViewHolder... params) {
            v = params[0];
            return BitmapFactory.decodeFile(cursor.getString(holder.col));
        }

        @Override
        protected void onPostExecute(Bitmap result) {
            super.onPostExecute(result);

            // Add the returned Bitmap to our Map if it's not null
            if(result != null)  alumbImages.put(cursor.getString(holder.col), result);

            if(v.position == position){
                if (result == null) {
                    holder.iv.setImageDrawable(mContext.getResources().getDrawable(R.drawable.album));
                }else{
                    holder.iv.setImageBitmap(result);
                }
            }
        }

Now let's go back to your getView() method and replace how you're getting and setting the images, and also make sure your AsyncTask isn't fired every time that method is called:

    Bitmap albumImage = albumImages.get(cursor.getString(holder.col));
    if(albumImage == null){
        holder.iv.setImageDrawable(mContext.getResources().getDrawable(R.drawable.album));

         // Now is where you can call the AsyncTask, so that it's only called if we don't already have the image:
          new AsyncTask<ViewHolder, Void, Bitmap>() { ... }
    }
    else holder.iv.setImageBitmap(albumImage); 
    holder.position = position;

However, this is still pretty far from optimal, and I would recommend looking into using a lazy-loading library that can better handle all of the resources that holding onto a lot of images takes. This one is pretty user friendly: https://github.com/nostra13/Android-Universal-Image-Loader