Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Move animation for StaggeredGridLayoutManager is broken when using DefaultItemAnimator

Tags:

android

I have a very straight forward implementation using StaggeredGridLayoutManager.

I want to achieve move animation based on DefaultItemAnimator

Switch 3rd item with 4th item

I try to switch 3rd item with 4th item as you can see in the following screen record. It works pretty well. Animation only involves 3rd and 4th item.

enter image description here

Switch 1st item with 2nd item

However, when I try to switch 1st item with 2nd item, the behavior is somehow broken.

I expect animation only happen between 1st item and 2nd item. However, it seems that the entire list is being animated, even though every item is having same width and height. (Hence, no gap filling action should be performed)

Everytime, after the switching, you need to scroll the RecyclerView, in order for the switched item to be visible.

enter image description here


What I had tried out

  1. Use

    staggeredGridLayoutManager.setGapStrategy( StaggeredGridLayoutManager.GAP_HANDLING_NONE); 
    

    will not help.

In order to know what happens behind the scene, I try to use the following DefaultItemAnimator with logging.

public class DebugDefaultItemAnimator extends DefaultItemAnimator {
    @Override
    public boolean animateRemove(RecyclerView.ViewHolder holder) {
        Log.i("CHEOK", "animateRemove");
        return super.animateRemove(holder);
    }

    @Override
    public boolean animateAdd(RecyclerView.ViewHolder holder) {
        Log.i("CHEOK", "animateAdd");
        return super.animateAdd(holder);

    }

    @Override
    public boolean animateMove(RecyclerView.ViewHolder holder, int fromX, int fromY, int toX, int toY) {
        Log.i("CHEOK", "animateMove (" + fromX + "," + fromY + ") to (" + toX + "," + toY + ")");
        return super.animateMove(holder, fromX, fromY, toX, toY);
    }

    @Override
    public boolean animateChange(RecyclerView.ViewHolder oldHolder, RecyclerView.ViewHolder newHolder, int fromLeft, int fromTop, int toLeft, int toTop) {
        Log.i("CHEOK", "animateChange");
        return super.animateChange(oldHolder, newHolder, fromLeft, fromTop, toLeft, toTop);
    }
}

When we switch between 3rd and 4th, everything is OK and here's the logs.

animateMove (24,250) to (564,250)
animateMove (564,250) to (24,250)

But when we switch between 1st item and 2nd item, things are broken and here's the log.

animateMove (24,928) to (564,702)
animateMove (564,476) to (24,476)
animateMove (24,1154) to (564,928)
animateMove (24,476) to (564,250)
animateMove (564,1154) to (24,1154)
animateMove (564,250) to (24,250)
animateMove (564,702) to (24,702)
animateMove (564,928) to (24,928)
animateRemove
animateMove (24,702) to (564,476)
animateMove (24,1380) to (564,1154)
animateMove (24,250) to (564,24)
animateAdd
animateMove (564,1380) to (24,1380)

My implementation is very straight forward.

Any idea what could went wrong? I try to replace StaggeredGridLayoutManager with LinearLayoutManager and GridLayoutManager. All works well except for StaggeredGridLayoutManager.

