[google_maps_flutter] Support for Ground Overlay #8432

jokerttu · 2025-01-15T07:44:45Z

Adds Ground Overlay support for google_maps_flutter plugin.

Fixes flutter/flutter#26479

Android:

iOS:

Web:

Documentation for groundOverlays parameter on app-facing plugin:

Pre-launch Checklist

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

Co-Authored-By: Ville Välimaa <[email protected]>

…ector.

jokerttu · 2025-01-16T11:28:28Z

...es/google_maps_flutter/google_maps_flutter/example/integration_test/src/tiles_inspector.dart

@@ -22,7 +21,7 @@ void main() {
 }

 void runTests() {
-  const double floatTolerance = 1e-8;
+  const double floatTolerance = 1e-6;


Added bigger tolerance as on Android and iOS tests failed as diff was bigger than 1e-8

jokerttu · 2025-01-16T11:31:14Z

...es/google_maps_flutter/google_maps_flutter/example/integration_test/src/tiles_inspector.dart

-          Color.fromARGB(255, 0, 0, 191),
-          0.6,
+      );
+      await tester.pumpAndSettle(const Duration(seconds: 3));


Without this line tests timeouted on Web platform, and we were not able to test ground overlay below.

Were they timing out in all tests, or wasm specifically? I know I had to revisit a bunch of plugins and add a few pumps and pumpAndSettle when enabling the wasm tests. See:

[ci] Run web tests in wasm (unit + integration). #8111

@ditman
Note that google_maps_flutter tests are currently excluded for web platform because failing tests:
https://github.com/flutter/packages/blob/main/script/configs/exclude_integration_web.yaml

New implemented tests pass with and without wasm enabled, but only if I disable other test sets (that fail because other reasons). The new tests are located in tiles_inspector.dart and I just run this set, which includes tests for tiles, heatmaps, and ground overlays. This file should probably be renamed or split?

Fixing other tests is out of scope of this PR, but this must include working integration tests for web for new features and functionality.

So when only tiles_inspector.dart tests are enabled tests pass with following commands:

Without wasm

dart run ./script/tool/bin/flutter_plugin_tools.dart drive-examples --web --run-chromedriver --packages=google_maps_flutter/google_maps_flutter

and with wasm

dart run ./script/tool/bin/flutter_plugin_tools.dart drive-examples --web --wasm --run-chromedriver --packages=google_maps_flutter/google_maps_flutter

...es/google_maps_flutter/google_maps_flutter/example/integration_test/src/tiles_inspector.dart

jokerttu · 2025-01-16T11:32:16Z

...es/google_maps_flutter/google_maps_flutter/example/integration_test/src/tiles_inspector.dart

-      expect(wll1.weight, wll2.weight);
-      expect(wll1.point.latitude, moreOrLessEquals(wll2.point.latitude));
-      expect(wll1.point.longitude, moreOrLessEquals(wll2.point.longitude));
+  group('Heatmaps', () {


Added Heatmap tests to own group, like tile overlay tests and ground overlay tests.

...ps_flutter_platform_interface/lib/src/platform_interface/google_maps_inspector_platform.dart

stuartmorgan

This looks really good! I did a pass over everything from web, and mostly just have minor comments and questions.

...google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/ground_overlay.dart

stuartmorgan · 2025-01-17T19:25:57Z

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

+}
+
+- (void)didTapGroundOverlayWithIdentifier:(NSString *)identifier {
+  if (!identifier) {


Here and in the next method: the identifier params were annotated as non-nullable in the interface, so the implementation code shouldn't be handling nil.

Good catch. Removed the null check.

It's still here in this method.

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

stuartmorgan · 2025-01-17T19:28:39Z

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

+NS_ASSUME_NONNULL_BEGIN
+
+@interface FGMUtils : NSObject
+ (UIImage *)iconFromBitmap:(FGMPlatformBitmap *)platformBitmap


These new APIs need declaration comments.

stuartmorgan · 2025-01-17T19:34:15Z

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

Can this be given a more specific name? It's already the case that this plugin has at least one other file of utils (the conversion function file).

I renamed it FGMImageUtils.

stuartmorgan · 2025-01-17T19:36:37Z

packages/google_maps_flutter/google_maps_flutter_ios/pigeons/messages.dart

+  final PlatformBitmap image;
+  final PlatformLatLng? position;
+  final PlatformLatLngBounds? bounds;
+  final PlatformDoublePair? anchor;


Isn't this conceptually a point? We already have PlatformPoint, so shouldn't need a new PlatformDoublePair for this unless there's a conceptual difference in meaning here.

Changed this to PlatformPoint.

* refactor: ios groundoverlay improvements * refactor: switch anchor to platformpoint on ios * refactor: refactor global functions * Format ios code --------- Co-authored-by: Joonas Kerttula <[email protected]>

- Add missing NonNull annotations for java - Change zIndex type from double to int for groundOverlays to avoid future issues on iOS platform - Documentation improvements

…lay_support

stuartmorgan

Just some scattered minor issues remaining. Feel free to split out the first sub-PR at this point!

...google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/ground_overlay.dart

stuartmorgan · 2025-01-24T19:56:04Z

...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java

+    if (west <= east) {
+      longitudeOffset = position.longitude - west;
+    } else {
+      longitudeOffset = position.longitude - west;


This is the same in both branches; is that a copy/paste mistake?

If not, it should be factored out of the if/else.

(And if so, that suggests missing test coverage for the behavior in these branches.)

stuartmorgan · 2025-01-27T16:04:18Z

...ogle_maps_flutter_ios/example/ios14/ios/RunnerTests/GoogleMapsGroundOverlayControllerTests.m

+@import GoogleMaps;
+
+#import <OCMock/OCMock.h>
+#import <google_maps_flutter_ios/FGMGroundOverlayController_Test.h>


It should be before all other imports; see https://google.github.io/styleguide/objcguide.html#order-of-includes

stuartmorgan · 2025-01-27T16:05:28Z

...ogle_maps_flutter_ios/example/ios14/ios/RunnerTests/GoogleMapsGroundOverlayControllerTests.m

+
+#import <google_maps_flutter_ios/FGMGroundOverlayController_Test.h>
+
+#import <OCMock/OCMock.h>


This should be grouped with the other framework imports. Grouping is by type of thing import, not by @ vs #.

stuartmorgan · 2025-01-27T16:07:16Z

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

+/// Returns true if a ground overlay with the given identifier exists on the map.
+- (bool)hasGroundOverlaysWithIdentifier:(NSString *)identifier;
+
+/// Returns FGMPlatformGroundOverlay for identifier.


"Returns the ground overlay with the given identifier."

stuartmorgan · 2025-01-27T16:10:55Z

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

+                              registrar:(NSObject<FlutterPluginRegistrar> *)registrar
+                            screenScale:(CGFloat)screenScale {
+  [self setConsumeTapEvents:groundOverlay.clickable];
+  [self setVisible:groundOverlay.visible];


Based on investigation of similar methods in a recent PR (which hasn't landed), this needs to be last to avoid potential flickering (likely due to Google Maps using its own render thread), so should be at the end of the method, and have a comment saying that nothing should be added after it.

stuartmorgan · 2025-01-27T16:13:13Z

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

+}
+
+- (void)didTapGroundOverlayWithIdentifier:(NSString *)identifier {
+  if (!identifier) {


It's still here in this method.

stuartmorgan · 2025-01-27T16:15:31Z

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

+static UIImage *scaleImage(UIImage *image, double scale);
+static UIImage *scaledImageWithScale(UIImage *image, CGFloat scale);
+static UIImage *scaledImageWithSize(UIImage *image, CGSize size);
+static UIImage *scaledImage(UIImage *image, NSNumber *width, NSNumber *height, CGFloat screenScale);


The declaration comments for all of these functions should be here, on the declarations, rather than on the definitions below.

stuartmorgan · 2025-01-27T16:16:13Z

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

+#import "FGMImageUtils.h"
+#import "Foundation/Foundation.h"
+
+static UIImage *scaleImage(UIImage *image, double scale);


This should be scaled, not scale, for consistency in naming.

stuartmorgan · 2025-01-27T16:18:10Z

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

+                                                    NSNumber *zoomLevel) {
+  /// Dummy image is used as image is required field of FGMPlatformGroundOverlay and converting
+  /// image back to bitmap image is not currently supported.
+  FGMPlatformBitmap *mockImage =


This should be called placeholderImage rather than mockImage, as "mock" generally refers to a specific test construct.

github-actions bot added p: google_maps_flutter platform-android platform-ios platform-web labels Jan 15, 2025

jokerttu and others added 13 commits January 16, 2025 12:34

Add ground overlay support to platform interface

473ae27

Co-Authored-By: Ville Välimaa <[email protected]>

Add ground overlay support for Android platform

c5e6d99

Add ground overlay support for iOS platform

57bb929

Co-Authored-By: Ville Välimaa <[email protected]>

Add ground overlay support for Web platform

f298f11

Add ground overlay support to app-facing package

34a6844

Improve example apps

e1a1e10

Make sure GroundOverlay.image is MapBitmap

89380c0

Fix app-facing tests

7e94c23

fix: ios analysis fixes

575eb79

chore: google maps flutter ios formatting

60917d6

test: fix ios tests

c5e2d09

Improve dart tests

ec9ccf0

Remove visibleForTesting annotation from method used in platform insp…

19a7808

…ector.

jokerttu force-pushed the feat/google_maps_flutter_ground_overlay_support branch from c94285d to 19a7808 Compare January 16, 2025 10:34

jokerttu commented Jan 16, 2025

View reviewed changes

...es/google_maps_flutter/google_maps_flutter/example/integration_test/src/tiles_inspector.dart Show resolved Hide resolved

jokerttu commented Jan 16, 2025

View reviewed changes

...ps_flutter_platform_interface/lib/src/platform_interface/google_maps_inspector_platform.dart Show resolved Hide resolved

jokerttu commented Jan 16, 2025

View reviewed changes

...ps_flutter_platform_interface/lib/src/platform_interface/google_maps_inspector_platform.dart Show resolved Hide resolved

jokerttu marked this pull request as ready for review January 16, 2025 11:38

jokerttu requested review from cbracken, reidbaker, ditman and stuartmorgan as code owners January 16, 2025 11:38

reidbaker requested a review from jesswrd January 17, 2025 16:28

stuartmorgan requested changes Jan 17, 2025

View reviewed changes

illuminati1911 and others added 5 commits January 22, 2025 18:40

refactor: ios groundoverlay pr improvements (#13)

8d68727

* refactor: ios groundoverlay improvements * refactor: switch anchor to platformpoint on ios * refactor: refactor global functions * Format ios code --------- Co-authored-by: Joonas Kerttula <[email protected]>

Minor improvements

59c6853

- Add missing NonNull annotations for java - Change zIndex type from double to int for groundOverlays to avoid future issues on iOS platform - Documentation improvements

Clarify ground overlay documentation

d5c7c54

Merge branch 'flutter:main' into feat/google_maps_flutter_ground_over…

3977077

…lay_support

fix: zIndex in android unit tests

9f427b8

jokerttu requested a review from stuartmorgan January 22, 2025 19:40

stuartmorgan requested changes Jan 27, 2025

View reviewed changes

This was referenced Jan 28, 2025

Ground overlay: address PR comments CodemateLtd/packages#16

Open

[google_maps_flutter] Support for Ground Overlay - platform interface #8518

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter] Support for Ground Overlay #8432

[google_maps_flutter] Support for Ground Overlay #8432

jokerttu commented Jan 15, 2025 •

edited

Loading

jokerttu Jan 16, 2025

jokerttu Jan 16, 2025

ditman Jan 16, 2025

jokerttu Jan 17, 2025

jokerttu Jan 16, 2025 •

edited

Loading

stuartmorgan left a comment

stuartmorgan Jan 17, 2025

illuminati1911 Jan 23, 2025

stuartmorgan Jan 27, 2025

stuartmorgan Jan 17, 2025

illuminati1911 Jan 23, 2025

stuartmorgan Jan 17, 2025

illuminati1911 Jan 23, 2025

stuartmorgan Jan 17, 2025

illuminati1911 Jan 23, 2025

stuartmorgan left a comment

stuartmorgan Jan 24, 2025

stuartmorgan Jan 27, 2025

stuartmorgan Jan 27, 2025

stuartmorgan Jan 27, 2025

stuartmorgan Jan 27, 2025

stuartmorgan Jan 27, 2025

stuartmorgan Jan 27, 2025

stuartmorgan Jan 27, 2025

stuartmorgan Jan 27, 2025

stuartmorgan Jan 27, 2025


		#import <google_maps_flutter_ios/FGMGroundOverlayController_Test.h>

		#import <OCMock/OCMock.h>

[google_maps_flutter] Support for Ground Overlay #8432

Are you sure you want to change the base?

[google_maps_flutter] Support for Ground Overlay #8432

Conversation

jokerttu commented Jan 15, 2025 • edited Loading

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokerttu Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokerttu commented Jan 15, 2025 •

edited

Loading

jokerttu Jan 16, 2025 •

edited

Loading