-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[google_maps_flutter_web] Add Advanced markers support (web) #9773
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
base: main
Are you sure you want to change the base?
[google_maps_flutter_web] Add Advanced markers support (web) #9773
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_web/lib/google_maps_flutter_web.dart
Show resolved
Hide resolved
| clusterManagersController: clusterManagersController! | ||
| as ClusterManagersController<gmaps.AdvancedMarkerElement>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/google_maps_flutter/google_maps_flutter_web/lib/src/marker.dart
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_web/lib/src/markers.dart
Show resolved
Hide resolved
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be ^1.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
From triage: @aednlaxer Are you still planning on updating this PR? |
|
FYI: After merging in |
Yes, please don't close it |
9da73a7 to
65b8c81
Compare
mdebbar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it looks good to me! I just have a few minor code improvements.
| options.glyphColor = _getCssColor(circleGlyph.color); | ||
| case final TextGlyph textGlyph: | ||
| final web.Element element = web.document.createElement('p'); | ||
| element.innerHTML = textGlyph.text.toJS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's safer to use element.text:
| element.innerHTML = textGlyph.text.toJS; | |
| element.text = textGlyph.text; |
| element.setAttribute( | ||
| 'style', | ||
| 'color: ${_getCssColor(textGlyph.textColor!)}', | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| element.setAttribute( | |
| 'style', | |
| 'color: ${_getCssColor(textGlyph.textColor!)}', | |
| ); | |
| element.style.color = _getCssColor(textGlyph.textColor!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Element type doesn't have style property:
https://github.com/dart-lang/web/blob/5a7d0be70a258252b95bac6b900f26d6dae4d433/web/lib/src/dom/dom.dart#L3132
| icon.setAttribute( | ||
| 'style', | ||
| <String>[ | ||
| if (size != null) ...<String>[ | ||
| 'width: ${size.width.toStringAsFixed(1)}px;', | ||
| 'height: ${size.height.toStringAsFixed(1)}px;', | ||
| ], | ||
| if (opacity != null) 'opacity: $opacity;', | ||
| if (isVisible != null) 'visibility: ${isVisible ? 'visible' : 'hidden'};', | ||
| ].join(' '), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| icon.setAttribute( | |
| 'style', | |
| <String>[ | |
| if (size != null) ...<String>[ | |
| 'width: ${size.width.toStringAsFixed(1)}px;', | |
| 'height: ${size.height.toStringAsFixed(1)}px;', | |
| ], | |
| if (opacity != null) 'opacity: $opacity;', | |
| if (isVisible != null) 'visibility: ${isVisible ? 'visible' : 'hidden'};', | |
| ].join(' '), | |
| final iconStyle = icon.style; | |
| if (size != null) { | |
| iconStyle | |
| ..width = '${size.width.toStringAsFixed(1)}px' | |
| ..height = '${size.height.toStringAsFixed(1)}px'; | |
| } | |
| if (opacity != null) { | |
| iconStyle.opacity = opacity; | |
| } | |
| if (isVisible != null) { | |
| iconStyle.visibility = isVisible ? 'visible' : 'hidden'; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Icon type of the web package doesn't have style property either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making icon of type HTMLImageElement should make it work.
| }); | ||
|
|
||
| @override | ||
| void initializeMarkerListener({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically use the "add listener" terminology in the flutter code base: https://api.flutter.dev/flutter/search.html?q=addlistener
What do you think about renaming this to addMarkerListeners?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to keep the naming consistent. I have changed the function name to addMarkerListener.
| if (_infoWindow != null && newInfoWindowContent != null) { | ||
| _infoWindow.content = newInfoWindowContent; | ||
| if (onTap != null) { | ||
| marker.onClick.listen((gmaps.MapMouseEvent event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to unsubscribe from these listeners when the marker is removed?
If yes, it can easily be done:
_subscriptions = [
if (onTap != null) marker.onClick.listen((_) => onTap.call()),
if (onDragStart != null) marker.onDragstart.listen((event) {
marker.position = event.latLng;
onDragStart.call(event.latLng ?? _nullGmapsLatLng);
}),
...
];and later in void remove():
_subscriptions?.forEach((sub) => sub.cancel);
_subscriptions = null;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| required VoidCallback? onTap, | ||
| }) { | ||
| if (onTap != null) { | ||
| marker.onClick.listen((gmaps.MapMouseEvent event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about unsubscribing from listeners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
c1047c3 to
f72d139
Compare
mdebbar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some suggestions to make element.style and icon.style available.
| final web.Element icon = web.document.createElement('img') | ||
| ..setAttribute('src', url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use a more specific type so we can use icon.style:
| final web.Element icon = web.document.createElement('img') | |
| ..setAttribute('src', url); | |
| final web.HTMLImageElement icon = web.HTMLImageElement()..src = url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. All changes done.
| final web.Element icon = web.document.createElement('img')..setAttribute( | ||
| 'src', | ||
| ui_web.assetManager.getAssetUrl(iconConfig[1]! as String), | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
| final web.Element icon = web.document.createElement('img') | ||
| ..setAttribute('src', web.URL.createObjectURL(blob as JSObject)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
| icon.setAttribute( | ||
| 'style', | ||
| <String>[ | ||
| if (size != null) ...<String>[ | ||
| 'width: ${size.width.toStringAsFixed(1)}px;', | ||
| 'height: ${size.height.toStringAsFixed(1)}px;', | ||
| ], | ||
| if (opacity != null) 'opacity: $opacity;', | ||
| if (isVisible != null) 'visibility: ${isVisible ? 'visible' : 'hidden'};', | ||
| ].join(' '), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making icon of type HTMLImageElement should make it work.
| final web.Element element = web.document.createElement('p'); | ||
| element.text = textGlyph.text; | ||
| if (textGlyph.textColor != null) { | ||
| element.setAttribute( | ||
| 'style', | ||
| 'color: ${_getCssColor(textGlyph.textColor!)}', | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a HTMLParagraphElement type will allow you to use element.style:
| final web.Element element = web.document.createElement('p'); | |
| element.text = textGlyph.text; | |
| if (textGlyph.textColor != null) { | |
| element.setAttribute( | |
| 'style', | |
| 'color: ${_getCssColor(textGlyph.textColor!)}', | |
| ); | |
| final web.HTMLParagraphElement element = web.HTMLParagraphElement(); | |
| element.text = textGlyph.text; | |
| if (textGlyph.textColor != null) { | |
| element.style.color = _getCssColor(textGlyph.textColor!); |
mdebbar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
4cbf9d7 to
71404f1
Compare
71404f1 to
f30050c
Compare
| final gmaps.Size gmapsSize = gmaps.Size(size.width, size.height); | ||
| icon.size = gmapsSize; | ||
| icon.scaledSize = gmapsSize; | ||
| // Sets the size and style of the [icon] element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc-style (i.e., caller-facing, rather than implementation-detail-describing) comments should still use /// even if the methods aren't public.
| BitmapDescriptor bitmapDescriptor, { | ||
| required double? opacity, | ||
| required bool isVisible, | ||
| required double? rotation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are two of these parameters both required and nullable? What does explicitly passing null mean conceptually? This should be explained in the doc comment.
| ..visibility = isVisible ? 'visible' : 'hidden' | ||
| ..opacity = opacity?.toString() ?? '1.0' | ||
| ..transform = rotation != null ? 'rotate(${rotation}deg)' : ''; | ||
| return htmlElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These implementations are all quite long, which makes it hard to see the overall structure of the method being essentially a switch on bitmapDescriptor. This would be much easier to follow if each of the bodies of the top-level ifs were extracted to their own helpers, and this method were just the branching.
| (final BytesMapBitmap bytesMapBitmap) => _bitmapBlobUrlCache.putIfAbsent( | ||
| bytesMapBitmap.byteData.hashCode, | ||
| () { | ||
| final web.Blob blob = web.Blob( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have the same TODO as the essentially identical code above?
|
|
||
| /// Gets marker Id from a [marker] object. | ||
| MarkerId getMarkerId(Object? marker) { | ||
| final JSObject object = marker! as JSObject; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the parameter nullable if the first line is going to assert that it's not null?
| /// Creates a `MarkerController`, which wraps a [gmaps.Marker] object, its `onTap`/`onDrag` behavior, and its associated [gmaps.InfoWindow]. | ||
| /// The `MarkerController` class wraps a [gmaps.AdvancedMarkerElement] | ||
| /// or [gmaps.Marker], how it handles events, and its associated (optional) | ||
| /// [gmaps.InfoWindow] widget. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should explain what T and O are in the context of the class.
|
|
||
| @override | ||
| void showInfoWindow() { | ||
| assert(_marker != null, 'Cannot `showInfoWindow` on a `remove`d Marker.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a (debug only) assert rather than a StateError?
| /// advanced [gmaps.AdvancedMarkerElement] class. | ||
| /// | ||
| /// [T] must extend [JSObject]. It's not specified in code because our mocking | ||
| /// framework does not support mocking JSObjects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, the role of T and O should be explained.
| ## 0.6.0 | ||
|
|
||
| * Adds Advanced markers support. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please list the breaking changes (following repo style), and provide more details about the new APIs, giving someone reading this enough pointers that they could figure out what APIs to go look at to adopt the new markers.
| ``` | ||
|
|
||
| Now you should be able to use the Google Maps plugin normally. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be helpful to add a new section discussing legacy vs advanced markers, with links to relevant underlying SDK docs.
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].///).