[google_maps_flutter] Use structured Pigeon data on iOS #8142

stuartmorgan-g · 2024-11-21T16:30:55Z

Converts most of the maps objects that were still using pre-Pigeon JSON serializations to use structured Pigeon classes instead, mirroring the recent work in the Android implementation:

Copies the Pigeon definitions from Android, with minor modifications.
Copies the Dart conversion code and tests from Android, with minor modifications.
Updates the native code to eliminate a lot of JSON boilerplate.

In addition to adding type safety, this mostly finishes addressing technical debt dating back to the initial federation of the plugin, where native code in the implementation package is relying on the JSON serialization of objects that is implemented in the platform interface package, meaning that the stringly-typed data had to match across packages in addition to languages.

This also backports some Dart unit test improvements to the Android implementation. While bringing the test code over I noticed that the expectations were based on running the Pigeon serialization and then asserting things about the results, which I missed in review. That approach is very fragile because the Pigeon serialization is an internal implementation detail of Pigeon and subject to change at any time. Instead the tests should be using the object directly.

Part of flutter/flutter#117907

Pre-launch Checklist

stuartmorgan-g · 2024-11-21T16:33:14Z

packages/google_maps_flutter/google_maps_flutter_ios/test/google_maps_flutter_ios_test.dart

@@ -537,6 +760,233 @@ void main() {
    expect((await stream.next).value.value, equals(objectId));
  });

