[google_maps_flutter_web] Add Advanced markers support (web) #9773
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR adds support for Advanced Markers to the web implementation of google_maps_flutter. The changes are extensive and well-structured, refactoring the marker handling logic to support both legacy and advanced markers through generics and abstract classes. New tests have been added for the new functionality, and existing ones have been updated. The implementation looks solid, but I've found a couple of critical issues that would prevent the code from compiling or running correctly.
| clusterManagersController: clusterManagersController! | ||
| as ClusterManagersController<gmaps.AdvancedMarkerElement>, |
There was a problem hiding this comment.
There's a typo here. clusterManagersController is used, but it should be _clusterManagersController. This will cause a compilation error as clusterManagersController is not defined in this scope.
clusterManagersController: _clusterManagersController!
as ClusterManagersController<gmaps.AdvancedMarkerElement>,
There was a problem hiding this comment.
Obviously this does compile, but it does make it order-sensitive to when the private variable is set, which we don't want, so this should use _clusterManagersController.
| MarkerId getMarkerId(Object? marker) { | ||
| final JSObject object = marker! as JSObject; | ||
| final gmaps.MVCObject mapObject = marker as gmaps.MVCObject; | ||
| if (object.isA<gmaps.Marker>()) { | ||
| return MarkerId((mapObject.get('markerId')! as JSString).toDart); | ||
| } else if (object.isA<gmaps.AdvancedMarkerElement>()) { | ||
| return MarkerId((mapObject.get('id')! as JSString).toDart); | ||
| } else { | ||
| throw ArgumentError( | ||
| 'Must be either a gmaps.Marker or a gmaps.AdvancedMarkerElement', | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of getMarkerId has a couple of issues for AdvancedMarkerElement:
gmaps.AdvancedMarkerElementis anHTMLElementand does not extendgmaps.MVCObject. The castmarker as gmaps.MVCObjectwill fail at runtime when the marker is anAdvancedMarkerElement.- The
idof anAdvancedMarkerElementis a standard DOM attribute, not a property set viaMVCObject.set. It should be accessed directly from the element.
Here's a corrected implementation.
MarkerId getMarkerId(Object? marker) {
final JSObject object = marker! as JSObject;
if (object.isA<gmaps.Marker>()) {
final gmaps.MVCObject mapObject = marker as gmaps.MVCObject;
return MarkerId((mapObject.get('markerId')! as JSString).toDart);
} else if (object.isA<gmaps.AdvancedMarkerElement>()) {
final gmaps.AdvancedMarkerElement element =
marker as gmaps.AdvancedMarkerElement;
return MarkerId(element.id);
} else {
throw ArgumentError(
'Must be either a gmaps.Marker or a gmaps.AdvancedMarkerElement',
);
}
}
| @@ -20,6 +20,7 @@ import 'package:google_maps/google_maps_visualization.dart' as visualization; | |||
| import 'package:google_maps_flutter_platform_interface/google_maps_flutter_platform_interface.dart'; | |||
| import 'package:sanitize_html/sanitize_html.dart'; | |||
| import 'package:stream_transform/stream_transform.dart'; | |||
| import 'package:web/web.dart' as web; | |||
There was a problem hiding this comment.
Why is this being imported twice, once with a namespace?
| clusterManagersController: clusterManagersController! | ||
| as ClusterManagersController<gmaps.AdvancedMarkerElement>, |
There was a problem hiding this comment.
Obviously this does compile, but it does make it order-sensitive to when the private variable is set, which we don't want, so this should use _clusterManagersController.
| /// The `MarkerController` class wraps a [gmaps.AdvancedMarkerElement] | ||
| /// or [gmaps.Marker], how it handles events, and its associated (optional) | ||
| /// [gmaps.InfoWindow] widget. | ||
| abstract class MarkerController<T, O> { |
There was a problem hiding this comment.
I missed this before; isn't this a breaking change for this package? It looks like this (via the part directive) is public API, and the public constructor can no longer be directly called.
That's not a significant issue if we don't expect people to generally be creating one directly, but it does mean we need to version this PR as a major version change.
| /// | ||
| /// [T] must extend [JSObject]. It's not specified in code because our mocking | ||
| /// framework does not support mocking JSObjects. | ||
| abstract class MarkersController<T, O> extends GeometryController { |
There was a problem hiding this comment.
Same here; this looks breaking.
| sanitize_html: ^2.0.0 | ||
| stream_transform: ^2.0.0 | ||
| web: ">=0.5.1 <2.0.0" | ||
| web: ">=1.0.0 <2.0.0" |
There was a problem hiding this comment.
This can just be ^1.0.0
This PR adds Advanced markers support to the web implementation of
google_maps_flutter.Approved combined PR: #7882
Approved and merged platform interface PR: #9737
Issue: #155526
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].///).