Skip to content

Remove isNewAndroidEmbeddingEnabled flag when reading an existing pro… #42684

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

Merged
merged 6 commits into from
Oct 16, 2019

Conversation

blasten
Copy link

@blasten blasten commented Oct 14, 2019

Description

The flag is still relevant for flutter create.

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@blasten blasten requested a review from amirh October 14, 2019 20:10
@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 14, 2019
@codecov
Copy link

codecov bot commented Oct 15, 2019

Codecov Report

Merging #42684 into master will decrease coverage by 0.37%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #42684      +/-   ##
==========================================
- Coverage   60.21%   59.84%   -0.38%     
==========================================
  Files         194      194              
  Lines       18877    18875       -2     
==========================================
- Hits        11367    11295      -72     
- Misses       7510     7580      +70
Flag Coverage Δ
#flutter_tool 59.84% <ø> (-0.38%) ⬇️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/plugins.dart 86.78% <ø> (-3.57%) ⬇️
...ckages/flutter_tools/lib/src/platform_plugins.dart 73% <ø> (-5.22%) ⬇️
...ages/flutter_tools/lib/src/commands/build_web.dart 29.41% <0%> (-64.71%) ⬇️
packages/flutter_tools/lib/src/web/compile.dart 10.34% <0%> (-62.07%) ⬇️
...ages/flutter_tools/lib/src/macos/macos_device.dart 22.22% <0%> (-29.63%) ⬇️
...ackages/flutter_tools/lib/src/commands/daemon.dart 21.8% <0%> (-12.07%) ⬇️
...lutter_tools/lib/src/commands/build_appbundle.dart 90.32% <0%> (-9.68%) ⬇️
...ages/flutter_tools/lib/src/test/test_compiler.dart 61.64% <0%> (-6.85%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a62bb3d...70a7947. Read the comment docs.

Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -273,7 +273,7 @@ void main() {
..writeAsStringSync('Existing release config');

final FlutterProject project = FlutterProject.fromPath('project');
await injectPlugins(project);
await injectPlugins(project, checkProjects: true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this related to this PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. When this flag is true, injectPlugins skips generating the Android plugin registrant since there isn't an android/ directory in the project under test.

@@ -360,9 +360,6 @@ List<Map<String, dynamic>> _extractPlatformMaps(List<Plugin> plugins, String typ
/// Returns the version of the Android embedding that the current
/// [project] is using.
String _getAndroidEmbeddingVersion(FlutterProject project) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR, just curious - would it be possible / make sense for the logic here and in _getEmbeddingVersion in platform_plugins.dart to be the same? (seems like one is deciding based on a manifest key and the other based on Java imports?)

Copy link
Author

@blasten blasten Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have the app's main class name in pubspec.yaml, so that would require an extra hop. One possibility is that we get the main app class/package from AndroidManifest.xml and then check the import in the java class. That would still require to parse the XML file.

@blasten blasten merged commit 0a93f4e into flutter:master Oct 16, 2019
@blasten blasten deleted the embedding_flag branch October 16, 2019 19:47
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants