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 support #3682

Merged
merged 6 commits into from
Aug 31, 2023

Conversation

amuramoto
Copy link
Contributor

@amuramoto amuramoto commented Apr 10, 2023

Recreates flutter/plugins#6553 form flutter/plugins which had approvals in-progress.

Fixes flutter/flutter#67631

@amuramoto
Copy link
Contributor Author

@stuartmorgan I believe review for this one was in progress with you in the plugins repo

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 look great, thanks for the test! I have a small nitpick, and a couple of questions. My biggest concern is the one about having separate MapIDs per platform, not sure how we want that to work. (In the current version it seems not supported directly by the plugin.)

@stuartmorgan stuartmorgan changed the title [google_maps_flutter] Recreating PR #6553 from flutter/plugins [google_maps_flutter] Cloud-based map styling support Apr 12, 2023
@stuartmorgan
Copy link
Contributor

Thanks for picking this up! I'll try to review this later this week. In the meantime, there are a ton of CI failures mostly due to Dart compilation errors that will need to be addressed. There's one from the use of both super.key and : super(key: key) on the same constructor; there may be others as well.

@leighajarett
Copy link
Contributor

leighajarett commented Apr 12, 2023

@jokerttu could you help look into the CI failures?

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 fixed the Dart compilation errors so we can get real CI runs.

@reidbaker
Copy link
Contributor

@amuramoto is this something you still plan to work on?

@amuramoto
Copy link
Contributor Author

@amuramoto is this something you still plan to work on?

I believe @jokerttu and team at Codemate are handling this. Please let me know if there's anything you need from me

@@ -58,6 +58,10 @@ The Android implementation supports multiple
[platform view display modes](https://flutter.dev/docs/development/platform-integration/platform-views).
For details, see [the Android README](https://pub.dev/packages/google_maps_flutter_android#display-mode).

#### Cloud-based map styling
Cloud-based map styling works on Android platform only if `AndroidMapRenderer.latest` map renderer has been initialized.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Blank line above this, matching the rest of the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

@@ -58,6 +58,10 @@ The Android implementation supports multiple
[platform view display modes](https://flutter.dev/docs/development/platform-integration/platform-views).
For details, see [the Android README](https://pub.dev/packages/google_maps_flutter_android#display-mode).

#### Cloud-based map styling
Cloud-based map styling works on Android platform only if `AndroidMapRenderer.latest` map renderer has been initialized.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove "platform"

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed

@@ -1,5 +1,5 @@
# Uncomment this line to define a global platform for your project
# platform :ios, '11.0'
# Global platform version is set to 12 for this example project to support cloud-based maps styling
Copy link
Contributor

Choose a reason for hiding this comment

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

The iOS 11 example should not be changed to use iOS 12. That's what the iOS 12 and iOS 13 examples are for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course not, accidentally missed this in rebase. Change reverted.

@stuartmorgan
Copy link
Contributor

It's fine to go ahead and split out the platform interface sub-PR at this point, there's nothing controversial about that part of the change.

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.

LGTM once the version bump is adjusted to a minor version.

@stuartmorgan
Copy link
Contributor

@bparrishMines could you do the secondary review here?

@jokerttu
Copy link
Contributor

Integration tests for web platform seems to hang and timeout after and hour. This is something new; I need to check tomorrow what's causing this. Or if it is even related to this PR.

@jokerttu
Copy link
Contributor

jokerttu commented Aug 25, 2023

Integration tests for web platform seems to hang and timeout after and hour. This is something new; I need to check tomorrow what's causing this. Or if it is even related to this PR.

@stuartmorgan @bparrishMines integration tests for web fixed at fb7a3ea

Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2023
@auto-submit auto-submit bot merged commit cd94db1 into flutter:main Aug 31, 2023
72 checks passed
@jokerttu jokerttu deleted the cbms branch September 1, 2023 04:37
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 7, 2023
flutter/packages@e7d812c...22d4754

2023-09-07 engine-flutter-autoroll@skia.org Roll Flutter (stable) from ff5b5b5 to 2524052 (6 revisions) (flutter/packages#4866)
2023-09-07 maurits@vnbskm.nl [webview_flutter_platform_interface] Adds option to override console log (flutter/packages#4701)
2023-09-01 stuartmorgan@google.com [tools,pigeon] Update tooling to handle Windows build output changes (flutter/packages#4826)
2023-08-31 amuramoto@users.noreply.github.com [google_maps_flutter] Cloud-based map styling support (flutter/packages#3682)
2023-08-31 stuartmorgan@google.com [ci] Convert version presubmit check to LUCI (flutter/packages#4822)
2023-08-31 rajveer0malviya@gmail.com [url_launcher_android] Add support for Custom Tabs (flutter/packages#4739)
2023-08-31 tarrinneal@gmail.com [webview_flutter] update pigeon to 11 (flutter/packages#4821)
2023-08-31 engine-flutter-autoroll@skia.org Roll Flutter (stable) from e1e4722 to ff5b5b5 (1 revision) (flutter/packages#4823)
2023-08-31 engine-flutter-autoroll@skia.org Roll Flutter from 1fe2495 to c175cf8 (30 revisions) (flutter/packages#4825)

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 flutter-ecosystem@google.com,rmistry@google.com 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: google_maps_flutter platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[google_map_flutter] New Cloud-based map style feature support request
7 participants