Enable Enable @ModelProp @TextProp and @CallbackProp with all options on field attribute #343

juliafu1 · 2017-11-06T06:39:10Z

Enable Enable @ModelProp @TextProp and @CallbackProp with all options on field attribute

elihart

@snowywinter2013 Cool, looks like the right idea!

We will want to make sure that @NullOnRecycle works.

Once this is cleaned up more I can help you write tests

elihart · 2017-11-10T20:55:53Z

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

-    ): Boolean =
-            validateExecutableElement(methodElement, propAnnotation, 1)
+    ): Boolean {
+        if (prop !is ExecutableElement && prop.kind != ElementKind.FIELD) {


return when (prop) {
is ExecutableElement -> validateExecutableElement
is VariableElement -> validateFieldElement
else -> {

log...
false
}

elihart · 2017-11-10T21:08:36Z

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

+                "\$L.\$L(\$L);\n")
+    }
+
+    private fun buildCodeBlockToSetAttribute(


if buildCodeBlockToSetAttribute isn't used elsewhere I don't think we need to make it a separate method. it's a bit weird to pull just this out and not the other code block

elihart · 2017-11-10T21:08:47Z

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

@@ -9,31 +9,46 @@ import java.util.*
 import javax.lang.model.element.*
 import javax.lang.model.type.*
 import javax.lang.model.util.*
+import com.sun.corba.se.impl.activation.ServerMain.logError


this seems like the wrong import

elihart · 2017-11-10T21:08:55Z

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

@@ -9,31 +9,46 @@ import java.util.*
 import javax.lang.model.element.*
 import javax.lang.model.type.*
 import javax.lang.model.util.*
+import com.sun.corba.se.impl.activation.ServerMain.logError
+import javax.lang.model.element.ElementKind
+import com.sun.tools.javac.util.BaseFileManager.getKind


this seems like the wrong import too

elihart · 2017-11-10T22:02:51Z

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


 private val NOT_NULL_ANNOTATION_SPEC = AnnotationSpec.builder(NotNull::class.java).build()
 private val NON_NULL_ANNOTATION_SPEC = AnnotationSpec.builder(NonNull::class.java).build()

+internal enum class ViewAttributeType {


can you make this a sealed class instead?

elihart · 2017-11-10T22:11:46Z

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

+            element: Element,
+            errorLogger: ErrorLogger
+    ) {
+        when {


it's cleaner to return a value than to set a field. the field should be a val to be safer

elihart · 2017-11-10T22:13:30Z

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


    init {
        val propAnnotation = setterElementOnView.getAnnotation(ModelProp::class.java)
        val textAnnotation = setterElementOnView.getAnnotation(TextProp::class.java)
        val callbackAnnotation = setterElementOnView.getAnnotation(CallbackProp::class.java)

        val options = HashSet<Option>()
-        val param = setterElementOnView.parameters[0]
+        val param = if (setterElementOnView is ExecutableElement) setterElementOnView.parameters[0]


setterElementOnView should be renamed, it doesn't make sense for fields

elihart · 2017-11-10T22:14:36Z

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

@@ -43,6 +58,11 @@ internal class ViewAttributeInfo(
            options.addAll(Arrays.asList(*propAnnotation.options))
            options.addAll(Arrays.asList(*propAnnotation.value))
        } else if (textAnnotation != null) {
+            //TODO juliafu support textProp for field
+            if (viewAttributeTypeName == ViewAttributeType.Field) {


we shouldn't have to do anything special here to support it

elihart · 2017-11-10T22:14:47Z

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

@@ -52,6 +72,10 @@ internal class ViewAttributeInfo(
            }
            options.add(Option.GenerateStringOverloads)
        } else if (callbackAnnotation != null) {
+            if (viewAttributeTypeName == ViewAttributeType.Field) {


elihart · 2017-11-10T22:15:02Z

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

@@ -75,8 +99,8 @@ internal class ViewAttributeInfo(
        modelName = modelInfo.generatedName.simpleName()
        modelPackageName = modelInfo.generatedClassName.packageName()

-        this.viewSetterMethodName = setterElementOnView.simpleName.toString()
-        propName = removeSetPrefix(viewSetterMethodName)
+        this.viewSetterPropName = setterElementOnView.simpleName.toString()


I don't know if the name viewSetterPropName makes sense for fields

juliafu1 · 2017-11-20T07:36:33Z

I updated the code and addressed all comments. This makes all props, options, default value works. It is quite complete and fully working.
The testing is here: https://docs.google.com/document/d/1sT-AUi-Iiv3qI6dTfzbW3szRteq9qwaY-m1L7vnSY58/edit#bookmark=id.t7rle9y2y6xm
@elihart PTAL when you have time. No hurry. I look forward to writing tests

elihart

@juliafu1 looks pretty great! just a bunch of small things, once it is cleaned up lets write some tests and then merge this :)

elihart · 2017-12-01T00:04:28Z

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

                                       resourceProcessor))
    }

+    //TODO juliafu handle the super class case


can you add more details to this comment? not sure what needs to be done exactly

elihart · 2017-12-01T00:06:35Z

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

            element.enclosingElement?.let { modelClassMap[it] }

    companion object {
        val modelPropAnnotations = listOf(ModelProp::class, TextProp::class,
                                          CallbackProp::class).map { it.java }
+        val modelPropAnnotationsNames = listOf(ModelProp::class, TextProp::class,


it would be nice not to duplicate the list of prop classes

elihart · 2017-12-01T00:13:12Z

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

-                            viewAttribute.viewSetterMethodName,
-                            getValueToSetOnView(viewAttribute, boundObjectParam))
+        val expression = if (viewAttribute.viewAttributeTypeName == ViewAttributeType.Field)
+            "\$L.\$L=\$L;\n" else "\$L.\$L(\$L);\n"


I think we can do CodeBlock.builder().addStatement()... to avoid having to do ;\n manually.

elihart · 2017-12-01T00:14:05Z

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

-                            viewAttribute.viewSetterMethodName,
-                            getValueToSetOnView(viewAttribute, boundObjectParam))
+        val expression = if (viewAttribute.viewAttributeTypeName == ViewAttributeType.Field)
+            "\$L.\$L=\$L;\n" else "\$L.\$L(\$L);\n"


this is mostly duplicated with the unbind code, the only difference is that the unbind code sets null. can you think of a way to DRY it out?

elihart · 2017-12-01T00:18:46Z

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

+    ): ViewAttributeType? {
+        return when {
+            element is ExecutableElement -> ViewAttributeType.Method
+            element.kind === ElementKind.FIELD -> ViewAttributeType.Field


don't use triple equals here

can you do when(element.kind) is FIELD ... is METHOD...?

It couldn't use 'is'. The compiler tells: "'is' over enum entry is not allowed, use comparison instead". I can use '==' instead of '===' but it actually doesn't make a difference for enum

elihart · 2017-12-01T00:21:50Z

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

+            if (ModelViewProcessor.modelPropAnnotationsNames.contains(ClassName.get(annotationElement as TypeElement))) {
+                continue
+            }
+            val builder = AnnotationSpec.builder(ClassName.get(annotationElement))


save ClassName.get(annotationElement) to a val since it is done twice?

elihart · 2017-12-01T00:22:25Z

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

@@ -274,7 +298,10 @@ internal class ViewAttributeInfo(
    ) {
        for (annotationMirror in paramElement.annotationMirrors) {
            val annotationElement = types.asElement(annotationMirror.annotationType)
-            val builder = AnnotationSpec.builder(ClassName.get(annotationElement as TypeElement))
+            if (ModelViewProcessor.modelPropAnnotationsNames.contains(ClassName.get(annotationElement as TypeElement))) {


can do if (annotationClass in ModelViewProcessor.modelPropAnnotationsNames) continue

juliafu1 · 2017-12-11T17:02:23Z

@elihart all comments addressed. I also tested the parent view case (if the parent view has an attribute, the generated child view model shall have that attribute as well). The test is added in https://docs.google.com/document/d/1sT-AUi-Iiv3qI6dTfzbW3szRteq9qwaY-m1L7vnSY58/edit#bookmark=id.t7rle9y2y6xm. Thank you.

juliafu1 · 2017-12-14T22:14:16Z

@elihart Tests added. PTAL. Thank you

elihart · 2017-12-16T06:41:20Z

Great! tests look pretty exhaustive - I'll try to give this one final lookover soon and then merge 👍

juliafu1 · 2017-12-21T22:25:31Z

Thank you!

elihart mentioned this pull request Nov 9, 2017

Support annotating a field with @Modelprop #292

Closed

elihart reviewed Nov 10, 2017

View reviewed changes

juliafu1 force-pushed the juliafu/add_model_prop_epoxy_attribute branch 2 times, most recently from 8703058 to 107eff6 Compare November 20, 2017 07:32

juliafu1 changed the title ~~Enable @ModelProp on field attribute~~ Enable Enable @ModelProp @TextProp and @CallbackProp with all options on field attribute Nov 20, 2017

juliafu1 force-pushed the juliafu/add_model_prop_epoxy_attribute branch from 107eff6 to 893e28a Compare November 22, 2017 17:25

elihart reviewed Dec 1, 2017

View reviewed changes

juliafu1 force-pushed the juliafu/add_model_prop_epoxy_attribute branch from 893e28a to 77e8427 Compare December 11, 2017 17:01

juliafu1 force-pushed the juliafu/add_model_prop_epoxy_attribute branch 4 times, most recently from 574a4f2 to b6ff338 Compare December 14, 2017 22:08

Enable props and options on fields

28cd1f9

juliafu1 force-pushed the juliafu/add_model_prop_epoxy_attribute branch from b6ff338 to 28cd1f9 Compare December 14, 2017 22:13

elihart merged commit 17a9be1 into airbnb:master Dec 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Enable @ModelProp @TextProp and @CallbackProp with all options on field attribute #343

Enable Enable @ModelProp @TextProp and @CallbackProp with all options on field attribute #343

juliafu1 commented Nov 6, 2017 •

edited

Loading

elihart left a comment

elihart Nov 10, 2017

elihart Nov 10, 2017

elihart Nov 10, 2017

elihart Nov 10, 2017

elihart Nov 10, 2017

elihart Nov 10, 2017

elihart Nov 10, 2017

elihart Nov 10, 2017

elihart Nov 10, 2017

elihart Nov 10, 2017

juliafu1 commented Nov 20, 2017 •

edited

Loading

elihart left a comment

elihart Dec 1, 2017

elihart Dec 1, 2017

elihart Dec 1, 2017

elihart Dec 1, 2017

elihart Dec 1, 2017

elihart Dec 1, 2017

juliafu1 Dec 11, 2017

elihart Dec 1, 2017

elihart Dec 1, 2017

juliafu1 commented Dec 11, 2017

juliafu1 commented Dec 14, 2017

elihart commented Dec 16, 2017

juliafu1 commented Dec 21, 2017

Enable Enable @ModelProp @TextProp and @CallbackProp with all options on field attribute #343

Enable Enable @ModelProp @TextProp and @CallbackProp with all options on field attribute #343

Conversation

juliafu1 commented Nov 6, 2017 • edited Loading

elihart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliafu1 commented Nov 20, 2017 • edited Loading

elihart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliafu1 commented Dec 11, 2017

juliafu1 commented Dec 14, 2017

elihart commented Dec 16, 2017

juliafu1 commented Dec 21, 2017

juliafu1 commented Nov 6, 2017 •

edited

Loading

juliafu1 commented Nov 20, 2017 •

edited

Loading