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

Allow a way to set options for Clustering like setMinClusterSize #388

Open
srenrd opened this issue Sep 1, 2023 · 13 comments · May be fixed by #473
Open

Allow a way to set options for Clustering like setMinClusterSize #388

srenrd opened this issue Sep 1, 2023 · 13 comments · May be fixed by #473
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@srenrd
Copy link

srenrd commented Sep 1, 2023

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

@srenrd srenrd added triage me I really want to be triaged. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Sep 1, 2023
@wangela
Copy link
Member

wangela commented Sep 1, 2023

If you would like to upvote the priority of this issue, please comment below or react on the original post above with 👍 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.

@kikoso
Copy link
Collaborator

kikoso commented Sep 29, 2023

Hi @srenrd .

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

@kikoso kikoso closed this as completed Sep 29, 2023
@wangela wangela reopened this Oct 1, 2023
@wangela wangela added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed triage me I really want to be triaged. labels Oct 1, 2023
@Jasperav
Copy link

@srenrd do you got it working? I also reported the same issue I see now (#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?

@srenrd
Copy link
Author

srenrd commented Nov 22, 2023

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

@Jasperav
Copy link

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

@kikoso
Copy link
Collaborator

kikoso commented Nov 27, 2023

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.

@yasintanriverdi
Copy link

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

@darronschall
Copy link
Contributor

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

@kikoso
Copy link
Collaborator

kikoso commented Dec 12, 2023

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.

@darronschall
Copy link
Contributor

@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
Copy link
Contributor

darronschall commented Dec 13, 2023

@kikoso What do you think about the approach I took in be54c55 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
Copy link
Contributor

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

@xxfast
Copy link

xxfast commented May 9, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
7 participants