Model group building with Kotlin DSL #1012

eboudrant · 2020-06-30T17:23:04Z

Proposal for DSL usage in with EpoxyModelGroup (Issue #979), the change is based on the `ModelCollector.

It require modification in code generation. Any epoxy model class that implements ModelCollector will have an associated builder that implements ModelCollector too.

It also require a change in `EpoxyModelGroup as we need the model / layout to be lazily set (require an empty constructor).

To do :

Add a unit test for code generation

Also:

Should we make the builder implements ModelCollector only if the model is an EpoxyModelGroup ?
Should GroupModel.kt added to epoxy-adapter ? It is currently in kotlinsample app.

I like how it is now as it give some flexibility (any model can be a collector). Maybe we could also improve carousel DSL so the helper extensions are not needed (which could happen in a subsequent pull request).

So here the usage in sample app :

group {
    id("epoxyModelGroupDsl")
    layout(R.layout.vertical_linear_group)

    coloredSquareView {
        id("coloredSquareView 1")
        color(Color.DKGRAY)
    }

    coloredSquareView {
        id("coloredSquareView 2")
        color(Color.GRAY)
    }

    coloredSquareView {
        id("coloredSquareView 3")
        color(Color.LTGRAY)
    }
}

…only.

…L can be used.

elihart

@eboudrant thanks for taking a shot at this, I like the look of the DSL you've come up with.

What do you think of adding ModelCollector to the original EpoxyModelGroup, and shipping a modelGroup builder function with the core library?

If there are any other changes we want to make to the model group for the DSL this would be the best time to make this, but I can't think of anything in particular

elihart · 2020-06-30T21:34:51Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyModelGroup.java

-  private final boolean shouldSaveViewState;
+  protected final List<EpoxyModel<?>> models;
+
+  private boolean shouldSaveViewState = false;


before the DSL, the recommended approach to customize this was to subclass the EpoxyModelGroup and override shouldSaveViewState. Since that isn't possible with a DSL what do you think of just making this not private?

We will want to make sure that the default handling of setting this based on added models does not override a custom setting, so we can do something like

private boolean defaultShouldSaveViewState = false; public Boolean shouldSaveViewState = null;

and then the value we use is shouldSaveViewState ?: defaultShouldSaveViewState (i wish this file was in kotlin :P )

Ok, I missed it, how about this :

group { id(...) layout(...) shouldSaveViewState(true) ... }

It should be possible with :

private boolean shouldSaveViewStateDefault = false; @Nullable public Boolean shouldSaveViewState = null; @NonNull public EpoxyModelGroup shouldSaveViewState(boolean shouldSaveViewState) { onMutation(); this.shouldSaveViewState = shouldSaveViewState; return this; } @Override public boolean shouldSaveViewState() { // By default state is saved if any of the models have saved state enabled. // Override this if you need custom behavior. if (shouldSaveViewState != null) { return shouldSaveViewState; } else { return shouldSaveViewStateDefault; } }

ps: I was really close to migrate the class to Kotlin :)

That sounds great

elihart · 2020-06-30T21:38:39Z

epoxy-processor/src/main/java/com/airbnb/epoxy/processor/ModelBuilderInterfaceWriter.kt

@@ -74,6 +74,14 @@ class ModelBuilderInterfaceWriter(
            addTypeVariables(modelInfo.typeVariables)
            addMethods(interfaceMethods)

+            for (implements in modelInfo.superClassElement.interfaces) {
+                if (implements.toString() == ClassNames.MODEL_COLLECTOR.toString()) {


it would be nice to memoize ClassNames.MODEL_COLLECTOR.toString(), or better yet, memoize on the class level via memoizer.implementsModelCollector(modelInfo.superClassElement)

The memoizer is a shared instance of Memoizer in the processor module that can be passed into this class

Done.

Are you worried about performance in general? Theses was simple .toString() and iteration on a likely small collections.

@eboudrant thanks! I am a bit worried about performance - I recently reworked a lot of code to memoize hot spots and it improved performance several times (we dropped processing in one module from 4 seconds to half a second).

The toString functions here are a bit deceiving, one is in the ClassName class, which calls emit to a code writer to format the code for display which isn't trivial. It is internally cached, which I forgot about, but that isn't synchronized, which we have to worry about now since I added parallel processing support. The interface toString function is on the javac class ClassType which potentially loads the class by reading it from file which is not thread safe, so the memoizer helps us to synchronize.

elihart · 2020-07-06T17:58:12Z

epoxy-processor/src/main/java/com/airbnb/epoxy/processor/Memoizer.kt

+    private val implementsModelCollectorMap = mutableMapOf<GeneratedModelInfo, Boolean>()
+    fun implementsModelCollector(modelInfo: GeneratedModelInfo): Boolean {
+        return synchronized(typeMap) {
+            val implementsModelCollector = modelInfo.superClassElement.interfaces.firstOrNull {


Suggested change

val implementsModelCollector = modelInfo.superClassElement.interfaces.firstOrNull {

val implementsModelCollector = modelInfo.superClassElement.interfaces.any {

I also need to remove != null :

.firstOrNull { ... } != null -> .any { ... }

elihart · 2020-07-06T18:03:32Z

epoxy-processor/src/main/java/com/airbnb/epoxy/processor/Memoizer.kt

+            val implementsModelCollector = modelInfo.superClassElement.interfaces.firstOrNull {
+                it.toString() == ClassNames.MODEL_COLLECTOR.toString()
+            } != null
+            implementsModelCollectorMap.getOrPut(modelInfo) { implementsModelCollector }


implementsModelCollector is always calculated, even when the map already contains the value.

Could you remove the implementsModelCollector intermediate property and just put the whole calculation in the getOrPut lambda?

elihart · 2020-07-06T18:04:33Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyModelGroup.java

-  private final boolean shouldSaveViewState;
+  protected final List<EpoxyModel<?>> models;
+
+  private boolean shouldSaveViewState = false;


That sounds great

elihart · 2020-07-06T18:11:30Z

epoxy-processor/src/main/java/com/airbnb/epoxy/processor/ModelBuilderInterfaceWriter.kt

@@ -74,6 +74,14 @@ class ModelBuilderInterfaceWriter(
            addTypeVariables(modelInfo.typeVariables)
            addMethods(interfaceMethods)

+            for (implements in modelInfo.superClassElement.interfaces) {
+                if (implements.toString() == ClassNames.MODEL_COLLECTOR.toString()) {


@eboudrant thanks! I am a bit worried about performance - I recently reworked a lot of code to memoize hot spots and it improved performance several times (we dropped processing in one module from 4 seconds to half a second).

The toString functions here are a bit deceiving, one is in the ClassName class, which calls emit to a code writer to format the code for display which isn't trivial. It is internally cached, which I forgot about, but that isn't synchronized, which we have to worry about now since I added parallel processing support. The interface toString function is on the javac class ClassType which potentially loads the class by reading it from file which is not thread safe, so the memoizer helps us to synchronize.

…oup_dsl

…lass with the `@EpoxyModelClass` annotation so it can be used in the DSL

eboudrant · 2020-07-21T18:52:54Z

What do you think of adding ModelCollector to the original EpoxyModelGroup, and shipping a modelGroup builder function with the core library?

@elihart I have EpoxyModelGroup implements ModelCollector now and I also added GroupModel.kt in the core library so group { ... } can be used as DSL out of the box :

@EpoxyModelClass
abstract class GroupModel : EpoxyModelGroup()

It works in sample app but what is scary is unit tests, theses ones started to fails :

> Task :epoxy-integrationtest:testDebugUnitTest

com.airbnb.epoxy.AutoModelIntegrationTest > implicitlyAddingAutoModels FAILED
    java.lang.AssertionError at AutoModelIntegrationTest.java:61

com.airbnb.epoxy.AutoModelIntegrationTest > implicitlyAddingAutoModels2 FAILED
    java.lang.AssertionError at AutoModelIntegrationTest.java:69

com.airbnb.epoxy.AutoModelIntegrationTest > implicitlyAddingAutoModels3 FAILED
    java.lang.AssertionError at AutoModelIntegrationTest.java:77

Expected :[Model_{value=1}Model_{id=-1, viewType=2131427371, shown=true, addedToAdapter=false}, Model_{value=0}Model_{id=1, viewType=2131427371, shown=true, addedToAdapter=false}, Model_{value=2}Model_{id=2, viewType=2131427371, shown=true, addedToAdapter=false}, Model_{value=2}Model_{id=-2, viewType=2131427371, shown=true, addedToAdapter=false}, Model_{value=3}Model_{id=3, viewType=2131427371, shown=true, addedToAdapter=false}, Model_{value=0}Model_{id=-4, viewType=2131427371, shown=true, addedToAdapter=false}]
Actual   :[Model_{value=0}Model_{id=1, viewType=2131427371, shown=true, addedToAdapter=false}, Model_{value=2}Model_{id=2, viewType=2131427371, shown=true, addedToAdapter=false}, Model_{value=3}Model_{id=3, viewType=2131427371, shown=true, addedToAdapter=false}, Model_{value=0}Model_{id=-4, viewType=2131427371, shown=true, addedToAdapter=false}]

Do you have any idea? Theses tests don't seems to use model group.

edit, tests seems to pass on pull request check but locally it fails for me.

eboudrant · 2020-07-22T16:24:31Z

It works in sample app but what is scary is unit tests, theses ones started to fails :

It seems test failures are related to code gen and are un-detected in pull request check, is the build incremental on CI ?
Somehow getConfigurationForElement(controller.controllerClassElement).implicitlyAddAutoModels always return false and break auto-model tests by not adding the statement "setControllerToStageTo(controller.\$L, controller)".

elihart

@eboudrant updates looks great! I just have a few small comments.

Sorry about the test trouble, I can pull your branch later today and dig around. The build should not be incremental on CI, it just builds from clean.

elihart · 2020-07-22T17:10:43Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyModelGroup.java

+  private boolean shouldSaveViewStateDefault = false;
+
+  @Nullable
+  public Boolean shouldSaveViewState = null;


should this not be public? do we want to force usage of shouldSaveViewState so the mutation check is always done?

Good catch, I can't remember why I changed it to public

elihart · 2020-07-22T17:11:47Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/GroupModel.kt

+package com.airbnb.epoxy
+
+/**
+ * An [EpoxyModelGroup] usable in DSL.


could be nice to give a small example of usage (or we can link to the wiki docs here later)

Ok, also wdyt to add the ModelCollector interface / add method in this small class instead of in EpoxyModelGroup class, like :

@EpoxyModelClass abstract class GroupModel : EpoxyModelGroup(), ModelCollector { override fun add(model: EpoxyModel<*>) { super.addModel(model) } }

Because in any case EpoxyModelGroup can't be used directly in the DSL. It allow to separate responsibilities.

elihart · 2020-07-22T17:17:54Z

epoxy-processor/src/main/java/com/airbnb/epoxy/processor/Memoizer.kt

@@ -359,6 +359,17 @@ class Memoizer(
            }
        }
    }
+
+    private val implementsModelCollectorMap = mutableMapOf<GeneratedModelInfo, Boolean>()


to actually get good cache hits on this the parameter of the function should be classElement: TypeElement
then the map can be mutableMapOf<Name, Boolean>(), with getOrPut(classElement.qualifiedName) as the key to make sure it is consistent across type instances

elihart · 2020-07-22T17:20:15Z

epoxy-processor/src/main/java/com/airbnb/epoxy/processor/Memoizer.kt

+    fun implementsModelCollector(modelInfo: GeneratedModelInfo): Boolean {
+        return synchronized(typeMap) {
+            implementsModelCollectorMap.getOrPut(modelInfo) {
+                modelInfo.superClassElement.interfaces.any {


Should this check the super class recursively? If there are multiple subclasses a superclass up the chain could have the interface instead.

maybe interfaces.any {...} || implementsModelCollector(superClassElement.superclass) (psuedocode)

elihart · 2020-07-22T18:06:58Z

@eboudrant I pulled your branch but can't reproduce the test failures. it passes for me with ./gradlew clean testDebug

…ed directly in the DSL

eboudrant · 2020-07-22T18:56:55Z

@eboudrant I pulled your branch but can't reproduce the test failures. it passes for me with ./gradlew clean testDebug

@elihart,./gradlew clean testDebug pass for me too, then when I make a modification to ControllerWithImplicitlyAddedModels3 and run ./gradlew testDebug, it fails. Ex, here the modification I do :

> Task :epoxy-integrationtest:testDebugUnitTest

com.airbnb.epoxy.AutoModelIntegrationTest > implicitlyAddingAutoModels FAILED
    java.lang.AssertionError at AutoModelIntegrationTest.java:61

com.airbnb.epoxy.AutoModelIntegrationTest > implicitlyAddingAutoModels2 FAILED
    java.lang.AssertionError at AutoModelIntegrationTest.java:69

com.airbnb.epoxy.AutoModelIntegrationTest > implicitlyAddingAutoModels3 FAILED
    java.lang.AssertionError at AutoModelIntegrationTest.java:77

100 tests completed, 3 failed

> Task :epoxy-integrationtest:testDebugUnitTest FAILED

Weird

elihart · 2020-07-23T17:02:27Z

@eboudrant this is very likely a bug with incremental processing, probably because of the kapt issue where package-info.java files are not properly included in incremental kapt processing. This leads to the configuration info from @PackageEpoxyConfig to not be included, which then causes the failure you see.

This is unrelated to your change, so let's just ignore it for now. The workaround I will have to apply l later is similar to #1020

elihart

Latest code looks great, thanks for all the iteration!

really happy how this turned out, great addition!

I'll get another beta release out soon to include this

eboudrant · 2020-07-24T16:39:57Z

@elihart how close are we from a 4.0.0 out of beta? Is there a list of things to do apart from the package info issue?

elihart · 2020-07-25T21:55:07Z

@eboudrant I would say it is very close. We have been using it in production for a long time with no issues. The package-info issue is the only known problem, and that is only going to be a sporadic incremental issue that requires a clean (or sometimes deleting local build cache), and only if you happen to be using that config annotation.

My current plan is to fix the package-info problem, and then I can make a normal release

plnice · 2020-08-05T10:00:37Z

@elihart is this available in any beta/snapshot build?

elihart · 2020-08-06T17:06:58Z

@plnice not yet, it will be in the next release, whenever I have time to do that

elihart · 2020-09-09T17:40:44Z

this is in the 4.0.0 release

elihart · 2020-09-09T18:12:02Z

I've updated the wiki to mention this dsl https://github.com/airbnb/epoxy/wiki/Grouping-Models#kotlin-dsl

eboudrant added 3 commits June 30, 2020 09:54

Add an empty constructor for EpoxyModelGroup, protected visibility …

0f470b6

…only.

If the model implements ModelCollector make the build too so the DS…

6e0d4f4

…L can be used.

Use the new model group DSL in the sample app

9bb7251

elihart reviewed Jun 30, 2020

View reviewed changes

Add implementsModelCollector in memoizer

dc287c5

elihart reviewed Jul 6, 2020

View reviewed changes

eboudrant added 4 commits July 21, 2020 09:02

Fix memoizer implementation

d632e86

Add shouldSaveViewState property to model group

894afef

Merge branch 'master' of https://github.com/airbnb/epoxy into modelgr…

d314a68

…oup_dsl

Make EpoxyModelGroup a ModelCollector and create a GroupModel c…

08e62be

…lass with the `@EpoxyModelClass` annotation so it can be used in the DSL

Can be simnplified

736dcb3

eboudrant added 2 commits July 22, 2020 09:41

force codegen on pr check

bcd2cb0

revert last change

106988e

elihart approved these changes Jul 22, 2020

View reviewed changes

eboudrant added 4 commits July 22, 2020 11:34

Move ModelCollector code to GroupModel as EpoxyModelGroup can't be us…

61af0bf

…ed directly in the DSL

Add a small doc/usage

f66121a

Check class hierarchy for ModelCollector

97a8a66

Reset shouldSaveViewState to private

42448ba

elihart approved these changes Jul 23, 2020

View reviewed changes

elihart merged commit 1f14d64 into airbnb:master Jul 23, 2020

plnice mentioned this pull request Aug 5, 2020

Allow specifying layout in GroupModel constructor #1026

Merged

This was referenced Mar 18, 2021

Bump epoxy-databinding from 3.11.0 to 4.4.3 ntsk/Qii#152

Closed

Bump epoxyVersion from 3.11.0 to 4.4.3 ntsk/Qii#153

Closed

	val implementsModelCollector = modelInfo.superClassElement.interfaces.firstOrNull {
	val implementsModelCollector = modelInfo.superClassElement.interfaces.any {

Model group building with Kotlin DSL #1012

Model group building with Kotlin DSL #1012

Uh oh!

Conversation

eboudrant commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elihart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eboudrant Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eboudrant commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eboudrant commented Jul 22, 2020

Uh oh!

elihart left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elihart commented Jul 22, 2020

Uh oh!

eboudrant commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elihart commented Jul 23, 2020

Uh oh!

elihart left a comment

Choose a reason for hiding this comment

Uh oh!

eboudrant commented Jul 24, 2020

Uh oh!

elihart commented Jul 25, 2020

Uh oh!

plnice commented Aug 5, 2020

Uh oh!

elihart commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elihart commented Sep 9, 2020

Uh oh!

elihart commented Sep 9, 2020

Uh oh!

Uh oh!

eboudrant commented Jun 30, 2020 •

edited

Loading

eboudrant Jul 21, 2020 •

edited

Loading

eboudrant commented Jul 21, 2020 •

edited

Loading

elihart left a comment •

edited

Loading

eboudrant commented Jul 22, 2020 •

edited

Loading

elihart commented Aug 6, 2020 •

edited

Loading