Android ViewBinding: adding an example in the sample project.

[glureau-betclic]

This will require everyone to use AndroidStudio 3.6+

AndroidStudio 3.6 + is required to develop on the epoxy project, and is required if you want to use ViewBinding in your app project. You can still use epoxy without ViewBinding with older versions of AndroidStudio.

Provide a sample for those who wants to use ViewBinding with a data class.
A new ViewBinding is created when the VH is bound, and nullified when unbound.

Updates were required for AndroidPlugin 3.5.3 -> 3.6.1 and Gradle 5.6.3 -> 5.6.4

I didn't note all not-relevant layout for ViewBinding, can do it if you prefer. (Other layouts could be ignored for better build time, but it's a simple, so may be better to keep it simple?)

The chosen strategy of bind/unbind with a new ViewBinding could be debatable, this is simpler to implement and ensure there is no memory leak, but I presume that ViewBinding should be coupled with the view (as a ViewHolder is supposed to do). I could easily use the inflate method on the generated binder in the buildView() and store the ViewBinding object in the View tag for example. What's your take on this @elihart ?

Eventually I got an AndroidStudio warning, even if it builds fine. Kotlin plugin issue?

[elihart]

@glureau-betclic Thanks for putting this up. I think you're right when you said that ViewBinding should be coupled with the view (as a ViewHolder is supposed to do). With the current implementation it will have to keep looking up the view binding each time a new view is bound, which happens a lot on scroll. I think we should keep the viewholder pattern where we only have to do it once for the lifetime of the view.

Can you subclass EpoxyModelWithHolder instead? I think a subclass of EpoxyHolder can manage the binding. It would also be good to handle the overloads like bind(@NonNull T holder, @NonNull EpoxyModel<?> previouslyBoundModel)

I don't think the binding ever needs to be nulled out either

[elihart]

bind can be called multiple times when data is updated. I think this only needs to be done if _binding is null. _binding = _binding ?: bindFunction(view)

[elihart]

what if this was T.bind() so they didn't have to do binding.textView binding.imageView etc. This all removes the need for them to know about the binding property name

[elihart]

why override this?

[glureau-betclic]

Subclass EpoxyModelWithHolder looks definitely better. ViewBinding as now the same lifecycle than the view as far as I understand, and I defined a T.bind() method instead, makes code clearer even if in this example the 2 'title' variables could be misleading.

To be honest, I started with the DataClass approach as I completely missed this approach that requires no kapt in the documentation and I think it could be really great to approach the problem with less kapt, but this is another discussion.

[glureau-betclic]

Travis don't seem to work well:

> Task :epoxy-modelfactorytest:testDebugUnitTest
com.airbnb.epoxy.ModelFactoryViewProcessorTest > baseModel FAILED
    java.lang.IllegalArgumentException at ModelFactoryViewProcessorTest.kt:13
com.airbnb.epoxy.ModelFactoryViewProcessorTest > baseModelWithFinalAttribute FAILED
    java.lang.IllegalArgumentException at ModelFactoryViewProcessorTest.kt:21

Is it related to my changes?

[elihart]

I would guess this is what is causing the tests to fail

[glureau-betclic]

You're right. I'm still trying to understand what's the actual change that breaks JavaFileObject when trying to get resources, and how to fix it. Any input appreciated.

JavaFileObjects.forResource(inputFile)

[elihart]

I think you can avoid passing ViewBindingHolderItemBinding::bind by using reflection inside ViewBindingEpoxyModel to get the static bind function. This would be a nicer user experience. That reflected method lookup can be cached so it is only done once for each class of viewholder (or done in a map to do once per binding type)

[glureau-betclic]

Implemented the reflection approach, a bit more verbose/cryptic and with 2 cache levels (EpoxyModel via lazy to retrieve the generics class info + Static for caching the binding method given a specific class).

Looks clearly better for the user experience.

[elihart]

I'm a bit rusty on the kotlin epoxy model approach, but I believe it would be possible to combine it with the viewholder approach, so we would get this improved ease of binding along with being able to use kotlin data classes instead of generated models off of annotations

[glureau-betclic]

Should I consider this in this PR? I feel like it's a distinct topic since this PR aims to provide a 1st approach with standard generation code while the data class approach is a bit different and could provide another way with different restrictions.

EDIT: I may have misunderstood your message, please can you provides some guidance?

[elihart]

nevermind, I don't know that that made sense

[glureau-betclic]

I've found that AGP 3.6 is not compatible with JavaFileObjects.forResources(String), a SSCCE to showcase the problem can be found here: https://github.com/glureau/AGP_3_6_JavaFileObjects_forResources

I've locally a patch version that replace string path like this:

    val model = JavaFileObjects.forResource(inputFile)

to this:

    val inputFileUrl =  File("build/intermediates/sourceFolderJavaResources/debug/$inputFile").toURI().toURL()
    val model = JavaFileObjects.forResource(inputFileUrl)

It patches the issue, but it involves updating a lot of files and it's really ugly, I don't think that's the way to go. I'll go create issues on AGP and google.testing.compile projects, to try to understand what's the problem, as it can be a blocker for Epoxy at some point (no matter if this PR is merged or not, Epoxy can't upgrade AGP due to that regression).

[glureau-betclic]

https://issuetracker.google.com/issues/154441059

[elihart]

I think this should be synchronized, in case two of the same model class access at the same time

[elihart]

use getOrPut to simplify?

[elihart]

i would throw here if it is null, and not make the return type nullable. better to give a clear error message

[elihart]

this may not be right in the case of multiple layers of subclasses

[elihart]

as an optimization I would:

1. make kClass accessed as a function, since the two lazy calls are slightly expensive and we only need to access it once
2. have the map key be this::class, falling back to calculate kclass on a cache miss to calculate the binding method. this prevents all the reflection in most cases

[glureau-betclic]

Since this::class is the naw key, I moved the reflection to the getOrPut. Is it ok?

[elihart]

I would do the binding method lookup in the viewholder instead of the model because:

1. many more models are created than viewholders. models are constantly rebuilt while viewholders are reused, which reduces the reflection
2. the method is only used in the viewholder, and the model doesn't need to know about it

[elihart]

@glureau-betclic thanks for looking into the JavaFileObjects.forResources problem, I appreciate you debugging! It seems like that will prevent us from merging this for now, but it will be great to have it up as an example.

I think the reflection approach makes for a really clean user experience, which is great. I just have some suggestions.

As far as fixing the tests, your workaround may be ok for now if we wrap that in a shared util or extension function, but I can do that work sometime when we want to upgrade AGP (although if you want to do it instead it would be most appreciated :) )

[ColtonIdle]

@elihart so if I understand the conversation of this PR correctly, the view binding example is mostly correct, it's just that there's some issue with Travis that's actually preventing this to be merged in?

[eboudrant]

From the description :

This will require everyone to use AndroidStudio 3.6+

If I understand correctly you will need AGP 3.6+ only if you want to use view binding in your project but a projet with AGP. 3.5 (or lower) will still be able to use Epoxy once merged, right ?

[elihart]

@ColtonIdle yes, this approach should work nicely. AGP 3.6 just has a change that breaks our test setup that we need to fix first.

@eboudrant yes, this PR only changes the sample project, which you will need AGP 3.6 to build. But as far as using the library there is no AGP version requirement.

[glureau-betclic]

Trying to push again this PR. After rebased, I've added the patch extension that I named 'GuavaPatch', as it looks like the issue comes from Guava relying on ClassLoader while AGP 3.6 change the behaviour of it. If you've any better name, let me know.

From there, AndroidStudio was failing to run some tests when using Robolectric runner. I've tried to upgrade Robolectric to 4.3.1 (currently 4.3), and the message error changed, but then going back to 4.3 to test, even after gradle sync, run on AS was not blocking anymore (still failing but for another reason). In the meantime, running tests from gradlew was ok. So I let this comment here as I don't know if it was just a bad local configuration or something else.

For example, ModelFactoryViewProcessorTest#baseModel outputs :

Found 16 nodes that differed in expected and actual trees. 

Difference in expected tree and actual tree.
Expected node: Line 141 COMPILATION_UNIT->CLASS(ModelFactoryBasicModelWithAttribute_)->METHOD(id)->VARIABLE(arg0)
Actual node: Line 141 COMPILATION_UNIT->CLASS(ModelFactoryBasicModelWithAttribute_)- 
METHOD(id)->VARIABLE(ids)
Expected variable name to be <arg0> but was <ids>.

I suppose that for some reasons, parameter names are not generated the same way in AS?!

To confirm my hypothesis, I changed the variable names from "arg0" to "ids" to fix all the errors in AndroidStudio for this specific test. It works, but indeed gradle tests are failing now... I've tried to invalidate AS cache, restart, removed the idea files and re-import from build.gradle, specify the gradle but I'm still blocked on this point. If someone want to clone and check if there is the same issue, it could help a lot, thanks.

At least UI tests should be fixed after the last commit.

[glureau-betclic]

I suppose this unit-test was broken. There is no ModelWithAbstractClass_.java in the repository, and since I'm not sure of the expected behaviour... can you help on this @elihart ?

[elihart]

not sure what this was doing, but it looks like there is a ModelWithAbstractClass.java file in test resources that maybe it was supposed to be

[glureau-betclic]

Actually the tests is in 2 steps, the 1st one assert the ModelWithAbstractClass (just a compileWithoutErrors), and the 2nd part (highlighted here) was about "not generating subclasses (?)" but I don't understand the case.

If I put the same file on this test, the path is good so it doesn't get IllegalArgumentException and test fails...

Looks like it's a 3 years-old code (you've written in feb. 2017), so maybe some library updates changed the expected behaviour, I don't know how to proceed on this... maybe just delete this 2nd part of the test?

[glureau-betclic]

Sorry I didn't remove the 2nd part of this test, was waiting your confirmation but it was merged before, oups 😄

[elihart]

you can add @file:JvmName("GuavaPatch") to the top of the file instead of making this object for Java

[elihart]

Can this holder class be parameterized with <in T : ViewBinding> too? that seems more accurate, since it should have the same type, and that may allow us to get the type directly from the holder instead of passing in epoxyModelClass

[glureau-betclic]

Not sure how to do that actually, because of the generated code:

 * Generated file. Do not modify! */
public class ItemViewBindingEpoxyHolder_ extends ItemViewBindingEpoxyHolder implements GeneratedModel<ViewBindingHolder<T>>, ItemViewBindingEpoxyHolderBuilder {
  private OnModelBoundListener<ItemViewBindingEpoxyHolder_, ViewBindingHolder<T>> onModelBoundListener_epoxyGeneratedModel;

In the generated file, the 'T' is not defined as generic of the ItemViewBindingEpoxyHolder_.
From my limited understanding, I suppose it's because the processor doesn't handle ViewHolder with generics. Am I wrong? We could change that but I feel it's not that easy :)

[elihart]

i see what you mean, this is fine :)

[elihart]

Thanks for adding the workaround for the tests and spending so much time looking into it!

[glureau-betclic]

Just a thought, why not providing micro-libraries with the samples generic classes?

For the user, no need to copy-paste a weird class into its codebase. (weird because using reflection, static fields, optimized/harder code or not respecting same code standards...) Instead user just needs to add a new dependency in the build.gradle.

For the maintainers, code improvements on these classes can be provided for everyone, instead of asking people to get a fresh copy-pasted file because someone noticed an issue (or worst, not communicating on the fix.). Also it will require to add some unit-tests on these classes (that could be great).

[elihart]

Just a thought, why not providing micro-libraries with the samples generic classes?

For the user, no need to copy-paste a weird class into its codebase. (weird because using reflection, static fields, optimized/harder code or not respecting same code standards...) Instead user just needs to add a new dependency in the build.gradle.

For the maintainers, code improvements on these classes can be provided for everyone, instead of asking people to get a fresh copy-pasted file because someone noticed an issue (or worst, not communicating on the fix.). Also it will require to add some unit-tests on these classes (that could be great).

A few reasons I don't want to commit to that:

• have to worry about breaking changes and api updates
• more code to be responsible for and have people report issues for
• there is a certain level of polish and performance I want in officially packaged and released code/api's - i don't want to have to do that for helper functions like this
• there are many many ways to approach this, and i don't want to commit to a single one, just give people options and ideas to choose from

[elihart]

@glureau-betclic merged, thanks again! I will update the wiki noting that this sample exists

[j-roskopf]

Awesome to see an example with ViewBinding! Are there any thoughts on a non reflection based solution? I have tried myself without much luck. Curious to hear if there are any concerns with moving forward with a reflection based approach?

[elihart]

@j-roskopf the reflection caching should be very efficient so i don't have performance concerns.

[ColtonIdle]

@glureau-betclic I may not be following correctly, but is there a TLDR on how to use epoxy with view binding? I wasn't able to find it in the readme or wiki (maybe my search skills are failing me), but I noticed a bunch of helper classes were added and I'm not sure what I'm supposed to copy into my project.

[ColtonIdle]

@elihart did you by chance update the wiki? I didn't see anything there (not sure if my searching skills are failing me) and I'm not sure where to begin to add view binding.

[elihart]

I didn't update the wiki yet. You can copy one of the helper class approaches in the sample app from this PR into your code base and either modify it or use it as is.

I would recommend understanding what the helpers are and what they do exactly instead of simply copying them - the point is this was an exercise to demonstrate how view binding could be integrated, but you should understand what is going on and decide on whether it is the right approach for you

[ColtonIdle]

Makes sense. Thanks

[ottne]

It should be mentioned that Proguard/R8 need a custom rule to not obfuscate the static bind methods. Otherwise, the method lookup fails at runtime leading to crashes.

Instead of taking the reflective approach, why not treat ViewBinding with holders as a first class citizen and generate code that uses each ViewBinding's static factory?

[elihart]

Thanks for pointing out the custom rule needed for obfuscation.

The generated approach seems possible and is an interesting idea. I'm not able to personally do that work now, but if someone would like to contribute to it I would be open to reviewing

[ottne]

Thanks for the positive feedback. I will try to contribute as soon as I find the time.

[glureau-betclic]

Thanks for pointing this out @bompf .

Have you already a proguard rule for that?

Could be great to have it here (until there is a more complete documentation), I could work on it if not ready (not super used to proguard but can make some tests).

[ottne]

I'm using the following rule at this moment. I'm not sure if it is too broad.

-keepclassmembers class * extends androidx.viewbinding.ViewBinding {
    public static *** bind(android.view.View);
}

[glureau-betclic]

Thanks @bompf, looks good enough to me 👍

@elihart Should we add this in a comment inside the relevant classes, for devs to "avoid" this trap?

[elihart]

sure, someone could make that change