Recyclerview items mixing up on refresh

Question

Possible Duplicate RecyclerView Mixing Up Items

I have a Recyclerview that i populate with items through an adapter and a List. The problem is, on refreshing(via SwipeRefreshLayout), data for a countdown timer mixes up and the wrong value shows up on another item as highlighted in the image below. Item 1 highlighted timer is supposed to show 'Expired' but on refresh instead it takes the timer value for item 2

Here is my onBindViewHolder code in my adapter. Am i implementing it correctly?

public class FeedAdapter extends RecyclerView.Adapter<FeedAdapter.FeedModelViewHolder> {
private List<FeedModel> feedModelList;
private MainFeedListener listener;
private Context mContext;
String cPrice;

public FeedAdapter(List<FeedModel>feedModelList, Context context, MainFeedListener bidFeedListener) {
    this.feedModelList = feedModelList;
    this.mContext = context;
    this.listener = bidFeedListener;
}

@NonNull
@Override
public FeedModelViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
    View v = LayoutInflater.from(parent.getContext()).inflate(R.layout.item_bid_feed, parent, false);
    return new FeedModelViewHolder(v);
}
@Override
public void onBindViewHolder(@NonNull final FeedModelViewHolder holder, final int position) {
    final FeedModel feedModel = feedModelList.get(position);
    ((TextView) holder.bidView.findViewById(R.id.bid_title)).setText(feedModel.getTitle());
    ((TextView) holder.bidView.findViewById(R.id.start_price)).setText(feedModel.getCurrency() +" "+Convert(feedModel.getStartPrice()));
    ((TextView) holder.bidView.findViewById(R.id.tags)).setText(feedModel.getTags());
    ((TextView) holder.bidView.findViewById(R.id.location)).setText(feedModel.getLocation());


    new CountDownTimer(feedModel.getDeadline(), 1000) {
        public void onTick(long millisUntilFinished) {
            ((TextView) holder.bidView.findViewById(R.id.deadline)).setText(formatMilliSecondsToTime( millisUntilFinished));
            feedModel.setTime(millisUntilFinished);
        }
        public void onFinish() {
            ((TextView) holder.bidView.findViewById(R.id.deadline)).setText("EXPIRED");
        }

    }.start();

    if (!TextUtils.isEmpty(feedModel.getImageUrl())) {
        Glide.with(mContext).load(feedModel.getImageUrl())
                .thumbnail(0.5f)
                .crossFade()
                .diskCacheStrategy(DiskCacheStrategy.ALL)
                .into((ImageView) holder.bidView.findViewById(R.id.bid_thumbnail));
     }
    ((LinearLayout) holder.bidView.findViewById(R.id.article_card_root)).setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View view) {
           listener.onBidRowClicked(feedModel);
        }
    });
}


@Override
public int getItemCount() {
    return feedModelList.size();
}

public static class FeedModelViewHolder extends RecyclerView.ViewHolder {
    private View bidView;
    public FeedModelViewHolder(View v) {
        super(v);
        bidView = v;
    }
}


String Convert(Double d){
    int i;
    d +=0.005;
    i= (int) (d*100);
    Double b = (double) (i / 100);
    return b.toString();
}    

public static  String formatMilliSecondsToTime(long milliseconds) {

    int seconds = (int) (milliseconds / 1000) % 60;
    int minutes = (int) ((milliseconds / (1000 * 60)) % 60);
    int hours = (int) ((milliseconds / (1000 * 60 * 60))); //
  //  int days = (int) ((milliseconds / (1000 * 60 * 60)) % 24); //
    return twoDigitString(hours) + ":" + twoDigitString(minutes) + ":"
            + twoDigitString(seconds);
}

private static String twoDigitString(long number) {

    if (number == 0) {
        return "00";
    }

    if (number / 10 == 0) {
        return "0" + number;
    }

    return String.valueOf(number);
}

}

I populate the list using a Volley library and pass the list to the adapter through the constructor

score 2 · Answer 1 · answered Jul 17 '18 at 07:16