+  test('moveCamera calls through with expected newCameraPosition', () async {


There didn't used to be any camera update Dart unit testing; it was added during the Android update, and I brought them over here.

stuartmorgan-g · 2024-11-21T16:34:15Z

...aps_flutter/google_maps_flutter_ios/example/ios14/ios/RunnerTests/ExtractIconFromDataTests.m

-  XCTAssertEqual(resultImage.size.width, 15.0);
-  XCTAssertEqual(resultImage.size.height, 45.0);
+  XCTAssertEqual(resultImage.size.width, width);
+  XCTAssertEqual(resultImage.size.height, height);


I didn't do it everywhere since it would have been a pretty deep rabbit hole, but I did a bit of opportunistic updating from "tests use repeated magic values" to "tests use named constants for things that should match in multiple places".

Commented above before I got to this comment, but thank you. Much nicer.

stuartmorgan-g · 2024-11-21T16:35:07Z

...er/google_maps_flutter_ios/example/ios14/ios/RunnerTests/FGMClusterManagersControllerTests.m

+                                                        visible:YES
+                                                         zIndex:1
+                                                       markerId:markerId2
+                                               clusterManagerId:clusterManagerId];


Some of the test setup (not just in this file) got a lot more verbose because they used to rely on the code tolerating missing JSON entries that weren't really supposed to be missing.

stuartmorgan-g · 2024-11-21T16:36:58Z

...r/google_maps_flutter_ios/example/ios14/ios/RunnerTests/GoogleMapsPolylinesControllerTests.m

@@ -65,17 +79,4 @@ - (void)testSetPatterns {
  XCTAssertNotNil(polylineController.polyline.spans);
 }

- (void)testStrokeStylesFromPatterns {


This test moved to the (increasingly poorly named; we can rename it as the final step after heatmaps are converted) JSON conversion test file, since that's where the code it is testing lives. I'm not sure why it was here, especially since it's companion length-extraction-method test was already there.

stuartmorgan-g · 2024-11-21T16:39:07Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapJSONConversions.h

-+ (NSArray<NSArray<CLLocation *> *> *)holesFromPointsArray:(NSArray *)data;
-+ (nullable GMSCameraPosition *)cameraPostionFromDictionary:(nullable NSDictionary *)channelValue;
-+ (GMSCoordinateBounds *)coordinateBoundsFromLatLongs:(NSArray *)latlongs;
-+ (nullable GMSCameraUpdate *)cameraUpdateFromArray:(NSArray *)channelValue;


The reason these are moved instead of just renamed is that I'm chipping away at the functions-as-bag-of-static-methods-on-a-fake-class approach that the JSON conversion methods used, to the IMO-cleaner functions-as-functions approach. Once the conversion is complete this class will be gone.

stuartmorgan-g · 2024-11-21T16:39:58Z

.../google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapTileOverlayController.m

@@ -224,8 +203,4 @@ - (nullable FLTGoogleMapTileOverlayController *)tileOverlayWithIdentifier:(NSStr
  return self.tileOverlayIdentifierToController[identifier];
 }

-+ (NSString *)identifierForTileOverlay:(NSDictionary *)tileOverlay {


I got rid of these helpers in a bunch of files because with structured classes they became trivial.

stuartmorgan-g · 2024-11-21T16:41:41Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapPolygonController.m

-  NSArray *pointArray = polygon[@"points"];
-  NSArray<CLLocation *> *points = [FLTGoogleMapJSONConversions pointsFromLatLongs:pointArray];
-  GMSMutablePath *path = [GMSMutablePath path];
-  for (CLLocation *location in points) {


This array conversion logic was duplicated in several places, so I consolidated it to the new FGMGetPathFromPoints.

stuartmorgan-g · 2024-11-21T16:42:08Z

...s/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapPolylineController_Test.h

-///
-/// @param polyline The polyline instance for which path is calculated.
-/// @return An instance of GMSMutablePath.
-+ (GMSMutablePath *)pathForPolyline:(NSDictionary *)polyline;


This is one of the things replaced by FGMGetPathFromPoints, so is tested that way instead.

cbracken · 2024-11-21T16:53:29Z

...aps_flutter/google_maps_flutter_ios/example/ios14/ios/RunnerTests/ExtractIconFromDataTests.m

-    @"imagePixelRatio" : @1,
-    @"width" : @15.0
-  };  // Target height
+  const CGFloat width = 15.0;


cbracken · 2024-11-21T16:57:13Z

..._maps_flutter_ios/example/ios14/ios/RunnerTests/FLTGoogleMapJSONConversionsConversionTests.m

+    [FGMPlatformPatternItem makeWithType:FGMPlatformPatternItemTypeGap length:@(1)],
+    [FGMPlatformPatternItem makeWithType:FGMPlatformPatternItemTypeDash length:@(1)]
+  ];
+  UIColor *strokeColor = [UIColor redColor];


Totally optional: Opportunistic UIColor.redColor here? And maybe patternStrokeStyle.count below? 😉

Done. I also found all the other UIColor fixed colors and updated them to the new style.

cbracken · 2024-11-21T17:00:48Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapJSONConversions.m

+  return [UIColor colorWithRed:((float)((rgba & 0xFF0000) >> 16)) / 255.0
+                         green:((float)((rgba & 0xFF00) >> 8)) / 255.0
+                          blue:((float)(rgba & 0xFF)) / 255.0
+                         alpha:((float)((rgba & 0xFF000000) >> 24)) / 255.0];


I realise this is just moving but... technically (CGFloat) though in the end entirely the same thing.

Fixed. It's technically not the same since CGFloat is double on all supported devices now, so we were pointlessly losing some precision (although probably not enough to actually matter in a color).

cbracken · 2024-11-21T17:01:57Z

.../google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapTileOverlayController.m

  self = [super init];
  if (self) {
    _layer = tileLayer;
    _mapView = mapView;
-    [self interpretTileOverlayOptions:optionsData];
+    // TODO(stuartmorgan: Refactor to avoid this call to an instance method in init.


cbracken · 2024-11-21T17:06:05Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapMarkerController.m

  NSAssert(screenScale > 0, @"Screen scale must be greater than 0");
+  // See comment in messages.dart for why this is so loosely typed.


Maybe add the issue link for searchability? flutter/flutter#117819

cbracken · 2024-11-21T17:08:48Z

packages/google_maps_flutter/google_maps_flutter_ios/lib/src/google_maps_flutter_ios.dart

+    }
+  }
+
+  /// Convert [MapBitmapScaling] from platform interface to [PlatformMapBitmapScaling] Pigeon.


Here and (a couple times, but not all times) below: ultra pedantic nit:

Suggested change

/// Convert [MapBitmapScaling] from platform interface to [PlatformMapBitmapScaling] Pigeon.

/// Converts [MapBitmapScaling] from platform interface to [PlatformMapBitmapScaling] Pigeon.

https://dart.dev/effective-dart/documentation#prefer-starting-function-or-method-comments-with-third-person-verbs

It's a nit I usually leave, so I'm happy to fix it :) I didn't notice when I pasted these in from Android.

