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.

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

Support Android Library Projects #613

Closed
wants to merge 1 commit into from

Conversation

@kageiit
Copy link
Contributor

@kageiit kageiit commented Jun 11, 2016

Summary

There have been many solutions proposed to support using Butterknife in Android Libraries i.e non-final IDs in R. This approach uses a combination of Gradle Plugin + Annotation Processing + Custom Lint to ensure this is done as safely as possible with minimal configuration.

Usage

  • Apply gradle plugin to the library project.
apply plugin: 'butterknife'

The plugin generates an R2 class which is essentially a copy of R with final values and the correct support annotations.

  • Specify the R class for the library using the @RClass annotation, just once, anywhere in code. Use R2 instead of R in all butterknife annotations.
package butterknife.library;

import android.view.View;
import butterknife.BindView;
import butterknife.OnClick;
import butterknife.RClass;

@RClass(R.class)
class Foo {

  @BindView(R2.id.footer) View bar;

  @OnClick(R2.id.hello) void sayHello() { }
}

The @Rclass annotation is used by the annotation processor to map the integer ids to symbols in R. This in turn generates code like

target.bar = finder.findRequiredView(source, com.example.butterknife.library.R.id.footer, "field 'bar'");
view = finder.findRequiredView(source, com.example.butterknife.library.R.id.hello, "method 'sayHello'");

in the ViewBinder. This ensures that the final values used from R values will be generated by the app consuming the library.

  • If R2 is being used outside butterknife annotations, a custom lint rule will throw an error
    screen shot 2016-06-10 at 9 54 12 pm
    This is important because values in R2 are final and would be inlined as constants when used outside annotations.
  • Profit!

Pros

  • Api invisible to consumers of the library. R2 is not packaged into the aar just like R.
  • Full compile time safety. No referring to R values using strings.
  • Migrating existing library projects to take advantage of this approach is very simple.
  • No changes to the butterknife annotation APIs.
  • Backward compatible with existing app projects that use butterknife. In fact, just adding @RClass will output human friendly generated code in existing app projects.
  • Forward compatible with Butterknife extensions (#573) as the approach makes no assumptions about listener classes.
  • Other build systems like buck and bazel can generate their equivalent of R2 very easily by implementing the logic on their own or by just reusing FinalRClassBuilder (written entirely in java) from the gradle plugin module.

Cons

  • When performing rename refactors of R values in the Android Studio/IntelliJ, they are kept in sync with the layout files etc. automatically by the IDE, but R2 is only generated after the gradle task that generates R. To keep R and R2 in sync, the rename would need to be done twice once for R and immediately in R2. Without doing so, compilation would fail. This problem can be fixed easily by writing a custom intellij plugin that links the values in R2 and R, similar to how the values in R are linked to the layout xml files etc. This can be a future enhancement.
@ZacSweers
Copy link
Contributor

@ZacSweers ZacSweers commented Jun 11, 2016

screenshot 2016-06-10 22 00 26

@ZacSweers
Copy link
Contributor

@ZacSweers ZacSweers commented Jun 11, 2016

Wish the processor could have extracted the fully qualified names by itself so the plugin wasn't necessary for that, but can't have it all :). Awesome work

@kageiit kageiit force-pushed the kageiit:android_library_support branch 2 times, most recently from 7536f36 to c23a2b0 Jun 11, 2016
@kageiit kageiit mentioned this pull request Jun 12, 2016
@peacepassion
Copy link

@peacepassion peacepassion commented Jun 12, 2016

This solution is similar as Butterfork.

Users have to replace R by R2/B.

I found a better solution ButterCookie. Using ButterCookie, you don't need to change any existing code with Butterknife in Library project.

@kageiit
Copy link
Contributor Author

@kageiit kageiit commented Jun 12, 2016

I found a better solution ButterCookie. Using ButterCookie, you don't need to change any existing code with Butterknife in Library project.

I have looked at ButterCookie, but here are some issues I noticed with it

  • Relies very heavily on what annotations are supported by Butterknife in its plugin class and the format of the generated code. This will break with butterknife extensions and new binding/listener annotations added to butterknife itself.
  • Manipulates the variant.javaCompiler dsl which will not work with jack.
  • Does reflection on the internal private APIs of the android gradle plugin and is very easily prone to breakage with android plugin updates.
  • Relies on the internal implementation details of the apt plugin

It would need to keep up with every possible combination of changes in butterknife/the android gradle plugin/apt plugin and would become very hard to maintain.

@kageiit
Copy link
Contributor Author

@kageiit kageiit commented Jun 12, 2016

This solution is similar as Butterfork.

Users have to replace R by R2/B.

This is slightly different than Butterfork because

  • It generates an R2 class with integers instead of strings. This makes the IDE refactor friendlier than Butterfork, as there is no need to do a full recompile when renaming R values.
  • Butterfork lacks the type safety ensured by using support annotations like @IdRes, @StringRes etc (which are part of butterknife) to make sure the integer you are passing to the annotation is of the correct type. The R2 class comes with the correct annotations on all the generated fields, so this issue does not exist.
  • It also gives safety in the form of custom lint checks to make sure you do not use R2 in places not intended.
  • Butterfork would need to keep in sync with butterknife always, which can get hard to maintain
@oguzbabaoglu
Copy link

@oguzbabaoglu oguzbabaoglu commented Jun 12, 2016

@kageiit Fully agreed on the points about Butterfork, I will deprecate it as soon as this gets merged. Great work! 👍

@ghost
Copy link

@ghost ghost commented Jun 12, 2016

Could IdScanner be adapted to generate R2 instead of using the Gradle plugin? It would have to be run in a round before the rest of ButterKnifeProcessor.

@ghost
Copy link

@ghost ghost commented Jun 12, 2016

Also, instead of annotating a random type with @RClass, I think it would make more sense to use package-info.java.

e.g.

@RClass(R.class)
package my.android.library;

would setup my.android.library.R2.

@kageiit
Copy link
Contributor Author

@kageiit kageiit commented Jun 12, 2016

@GrahamRogers I spent a lot of time trying to generate R2 using during the annotation processor rounds. The problem lies in the fact that since R2 does not exist before the first round, the processor would need to defer processing on all the butterknife annotated elements to the next round. But then, the type mirror of R2.id.value etc would have changed on the next round, so the int value cannot be extracted easily by querying the value parameters on the butterknife annotations.

It also makes the logic inside the processor needlessly complex as it would have to handle the cases when the RClass annotation exists and not. Would be hard to maintain as butterknife evolves as well.

Using apt for R2 generation means the generated R2 class will end up in the aar as well which is not desirable.

I settled on this approach as a nice balance between ease of use, a clean api and maintainability.

The location of where to put the RClass does not matter, but the import to R does not exist yet if it's on top of a package element. One would then have to use the fully qualified name, but I prefer for it to look terse. Also, the package of R2 is always the same as R, so putting it on top of a package element does not actually use any information from that element. The decision of where to put the RClass annotation can be left to the consumer and we can suggest an appropriate location as best practice.

@ghost
Copy link

@ghost ghost commented Jun 13, 2016

For the Bazel case, since there is one Android library per package annotating the package would make sense since the annotated package would be the same as the package used for R and R2. It's only a minor detail, but adding PACKAGE as a Target for RClass would at least give people the option and wouldn't change the implementation.

Also, you can do this:

@RClass(R.class)
package my.package;

import butterknife.RClass;
import my.other.package.R;

Although I admit it would be weird for that to produce my.other.package.R2 instead of my.package.R2.

You raise good points about using code generation for R2, although I would question whether they outweigh having to maintain a plugin for each supported build system. I don't think there's a right answer, so plugins are fine for me (as long as a Bazel one is made 😛).

@kageiit
Copy link
Contributor Author

@kageiit kageiit commented Jun 13, 2016

Not a big fan about adding a package target for RClass. The package for R2 is decided by the plugin of the build system so you could just put that detail in bazel to allow it. The same goes for buck.

The build systems are responsible for generating R. They should be able to generate R2 as well. I don't think a plugin will be provided out of the box for buck/bazel in butterknife. I'm planning to add first party support for R2 in buck after this merges. You may need to propose the same in bazel for your use case.


apply plugin: 'com.android.library'
apply plugin: 'com.neenbedankt.android-apt'
apply plugin: 'butterknife'

This comment has been minimized.

@JakeWharton

JakeWharton Jun 14, 2016
Owner

It's unfortunate that you need both a plugin and the @RClass annotation. Couldn't this plugin modify the annotation processor options or register a task to generate a file with the right information?

This comment has been minimized.

@kageiit

kageiit Jun 14, 2016
Author Contributor

RClass and plugin are two separate things. RClass is used as a hint to the processor to find the symbol mapping to generate nicer generated code. It works on apps without using the plugin.

The plugin is just used to create a R with all final values.

This comment has been minimized.

@JakeWharton

JakeWharton Jun 14, 2016
Owner

Got it. You might be interested in this issue I filed a while back for exposing things to processors automatically.

This comment has been minimized.

@oguzbabaoglu

oguzbabaoglu Jun 15, 2016

Libraries depend on the plugin so no issue there. For apps that don't depend on it; "if you want nicer generated code, add the @RClass annotation" and "if you want nicer generated code, add the plugin" sounds like equivalent effort for the consumer.

Moving the responsibility to the plugin does mean it needs to be replicated for each build system (Buck, Bazel etc.). The variant.javaCompile.options method might not be as straightforward, but "generate a file with information" should be simple enough to do for each?

Native support by android-tools would definitely be awesome. Parsing the manifest and deriving information always feels a bit hacky.

@kageiit
Copy link
Contributor Author

@kageiit kageiit commented Jun 15, 2016

I think the effort of having to use RClass in conjunction with the plugin is minimal. In buck, we just need to add an option for generating R2 and the rest will work the same. Modifying the annotation options as part of buck itself just to support usage of butterknife is not ideal.

The same goes for bazel. This way, there is no need to wait on android-tools for this support, since that will be purely be scoped to only gradle users. Once that support is available, we can make the usage of R2 optional as well for gradle users by enhancing the plugin.

@kageiit
Copy link
Contributor Author

@kageiit kageiit commented Jun 15, 2016

We should also not rely on variant.javacompile dsl as that is not compatible with Jack. The android apt plugin currently does not work when Jack is enabled due to the same reason.

@kageiit
Copy link
Contributor Author

@kageiit kageiit commented Jun 15, 2016

Also, if we go down the approach of "generating a file with information", anytime that file format changes, all build systems would need to fix their logic based on the version of butterknife they depend on which is not desirable. This decouples the logic and is single responsibility for the annotation and the plugin.

@kageiit kageiit force-pushed the kageiit:android_library_support branch 6 times, most recently from 62e51c2 to 544f578 Jun 17, 2016
private Elements elementUtils;
private Types typeUtils;
private Filer filer;
private Trees trees;

private final Map<Integer, Id> symbols = new HashMap<>();

This comment has been minimized.

@JakeWharton

JakeWharton Jun 19, 2016
Owner

nit: LinkedHashMap

}
}

private static class VarScanner extends TreeScanner {

This comment has been minimized.

@JakeWharton

JakeWharton Jun 19, 2016
Owner

Any idea how all this stuff fares in Jack?

This comment has been minimized.

@kageiit

kageiit Jun 19, 2016
Author Contributor

Tested all this with jack enabled already :) It works as expected.

