[google_maps_flutter] Semi-convert remaining Android host API calls to Pigeon #6980

stuartmorgan-g · 2024-06-24T20:23:27Z

Does a "shallow" Pigeon conversion of the remaining host API calls in the Android implementation. All of the actual method calls are now Pigeon, but for the most part the data structures are not yet converted, and the Pigeon representations of the map objects are instead placeholders that currently just wrap the existing JSON serialization. This is done for a few reasons:

Keeps the incremental PR relatively small and easy to understand.
Quickly gets us to a state where any new APIs added will automatically use Pigeon, reducing further accumulation of technical debt.
Avoids duplication of handling code until [pigeon] Make the codec public flutter#150631 is resolved. As noted in a TODO added in this PR, almost all of the data structures that are passed in these methods are also passed through the PlatformView factory constructor, and Pigeon doesn't yet support using the Pigeon codec in non-Pigeon-generated code, so we would need to have both the structured and JSON handler for all of these objects if we converted them now.

Future PRs will be able to incrementally convert each map object to a structured form. Converting the Flutter APIs (Java->Dart) will also be done in a follow-up, to limit the scope of this PR.

Adds Dart unit tests for each method call, as they were previously not unit tested.

Part of flutter/flutter#117907

Pre-launch Checklist

Eliminates the remaining google_maps_flutter cases of the once-common-in-this-repo type abuse of getting a value from a method channel arguments dictionary that may either be type `T` or `NSNull`, but assigning directly to a variable of type `T`, and then later comparing that variable to `NSNull` with a cast to silence the (correct) warning from the compiler. Longer term this will be eliminated entirely when the Pigeon conversion is complete, but currently I expect that in the short term we will only do a "shallow" Pigeon conversion (see flutter#6980 for an Android example), meaning this code will likely stay around for a while.

Eliminates the remaining google_maps_flutter cases of the once-common-in-this-repo type abuse of getting a value from a method channel arguments dictionary that may either be type `T` or `NSNull`, but assigning directly to a variable of type `T`, and then later comparing that variable to `NSNull` with a cast to silence the (correct) warning from the compiler. Longer term this will be eliminated entirely when the Pigeon conversion is complete, but currently I expect that in the short term we will only do a "shallow" Pigeon conversion (see #6980 for an Android example), meaning this code will likely stay around for a while.

reidbaker · 2024-06-28T15:28:20Z

...lutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapInitializer.java

-        case "default":
-          initializeWithRendererRequest(null);
-          break;
-        default:


Is the default condition no longer possible?

The special "default" value no longer exists, because requesting the default is now expressed as null in the Pigeon API, just as it already was at both the Dart API and Maps SDK API levels.

reidbaker · 2024-06-28T15:29:49Z

...lutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapInitializer.java

-              "Renderer initialization called with invalid renderer type",
-              null);
-          initializationResult = null;
+      MapsInitializer.Renderer rendererType = null;


Nit: move this to a Convert.toMapRendererType

I will understand if you tell me this is overkill.

reidbaker · 2024-06-28T15:31:18Z

...lutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapInitializer.java

          break;
        default:
          initializationResult.error(
-              "Unknown renderer type", "Initialized with unknown renderer type", null);
+              new Messages.FlutterError(
+                  "Unknown renderer type", "Initialized with unknown renderer type", null));


I can see the argument for keeping this error the same but consider passing the type instead of null so that if we ever hit this condition we might be able to trace why the type was wrong.

Added renderer.name() as the details; we don't like to change errors since it could break people, but adding data that just wasn't there is safe for any but the most contrived case.

reidbaker · 2024-06-28T15:36:06Z

...oogle_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Messages.java

@@ -647,42 +1120,88 @@ private PigeonCodec() {}
    protected Object readValueOfType(byte type, @NonNull ByteBuffer buffer) {
      switch (type) {
        case (byte) 129:


Possibly for future work but consider either extracting these to named constants in this class or putting the byte as a static value on each of the class items then having a list of supported object types that all have a byte identifier and a read/write Value method.

Second pass note: Unless this is generated (which I don't think it is)

This entire file is generated (see comment at the very top). In all other languages we make that clear with a .g. in the file name, but annoyingly Java doesn't allow that since the file name and the class name have to exactly match.

… calls to Pigeon (flutter/packages#6980)

@matanlurey

flutter/packages@412ec46...d2705fb 2024-07-01 [email protected] Roll Flutter from 651a17d to 99bb2ff (10 revisions) (flutter/packages#7038) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.test.espresso:espresso-core from 3.5.1 to 3.6.1 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#7032) 2024-07-01 [email protected] [google_maps_flutter] Semi-convert remaining Android host API calls to Pigeon (flutter/packages#6980) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [local_auth]: Bump androidx.test.espresso:espresso-core from 3.5.1 to 3.6.1 in /packages/local_auth/local_auth_android/android (flutter/packages#7022) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [video_player]: Bump com.android.tools.build:gradle from 7.2.1 to 8.5.0 in /packages/video_player/video_player_android/android (flutter/packages#6931) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump com.android.tools.build:gradle from 7.3.0 to 8.5.0 in /packages/camera/camera_android/android (flutter/packages#6928) 2024-07-01 [email protected] [ci] Adds @matanlurey to some Android CODEOWNERS until Impeller is enabled. (flutter/packages#7014) 2024-06-29 [email protected] [video_player] Bumps web implementation dependency. (flutter/packages#7015) 2024-06-29 [email protected] Manual roll Flutter from 15f95ce to 651a17d (7 revisions) (flutter/packages#7013) 2024-06-28 [email protected] [flutter_markdown] Add horizontal scroll for markdown table (flutter/packages#6983) 2024-06-28 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.3.1 to 2.3.3 (flutter/packages#6709) 2024-06-28 [email protected] [google_maps_flutter] Fix Obj-C type handling (flutter/packages#7010) 2024-06-28 [email protected] Manual roll Flutter from e726eb4 to 15f95ce (48 revisions) (flutter/packages#7002) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

@matanlurey

flutter/packages@412ec46...d2705fb 2024-07-01 [email protected] Roll Flutter from 651a17d to 99bb2ff (10 revisions) (flutter/packages#7038) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.test.espresso:espresso-core from 3.5.1 to 3.6.1 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#7032) 2024-07-01 [email protected] [google_maps_flutter] Semi-convert remaining Android host API calls to Pigeon (flutter/packages#6980) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [local_auth]: Bump androidx.test.espresso:espresso-core from 3.5.1 to 3.6.1 in /packages/local_auth/local_auth_android/android (flutter/packages#7022) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [video_player]: Bump com.android.tools.build:gradle from 7.2.1 to 8.5.0 in /packages/video_player/video_player_android/android (flutter/packages#6931) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump com.android.tools.build:gradle from 7.3.0 to 8.5.0 in /packages/camera/camera_android/android (flutter/packages#6928) 2024-07-01 [email protected] [ci] Adds @matanlurey to some Android CODEOWNERS until Impeller is enabled. (flutter/packages#7014) 2024-06-29 [email protected] [video_player] Bumps web implementation dependency. (flutter/packages#7015) 2024-06-29 [email protected] Manual roll Flutter from 15f95ce to 651a17d (7 revisions) (flutter/packages#7013) 2024-06-28 [email protected] [flutter_markdown] Add horizontal scroll for markdown table (flutter/packages#6983) 2024-06-28 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.3.1 to 2.3.3 (flutter/packages#6709) 2024-06-28 [email protected] [google_maps_flutter] Fix Obj-C type handling (flutter/packages#7010) 2024-06-28 [email protected] Manual roll Flutter from e726eb4 to 15f95ce (48 revisions) (flutter/packages#7002) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

@matanlurey

flutter/packages@412ec46...d2705fb 2024-07-01 [email protected] Roll Flutter from 651a17d to 99bb2ff (10 revisions) (flutter/packages#7038) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.test.espresso:espresso-core from 3.5.1 to 3.6.1 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#7032) 2024-07-01 [email protected] [google_maps_flutter] Semi-convert remaining Android host API calls to Pigeon (flutter/packages#6980) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [local_auth]: Bump androidx.test.espresso:espresso-core from 3.5.1 to 3.6.1 in /packages/local_auth/local_auth_android/android (flutter/packages#7022) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [video_player]: Bump com.android.tools.build:gradle from 7.2.1 to 8.5.0 in /packages/video_player/video_player_android/android (flutter/packages#6931) 2024-07-01 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump com.android.tools.build:gradle from 7.3.0 to 8.5.0 in /packages/camera/camera_android/android (flutter/packages#6928) 2024-07-01 [email protected] [ci] Adds @matanlurey to some Android CODEOWNERS until Impeller is enabled. (flutter/packages#7014) 2024-06-29 [email protected] [video_player] Bumps web implementation dependency. (flutter/packages#7015) 2024-06-29 [email protected] Manual roll Flutter from 15f95ce to 651a17d (7 revisions) (flutter/packages#7013) 2024-06-28 [email protected] [flutter_markdown] Add horizontal scroll for markdown table (flutter/packages#6983) 2024-06-28 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.3.1 to 2.3.3 (flutter/packages#6709) 2024-06-28 [email protected] [google_maps_flutter] Fix Obj-C type handling (flutter/packages#7010) 2024-06-28 [email protected] Manual roll Flutter from e726eb4 to 15f95ce (48 revisions) (flutter/packages#7002) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…geon (#7079) Does a "shallow" Pigeon conversion of the remaining host API calls in the iOS implementation. All of the actual method calls are now Pigeon, but for the most part the data structures are not yet converted, and the Pigeon representations of the map objects are instead placeholders that currently just wrap the existing JSON serialization. This is done for a few reasons: - Keeps the incremental PR relatively small and easy to understand. - Quickly gets us to a state where any new APIs added will automatically use Pigeon, reducing further accumulation of technical debt. - Avoids duplication of handling code until flutter/flutter#150631 is resolved. As noted in a TODO added in this PR, almost all of the data structures that are passed in these methods are also passed through the PlatformView factory constructor, and Pigeon doesn't yet support using the Pigeon codec in non-Pigeon-generated code, so we would need to have both the structured *and* JSON handler for all of these objects if we converted them now. Future PRs will be able to incrementally convert each map object to a structured form. Converting the Flutter APIs (Java->Dart) will also be done in a follow-up, to limit the scope of this PR. This is the iOS equivalent of #6980, and the Dart code is largely the same. Part of flutter/flutter#117907

Eliminates the remaining google_maps_flutter cases of the once-common-in-this-repo type abuse of getting a value from a method channel arguments dictionary that may either be type `T` or `NSNull`, but assigning directly to a variable of type `T`, and then later comparing that variable to `NSNull` with a cast to silence the (correct) warning from the compiler. Longer term this will be eliminated entirely when the Pigeon conversion is complete, but currently I expect that in the short term we will only do a "shallow" Pigeon conversion (see flutter/packages#6980 for an Android example), meaning this code will likely stay around for a while.

…geon (#7079) Does a "shallow" Pigeon conversion of the remaining host API calls in the iOS implementation. All of the actual method calls are now Pigeon, but for the most part the data structures are not yet converted, and the Pigeon representations of the map objects are instead placeholders that currently just wrap the existing JSON serialization. This is done for a few reasons: - Keeps the incremental PR relatively small and easy to understand. - Quickly gets us to a state where any new APIs added will automatically use Pigeon, reducing further accumulation of technical debt. - Avoids duplication of handling code until flutter/flutter#150631 is resolved. As noted in a TODO added in this PR, almost all of the data structures that are passed in these methods are also passed through the PlatformView factory constructor, and Pigeon doesn't yet support using the Pigeon codec in non-Pigeon-generated code, so we would need to have both the structured *and* JSON handler for all of these objects if we converted them now. Future PRs will be able to incrementally convert each map object to a structured form. Converting the Flutter APIs (Java->Dart) will also be done in a follow-up, to limit the scope of this PR. This is the iOS equivalent of flutter/packages#6980, and the Dart code is largely the same. Part of flutter/flutter#117907

stuartmorgan-g added 5 commits June 21, 2024 13:50

Shallow conversion of camera updates

510befe

Shallow convert the object updates

260141f

Shallow convert options update

f183559

Convert the global init

e727be7

Version bump

660834c

stuartmorgan-g requested a review from reidbaker as a code owner June 24, 2024 20:23

github-actions bot added p: google_maps_flutter platform-android labels Jun 24, 2024

stuartmorgan-g mentioned this pull request Jun 28, 2024

[google_maps_flutter] Fix Obj-C type handling #7010

Merged

11 tasks

reidbaker approved these changes Jul 1, 2024

View reviewed changes

Review comments

f70856f

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 1, 2024

auto-submit bot merged commit eff249d into flutter:main Jul 1, 2024
74 checks passed

stuartmorgan-g deleted the maps-pigeon-android-host-api-updates-shallow branch July 1, 2024 18:00

engine-flutter-autoroll mentioned this pull request Jul 1, 2024

Roll Packages from 412ec4615aa4 to eff249d72896 (11 revisions) flutter/flutter#151124

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 1, 2024

eff249d [google_maps_flutter] Semi-convert remaining Android host API…

7f8b582

… calls to Pigeon (flutter/packages#6980)

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Packages from 412ec4615aa4 to d2705fb82b26 (13 revisions) flutter/flutter#151169

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 2, 2024

eff249d [google_maps_flutter] Semi-convert remaining Android host API…

01a73d3

… calls to Pigeon (flutter/packages#6980)

reidbaker mentioned this pull request Jul 2, 2024

i151188 update espresso dependencies v2 reidbaker/packages#6

Closed

11 tasks

stuartmorgan-g mentioned this pull request Jul 8, 2024

[google_maps_flutter] Semi-convert remaining iOS host API calls to Pigeon #7079

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[google_maps_flutter] Semi-convert remaining Android host API calls to Pigeon #6980

[google_maps_flutter] Semi-convert remaining Android host API calls to Pigeon #6980

Uh oh!

stuartmorgan-g commented Jun 24, 2024 •

edited

Loading

Uh oh!

reidbaker Jun 28, 2024

Uh oh!

stuartmorgan-g Jul 1, 2024

Uh oh!

reidbaker Jun 28, 2024

Uh oh!

stuartmorgan-g Jul 1, 2024

Uh oh!

reidbaker Jun 28, 2024

Uh oh!

stuartmorgan-g Jul 1, 2024

Uh oh!

reidbaker Jun 28, 2024

Uh oh!

stuartmorgan-g Jul 1, 2024

Uh oh!

Uh oh!

Uh oh!

[google_maps_flutter] Semi-convert remaining Android host API calls to Pigeon #6980

[google_maps_flutter] Semi-convert remaining Android host API calls to Pigeon #6980

Uh oh!

Conversation

stuartmorgan-g commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g commented Jun 24, 2024 •

edited

Loading