CODEWITHSUNDEEP
androidandroid-recyclerview
Graeme
Graeme

Reputation: 25864

RecyclerView corrupts view using notifyItemMoved()

I'm having a problem with using the notifyItemMoved() method. It seems to be incorrectly displaying unmoved views.

My list has 4 element in it. What I want to do is animate a swap between item 1 and item 3. Items 1 and 3 swap correctly, but item 2 displays what was at item 3!

So the list starts off looking something like this:

Item 0
Item 1
Item 2
Item 3

And ends like this:

Item 0
Item 3
Item 3 <-- What the heck has this changed for?
Item 1

My adapter is backed by a List mProductList. I call the following code:

public void sortBackingListUsingSortingList(List<ProductWrapper> newProductItems) {
    Log.e("", "Before:");
    for(ProductWrapper wrapper : mProductItems) wrapper.log();
    for(int i = 0; i < newProductItems.size(); i++) {
        ProductWrapper currentItem   = mProductItems.get(i);
        ProductWrapper correctItem   = newProductItems.get(i);

        if(!currentItem.equals(correctItem)) {
            // Item in wrong place
            int indexOfCorrectItem = getIndexOfItemInList(mProductItems, correctItem);
            Collections.swap(mProductItems, i, indexOfCorrectItem);
            notifyItemMoved(i, indexOfCorrectItem);
            Log.e("", "notifyItemMoved(" + i + ", " + indexOfCorrectItem+")");
            Log.e("", "After:");
            for(ProductWrapper wrapper : mProductItems) wrapper.log();
        }
    }
}

I've also added logging to onBindViewHolder to check if my view logic is being called:

@Override
public void onBindViewHolder(HolderBasic holder, int position) {
    Log.e("", "onBindViewHolder(holder, " + position + ")");
    holder.fill(mProductItems.get(position));
}

My logs look like this:

09-02 14:39:17.853: Before:
09-02 14:39:17.853: Item 0
09-02 14:39:17.853: Item 1
09-02 14:39:17.853: Item 2
09-02 14:39:17.853: Item 3

09-02 14:39:17.854: notifyItemMoved(1, 3)

09-02 14:39:17.854: After:
09-02 14:39:17.854: Item 0
09-02 14:39:17.854: Item 3
09-02 14:39:17.854: Item 2
09-02 14:39:17.854: Item 1

09-02 14:39:17.867: onBindViewHolder(holder, 1)
09-02 14:39:17.874: onBindViewHolder(holder, 3)

As you can see, no reason for Item 2 to have change it's display at all - and yet, it does. Anybody know why?

EDIT

I can get around above by looping through the entire adapter and calling notifyItemChanged() on every item. Inefficient and not a good solution, but is invisible to the user.

Upvotes: 14

Views: 10260

Answers (6)

Denny Mathew
Denny Mathew

Reputation: 853

This method worked for me, but I see a very slight jitter in animation. In case if there is a better solution do let me know :)

 public void moveItem(int oldPos, int newPos) {
    Uri item = mDataSet.get(oldPos);
    mDataSet.remove(oldPos);
    mDataSet.add(newPos, item);
    notifyItemMoved(oldPos, newPos);

    notifyItemRangeChanged(0, mDataSet.size());
}

Upvotes: 1

Bastien Grignon
Bastien Grignon

Reputation: 90

I had the same issue and I actually handled it differently too, with my way, the animation will stay the same and you won't have any trouble of items at wrong positions anymore :

@Override
public void onRowMoved(int fromPosition, int toPosition) {
    if (fromPosition < toPosition) {
        for (int i = fromPosition; i < toPosition; i++) {
            Collections.swap(listOfItem, i, i + 1);
        }
    } else {
        for (int i = fromPosition; i > toPosition; i--) {
            Collections.swap(listOfItem, i, i - 1);
        }
    }
    notifyItemMoved(fromPosition, toPosition);
}

With this, you can reorder items without any issues

Tutorial used to fix my issue

Upvotes: 1

Faisal
Faisal

Reputation: 772

Well, I handled it in a slightly different way, might help others. 

        Collections.swap(mItemList, fromPosition, toPosition);
        // Need to do below, because NotifyItemMove only handle one sided move
        Item fromItem = mItemList.get(fromPosition);
        Item toItem = mItemList.get(toPosition);
        notifyItemChanged(fromPosition, toItem);
        notifyItemChanged(toPosition, fromItem);

I had to reorder items on a grid, and save the positions to a file. @Graeme was right, but i didn't wanted to give up on swapping. So just like @saganaut i sticked to notifyItemChanged. But only using notifyItemChanged sometimes left both swapped items on my grid with same items, so I binded the items with notifyItemChanged. It is not killing the animation, and is working as expected.

Upvotes: 0

lenooh
lenooh

Reputation: 10682

To get the actual position of your item after a drag&drop, add this method to your adapter:

private int getItemPosition(Item item){ // (<-- replace with your item)
    int i = 0;
    // (replace with your items and methods here)
    for (Item currentItem : mItems) {
        if (currentItem.getItemId() == item.getItemId()) break;
        i++;
    }
    return i;
}

and call this instead of the position given by the viewHolder.

Upvotes: 0

walex
walex

Reputation: 17

I had the same issue. RecyclerView-Items are corrupt on drag&drop. But I have found a simple solution: In your RecyclerView.Adapter.class be sure to have the following

@Override
public long getItemId(int position) {
    // here code for getting the right itemID, 
    // i.e. return super.getItemId(mPosition);
    // where mPosition ist the Position in the Collection.
}

You must return the right itemID for the position. From now on the Items are not corrupt.

Upvotes: 1

Graeme
Graeme

Reputation: 25864

Thank you to @david.mihola for leading me to what I'm doing wrong.

This took so long to figure out as the symptom didn't make the problem obvious!

I was doing this:

Collections.swap(mProductItems, i, indexOfCorrectItem);
notifyItemMoved(i, indexOfCorrectItem)

But, I obviously didn't think through what notifyItemMoved() was actually doing. It is only notifying the adapter that item i has moved to indexOfCorrectItem it isn't telling the adapter that indexOfCorrectItem has also moved to i.

Under the covers it was doing the following:

  1. Move item 1 to 3
  2. Move what was at 2 to 1 to fill the gap
  3. Move what was at 3 to 2 to fill the gap
  4. notifyItemChanged(1);
  5. notifyItemChanged(3);

The above of course leaves item 3 moved down to item 2 without a refreshed view! It was steps 4 and 5 which were hiding the problem by making item1 and item3 display correctly and leaving item2 incorrect!

As soon as I realised this I tried the following code:

notifyItemMoved(indexOfCorrectItem, i);
notifyItemMoved(i, indexOfCorrectItem);

This left the list in the correct order, but it short circuited the animation.

So, instead, I dumped swapping altogether:

mProductItems.remove(indexOfCorrectItem);
mProductItems.add(i, correctItem);
notifyItemMoved(indexOfCorrectItem, i);

Upvotes: 27

Related Questions