remove conditional code for handling very old AGP versions #162053
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
e38b007 to
60edbfd
Compare
81bc9fb to
8ecd473
Compare
bde6399 to
28502a5
Compare
| @@ -468,7 +475,8 @@ class FlutterPlugin implements Plugin<Project> { | |||
| // | |||
| // The output file is parsed and used by devtool. | |||
| private static void addTasksForOutputsAppLinkSettings(Project project) { | |||
| project.android.applicationVariants.all { variant -> | |||
| BaseAppModuleExtension android = project.extensions.findByType(BaseAppModuleExtension.class) | |||
There was a problem hiding this comment.
Is it important that this be a BaseAppModuleExtension? I believe ApplicationExtension is a supertype that isn't marked internal. I'd like to avoid importing internal classes if possible here.
There was a problem hiding this comment.
By marked internal I mean has internal in it's package name. For some reason AGP has a lot of classes with internal in their package name that are still public, but I assume that could change any release without notice.
There was a problem hiding this comment.
Agree, if an internal import is required then lets add a comment explaining why and moving forward.
There was a problem hiding this comment.
got it, will update it soon
| @@ -1415,14 +1416,15 @@ class FlutterPlugin implements Plugin<Project> { | |||
| return copyFlutterAssetsTask | |||
| } // end def addFlutterDeps | |||
| if (isFlutterAppProject()) { | |||
| project.android.applicationVariants.all { variant -> | |||
| Task assembleTask = getAssembleTask(variant) | |||
| BaseAppModuleExtension android = project.extensions.findByType(BaseAppModuleExtension.class) | |||
| @@ -468,7 +475,8 @@ class FlutterPlugin implements Plugin<Project> { | |||
| // | |||
| // The output file is parsed and used by devtool. | |||
| private static void addTasksForOutputsAppLinkSettings(Project project) { | |||
| project.android.applicationVariants.all { variant -> | |||
| BaseAppModuleExtension android = project.extensions.findByType(BaseAppModuleExtension.class) | |||
There was a problem hiding this comment.
Agree, if an internal import is required then lets add a comment explaining why and moving forward.
|
from android triage: @bartekpacia Is this still on your radar? If not, no worries. We are happy to close it. |
|
Yup thanks for reminding. I'll get it finished very soon |
28502a5 to
25c34f2
Compare
| @@ -1426,6 +1440,7 @@ class FlutterPlugin implements Plugin<Project> { | |||
| assert(appProject != null) : "Project :${hostAppProjectName} doesn't exist. To customize the host app project name, set `flutter.hostAppProjectName=<project-name>` in gradle.properties." | |||
| // Wait for the host app project configuration. | |||
| appProject.afterEvaluate { | |||
| // AndroidComponentsExtension android = appProject.extensions.findByType(AndroidComponentsExtension.class) | |||
There was a problem hiding this comment.
Probably meant to delete this?
| @@ -467,7 +474,8 @@ class FlutterPlugin implements Plugin<Project> { | |||
| // | |||
| // The output file is parsed and used by devtool. | |||
| private static void addTasksForOutputsAppLinkSettings(Project project) { | |||
| project.android.applicationVariants.all { variant -> | |||
| AbstractAppExtension android = (AbstractAppExtension) project.extensions.findByName("android") | |||
There was a problem hiding this comment.
Does this not work with the findByType method? I prefer that to casting, if it does work. No worries if not.
There was a problem hiding this comment.
Apparently findByType needs an specific type, not some supertype. When I passed AbstractAppExtension to findByType it crashed :(
There was a problem hiding this comment.
Hmm would it maybe work with ApplicationExtension instead of AbstractApplicationExtension? I believe we are already doing this here
There was a problem hiding this comment.
unfortunately then applicationVariants isn't recognized, and this is why I put an explicit type here in the first place (to aid in migration to statically typed Kotlin later on)
25c34f2 to
f741f50
Compare
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@gmackall something's up with goldens? |
updating branch I dont think this pr would update goldens |
I think code paths for AGP version <7 aren't needed anymore.
I can also imagine it will be hard to do such dynamic stuff in Kotlin, so it's better to get rid of it.
Also, I added some explicit types, this reduces the amount of grey/unresolved identifiers in IntelliJ by quite a lot, making it easier to understand code (again, this should help in conversion to Kotlin later on).
Pre-launch Checklist
///).