Make android.app.Fragment handling way more explicitly

[hotchemi]

Overview

Now we have the problem(#279) that is caused by optional-base plugin and new Gradle. And I also found out that there's an unresolvable case like below with current approach:

• When user want to use both v4.Fragment and normal Fragment in the same app with v13 support library dependency

To avoid that I'd like to propose the following interface change.

• Remove optional-base gradle plugin
• Add v13 argument in @RuntimePermission, if its value is true the library generates a code with FragmentCompat.

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.CLASS)
public @interface RuntimePermissions {
  boolean v13() default false;
}

I'd say that by changing the way of dealing with Fragment explicitly we'll be able to avoid unexpected bugs eternally and make code much more declarative.

What do you think? @aurae @shiraji

[mannodermaus]

I'm unsure why using both Fragment variants together would become an issue. Could you elaborate on that? As far as I remember, the different ProcessorUnit implementations would apply to only their respective variant, so we can't get into a state where they get mixed up.

I agree that we should get rid of the Optional plugin - at this point, there are cleaner ways to achieve optional dependencies. I'm still under the impression that we can make the Class lookup work dynamically, without having to require our users to modify their RuntimePermissions annotations. Ideally, the processor would do that automatically, but alas...

For now, I'm fine with adding the v13 argument, as we do have a significant amount of users that rely on this feature, and we should mitigate the problem in a short-term way. However, I'm taking the liberty to experiment a bit more with the new possibilities of Gradle & the Android plugin (e.g. 2.5's migration to the new dependency scopes api, implementation et cetera), trying to get a deeper understanding of how its classpath might've changed in a recent version.

Edit: Additionally, we could also transition into a Gradle plugin, which would be able to perform some logic depending on what's available on the classpath, then pass that as an argument to our annotation processor. Just an unreflected thought that came to my mind a second ago, but which I found to be worth pointing out.

[shiraji]

I have some questions. Since the plugin sit here before I contribute to this project, I'm not sure the issue here.

• Are you sure optional-base cause the error for permissions.dispatcher.processor.exception.SupportV13MissingException with gradle plugin 2.4.0-dev #279 (comment) ?
• It seems we use older version of the plugin. Can we upgrade this plugin?
• Isn't it possible to fix the plugin? I can contribute it if you want.
• It seems no issue for this problem. Why don't we report the issue?

Personally, I don't like adding v13 argument because the developer requires to think additional step to handling wether the Fragment is v13 or not. And the most developer will tend to forget about this feature. It leads to raise issues 🙅‍♂️

[hotchemi]

Thx guys. I (deliberately) mixed two problems so let's go into one by one.

1. latent problem

so we can't get into a state where they get mixed up.

As @aurae described currently user can't mix support.v4.Fragment and android.app.Fragment with @RuntimePermissions simultaneously because PermissionsDispatcher scans the existence of v13 support library implicitly and switch to use FragmentCompat if it exists.
I'd like to resolve the problem above because there's a possibility to deal with such cases I guess.

2. SupportV13MissingException with newer ver of Gradle

As @shiraji described we might be able to solve the problem by just updating optional-base Gradle plugin and I was aware of it actually.
But, I'm not a huge fun of do something implicitly on the library layer so I'd say it's a good time to switch to rely on a explicit way.
For instance Retrofit1 uses OkHttp if it exists on an app classpath implicitly but they changed the way to define http client with Retrofit.Builder explicitly from Retrofit2. The change allows user to what's going on under the hood and increases the traceability of debug I guess.

Say, I'm aware of that v13 is kind of counter-intuitive method name so it'd be better that the processor would judge Fragment package name and generates corresponding code automatically if possible. But if it's impossible we gotta make user declare the use of v13 FragmentCompat.

[mannodermaus]

Regarding 1: Were you able to confirm that you can't use both Fragment types at the same time? Again, I feel like this shouldn't be an issue. Our ProcessorUnit implementations take the fully-qualified class name into account, and we can distinguish between support and native Fragments on a per-case basis. (See ProcessorUnit#getTargetType and #checkPrerequisites.)

Regarding 2: In a way, we're going to get first-hand support for optional dependencies with Android Gradle Plugin 2.5 and Gradle 3.5 (have any of you tried its preview? It's glorious). We can't force everybody to use a super-early preview version of an unstable plugin just yet, but eventually this is going to replace the need for a third-party Optional plugin. It remains to be seen if #279 is actually that plugin's fault, though. I believe that it's more likely a behavioral change to the way classpaths are set up between older versions of the Android Gradle plugin, and newer ones. Do you guys know if there's an official source for diffs of the plugin available somewhere? I always dig right into the source, however it would be helpful to find the differences in-between versions, once we isolated which version introduced the change.

[hotchemi]

@aurae Ah sorry I missed your saying and found out that I was completely wrong!
In that sense addressing would be pretty simple...how about just bundling support v13 library without optional plugin? Say, v4 library is splitted to each components so we'll be able to include only needed dependencies and there's a space to bundle v13 I guess!

[mannodermaus]

Removing the "optionality" & bundling support-v13 directly inside our library artifact would resolve the issue, yes. In fact, we could even move to a different paradigm related to supporting native Fragments:

• Up until now, users had to include the support-v13 dependency separately, and they couldn't use @RuntimePermissions on native Fragment otherwise. Users that don't use native Fragments didn't have to add another dependency
• The new way could be to always include support-v13, so that native Fragments can be annotated straight away. Users that don't use native Fragments can exclude the transitive dependency using a Gradle exclude clause

I actually like the second approach, but that requires us not to change the way NativeProcessorUnit works. (If a user excludes the support-v13 library, the library should still work. Our Class.forName check still checks for the presence of FragmentCompat, but it's "available by default", as opposed to "absent by default".)

The usage section of our README would turn into something like this (artifacts abbreviated):

Add to your module's build.gradle file:

compile "com.hotchemi:pd:2.4.x"
annotationProcessor "com.hotchemi:pd-processor:2.4.x"

If you don't use native Fragments (android.app.Fragment), you can exclude support for them:
compile ("com.hotchemi:pd:2.4.x") {
    exclude name: "support-v13"
}
annotationProcessor "com.hotchemi:pd-processor:2.4.x"

[hotchemi]

@aurae Cool! Just in case I checked methods count:

Now:

• com.android.support:support-v4:25.2.0 12879
  New:
• com.android.support:support-compat:25.2.0 6564
• com.android.support:support-v13:25.2.0 302

About one half, we should go this way 😃

[mannodermaus]

Getting rid of the monolithic support-v4 dependency is a big thing as well, I agree. We should've done this long ago. 😁

[shiraji]

Looks good. Could we bump version to 3.0.0 for this feature? Users obviously know we change something.

[hotchemi]

@shiraji I was thinking the same thing and checked Semantic Versioning, it says MAJOR version when you make incompatible API changes. Is it worth to bump the major ver in this case? 👀