Skip to content
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

[google_maps_flutter] Marker clustering support #4319

Merged

Conversation

jokerttu
Copy link
Contributor

@jokerttu jokerttu commented Jun 27, 2023

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.

This PR is created from previous PR: flutter/plugins#6752

Resolves flutter/flutter#26863

Android:
image

iOS:
image

Web:
image

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/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package 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.

@Hixie
Copy link
Contributor

Hixie commented Aug 22, 2023

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

@thedalelakes
Copy link

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

@jokerttu
Copy link
Contributor Author

@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 jokerttu force-pushed the google_maps_flutter_cluster_support branch 5 times, most recently from b44d05a to e052a4c Compare October 26, 2023 12:33
@jokerttu jokerttu marked this pull request as ready for review October 27, 2023 14:52
@wangela
Copy link

wangela commented Oct 30, 2023

@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?

@jokerttu
Copy link
Contributor Author

jokerttu commented Oct 31, 2023

Note: that the original PR was reviewed at plugins repository at flutter/plugins#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

@stuartmorgan
Copy link
Contributor

@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.

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.

Thanks for your patience on this; 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.

I should have the rest of the re-review done later today but I wanted to get this part out first so you could work on that in parallel.

@stuartmorgan
Copy link
Contributor

@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.

@apackin
Copy link

apackin commented Jan 16, 2024

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

@hellohuanlin
Copy link
Contributor

@jokerttu is this ready for another review?

@jokerttu
Copy link
Contributor Author

jokerttu commented Jan 18, 2024

@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: (#4319 (comment))

@jokerttu jokerttu force-pushed the google_maps_flutter_cluster_support branch from 850bf1e to bf56884 Compare August 6, 2024 19:58
auto-submit bot pushed a commit that referenced this pull request Aug 6, 2024
…ion (#6186)

This PR introduces support for marker clustering for iOS platform

An example usabe is available in the example application at ./packages/google_maps_flutter/google_maps_flutter_ios/example/ios12 on the page `Manage clustering`

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

Containing only changes to `google_maps_flutter_ios` package.

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

Linked issue: flutter/flutter#26863
@jokerttu jokerttu force-pushed the google_maps_flutter_cluster_support branch from bf56884 to 0d9224c Compare August 6, 2024 20:06
@jokerttu jokerttu marked this pull request as ready for review August 6, 2024 20:47
@flutter-dashboard

This comment was marked as resolved.

@jokerttu jokerttu force-pushed the google_maps_flutter_cluster_support branch 2 times, most recently from d5d476a to 19f7ea4 Compare August 7, 2024 06:32
@jokerttu jokerttu force-pushed the google_maps_flutter_cluster_support branch from 19f7ea4 to 7632a2e Compare August 7, 2024 09:10
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.

LGTM!

@jokerttu jokerttu added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 7, 2024
@auto-submit auto-submit bot merged commit 244cd4c into flutter:main Aug 7, 2024
76 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 7, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 7, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 7, 2024
flutter/packages@551bde5...5cc0a01

2024-08-07 stuartmorgan@google.com [ci] Update repository for the release of Flutter 3.24 (flutter/packages#7331)
2024-08-07 engine-flutter-autoroll@skia.org Roll Flutter (stable) from b0850be to 80c2e84 (1397 revisions) (flutter/packages#7322)
2024-08-07 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.5 to 4.3.6 (flutter/packages#7330)
2024-08-07 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump io.mockk:mockk from 1.13.7 to 1.13.12 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#7189)
2024-08-07 joonas.kerttula@codemate.com [google_maps_flutter] Marker clustering support (flutter/packages#4319)
2024-08-07 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.15 to 3.26.0 (flutter/packages#7329)
2024-08-07 engine-flutter-autoroll@skia.org Manual roll Flutter from 1dd7141 to 0a7f8af (23 revisions) (flutter/packages#7328)
2024-08-06 magder@google.com [image_picker_ios] Update image picker UI test query for iOS 18 (flutter/packages#7325)
2024-08-06 joonas.kerttula@codemate.com [google_maps_flutter] Add marker clustering support - iOS implementation (flutter/packages#6186)
2024-08-06 8014077+Frank3K@users.noreply.github.com [url_launcher] launchUrl always returns true for valid schemes on the web. (flutter/packages#7229)
2024-08-06 rexios@rexios.dev [google_maps_flutter] Add heatmap support (flutter/packages#3257)
2024-08-06 stuartmorgan@google.com [local_auth] Endorse macOS (flutter/packages#7274)

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,rmistry@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
TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this pull request Aug 7, 2024
…er#153040)

flutter/packages@551bde5...5cc0a01

2024-08-07 stuartmorgan@google.com [ci] Update repository for the release of Flutter 3.24 (flutter/packages#7331)
2024-08-07 engine-flutter-autoroll@skia.org Roll Flutter (stable) from b0850be to 80c2e84 (1397 revisions) (flutter/packages#7322)
2024-08-07 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.5 to 4.3.6 (flutter/packages#7330)
2024-08-07 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump io.mockk:mockk from 1.13.7 to 1.13.12 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#7189)
2024-08-07 joonas.kerttula@codemate.com [google_maps_flutter] Marker clustering support (flutter/packages#4319)
2024-08-07 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.15 to 3.26.0 (flutter/packages#7329)
2024-08-07 engine-flutter-autoroll@skia.org Manual roll Flutter from 1dd7141 to 0a7f8af (23 revisions) (flutter/packages#7328)
2024-08-06 magder@google.com [image_picker_ios] Update image picker UI test query for iOS 18 (flutter/packages#7325)
2024-08-06 joonas.kerttula@codemate.com [google_maps_flutter] Add marker clustering support - iOS implementation (flutter/packages#6186)
2024-08-06 8014077+Frank3K@users.noreply.github.com [url_launcher] launchUrl always returns true for valid schemes on the web. (flutter/packages#7229)
2024-08-06 rexios@rexios.dev [google_maps_flutter] Add heatmap support (flutter/packages#3257)
2024-08-06 stuartmorgan@google.com [local_auth] Endorse macOS (flutter/packages#7274)

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,rmistry@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
@Rishyash

This comment was marked as off-topic.

@stuartmorgan

This comment was marked as off-topic.

@Rishyash

This comment was marked as off-topic.

@Mohit8G

This comment was marked as off-topic.

@stuartmorgan

This comment was marked as off-topic.

@stuartmorgan

This comment was marked as off-topic.

@flutter flutter locked as off-topic and limited conversation to collaborators Aug 8, 2024
@stuartmorgan
Copy link
Contributor

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.

@jokerttu jokerttu deleted the google_maps_flutter_cluster_support branch September 9, 2024 10:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App federated: all_changes PR that contains changes for all packages for a federated plugin change p: google_maps_flutter platform-web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[google_maps_flutter] Support marker clustering