[google_maps_flutter] Add Advanced Markers support#11188
[google_maps_flutter] Add Advanced Markers support#11188stuartmorgan-g wants to merge 10 commits intoflutter:mainfrom
Conversation
|
@bparrishMines / @tarrinneal could one of you do a final review+approval on this? It's #7882 with minor changes to bring it up to date, and to prevent an accidental breaking change. |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Advanced Markers in google_maps_flutter. The changes include a new GoogleMapMarkerType enum, a mapId property on the GoogleMap widget to replace the now-deprecated cloudMapId, and updated dependencies. The PR also adds several new, comprehensive example pages demonstrating advanced marker features such as custom icons, clustering, and collision behavior. The implementation is solid and includes corresponding tests. My review includes one suggestion to refactor a new widget in an example file for improved efficiency and adherence to Flutter's state management best practices.
Note: Security Review did not run due to the size of the PR.
| class _AdvancedMarkersCapabilityStatusState | ||
| extends State<AdvancedMarkersCapabilityStatus> { | ||
| /// Whether map supports advanced markers. Null indicates capability check | ||
| /// is in progress. | ||
| bool? _isAdvancedMarkersAvailable; | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| if (widget.controller != null) { | ||
| GoogleMapsFlutterPlatform.instance | ||
| .isAdvancedMarkersAvailable(mapId: widget.controller!.mapId) | ||
| .then((bool result) { | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| setState(() { | ||
| _isAdvancedMarkersAvailable = result; | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| return Padding( | ||
| padding: const EdgeInsets.all(16), | ||
| child: Text( | ||
| switch (_isAdvancedMarkersAvailable) { | ||
| null => 'Checking map capabilities…', | ||
| true => | ||
| 'Map capabilities check result:\nthis map supports advanced markers', | ||
| false => | ||
| "Map capabilities check result:\nthis map doesn't support advanced markers. Please check that map ID is provided and correct map renderer is used", | ||
| }, | ||
| textAlign: TextAlign.center, | ||
| style: Theme.of(context).textTheme.bodyLarge?.copyWith( | ||
| color: switch (_isAdvancedMarkersAvailable) { | ||
| true => Colors.green.shade700, | ||
| false => Colors.red, | ||
| null => Colors.black, | ||
| }, | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The build method of _AdvancedMarkersCapabilityStatusState triggers an asynchronous check on every build when a controller is available. This is inefficient as it can lead to multiple unnecessary API calls.
Consider moving the capability check logic into didUpdateWidget to only perform the check when the GoogleMapController instance actually changes. This makes the build method purely for rendering the UI based on the current state.
Also, when using .then() on a future, it's a good practice to check if the widget is still mounted before calling setState to avoid errors if the widget is disposed before the future completes.
class _AdvancedMarkersCapabilityStatusState
extends State<AdvancedMarkersCapabilityStatus> {
/// Whether map supports advanced markers. Null indicates capability check
/// is in progress.
bool? _isAdvancedMarkersAvailable;
@override
void didUpdateWidget(AdvancedMarkersCapabilityStatus oldWidget) {
super.didUpdateWidget(oldWidget);
if (widget.controller != oldWidget.controller) {
_checkCapability();
}
}
void _checkCapability() {
if (widget.controller == null) {
if (mounted) {
setState(() {
_isAdvancedMarkersAvailable = null;
});
}
return;
}
GoogleMapsFlutterPlatform.instance
.isAdvancedMarkersAvailable(mapId: widget.controller!.mapId)
.then((bool result) {
if (mounted) {
setState(() {
_isAdvancedMarkersAvailable = result;
});
}
});
}
@override
Widget build(BuildContext context) {
return Padding(
padding: const EdgeInsets.all(16),
child: Text(
switch (_isAdvancedMarkersAvailable) {
null => 'Checking map capabilities…',
true =>
'Map capabilities check result:\nthis map supports advanced markers',
false =>
"Map capabilities check result:\nthis map doesn't support advanced markers. Please check that map ID is provided and correct map renderer is used",
},
textAlign: TextAlign.center,
style: Theme.of(context).textTheme.bodyLarge?.copyWith(
color: switch (_isAdvancedMarkersAvailable) {
true => Colors.green.shade700,
false => Colors.red,
null => Colors.black,
},
),
),
);
}
}
Updates #7882 to a landable state (since that PR is from an org repo, so is not writable by Flutter team). All of the sub-PRs have now landed, so this is just the final version of the app-facing package changes.
Fixes flutter/flutter#155526
Pre-Review Checklist
[shared_preferences]///).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