[google_maps_flutter] Add `style` to widget #6192

stuartmorgan · 2024-02-23T18:16:44Z

Adds style to the GoogleMap widget construction, and internally to the MapConfiguration structure, wiring updates to style through the existing configuration update system instead of needing to be set via a separate channel. This makes things more consistent (style is conceptually similar to many of the other configuration options, and is not something that's likely to require frequent dynamic updates), and more importantly allows setting the style as part of creation instead of asynchronously after the map has likely already been displayed.

Because of flutter/flutter#24327, this adds a way to query for style errors after the fact. (It's possible that it's why setMapStyle was done that way in the first place; the initial PR doesn't explain.) While this is slightly clunky, it's unlikely that this method would actually be used in production code anyway, since presumably people are defining their styles at build time, not runtime, and thus probably don't need runtime error handling. Long term, the solution to this problem is likely to invert the widget/controller relationship, as was done in webview_flutter, but that's a major breaking change and not something we're ready to take on at this point.

Since we don't really want two recommended ways of doing the same thing, this deprecates the existing setMapStyle method in favor of the new approach.

Fixes flutter/flutter#66207

Pre-launch Checklist

hellohuanlin

only reviewed the ios part

hellohuanlin · 2024-02-23T19:01:12Z

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

@@ -506,7 +513,7 @@ - (void)setMyLocationButtonEnabled:(BOOL)enabled {
 }

 - (NSString *)setMapStyle:(NSString *)mapStyle {
-  if (mapStyle == (id)[NSNull null] || mapStyle.length == 0) {
+  if (mapStyle.length == 0) {
    self.mapView.mapStyle = nil;


should we clear the error here?

The caller is responsible for using the return value to update the error state.

if the caller successfully set the style (to be nil), shouldn't we mark our internal state as no error? (The error ivar is used to track the error happened in previous setStyle call right?)

I don't understand what code specifically you are referring to as "we" and "our" in that statement, so I don't know how to answer.

(The error ivar is used to track the error happened in previous setStyle call right?)

The same code that set it previously would clear it. I'm not sure what the call sequence you are concerned about here is exactly.

Oh nvm. I was thinking about this

if (mapStyle.length == 0) { self.mapView.mapStyle = nil; self.styleError = nil; }

But I overlooked that the caller already sets it using the returned value.

hellohuanlin · 2024-02-23T19:01:40Z

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

@@ -658,6 +665,10 @@ - (void)interpretMapOptions:(NSDictionary *)data {
  if (myLocationButtonEnabled && myLocationButtonEnabled != (id)[NSNull null]) {
    [self setMyLocationButtonEnabled:[myLocationButtonEnabled boolValue]];
  }
+  NSString *style = data[@"style"];
+  if (style) {


will this be NSNull?

No, the corresponding JSON serialization code on the Dart side is:

if (config.style != null) 'style': config.style!,

Now that we are using in-package method channels everywhere, meaning that method channels in our plugins now map directly from Dart to a single implementation, I don't think there's much value in doing defensive coding against potential changes in Dart, because the only reason someone would change that Dart code would be to change this one other endpoint at the same time (unlike the old days, when someone might change the Dart code to do something for Android, not realizing that it would break iOS because they didn't even look).

stuartmorgan · 2024-02-23T19:41:33Z

@ditman Any idea what those web failures mean? It looks like the driver itself failed rather than a specific test, but it happened to both master and stable which suggest that it's my PR.

reidbaker · 2024-02-23T20:34:23Z

packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart

+  ///
+  /// The style string can be generated using the
+  /// [map style tool](https://mapstyle.withgoogle.com/).
+  final String? style;


Is there any way at all to make this a type safe object?

If not can we at least point users to https://developers.google.com/maps/documentation/cloud-customization which does not require app updates and a stringly typed api.

Is there any way at all to make this a type safe object?

Not in any useful way. The native objects are opaque objects created from a string that is supposed to be a JSON string. So the options are:

Make a do-nothing MapStyle object that just wraps a String, but in practice that would almost certainly just convert every call from style: someString to style: MapStyle(someString)

Pre-parse the JSON on the Dart side, only to convert it back to a string and then have the Maps SDK re-parse it anyway.

If not can we at least point users to https://developers.google.com/maps/documentation/cloud-customization which does not require app updates and a stringly typed api.

If you follow the link there in this comment for generating the JSON style, you'll see that we don't really need to do that at the plugin level, as the SDK's docs and tool are very aggressive about this. (Also, there are significant caveats, like cloud style requires a per-load charge, and doesn't work on all Android devices, which is more than I want to get into in the comments here. I'd rather err on the side of being a neutral wrapper of the SDKs, and let the SDK docs steer people.)

It can definitely be made a type-safe object (we have a "type-safe-ish version" in the web version); the value of the String version of this property is that it can be generated/tweaked (and then copy-pasted) from here: https://mapstyle.withgoogle.com (and probably others)

HOWEVER, one thing that Google Maps SDK seems to be doing is to call this feature "legacy styling", so maybe this property could be called: legacyStyle and in the documentation point them to the link that you're proposing? That way people can learn about cloud styling, the mapId property and the new way (and cost) of doing things?

There's even documentation on how to migrate from this legacyStyle value to the new cloud-based one: https://developers.google.com/maps/documentation/javascript/cloud-customization/map-styles#import-json-styling

It can definitely be made a type-safe object

Sorry, technically there is a third option: we could—in our wrapper that is already under-resourced—reverse-engineer a complex object graph from docs, thus building and maintaining a much more complex API than what the SDKs provide, creating a ton of work for us, only to throw away all that structure and re-encode it as a string anyway at the SDK boundary. And thereby make the existing tools for these styles harder for our clients to use. And also being inconsistent with our setMapStyle, making it significantly harder to migrate to this new API.

It's theoretically possible, but I don't see any actual upsides.

HOWEVER, one thing that Google Maps SDK seems to be doing is to call this feature "legacy styling", so maybe this property could be called: legacyStyle

Their high-level docs call it legacy style. The actual SDKs, and the code-level API docs for those SDKs, do not use that language anywhere that I have seen.

I do not want to be in the business of being significantly more opinionated in the code about what APIs people should or should not use than the people actually maintaining the SDK we are wrapping.

I also don't want setMapStyle (the existing method) and legacyStyle (the new, preferred approach), since that will make the deprecated one seem preferable and almost certainly confuse everyone.

SGTM, legacyStyle seems to be only mentioned in the Cloud Styling docs.

reidbaker · 2024-02-23T20:38:31Z

packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart

  Future<void> setMapStyle(String? mapStyle) {
    return GoogleMapsFlutterPlatform.instance
        .setMapStyle(mapStyle, mapId: mapId);
  }

+  /// Returns the last style error, if any.


Should this say that it will get errors for the current mapid only?

MapController is a per-map object; it's created with this ID.

ditman

I like this change! Maybe consider calling the new property legacyStyle to follow the naming used in the maps sdk documentation? https://developers.google.com/maps/documentation/javascript/cloud-customization/legacy-overview

(I don't know if this is referred to as "legacy" in other platforms, though)

((PS: Docs seem to call this "JSON Styling", and only refer to it as "legacy" in the Cloud Customization section))

stuartmorgan · 2024-02-23T21:24:02Z

I'll split out the first sub-PR. Meanwhile, @ditman any thoughts on my question above about the web integration test failure?

ditman · 2024-02-23T21:38:45Z

@ditman any thoughts on my question above about the web integration test failure?

Nope, let me clone your branch and attempt to step through the test, ~~(maybe an unexpected null somewhere?)~~

Ran the test locally; from our tooling PoV, the test never finishes (successfully or with errors), and we get a timeout.

When the test is run locally, we get the following:

00:00 +0: testInitialCenterLocationAtCenter
dart_sdk.js:40468 00:00 +0 ~1: testGetVisibleRegion
dart_sdk.js:40468 00:05 +1 ~1: testSetMapStyle valid Json String
dart_sdk.js:40468 00:06 +2 ~1: testSetMapStyle invalid Json String
dart_sdk.js:40468 00:06 +3 ~1: testSetMapStyle null string
dart_sdk.js:40468 00:07 +4 ~1: testGetLatLng
dart_sdk.js:40468 00:09 +5 ~1: testGetZoomLevel
dart_sdk.js:40468 00:10 +6 ~1: testScreenCoordinate
dart_sdk.js:40468 00:11 +6 ~1: testGetVisibleRegion
dart_sdk.js:40468 ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
dart_sdk.js:40468 The following StateError was thrown running a test (but after the test had completed):
dart_sdk.js:40468 Bad state: Cannot add new events after calling close
dart_sdk.js:40468 
dart_sdk.js:40468 When the exception was thrown, this was the stack:
dart_sdk.js:40468 dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 328:3  throw_
dart_sdk.js:40468 dart-sdk/lib/async/broadcast_stream_controller.dart 243:24                   add
dart_sdk.js:40468 packages/google_maps_flutter_web/src/types.dart.js 740:33                    <fn>
dart_sdk.js:40468 dart-sdk/lib/async/zone.dart 1415:13                                         _rootRunUnary
dart_sdk.js:40468 dart-sdk/lib/async/zone.dart 1308:19                                         runUnary
dart_sdk.js:40468 dart-sdk/lib/async/zone.dart 1217:7                                          runUnaryGuarded
dart_sdk.js:40468 dart-sdk/lib/async/stream_impl.dart 339:5                                    [_sendData]
dart_sdk.js:40468 dart-sdk/lib/async/stream_impl.dart 515:13                                   perform
dart_sdk.js:40468 dart-sdk/lib/async/stream_impl.dart 620:10                                   handleNext
dart_sdk.js:40468 dart-sdk/lib/async/stream_impl.dart 591:7                                    callback
dart_sdk.js:40468 dart-sdk/lib/async/schedule_microtask.dart 40:11                             _microtaskLoop
dart_sdk.js:40468 dart-sdk/lib/async/schedule_microtask.dart 49:5                              _startMicrotaskLoop
dart_sdk.js:40468 dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 181:7           <fn>
dart_sdk.js:40468 ════════════════════════════════════════════════════════════════════════════════════════════════════
dart_sdk.js:40468 00:12 +7 ~1: testResizeWidget

dart_sdk.js:40468 00:14 +8 ~1: testToggleInfoWindow
js?key=xxxx&libraries=geometry:176 As of February 21st, 2024, google.maps.Marker is deprecated. Please use google.maps.marker.AdvancedMarkerElement instead. Please see https://developers.google.com/maps/deprecations for deprecation details.

dart_sdk.js:40468 00:16 +9 ~1: testTakeSnapshot

dart_sdk.js:40468 00:16 +9 ~2: testCloudMapId
js?key=xxxx&libraries=geometry:341 Google Maps JavaScript API: A Map's styles property cannot be set when a mapId is present. When a mapId is present, Map styles are controlled via the cloud console. Please see documentation at https://developers.google.com/maps/documentation/javascript/styling#cloud_tooling

dart_sdk.js:40468 00:16 +10 ~2: getStyleError reports last error

I'm going to see what's the test that's messing with the suite next, and find a solution.

ditman · 2024-02-24T01:41:29Z

I've pushed a small fix to the test, it just needed to use the pumpMap helper.

I introduced that helper when we endorsed the package (and integration_tests started to attempt to run on the web). The web platform needs a little bit more "scaffolding" (literally) on the app under testing than mobile to properly render a functional Map widget.

Here's the PR where pumpMap was originally added:

[google_maps] Prepares packages to endorse web. #4064

(The test should pass in CI now, at least it does locally)

(Also: I just noticed that the FIRST test of that file doesn't use the helper and does a skip: web, can't remember exactly what's up with that :S)

stuartmorgan · 2024-02-26T12:39:01Z

Awesome, thanks! I think I wrote the test in Android or iOS and then duplicated it to there, not realizing it used a different structure.

Platform interface portion of #6192 Adds `style` to `MapConfiguration` and adds new `getStyleError` method. Part of Fixes flutter/flutter#66207

…itial-style

Platform implementations portion of #6192 Adds handling of `MapConfiguration.style` and implements new `getStyleError` method. Part of flutter/flutter#66207

stuartmorgan · 2024-02-28T17:39:13Z

I'm very confused by this error on stable:

Compiling lib/main.dart for the Web...                          

Target dart2js failed: ProcessException: Process exited abnormally with exit code 253:
../../../../../.pub-cache/hosted/pub.dev/google_maps_flutter_web-0.5.6/lib/src/map_styler.dart:15:20:
Internal Error: Unknown external method j:method(MapStyler|constructor#)
  external factory MapStyler({
                   ^
The compiler is broken.

I don't see how this would be showing up here, but not #6205.

stuartmorgan · 2024-02-28T17:44:11Z

Oh, it looks like this is related to #5254 rather than my PR, given discussion like dart-lang/sdk#55039

stuartmorgan · 2024-02-28T17:47:56Z

Hopefully this will go away once #6216 lands and publishes.

stuartmorgan · 2024-02-28T20:04:36Z

That did indeed fix it. This is now just the app-facing bits, with the update constraints, so landing.

…#6192)

flutter/packages@353086c...6d02f03 2024-02-28 [email protected] Manual roll Flutter from c30f998 to d00bfe8 (32 revisions) (flutter/packages#6222) 2024-02-28 [email protected] [google_maps_flutter] Add `style` to widget (flutter/packages#6192) 2024-02-28 [email protected] Add library annotations for js interop (flutter/packages#6216) 2024-02-28 [email protected] [google_map_flutter] Add style to widget - platform impls (flutter/packages#6205) 2024-02-28 [email protected] [google_maps_flutter_web] Migrate to package:web (flutter/packages#5254) 2024-02-28 [email protected] [pigeon] Remove heap allocation in generated C++ code (flutter/packages#6196) 2024-02-27 [email protected] [pigeon] Allows kotlin generator to skip error class generation (flutter/packages#6183) 2024-02-27 [email protected] [camerax] Implements `setExposureMode` (flutter/packages#6110) 2024-02-27 [email protected] Roll Flutter from b77560e to c30f998 (12 revisions) (flutter/packages#6211) 2024-02-26 [email protected] Add `InkResponse`, `Material` and fix `Opacity` (flutter/packages#6199) 2024-02-26 [email protected] [url_launcher] Add explicit imports of UIKit (flutter/packages#6208) 2024-02-26 [email protected] [pigeon] Fix tool hangs on verbose sub-processes (flutter/packages#6198) 2024-02-26 [email protected] [tool] Ignore GeneratedPluginRegistrant.swift for `format` (flutter/packages#6195) 2024-02-26 [email protected] Roll Flutter from 1e8dd1e to b77560e (8 revisions) (flutter/packages#6207) 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

…r#6197) Platform interface portion of flutter#6192 Adds `style` to `MapConfiguration` and adds new `getStyleError` method. Part of Fixes flutter/flutter#66207

Platform implementations portion of flutter#6192 Adds handling of `MapConfiguration.style` and implements new `getStyleError` method. Part of flutter/flutter#66207

Adds `style` to the `GoogleMap` widget construction, and internally to the `MapConfiguration` structure, wiring updates to style through the existing configuration update system instead of needing to be set via a separate channel. This makes things more consistent (style is conceptually similar to many of the other configuration options, and is not something that's likely to require frequent dynamic updates), and more importantly allows setting the style as part of creation instead of asynchronously after the map has likely already been displayed. Because of flutter/flutter#24327, this adds a way to query for style errors after the fact. (It's possible that it's why `setMapStyle` was done that way in the first place; the initial PR doesn't explain.) While this is slightly clunky, it's unlikely that this method would actually be used in production code anyway, since presumably people are defining their styles at build time, not runtime, and thus probably don't need runtime error handling. Long term, the solution to this problem is likely to invert the widget/controller relationship, as was done in webview_flutter, but that's a major breaking change and not something we're ready to take on at this point. Since we don't really want two recommended ways of doing the same thing, this deprecates the existing `setMapStyle` method in favor of the new approach. Fixes flutter/flutter#66207

…r#6197) Platform interface portion of flutter#6192 Adds `style` to `MapConfiguration` and adds new `getStyleError` method. Part of Fixes flutter/flutter#66207

Platform implementations portion of flutter#6192 Adds handling of `MapConfiguration.style` and implements new `getStyleError` method. Part of flutter/flutter#66207

Adds `style` to the `GoogleMap` widget construction, and internally to the `MapConfiguration` structure, wiring updates to style through the existing configuration update system instead of needing to be set via a separate channel. This makes things more consistent (style is conceptually similar to many of the other configuration options, and is not something that's likely to require frequent dynamic updates), and more importantly allows setting the style as part of creation instead of asynchronously after the map has likely already been displayed. Because of flutter/flutter#24327, this adds a way to query for style errors after the fact. (It's possible that it's why `setMapStyle` was done that way in the first place; the initial PR doesn't explain.) While this is slightly clunky, it's unlikely that this method would actually be used in production code anyway, since presumably people are defining their styles at build time, not runtime, and thus probably don't need runtime error handling. Long term, the solution to this problem is likely to invert the widget/controller relationship, as was done in webview_flutter, but that's a major breaking change and not something we're ready to take on at this point. Since we don't really want two recommended ways of doing the same thing, this deprecates the existing `setMapStyle` method in favor of the new approach. Fixes flutter/flutter#66207

stuartmorgan added 13 commits February 21, 2024 14:17

Adjust example app for visual repro

15539cc

WIP

fc93d0b

Pathify platform interface

fedcb40

Add style support to MapConfiguration

1dddbb0

Add getStyleError

2434a00

Android implementation

e0355df

Improve documentation

be284f3

Clean up iOS example changes

22837ba

web implementation

7721166

Plumb through app-facing

d4c9bfb

Fix test

dd188d1

version bump

2e8a190

Autoformat

36a6f84

stuartmorgan added the federated: all_changes PR that contains changes for all packages for a federated plugin change label Feb 23, 2024

stuartmorgan requested review from hellohuanlin, reidbaker and ditman as code owners February 23, 2024 18:16

github-actions bot added p: google_maps_flutter platform-android platform-ios platform-web labels Feb 23, 2024

This was referenced Feb 23, 2024

Updating Google Maps options needs a way to fail flutter/flutter#24327

Open

google_maps_flutter setMapStyle before GoogleMap widget is rendered flutter/flutter#66207

Closed

hellohuanlin reviewed Feb 23, 2024

View reviewed changes

reidbaker approved these changes Feb 23, 2024

View reviewed changes

ditman approved these changes Feb 23, 2024

View reviewed changes

stuartmorgan mentioned this pull request Feb 23, 2024

[google_map_flutter] Add style to widget - platform interface #6197

Merged

11 tasks

stuartmorgan added 3 commits February 26, 2024 07:40

Merge branch 'main' into maps-initial-style

ffbfa0a

Unwind PI overrides

953fb8e

Merge remote-tracking branch 'origin/maps-initial-style' into maps-in…

4a8acf3

…itial-style

stuartmorgan mentioned this pull request Feb 26, 2024

[google_map_flutter] Add style to widget - platform impls #6205

Merged

11 tasks

stuartmorgan added 2 commits February 28, 2024 09:59

Merge branch 'main' into maps-initial-style

f74ea3b

Remove remaining overrides; update actual constraints

db1c58e

github-actions bot removed platform-android platform-ios platform-web labels Feb 28, 2024

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 28, 2024

auto-submit bot merged commit 930318a into flutter:main Feb 28, 2024
78 checks passed

engine-flutter-autoroll mentioned this pull request Feb 29, 2024

Roll Packages from 353086c9a61e to 6d02f0359368 (14 revisions) flutter/flutter#144393

Merged

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

930318a [google_maps_flutter] Add style to widget (flutter/packages…

486f1cb

…#6192)

timbotimbo mentioned this pull request Mar 4, 2024

Deprecated GoogleMapController.setMapStyle prevents toggling theme modes flutter/flutter#144524

Closed

reidbaker mentioned this pull request Apr 1, 2024

[camera_android_camerax] Move integration_test dependency to tests reidbaker/packages#4

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter] Add `style` to widget #6192

[google_maps_flutter] Add `style` to widget #6192

stuartmorgan commented Feb 23, 2024

hellohuanlin left a comment

hellohuanlin Feb 23, 2024

stuartmorgan Feb 23, 2024

hellohuanlin Feb 23, 2024

stuartmorgan Feb 23, 2024

hellohuanlin Feb 23, 2024

hellohuanlin Feb 23, 2024

stuartmorgan Feb 23, 2024

stuartmorgan commented Feb 23, 2024

reidbaker Feb 23, 2024

stuartmorgan Feb 23, 2024

ditman Feb 23, 2024 •

edited

Loading

stuartmorgan Feb 23, 2024 •

edited

Loading

ditman Feb 23, 2024

reidbaker Feb 23, 2024

stuartmorgan Feb 23, 2024

ditman left a comment •

edited

Loading

stuartmorgan commented Feb 23, 2024

ditman commented Feb 23, 2024 •

edited

Loading

ditman commented Feb 24, 2024 •

edited

Loading

stuartmorgan commented Feb 26, 2024

stuartmorgan commented Feb 28, 2024

stuartmorgan commented Feb 28, 2024

stuartmorgan commented Feb 28, 2024

stuartmorgan commented Feb 28, 2024

[google_maps_flutter] Add style to widget #6192

[google_maps_flutter] Add style to widget #6192

Conversation

stuartmorgan commented Feb 23, 2024

Pre-launch Checklist

hellohuanlin 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

stuartmorgan commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

stuartmorgan Feb 23, 2024 • edited Loading

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

ditman left a comment • edited Loading

Choose a reason for hiding this comment

stuartmorgan commented Feb 23, 2024

ditman commented Feb 23, 2024 • edited Loading

ditman commented Feb 24, 2024 • edited Loading

stuartmorgan commented Feb 26, 2024

stuartmorgan commented Feb 28, 2024

stuartmorgan commented Feb 28, 2024

stuartmorgan commented Feb 28, 2024

stuartmorgan commented Feb 28, 2024

[google_maps_flutter] Add `style` to widget #6192

[google_maps_flutter] Add `style` to widget #6192

ditman Feb 23, 2024 •

edited

Loading

stuartmorgan Feb 23, 2024 •

edited

Loading

ditman left a comment •

edited

Loading

ditman commented Feb 23, 2024 •

edited

Loading

ditman commented Feb 24, 2024 •

edited

Loading