Please change your holder class to this:

 public static class FeedModelViewHolder extends RecyclerView.ViewHolder {
        private View bidView;
        private TextView title;
        private TextView price;
        private TextView tags;
        private TextView location;
       public FeedModelViewHolder(View v) {
            super(v);
        title = v.findViewById(R.id.bid_title);
        price = v.findViewById(R.id.start_price)
        tags = v.findViewById(R.id.tags)
        location= v.findViewById(R.id.location);
        bidView = v;
        }
    }

On BindViewHolder

@Override
public void onBindViewHolder(@NonNull final FeedModelViewHolder holder, final int position) {
    final FeedModel feedModel = feedModelList.get(position);
   holder,title.setText(feedModel.getTitle());
    holder.price.setText(feedModel.getCurrency() +" "+Convert(feedModel.getStartPrice()));
    holder.tags.setText(feedModel.getTags());
    holder.location.setText(feedModel.getLocation());
}

Hello @faran, am i correct in concluding this approach is mostly for readability? — tendai, Jul 17 '18 at 07:31
Yeah!But ViewHolder Pattern will also do this to prevents findViewById() to being called lots of times uselessy, keeping the views on a static reference, it is a good pattern to save some resources (expecially when you need to reference many views in your listview items). — faran.javed, Jul 17 '18 at 07:44
Thank you very much for the informative description and help — tendai, Jul 18 '18 at 05:28

score 1 · Accepted Answer · answered Jul 17 '18 at 07:13

1

This problem is simple.

RecyclerView reuses the holders, calling bind each time to update the data in them.

Since you create a countdown timer each time any data is bound, you will end up with multiple timers updating the same view holder.

The best thing here would be to move the countdown timer in the FeedViewHolder as a reference, cancel it before binding the data (if started) and rescheduling to the desired duration.

public void onBindViewHolder(final FeedViewHolder holder, final int position) {
...
if (holder.timer != null) {
    holder.timer.cancel();
}
holder.timer = new CountDownTimer(expiryTime, 500) {
    ...
}.start();
 }

public static class FeedViewHolder extends RecyclerView.ViewHolder {
...
CountDownTimer timer;

public FeedViewHolder(View itemView) {
    ...
 }
 }

answered Jul 17 '18 at 07:13

hasan_shaikh

1,434
1
15
38

Awesome, this means i need to cancel the timer before every refresh, correct ? – tendai Jul 17 '18 at 07:30
It works, i'm comparing it to @Viktor 's for what i think is the best approach before i accept – tendai Jul 17 '18 at 07:44
This method is the simplest while effective solution to my problem. Thank you very much for educating me. – tendai Jul 17 '18 at 09:30

Viktor Stojanov · Answer 3 · 2018-07-17T08:14:51.970

You need to utilise the onCreateViewHolder more. First(not a real issue, but a tip for readability) bind your views to fields

public class FeedModelViewHolder extends RecyclerView.ViewHolder {

    TextView bigTitle;
    TextView deadline;
    ...

    public FeedModelViewHolder(View v) {
        super(v);

        bitTitle =  = (TextView) v.findViewById(R.id.bid_title);
        deadline =  = (TextView) v.findViewById(R.id.deadline);
        ...

        new CountDownTimer(feedModel.getDeadline(), 1000) {
            public void onTick(long millisUntilFinished) {
                dealine.setText(formatMilliSecondsToTime( millisUntilFinished));
                feedModelList.get(getAdapterPosition()).setTime(millisUntilFinished);
            }
            public void onFinish() {
                dealine.setText("EXPIRED");
            }

        }.start();
    }
}

and then initialise your timer again in onCreateViewHolder since it's called only once. The onBindViewHolder is called at start and each time it is being redrawn.

This approach however starts the timer when the view is being created and may not produce desirable results. It's advisable to track this times somewhere out of the Adapter and just pass a reference to them.

placing the `CountDownTimer()` method in the FeedModelViewHolder() would require a couple of adjustments like making the list static or the FeedModelViewHolder non static etc, is this correct? — tendai, Jul 17 '18 at 08:11
The `FeedModelViewHolder` doesn't actually needs to be static. I updated the answer removing the `static` type - https://stackoverflow.com/questions/40584424/simple-android-recyclerview-example — Viktor Stojanov, Jul 17 '18 at 08:14

Recyclerview items mixing up on refresh

3 Answers3