Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[google_maps_flutter] cloud-based map styling implementation #4638

Merged
merged 9 commits into from
Aug 24, 2023
Merged

[google_maps_flutter] cloud-based map styling implementation #4638

merged 9 commits into from
Aug 24, 2023

Conversation

jokerttu
Copy link
Contributor

@jokerttu jokerttu commented Aug 3, 2023

This PR is sub-PR splitted out from the #3682
containing only following packages:

  • google_maps_flutter_web
  • google_maps_flutter_android
  • google_maps_flutter_ios

Related to issue flutter/flutter#67631

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

 - android ios and web implementation
@jokerttu jokerttu changed the title [google_maps_flutter] cloud-based map styling [google_maps_flutter] cloud-based map styling implementation Aug 3, 2023
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of comments about some small testing improvements that should be easy to add.

Unfortunately the overall testing of this feature is quite thin; the Dart integration test doesn't actually test that anything works beyond not crashing (i.e., they would pass even if we reverted all of the native changes), but given that we don't have real API testing working in general I don't see a way to do address that without blocking this feature indefinitely, so we'll have to live with that for now.

(Ideally we'd also do native unit testing of the native code, but we don't have the native test scaffolding set up for that; I'll file an issue for that rather than blocking this.)

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web bits LGTM.

I agree with Stuart that the testing is very bare bones, but this is the downside of a cloud-based feature, the plugin is completely unaware of what the endpoints end up doing.

Every testing solution I can think of is pretty brittle; for example: taking a screenshot and comparing against a reference image, or (in the web) inspecting the inner DOM of the map and see if we can find the mapId somewhere, or traces of WebGL ;)

@stuartmorgan
Copy link
Contributor

Yes, I think the best we'll be able to do is just test that we're passing it through both the Dart->native (what I requested above) and native->SDK layers (filed as flutter/flutter#131851 for follow-up). Once we have that, the only plausible breakages I can think of on our side would be if that was insufficient (e.g., if at some point there were a requirement that some other data be passed with the cloud ID in order for it to work, in which case our tests would still pass but the feature wouldn't), but that's relatively low risk.

@ditman
Copy link
Member

ditman commented Aug 3, 2023

Yep, there's not much more we can do. The only breaking scenario that I can think of is if mapId makes some of the other properties that we accept incompatible/obsolete (imagine a map that happens to have "cloud styling" applied suddenly starts rejecting the local mapStyle property with an error)

@jokerttu
Copy link
Contributor Author

jokerttu commented Aug 4, 2023

I have added a few Dart unit tests as requested. These tests verify that the given cloudMapId is passed to the platform view via the method channel upon its creation.

Since the Maps SDK doesn't provide a means to retrieve the passed cloudMapId after a map instance is initialized, or even a status to indicate if the map has loaded with the applied cloud-based styling, I don't see any alternatives for effectively checking whether the cloud-based map styling is really applied, other than visual test comparison using screenshots.

It is possible to save the passed cloudMapId string at native side for our instance and pass that back over the GoogleMapsInspectorPlatform method implementation. Then it would be possible to test on integration tests that the requested mapId has been passed from the dart->native, but this won't test if the map is really initialized with the given mapId. If this is something you want to have, I can update the PR with this feature.

@jokerttu jokerttu requested a review from stuartmorgan August 4, 2023 18:39
@jokerttu jokerttu marked this pull request as draft August 4, 2023 19:16
@jokerttu jokerttu marked this pull request as ready for review August 4, 2023 19:47
Copy link
Contributor

@reidbaker reidbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving final approval for stuart but android and dart bits LGTM.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, LGTM. Thanks for adding the tests!

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 23, 2023

auto label is removed for flutter/packages/4638, due to Pull request flutter/packages/4638 is not in a mergeable state.

@jokerttu
Copy link
Contributor Author

@stuartmorgan: merged with main and fixed Changelog conflicts

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 24, 2023
@auto-submit auto-submit bot merged commit 383bffa into flutter:main Aug 24, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 24, 2023
flutter/packages@3060b1a...383bffa

2023-08-24 [email protected] [google_maps_flutter] cloud-based map styling implementation (flutter/packages#4638)
2023-08-23 [email protected] [image_picker] Fix exception when canceling `pickMultipleMedia` on iOS (flutter/packages#4761)
2023-08-23 [email protected] Roll Flutter from 54c98d7 to bd836cc (24 revisions) (flutter/packages#4760)

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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@reidbaker reidbaker mentioned this pull request Sep 15, 2023
11 tasks
@cedvdb
Copy link
Contributor

cedvdb commented Dec 6, 2023

Note that the styles seem to be passed into the map still. On the web platform this gives a warning

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

The property styles should not be part of the map options when a mapId is set.

@stuartmorgan
Copy link
Contributor

@cedvdb Please report issues with landed code in the issue tracker; changes can't be made here as this is already merged.

@cedvdb
Copy link
Contributor

cedvdb commented Dec 7, 2023

@stuartmorgan will do, I was too lazy to make a repro but I can probably find the strength to do it today.

@kornha

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants