-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add option to enable isolating incremental annotation processing mode #121
Add option to enable isolating incremental annotation processing mode #121
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well, but I've left some small notes
I think we should make minor release after merge
please report them using https://github.com/moxy-community/Moxy/issues/new. | ||
|
||
In the same time to fix such problems while waiting for fix on our side you have two options: | ||
1. Just disable incremental mode. This will switch Moxy processor back to aggregating mode, so incremental compilation will be ok. This could hurt you compilation time a bit, hopefully not very much. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Just disable incremental isolating mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
In the same time to fix such problems while waiting for fix on our side you have two options: | ||
1. Just disable incremental mode. This will switch Moxy processor back to aggregating mode, so incremental compilation will be ok. This could hurt you compilation time a bit, hopefully not very much. | ||
2. Do clean build. This will force to recompile all generated from scratch. Sure this is also bad for compilation time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing nouns: This will force Gradle to recompile all generated code from scratch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -192,7 +192,13 @@ To switch the error output method enable this option | |||
enableEmptyStrategyHelper : 'true' | |||
``` | |||
|
|||
How to correctly use compilation flags check out the [sample-app build.gradle file](https://github.com/moxy-community/Moxy/blob/develop/sample-app/build.gradle) | |||
To enable `Isolating` incremental annotation processor mode for Moxy processor use this option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add something like "for faster incremental builds" for people, that don't know what isolating mode is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. I also add link to gradle documentation page about incremental annotation processing support.
messager.printMessage( | ||
Diagnostic.Kind.WARNING, | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This blank line looks strange from android studio's build window, as it only prints first line. So maybe we should put exhaustive summary in the first line, something like: "Experimental isolating annotation processor mode was enabled for Moxy"
On the other hand - maybe it should be INFO instead of WARNING? This feature is opt-in, and warning may become annoying after several builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right. If someone enable this option I think he knows what's he doing and in this case this warning indeed may become really annoying.
It seems like if I edit View interfaces, Gradle just removes all files in generated folder and builds them again, but it does not break incremental builds for further tasks. In other words, Gradle will use isolating mode, if you edit presenters or fragments/activities, and fallback to something like aggregating mode, if you edit View interfaces, which is fine |
I have an idea. What if we annotate |
Yeah, that could work (if you talking about |
@bejibx @aasitnikov how does that combine with our troubles with duplicate generated ViewStates for MvpView classes? Currently it's "fixed" by not generating ViewStates for abstract presenters, and if we annotate Views, this fix would no longer work. We may experiment with further optimisations, but for now I'd rather not break anything. Besides, View interfaces don't change that often) |
I changed my mind - this idea not gonna optimize anything because presenters are transitively dependent on view interfaces. What this mean is if view interface is changed, presenter will be recompiled no matter what. You can find more explanation about transitive dependency analysis from this thread (starting from here especially). The only way to separate presenter and view state compilation is to remove view interface from presenter AST completely, which seems impossible for us. Also, I think you are not right about That being said, I don't plan to add more changes to this PR. |
This fixes #38