Generate projects using the new Android embedding #41666
Conversation
fa4fc85 to
fbfccc6
Compare
Codecov Report
@@ Coverage Diff @@
## master #41666 +/- ##
==========================================
+ Coverage 58.69% 59.37% +0.68%
==========================================
Files 194 193 -1
Lines 18934 18936 +2
==========================================
+ Hits 11113 11243 +130
+ Misses 7821 7693 -128
Continue to review full report at Codecov.
|
35c96bb to
76b3f8b
Compare
| @@ -30,4 +29,8 @@ | |||
| </intent-filter> | |||
| </activity> | |||
| </application> | |||
| {{#useNewAndroidEmbedding}} | |||
| <!-- Don't delete this comment. | |||
There was a problem hiding this comment.
I wonder if we can leave some breadcrumbs for future maintainers to realize what this is for. Can be sparse since it'll be in users' projects.
There was a problem hiding this comment.
I think we should find another approach to this.
There was a problem hiding this comment.
@matthew-carroll any suggestion?
There was a problem hiding this comment.
Can you clarify the precise reason that we need this information?
There was a problem hiding this comment.
app1 uses the old embedding
app2 uses the new embedding.
This hint is used by the tool to generate the new plugin registrant for app2.
| @@ -63,9 +63,8 @@ class Plugin { | |||
| } | |||
| if (pluginYaml != null && pluginYaml['platforms'] != null) { | |||
| return Plugin._fromMultiPlatformYaml(name, path, pluginYaml); | |||
| } else { | |||
There was a problem hiding this comment.
Do we need to remove this else statement? Unless we have a series of if-return, if-return, if-return, return then the if-else looks more appropriate to me.
There was a problem hiding this comment.
This is the last statement and the else statement is redundant/unnecessary nesting.
There was a problem hiding this comment.
Ok. I wouldn't call it redundant, but it's your call.
.../flutter_tools/templates/module/android/library_new_embedding/Flutter.tmpl/build.gradle.tmpl
Outdated
Show resolved
Hide resolved
| android:name="flutterProjectType" | ||
| android:value="module" /> | ||
| </application> | ||
| <!-- Don't delete this comment. |
There was a problem hiding this comment.
I don't think we should have this.
...ols/templates/plugin/android-java.tmpl/src/main/java/androidIdentifier/pluginClass.java.tmpl
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public void onMethodCall(MethodCall call, Result result) { |
There was a problem hiding this comment.
Can we generate 2 files instead of 1? I think we should incentivize channel comms in a dedicated file so that people might actually unit test message serialization.
There was a problem hiding this comment.
The templates in this directory is used for plugin projects that don't use the new embedding. However, if 1 file is applicable to the new embedding only, then we would need to duplicate the directory or refactor the code to exclude certain files. I can follow up with this later, wdyt?
There was a problem hiding this comment.
The top of this template says "implements FlutterPlugin", which is the new embedding.
There was a problem hiding this comment.
This file contains the code for both embedding. see {{#useNewAndroidEmbedding}} and {{^useNewAndroidEmbedding}} sections.
...s/templates/plugin/android-kotlin.tmpl/src/main/kotlin/androidIdentifier/pluginClass.kt.tmpl
Show resolved
Hide resolved
|
@matthew-carroll I reworked the detection mechanism using |
| for (xml.XmlElement metaData in document.findAllElements('meta-data')) { | ||
| final String name = metaData.getAttribute('android:name'); | ||
| if (name == 'flutterEmbedding') { | ||
| return metaData.getAttribute('android:value'); |
There was a problem hiding this comment.
It would probably be a good idea to check accepted values here and throw an appropriate error if a bad value is passed in.
| import io.flutter.plugin.common.MethodChannel.Result; | ||
|
|
||
| /** {{pluginClass}} */ | ||
| public class {{pluginClass}} implements FlutterPlugin, MethodCallHandler { |
There was a problem hiding this comment.
Plugins generated with this template will not be backward compatible with apps that are using the v1 embedding.
Should we have a registerWith here as well? at least until we are ready to deprecate the v1 embedding?
There was a problem hiding this comment.
so far, the plan is to exit the tool if an app using the v1 embedding attempts to use a plugin using the v2 embedding. Issue: #43779
There was a problem hiding this comment.
Offline discussion - we should probably see enough adoption of v2 embedding in Flutter apps and be willing to deprecate it before we make it easy to create plugins that do not support the v1 embedder.
There was a problem hiding this comment.
I also agree with that conclusion. For at least 2-3 quarters, I would recommend generating backward compatible plugins with appropriate comment instructions.
* Generate projects using the new Android embedding * Add comment about usesNewEmbedding:true * Feedback * Rework way to detect new embedding in new apps
Description
Generate projects using the new Android embedding. This feature is under a flag, so to generate new projects, run:
Pending feature: #41757
Related Issues
Fixes #33161
Fixes #35509
Tests
Unit tests
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?