[google_maps_flutter] Add MapColorScheme support to Android and iOS#11736
[google_maps_flutter] Add MapColorScheme support to Android and iOS#11736snaeji wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
4da8e36 to
23d6cae
Compare
There was a problem hiding this comment.
Code Review
This pull request implements colorScheme support for cloud-based maps styling brightness on Android and iOS, extending the existing web functionality. The updates span the platform interface, native platform implementations, and Pigeon message definitions. A review comment correctly identifies that the documentation for colorScheme should be updated to reflect that it can be modified after map creation on iOS, aligning the documentation with the new implementation.
Wire up MapColorScheme (light/dark/followSystem) to native APIs on both mobile platforms. The platform interface and app-facing package already support this parameter (added in 2.16.0 for web), but the Android and iOS plugins silently ignored it. Android: Uses GoogleMap.setMapColorScheme() with MapColorScheme constants. Handles both creation-time and runtime updates. iOS: Maps to UIView.overrideUserInterfaceStyle (light/dark/unspecified) on the GMSMapView, matching the Google Maps SDK for iOS documented approach. Changes per platform: - Pigeon: Add PlatformMapColorScheme enum + field on PlatformMapConfiguration - Dart: Add converter + wire into config mapper - Native: Read field, convert to native API, apply at init and on update - Tests: Convert mapper + dispatcher tests + creation-params plumbing Updates app-facing google_maps_flutter and platform_interface doc comments to reflect Android and iOS support (previously documented as "Web only"). Mirrors changes to google_maps_flutter_ios_sdk9, google_maps_flutter_ios_sdk10, and google_maps_flutter_ios_shared_code via dart tool/sync_shared_files.dart.
23d6cae to
1d4d137
Compare
LongCatIsLooong
left a comment
There was a problem hiding this comment.
The iOS implementation LGTM modulo https://github.com/flutter/packages/pull/11736/changes#r3270153752
| return PlatformMapColorScheme.followSystem; | ||
| } | ||
|
|
||
| // The enum comes from a different package, which could get a new value at |
There was a problem hiding this comment.
I believe switch expressions / statements have to be exhaustive or it is gong to be a compile time error. The code isn't going to compile if there's an unhandled case. IOW the runtime error will never be reachable, unless you add a default case. But adding a new case to an existing enum should require a major version bump I think? So we don't really have to handle that at runtime.
There was a problem hiding this comment.
Also this could just be an expression:
return switch (colorScheme) {
null => null,
.light => .light,
.dart => .dart,
.followSystem => .followSystem,
};There was a problem hiding this comment.
Actually, why do we need 2 enums that represent the same thing? Can we just re-export one / making one of the an alias of the other? Then we don't need this since there's only one type under the hood.
There was a problem hiding this comment.
I believe switch expressions / statements have to be exhaustive or it is gong to be a compile time error.
Switch expressions do, not arbitrary switch statements.
But adding a new case to an existing enum should require a major version bump I think?
We have not traditionally considered that to be a major version change. We may need to revisit that at some point as switch expressions become more common, but for now we don't (at least for platform interfaces).
So we don't really have to handle that at runtime.
We do, for the specific goal of not requiring breaking the platform interface all the time.
There was a problem hiding this comment.
Actually, why do we need 2 enums that represent the same thing?
Because Pigeon definitions currently aren't allowed to import things from other files. And doing so in the case of an enum from another package would be extremely dangerous since it would allow for Dart code to change without the native side changing.
Description
Wires up
MapColorScheme(light/dark/followSystem) to native APIs on Android and iOS. The platform interface and app-facinggoogle_maps_flutterpackage already exposed this parameter (added in 2.16.0 for web), but the Android and iOS plugins silently ignored it.GoogleMap.setMapColorScheme()is invoked with the matchingMapColorSchemeconstant. Both creation-time and runtime updates (viainterpretMapConfiguration) are supported.GMSMapView.overrideUserInterfaceStyleis set toUIUserInterfaceStyleLight/Dark/Unspecified— the documented API per the Google Maps SDK for iOS.The change is mirrored across
google_maps_flutter_ios,google_maps_flutter_ios_sdk9,google_maps_flutter_ios_sdk10, andgoogle_maps_flutter_ios_shared_codeviadart tool/sync_shared_files.dart.The
colorSchemedoc comment ingoogle_maps_flutterandgoogle_maps_flutter_platform_interfaceis updated to reflect the new platform support (previously documented as "Web only").Tests added:
Convert.toMapColorSchemeJUnit tests for all three enum values;interpretMapConfiguration_handlesColorSchemedispatcher test mirroring_handlesStyle; Dart-side creation-params test assertingcolorSchemeflows through to the platform view.light,dark,followSystem, andnull; matching tests propagated to sdk9 and sdk10 via the shared-code sync.Fixes flutter/flutter#186737
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2