Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kotlin synthetic caching not working in library artifact #235

Closed
leobrujah opened this issue Nov 29, 2018 · 14 comments
Closed

Kotlin synthetic caching not working in library artifact #235

leobrujah opened this issue Nov 29, 2018 · 14 comments
Labels

Comments

@leobrujah
Copy link

I've created a self-contained project using groupie where I checked that seems the caching of the kotlin synthetic views seems not being used whatsover .
if you look at the code

class GroupieDummyModel(val content: String = "Hello World") : Item() {
    override fun bind(viewHolder: ViewHolder, position: Int) {
        viewHolder.textView.text = content
    }
    override fun getLayout(): Int = R.layout.item_content
}

It will produce the following bytecode

   public void bind(@NotNull ViewHolder viewHolder, int position) {
      Intrinsics.checkParameterIsNotNull(viewHolder, "viewHolder");
      TextView var10000 = (TextView)((LayoutContainer)viewHolder).getContainerView().findViewById(id.textView);
      Intrinsics.checkExpressionValueIsNotNull(var10000, "viewHolder.textView");
      var10000.setText((CharSequence)this.content);
   }

which it stills use findViewById and not findViewByCache since Item doesn't implements LayoutContainer doesn't really matter that ViewHolder implements its Ithink, the Kotlin Compiler doesn´t know about that because it really expects the Item implementing it so it doesn´t generate the caching bytecode, seems like doing this is the same as not using LayoutContainer whatsover since it stills make the lookup in the hole itemView tree view.

Or maybe I'm really missing something here

Project:
https://gitlab.com/leobrujah/kotlin-synthetic-test/blob/master/app/src/main/java/com/test/kotlinsyntheticssample/MainActivity.kt

@wbinarytree
Copy link
Collaborator

You need to use the specific typed ViewHolder in your bind function but not the RecylerView.ViewHolder.

@leobrujah
Copy link
Author

@wbinarytree
I'm

import com.xwray.groupie.kotlinandroidextensions.Item
import com.xwray.groupie.kotlinandroidextensions.ViewHolder

As I said, kapt doesn't know enough about Item in order to generate the caching code

@wbinarytree
Copy link
Collaborator

I just tested my code as well. It seems a kotlin bug(or they changed the implementation). I have a project that runs on kotlin 1.2.31 which works fine with findCachedViewById but with the same code on kotlin 1.2.71 decompiled into getContainerView.findViewById

@leobrujah
Copy link
Author

leobrujah commented Nov 29, 2018

I'm using 1.3.10 and I get getContainerView.findViewById I'm gonna try 1.2.31 just in case

Update: still the same for me after downgrading the kotlin compiler, android studio and the version in the project

@leobrujah leobrujah changed the title Kotlin synthetic not working (possible) Kotlin synthetic caching not working (possible) Nov 30, 2018
@lisawray
Copy link
Owner

lisawray commented Dec 4, 2018

I see this also in the example project. This is extremely strange. kapt doesn't need to know anything about Item to make caching work; caching should be implemented on the ViewHolder because it extends LayoutContainer.

I'm going to open a bug and then see if I can find what the *#$% happened ...

@lisawray
Copy link
Owner

lisawray commented Dec 4, 2018

OK, bizarre. It's happening only with the published library...

If you go in https://github.com/lisawray/groupie/blob/master/example/build.gradle
and change the dependency to the library module rather than the published library

    implementation project(':library-kotlin-android-extensions')
//    implementation 'com.xwray:groupie-kotlin-android-extensions:2.2.1'

Then you see any Item (say CardItem) using findCachedViewById. If you do the reverse, it uses findViewById.

@lisawray lisawray changed the title Kotlin synthetic caching not working (possible) Kotlin synthetic caching not working in library artifact Dec 4, 2018
@lisawray
Copy link
Owner

lisawray commented Dec 4, 2018

I opened an issue with the Kotlin team: https://youtrack.jetbrains.com/issue/KT-28617

@lisawray lisawray added the bug label Dec 4, 2018
@remcomokveld
Copy link
Contributor

remcomokveld commented Aug 13, 2019

I feel like this is a pretty big problem and maybe the groupie artifact should be removed from maven as long as this is not fixed, or overridden with the two classes removed. Most users won't notice this bug but it can have a bad influence on performance.

@ValCanBuild
Copy link
Collaborator

ValCanBuild commented Aug 13, 2019

@remcomokveld I agree we need to make people aware of this issue. Would you agree if we added a note to the README and document the behaviour on com.xwray.groupie.kotlinandroidextensions.Item that would cover it?

Unpublishing seems a bit too harsh, since this is a bug that will be hopefully fixed by the Kotlin team.

@remcomokveld
Copy link
Contributor

People are most likely not going to read the readme or docs on the item, maybe deprecate it so long with that message that it will work if you copy the two classes into your own project?

@TylerMcCraw
Copy link
Contributor

I'm currently trying to track down why this is happening.
I decompiled the Kotlin bytecode for the CardItem class, first using the example project with dependencies on the library modules and then with dependencies on the published libraries.
The single difference between the two decompiled classes is this:

When depending on the published libraries, CardItem contains an extra import:
import kotlinx.android.extensions.LayoutContainer;
and it explicitly casts the viewholder reference to LayoutContainer.

published libraries:
TextView var10000 = (TextView)((LayoutContainer)viewHolder).getContainerView().findViewById(id.text);
library modules:
TextView var10000 = (TextView)viewHolder._$_findCachedViewById(id.text);

@ValCanBuild
Copy link
Collaborator

I've now updated the readme to warn people about this bug and provided instructions on what to do.

ValCanBuild pushed a commit that referenced this issue Aug 22, 2019
@ValCanBuild
Copy link
Collaborator

Great news! Yan Zhulanow from Jetbrains provided the solution to this on the Youtrack issue. New build 2.5.1 is now available, please use it and verify it fixes the problem.

@TylerMcCraw
Copy link
Contributor

@ValCanBuild 💥 It's fixed!
Thanks for the quick turnaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants