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.

Sign up for GitHub

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

Allow to annotate private fields based on their getters and setters #144

Merged
merged 7 commits into from
Mar 6, 2017
Merged

Allow to annotate private fields based on their getters and setters #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
72eb177
Allow to annotate private fields based on their getters and setters
geralt-encore Mar 5, 2017
eb3eea6
Merge remote-tracking branch 'upstream/2.0' into kotlin
geralt-encore Mar 6, 2017
6445636
Fix failing tests
geralt-encore Mar 6, 2017
8e3093d
Fix PR comments
geralt-encore Mar 6, 2017
28d1f1a
Fix checkstyle
geralt-encore Mar 6, 2017
7e7c278
Use equals instead of equalsIgnoreCase
geralt-encore Mar 6, 2017
e29e4d0
Remove unused import
geralt-encore Mar 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 64 additions & 1 deletion epoxy-processor/src/main/java/com/airbnb/epoxy/AttributeInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,19 @@
import javax.lang.model.element.AnnotationMirror;
import javax.lang.model.element.Element;
import javax.lang.model.element.ElementKind;
import javax.lang.model.element.ExecutableElement;
import javax.lang.model.element.Modifier;
import javax.lang.model.element.TypeElement;
import javax.lang.model.type.DeclaredType;
import javax.lang.model.util.Types;

import static com.airbnb.epoxy.ProcessorUtils.capitalizeFirstLetter;
import static com.airbnb.epoxy.ProcessorUtils.isViewClickListenerType;
import static javax.lang.model.element.Modifier.FINAL;
import static javax.lang.model.element.Modifier.PRIVATE;
import static javax.lang.model.element.Modifier.PROTECTED;
import static javax.lang.model.element.Modifier.PUBLIC;
import static javax.lang.model.element.Modifier.STATIC;

class AttributeInfo {

Expand All @@ -49,6 +52,11 @@ class AttributeInfo {

private final TypeElement classElement;

// for private fields (Kotlin case)
private final boolean isPrivate;
private String getter;
private String setter;

AttributeInfo(Element attribute, Types typeUtils, ErrorLogger errorLogger) {
attributeElement = attribute;
this.typeUtils = typeUtils;
Expand All @@ -69,6 +77,12 @@ class AttributeInfo {

generateSetter = annotation.setter() && !options.contains(Option.NoSetter);
generateGetter = !options.contains(Option.NoGetter);

isPrivate = attribute.getModifiers().contains(PRIVATE);
if (isPrivate) {
findGetterAndSetterForPrivateField(errorLogger);
}

buildAnnotationLists(attribute.getAnnotationMirrors());
}

Expand Down Expand Up @@ -141,6 +155,43 @@ private boolean isFieldPackagePrivate(Element attribute) {
&& !modifiers.contains(PRIVATE);
}

/**
* Checks if the given private field has getter and setter for access to it
*/
private void findGetterAndSetterForPrivateField(ErrorLogger errorLogger) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To simplify, what if we assumed/enforced that the getter/setter have the same name as the field. Our generated getter/setter will already share that name so it seems like it would be a good thing for consistency anyway. Does this make sense in kotlin?

I previously made a ProcessorUtilsgetMethodOnClass method that you may be able to reuse to simplify this, although you may want to add some modifier checking into that method (to check !static, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that we can't enforce it since Kotlin generate automatically getters and setters for properties with this get/set naming pattern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah makes sense. oh well, no big deal

for (Element element : classElement.getEnclosedElements()) {
if (element.getKind() == ElementKind.METHOD) {
ExecutableElement method = (ExecutableElement) element;
String methodName = method.getSimpleName().toString();
// check if it is a valid getter
if ((methodName.equals(String.format("get%s", capitalizeFirstLetter(name)))
|| methodName.equals(String.format("is%s", capitalizeFirstLetter(name))))
&& !method.getModifiers().contains(PRIVATE)
&& !method.getModifiers().contains(STATIC)
&& method.getParameters().isEmpty()
&& TypeName.get(method.getReturnType()).equals(type)) {
getter = methodName;
}
// check if it is a valid setter
if ((methodName.equals(String.format("set%s", capitalizeFirstLetter(name))))
&& !method.getModifiers().contains(PRIVATE)
&& !method.getModifiers().contains(STATIC)
&& method.getParameters().size() == 1
&& TypeName.get(method.getParameters().get(0).asType()).equals(type)) {
setter = methodName;
}
}
}
if (getter == null || setter == null) {
errorLogger
.logError("%s annotations must not be on private fields"
+ " without proper getter and setter methods. (class: %s, field: %s)",
EpoxyAttribute.class,
classElement.getSimpleName(),
name);
}
}

/**
* Keeps track of annotations on the attribute so that they can be used in the generated setter
* and getter method. Setter and getter annotations are stored separately since the annotation may
Expand Down Expand Up @@ -204,7 +255,7 @@ List<AnnotationSpec> getSetterAnnotations() {
return setterAnnotations;
}

public boolean generateGetter() {
boolean generateGetter() {
return generateGetter;
}

Expand All @@ -224,6 +275,18 @@ boolean isPackagePrivate() {
return packagePrivate;
}

boolean isPrivate() {
return isPrivate;
}

String getter() {
return getter;
}

String setter() {
return setter;
}

@Override
public String toString() {
return "ModelAttributeData{"
Expand Down
133 changes: 92 additions & 41 deletions epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
import static com.squareup.javapoet.TypeName.INT;
import static com.squareup.javapoet.TypeName.LONG;
import static com.squareup.javapoet.TypeName.SHORT;
import static java.awt.SystemColor.info;
import static javax.lang.model.element.Modifier.FINAL;
import static javax.lang.model.element.Modifier.PRIVATE;
import static javax.lang.model.element.Modifier.PUBLIC;
Expand Down Expand Up @@ -215,29 +214,35 @@ private Iterable<MethodSpec> generateBindMethods(ClassToGenerateInfo classInfo)
// view click listener of the original model.

String modelClickListenerField = attribute.getModelClickListenerName();
preBindBuilder
.beginControlFlow("if ($L != null)", modelClickListenerField)
.addCode(CodeBlock.of(
"super.$L = new $T() {\n"
+ " // Save the original click listener so if it gets changed on\n"
+ " // the generated model this click listener won't be affected\n"
+ " // if it is still bound to a view.\n"
+ " private final $T $L = $T.this.$L;\n"
+ " public void onClick($T v) {\n"
+ " $L.onClick($T.this, object, v,\n"
+ " holder.getAdapterPosition());\n"
+ " }\n"
+ " public int hashCode() {\n"
+ " // Use the hash of the original click listener so we don't change the\n"
+ " // value by wrapping it with this anonymous click listener\n"
+ " return $L.hashCode();\n"
+ " }\n"
+ " };\n", attribute.getName(), viewClickListenerType,
getModelClickListenerType(classInfo),
modelClickListenerField, classInfo.getGeneratedName(),
modelClickListenerField, viewType, modelClickListenerField,
classInfo.getGeneratedName(), modelClickListenerField))
.endControlFlow();
preBindBuilder.beginControlFlow("if ($L != null)", modelClickListenerField);
CodeBlock clickListenerCodeBlock = CodeBlock.of(
"{\n"
+ " // Save the original click listener so if it gets changed on\n"
+ " // the generated model this click listener won't be affected\n"
+ " // if it is still bound to a view.\n"
+ " private final $T $L = $T.this.$L;\n"
+ " public void onClick($T v) {\n"
+ " $L.onClick($T.this, object, v,\n"
+ " holder.getAdapterPosition());\n"
+ " }\n"
+ " public int hashCode() {\n"
+ " // Use the hash of the original click listener so we don't change the\n"
+ " // value by wrapping it with this anonymous click listener\n"
+ " return $L.hashCode();\n"
+ " }\n"
+ " }",
getModelClickListenerType(classInfo),
modelClickListenerField, classInfo.getGeneratedName(),
modelClickListenerField, viewType, modelClickListenerField,
classInfo.getGeneratedName(), modelClickListenerField);
if (attribute.isPrivate()) {
preBindBuilder.addCode("super.$L(new $T() $L);\n", attribute.setter(),
viewClickListenerType, clickListenerCodeBlock);
} else {
preBindBuilder.addCode("super.$L = new $T() $L;\n", attribute.getName(),
viewClickListenerType, clickListenerCodeBlock);
}
preBindBuilder.endControlFlow();
}
methods.add(preBindBuilder.build());

Expand Down Expand Up @@ -527,9 +532,15 @@ private MethodSpec generateSetClickModelListener(ClassToGenerateInfo helperClass
.addModifiers(PUBLIC)
.returns(helperClass.getParameterizedGeneratedName())
.addParameter(param)
.addAnnotations(attribute.getSetterAnnotations())
.addStatement("super.$L = null", attributeName)
.addStatement("this.$L = $L", attribute.getModelClickListenerName(), attributeName);
.addAnnotations(attribute.getSetterAnnotations());

if (attribute.isPrivate()) {
builder.addStatement("super.$L(null)", attribute.setter());
} else {
builder.addStatement("super.$L = null", attributeName);
}

builder.addStatement("this.$L = $L", attribute.getModelClickListenerName(), attributeName);

return builder
.addStatement("return this")
Expand Down Expand Up @@ -575,7 +586,13 @@ private MethodSpec generateEquals(ClassToGenerateInfo helperClass) {
continue;
}

addEqualsLineForType(builder, attributeInfo.useInHash(), type, attributeInfo.getName());
if (attributeInfo.isPrivate()) {
addEqualsLineForType(builder, attributeInfo.useInHash(), type,
String.format("%s()", attributeInfo.getter()));
} else {
addEqualsLineForType(builder, attributeInfo.useInHash(), type, attributeInfo.getName());
}

if (attributeInfo.isViewClickListener()) {
// Add the model click listener as well
addEqualsLineForType(builder, attributeInfo.useInHash(), type,
Expand Down Expand Up @@ -661,7 +678,12 @@ private MethodSpec generateHashCode(ClassToGenerateInfo helperClass) {
continue;
}

addHashCodeLineForType(builder, attributeInfo.useInHash(), type, attributeInfo.getName());
if (attributeInfo.isPrivate()) {
addHashCodeLineForType(builder, attributeInfo.useInHash(), type,
String.format("%s()", attributeInfo.getter()));
} else {
addHashCodeLineForType(builder, attributeInfo.useInHash(), type, attributeInfo.getName());
}

if (attributeInfo.isViewClickListener()) {
// Add the model click listener as well
Expand Down Expand Up @@ -714,10 +736,18 @@ private MethodSpec generateToString(ClassToGenerateInfo helperClass) {
for (AttributeInfo attributeInfo : helperClass.getAttributeInfo()) {
String attributeName = attributeInfo.getName();
if (first) {
sb.append(String.format("\"%s=\" + %s +\n", attributeName, attributeName));
if (attributeInfo.isPrivate()) {
sb.append(String.format("\"%s=\" + %s() +\n", attributeName, attributeInfo.getter()));
} else {
sb.append(String.format("\"%s=\" + %s +\n", attributeName, attributeName));
}
first = false;
} else {
sb.append(String.format("\", %s=\" + %s +\n", attributeName, attributeName));
if (attributeInfo.isPrivate()) {
sb.append(String.format("\", %s=\" + %s() +\n", attributeName, attributeInfo.getter()));
} else {
sb.append(String.format("\", %s=\" + %s +\n", attributeName, attributeName));
}
}
}

Expand All @@ -729,12 +759,18 @@ private MethodSpec generateToString(ClassToGenerateInfo helperClass) {
}

private MethodSpec generateGetter(AttributeInfo data) {
return MethodSpec.methodBuilder(data.getName())
MethodSpec.Builder builder = MethodSpec.methodBuilder(data.getName())
.addModifiers(PUBLIC)
.returns(data.getType())
.addAnnotations(data.getGetterAnnotations())
.addStatement("return $L", data.getName())
.build();
.addAnnotations(data.getGetterAnnotations());

if (data.isPrivate()) {
builder.addStatement("return $L()", data.getter());
} else {
builder.addStatement("return $L", data.getName());
}

return builder.build();
}

private MethodSpec generateSetter(ClassToGenerateInfo helperClass, AttributeInfo attribute) {
Expand All @@ -743,8 +779,13 @@ private MethodSpec generateSetter(ClassToGenerateInfo helperClass, AttributeInfo
.addModifiers(PUBLIC)
.returns(helperClass.getParameterizedGeneratedName())
.addParameter(ParameterSpec.builder(attribute.getType(), attributeName)
.addAnnotations(attribute.getSetterAnnotations()).build())
.addStatement("this.$L = $L", attributeName, attributeName);
.addAnnotations(attribute.getSetterAnnotations()).build());

if (attribute.isPrivate()) {
builder.addStatement("this.$L($L)", attribute.setter(), attributeName);
} else {
builder.addStatement("this.$L = $L", attributeName, attributeName);
}

if (attribute.isViewClickListener()) {
// Null out the model click listener since this view click listener should replace it
Expand All @@ -770,8 +811,13 @@ private MethodSpec generateReset(ClassToGenerateInfo helperClass) {

for (AttributeInfo attributeInfo : helperClass.getAttributeInfo()) {
if (!attributeInfo.hasFinalModifier()) {
builder.addStatement("this.$L = $L", attributeInfo.getName(),
getDefaultValue(attributeInfo.getType()));
if (attributeInfo.isPrivate()) {
builder.addStatement("this.$L($L)", attributeInfo.setter(),
getDefaultValue(attributeInfo.getType()));
} else {
builder.addStatement("this.$L = $L", attributeInfo.getName(),
getDefaultValue(attributeInfo.getType()));
}
}

if (attributeInfo.isViewClickListener()) {
Expand All @@ -788,9 +834,14 @@ private MethodSpec generateReset(ClassToGenerateInfo helperClass) {
private static String getDefaultValue(TypeName attributeType) {
if (attributeType == BOOLEAN) {
return "false";
} else if (attributeType == BYTE || attributeType == CHAR || attributeType == SHORT
|| attributeType == INT) {
} else if (attributeType == INT) {
return "0";
} else if (attributeType == BYTE) {
return "(byte) 0";
} else if (attributeType == CHAR) {
return "(char) 0";
} else if (attributeType == SHORT) {
return "(short) 0";
} else if (attributeType == LONG) {
return "0L";
} else if (attributeType == FLOAT) {
Expand Down
2 changes: 1 addition & 1 deletion epoxy-processor/src/main/java/com/airbnb/epoxy/ModelProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ private void addAttributeToGeneratedClass(Element attribute,
}

private AttributeInfo buildAttributeInfo(Element attribute) {
validateFieldAccessibleViaGeneratedCode(attribute, EpoxyAttribute.class, errorLogger);
validateFieldAccessibleViaGeneratedCode(attribute, EpoxyAttribute.class, errorLogger, true);
return new AttributeInfo(attribute, typeUtils, errorLogger);
}

Expand Down
16 changes: 14 additions & 2 deletions epoxy-processor/src/main/java/com/airbnb/epoxy/ProcessorUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,12 +210,12 @@ static TypeMirror getEpoxyObjectType(TypeElement clazz, Types typeUtils) {
}

static void validateFieldAccessibleViaGeneratedCode(Element fieldElement,
Class<?> annotationClass, ErrorLogger errorLogger) {
Class<?> annotationClass, ErrorLogger errorLogger, boolean skipPrivateFieldCheck) {
TypeElement enclosingElement = (TypeElement) fieldElement.getEnclosingElement();

// Verify method modifiers.
Set<Modifier> modifiers = fieldElement.getModifiers();
if (modifiers.contains(PRIVATE) || modifiers.contains(STATIC)) {
if ((modifiers.contains(PRIVATE) && !skipPrivateFieldCheck) || modifiers.contains(STATIC)) {
errorLogger.logError(
"%s annotations must not be on private or static fields. (class: %s, field: %s)",
annotationClass.getSimpleName(),
Expand Down Expand Up @@ -248,4 +248,16 @@ static void validateFieldAccessibleViaGeneratedCode(Element fieldElement,
enclosingElement.getSimpleName(), fieldElement.getSimpleName());
}
}

static void validateFieldAccessibleViaGeneratedCode(Element fieldElement,
Class<?> annotationClass, ErrorLogger errorLogger) {
validateFieldAccessibleViaGeneratedCode(fieldElement, annotationClass, errorLogger, false);
}

static String capitalizeFirstLetter(String original) {
if (original == null || original.isEmpty()) {
return original;
}
return original.substring(0, 1).toUpperCase() + original.substring(1);
}
}
Loading