[google_maps_flutter] Converts map configuration and platform view creation params to Pigeon #7207

stuartmorgan-g · 2024-07-24T16:52:43Z

For both Android and iOS, convert the platform view creation parameters to a typed structure using Pigeon, via the ability to set a custom codec for the parameter, instead of a JSON dictionary, as well as converting the map configuration structure used in both those params and updateMapConfiguration.

This change removes the last high-level use of non-Pigeon code in the plugin. The remaining uses of JSON are now internal to individual maps objects, which means that it should now be the case that all future changes to this plugin use Pigeon structures rather than JSON. (The individual objects don't change often, if at all, and it would be reasonable if someone does want to add a new parameter to one of those objects to require converting that specific object, as that would not require sweeping changes or knowledge of how to do a more holistic Pigeon conversion.)

Part of flutter/flutter#117907

Pre-launch Checklist

stuartmorgan-g · 2024-07-24T16:53:42Z

I put both platforms together this time since the scope is smaller than the previous PRs, and the structure of the change is the same between the two, but you can each just review your platform's half.

stuartmorgan-g · 2024-07-24T16:55:24Z

...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/CirclesController.java

    this.googleMap = googleMap;
  }

-  void addJsonCircles(List<Object> circlesToAdd) {


On both platforms, the pure removal of the addJson* methods is because I added the Pigeon version in previous PRs, so this is removing temporarily-quasi-duplicated code.

stuartmorgan-g · 2024-07-24T16:57:20Z

...flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java


  private void updateInitialMarkers() {
-    markersController.addJsonMarkers(initialMarkers);
+    if (initialMarkers != null) {


These new null checks are because addJson* was internally no-oping for nil, but the typed versions don't do that because I think it's cleaner to forbid calling add* than to silently no-op internally.

stuartmorgan-g · 2024-07-24T16:58:42Z

...droid/android/src/test/java/io/flutter/plugins/googlemaps/ClusterManagersControllerTest.java


  @Before
  public void setUp() {
-    MockitoAnnotations.openMocks(this);


Opportunistic cleanup; there were warnings about the fact that we weren't cleaning this up, and when I went to fix that I noticed that there were no mockito-annotated ivars in this test class.

stuartmorgan-g · 2024-07-24T16:59:07Z

...le_maps_flutter_android/android/src/test/java/io/flutter/plugins/googlemaps/ConvertTest.java

  }

+  @Test
+  public void interpretMapConfiguration_handlesNulls() {


New tests because that entire method was untested before, which didn't seem great.

stuartmorgan-g · 2024-07-24T17:31:34Z

...ges/google_maps_flutter/google_maps_flutter_android/lib/src/google_maps_flutter_android.dart

+    Map<String, Object?> options) {
+  // All of these hard-coded values and structures come from
+  // google_maps_flutter_platform_interface/lib/src/types/utils/map_configuration_serialization.dart
+  // to support this legacy API that relied on cross-package magic strings.


(This applies to Android and iOS.)

This method is gross in that it uses magic strings from another package, but the good news is that it is only in the codepath for a platform interface API that we don't use in recent versions of the app-facing package. At some point we could just decide to do a breaking change to the implementation packages and remove this, without needing to do a breaking change to the app-facing or platform interface packages. I didn't want to do that in this PR though as it requires some extra steps.

(And while this looks like new gross code, it isn't conceptually: it's just moving from native to Dart, and moving from every codepath to a codepath that will in practice almost never be used.)

Thank you for adding a link to where the magic strings come from

stuartmorgan-g · 2024-07-24T17:35:47Z

...ges/google_maps_flutter/google_maps_flutter_android/lib/src/google_maps_flutter_android.dart

+      (zoomPrefsJson as List<Object?>).cast<double?>();
+  return PlatformZoomRange(min: minMaxZoom[0], max: minMaxZoom[1]);
+}
+


(This applies to Android and iOS.)

The reason there's a bunch of added conversion code without any corresponding removal of JSON versions is that the JSON serialization code is in google_maps_flutter_platform_interface. That's ugly technical debt, because having the serialization and deserialization in different packages is a bad idea, and means the entire JSON surface is effective part of the public API surface of the platform interface package, and we therefore can't do anything to that code other than add things to dictionaries.

So it looks like it's adding complexity, but is actually reducing technical debt by moving the serialization into the same package as the deserialization where it belongs. (There's actually a TODO about this at the top of the file.)

stuartmorgan-g · 2024-07-24T17:36:50Z

...flutter/google_maps_flutter_ios/example/shared/maps_example_dart/lib/example_google_map.dart

    this.onMapCreated,
    this.gestureRecognizers = const <Factory<OneSequenceGestureRecognizer>>{},
    this.compassEnabled = true,
-    this.mapToolbarEnabled = true,


These changes to the iOS example are opportunistic cleanup; these have always been Android-only, and just got copypasta'd here when I federated the implementations out of the original shared implementation.

stuartmorgan-g · 2024-07-25T14:08:29Z

I'm not sure what's going on with that Android test; there's not much info in FTL output, and I haven't been able to repro locally. I'll keep investigating that in parallel with review.

stuartmorgan-g · 2024-07-30T18:12:20Z

there's not much info in FTL output

Turns out there was, it was just buried in all the other log output.

It found a real bug where I missed a change I needed to make in Convert.java (which the compiler didn't flag because it's code casts Object to other things; the usual problem of dealing with pre-Pigeon code) and I had missed that branch when adding unit tests. Pushed a fix and a unit test that exercised that specific section of code.

reidbaker

Android LGTM

reidbaker · 2024-07-30T18:51:35Z

...ges/google_maps_flutter/google_maps_flutter_android/lib/src/google_maps_flutter_android.dart

+    Map<String, Object?> options) {
+  // All of these hard-coded values and structures come from
+  // google_maps_flutter_platform_interface/lib/src/types/utils/map_configuration_serialization.dart
+  // to support this legacy API that relied on cross-package magic strings.


Thank you for adding a link to where the magic strings come from

… view creation params to Pigeon (flutter/packages#7207)

flutter/packages@5cc0a01...bb797b9 2024-08-08 [email protected] [google_maps_flutter] Converts map configuration and platform view creation params to Pigeon (flutter/packages#7207) 2024-08-08 [email protected] [flutter_markdown] Fix table column alignment (flutter/packages#7327) 2024-08-08 [email protected] [interactive_media_ads] Fix names of unit tests (flutter/packages#7336) 2024-08-07 [email protected] [url_launcher] Convert Linux to Pigeon (flutter/packages#7215) 2024-08-07 [email protected] Roll Flutter from 0a7f8af to d595e98 (4 revisions) (flutter/packages#7334) 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

stuartmorgan-g added 6 commits July 22, 2024 10:54

iOS Dart

64c78ec

Convert iOS native

f88eb97

Add Android native unit tests

46e0805

Convert platform view creation params to Pigeon on Android

72afd22

Convert platform view creation params to Pigeon on iOS

e8a5e8d

Version bumps

bb40332

stuartmorgan-g requested review from cbracken and reidbaker as code owners July 24, 2024 16:52

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

stuartmorgan-g added triage-ios Should be looked at in iOS triage triage-android Should be looked at in Android triage labels Jul 24, 2024

stuartmorgan-g commented Jul 24, 2024

View reviewed changes

stuartmorgan-g mentioned this pull request Jul 24, 2024

[google_maps_flutter] Convert to Pigeon flutter/flutter#117907

Open

26 tasks

stuartmorgan-g added 3 commits July 24, 2024 14:14

Fix Dart unit tests

085864b

Fix doc comment

6c032cc

Update iOS 15 tests too

df2b709

stuartmorgan-g added 3 commits July 30, 2024 13:47

Merge branch 'main' into maps-pigeon-options-structure

b1dd015

New unit test, minor test file cleanup

6954c4a

Fix bug

d5cde49

reidbaker approved these changes Jul 30, 2024

View reviewed changes

stuartmorgan-g mentioned this pull request Aug 8, 2024

[google_maps_flutter_android] Convert Circle and Marker to Pigeon #7326

Merged

11 tasks

stuartmorgan-g added 4 commits August 8, 2024 11:28

Merge branch 'main' into maps-pigeon-options-structure

6bbb2fa

Remove obsolete test file

fd0f0f4

iOS 15 native tests

db7fbcf

Typo

11f4ecf

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

auto-submit bot merged commit bb797b9 into flutter:main Aug 8, 2024

engine-flutter-autoroll mentioned this pull request Aug 8, 2024

Roll Packages from 5cc0a0160194 to bb797b9338c8 (5 revisions) flutter/flutter#153107

Merged

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

bb797b9 [google_maps_flutter] Converts map configuration and platform…

dd27d5c

… view creation params to Pigeon (flutter/packages#7207)

[google_maps_flutter] Converts map configuration and platform view creation params to Pigeon #7207

[google_maps_flutter] Converts map configuration and platform view creation params to Pigeon #7207

Uh oh!

Conversation

stuartmorgan-g commented Jul 24, 2024

Pre-launch Checklist

Uh oh!

stuartmorgan-g commented Jul 24, 2024

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!

stuartmorgan-g commented Jul 25, 2024

Uh oh!

stuartmorgan-g commented Jul 30, 2024

Uh oh!

reidbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants