[google_maps_flutter_android] Add advanced markers support #10381
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 pull request introduces support for Advanced Markers in the Android implementation of google_maps_flutter. The changes are comprehensive, including dependency updates, new logic for handling AdvancedMarkerOptions like PinConfig and collisionBehavior, and the addition of AdvancedMarkerClusterRenderer for clustering. A new API, isAdvancedMarkersAvailable, is also included. The example app has been updated with new pages to demonstrate these features. The implementation is solid and well-tested. I have a couple of minor suggestions to improve code quality and maintainability.
...r_android/android/src/main/java/io/flutter/plugins/googlemaps/ClusterManagersController.java
Outdated
Show resolved
Hide resolved
...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java
Show resolved
Hide resolved
jokerttu
force-pushed
the
feature/advanced_markers_google_maps_flutter_android
branch
from
44e6b7a to
3cfb6f4
Compare
reidbaker
left a comment
reidbaker
left a comment
There was a problem hiding this comment.
Java code reviewed. Sorry this took so long but thank you so much for your contribution.
| */ | ||
| private void initializeRenderer(ClusterManager<MarkerBuilder> clusterManager) { | ||
| final ClusterRenderer<MarkerBuilder> clusterRenderer = | ||
| markerType == PlatformMarkerType.ADVANCED_MARKER |
There was a problem hiding this comment.
Nit: Can you make this a switch instead of a turnary for readability?
| getPinConfigFromPlatformPinConfig( | ||
| pinConfigBitmap, assetManager, density, bitmapDescriptorFactory); | ||
| return bitmapDescriptorFactory.fromPinConfig(pinConfig); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Can you pick a more specific set of exceptions here?
| ? toInt(pinConfigBitmap.getBackgroundColor()) | ||
| : null; | ||
| final Integer borderColor = | ||
| pinConfigBitmap.getBorderColor() != null ? toInt(pinConfigBitmap.getBorderColor()) : null; |
There was a problem hiding this comment.
There is a lot of null handling in this code. Each line is easy enough to understand but i think it would be more readable if you made helper methods.
| return JointType.DEFAULT; | ||
| } | ||
|
|
||
| static int collisionBehaviorFromPigeon( |
There was a problem hiding this comment.
Can you add a javadoc for this method?
| if (googleMap == null) { | ||
| throw new FlutterError( | ||
| "GoogleMap uninitialized", | ||
| "getMapCapabilities() called prior to map initialization", |
There was a problem hiding this comment.
Should this say isAdvancedMarkersAvailable somewhere in the error?
| this.markerOptions = new MarkerOptions(); | ||
| MarkerBuilder(String markerId, String clusterManagerId, PlatformMarkerType markerType) { | ||
| this.markerOptions = | ||
| markerType == PlatformMarkerType.ADVANCED_MARKER |
There was a problem hiding this comment.
Same here can you make this a switch instead of a turnary?
|
|
||
| @Override | ||
| public void setCollisionBehavior(@AdvancedMarkerOptions.CollisionBehavior int collisionBehavior) { | ||
| if (markerOptions.getClass() == AdvancedMarkerOptions.class) { |
There was a problem hiding this comment.
Can you write this to not use getClass? In general we dont like having to use getClass or instanceOf they are are code smells.
...droid/android/src/test/java/io/flutter/plugins/googlemaps/ClusterManagersControllerTest.java
Outdated
Show resolved
Hide resolved
| .setIcon(icon) | ||
| .setInfoWindow(infoWindow); | ||
| .setInfoWindow(infoWindow) | ||
| .setCollisionBehavior(PlatformMarkerCollisionBehavior.REQUIRED_DISPLAY); |
There was a problem hiding this comment.
Do you have a test for the behavior if someone passes an invalid version? if not can you add one
There was a problem hiding this comment.
I don't think It's possible to pass an invalid value nor test it here.
jokerttu
force-pushed
the
feature/advanced_markers_google_maps_flutter_android
branch
2 times, most recently
from
304d11c to
4e3e0e9
Compare
jokerttu
force-pushed
the
feature/advanced_markers_google_maps_flutter_android
branch
from
4e3e0e9 to
82fa871
Compare
3 participants
This PR adds Advanced markers support to the Android implementation of google_maps_flutter.
Approved combined PR: #7882
Approved and merged platform interface PR: #9737
Issue: flutter/flutter#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 under1.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 under1.///).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 ↩3