The source code is as follow. (The complete source code can be downloaded from https://github.com/yccheok/StaggeredGridLayoutManagerProblem)


Adapter.java

public class Adapter extends RecyclerView.Adapter<Adapter.ViewHolder> {

    private List<Data> datas;

    public static class ViewHolder extends RecyclerView.ViewHolder {
        // each data item is just a string in this case
        public final TextView titleTextView;
        public final TextView bodyTextView;
        public ViewHolder(View view) {
            super(view);

            titleTextView = view.findViewById(R.id.title_text_view);
            bodyTextView = view.findViewById(R.id.body_text_view);
        }
    }

    public Adapter(List<Data> datas) {
        this.datas = datas;

        setHasStableIds(true);
    }

    @Override
    public long getItemId(int position) {
        return datas.get(position).id;
    }

    public Adapter.ViewHolder onCreateViewHolder(ViewGroup parent,
                                                   int viewType) {
        // create a new view
        View view = LayoutInflater.from(parent.getContext())
                .inflate(R.layout.item, parent, false);

        ViewHolder vh = new ViewHolder(view);
        return vh;
    }

    @Override
    public void onBindViewHolder(ViewHolder holder, int position) {
        holder.titleTextView.setText(datas.get(position).title);
        holder.bodyTextView.setText(datas.get(position).body);

    }

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

MainActivity.java

public class MainActivity extends AppCompatActivity {

    private RecyclerView recyclerView;

    private List<Data> datas = new ArrayList<>();
    private Adapter adapter;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        recyclerView = findViewById(R.id.recycler_view);

        StaggeredGridLayoutManager staggeredGridLayoutManager = new StaggeredGridLayoutManager(2, StaggeredGridLayoutManager.VERTICAL);

        //staggeredGridLayoutManager.setGapStrategy(StaggeredGridLayoutManager.GAP_HANDLING_NONE);

        this.recyclerView.setLayoutManager(staggeredGridLayoutManager);

        datas.add(new Data(0, "A title", "A body"));
        datas.add(new Data(1, "B title", "B body"));
        datas.add(new Data(2, "C title", "C body"));
        datas.add(new Data(3, "D title", "D body"));
        datas.add(new Data(4, "E title", "E body"));
        datas.add(new Data(5, "F title", "F body"));
        datas.add(new Data(6, "G title", "G body"));
        datas.add(new Data(7, "H title", "H body"));
        datas.add(new Data(8, "I title", "I body"));
        datas.add(new Data(9, "J title", "J body"));
        datas.add(new Data(10, "K title", "K body"));
        datas.add(new Data(11, "L title", "L body"));
        datas.add(new Data(12, "M title", "M body"));
        datas.add(new Data(13, "N title", "N body"));
        datas.add(new Data(14, "O title", "O body"));
        adapter = new Adapter(datas);

        recyclerView.setAdapter(adapter);
    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        MenuInflater inflater = getMenuInflater();
        inflater.inflate(R.menu.menu, menu);
        return true;
    }

    @Override
    public boolean onOptionsItemSelected(MenuItem item) {
        // Handle item selection
        switch (item.getItemId()) {
            case R.id.debug:

                Data data0 = datas.get(0);
                Data data1 = datas.get(1);

                datas.set(0, data1);
                datas.set(1, data0);

                adapter.notifyItemMoved(0, 1);

                return true;

            case R.id.debug2:

                Data data2 = datas.get(2);
                Data data3 = datas.get(3);

                datas.set(2, data3);
                datas.set(3, data2);

                adapter.notifyItemMoved(2, 3);

                return true;

            default:
                return super.onOptionsItemSelected(item);
        }
    }
}

Data.java

public class Data {
    public final int id;
    public final String title;
    public final String body;

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;

        Data data = (Data) o;

        if (id != data.id) return false;
        if (title != null ? !title.equals(data.title) : data.title != null) return false;
        return body != null ? body.equals(data.body) : data.body == null;
    }

    @Override
    public int hashCode() {
        int result = id;
        result = 31 * result + (title != null ? title.hashCode() : 0);
        result = 31 * result + (body != null ? body.hashCode() : 0);
        return result;
    }

    public Data(int id, String title, String body) {

        this.id = id;
        this.title = title;
        this.body = body;
    }
}

Important Note

Please don't suggest using notifyItemRangeChanged or notifyItemChanged. As, this is a "move" operation, not "change" operation.

If you use DiffUtil for the above case, notifyItemMoved will definitely be fired.

I already updated the code in GitHub, to prove DiffUtil will fire notifyItemMoved. Hence, DiffUtil will fail too as far as animation is concern - https://github.com/yccheok/StaggeredGridLayoutManagerProblem/commit/cfa2bc9659f11e52dcee97ce8e78dcfcb6ad5e8c

I'm more interested to know why notifyItemMoved doesn't work for the above case, and how to make it work.


Conclusion

I filed a bug report in https://issuetracker.google.com/issues/78373192 . Please star it if you would like to see it to be solved.

like image 759
Cheok Yan Cheng Avatar asked Apr 22 '18 05:04

Cheok Yan Cheng


4 Answers

This looks like a bug in StaggeredGridLayoutManager. I have taken your app and made a few modifications to it for the following demo. Consider the following that makes this point:

enter image description here

