SelectExtension vs. updating lists

RobbWatershed commented 3 years ago

About this issue

I'm using FastAdapter to manage a list of books. This list is automatically updated from the DB through LiveData everytime something changes in the source dataset (added item, removed item, updated field such as last read date).

Diff'ing is done by FastAdapter's default DiffCallbackImpl class :

Single.fromCallable(() -> FastAdapterDiffUtil.INSTANCE.calculateDiff(itemAdapter, contentItems))
        .subscribeOn(Schedulers.computation())
        .observeOn(AndroidSchedulers.mainThread())
        .subscribe(diffResult -> {
            FastAdapterDiffUtil.INSTANCE.set(itemAdapter, diffResult);
        }

I'm also using SelectExtension to allow users to select books.

What I'm observing is the following :

Selection works fine when the list isn't updated.
As soon as the dataset is updated (e.g. when a new book arrives, a book is removed or a book is updated), selectExtension.getSelectedItems() and selectExtension.getSelections() always return an empty list even though the items have been selected visually, which bugs my selection system.

I've been trying to circumvent this problem for the past few months with no success, creating uncontrolled crashes on some devices instead. This post is my attempt to document and fix the core issue.

NB : That issue stays the same whether the source is a PagedList updated through PagedModelAdapter.submitList or a traditional list updated with FastAdapterDiffUtil.set

Analysis

selectExtension.getSelections() calls adapterSizes.valueAt(index).getAdapterItem to get the list of selected items.

adapterSizes is the "main list" of the adapter, which is updated as every new dataset is set / submitted to the adapter. When the bug occurs, the isSelected property of these items is never set to true, even though selection has happened visually.

So what happens when we select an item ? The select action happens on SelectExtension.handleSelection, where item selection is toggled.

However, the toggled item is provided by EvenHookUtil.OnLongClickListener, which uses FastAdapter.getHolderAdapterItemTag : the item which is toggled is actually a reference stored as a tag inside each ViewHolder.

At this point, my theory is that this reference is lost when the list is updated, leaving a reference to a stale object inside the viewholder, and brand new, unselected objects in the adapter's main list.

This is confirmed by the way updates are handled by SelectExtension : updating is done through DiffUtil.dispatchUpdatesTo (Android class) and ripples to SelectExtension.notifyAdapterItemRangeInserted, which does... nothing ! (empty implementation)

Suggested fixes (to discuss)

I understand item references are stored as viewholder tags because : 1- we need to keep track of the selection status, which is not part of the information received from the data source 2- the selected state is a visual indicator that can "handily" be stored at the viewholder level

However, this only works when these references are updated along the main list.

SOLUTION A

Abandon the viewholder tag storage and manage selected / deselected states in a dedicated Map<long, bool> where the key is the item identifier (as in IIdentifyable.identifier) and the bool is the selected state.

SOLUTION B

Implement SelectExtension.notifyAdapterItemRangexxx so these handlers update tags stored inside the viewholders with the new references they receive.

The macro-algorithm may be 1- Memorize the selected states (e.g. to a Map<long, bool>) 2- Store the new item references to the "main" list 3- On these new items, restore the selected state on matching IDs

Thanks for reading through all of this 😄 Any opinion about these suggestions, or a 3rd solution I didn't see ?

Cheers~

Details

[x] Used library version > 5.3.1
[x] Used support library version > Not sure what you mean
[x] Used gradle build tools version > com.android.tools.build:gradle:4.1.1
[x] Used tooling / Android Studio version > 4.1.1
[x] Other used libraries, potential conflicting libraries > None that I know of

Checklist

[x] Searched for similar issues
[x] Checked out the sample application
[x] Read the README
[x] Checked out the CHANGELOG
[x] Checked out the MIGRATION GUIDE

mikepenz commented 3 years ago

Thanks for the very detailed report. I can't give a very elaborate answer right now, but would come back to it hopefully over the weekend.

Regarding Solution A.

That's actually basically what we had in prior releases, but it proofed to be very problematic as it breaks the "single source of truth" and keeping this in sync proofed to be very critical and risky, as such this approach was abadone in favor to a much more simple and streamlined definition.

Solution B

this may be working but I believe it similarly could result in problems and invalid states.

Would it be possible to have this occur in a automated test case which makes it easier to implement a proper fix for it? Based on the very quick run through I believe the best approach will be to take a look at this bugs root, because it is very important that the Item has the proper selection state -> because when you scroll things have to be properly reselected with invalidation of the view.

so A nor B would fix that

RobbWatershed commented 3 years ago

Thanks a lot for the input. I'm glad we can work together on that one !

Would it be possible to have this occur in a automated test case which makes it easier to implement a proper fix for it?

Alright, I don't promise anything but I'll have a go at it !

RobbWatershed commented 3 years ago

It wasn't that hard - see https://github.com/mikepenz/FastAdapter/pull/956

RobbWatershed commented 3 years ago

Solved by using selectExtension.setSelectWithItemUpdate(true)

mikepenz / FastAdapter