android-maps-compose icon indicating copy to clipboard operation
android-maps-compose copied to clipboard

Allow a way to set options for Clustering like setMinClusterSize

Open srenrd opened this issue 2 years ago • 13 comments

I'm really missing a way to customize the MinClusterSize on the Clustering composable.

srenrd avatar Sep 01 '23 09:09 srenrd

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:

This is an automated message, feel free to ignore.

wangela avatar Sep 01 '23 09:09 wangela

Hi @srenrd .

The following PR will allow using custom renderers. This will allow you to use setMinClusterSize. Please stay tuned!

kikoso avatar Sep 29 '23 10:09 kikoso

@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 avatar Nov 21 '23 19:11 Jasperav

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

srenrd avatar Nov 22 '23 08:11 srenrd

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

Jasperav avatar Nov 25 '23 18:11 Jasperav

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 avatar Nov 27 '23 19:11 kikoso

@kikoso any update on this? Is there any target to add such an option?

yasintanriverdi avatar Dec 11 '23 11:12 yasintanriverdi

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

darronschall avatar Dec 12 '23 16:12 darronschall

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 avatar Dec 12 '23 20:12 kikoso

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

darronschall avatar Dec 12 '23 22:12 darronschall

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

darronschall avatar Dec 13 '23 14:12 darronschall

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

darronschall avatar Feb 15 '24 14:02 darronschall

The fix from @darronschall looks pretty good 👍 Would we be able to prioritise this?

xxfast avatar May 09 '24 01:05 xxfast

:tada: This issue has been resolved in version 6.4.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

googlemaps-bot avatar Dec 06 '24 17:12 googlemaps-bot