Adding InjectResource to the annotation processor #140

Closed
wants to merge 12 commits into
from

Projects

None yet

4 participants

@VigneshPeriasami

Added Resource Binding module that supports the following data types String, Drawable & Animation.

@SimonVT
Contributor
SimonVT commented May 18, 2014

Formatting is messed up for pretty much every single change.

What's the point of the two extra inject methods?

@VigneshPeriasami

two extra inject methods? I believe that you are asking from ButterKnife class, just to give an additional option to ignore resource context parameter if no resource injection is required.

@SimonVT
Contributor
SimonVT commented May 18, 2014

Yes, the inject methods in the ButterKnife class. The context parameter is pointless, both View and Dialog has getters for this.

Also, I don't think you should keep the Context in a field either. Add a method to the enums, like findOptionalView.

@VigneshPeriasami

Oh great I missed it, will clean up the code with those changes.

@VigneshPeriasami

context parameter is removed from ButterKnife class and enum abstract method is added to get the context from activity / view / Dialog. Code is formatted as per checkstyle.

@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
...n/java/butterknife/internal/ButterKnifeProcessor.java
@@ -54,7 +57,11 @@
public final class ButterKnifeProcessor extends AbstractProcessor {
public static final String SUFFIX = "$$ViewInjector";
- static final String VIEW_TYPE = "android.view.View";
+ public static final String VIEW_TYPE = "android.view.View";
+ public static final String STRING_TYPE = "java.lang.String";
+ public static final String DRAWABLE_TYPE = "android.graphics.drawable.Drawable";
+
@JakeWharton
JakeWharton May 19, 2014 Owner

kill empty line

@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
...n/java/butterknife/internal/ButterKnifeProcessor.java
@@ -141,6 +149,20 @@
}
}
+ // Process each @InjectResource element
@JakeWharton
JakeWharton May 19, 2014 Owner

Needs period at the end. Comments are sentences.

@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
.../src/main/java/butterknife/internal/ViewInjector.java
}
for (ViewInjection injection : viewIdMap.values()) {
for (ViewBinding viewBinding : injection.getViewBindings()) {
- builder.append(" target.").append(viewBinding.getName()).append(" = null;\n");
+ builder.append(" target.").append(viewBinding.getName())
+ .append(" = null;\n");
+ }
+ }
+ for (ResourceInjection injection : resourceIdMap.values()) {
@JakeWharton
JakeWharton May 19, 2014 Owner

This can be dropped. You don't need to clean up non-View resources.

@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
butterknife/src/main/java/butterknife/ButterKnife.java
+ }
+
+ public Object findOptionalResource(Object source, int id, String type) {
+ Context context = getContext(source);
+ Object resource = null;
+ try {
+ if (type.equals(ButterKnifeProcessor.STRING_TYPE)) {
+ resource = context.getResources().getString(id);
+ } else if (type.equals(ButterKnifeProcessor.DRAWABLE_TYPE)) {
+ resource = context.getResources().getDrawable(id);
+ } else if (type.equals(ButterKnifeProcessor.ANIMATION_TYPE)) {
+ resource = context.getResources().getAnimation(id);
+ }
+ } catch (NotFoundException e) {
+ // Ignore resource not found exception for findOptionalResource
+ resource = null;
@JakeWharton
JakeWharton May 19, 2014 Owner

This is redundant and can be deleted.

@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
butterknife/src/main/java/butterknife/ButterKnife.java
return ((Activity) source).findViewById(id);
}
+
+ @Override
+ public Context getContext(Object source) {
+ return ((Activity) source).getApplicationContext();
@JakeWharton
JakeWharton May 19, 2014 Owner

Calling getApplicationContext() has implications on the theme. Just return the activity instance.

@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
butterknife/src/main/java/butterknife/ButterKnife.java
+ public Object findRequiredResource(Object source, int id, String type,
+ String who) {
+ Object resource = findOptionalResource(source, id, type);
+ if (resource == null) {
+ throw new IllegalStateException(
+ "Required resource with id '"
+ + id
+ + "' for "
+ + who
+ + " was not found. If this resource is optional add"
+ + " '@Optional' annotation.");
+ }
+ return resource;
+ }
+
+ public Object findOptionalResource(Object source, int id, String type) {
@JakeWharton
JakeWharton May 19, 2014 Owner

Do optional resources even make sense?

@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
...c/main/java/butterknife/internal/ResourceBinding.java
@@ -0,0 +1,31 @@
+package butterknife.internal;
+
+final class ResourceBinding implements Binding {
+
@JakeWharton
JakeWharton May 19, 2014 Owner

kill empty line

@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
...main/java/butterknife/internal/ResourceInjection.java
@@ -0,0 +1,40 @@
+package butterknife.internal;
+
+import java.util.ArrayList;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Set;
+
+final class ResourceInjection {
+
@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
...main/java/butterknife/internal/ResourceInjection.java
+
+ public int getId() {
+ return id;
+ }
+
+ public Set<ResourceBinding> getResourceBindings() {
+ return resourceBindings;
+ }
+
+ public void addResourceBinding(ResourceBinding resourceBinding) {
+ resourceBindings.add(resourceBinding);
+ }
+
+ public List<Binding> getRequiredBindings() {
+ List<Binding> requiredBindings = new ArrayList<Binding>();
+
@JakeWharton
JakeWharton May 19, 2014 Owner

kill empty line

@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
.../src/main/java/butterknife/internal/ViewInjector.java
@@ -11,9 +11,12 @@
import static butterknife.internal.ButterKnifeProcessor.VIEW_TYPE;
final class ViewInjector {
+
@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
.../src/main/java/butterknife/internal/ViewInjector.java
@@ -152,6 +173,28 @@ private void emitViewInjection(StringBuilder builder, ViewInjection injection) {
emitListenerBindings(builder, injection);
}
+ private void emitResourceInjection(StringBuilder builder,
+ ResourceInjection injection) {
+
@JakeWharton JakeWharton commented on an outdated diff May 19, 2014
butterknife/src/main/java/butterknife/ButterKnife.java
+ throw new IllegalStateException(
+ "Required resource with id '"
+ + id
+ + "' for "
+ + who
+ + " was not found. If this resource is optional add"
+ + " '@Optional' annotation.");
+ }
+ return resource;
+ }
+
+ public Object findOptionalResource(Object source, int id, String type) {
+ Context context = getContext(source);
+ Object resource = null;
+ try {
+ if (type.equals(ButterKnifeProcessor.STRING_TYPE)) {
@JakeWharton
JakeWharton May 19, 2014 Owner

I don't know if I'm a huge fan of this approach in general. I think it would be much more useful to provide @InjectColor, @InjectDrawable, etc. instead.

VigneshPeriasami added some commits May 19, 2014
@VigneshPeriasami VigneshPeriasami getContext for Activity to return activity instance
kill empty line done
dropped clean up on non-View resources
23db8d9
@VigneshPeriasami VigneshPeriasami Removed findOptionalResource from Finder
InjectResource cannot have @Optional annotation check.
c4f02d9
@VigneshPeriasami

Removed findOptionalResource as it was not making any sense, And also writing separate annotations for each resource type sounds good to me too, will see what I can do.

@VigneshPeriasami

Added separate annotation @InjectColor, @InjectDrawable, @InjectString and removed general approach.
Find one resource per ID is pending.

@jdreesen
jdreesen commented Jun 7, 2014

Why was this closed?

@JakeWharton
Owner

See #244

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment