Allow a way to set options for Clustering like setMinClusterSize
I'm really missing a way to customize the MinClusterSize on the Clustering composable.
If you would like to upvote the priority of this issue, please comment below or react on the original post above with :+1: so we can see what is popular when we triage.
@srenrd Thank you for opening this issue. 🙏 Please check out these other resources that might help you get to a resolution in the meantime:
- Check the issue tracker - bugs and feature requests for Google Maps Platform APIs and SDKs
- Open a support case - Get 1:1 support in Cloud Console.
- Discord - chat with other developers
-
StackOverflow - use the
google-mapstag
This is an automated message, feel free to ignore.
Hi @srenrd .
The following PR will allow using custom renderers. This will allow you to use setMinClusterSize. Please stay tuned!
@srenrd do you got it working? I also reported the same issue I see now (https://github.com/googlemaps/android-maps-compose/issues/443) but the response was that I should write my own renderer which looks huge overkill for setting such a simple configuration, or do you have anything that could work?
@Jasperav no i've yet to create a custom manager. Like you, it seems overkill that if you want to customize a single property you have to customize everything.
@kikoso Can you please respond to this what the most easiest way is now with the linked PR? As far as I can see is that I have to write my complete renderer while I just want to customize 1 single property on the DefaultRenderer.
Hi @Jasperav .
Depending on the property you want to modify, a custom Renderer or a Manager could serve you.
I understand it can feel like overkill, but on the other hand, exposing internal properties on the Clustering() Composable it is arguably a good practice. We also want to keep things in sync with the android-maps-utils, since the Clustering() Composable relies internally on it.
On the other hand, arguably a Compose API could benefit from having the minSdk as an extra parameter. The previous android-maps-utils is, for obvious reasons, not following a Compose/Declarative approach. We also have already some minor misalignment (for instance, passing the click listeners as lambdas).
Let me create a PR where we can discuss this.
@kikoso any update on this? Is there any target to add such an option?
@kikoso I submitted #473 for discussion around this issue. When using the default renderer, it made sense to me allow an optional minClusterSize on Clustering. I created an overload for Clustering to add the param to avoid breaking existing API:
Clustering(
items = items,
minClusterSize = 3,
// ...
)
I'm not sure if this is what you were thinking or not... just trying to help things along.
Hi @darronschall ,
One counterargument against this is that we are expanding the Clustering() interface with items that do not fully belong to the Clustering itself, but to the renderer.
This in its own does not pose any risk, but the problem here is to draw a line. Is the minClusterSize enough? Should we add the initial zoom, or the listeners?
I am tempted to say that the best solution would be to refactor the underlying android-maps-compose and move the minClusterSize management into the ClusterRenderer. This would be a breaking change in the android-maps-utils for any folks implementing their custom Renderer, but would cover the minClusterSize in the default ones offered by android-maps-utils, since currently we only have two renderers: DefaultClusterRenderer and DefaultAdvancedMarkersClusterRenderer.
Then, when this is implemented, we could do this on the clusterManager:
clusterManager.renderer.setMinClusterSize(4)
If this sounds good I can put a PR together to discuss it.
@kikoso I suspect the common case is using Clustering with clusterContent and clusterItemContent set, and letting the library provide the default manager and renderer. That's why I opted for adding minClusterSize to Clustering itself via an override. (Confession: I've only started using this library recently.)
Unfortunately, both DefaultClusterRenderer and DefaultAdvancedMarkersClusterRenderer are unaware of compose content, so leveraging ComposeUiClusterRenderer is the only option when using compose content for clusters.
I agree with your concerns on where to draw the customization line.
I think the pain point here is that it's hard to get a handle on the default renderer to change it, and it's also hard to create and use a custom renderer. There isn't a combination of both Clustering and rememberClusterRenderer that make call-site usage obvious.
The Clustering that allows setting clusterRenderer is deprecated.
The rememberClusterRenderer can create the ComposeUiClusterRenderer that we can customize (after casting to DefaultClusterRenderer) but which version of Clustering should downstream users leverage? (The overload version that allows setting the clusterManager).
All of that said, the workaround for this bug is really just piecing together the different parts of the library in perhaps a non-obvious-to-newcomers way. It's not a bug in the code, it's a "bug" in the current API design; it's possible to set minClusterSize, but, again, it's not obvious or easy.
For example, to change the existing the CustomUiClustering example to set the minClusterSize, the example needs to be rewritten as:
val clusterManager = rememberClusterManager<MyItem>()
val clusterRenderer = rememberClusterRenderer(
clusterContent = { cluster ->
CircleContent(
modifier = Modifier.size(40.dp),
text = "%,d".format(cluster.size),
color = Color.Blue,
)
},
clusterItemContent = null,
clusterManager = clusterManager,
) as? DefaultClusterRenderer
LaunchedEffect(clusterManager, clusterRenderer) {
clusterRenderer?.let {
clusterRenderer.minClusterSize = 2 // <--- here!
clusterManager?.renderer = clusterRenderer
clusterManager?.setOnClusterClickListener {
Log.d(TAG, "Cluster clicked! $it")
false
}
clusterManager?.setOnClusterItemClickListener {
Log.d(TAG, "Cluster item clicked! $it")
false
}
clusterManager?.setOnClusterItemInfoWindowClickListener {
Log.d(TAG, "Cluster item info window clicked! $it")
}
}
}
clusterManager?.let {
Clustering(
items = items,
clusterManager = clusterManager,
)
}
... that's a lot of effort just for that one clusterRenderer.minClusterSize = 2 line in there, and we end up very similar to the CustomRendererClustering example, when all we wanted was to customize the default renderer a little.
I don't have a strong opinion on a path forward, but I lean towards making Clustering easier to use, especially for newcomers. Mostly, I just wanted to kick-start the discussion. I'm curious to see what your PR might look like!
@kikoso What do you think about the approach I took in https://github.com/googlemaps/android-maps-compose/pull/473/commits/be54c5507448ec93c85ca5f672ad98d52938c618 instead?
Rather than clutter Clustering with various params like minClusterSize, I introduced an optional onClusterManager that accepts the clusterManager as a param. This block is called after the clusterManager and renderer are created and set up by the library, allowing easy and obvious customization by library users.
I updated CustomUiClustering in MarkerClusteringActivity in the example with this approach:
Clustering(
items = items,
// Optional: Handle clicks on clusters, cluster items, and cluster item info windows
onClusterClick = {
Log.d(TAG, "Cluster clicked! $it")
false
},
onClusterItemClick = {
Log.d(TAG, "Cluster item clicked! $it")
false
},
onClusterItemInfoWindowClick = {
Log.d(TAG, "Cluster item info window clicked! $it")
},
// Optional: Custom rendering for clusters
clusterContent = { cluster ->
CircleContent(
modifier = Modifier.size(40.dp),
text = "%,d".format(cluster.size),
color = Color.Blue,
)
},
// Optional: Custom rendering for non-clustered items
clusterItemContent = null,
onClusterManager = { clusterManager ->
(clusterManager.renderer as DefaultClusterRenderer).minClusterSize = 2
},
)
👍 / 👎? Was this more in line with what you had in mind?
@kikoso It's been a little while since there was movement on this issue... I rebased and resolved conflicts in #473 in hopes of either getting it merged, or to kick-start discussion here again. It'd be great to get your feedback when you have some availability! Thanks. :-)
The fix from @darronschall looks pretty good 👍 Would we be able to prioritise this?
:tada: This issue has been resolved in version 6.4.0 :tada:
The release is available on:
-
v6.4.0 - GitHub release
Your semantic-release bot :package::rocket: