Support ResType on PropDefault annotation #174

pavlospt · 2017-05-04T10:07:04Z

Initial work on adding support for ResType on @PropDefault ( #81 )

I went with the solution of having the Builder initializing the Props that have a default value marked as ResType, instead of passing down the ComponentContext which I tried to implement but it was introducing a quite huge change.

In order to reduce the review scope I have only added generation for resolveDimenSizeRes() and will add the rest before closing the PR, in order to have a naming and feature compliance agreement after the initial review. Also, I believe that after resolving the resource on a marked @PropDefault we should generate resource helper methods like we do for @Prop.

Let me know what you think!

/cc: @emilsjolander

P.S.: There is a big diff on the ComponentImplGenerator due to different Code Style, so If you could share the one you are using so that we could be aligned and reduce the diffs, would be great.

emilsjolander · 2017-05-04T11:22:58Z

@pavlospt Thanks! I'll have a look today or tomorrow and give more detailed feedback. Before that could you please revert the style changes you made? We have our preferred formatting style and would rather not change it, also makes the PR harder to review.

pavlospt · 2017-05-04T11:30:33Z

@emilsjolander Yeap that's what I wrote on the ps, could you share your formatting style so that I can revert it? Because right now I don't see how I could that. Let me know if you have any ideas!

pavlospt · 2017-05-04T12:40:52Z

Code style issues are fixed!

emilsjolander · 2017-05-04T12:51:24Z

We don't have a formal formatting style really. We should probably work on one though 👍 Just look at how the rest of the code in the files are formatted and try to follow the same style.

emilsjolander

cc @IanChilds for annotation processor changes.

emilsjolander · 2017-05-04T12:57:44Z

sample/src/main/java/com/facebook/samples/litho/lithography/FeedItemCardSpec.java

@@ -8,8 +8,10 @@

 package com.facebook.samples.litho.lithography;

+import android.graphics.drawable.Drawable;


Seems unused

emilsjolander · 2017-05-04T12:57:49Z

sample/src/main/java/com/facebook/samples/litho/lithography/FeedItemCardSpec.java

 import com.facebook.litho.Column;

+import com.facebook.litho.annotations.ResType;


seems unused

emilsjolander · 2017-05-04T12:58:29Z

sample/src/main/java/com/facebook/samples/litho/playground/PlaygroundComponentSpec.java


 @LayoutSpec
 public class PlaygroundComponentSpec {

+  @PropDefault(resType = ResType.DIMEN_SIZE)


Could you revert these changes? Good to see example used but we don't want to add a bunch of stuff to the playground.

emilsjolander · 2017-05-04T12:58:35Z

sample/src/main/res/values/dimens.xml

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="utf-8"?>


Remove this file.

emilsjolander · 2017-05-04T13:01:08Z

litho-processor/src/main/java/com/facebook/litho/specmodels/generator/BuilderGenerator.java

@@ -733,4 +768,21 @@ private static MethodSpec generateReleaseMethod(SpecModel specModel) {
        .addStatement("$L.release(this)", BUILDER_POOL_FIELD)
        .build();
  }
+
+  private static String generateInitializer(PropDefaultModel propDefaultModel,


If a line exceeds 100 we put all the parameters on the next line, including the first one 👍

@emilsjolander Do you mean 100 chars? This line seems to be 78 chars including the leading space.

emilsjolander · 2017-05-04T13:01:57Z

litho-processor/src/main/java/com/facebook/litho/specmodels/generator/BuilderGenerator.java

+    StringBuilder builtInitializer = new StringBuilder();
+
+    if (propDefaultModel.hasResType()) {
+      if (propDefaultModel.mResType == ResType.DIMEN_SIZE) {


We should handle all res types. See how this is done for the builder methods.

pavlospt · 2017-05-04T13:23:01Z

@emilsjolander thank you for the review, will try to fix them and finalize it.

pavlospt · 2017-05-04T16:40:15Z

@emilsjolander Fixed most of the review changes, one thing that is not clear though: in order to generate the initializers, should I create new variables?

What I mean is:

If someone marks a @PropDefault with a ResType then it would most probably require it to be an int in order to accept the R reference, which means that we would need to generate a variable for the resolved type of the ResType.

pavlospt · 2017-05-04T19:20:52Z

I had a discussion with @charbgr for possible approaches on the above issue:

The outcome was that we could add another key on @PropDefault called resId possibly and there the user would declare the resourceId that we would resolve.

That means that the user gets to declare the same type of variable as the @Prop annotated and we would not need to create other variables as mediators.

Example:

@PropDefault(resType = DRAWABLE, resId = R.mipmap.ic_launcher_round)
Drawable launcherImage;

emilsjolander · 2017-05-09T12:49:00Z

Looks pretty good! thanks 👍 Could you please add tests for the added features?

pavlospt · 2017-05-09T15:07:24Z

@emilsjolander Sure thing, is there a specific directory for the Annotation Processor tests ?

passy · 2017-05-09T15:08:28Z

Oh shoot, we're still working on getting the annotation processor tests open-sourced. There were some issues with Gradle and @IanChilds is working on fixing those.

pavlospt · 2017-05-09T15:09:21Z

@passy Hmmm, is there any way around this then ?

IanChilds · 2017-05-10T09:23:39Z

We should have them available today or tomorrow :)

pavlospt · 2017-05-10T09:26:45Z

@IanChilds Great!

nscoding · 2017-05-30T18:22:43Z

Any updates here? would be great to see this merged.

pavlospt · 2017-05-30T18:36:51Z

@nscoding Did not find some free time to write the tests yet..Hopefully will be able to do so, in the following week!

pavlospt · 2017-06-01T10:26:44Z

@IanChilds can you please review the last commit? I had to use 12345 as a constant for the BuildGeneratorTest additions due to limitation annotation fields that need to be a constant expression. Inside a ComponentSpec it works like a charm because the R field reference is resolved into a constant ID in advance.

passy

Awesome! Just a nit about the .gitignore. I'll leave the annotation processor changes to @IanChilds to review. :)

passy · 2017-06-01T12:08:19Z

.gitignore

@@ -18,4 +18,9 @@ docs/.bundle/
 # Android Studio/IntelliJ
 .idea
 *.iml
+.gradle/
+lib/fb/


We need those. :)
Can you revert that?

passy · 2017-06-01T12:08:33Z

.gitignore

+lib/fb/
+lib/yoga/
+lib/yogajni/
+*/**.iml


This should already be covered by the one above.

IanChilds

This is really awesome! Thanks for doing this!

IanChilds · 2017-06-01T16:37:52Z

litho-processor/src/main/java/com/facebook/litho/specmodels/generator/BuilderGenerator.java

+      for (PropDefaultModel propDefault : propDefaults) {
+        if (!propDefault.isResResolvable()) continue;
+
+        initResTypePropDefaultsSpec.addStatement("this.$L.$L = $L",


one param per line please :)

IanChilds · 2017-06-01T16:39:51Z

litho-processor/src/main/java/com/facebook/litho/specmodels/generator/BuilderGenerator.java

    propsBuilderClassBuilder.addMethod(initMethodSpec.build());

+    if (initResTypePropDefaultsSpec != null) {


why don't you just do everything at this point and then you don't have to do the null checks?

@IanChilds can you explain this a little bit more? :)

i think what he's saying is if you move this up to the bottom of the if on line 221, you can drop the null check.

IanChilds · 2017-06-01T16:42:27Z

litho-processor/src/main/java/com/facebook/litho/specmodels/model/PropDefaultModel.java

@@ -53,6 +96,8 @@ public int hashCode() {
    int result = mType.hashCode();
    result = 17 * result + mName.hashCode();
    result = 31 * result + mModifiers.hashCode();
+    result = 31 * result + mResType.hashCode();


use a different number here and on the line below

IanChilds · 2017-06-01T16:44:12Z

litho-processor/src/main/java/com/facebook/litho/specmodels/model/PropModel.java

@@ -97,6 +97,17 @@ public boolean hasDefault(ImmutableList<PropDefaultModel> propDefaults) {
    return false;
  }

+  public PropDefaultModel getDefault(ImmutableList<PropDefaultModel> propDefaults) {


what is this for?

@IanChilds unused, I removed it. Thanks!

lucasr · 2017-06-01T17:40:13Z

I'd update the docs for PropDefaults (https://fblitho.com/docs/props#prop-defaults) to cover this new feature :-)

pavlospt · 2017-06-01T18:40:40Z

@lucasr I can do this as well and include it in this PR :)

ahmedre · 2017-06-01T19:41:41Z

litho-processor/src/main/java/com/facebook/litho/specmodels/generator/BuilderGenerator.java

    propsBuilderClassBuilder.addMethod(initMethodSpec.build());

+    if (initResTypePropDefaultsSpec != null) {


i think what he's saying is if you move this up to the bottom of the if on line 221, you can drop the null check.

ahmedre · 2017-06-01T19:41:51Z

litho-processor/src/main/java/com/facebook/litho/specmodels/generator/BuilderGenerator.java

@@ -199,8 +216,26 @@ static TypeSpecDataHolder generateBuilder(SpecModel specModel) {
      initMethodSpec.addStatement("mRequired.clear()");
    }

+    MethodSpec.Builder initResTypePropDefaultsSpec = null;


move into the if statement below

ahmedre · 2017-06-01T19:43:51Z

sample/src/main/java/com/facebook/samples/litho/playground/PlaygroundComponentSpec.java

-import com.facebook.yoga.YogaAlign;
-
-import com.facebook.yoga.YogaFlexDirection;
+import com.facebook.litho.widget.Text;


nitpick: can you undo this change (it's nicer when we keep them alphabetized).

ahmedre · 2017-06-01T19:46:10Z

litho-processor/src/main/java/com/facebook/litho/specmodels/model/PropDefaultModel.java

@@ -53,6 +96,8 @@ public int hashCode() {
    int result = mType.hashCode();
    result = 17 * result + mName.hashCode();
    result = 31 * result + mModifiers.hashCode();
+    result = 43 * result + mResType.hashCode();
+    result = 65 * result + mResId;


nitpick - for consistency, can you replace 65 with 47 or 53 or 59 or 61 to keep it as prime numbers?

pavlospt · 2017-06-02T07:16:52Z

@ahmedre Updated to reflect review requests. Can you check again :) ?

ahmedre · 2017-06-02T08:44:08Z

@pavlospt may you also update props.md with these changes? btw, thanks for doing this!

lgtm otherwise, but will leave the remaining review checks to the Litho core team

pavlospt · 2017-06-02T08:55:12Z

@ahmedre Done :)

facebook-github-bot · 2017-06-02T09:01:26Z

@IanChilds has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot added the CLA Signed label May 4, 2017

emilsjolander suggested changes May 4, 2017

View reviewed changes

pavlospt added 4 commits June 1, 2017 12:22

Support ResType on PropDefault annotation

17ae288

Generate proper initializers

6a817b6

Remove ResType.DRAWABLE_REFERENCE

e85aad7

Add test cases for PropDefault annotation with ResType support

1ced675

passy requested changes Jun 1, 2017

View reviewed changes

Update .gitignore to match upstream origin

8b19aad

IanChilds reviewed Jun 1, 2017

View reviewed changes

pavlospt added 3 commits June 1, 2017 20:30

Remove unused method

1fbc973

Update hashCode() method in PropDefaultModel

a49256b

Re-order method parameters

fd55b3a

ahmedre reviewed Jun 1, 2017

View reviewed changes

Review fixes

bdecfd1

Document resource type definition for @PropDefault

84ffa9b

passy approved these changes Jun 2, 2017

View reviewed changes

facebook-github-bot closed this in bbe6998 Jun 5, 2017

		@@ -8,8 +8,10 @@

		package com.facebook.samples.litho.lithography;

		import android.graphics.drawable.Drawable;

		import com.facebook.litho.Column;

		import com.facebook.litho.annotations.ResType;

		propsBuilderClassBuilder.addMethod(initMethodSpec.build());

		if (initResTypePropDefaultsSpec != null) {

Support ResType on PropDefault annotation #174

Support ResType on PropDefault annotation #174

Conversation

pavlospt commented May 4, 2017

emilsjolander commented May 4, 2017

pavlospt commented May 4, 2017

pavlospt commented May 4, 2017

emilsjolander commented May 4, 2017

emilsjolander 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

pavlospt commented May 4, 2017

pavlospt commented May 4, 2017

pavlospt commented May 4, 2017 • edited

emilsjolander commented May 9, 2017

pavlospt commented May 9, 2017

passy commented May 9, 2017

pavlospt commented May 9, 2017

IanChilds commented May 10, 2017

pavlospt commented May 10, 2017

nscoding commented May 30, 2017

pavlospt commented May 30, 2017

pavlospt commented Jun 1, 2017 • edited

passy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanChilds 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

lucasr commented Jun 1, 2017

pavlospt commented Jun 1, 2017

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

pavlospt commented Jun 2, 2017

ahmedre commented Jun 2, 2017

pavlospt commented Jun 2, 2017

facebook-github-bot commented Jun 2, 2017

pavlospt commented May 4, 2017 •

edited

pavlospt commented Jun 1, 2017 •

edited