cbracken · 2024-11-21T17:12:34Z

packages/google_maps_flutter/google_maps_flutter_ios/pigeons/messages.dart

+  /// camera update, and each holds a different set of data, preventing the
+  /// use of a single unified class.
+  // Pigeon does not support inheritance, which prevents a more strict type
+  // bound. See https://github.com/flutter/flutter/issues/117819.


I'll take this TODO over the one above any day. 🙂

cbracken

LGTM modulo some pedantic nits. Much cleaner!

* main: (64 commits) [quick_actions_plaform_interface] add localizedSubtitle (flutter#8112) [tools] Don't check license of generated Swift package (flutter#8137) Roll Flutter from 8536b96 to 93d772c (37 revisions) (flutter#8147) [go_router] Fix: Consistent PopScope Handling on Root Routes issue #140869 (flutter#8045) [in_app_purchase_storekit] fix price displayed with wrong precision (flutter#8127) [pigeon] Removes the `@protected` annotation from the InstanceManager field of the `PigeonInternalProxyApiBaseClass` (flutter#8125) [google_maps_flutter] Use structured Pigeon data on iOS (flutter#8142) [vector_graphics] handle errors from bytes loader (flutter#8080) [flutter_svg] Fix SvgNetworkLoader not closing internal http client (flutter#8126) [video_player_avfoundation] send video load failure even when eventsink was initialized late (flutter#7194) [flutter_markdown] enable Wasm support (flutter#8120) Reverts "[url_launcher] Add Swift Package Manager integration to example app (flutter#8128)" (flutter#8136) [url_launcher] Add Swift Package Manager integration to example app (flutter#8128) [pigeon] Enable example app build in CI (flutter#8119) [in_app_purchase_storekit] disallow ios versions lower than supported from enabling storekit (flutter#8110) [interactive_media_ads]: Bump com.google.ads.interactivemedia.v3:interactivemedia from 3.35.1 to 3.36.0 in /packages/interactive_media_ads/android (flutter#8046) [interactive_media_ads]: Bump androidx.annotation:annotation from 1.8.2 to 1.9.1 in /packages/interactive_media_ads/android (flutter#7980) [webview_flutter_android] Updates plugin to use `ProxyApis`s (flutter#7794) [interactive_media_ads] Adds support to define parameters that control the rendering of ads (flutter#8057) Roll Flutter from b3818f6 to 8536b96 (22 revisions) (flutter#8124) ... # Conflicts: # packages/quick_actions/quick_actions_platform_interface/CHANGELOG.md

…utter/packages#8142)

flutter/packages@913b99e...9203213 2024-11-22 [email protected] Reland "[url_launcher] Add Swift Package Manager integration to example app" (flutter/packages#8148) 2024-11-22 [email protected] [webview_flutter_wkwebview] Webkit webview controller multiple registration fix (flutter/packages#8078) 2024-11-22 [email protected] [quick_actions_plaform_interface] add localizedSubtitle (flutter/packages#8112) 2024-11-22 [email protected] [tools] Don't check license of generated Swift package (flutter/packages#8137) 2024-11-21 [email protected] Roll Flutter from 8536b96 to 93d772c (37 revisions) (flutter/packages#8147) 2024-11-21 [email protected] [go_router] Fix: Consistent PopScope Handling on Root Routes issue #140869 (flutter/packages#8045) 2024-11-21 [email protected] [in_app_purchase_storekit] fix price displayed with wrong precision (flutter/packages#8127) 2024-11-21 [email protected] [pigeon] Removes the `@protected` annotation from the InstanceManager field of the `PigeonInternalProxyApiBaseClass` (flutter/packages#8125) 2024-11-21 [email protected] [google_maps_flutter] Use structured Pigeon data on iOS (flutter/packages#8142) 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] 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

Fixes a regression introduced during the migration to structured Pigeon data (#8142), where the info window data was not passed through to the underlying SDK marker object. This wasn't caught because there was apparently no native unit testing of marker properties, and this level of info window detail isn't inspected in integration tests. Adds missing tests, and backfills tests for other marker properties as well. Fixes flutter/flutter#159471 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.

Fixes a regression introduced during the migration to structured Pigeon data (flutter#8142), where the info window data was not passed through to the underlying SDK marker object. This wasn't caught because there was apparently no native unit testing of marker properties, and this level of info window detail isn't inspected in integration tests. Adds missing tests, and backfills tests for other marker properties as well. Fixes flutter/flutter#159471 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.

Fixes a regression introduced during the migration to structured Pigeon data (flutter/packages#8142), where the info window data was not passed through to the underlying SDK marker object. This wasn't caught because there was apparently no native unit testing of marker properties, and this level of info window detail isn't inspected in integration tests. Adds missing tests, and backfills tests for other marker properties as well. Fixes flutter/flutter#159471 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.

stuartmorgan-g added 11 commits November 13, 2024 14:26

WIP for cameraUpdate, updating native tests

f3b73b0

Update Pigeon definition to (mostly) match Android

1fe07c9

Update Dart code

ac98522

Update Dart unit tests from Android version

5e88615

Remove all the test dependencies on Pigeon encoding format

58aae26

Port test improvements back to Android

9c6981d

WP - Update native code

2b04e31

Native code updates

73d9e15

Revert unrelated project changes

f212967

Version bump

87711b6

Revert iOS 15 project too

27a3625

stuartmorgan-g requested review from cbracken and reidbaker as code owners November 21, 2024 16:30

github-actions bot added p: google_maps_flutter platform-android platform-ios labels Nov 21, 2024

stuartmorgan-g commented Nov 21, 2024

View reviewed changes

stuartmorgan-g added 3 commits November 21, 2024 11:48

Remove cap, which iOS doesn't use

8cdd24f

Remove Android SDK references

416bab0

Doc lint fix

83721b7

cbracken reviewed Nov 21, 2024

View reviewed changes

Warning fixes, cap test cleanup

7fc67d1

cbracken reviewed Nov 21, 2024

View reviewed changes

cbracken approved these changes Nov 21, 2024

View reviewed changes

Review comments

96ccac0

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

auto-submit bot merged commit c55e398 into flutter:main Nov 21, 2024
77 checks passed

engine-flutter-autoroll mentioned this pull request Nov 22, 2024

Roll Packages from 913b99ed8542 to 761812665448 (7 revisions) flutter/flutter#159330

Closed

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

c55e398da [google_maps_flutter] Use structured Pigeon data on iOS (fl…

162b4f4

…utter/packages#8142)

engine-flutter-autoroll mentioned this pull request Nov 22, 2024

Roll Packages from 913b99ed8542 to 920321356e45 (9 revisions) flutter/flutter#159338

Merged

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

c55e398da [google_maps_flutter] Use structured Pigeon data on iOS (fl…

d51a6f3

…utter/packages#8142)

stuartmorgan-g mentioned this pull request Mar 31, 2025

[google_maps_flutter] Fix iOS info window regression #8939

Merged

11 tasks

		NSAssert(screenScale > 0, @"Screen scale must be greater than 0");
		// See comment in messages.dart for why this is so loosely typed.

	/// Convert [MapBitmapScaling] from platform interface to [PlatformMapBitmapScaling] Pigeon.
	/// Converts [MapBitmapScaling] from platform interface to [PlatformMapBitmapScaling] Pigeon.

[google_maps_flutter] Use structured Pigeon data on iOS #8142

[google_maps_flutter] Use structured Pigeon data on iOS #8142

Uh oh!

Conversation

stuartmorgan-g commented Nov 21, 2024

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cbracken Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cbracken Nov 21, 2024 •

edited

Loading

cbracken Nov 21, 2024 •

edited

Loading