[google_maps_flutter] Cloud-based map styling support #6553

jokerttu · 2022-10-08T08:51:38Z

Adds support for cloud-based map styling.

On Android, cloud-based maps styling works only with the new (opt-in) renderer, and feature to prefer latest renderer via AndroidManifest meta-data field is added as well.
New renderer will be rolled as default renderer in future releases of the Google Maps Android SDK. After that this feature can be still used to prefer legacy renderer, or can be obsoleted/removed from the package.

When using a cloud-based map Id, it can only be set during map initialization. Changing the map Id for an existing map instance is not supported. If the developer wants to change the map Id, this is easy to implement by changing the key of the widget, in which case the widget is re-initialized with a new map Id.

This is first step of the federated plugin multi-package PR process

Resolves flutter/flutter#67631

Pre-launch Checklist

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

jokerttu · 2022-10-08T12:08:20Z

Apparently updating the Google Maps iOS SDK to support GMMapID changes the required iOS target platform from version 9 to 12, which means this is a breaking change. I'll turn this PR into a draft for now, to find out how to proceed with this.

stuartmorgan-g · 2022-10-19T14:20:08Z

Apparently updating the Google Maps iOS SDK to support GMMapID changes the required iOS target platform from version 9 to 12, which means this is a breaking change. I'll turn this PR into a draft for now, to find out how to proceed with this.

I can see two options:

