[url_launcher] Convert Android to Pigeon #4045

stuartmorgan-g · 2023-05-19T19:13:26Z

Converts from direct use of method channels to using Pigeon.

Adds unit testing of the actual implementation logic. The previous unit tests only covered the intermediate class that converted method calls/arguments into Java calls, which has been removed since that's now handled by Pigeon.

Part of flutter/flutter#117915

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…the class

stuartmorgan-g · 2023-05-19T19:17:03Z

...r/url_launcher_android/android/src/main/java/io/flutter/plugins/urllauncher/UrlLauncher.java

+        context,
+        intent -> {
+          ComponentName componentName = intent.resolveActivity(context.getPackageManager());
+          return componentName == null ? null : componentName.toShortString();


I was able to narrow the code that's not being unit tested from "all of UrlLauncher" to just these two lines (which come from the old canLaunch). I spent some time looking at this, and didn't see how to mock enough of context to make intent.resolveActivity return a dummy ComponentName, so I just extracted this one small piece into something tests could replace.

stuartmorgan-g · 2023-05-19T19:18:04Z

...r/url_launcher_android/android/src/main/java/io/flutter/plugins/urllauncher/UrlLauncher.java

-    NO_ACTIVITY,
-    /** No Activity found that can handle given intent. */
-    ACTIVITY_NOT_FOUND,
+  private static @NonNull Bundle extractBundle(Map<String, String> headersMap) {


This, and the strings in the error in ensureActivity, are the surviving code from MethodCallHandlerImpl.

gmackall

This LGTM, though I haven't reviewed any conversions to pigeon before. In your opinion is this a change that it would be good for me to find a second reviewer on, or is the perceived risk low?

stuartmorgan-g · 2023-05-25T16:47:14Z

The pigeon surface here is quite small, so between that and the expanded native unit testing I think the risk is pretty low.

bparrishMines · 2023-05-25T21:46:23Z

@stuartmorgan This is causing a failure on main:

Analyzing url_launcher_android...

   info - test/url_launcher_android_test.dart:89:7 - Missing an 'await' for the 'Future' computed by this expression. Try adding an 'await' or wrapping the expression un 'unawaited'. - unawaited_futures
   info - test/url_launcher_android_test.dart:104:7 - Missing an 'await' for the 'Future' computed by this expression. Try adding an 'await' or wrapping the expression un 'unawaited'. - unawaited_futures
   info - test/url_launcher_android_test.dart:170:7 - Missing an 'await' for the 'Future' computed by this expression. Try adding an 'await' or wrapping the expression un 'unawaited'. - unawaited_futures
   info - test/url_launcher_android_test.dart:185:7 - Missing an 'await' for the 'Future' computed by this expression. Try adding an 'await' or wrapping the expression un 'unawaited'. - unawaited_futures

bparrishMines · 2023-05-25T22:12:57Z

#4086

Fixes `unawaited_futures` lint errors from #4045. This only updates the tests, so no version bump. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use `dart format`.) - [x] I signed the [CLA]. - [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. `[shared_preferences]` - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes]. - [ ] I updated `CHANGELOG.md` to add a description of the change, [following repository CHANGELOG style]. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [relevant style guides]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat [pub versioning philosophy]: https://dart.dev/tools/pub/versioning [exempt from version changes]: https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#version-and-changelog-updates [following repository CHANGELOG style]: https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changelog-style [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests

flutter/packages@995bfc5...9f8dcc5 2023-05-25 [email protected] Append $flutter/osx sdk property to existing platforms/targets relying on xcode (flutter/packages#4074) 2023-05-25 [email protected] [url_launcher_android] Fix `unawaited_futures` lint errors (flutter/packages#4086) 2023-05-25 [email protected] [url_launcher] Convert Android to Pigeon (flutter/packages#4045) 2023-05-25 [email protected] [multiple packages] Update collection-fors to prefer final (as per updated `prefer_final_in_for_each`) (flutter/packages#4073) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

rafaelmeteoro · 2023-05-29T22:59:26Z

I catch an error on build android:

/opt/flutter_3.3.0/.pub-cache/hosted/pub.dartlang.org/url_launcher_android-6.0.35/android/src/main/java/io/flutter/plugins/urllauncher/UrlLauncher.java:66: error: cannot find symbol01:34
    if (BuildConfig.DEBUG) {
        ^
  symbol:   variable BuildConfig01:34
  location: class UrlLauncher

anyone else with this?

bparrishMines · 2023-06-01T16:18:21Z

@rafaelmeteoro Could you open a new issue with an explanation of when you are seeing this error? https://github.com/flutter/flutter/issues/new/choose

stuartmorgan-g added 19 commits May 18, 2023 13:59

Minimial native conversion

0b269e8

Version bump

59320ef

Move setup

e903391

Rename method handler class

79edacf

Fix existing unit tests

0c9d14b

Split launch helper methods, add test

bba0f05

Increase test coverage of canLaunch and launch

8bae9b4

Merge in launch

a66adf2

Expand openWebView testing

2b2da9c

Move openWebView impl

5a217f9

Cleanup

96691af

Improve close testing

1bb4d99

Merge in close

fb8fd44

Move even more logic out of the resolver

690c5f1

Fold the last bit of UrlLauncher into UrlLauncherApiImpl, and remove …

48a179b

…the class

Rename impl class to just UrlLauncher

d65aea2

Rename variable to match

0dac205

Update Dart

67b825f

Undo the use of a result enum

97f1be2

stuartmorgan-g requested a review from gmackall as a code owner May 19, 2023 19:13

stuartmorgan-g mentioned this pull request May 19, 2023

[url_launcher] Convert to Pigeon flutter/flutter#117915

Closed

5 tasks

github-actions bot added p: url_launcher platform-android labels May 19, 2023

stuartmorgan-g commented May 19, 2023

View reviewed changes

stuartmorgan-g mentioned this pull request May 21, 2023

[url_launcher] Support requiring non-browser for launchUrl on Android flutter/flutter#66721

Closed

gmackall approved these changes May 25, 2023

View reviewed changes

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label May 25, 2023

auto-submit bot merged commit e14d0d5 into flutter:main May 25, 2023

bparrishMines mentioned this pull request May 25, 2023

[url_launcher_android] Fix unawaited_futures lint errors #4086

Merged

11 tasks

stuartmorgan-g deleted the url-launcher-android-pigeon branch May 25, 2023 22:35

engine-flutter-autoroll mentioned this pull request May 26, 2023

Roll Packages from 995bfc5f9857 to 9f8dcc562b55 (4 revisions) flutter/flutter#127671

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 26, 2023

e14d0d5 [url_launcher] Convert Android to Pigeon (flutter/packages#4045)

9f3d1cb

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[url_launcher] Convert Android to Pigeon #4045

[url_launcher] Convert Android to Pigeon #4045

Uh oh!

stuartmorgan-g commented May 19, 2023

Uh oh!

stuartmorgan-g May 19, 2023

Uh oh!

stuartmorgan-g May 19, 2023

Uh oh!

gmackall left a comment

Uh oh!

stuartmorgan-g commented May 25, 2023

Uh oh!

bparrishMines commented May 25, 2023

Uh oh!

bparrishMines commented May 25, 2023

Uh oh!

rafaelmeteoro commented May 29, 2023

Uh oh!

bparrishMines commented Jun 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[url_launcher] Convert Android to Pigeon #4045

[url_launcher] Convert Android to Pigeon #4045

Uh oh!

Conversation

stuartmorgan-g commented May 19, 2023

Pre-launch Checklist

Uh oh!

stuartmorgan-g May 19, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g May 19, 2023

Choose a reason for hiding this comment

Uh oh!

gmackall left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented May 25, 2023

Uh oh!

bparrishMines commented May 25, 2023

Uh oh!

bparrishMines commented May 25, 2023

Uh oh!

rafaelmeteoro commented May 29, 2023

Uh oh!

bparrishMines commented Jun 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants