packages icon indicating copy to clipboard operation
packages copied to clipboard

[google_maps_flutter] Marker clustering support

Open jokerttu opened this issue 1 year ago • 22 comments

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.

The Google Maps Utils libraries provides the capability for future additions of features available under these libraries to the google_maps_flutter plugin, such as heatmaps, although they are not included in this PR.

This PR is created from previous PR: https://github.com/flutter/plugins/pull/6752

Resolves https://github.com/flutter/flutter/issues/26863

Android: image

iOS: image

Web: image

Pre-launch Checklist

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

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

jokerttu avatar Jun 27 '23 07:06 jokerttu

@jokerttu Is this still something you are interested in working on? How can we help you?

Hixie avatar Aug 22 '23 22:08 Hixie

@jokerttu is this still something you're working on?

thedalelakes avatar Oct 05 '23 15:10 thedalelakes

@jokerttu is this still something you're working on?

Yes, I am scheduled to work on this issue next week and will update the PR accordingly in the near future. The recent architectural changes in the google_maps_flutter_web implementation have affected the integration tests, which I am addressing. Additionally, some new methods introduced in this PR will likely require updates to the native unit tests.

jokerttu avatar Oct 13 '23 13:10 jokerttu

@stuartmorgan this is ready for review. We inspected the logs for the failing web platform tests and it seems to be reporting based on Android?

wangela avatar Oct 30 '23 15:10 wangela

Note: that the original PR was reviewed at plugins repository at https://github.com/flutter/plugins/pull/6752 Implementation parts have been mainly same, but small fixes and changes has been done to fix conflicts. Web tests are rewritten because of changes on the web implementation (web plugin was endorsed). iOS Google Maps Utils library version is now pinned to 4.1.0 as newer versions do not support iOS platform versions 11 and 12

jokerttu avatar Oct 31 '23 08:10 jokerttu

@stuartmorgan this is ready for review. We inspected the logs for the failing web platform tests and it seems to be reporting based on Android?

This is because there is a weird circular dependency in the tests where google_maps_flutter_web's example depends on google_maps, which then is depending on a non-overridden version of google_maps_flutter_android so things no longer match. Adding a dependency override for _android in google_maps_flutter_web/example/pubspec.yaml should allow the tests to run.

stuartmorgan avatar Nov 17 '23 12:11 stuartmorgan

@ditman Looks like the original never got your review for web; if you could take a look here when you get a chance that would be great.

stuartmorgan avatar Nov 17 '23 13:11 stuartmorgan

Any progress updates on when we can expect this to come out?

apackin avatar Jan 16 '24 18:01 apackin

@jokerttu is this ready for another review?

hellohuanlin avatar Jan 17 '24 21:01 hellohuanlin

@hellohuanlin

@jokerttu is this ready for another review?

Not yet, most of the issues @stuartmorgan pointed out are fixed, but there are still few that are not yet done. Also, before this can be finalized, we need also some resolution to this question waiting review from @ditman: (https://github.com/flutter/packages/pull/4319#discussion_r1424267362)

jokerttu avatar Jan 18 '24 18:01 jokerttu

@stuartmorgan @ditman I've addressed most of the review comments and made the corresponding updates to the PR (with help of @illuminati1911). However, a few discussions remain open, and I'm actively seeking further input on those.

Regarding the web implementation, I encountered challenges with using @staticInterop annotations for methods that involve google-maps interop objects as parameters. Since google-maps objects do not utilize @staticInterop, it appears we cannot proceed in this direction for the time being. I'm open to suggestions or alternative approaches to resolve this issue.

(Edit: this PR updates the google_maps package, but it does not seem to use @staticInterop so that might not help with this at all)

jokerttu avatar Feb 14 '24 20:02 jokerttu

I've finished reviewing the platform interface, and since the overall structure was already reviewed in the previous PR you are good to split that out into the first prequel PR. I've left some comments that you can either address here before splitting, or just address in the new PR.

It looks like this still hasn't been split out?

stuartmorgan avatar Feb 16 '24 18:02 stuartmorgan

@stuartmorgan

It looks like this still hasn't been split out?

The review comments were resolved in this PR before splitting so that the platform-specific implementations would not stretch too far in time after the platform interface update.

Here is the first prequel PR for google_maps_flutter_platform_interface: https://github.com/flutter/packages/pull/6158

jokerttu avatar Feb 19 '24 08:02 jokerttu

However, a few discussions remain open, and I'm actively seeking further input on those.

Was there anything other than web-specific questions? I looked back through the comments and didn't find any unresolved discussions with questions other than the web ones, but I may have missed something.

stuartmorgan avatar Feb 20 '24 19:02 stuartmorgan

@stuartmorgan

However, a few discussions remain open, and I'm actively seeking further input on those.

Was there anything other than web-specific questions? I looked back through the comments and didn't find any unresolved discussions with questions other than the web ones, but I may have missed something.

Yes, checked and only web-specific discussions are still open.

jokerttu avatar Feb 21 '24 13:02 jokerttu

@stuartmorgan added prequel PR:s for platforms as well as draft, as the platform_interface (https://github.com/flutter/packages/pull/6158) PR has not yet landed: Android: https://github.com/flutter/packages/pull/6185 iOS: https://github.com/flutter/packages/pull/6186 Web: https://github.com/flutter/packages/pull/6187 (@ditman)

jokerttu avatar Feb 23 '24 14:02 jokerttu

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

flutter-dashboard[bot] avatar Mar 01 '24 13:03 flutter-dashboard[bot]

Converted to draft to avoid unnecessary reviews before the platform implementations are landed.

jokerttu avatar Mar 01 '24 13:03 jokerttu

@jokerttu any updates on this 🥺 Really looking forward to it. Really appreciate you for taking this on!

ParkBud avatar Apr 04 '24 22:04 ParkBud

@jokerttu any updates on this 🥺 Really looking forward to it. Really appreciate you for taking this on!

The work is ongoing on platform implementation PR:s ~https://github.com/flutter/packages/pull/6185~ (merged) ~https://github.com/flutter/packages/pull/6186~ (merged) ~https://github.com/flutter/packages/pull/6187~ (merged)

jokerttu avatar Apr 05 '24 10:04 jokerttu

Status update from triage: Blocked on landing the iOS platform implementation, which is turn blocked on Google Maps SDK-level issues.

stuartmorgan avatar Jun 04 '24 19:06 stuartmorgan

Any update?

Mohit8G avatar Jun 13 '24 14:06 Mohit8G

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

flutter-dashboard[bot] avatar Aug 06 '24 23:08 flutter-dashboard[bot]

https://medium.com/@rishyash8/clustering-support-in-google-map-flutter-2024-d8b1c68e515b

Basic implementation as example code is not updated in google_map_flutter on pub.dev

Rishyash avatar Aug 08 '24 10:08 Rishyash

Pub.dev only shows a single file in the example tab, and the example app for this package is quite large; all the individual features are on sub-pages, and clustering is no different. The example app can be viewed or run locally to see specific features.

stuartmorgan avatar Aug 08 '24 11:08 stuartmorgan

Pub.dev only shows a single file in the example tab, and the example app for this package is quite large; all the individual features are on sub-pages, and clustering is no different. The example app can be viewed or run locally to see specific features.

yep, but the example which was present in the package was quite complex to understand as a beginner so created a simple example for that.

Rishyash avatar Aug 08 '24 11:08 Rishyash

Hey Everyone, just gone through whole code for clustering part for flutter, cant find Custom Cluster Renderer for customising the cluster icon. Any update on that, like are we picking it in near future.

Mohit8G avatar Aug 08 '24 11:08 Mohit8G

@Mohit8G This is a PR that has landed, so it cannot be changed. If you have follow-up feature requests, please file them in the issue tracker.

stuartmorgan avatar Aug 08 '24 11:08 stuartmorgan

yep, but the example which was present in the package was quite complex to understand as a beginner so created a simple example for that.

"I wanted to make a simpler example for beginners" and "the updated example code was not published to pub.dev" are very different statements; the latter would have indicated some kind of serious bug in the release process. Please don't post to a PR to promote your post but describe it as if it were a bug in the PR or release.

stuartmorgan avatar Aug 08 '24 11:08 stuartmorgan

Temporarily locking, as this is attracting a lot of off-topic comments. Discussion on a PR should be about the code in the PR, not things like follow-up requests.

stuartmorgan avatar Aug 08 '24 11:08 stuartmorgan