oppia / oppia-android

A free, online & offline learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
314 stars 517 forks source link

Introduce support for diffing subsets of data in generic RecyclerView adapter #171

Open BenHenning opened 5 years ago

BenHenning commented 5 years ago

The initial implementation of the generic RecyclerView BindableAdapter notifies that the whole data set is changed for each re-bind, which is sometimes wrong (the data may be the same) and highly inefficient (1 element might have changed out of hundreds). Diffing logic should be introduced to only notify that a subset of the data has changed, which also enables support for RecyclerView animations.

anandwana001 commented 4 years ago

@BenHenning Tried this issue using AsyncListDiffer and setting the new data inside our setData(newDataList: List<T>) with differ.submitList(buildNewList(newDataList))

Video link - https://drive.google.com/drive/folders/1HAfzcLl2BkSddAjuUuCBJAmf5RwP9Atn?usp=sharing

Before Before

After After

BenHenning commented 4 years ago

Reopened due to revert: https://github.com/oppia/oppia-android/pull/1627.

khushal1707 commented 3 years ago

@anandwana001 I like to take this issue, could you give me few pointers on what was the problem with implementation of DiffUtil? Were test cases complete and passing?

anandwana001 commented 3 years ago

@khushal1707 Things which are failing:

  1. RecyclerView loading is smooth but slow
  2. Drag and Drop doesn't work correctly

These are the 2 issues we face after merging my earlier pr

  1. https://github.com/oppia/oppia-android/issues/1625
  2. https://github.com/oppia/oppia-android/issues/1626
aggarwalpulkit596 commented 3 years ago

@anandwana001 didn't we solved this earlier ?

anandwana001 commented 3 years ago

No, the linking thing in drag and drop doesn't work with the diffing.

aggarwalpulkit596 commented 3 years ago

I'll take a look at it today i believe i have a solution for this

anandwana001 commented 3 years ago

Thanks, this will be helpful.

khushal1707 commented 3 years ago

@anandwana001 So the problem which I understood is that grouping items doesn't invoke DiffUtil, whereas on dragging and dropping it refreshes and shows the the grouped item. I haven't found any function in DiffUtil to solve this issue but there is a workaround which invoke DiffUtil to refresh list i.e. to call function onItemMoved() in DragAndDropSortInteractionViewModel twice such that its effect on the list got nullify( this is just the workaround and doesn't completely solves the problem ). So should I implements these workarounds or try something else?

anandwana001 commented 3 years ago

let's have. a draft pr, which solves this issue and will discuss more on this. Also, keeping @aggarwalpulkit596 in loop for drag and drop feature.

khushal1707 commented 3 years ago

Sure!

aggarwalpulkit596 commented 3 years ago

OnItemMoved won't work correctly if i remember what i did while trying to fix this.

khushal1707 commented 3 years ago

OnItemMoved won't work correctly if i remember what i did while trying to fix this.

Yes, it didn't refresh when there is only one item on list and also cashes app sometimes. Is there any other way to invoke DiffUtil when grouping occurs @aggarwalpulkit596 ?

aggarwalpulkit596 commented 3 years ago

I'd have to try it out myself first it's been quite some time 😅

khushal1707 commented 3 years ago

I'd have to try it out myself first it's been quite some time 😅

Okay, till then I also try to figure out something and if something works I'll make a draft PR and notify.

khushal1707 commented 3 years ago

Hi @anandwana001 @aggarwalpulkit596, can we use notifyDataSetChanged() for nested recyclerview of DragAndDropSortInteraction only after merging in DragAndDropSortInteractionViewModel and use Diffing in main and other recyclerviews. Merging works fine with this approach.

khushal1707 commented 3 years ago

@anandwana001 @aggarwalpulkit596 PTAL: https://github.com/oppia/oppia-android/pull/2666 Haven't added tests yet, will be adding as soon as issue with diffing resolves. Thanks!