Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[google_maps_flutter] Marker clustering support #6752

Closed
wants to merge 7 commits into from
Closed

[google_maps_flutter] Marker clustering support #6752

wants to merge 7 commits into from

Conversation

jokerttu
Copy link
Contributor

@jokerttu jokerttu commented Nov 23, 2022 •
edited
Loading

Adds the initial support for the marker clustering for Android, iOS and Web platforms.
Clustering is implemented using native google maps utils libraries for Android, iOS and Web.

Android:
google_maps_clustering_android_small

iOS:
google_maps_clustering_ios_small

Web:
google_maps_clustering_web_small

Resolves flutter/flutter#26863

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@MatijaNovosel

This comment was marked as off-topic.

Sign in to view
stuartmorgan
stuartmorgan suggested changes Dec 7, 2022
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some high level notes/questions on the Dart parts; I'll leave primary review of the platform implementations to the platform-specific owners.

...maps_flutter_platform_interface/lib/src/platform_interface/google_maps_flutter_platform.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/cluster.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/cluster.dart Outdated Show resolved Hide resolved
...oogle_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/cluster_manager.dart Outdated Show resolved Hide resolved
...es/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/info_window.dart Outdated Show resolved Hide resolved
...oogle_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/cluster_manager.dart Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/pubspec.yaml Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart Outdated Show resolved Hide resolved
...ges/google_maps_flutter/google_maps_flutter_android/lib/src/google_maps_flutter_android.dart Outdated Show resolved Hide resolved
@cedvdb
Copy link

cedvdb commented Jan 11, 2023

@jokerttu FYI one of the change requested was not marked as resolved but it was

cyanglaz
cyanglaz suggested changes Jan 19, 2023
View reviewed changes
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/ClusterManagersController.h Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/ClusterManagersController.h Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/ClusterManagersController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/ClusterManagersController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/ClusterManagersController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapMarkerController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/ClusterManagersController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/ClusterManagersController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapController.m Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapMarkerController.h Outdated Show resolved Hide resolved
cyanglaz
cyanglaz reviewed Jan 19, 2023
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for creating this PR! I looked at the iOS code it mostly looks good! I left some nits.

@reidbaker reidbaker requested review from reidbaker and removed request for GaryQian January 26, 2023 15:25
reidbaker
reidbaker reviewed Jan 26, 2023
View reviewed changes
packages/google_maps_flutter/google_maps_flutter/example/lib/clustering.dart Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/lib/clustering.dart Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/lib/clustering.dart Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/lib/clustering.dart Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/pubspec.yaml Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_android/example/lib/clustering.dart Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_android/example/pubspec.yaml Show resolved Hide resolved
...es/google_maps_flutter/google_maps_flutter_android/lib/src/google_map_inspector_android.dart Show resolved Hide resolved
...maps_flutter_platform_interface/lib/src/platform_interface/google_maps_flutter_platform.dart Show resolved Hide resolved
...maps_flutter/google_maps_flutter_platform_interface/lib/src/types/utils/cluster_manager.dart Show resolved Hide resolved
@stuartmorgan stuartmorgan self-requested a review January 27, 2023 18:44
stuartmorgan
stuartmorgan reviewed Jan 27, 2023
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a 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 cross-platform parts of this; one question above, but otherwise those aspects look good to me.

@jokerttu jokerttu force-pushed the google_maps_flutter_cluster_support branch from 4d99d2c to a8efc71 Compare February 3, 2023 10:16
TimoPieti and others added 3 commits February 8, 2023 09:41
@TimoPieti
[google_maps_flutter] Google maps flutter cluster support improvement…
35b2f4a 
…s for iOS

* Rename init method

* Improvements

* Combine marker handling

* Add documentation for cluster tap and consume click

* Improve on cluster tap handling

* Improve how markers are added or removed from cluster

* Remove public methods for single add, delete and change

* Add ref for integral zoom

* Move marker and cluster manager id handling to category

* Fix headers for podspec
@TimoPieti
[google_maps_flutter] Refactored syntax
dadf697
@jokerttu
[google_maps_flutter_android] Remove unnecessary comments
a886b6e
@jokerttu jokerttu requested review from stuartmorgan and cyanglaz and removed request for ditman and stuartmorgan February 8, 2023 09:07
@jokerttu jokerttu requested review from stuartmorgan and cyanglaz and removed request for cyanglaz and stuartmorgan February 8, 2023 09:08
@jokerttu
Copy link
Contributor Author

jokerttu commented Feb 9, 2023

All pending CRs have been now fixed.
I think this is still missing reviewers for Web (@ditman) and Android implementations?

@reidbaker
Copy link
Contributor

I think my approval counts for Android

@Ahmadre
Copy link

Ahmadre commented Feb 10, 2023 •
edited
Loading

@jokerttu we're using your PR right now and encountered that we cannot define a custom Cluster-Icon...will this also be available with this PR?

For Android for example you'll find here the implementation: https://github.com/googlemaps/android-maps-utils/blob/main/library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java#L81

@stuartmorgan
Copy link
Contributor

We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages.

Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord.

Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state!

@thedalelakes thedalelakes mentioned this pull request Apr 11, 2023
Closed
@jokerttu jokerttu mentioned this pull request Jun 27, 2023
Merged
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stuartmorgan stuartmorgan stuartmorgan requested changes

@TimoPieti TimoPieti TimoPieti left review comments

@reidbaker reidbaker reidbaker approved these changes

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

Assignees
No one assigned
Labels
p: google_maps_flutter platform-android platform-ios platform-web
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[google_maps_flutter] Support marker clustering
8 participants
@jokerttu @MatijaNovosel @cedvdb @reidbaker @Ahmadre @stuartmorgan @cyanglaz @TimoPieti