[google_maps_flutter_android] Batch clustered marker operations#10940
[google_maps_flutter_android] Batch clustered marker operations#10940auto-submit[bot] merged 7 commits intoflutter:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces batch operations for clustered markers to improve performance, which is a great enhancement. The changes involve adding addItems() and removeItems() methods to ClusterManagersController and refactoring MarkersController to group marker operations by cluster manager and execute them in batches. The new logic is well-tested with new unit tests. My review includes a few suggestions to use Map.computeIfAbsent in MarkersController for more concise and modern Java code.
...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkersController.java
Outdated
Show resolved
Hide resolved
...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkersController.java
Outdated
Show resolved
Hide resolved
...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkersController.java
Outdated
Show resolved
Hide resolved
...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkersController.java
Outdated
Show resolved
Hide resolved
22e3623 to
22037dc
Compare
| } | ||
|
|
||
| /** Removes multiple items from the ClusterManager with the given ID. */ | ||
| public void removeItems(String clusterManagerId, List<MarkerBuilder> items) { |
| } | ||
|
|
||
| /** Adds multiple items to the ClusterManager with the given ID. */ | ||
| public void addItems(String clusterManagerId, List<MarkerBuilder> items) { |
| public void removeItems(String clusterManagerId, List<MarkerBuilder> items) { | ||
| ClusterManager<MarkerBuilder> clusterManager = clusterManagerIdToManager.get(clusterManagerId); | ||
| if (clusterManager != null) { | ||
| clusterManager.removeItems(items); |
There was a problem hiding this comment.
What does remove items do if one of the items is not present?
There was a problem hiding this comment.
ClusterManager.removeItems() from the maps-utils library silently ignores any items that aren't present. It's a no-op for missing items (returns false per item but doesn't throw).
This is consistent with the behavior of the existing single-item removeItem() method, which also makes no prior existence check before delegating to the underlying ClusterManager.
|
|
||
| Mockito.verify(spyMarkerCollection, times(1)).remove(marker); | ||
| } | ||
|
|
There was a problem hiding this comment.
Would you be willing to add a test that handles updating an existing marker in place. (starting at line 138 in packages/google_maps_flutter/google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkersController.java Or point to where that code is executed in the tests you have written if I missed it.
There was a problem hiding this comment.
There's now a new controller_ChangeMarkerInPlace test for this.
|
I should have led with Thank you for your contribution |
|
I'll see if I can get this rebased and add the requested items sometime later this week. Thanks |
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I'll wait on a final review until Reid's review comments have been incorporated, but from an initial review everything looks good to me modulo those comments.
4752f7b to
26401f7
Compare
|
I'm still working on this one, will update once I've made all my corrections. |
26401f7 to
eb6deaa
Compare
- Incorporate upstream advanced markers support (v2.19.0–2.19.2): PlatformMarkerType, MarkerClusterRenderer/AdvancedMarkerClusterRenderer, 3-arg MarkerBuilder constructor, isAdvancedMarkersAvailable(), etc. - Keep batch add/remove logic in ClusterManagersController and MarkersController - Add @nonnull to addItems/removeItems parameters (r2823177045, r2823177715) - Add controller_ChangeMarkerInPlace test (r2823210221) - Bump version to 2.19.3
eb6deaa to
b043794
Compare
|
@reidbaker I've made the requested changes and rebased on top of the latest code. Thanks |
|
Oops, sorry @reidbaker got trigger happy with the re-review button. |
|
Hi! Just checking in to see if you need anything else from me for this one. Thanks! |
|
Sorry for the delay! Approved |
|
auto label is removed for flutter/packages/10940, Failed to merge flutter/packages/10940 with Pull request flutter/packages/10940 could not be merged: Required status check "ci.yaml validation" is expected.. |
flutter/packages@afa1a1c...99155a8 2026-03-19 engine-flutter-autoroll@skia.org Roll Flutter from d117642 to dd64978 (24 revisions) (flutter/packages#11281) 2026-03-18 stuartmorgan@google.com [local_auth] Convert to Kotlin gradle for the plugin build files (flutter/packages#11169) 2026-03-18 stuartmorgan@google.com [google_maps_flutter] Add color scheme support to app-facing package (flutter/packages#11280) 2026-03-18 10687576+bparrishMines@users.noreply.github.com [webview_flutter_wkwebview] Updates platform views on iOS to only have a weak reference to the native view (flutter/packages#11175) 2026-03-18 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump androidx.core:core from 1.17.0 to 1.18.0 in /packages/local_auth/local_auth_android/android (flutter/packages#11256) 2026-03-18 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump androidx.core:core-ktx from 1.13.0 to 1.18.0 in /packages/interactive_media_ads/android (flutter/packages#11255) 2026-03-18 engine-flutter-autoroll@skia.org Roll Flutter (stable) from ff37bef to 2c9eb20 (6 revisions) (flutter/packages#11285) 2026-03-18 stuartmorgan@google.com [google_maps_flutter] Add color scheme support to web implementation (flutter/packages#11279) 2026-03-18 elitree@gmail.com [google_maps_flutter_android] Batch clustered marker operations (flutter/packages#10940) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Note: this PR attempts to use the same strategy from the Google Maps Flutter Web PR #10562 and apply it to Google Maps Flutter Android.
Summary
addItems()andremoveItems()methods toClusterManagersControllerMarkersControllerto batch add/remove/change operations for clustered markersThis improves performance when working with large numbers of clustered markers by calling
ClusterManager.cluster()once per cluster manager instead of once per marker.Test plan
Addresses flutter/flutter#170573
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.🤖 Generated with Claude Code
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