recyclerview-selection: RecyclerView items fire onClick events while SelectionTracker has active selection [161162268]

Fixed

Bug

NeedsInfo

[AOSP] assigned

Status Update

No update yet.

Description

ki...@gmail.com

created issue #1

Jul 13, 2020 09:29PM

Artifact used (ex. androidx.appcompat:appcompat:1.0.0-alpha1): androidx.recyclerview:recyclerview-selection:1.1.0-rc01
Version used: 1.1.0-rc01
Theme used: Theme.AppCompat.Light.DarkActionBar
Devices/Android versions reproduced on: Motorola Moto G6, Android 9

When the item views of a RecyclerView have OnClickListeners attached, the behaviour of firing onClick events during selection is quite unpredictable. As soon as at least one item was selected, classic ListView did no longer fire any click events for individual list items until all items were unselected.
Current recyclerview-selection exhibits two behaviours I would consider a bug, but maybe I'm using the API in a wrong way:
1. When long-clicking an item to enable multi-item selection, the item is marked as selected/activated as expected. But as soon as you release the finger, an onClick event will be triggered, despite the fact that RecyclerView is currently in selection mode. This behaviour is new since 1.1.0-rc01 and was not present in 1.0.0.
2. After enabling selection mode by long-clicking an item, you can select/activate other items by simply clicking on them. This works fine as long as you click through the items slowly. Then one by one gets selected and no onClick events get fired. But when you click a bit faster, every second item is not selected, but instead clicked (triggering onClick event). This problem existed in 1.0.0 and is still present in 1.1.0-rc01.

The following log shows a sequence of onItemStateChanged and onClick events triggered by long-clicking item 0 to enable selection, then clicking items 1, 2 and 3 quickly.
Expected result: Item 0 is selected, then items 1, 2 and 3 are selected, no onClicks get triggered.
Result on my device: Item 0 is selected, then onClick is triggered for item 0. Items 1 and 3 are selected and item 2 fires an onClick event in between.

click item 0 and hold button:
2020-07-13 21:55:01.402 15005-15005/com.example.recyclerviewdemo E/onItemStateChanged: 0: true
item 0 is selected, now release finger:
2020-07-13 21:55:02.306 15005-15005/com.example.recyclerviewdemo E/onClick: item 0
now click (and release) items 1, 2 and 3 quickly:
2020-07-13 21:55:06.850 15005-15005/com.example.recyclerviewdemo E/onItemStateChanged: 1: true
2020-07-13 21:55:07.038 15005-15005/com.example.recyclerviewdemo E/onClick: item 2
2020-07-13 21:55:07.236 15005-15005/com.example.recyclerviewdemo E/onItemStateChanged: 3: true

Attached is an Android Studio project to reproduce the behaviour.
Item IDs == item positions for brevity, the library behaves the same way in a more complex project with "real" item IDs and a custom key provider.

recyclerviewdemo.zip

431 KB

Download

Comments

ki...@google.com <ki...@google.com> Jul 20, 2020 01:13PM

Assigned to sh...@google.com.

da...@gmail.com <da...@gmail.com> #2Jul 21, 2020 01:15PM

My guess is an AutoValue extension is what's needed here:

http://ryanharter.com/blog/2016/05/16/autovalue-extensions/

We accept pull requests! :)

sh...@google.com <sh...@google.com> Jul 21, 2020 08:31PM

Reassigned to sm...@google.com.

sm...@google.com <sm...@google.com> #3Jul 27, 2020 05:19PM

> We accept pull requests! :)

Is there a public repo somewhere? I don't see any obvious repo for it in

https://android.googlesource.com, and it doesn't seem to be inside

https://android.googlesource.com/platform/frameworks/support.

Room supports final fields (yay!), which probably will suffice for many people with respect to this feature request.

ki...@gmail.com <ki...@gmail.com> #4Jul 27, 2020 07:52PM

Room supports immutability (it can use arg constructors) but does not directly support AutoValue. It is in the schedule but not high priority :/. Idk much about its internals at this stage so I'm not sure how we would implement it but should be totally doable.

Sorry we don't have the source release yet :/.

sm...@google.com <sm...@google.com> #5Jul 28, 2020 03:44PM

"It is in the schedule but not high priority" -- completely understandable.

"Sorry we don't have the source release yet :/." -- ah, OK, I thought perhaps with the pull request comment, that meant that there was a repo somewhere that I had overlooked.

Thanks!

ki...@gmail.com <ki...@gmail.com> #6Jul 28, 2020 04:26PM

Add autovalue support also means you can easily achieve parcelable by

https://github.com/rharter/auto-value-parcel. Please consider support this.

sm...@google.com <sm...@google.com> #7Jul 28, 2020 04:58PM

AutoValue is really a handy way to ensure data integrity.

sm...@google.com <sm...@google.com> #8Aug 14, 2020 06:14PM

Please add AutoValue support. AutoValue is a Google library with really good benefits such as toString() , hashCode() , AutoValue.Builder , checks at creation time if @NonNull values are null, etc.

sm...@google.com <sm...@google.com> #9Aug 15, 2020 12:25AM

FWIW,

issue 64206877

is not publicly accessible.

sm...@google.com <sm...@google.com> #10Aug 15, 2020 12:38AM

@Yigit: That appears to be a private ticket. Any way we can have access to keep up with it?

ap...@google.com <ap...@google.com> #11Aug 17, 2020 07:45PM

Have any updates?

sm...@google.com <sm...@google.com> #12Aug 17, 2020 08:00PM

news?

sm...@google.com <sm...@google.com> #13Aug 17, 2020 08:01PM

Is there any update? This is huge for app that absolutely enforces immutable entities.

ap...@google.com <ap...@google.com> #14Aug 19, 2020 12:20AM

SqlDelight has out of the box support for AutoValue. Use it with SqlBrite and you've got *pretty good* parity with Room. With the lack of movement on this ticket, I'd suggest at least taking a look at those if AutoValue is important to you.

ap...@google.com <ap...@google.com> #15Aug 19, 2020 12:22AM

Support on olways

ap...@google.com <ap...@google.com> #16Aug 19, 2020 01:54AM

Sorry this used to be higher priority but other features got in the way (and to be honest, androidX migration took a lot more time than we thought).

I want to make 1 thing clear though, Room does support immutable entities. One of the reasons why this ticket lost some priority is that it is specific to AutoValue, not immutability. If you are using Kotlin, data classes give you the same power and Room supports them properly. (hence, there is an easy way to achieve immutability w/ Room).

The issue in this one is that we don't have out of the box support to discover AutoValue builders.
We still plan to do this, sorry that it is taking longer than we initially communicated.

ap...@google.com <ap...@google.com> #17Aug 19, 2020 01:57AM

I'd like to piggy back onto this issue, because I've fun into a similar problem with immutability that does NOT involve autovalue. What I have found is either a bug or a missing feature, depending on your perspective.

Let's say I have a Kotlin data class with only `val` properties. I want to serialize this into my Room database, but I want to decorate it with a few other values, for example a timestamp so I know when it was last updated, and a userId because my database supports multiple users. Oh look! There's an annotation `@Embedded` that seems to do just want I want. I can create a new class specific to Room persistence, add the few new properties I want, and then "embed" the original object, and damn that was easy.

Oh no. Now my project won't compile. It seems that embedded objects must have public getters -- all my `val`s must now be `var`s. D:

Sigh. I guess I'll just write a new class that has _all_ the properties of the original, plus a few more, and write an extension function to transform from one to the other. Really glad Room saved me all that Sqlite boilerplate, so I now have time for Room-specific boilerplate ;-)

ap...@google.com <ap...@google.com> #18Aug 19, 2020 02:18AM

*public setters, not public getters

an...@google.com <an...@google.com> #19Aug 19, 2020 02:18AM

#17 seems like a bug, we are supposed to handle Embedded fields in constructor parameters.

Can you file a bug with a sample project ?

We do have a test for having embedded as a constructor parameter, so maybe something else is wrong.

from one of our tests:

@Entity(tableName = "fc")
static class FullConstructor {
@PrimaryKey
public final int a;
public final int b;
@Embedded
public final MyEmbedded embedded;

FullConstructor(int a, int b, MyEmbedded embedded) {
this.a = a;
this.b = b;
this.embedded = embedded;
}
}

sm...@google.com <sm...@google.com> #20Aug 28, 2020 11:15PM

Marked as fixed.

Sure, I'd be happy to put up a sample project. I'll try to pull something together later today or maybe tomorrow.

sm...@google.com <sm...@google.com> #21Aug 28, 2020 11:27PM

Well, I tried. Either I did something wrong the first time, or... I did something wrong the first time :shrug-emoji: Tried reproducing in a sample project, couldn't. Mystified, I tried again in my much more complex work project -- couldn't. So, I guess that's a good thing!

ki...@gmail.com <ki...@gmail.com> #22Sep 2, 2020 08:52PM

if added this feature will be good!

sm...@google.com <sm...@google.com> #23Sep 2, 2020 10:20PM

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

commit 8c2d348127161daaf624f77d80cb662e65e3af2a
Author: Daniel Santiago Rivera <danysantiago@google.com>
Date: Fri Jul 13 15:18:42 2018

Add support for using AutoValue classes as Pojos and Entities.

Support using AutoValue generated classes as Pojos by processing the
AutoValue_* generated class that contains fields and getter
implementations.

To support the AutoValue api @PrimaryKey and @ColumnInfo have been
opened up to target methods. However, since we only want to allow their
usage in methods to support AutoValue a new processing step was added
that throws an error if it finds a method not implement by AutoValue
with any of these two annotations.

Note that Embedded and Relations are not yet supported.

Bug: 62408420
Test: ./gradlew :room:room-compiler:test \
:room:integration-tests:testapp:cC \
:room:integration-tests:autovaluetestapp:cC
Change-Id: I782d6ec074776f7af7cae977848443f3311b1d87

M app-toolkit/settings.gradle
M buildSrc/src/main/kotlin/androidx/build/dependencies/Dependencies.kt
M room/common/src/main/java/androidx/room/ColumnInfo.java
M room/common/src/main/java/androidx/room/PrimaryKey.java
M room/compiler/build.gradle
M room/compiler/src/main/kotlin/androidx/room/RoomProcessor.kt
A room/compiler/src/main/kotlin/androidx/room/checker/AutoValueTargetChecker.kt
M room/compiler/src/main/kotlin/androidx/room/ext/element_ext.kt
M room/compiler/src/main/kotlin/androidx/room/processor/EntityProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/processor/PojoProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/processor/ProcessorErrors.kt
M room/compiler/src/main/kotlin/androidx/room/processor/RawQueryMethodProcessor.kt
A room/compiler/src/main/kotlin/androidx/room/processor/autovalue/AutoValuePojoProcessorDelegate.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/vo/Constructor.kt
M room/compiler/src/main/kotlin/androidx/room/vo/Warning.kt
M room/compiler/src/main/kotlin/androidx/room/writer/FieldReadWriteWriter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/EntityProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/processor/PojoProcessorTest.kt
A room/compiler/src/test/kotlin/androidx/room/processor/autovalue/AutoValuePojoProcessorDelegateTest.kt
A room/compiler/src/test/kotlin/androidx/room/processor/checker/AutoValueTargetCheckerTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/test_util.kt
A room/integration-tests/autovaluetestapp/.gitignore
A room/integration-tests/autovaluetestapp/build.gradle
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/TestDatabase.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/dao/ParcelableEntityDao.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/dao/PersonDao.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/dao/PetDao.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/ParcelableEntityDaoTest.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/PersonDaoTest.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/PetDaoTest.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/TestDatabaseTest.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/EmbeddedAutoValue.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/ParcelableEntity.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/Person.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/Pet.java
A room/integration-tests/autovaluetestapp/src/main/AndroidManifest.xml
M settings.gradle

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

https://goto.google.com/android-sha1/8c2d348127161daaf624f77d80cb662e65e3af2a

ki...@gmail.com <ki...@gmail.com> #24Sep 3, 2020 06:14AM

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

commit 038e674add5afddbb34371b9c1fda6885efbd7be
Author: Daniel Santiago Rivera <danysantiago@google.com>
Date: Fri Jul 13 15:18:42 2018

Add support for using AutoValue classes as Pojos and Entities.

Support using AutoValue generated classes as Pojos by processing the
AutoValue_* generated class that contains fields and getter
implementations.

To support the AutoValue api @PrimaryKey and @ColumnInfo have been
opened up to target methods. However, since we only want to allow their
usage in methods to support AutoValue a new processing step was added
that throws an error if it finds a method not implement by AutoValue
with any of these two annotations.

Note that Embedded and Relations are not yet supported.

Bug: 62408420
Test: ./gradlew :room:room-compiler:test \
:room:integration-tests:testapp:cC \
:room:integration-tests:autovaluetestapp:cC
Change-Id: Idce0b5934e0aa864117fc95e7d19a7edc2c94f9f