As you can see, initially everything works as expected: Items move and the animations are as they should be when the screen is not full. However, when we add item "M", everything goes haywire as you have presented. My conclusion is that this is a bug. It looks like a boundary problem related to gap management but that is just my guess.

I have further confirmed this as a layout issue and can demonstrate that while the order of the children of the RecyclerView is correct, StaggeredGridLayout fails to layout the children in the same order. When you scroll, you are forcing the layout manager to take another look at the layout and then it gets it right although there are no changes to the adapter.

I will add that this is not an issue with the item animator. If you set the animation off (recyclerView.setItemAnimator(null);) the problem persists.

One workaround is to customize ListUpdateCallback to capture the problem situation and do something different. In the code below, the onMoved() method of MyListUpdateCallback looks for a move to position 0 and call notifyItemChanged on the from the to positions; otherwise, processing proceeds as normal.

Here is the app with the fix applied:

enter image description here

While this solution addresses the issues with the MCVE that you offered, it may not address the issue in the real app exactly as written. If it doesn't, I believe that this approach can be adapted to work.

MainActivity.java

This is the updated code for the demo. Set the boolean mDoTheFix to true to apply the fix. If mDoTheFix is false, the app will exhibit the broken behavior.

public class MainActivity extends AppCompatActivity implements View.OnClickListener {

    private RecyclerView recyclerView;

    private List<Data> datas = new ArrayList<>();
    private Adapter adapter;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        recyclerView = findViewById(R.id.recycler_view);

        StaggeredGridLayoutManager staggeredGridLayoutManager =
            new StaggeredGridLayoutManager(2, StaggeredGridLayoutManager.VERTICAL) {
                @Override
                public void onLayoutCompleted(RecyclerView.State state) {
//                    super.onLayoutCompleted(state);
                }
            };
        //        recyclerView.setItemAnimator(null);
//        staggeredGridLayoutManager.setGapStrategy(StaggeredGridLayoutManager.GAP_HANDLING_NONE);
        this.recyclerView.setLayoutManager(staggeredGridLayoutManager);
        datas.add(new Data(0, "A title", "A body"));
        datas.add(new Data(1, "B title", "B body"));
        datas.add(new Data(2, "C title", "C body"));
        datas.add(new Data(3, "D title", "D body"));
        datas.add(new Data(4, "E title", "E body"));
        datas.add(new Data(5, "F title", "F body"));
        datas.add(new Data(6, "G title", "G body"));
        datas.add(new Data(7, "H title", "H body"));
        datas.add(new Data(8, "I title", "I body"));
        datas.add(new Data(9, "J title", "J body"));
        datas.add(new Data(10, "K title", "K body"));
        datas.add(new Data(11, "L title", "L body"));
        datas.add(new Data(12, "M title", "M body"));
//        datas.add(new Data(13, "N title", "N body"));
//        datas.add(new Data(14, "O title", "O body"));
//        datas.add(new Data(11, "P title", "P body"));
//        datas.add(new Data(12, "Q title", "Q body"));
//        datas.add(new Data(13, "R title", "R body"));
//        datas.add(new Data(14, "S title", "S body"));
        adapter = new Adapter(datas, this);