Do a breaking change and make iOS 12 the minimum supported version. This would also address the ARM issue (see [google_maps_flutter] Update minimum GoogleMaps dependency to a version that supports ARM simulators flutter#94491)
Look up the new selector dynamically, and call it if present, and then document that using this functionality requires setting your app's minimum iOS version to 12. This would be uglier, but would let us keep support for older versions of iOS.

@jmagman Which way do you want to go with this?

jmagman · 2022-10-20T00:04:53Z

Apparently updating the Google Maps iOS SDK to support GMMapID changes the required iOS target platform from version 9 to 12, which means this is a breaking change. I'll turn this PR into a draft for now, to find out how to proceed with this.

I can see two options:

Do a breaking change and make iOS 12 the minimum supported version. This would also address the ARM issue (see [google_maps_flutter] Update GoogleMaps dependency to 6.0 flutter#94491)

Look up the new selector dynamically, and call it if present, and then document that using this functionality requires setting your app's minimum iOS version to 12. This would be uglier, but would let us keep support for older versions of iOS.

@jmagman Which way do you want to go with this?

Upping the iOS min version is such a poor experience for the developer using the plugin since we have no helpful Flutter tooling, I think it just fails to compile, or maybe the pod install fails?. Ideally the tool would parse the podspec somehow and tell the developer how to up the minimum version in their Xcode iOS app, or better do it for them automatically, with a warning perhaps. Since we don't have any of that, I prefer the gross hack of 2. 😞

stuartmorgan-g · 2022-10-20T15:11:48Z

Upping the iOS min version is such a poor experience for the developer using the plugin since we have no helpful Flutter tooling, I think it just fails to compile, or maybe the pod install fails?. Ideally the tool would parse the podspec somehow and tell the developer how to up the minimum version in their Xcode iOS app, or better do it for them automatically, with a warning perhaps.

I filed flutter/flutter#113762

jokerttu · 2022-10-21T21:07:45Z

When using runtime checks eg. @available(iOS 12, *) podfile lint test failed with command: pod lib lint ./packages/google_maps_flutter/google_maps_flutter_ios/ios/google_maps_flutter_ios.podspec --configuration=Debug --skip-tests --use-modular-headers --use-libraries

Instead, using compile time #if functionality fixed linting issues, and seems like mapId can now be used when iOS platform 12+ is targeted.
#if defined(__IPHONE_12_0) && __IPHONE_OS_VERSION_MIN_REQUIRED >= __IPHONE_12_0

Should I upgrade the iOS platform version for example projects, as those are used for integration tests. If platform target version is not 12+, cloud mapId tests are run and passed, but the code block inside the #if -block does not get ever tested.

Also if plugin is marked to be able to be run with iOS platform version 9, can the examples still target version 12, as this mean example projects cannot be run by users with iOS platform version 9 anymore?
Flutter stable now targets iOS platform version 11 by default, is it ok still to have version 12 used in integration tests?

Haven't tested the plugin with iOS platform version <11 as current flutter version forces project to use version 11.

For android, the cloud map id support is available only with LATEST renderer, which is not yet the default renderer for the Google Maps Android plugin. This PR adds also feature to target this latest renderer (and also legacy renderer if ever needed). This is now bundled with the plugin; another option would have been to point users to this documentation showing how to prefer latest renderer, but I see this would have been more complicated option for developers.

stuartmorgan-g · 2022-10-24T15:04:25Z

When using runtime checks eg. @available(iOS 12, *) podfile lint test failed with command: pod lib lint ./packages/google_maps_flutter/google_maps_flutter_ios/ios/google_maps_flutter_ios.podspec --configuration=Debug --skip-tests --use-modular-headers --use-libraries

What exactly was the error?

If we can't use @available we can do it the old fashioned way with respondsToSelector:.

For android, the cloud map id support is available only with LATEST renderer, which is not yet the default renderer for the Google Maps Android plugin. This PR adds also feature to target this latest renderer (and also legacy renderer if ever needed). This is now bundled with the plugin; another option would have been to point users to this documentation showing how to prefer latest renderer, but I see this would have been more complicated option for developers.

Adding this is fine; I was actually planning on doing this anyway after recent discussions with the Google Maps team. Could you split that out into a separate PR that we can land first though, to simplify reviews?

jokerttu · 2022-10-24T18:10:50Z

When using runtime checks eg. @available(iOS 12, *) podfile lint test failed with command: pod lib lint ./packages/google_maps_flutter/google_maps_flutter_ios/ios/google_maps_flutter_ios.podspec --configuration=Debug --skip-tests --use-modular-headers --use-libraries

What exactly was the error?

If we can't use @available we can do it the old fashioned way with respondsToSelector:.

With simplified example:

if (args[@"options"][@"cloudMapId"] && [GMSMapView respondsToSelector:@selector(mapWithFrame:mapID:camera:)]) {
  //GMSMapID *mapID = [GMSMapID mapIDWithIdentifier:args[@"options"][@"cloudMapId"]];
  mapView = [GMSMapView mapWithFrame:frame mapID:nil camera:camera];
} else {
  mapView = [GMSMapView mapWithFrame:frame camera:camera];
}

Running this command (runned in github actions at darwin-lint_podspecs step):
pod lib lint ./packages/google_maps_flutter/google_maps_flutter_ios/ios/google_maps_flutter_ios.podspec --configuration=Debug --skip-tests --use-modular-headers --use-libraries":

Output:
- ERROR | [iOS] xcodebuild: ./packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapController.m:85:27: error: no known class method for selector 'mapWithFrame:mapID:camera:'

@available is giving similar results.
Compile time check is ugly, but as the GoogleMaps pod version is selected before compiling the project, this feature will only be available at runtime if project is targeted to iOS 12+

So how to bypass the pod lib lint error for unknown identifiers with runtime checks? Maybe using even older style NSClassFromString and dynamic selectors with NSSelectorFromString but this sounds even more hacky solution.

For android, the cloud map id support is available only with LATEST renderer, which is not yet the default renderer for the Google Maps Android plugin. This PR adds also feature to target this latest renderer (and also legacy renderer if ever needed). This is now bundled with the plugin; another option would have been to point users to this documentation showing how to prefer latest renderer, but I see this would have been more complicated option for developers.

Adding this is fine; I was actually planning on doing this anyway after recent discussions with the Google Maps team. Could you split that out into a separate PR that we can land first though, to simplify reviews?

Is there already issue for this?
If not, I can create one and the separate PR for preferred renderer selection tomorrow.

stuartmorgan-g · 2022-10-25T10:17:42Z

this feature will only be available at runtime if project is targeted to iOS 12+

🤦🏻 Sorry, right; this is why I had suggested dynamic lookup initially, then I forgot over the weekend.

If the only downside to #if vs dynamic lookup is example support, I think going that route and making the example 12+ is probably fine. We've talked about adding iOS 11 CI testing, but it's not something we currently do. (We could always switch to dynamic lookup later if we do add that testing.) @jmagman thoughts?

stuartmorgan-g · 2022-10-25T10:18:06Z

Is there already issue for this? If not, I can create one and the separate PR for preferred renderer selection tomorrow.

Not yet, no.

jokerttu · 2022-10-25T11:02:56Z

Adding this is fine; I was actually planning on doing this anyway after recent discussions with the Google Maps team. Could you split that out into a separate PR that we can land first though, to simplify reviews?

Separate PR for prefer map renderer for Android: #6619

jokerttu · 2022-10-28T07:51:27Z

Marking as draft until #6619 is landed and this PR updated to work against it.

jokerttu · 2022-12-08T09:53:02Z

Marking as draft until #6619 is landed and this PR updated to work against it.

PR is now updated and implements map renderer selection for android using existing api.

cyanglaz · 2023-01-18T23:50:28Z

...gle_maps_flutter/google_maps_flutter_android/example/integration_test/google_maps_tests.dart

@@ -17,6 +17,7 @@ const LatLng _kInitialMapCenter = LatLng(0, 0);
 const double _kInitialZoomLevel = 5;
 const CameraPosition _kInitialCameraPosition =
    CameraPosition(target: _kInitialMapCenter, zoom: _kInitialZoomLevel);
+const String _kCloudMapId = '8e0a97af9386fef';


Not sure what we should do with the testing map Id, since it is associated with the cloud account. We might need to inject this from ci?

@stuartmorgan

I'm not familiar with the details of our current testing setup for maps; do we have an account already for this? Or does this mean we need to set up and manage a new testing account, and associate our example app with that account? Is this directly connected to the API key, or orthogonal?

We do have a testing account, whose key is encrypted in https://github.com/flutter/plugins/blob/main/.cirrus.yml#L262

On iOS it is used as

plugins/packages/google_maps_flutter/google_maps_flutter_ios/example/ios/Runner/AppDelegate.m

Line 15 in edc6c0e

NSString *mapsApiKey = [[NSProcessInfo processInfo] environment][@"MAPS_API_KEY"];

I believe we will need to do something similar here. I think we should add the environment variable in a separate PR, update this PT after the environment variable is available on main.

_kCloudMapId value does not need to be valid mapId at google maps platform. Google Maps Native SDK's silently fall back to normal map if mapId is not available. This test is meant to test the API and not the SDK behaviour itself. I'll comment this variable, and test if I can change the hash to something that is clearly random id like "0000000000000000", and test that it still passes the tests on each platform.

Replaced with dummy map id 0000000000000000, and comment // Dummy map ID..

stuartmorgan-g

Overall this looks great, just a couple of minor questions. We just need to figure out the testing approach here.

Worst case we could probably just not integration test this, and rely on unit tests on both sides (Dart and native) to ensure that we're plumbing the ID through. That's less robust, but there's also not a lot of code of our own here so it's less critical.

...gle_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/map_configuration.dart

stuartmorgan-g · 2023-01-27T18:56:38Z

packages/google_maps_flutter/google_maps_flutter/example/lib/main.dart

@@ -75,6 +79,34 @@ void main() {
      GoogleMapsFlutterPlatform.instance;
  if (mapsImplementation is GoogleMapsFlutterAndroid) {
    mapsImplementation.useAndroidViewSurface = true;
+    initializeMapRenderer();


The README should be updated to explain the requirement here, and this should have a comment referring to that to explain why it's here.

Map renderers are already explained under google_maps_flutter_android package README.
I can update the app-facing plugin README and add information that cloud-based mapid works only on Android if latest renderer is used.

README updated

packages/google_maps_flutter/google_maps_flutter/example/pubspec.yaml

stuartmorgan-g · 2023-02-22T15:55:30Z

We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages.

Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord.

Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state!

jokerttu requested review from cyanglaz, GaryQian, ditman and stuartmorgan-g as code owners October 8, 2022 08:51

github-actions bot added p: google_maps_flutter platform-android platform-ios platform-web labels Oct 8, 2022

jokerttu changed the title ~~Google maps flutter map id support~~ [google_maps_flutter] Cloud-based map styling support Oct 8, 2022

jokerttu marked this pull request as draft October 8, 2022 12:08

jokerttu marked this pull request as ready for review October 21, 2022 21:11

jokerttu marked this pull request as draft October 28, 2022 07:51

jokerttu force-pushed the google_maps_flutter_map_id_support branch from c47bb4a to 8086f9d Compare December 8, 2022 09:19

jokerttu marked this pull request as ready for review December 8, 2022 09:51

cyanglaz reviewed Jan 18, 2023

View reviewed changes

stuartmorgan-g reviewed Jan 27, 2023

View reviewed changes

jokerttu added 2 commits February 1, 2023 14:41

[google_maps_flutter] cloud-based map id support

42cbd75

[google_maps_flutter] examples - move renderer initialization

7e09827

jokerttu force-pushed the google_maps_flutter_map_id_support branch from fa48a28 to 7e09827 Compare February 1, 2023 12:43

jokerttu added 4 commits February 1, 2023 15:17

[google_maps_flutter] Fix versioning

728053b

[google_maps_flutter] updated tests and documentation

685b8f1

[google_maps_flutter] Fix readme

e8efba6

Fix violations

2673fb3

jokerttu requested review from stuartmorgan-g and removed request for ditman and GaryQian February 2, 2023 12:25

reidbaker self-requested a review February 2, 2023 19:38

stuartmorgan-g closed this Feb 22, 2023

This was referenced Apr 10, 2023

[google_map_flutter] New Cloud-based map style feature support request flutter/flutter#67631

Closed

[google_maps_flutter] Cloud-based map styling support flutter/packages#3682

Merged

[google_maps_flutter] Cloud-based map styling support #6553

[google_maps_flutter] Cloud-based map styling support #6553

Uh oh!

Conversation

jokerttu commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jokerttu commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Oct 19, 2022

Uh oh!

jmagman commented Oct 20, 2022

Uh oh!

stuartmorgan-g commented Oct 20, 2022

Uh oh!

jokerttu commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Oct 24, 2022

Uh oh!

jokerttu commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jokerttu commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jokerttu commented Oct 28, 2022

Uh oh!

jokerttu commented Dec 8, 2022 • edited by cyanglaz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyanglaz Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

cyanglaz Jan 30, 2023 • 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!

jokerttu Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

jokerttu Feb 2, 2023 • 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!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

jokerttu Jan 31, 2023 • 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!

jokerttu Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g commented Feb 22, 2023

Uh oh!

Uh oh!

jokerttu commented Oct 8, 2022 •

edited

Loading

jokerttu commented Oct 8, 2022 •

edited

Loading

jokerttu commented Oct 21, 2022 •

edited

Loading

jokerttu commented Oct 24, 2022 •

edited

Loading

stuartmorgan-g commented Oct 25, 2022 •

edited

Loading

stuartmorgan-g commented Oct 25, 2022 •

edited

Loading

jokerttu commented Oct 25, 2022 •

edited

Loading

jokerttu commented Dec 8, 2022 •

edited by cyanglaz

Loading

cyanglaz Jan 30, 2023 •

edited

Loading

jokerttu Feb 2, 2023 •

edited

Loading

jokerttu Jan 31, 2023 •

edited

Loading