[local_auth] Convert Android to Pigeon #3748

stuartmorgan · 2023-04-17T20:10:02Z

Updates the platform communication to use Pigeon. This includes some changes to the API boundary:

Collected authentication options into an options object; this avoids having a lot of positional boolean arguments (since Pigeon doesn't currently support named arguments).
Collected strings into a strings object, since having them as individual parameters would have been extremely messy.
Changes the authenticate return from a bool+exceptions to an enum that encompasses all of the explicitly known failure modes. To avoid a breaking change for clients, the Dart code creates PlatformExceptions that match the old ones, but this will make it much easier to someday make a (cross-platform) breaking change to replace them with better errors per the best practices we have documented on the wiki. Using an enum rather than throwing errors avoids the need to do string matching when we want to eventually translate them into something other than PlatformException.

This removes all Java warnings from the baseline file; the remaining issues in code I wasn't already changing were easy enough to fix opportunistically. There are still XML-based warnings, but fixing those was well out of scope so I left them.

Part of flutter/flutter#117912

Pre-launch Checklist

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

stuartmorgan · 2023-04-17T20:11:28Z

...h/local_auth_android/android/src/main/java/io/flutter/plugins/localauth/LocalAuthPlugin.java

-  /*
-   * Stops the authentication if in progress.
-   */
-  private void stopAuthentication(Result result) {


Almost all of the "removed" code in this removed-code diff block is actually just moved up inline into the new Pigeon API implementation functions above.

stuartmorgan · 2023-04-17T20:13:51Z

...h/local_auth_android/android/src/main/java/io/flutter/plugins/localauth/LocalAuthPlugin.java

    binding.addActivityResultListener(resultListener);
    setServicesFromActivity(binding.getActivity());
    lifecycle = FlutterLifecycleAdapter.getActivityLifecycle(binding);
-    channel.setMethodCallHandler(this);


We're no longer creating and removing the channel as part of the activity lifecycle. It's inconvenient to do with Pigeon (since we need the messenger), and I don't see any need for it since we're checking the activity in the necessary functions anyway. In fact, I think with the old code people would see "missing plugin" exceptions instead of our "this plugin needs a foreground activity" errors, which seems strictly worse.

flutter/packages@faf53fb...88591be 2023-04-19 engine-flutter-autoroll@skia.org Roll Flutter from 42fb0b2 to 3476b96 (20 revisions) (flutter/packages#3760) 2023-04-19 49699333+dependabot[bot]@users.noreply.github.com Bump cirrusci/flutter from `794fbbc` to `d99b1ba` in /.ci (flutter/packages#3724) 2023-04-18 10687576+bparrishMines@users.noreply.github.com [webview_flutter] Adds support to listen to url changes (flutter/packages#3313) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter from 15cb1f8 to 42fb0b2 (19 revisions) (flutter/packages#3756) 2023-04-18 49699333+dependabot[bot]@users.noreply.github.com [webview]: Bump com.android.tools.build:gradle from 7.2.2 to 8.0.0 in /packages/webview_flutter/webview_flutter_android/android (flutter/packages#3739) 2023-04-18 stuartmorgan@google.com [local_auth] Convert Android to Pigeon (flutter/packages#3748) 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 flutter-ecosystem@google.com,rmistry@google.com 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

asaarnak · 2023-04-20T11:09:20Z

When we updated from 1.0.21 to 1.0.23 we started getting this error:

Fatal Exception: io.flutter.plugins.firebase.crashlytics.FlutterError: PlatformException(channel-error, Unable to establish connection on channel., null, null)
       at LocalAuthApi.getEnrolledBiometrics(messages.g.dart:313)
       at LocalAuthAndroid.getEnrolledBiometrics(local_auth_android.dart:99)

stuartmorgan · 2023-04-20T14:28:15Z

Please file issues in the issue tracker.

The Pigeon conversion in flutter#3748 used a `List<enum>` return value, which Pigeon doesn't support correctly (see flutter/flutter#125230 for catching this earlier next time). Because the method doesn't return any values in integration test environments it wasn't caught (since the issue is in Pigeon message handling, and thus doesn't show up in unit tests of the plugin). This adds a wrapper class, since enum fields do work. Fixes flutter/flutter#125214

asaarnak · 2023-04-21T07:18:45Z

Sorry, next time i'll file an issue. Found that there already exists one: flutter/flutter#125236

The Pigeon conversion in #3748 used a `List<enum>` return value, which Pigeon doesn't support correctly (see flutter/flutter#125230 for catching this earlier next time). Because the method doesn't return any values in integration test environments it wasn't caught (since the issue is in Pigeon message handling, and thus doesn't show up in unit tests of the plugin). This adds a wrapper class, since enum fields do work. Fixes flutter/flutter#125214

Updates the platform communication to use Pigeon. This includes some changes to the API boundary: - Collected authentication options into an options object; this avoids having a lot of positional boolean arguments (since Pigeon doesn't currently support named arguments). - Collected strings into a strings object, since having them as individual parameters would have been extremely messy. - Changes the `authenticate` return from a bool+exceptions to an enum that encompasses all of the explicitly known failure modes. To avoid a breaking change for clients, the Dart code creates `PlatformException`s that match the old ones, but this will make it much easier to someday make a (cross-platform) breaking change to replace them with better errors per the best practices we have documented on the wiki. Using an enum rather than throwing errors avoids the need to do string matching when we want to eventually translate them into something other than `PlatformException`. The Pigeon file, Dart implementation, and rewritten tests are all based very heavily on the recent Android migration: #3748 I noted several bugs that I noticed in the existing implementation during the migration. I intentionally didn't fix them so this isn't changing behavior at the same time that it's changing so much of the code and tests, but I marked them with TODO comments. Fixes flutter/flutter#117912

Updates the platform communication to use Pigeon. This includes some changes to the API boundary: - Collected authentication options into an options object; this avoids having a lot of positional boolean arguments (since Pigeon doesn't currently support named arguments). - Collected strings into a strings object, since having them as individual parameters would have been extremely messy. - Changes the `authenticate` return from a bool+exceptions to an enum that encompasses all of the explicitly known failure modes. To avoid a breaking change for clients, the Dart code creates `PlatformException`s that match the old ones, but this will make it much easier to someday make a (cross-platform) breaking change to replace them with better errors per the best practices we have documented on the wiki. Using an enum rather than throwing errors avoids the need to do string matching when we want to eventually translate them into something other than `PlatformException`. This removes all Java warnings from the baseline file; the remaining issues in code I wasn't already changing were easy enough to fix opportunistically. There are still XML-based warnings, but fixing those was well out of scope so I left them. Part of flutter/flutter#117912

The Pigeon conversion in flutter#3748 used a `List<enum>` return value, which Pigeon doesn't support correctly (see flutter/flutter#125230 for catching this earlier next time). Because the method doesn't return any values in integration test environments it wasn't caught (since the issue is in Pigeon message handling, and thus doesn't show up in unit tests of the plugin). This adds a wrapper class, since enum fields do work. Fixes flutter/flutter#125214

Updates the platform communication to use Pigeon. This includes some changes to the API boundary: - Collected authentication options into an options object; this avoids having a lot of positional boolean arguments (since Pigeon doesn't currently support named arguments). - Collected strings into a strings object, since having them as individual parameters would have been extremely messy. - Changes the `authenticate` return from a bool+exceptions to an enum that encompasses all of the explicitly known failure modes. To avoid a breaking change for clients, the Dart code creates `PlatformException`s that match the old ones, but this will make it much easier to someday make a (cross-platform) breaking change to replace them with better errors per the best practices we have documented on the wiki. Using an enum rather than throwing errors avoids the need to do string matching when we want to eventually translate them into something other than `PlatformException`. The Pigeon file, Dart implementation, and rewritten tests are all based very heavily on the recent Android migration: flutter#3748 I noted several bugs that I noticed in the existing implementation during the migration. I intentionally didn't fix them so this isn't changing behavior at the same time that it's changing so much of the code and tests, but I marked them with TODO comments. Fixes flutter/flutter#117912

stuartmorgan added 2 commits April 17, 2023 15:59

Define API and update Java

936f7a2

Update Dart, and add more Dart testing

3ab9b44

stuartmorgan requested a review from reidbaker April 17, 2023 20:10

stuartmorgan requested a review from camsim99 as a code owner April 17, 2023 20:10

stuartmorgan mentioned this pull request Apr 17, 2023

[local_auth] Convert to Pigeon flutter/flutter#117912

Closed

3 tasks

github-actions bot added p: local_auth platform-android labels Apr 17, 2023

stuartmorgan commented Apr 17, 2023

View reviewed changes

Version bump

ba64405

reidbaker approved these changes Apr 18, 2023

View reviewed changes

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 18, 2023

auto-submit bot merged commit 773c7a8 into flutter:main Apr 18, 2023

engine-flutter-autoroll mentioned this pull request Apr 19, 2023

Roll Packages from faf53fb3998c to c6f207ca73ce (5 revisions) flutter/flutter#125136

Closed

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

773c7a8 [local_auth] Convert Android to Pigeon (flutter/packages#3748)

b7388eb

engine-flutter-autoroll mentioned this pull request Apr 19, 2023

Roll Packages from faf53fb3998c to c6f207ca73ce (5 revisions) flutter/flutter#125149

Closed

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

773c7a8 [local_auth] Convert Android to Pigeon (flutter/packages#3748)

1eb0848

engine-flutter-autoroll mentioned this pull request Apr 19, 2023

Roll Packages from faf53fb3998c to 88591becfb3f (6 revisions) flutter/flutter#125153

Merged

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

773c7a8 [local_auth] Convert Android to Pigeon (flutter/packages#3748)

6c76286

stuartmorgan deleted the local-auth-android-pigeon branch April 20, 2023 14:25

stuartmorgan mentioned this pull request Apr 20, 2023

[local_auth] Fix enum return on Android #3780

Merged

11 tasks

huycozy mentioned this pull request Apr 21, 2023

[local_auth_android 1.0.23] getAvailableBiometrics - Unhandled Exception: PlatformException flutter/flutter#125236

Closed

2 tasks

reidbaker mentioned this pull request Apr 25, 2023

[various] update agp and gradle for all examples in packages #3822

Merged

11 tasks

stuartmorgan mentioned this pull request May 12, 2023

[local_auth] Migrate iOS to Pigeon #3974

Merged

11 tasks

stuartmorgan mentioned this pull request Jul 7, 2023

[local_auth]: The targetSdk version should not be declared in the android manifest file. flutter/flutter#78541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[local_auth] Convert Android to Pigeon #3748

[local_auth] Convert Android to Pigeon #3748

stuartmorgan commented Apr 17, 2023

stuartmorgan Apr 17, 2023

stuartmorgan Apr 17, 2023 •

edited

Loading

asaarnak commented Apr 20, 2023

stuartmorgan commented Apr 20, 2023

asaarnak commented Apr 21, 2023

[local_auth] Convert Android to Pigeon #3748

[local_auth] Convert Android to Pigeon #3748

Conversation

stuartmorgan commented Apr 17, 2023

Pre-launch Checklist

stuartmorgan Apr 17, 2023

Choose a reason for hiding this comment

stuartmorgan Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

asaarnak commented Apr 20, 2023

stuartmorgan commented Apr 20, 2023

asaarnak commented Apr 21, 2023

stuartmorgan Apr 17, 2023 •

edited

Loading