        recyclerView.setAdapter(adapter);
    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        MenuInflater inflater = getMenuInflater();
        inflater.inflate(R.menu.menu, menu);
        return true;
    }

    @Override
    public boolean onOptionsItemSelected(MenuItem item) {
        // Handle item selection
        switch (item.getItemId()) {
            case R.id.debug: {
                String s = (String) ((TextView) (((LinearLayout) ((FrameLayout) recyclerView.getChildAt(0)).getChildAt(0)).getChildAt(0)))
                    .getText();
                Data data0 = datas.get(0);
                Data data1 = datas.get(1);

                datas.set(0, data1);
                datas.set(1, data0);

                new MyListUpdateCallback().onMoved(1, 0);

                return true;
            }

            case R.id.debug2:

                Data data2 = datas.get(2);
                Data data3 = datas.get(3);

                datas.set(2, data3);
                datas.set(3, data2);

                adapter.notifyItemMoved(2, 3);

                return true;

            case R.id.debug3: {
                List<Data> oldDatas = new ArrayList<>(datas);

                Data data0 = datas.get(0);
                Data data1 = datas.get(1);

                datas.set(0, data1);
                datas.set(1, data0);

                MyNoteDiffUtilCallback noteDiffUtilCallback = new MyNoteDiffUtilCallback(datas, oldDatas);
                DiffUtil.calculateDiff(noteDiffUtilCallback).dispatchUpdatesTo(new MyListUpdateCallback());

                return true;
            }

            case R.id.debug4:
                String ABC = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
                int i = datas.size();
                if (i < 26) {
                    String ch = ABC.substring(i, i + 1);
                    datas.add(new Data(12, ch + " title", ch + " body"));
                    adapter.notifyItemInserted(i - 1);
                }
                return true;

            case R.id.debug5:
                int toRemove = datas.size() - 1;
                datas.remove(toRemove);
                adapter.notifyItemRemoved(toRemove);
                return true;

            default:
                return super.onOptionsItemSelected(item);
        }
    }

    @Override
    public void onClick(View v) {
        int oldPos = recyclerView.getLayoutManager().getPosition(v);
        int newPos = oldPos - 1;
        if (newPos < 0) {
            return;
        }
        Data data0 = datas.get(oldPos);
        Data data1 = datas.get(newPos);

        datas.set(oldPos, data1);
        datas.set(newPos, data0);
        new MyListUpdateCallback().onMoved(oldPos, newPos);
    }

    public class MyNoteDiffUtilCallback extends DiffUtil.Callback {
        private List<Data> newsDatas;
        private List<Data> oldDatas;


        public MyNoteDiffUtilCallback(List<Data> newsDatas, List<Data> oldDatas) {
            this.newsDatas = newsDatas;
            this.oldDatas = oldDatas;
        }

        @Override
        public int getOldListSize() {
            return oldDatas.size();
        }

        @Override
        public int getNewListSize() {
            return newsDatas.size();
        }

        @Override
        public boolean areItemsTheSame(int oldItemPosition, int newItemPosition) {
            return oldDatas.get(oldItemPosition).id == newsDatas.get(newItemPosition).id;
        }

        @Override
        public boolean areContentsTheSame(int oldItemPosition, int newItemPosition) {
            return oldDatas.get(oldItemPosition).equals(newsDatas.get(newItemPosition));
        }
    }

    boolean mDoTheFix = true;

    private class MyListUpdateCallback implements ListUpdateCallback {

        @Override
        public void onMoved(int fromPosition, int toPosition) {
            if (mDoTheFix && toPosition == 0) {
                adapter.notifyItemChanged(fromPosition);
                adapter.notifyItemChanged(toPosition);
            } else {
                adapter.notifyItemMoved(fromPosition, toPosition);
            }
        }

        public void onInserted(int position, int count) {
            adapter.notifyItemRangeInserted(position, count);
        }

        @Override
        public void onRemoved(int position, int count) {
            adapter.notifyItemRangeRemoved(position, count);
        }

        @Override
        public void onChanged(int position, int count, Object payload) {
            adapter.notifyItemRangeChanged(position, count, payload);
        }
    }
}

menu.xml

<menu xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:myApp="http://schemas.android.com/apk/res-auto">
    <item android:id="@+id/debug"
        android:title="0->1"
        myApp:showAsAction="always" />

    <item android:id="@+id/debug2"
        android:title="2->3"
        myApp:showAsAction="always" />

    <item android:id="@+id/debug3"
        android:title="DiffUtil"
        myApp:showAsAction="always" />

    <item android:id="@+id/debug4"
        android:title="Add"
        myApp:showAsAction="always" />

    <item android:id="@+id/debug5"
        android:title="Del"
        myApp:showAsAction="always" />
</menu>
like image 193
Cheticamp Avatar answered Nov 15 '22 16:11

Cheticamp


I did some analysis for the StaggeredGridLayoutManager by scanning grep code. Looks like there is an issue with calculation when itemMoved is at index 0. You can refer this method which will be called when notifyItemMoved() is called implicitly by the RecyclerView's adapter.

It works for other layouts because, other layouts simply call invalidateSpanCache() to perform correct recalculation in next layout which will be cased by notifyItemMoved(), since its a structural change event.

So the solution/workaround which I found are as follows, which only focuses on dealing with position movement of index 0:

Option 1: Call notifyDataSetChanged

For your case, in order to enforce StaggeredGridLayoutManager to react correctly for item moved at index 0, you need to enforce the StaggeredGridLayoutManager fully rebind and relayout all visible views. This can be achieved by calling notifyDataSetChanged() as follows:

case R.id.debug: {

     Data data0 = datas.get(0);
     Data data1 = datas.get(1);

     datas.set(0, data1);
     datas.set(1, data0);
     adapter.notifyDataSetChanged();
     return true;
 }

This triggers your expected animation. But the drawback is this operation can be expensive when you are dealing with huge data

Option 2: Call onItemMoved directly on layout

This method somehow performs correct calculation and views are not messed up. You need to maintain reference for the layout object:

case R.id.debug:{

    Data data0=datas.get(0);
    Data data1=datas.get(1);

    datas.set(0,data1);
    datas.set(1,data0);
    staggeredGridLayoutManager.requestSimpleAnimationsInNextLayout();
    staggeredGridLayoutManager.onItemsMoved(recyclerView,0,1,1);


    return true;
}

Drawback here is, it applies different animation (fade in/fade out). This will also not call any data change observers since StaggeredGridLayoutManager doesnot propogate the call to RecyclerView. You might need to create your animation to have same effect.

Option 3: Notify notifyItemChanged

This is the option you don't like for obvious valid reasons:

case R.id.debug: {
    Collections.swap(datas, 0, 1);
    adapter.notifyItemChanged(0);
    adapter.notifyItemChanged(1);
    return true;
}

To conclude, based on the nature of code, unclear documentation and nature of usage StaggeredGridLayoutManager development looks to be in inchoate stage and would mature in near future. A good solution will only be possible, when the issues with the layout has been fixed. Till then we have to use some workarounds with different trade offs.

like image 22
Sagar Avatar answered Nov 15 '22 16:11

Sagar


i tried that code and it works great
(instead of using notifyItemMoved I used notifyItemRangeChanged that is the correct notify function that you need)

you can see it also here https://jumpshare.com/v/fQklba8yFs4YTqkxoyMH

@Override
public boolean onOptionsItemSelected(MenuItem item) {
    // Handle item selection
    switch (item.getItemId()) {
        case R.id.debug:

            Data data0 = datas.get(0);
            Data data1 = datas.get(1);

            datas.set(0, data1);
            datas.set(1, data0);

            adapter.notifyItemRangeChanged(0,2);

            return true;

        case R.id.debug2:

            Data data2 = datas.get(2);
            Data data3 = datas.get(3);

            datas.set(2, data3);
            datas.set(3, data2);

            adapter.notifyItemRangeChanged(2,2);

            return true;

        default:
            return super.onOptionsItemSelected(item);
    }
}

Update

it is wrong to use notifyItemMoved for swap. You have to use 2 notifyItemChanged or a notifyItemRangeChanged or for all the dataset.

2nd working code

    @Override
public boolean onOptionsItemSelected(MenuItem item) {
    // Handle item selection
    switch (item.getItemId()) {
        case R.id.debug:

            Data data0 = datas.get(0);
            Data data1 = datas.get(1);

            datas.set(0, data1);
            datas.set(1, data0);
            adapter.notifyItemChanged(0);
            adapter.notifyItemChanged(1);

            return true;

        case R.id.debug2:

            Data data2 = datas.get(2);
            Data data3 = datas.get(3);

            datas.set(2, data3);
            datas.set(3, data2);
            adapter.notifyItemChanged(2);
            adapter.notifyItemChanged(3);

            return true;

        default:
            return super.onOptionsItemSelected(item);
    }
}
like image 29
gmetax Avatar answered Nov 15 '22 17:11

gmetax


Found a workaround that seems to work perfectly:

class LayoutManager : StaggeredGridLayoutManager(2, VERTICAL) {
    override fun onItemsMoved(recyclerView: RecyclerView, from: Int, to: Int, itemCount: Int) {
        try {
            moveView(from, to)
        } catch (e: IllegalArgumentException) {
        }
    }
}

It's written in Kotlin but should be trivial to translate to java. Just use this class intead of StaggeredGridLayoutManager.

like image 22
user2509335 Avatar answered Nov 15 '22 16:11

user2509335