(Selection) Unrecycled detached ViewHolders can't be updated when using StableIdKeyProvider. [145767095]

Fixed

Bug

Status Update

No update yet.

Description

na...@gmail.com

created issue #1

Dec 6, 2019 01:33AM

The scenario:

- I have a RecyclerView using Selection with SelectionPredicates.createSelectSingleAnything() and StableIdKeyProvider.
- A selected item will be colored RED, unselected item will be colored GREEN.
- I select item whose adapterPosition is 0.
- I scroll the list so the item whose adapterPosition is 1 will be on top.
--> What I actually want is the item at '0' will be offscreen and will be detached, but not enough for recycle.
- I select item of adapterPosition '1'.
--> Because it is single selection list, I expect that the item of position '0' will *appear to be unselected*.
- I scroll to top and see if item of position '0' appears to be unselected or not.

Expect: item of position 0 will be colored GREEN (it should appear to be unselected), item of position 1 will be colored RED (it should appear to be selected).

Actual: item of position 0 is colored RED (it appears to be selected), item of position 1 is colored RED (it appears to be selected). (See attachment: recyclerview_selection_issue.mp4).

Note that, the item is correctly unselected, but the change is never delivered to the Adapter due to the behavior of StableIdKeyProvider (I have post about this here:

https://twitter.com/ene__im/status/1202477885505691648), so it cannot "appear" to be unselected.

Source code:

https://github.com/eneim/RecyclerView-Selection-Bug

Version used (can be found in the repo): RecyclerView 1.1.0, Selection 1.1.0-beta01.

Thanks in advance.

recyclerview_selection_issue.mp4

4.2 MB

Download

Comments

em...@google.com <em...@google.com> May 4, 2020 10:38PM

Assigned to ry...@google.com.

ry...@google.com <ry...@google.com> May 6, 2020 06:21AM

Reassigned to sm...@google.com.

sm...@google.com <sm...@google.com> #2Jul 27, 2020 07:19PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019

Change the way to detect mismatch between POJO and query

This fixes cursor mismatch warnings with expandProjection.

Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8

M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt

https://android-review.googlesource.com/1123258

https://goto.google.com/android-sha1/b90079595f33f58fece04026a97faa0d243acdb1

na...@gmail.com <na...@gmail.com> #3Jul 28, 2020 12:22AM

The following changes were cherrypicked through Coastguard (

https://android-build.googleplex.com/coastguard/)
Release Track: (

https://android-build.googleplex.com/coastguard/dashboard/6093116828680192/#/cherrypickHistory/all)
Created: 2019-10-14 17:23:42.610596+00:00
Changes: aosp/1123258

sm...@google.com <sm...@google.com> #4Jul 28, 2020 04:24PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020

Re-factor how errors are dismissed when query is re-written

This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (

b/140759491

).
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.

This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.

Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f

M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt

https://android-review.googlesource.com/1288456

ap...@google.com <ap...@google.com> #5Jul 31, 2020 09:32PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit a938a74a5ecb72f8e74bcc9a1722ac6bc07b89a2
Author: Steve McKay <smckay@google.com>
Date: Wed Jul 29 11:38:24 2020

StableIdKeyProvider improvements.

Fix bug where key/position mapping in KeyProvider was lost while entry was not yet recycled.
- In order to achieve adequate unit test coverage I added a new abstraction on-top of RecyclerView, the "ViewHost".
- Note testing could be facilitated with fewer back-flips if selection lib was in the same package as RecyclerView.
- In order to listen to recycle events we use the single use setRecyclerListener method. TODO: Add support for multiple listeners (or possibly just add a new method to OnChildAttachStateChangeListener.
Update TestAdapter to support setting setHashStableIds prior to registration of internal AdapterDataObserver.
Add some missing @NonNull, @Nullable, and @Override annotations.
Added a new single-select, stable-id based demo app.
Updated demo apps to use getAbsoluteAdapterPosition.
Remove unnecessary fancy stuff from the "Simple" demo app.

Bug: 145767095
Test: Updated StableIdKeyProviderTest to actually test stuff :)
Change-Id: If8ff8256cff49d0cbf12f2175af3c3ca62b7c68b

M recyclerview/recyclerview-selection/src/androidTest/java/androidx/recyclerview/selection/DefaultSelectionTrackerTest.java
M recyclerview/recyclerview-selection/src/androidTest/java/androidx/recyclerview/selection/DefaultSelectionTracker_SingleSelectTest.java
M recyclerview/recyclerview-selection/src/androidTest/java/androidx/recyclerview/selection/StableIdKeyProviderTest.java
M recyclerview/recyclerview-selection/src/androidTest/java/androidx/recyclerview/selection/testing/TestAdapter.java
M recyclerview/recyclerview-selection/src/androidTest/java/androidx/recyclerview/selection/testing/TestHolder.java
M recyclerview/recyclerview-selection/src/main/java/androidx/recyclerview/selection/DefaultSelectionTracker.java
M recyclerview/recyclerview-selection/src/main/java/androidx/recyclerview/selection/ItemDetailsLookup.java
M recyclerview/recyclerview-selection/src/main/java/androidx/recyclerview/selection/StableIdKeyProvider.java
M samples/Support7Demos/src/main/AndroidManifest.xml
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/fancy/DemoAdapter.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/fancy/DemoDetailsLookup.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/fancy/DemoHeaderHolder.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/fancy/DemoHolder.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/fancy/DemoItemHolder.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/fancy/FancySelectionDemoActivity.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/simple/DemoAdapter.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/simple/DemoHolder.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/simple/SimpleSelectionDemoActivity.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/single/DemoAdapter.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/single/DemoDetailsLookup.java
M samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/single/DemoHolder.java
A samples/Support7Demos/src/main/java/com/example/android/supportv7/widget/selection/single/SingleStableIdSelectionDemoActivity.java
M samples/Support7Demos/src/main/res/values/strings.xml

https://android-review.googlesource.com/1380276

sm...@google.com <sm...@google.com> #6Aug 3, 2020 08:42PM

Unfortunately, due to the reliance of the fix (

https://android-review.googlesource.com/1380276) on setRecyclerListener (which supports a single listener only), the change opens the door to potential bugs where the selection library or app author overwrite the others' registered RecyclerListener.

https://r.android.com/1381078 adds support to RecyclerView for multiple RecyclerListeners, and

https://r.android.com/1384002 updates Selection lib to use the new addRecyclerListener API. But the selection lib's rc release cannot depend on an API that is not also RC or above. Given the fact that there's a work around (SCOPE_MAPPED provider, see

comment#2

) will not include a fix for this issue in the 1.1.0-rc02 release.

Will leave this bug open to track inclusion in future release.

ap...@google.com <ap...@google.com> #7Aug 5, 2020 06:35PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 66c5c69e735df7a0540ac621e0c084ac87be0cb1
Author: Steve McKay <smckay@google.com>
Date: Fri Jul 31 14:42:15 2020

Update StableIdKeyProvider to use addRecyclerListener.

Bug: 145767095
Test: All tests passing, manual testing via demo app.
Change-Id: I21df99af34c2d34b720303858785eddb2a5314f6

M recyclerview/recyclerview-selection/src/main/java/androidx/recyclerview/selection/StableIdKeyProvider.java

https://android-review.googlesource.com/1384002

ap...@google.com <ap...@google.com> #8Aug 5, 2020 06:35PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit ce1b333030414b89b1980fa2327774af1b5416dc
Author: Steve McKay <smckay@google.com>
Date: Thu Jul 30 09:33:41 2020

Add support for multiple RecyclerListeners.

This feature is necessitated by RecyclerView-Selection dependency on listening
to recycle events.

Bug: 162504379
Test: Added new coverage.
Relnote: Add support for multiple RecyclerListeners as necessitated to fix

b/145767095

affecting RecyclerView-Selection.
Change-Id: I70ad8f9bcf25c2c00fbf5f71d5a991287bef1606

M leanback/leanback/api/current.txt
M leanback/leanback/api/public_plus_experimental_current.txt
M leanback/leanback/api/restricted_current.txt
M recyclerview/recyclerview/api/current.txt
M recyclerview/recyclerview/api/public_plus_experimental_current.txt
M recyclerview/recyclerview/api/restricted_current.txt
A recyclerview/recyclerview/src/androidTest/java/androidx/recyclerview/widget/RecyclerListenerTest.java
M recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/RecyclerView.java

https://android-review.googlesource.com/1381078

sm...@google.com <sm...@google.com> #9Apr 22, 2021 09:24PM

Planning on the fix being released in 1.2. Should be available in 1.2.0-alpha01 in the next few weeks. No telling for sure how long it will take to get 1.2.0 to stable.

cr...@gmail.com <cr...@gmail.com> #10May 22, 2024 09:42PM

With the 1.2.0-alpha01 it seems there's a new issue now, caused by the changes, that results in the holder at the top of StableIdKeyProvider.onRecycled() to always be null. This still results in the same issue that the cache isn't updated properly and the that can cause crashes, because the lookup is wrong.