+ " }\n"
+ " @SuppressWarnings(\"ResourceType\")\n"
+ " protected static void bindToTarget(Test target, Resources res) {\n"
+ " target.one = res.getInteger(test.R.integer.res);\n"

This comment has been minimized.

@JakeWharton

JakeWharton Jun 19, 2016
Owner

Looks like we can clean this up so that it's not always going to use a FQCN. I can do that in a follow-up.

This comment has been minimized.

@kageiit

kageiit Jun 19, 2016
Author Contributor

Ya for sure. i didnt get time to clean that part up.

@@ -0,0 +1,27 @@
apply plugin: 'groovy'

This comment has been minimized.

@JakeWharton

JakeWharton Jun 19, 2016
Owner

Going to convert this to Kotlin post-merge. Groovy is toxic.

This comment has been minimized.

@kageiit

kageiit Jun 19, 2016
Author Contributor

sure :) The plugin itself is rather simple so it should not be too hard

@@ -1,4 +1,5 @@
apply plugin: 'com.android.library'
apply plugin: 'com.kageiit.lintrules'

This comment has been minimized.

@JakeWharton

JakeWharton Jun 19, 2016
Owner

The implementation of this plugin is a little scary!

This comment has been minimized.

@JakeWharton

JakeWharton Jun 19, 2016
Owner

(The modifying the user's home directory part which isn't triggered here)

This comment has been minimized.

@kageiit

kageiit Jun 19, 2016
Author Contributor

Ya that part is for apps, which we are not using here. I want to remove custom lint for app projects from the plugin and cut a new release soon.

@JakeWharton
Copy link
Owner

@JakeWharton JakeWharton commented Jun 19, 2016

First IDE import is problematic:

Error:Could not find com.jakewharton:butterknife-gradle-plugin:8.1.1-SNAPSHOT.
Searched in the following locations:
    file:/Applications/Android Studio 2.2.app/Contents/gradle/m2repository/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/maven-metadata.xml
    file:/Applications/Android Studio 2.2.app/Contents/gradle/m2repository/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/butterknife-gradle-plugin-8.1.1-SNAPSHOT.pom
    file:/Applications/Android Studio 2.2.app/Contents/gradle/m2repository/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/butterknife-gradle-plugin-8.1.1-SNAPSHOT.jar
    https://repo1.maven.org/maven2/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/maven-metadata.xml
    https://repo1.maven.org/maven2/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/butterknife-gradle-plugin-8.1.1-SNAPSHOT.pom
    https://repo1.maven.org/maven2/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/butterknife-gradle-plugin-8.1.1-SNAPSHOT.jar
    https://plugins.gradle.org/m2/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/maven-metadata.xml
    https://plugins.gradle.org/m2/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/butterknife-gradle-plugin-8.1.1-SNAPSHOT.pom
    https://plugins.gradle.org/m2/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/butterknife-gradle-plugin-8.1.1-SNAPSHOT.jar
    file:/Users/jw/.m2/repository/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/maven-metadata.xml
    file:/Users/jw/.m2/repository/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/butterknife-gradle-plugin-8.1.1-SNAPSHOT.pom
    file:/Users/jw/.m2/repository/com/jakewharton/butterknife-gradle-plugin/8.1.1-SNAPSHOT/butterknife-gradle-plugin-8.1.1-SNAPSHOT.jar
Required by:
    com.jakewharton:butterknife-sample-lib:8.1.1-SNAPSHOT

The one thing we do in other projects is keep the samples on the release version and only update them after a new release. I'll probably do that here too once the version with this is released.

@JakeWharton
Copy link
Owner

@JakeWharton JakeWharton commented Jun 19, 2016

merged as 20aad6c

@kageiit
Copy link
Contributor Author

@kageiit kageiit commented Jun 23, 2016

Buck support here: facebook/buck#789

@kageiit kageiit deleted the kageiit:android_library_support branch Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.