[google_maps_flutter] Add marker clustering support - android implementation #6185

jokerttu · 2024-02-23T14:32:33Z

This PR introduces support for marker clustering for Android platform

An example usage is available in the example application at ./packages/google_maps_flutter/google_maps_flutter_android/example on the page Manage clustering

This is prequel PR for: #4319
and sequel PR for: #6158

Containing only changes to google_maps_flutter_android package.

Follow up PR will hold the app-facing plugin implementation.

Linked issue: flutter/flutter#26863

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

reidbaker · 2024-03-15T15:20:14Z

Did not mean to close.

reidbaker

Partial review. Sorry you waited so long this slipped though my radar. Will continue the review tomorrow.

...r_android/android/src/main/java/io/flutter/plugins/googlemaps/ClusterManagersController.java

...flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java

camsim99

Looks great! Just left some comments about nits, tests, and naming :) Would not merge until @reidbaker gives a second review though

...r_android/android/src/main/java/io/flutter/plugins/googlemaps/ClusterManagersController.java

...flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java

...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkersController.java

...utter_android/android/src/test/java/io/flutter/plugins/googlemaps/MarkersControllerTest.java

...r_android/android/src/main/java/io/flutter/plugins/googlemaps/ClusterManagersController.java

...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java

...flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java

..._maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkerBuilder.java

...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkersController.java

...le_maps_flutter_android/android/src/test/java/io/flutter/plugins/googlemaps/ConvertTest.java

...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkersController.java

reidbaker · 2024-04-08T14:27:30Z

...ges/google_maps_flutter/google_maps_flutter_android/lib/src/google_maps_flutter_android.dart

Document/test the behavior here if onClusterTap is called with an invalid mapId

Please check how this _events method is used elsewhere in the file.
_events function just creates event matcher that matches the mapId, and creating stream with invalid mapId will just create stream that does not match any events ever.
The stream is created for the current mapId and I don't see case where the mapId could be invalid.
Testing this method would test mostly the dart Stream functionality. Not sure how this should be tested.

Ok so what you are saying is that an invalid map id is a user error and is safe to call?

Yes it is. The app facing plugin implementation is also hiding the map id from the developer interface.
If anyone uses these plarform interface implementations directly, it is user error to listen wron mapId.
Note that this whole multiplexing behaviour will be removed when this plugin is transformed to use pigeon messaging.

...ges/google_maps_flutter/google_maps_flutter_android/lib/src/utils/cluster_manager_utils.dart

...r_android/android/src/main/java/io/flutter/plugins/googlemaps/ClusterManagersController.java

...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java

..._maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkerBuilder.java

...ps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkerController.java

...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/MarkersController.java

camsim99

Left some nits but all of my comments have been addressed! Looks good :)

...r_android/android/src/main/java/io/flutter/plugins/googlemaps/ClusterManagersController.java

...ges/google_maps_flutter/google_maps_flutter_android/lib/src/utils/cluster_manager_utils.dart

camsim99

Thanks for fixing my nits! @reidbaker for second review

reidbaker · 2024-04-29T15:11:57Z

...ges/google_maps_flutter/google_maps_flutter_android/lib/src/google_maps_flutter_android.dart

Ok so what you are saying is that an invalid map id is a user error and is safe to call?

…y test the right constructor

…lutter/packages#6185)

flutter/packages@87a7c51...cc47b06 2024-04-30 [email protected] [google_maps_flutter_web] Add marker clustering support (flutter/packages#6187) 2024-04-30 [email protected] [google_maps_flutter_android] Add marker clustering support (flutter/packages#6185) 2024-04-29 [email protected] [go_router] Don't log if `hierarchicalLoggingEnabled` is `true` (flutter/packages#6019) 2024-04-29 [email protected] [file_selector_android] Update `LICENSE` file to include newly added licensed code (flutter/packages#6626) 2024-04-29 [email protected] [file_selector_android] Modifies `getDirectoryPath`, `openFile`, `openFiles` to return file/directory paths instead of URIs (flutter/packages#6438) 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 [email protected],[email protected] 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

…6185) This PR introduces support for marker clustering for Android platform An example usage is available in the example application at `./packages/google_maps_flutter/google_maps_flutter_android/example` on the page `Manage clustering` This is prequel PR for: flutter#4319 and sequel PR for: flutter#6158 Containing only changes to `google_maps_flutter_android` package. Follow up PR will hold the app-facing plugin implementation. Linked issue: flutter/flutter#26863

github-actions bot added p: google_maps_flutter platform-android labels Feb 23, 2024

jokerttu mentioned this pull request Feb 23, 2024

[google_maps_flutter] Marker clustering support #4319

Merged

11 tasks

jokerttu changed the title ~~[google_maps_flutter_android] Add marker clustering support~~ [google_maps_flutter] Add marker clustering support - android implementation Feb 29, 2024

jokerttu force-pushed the feature/google_maps_flutter_android_marker_clustering branch 2 times, most recently from 1410d4e to 8608e82 Compare March 1, 2024 06:47

jokerttu marked this pull request as ready for review March 1, 2024 07:23

jokerttu requested a review from reidbaker as a code owner March 1, 2024 07:23

jokerttu force-pushed the feature/google_maps_flutter_android_marker_clustering branch 2 times, most recently from 9444aa9 to cb542bf Compare March 7, 2024 10:01

reidbaker closed this Mar 15, 2024

reidbaker reopened this Mar 15, 2024

jokerttu force-pushed the feature/google_maps_flutter_android_marker_clustering branch from cb542bf to 78f9a40 Compare April 3, 2024 12:11

reidbaker requested a review from a team April 3, 2024 16:59

reidbaker reviewed Apr 3, 2024

View reviewed changes

camsim99 reviewed Apr 4, 2024

View reviewed changes

jokerttu force-pushed the feature/google_maps_flutter_android_marker_clustering branch 4 times, most recently from b67ab37 to 1e71fb3 Compare April 8, 2024 08:01

reidbaker reviewed Apr 8, 2024

View reviewed changes

jokerttu commented Apr 11, 2024

View reviewed changes

jokerttu requested review from camsim99 and reidbaker April 12, 2024 14:36

jokerttu force-pushed the feature/google_maps_flutter_android_marker_clustering branch from 4c3ecb0 to ff0ce89 Compare April 15, 2024 06:20

camsim99 approved these changes Apr 15, 2024

View reviewed changes

...r_android/android/src/main/java/io/flutter/plugins/googlemaps/ClusterManagersController.java Outdated Show resolved Hide resolved

...ges/google_maps_flutter/google_maps_flutter_android/lib/src/utils/cluster_manager_utils.dart Outdated Show resolved Hide resolved

jokerttu force-pushed the feature/google_maps_flutter_android_marker_clustering branch from e2df683 to 51d1517 Compare April 24, 2024 13:07

jokerttu requested a review from camsim99 April 24, 2024 17:35

camsim99 approved these changes Apr 29, 2024

View reviewed changes

reidbaker approved these changes Apr 29, 2024

View reviewed changes

jokerttu added 6 commits April 30, 2024 09:29

[google_maps_flutter_android] Add clustering support

8bfc2cf

[google_maps_flutter_android] Clustering support review fixes

21b4b28

[google_maps_flutter_android] Improve test implementations.

99fabf8

[google_maps_flutter_android] Add tests for cluster manager utils

b9b1b57

[google_maps_flutter_android] GoogleMapsController tests should mainl…

041b992

…y test the right constructor

[google_maps_flutter_android] fix review nits.

8bec3de

jokerttu force-pushed the feature/google_maps_flutter_android_marker_clustering branch from 51d1517 to 8bec3de Compare April 30, 2024 06:29

jokerttu merged commit bc6c186 into flutter:main Apr 30, 2024

engine-flutter-autoroll mentioned this pull request Apr 30, 2024

Roll Packages from 87a7c51cc633 to cc47b06b438d (5 revisions) flutter/flutter#147580

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 30, 2024

bc6c186 [google_maps_flutter_android] Add marker clustering support (f…

232fc74

…lutter/packages#6185)

reidbaker mentioned this pull request Jul 2, 2024

i151188 update espresso dependencies v2 reidbaker/packages#6

Closed

11 tasks

jokerttu deleted the feature/google_maps_flutter_android_marker_clustering branch September 9, 2024 10:44

[google_maps_flutter] Add marker clustering support - android implementation #6185

[google_maps_flutter] Add marker clustering support - android implementation #6185

Uh oh!

Conversation

jokerttu commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

reidbaker commented Mar 15, 2024

Uh oh!

reidbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camsim99 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reidbaker Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

jokerttu Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reidbaker Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

jokerttu Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

camsim99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

camsim99 left a comment

Choose a reason for hiding this comment

Uh oh!

reidbaker Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jokerttu commented Feb 23, 2024 •

edited

Loading

camsim99 left a comment •

edited

Loading

jokerttu Apr 12, 2024 •

edited

Loading

jokerttu Apr 30, 2024 •

edited

Loading