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

feat: Implemented so that ClusterManager#setAlgorithm can be use. #421

Merged
merged 8 commits into from
Nov 17, 2023
Merged

feat: Implemented so that ClusterManager#setAlgorithm can be use. #421

merged 8 commits into from
Nov 17, 2023

Conversation

matsudamper
Copy link
Contributor

@matsudamper matsudamper commented Oct 4, 2023

Existing problems

  • ClusterManager#setAlgorithm is not available.
  • clusterContent and clusterItemContent are not used when clusterRenderer is specified.

As described in the Issue, we thought it would be less confusing for users to separate the low-level API and the high-level API.

Clustering Composable Function that takes a clusterManager as an argument, memberClusterRenderer and memberClusterManager are implemented and make public.
The existing Clustering Composable Function calls the low-level Clustering Composable Function.


Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #420 🦕

@matsudamper matsudamper changed the title Implemented so that ClusterManager can be use. Implemented so that ClusterManager#setAlgorithm can be use. Oct 4, 2023
@wangela wangela requested a review from kikoso October 6, 2023 16:14
@matsudamper matsudamper changed the title Implemented so that ClusterManager#setAlgorithm can be use. feat: Implemented so that ClusterManager#setAlgorithm can be use. Oct 10, 2023
@kikoso
Copy link
Collaborator

kikoso commented Nov 9, 2023

Hello @matsudamper . I like the idea of having a lean API to pass the ClusterManager and set the algorithm directly. This can be arguably a reasonable use case.

However, this PR introduces one issue. You can quickly see it by running the Marker Clustering sample, and see that one marker always remains with the background of the shape from the Clustering.

Can you check it out? Thanks!

@matsudamper
Copy link
Contributor Author

matsudamper commented Nov 13, 2023

Even if you branch Clustering with if, the marker remains displayed. This issue may be related.
investigating...

@matsudamper
Copy link
Contributor Author

matsudamper commented Nov 13, 2023

fixed. Please confirm.

Comment on lines +206 to +210
SideEffect {
if (clusterManager?.renderer != renderer) {
clusterManager?.renderer = renderer ?: return@SideEffect
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the high-level APIs, Processing is performed in Clustering Composable.
For the low-level API, I wondered what to do, but did not include it. Because I may be some inconvenience I'm not aware of.

@matsudamper
Copy link
Contributor Author

java.lang.IllegalStateException: Maps API key not specified
https://github.com/googlemaps/android-maps-compose/actions/runs/6847027102/job/18629617389?pr=421

The test was successful in my environment. I didn't know the cause.

@kikoso
Copy link
Collaborator

kikoso commented Nov 13, 2023

Hi @matsudamper ,

Do not worry about it, currently the Map Key is not being injected into external contributions.

@kikoso
Copy link
Collaborator

kikoso commented Nov 17, 2023

LGTM

@wangela
Copy link
Member

wangela commented Nov 17, 2023

This will also help us customize the algorithm for developing and testing fixes to #380

Copy link
Member

@wangela wangela left a comment

Choose a reason for hiding this comment

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

Thank you for contributing this feature, @matsudamper!

@wangela wangela merged commit b892e61 into googlemaps:main Nov 17, 2023
8 of 9 checks passed
googlemaps-bot pushed a commit that referenced this pull request Nov 17, 2023
# [4.3.0](v4.2.0...v4.3.0) (2023-11-17)

### Features

* expose the ClusterManager to support custom algorithms ([#421](#421)) ([b892e61](b892e61))
@googlemaps-bot
Copy link
Contributor

🎉 This PR is included in version 4.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allows use of ClusterManager#setAlgorithm
4 participants