Skip to content

Remove isNewAndroidEmbeddingEnabled flag when reading an existing pro…#42684

Merged
blasten merged 6 commits intoflutter:masterfrom
blasten:embedding_flag
Oct 16, 2019
Merged

Remove isNewAndroidEmbeddingEnabled flag when reading an existing pro…#42684
blasten merged 6 commits intoflutter:masterfrom
blasten:embedding_flag

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
@mklim mklim mentioned this pull request Oct 14, 2019
Closed
@codecov
Copy link

codecov bot commented Oct 15, 2019 •
edited
Loading

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.

@blasten blasten mentioned this pull request Oct 15, 2019
Merged
amirh
amirh approved these changes Oct 15, 2019
View reviewed changes
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

packages/flutter_tools/test/general.shard/macos/cocoapods_test.dart

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

@amirh amirh Oct 15, 2019

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

@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.

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.

packages/flutter_tools/lib/src/plugins.dart
@@ -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

@amirh amirh Oct 15, 2019

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 •
edited
Loading

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
@Inconnu08
Remove isNewAndroidEmbeddingEnabled flag when reading an existing pro… (
15b3d3a 
flutter#42684)
@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.

Reviewers

1 more reviewer

@amirh amirh amirh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@blasten @amirh @googlebot @fluttergithubbot