M app-toolkit/settings.gradle
M buildSrc/src/main/kotlin/androidx/build/dependencies/Dependencies.kt
M room/common/src/main/java/androidx/room/ColumnInfo.java
M room/common/src/main/java/androidx/room/PrimaryKey.java
M room/compiler/build.gradle
M room/compiler/src/main/kotlin/androidx/room/RoomProcessor.kt
A room/compiler/src/main/kotlin/androidx/room/checker/AutoValueTargetChecker.kt
M room/compiler/src/main/kotlin/androidx/room/ext/element_ext.kt
M room/compiler/src/main/kotlin/androidx/room/processor/EntityProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/processor/PojoProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/processor/ProcessorErrors.kt
M room/compiler/src/main/kotlin/androidx/room/processor/RawQueryMethodProcessor.kt
A room/compiler/src/main/kotlin/androidx/room/processor/autovalue/AutoValuePojoProcessorDelegate.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/vo/Constructor.kt
M room/compiler/src/main/kotlin/androidx/room/vo/Warning.kt
M room/compiler/src/main/kotlin/androidx/room/writer/FieldReadWriteWriter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/EntityProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/processor/PojoProcessorTest.kt
A room/compiler/src/test/kotlin/androidx/room/processor/autovalue/AutoValuePojoProcessorDelegateTest.kt
A room/compiler/src/test/kotlin/androidx/room/processor/checker/AutoValueTargetCheckerTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/test_util.kt
A room/integration-tests/autovaluetestapp/.gitignore
A room/integration-tests/autovaluetestapp/build.gradle
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/TestDatabase.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/dao/ParcelableEntityDao.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/dao/PersonDao.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/dao/PetDao.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/ParcelableEntityDaoTest.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/PersonDaoTest.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/PetDaoTest.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/TestDatabaseTest.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/EmbeddedAutoValue.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/ParcelableEntity.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/Person.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/Pet.java
A room/integration-tests/autovaluetestapp/src/main/AndroidManifest.xml
M settings.gradle

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

https://goto.google.com/android-sha1/038e674add5afddbb34371b9c1fda6885efbd7be

sm...@google.com <sm...@google.com> #25Sep 3, 2020 04:20PM

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

commit b4e0fe5d54a89ce8ce9a852774892c498edd4508
Author: Daniel Santiago Rivera <danysantiago@google.com>
Date: Mon Jul 23 19:51:46 2018

Add support for @Embedded in AutoValue Pojos.

Bug: 62408420
Test: ./gradlew :room:integration-tests:autovaluetestapp:cC
Change-Id: I4aa02aae022e4971a8708c4aba4cf9f3e90f65a5

M room/common/src/main/java/androidx/room/Embedded.java
M room/compiler/src/main/kotlin/androidx/room/checker/AutoValueTargetChecker.kt
M room/compiler/src/test/kotlin/androidx/room/processor/checker/AutoValueTargetCheckerTest.kt
M room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/dao/PersonDao.java
M room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/dao/PetDao.java
M room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/PersonDaoTest.java
M room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/PetDaoTest.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/PersonAndCat.java
M room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/Pet.java

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

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

ki...@gmail.com <ki...@gmail.com> #26Sep 3, 2020 06:19PM

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

commit 0f55cbbe13e26294af67aca6bce0e13a960ae752
Author: Daniel Santiago Rivera <danysantiago@google.com>
Date: Mon Jul 30 17:06:22 2018

Add support for @Relation in AutoValue Pojos.

Bug: 62408420
Test: ./gradlew :room:integration-tests:autovaluetestapp:cC
Change-Id: I7634ac388e328dca9c33f7ac422080b42b362ee0

M room/common/src/main/java/androidx/room/Relation.java
M room/compiler/src/main/kotlin/androidx/room/checker/AutoValueTargetChecker.kt
M room/compiler/src/test/kotlin/androidx/room/processor/checker/AutoValueTargetCheckerTest.kt
M room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/dao/PersonDao.java
M room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/dao/PetDao.java
M room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/test/PersonDaoTest.java
A room/integration-tests/autovaluetestapp/src/androidTest/java/androidx/room/integration/autovaluetestapp/vo/PersonWithCats.java

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

https://goto.google.com/android-sha1/0f55cbbe13e26294af67aca6bce0e13a960ae752

sm...@google.com <sm...@google.com> #27Sep 3, 2020 06:43PM

This feature will be available in Room 2.1.0

ki...@gmail.com <ki...@gmail.com> #28Sep 4, 2020 06:42AM

Does this means it could be used only with AutoValue, and not with other code generators like Immutables

https://immutables.github.io/ ?
In that case it's really limited.

Can you provide way to have static factory methods instead of constructors? Then we can use whatever code generator.
Like:

@RoomFactoryMethod
public static MyEntity create(int id, String name) {
return ImmutableMyEntity.of(id, name);
}

sm...@google.com <sm...@google.com> #29Sep 4, 2020 05:13PM

Yes, this is specific to AutoValue (we also support regular java immutables and kotlin) as this solves the problem w/o any extra code on the developer side.
Having an entity factory method seems reasonable too as a future improvement.