-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[google_maps_flutter] Add Advanced Markers support #7882
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] Add Advanced Markers support #7882
Conversation
reidbaker
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.
High level android code review. I am concerned by having to runtime type check class.
That said thank you for opening this pull request.
packages/google_maps_flutter/google_maps_flutter/example/android/app/build.gradle
Outdated
Show resolved
Hide resolved
...r_android/android/src/main/java/io/flutter/plugins/googlemaps/ClusterManagersController.java
Outdated
Show resolved
Hide resolved
...r_android/android/src/main/java/io/flutter/plugins/googlemaps/ClusterManagersController.java
Outdated
Show resolved
Hide resolved
...flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
Outdated
Show resolved
Hide resolved
...ps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapFactory.java
Outdated
Show resolved
Hide resolved
...ps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkerController.java
Outdated
Show resolved
Hide resolved
stuartmorgan-g
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.
I've only looked at the app-facing and platform interface packages so far, but there are some fundamental structure questions at the interface layer.
...ages/google_maps_flutter/google_maps_flutter/example/lib/readme_sample_advanced_markers.dart
Outdated
Show resolved
Hide resolved
...ages/google_maps_flutter/google_maps_flutter/example/lib/readme_sample_advanced_markers.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/lib/clustering.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/lib/main.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/lib/marker_icons.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/glyph.dart
Outdated
Show resolved
Hide resolved
...gle_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/map_configuration.dart
Outdated
Show resolved
Hide resolved
...gle_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/map_configuration.dart
Outdated
Show resolved
Hide resolved
...s_flutter/google_maps_flutter_platform_interface/lib/src/types/map_widget_configuration.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/marker.dart
Outdated
Show resolved
Hide resolved
….com:CodemateLtd/packages into feature/google-maps-advanced-markers-support
packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart
Outdated
Show resolved
Hide resolved
camsim99
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, the Android implementation looks good to me! Left some questions regarding testing.
...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java
Outdated
Show resolved
Hide resolved
...le_maps_flutter_android/android/src/test/java/io/flutter/plugins/googlemaps/ConvertTest.java
Outdated
Show resolved
Hide resolved
...ter_android/android/src/test/java/io/flutter/plugins/googlemaps/GoogleMapControllerTest.java
Show resolved
Hide resolved
..._maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkerBuilder.java
Show resolved
Hide resolved
| skip: true, | ||
| ); | ||
|
|
||
| testWidgets('markerWithPinConfig', (WidgetTester tester) async { |
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.
Is this testing anything? Can we remove it?
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 doesn't explicitly check anything with expect(), it's similar to other marker tests (markerWithLegacyBytes, markerWithAssetMapBitmapCreate etc) which are "run it to make sure it doesn't throw an exception or fail in a different way" kind of tests
vashworth
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.
iOS side LGTM
This comment was marked as off-topic.
This comment was marked as off-topic.
….com:CodemateLtd/packages into feature/google-maps-advanced-markers-support
camsim99
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.
Android LGTM!
|
@aednlaxer This has high-level LGs for all components now, so please feel free to split out the platform interface PR. I'll re-review that piece here in the meantime, but any comments can be addressed in the sub-PR. |
stuartmorgan-g
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.
I re-reviewed the platform interface portion, and everything looks good there :)
…#9737) This PR adds Advanced markers support to the platform interface of `google_maps_flutter`. Approved combined PR: #7882 Issue: [#155526](flutter/flutter#155526) ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
|
Greetings from stale PR triage! 👋 |
|
This is waiting on the sub-PR at #9773 |
|
@aednlaxer It looks like only web was split out; is there a reason the Android and iOS implementations weren't included in that PR (or a parallel split-out PR)? |
@stuartmorgan-g Discussed with @aednlaxer and I'll take over @aednlaxer 's work, and continue splitting the next PR's. |
|
Created split PR for Android. |
This PR adds Advanced Markers support to
google_maps_flutteras discussed in #155526. The document from the issue offers several options to implement Advanced Markers support, this PR uses option 1 (Advanced Marker Dart class is a subclass Marker class).Summary of changes:
AdvancedMarkerclassMarkerCollisionBehaviorenum to control Advanced Marker's behavior when it collides with another markerPinConfigbitmap descriptor for customizing Advanced Marker's pin and iconmarkerTypeparameter to indicate that Advanced Markers should be usedcloudMapIdtomapIdNotes:
markerTypeoption when creating aGoogleMap(could bemarkeroradvancedMarker). Default option ismarkercloudMapIdis deprecated in favor ofmapId. New name follows SDKs, documentation and Cloud Console naming.gmaps.Markerandgmaps.AdvancedMarkerElementare not related to each other and should be handled differentlygoogle_maps_flutterstill uses them by default in this PR because of backward-compatibility. #130472 is related, package users will be able to use Advanced Markers to fix the deprecation warningResolves #155526
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.