Relax generic requirement on ClusterItem on clustering related interfaces

[brezinajn]

Thanks for stopping by to let us know something could be better!

---

PLEASE READ

If you have a support contract with Google, please create an issue in the support console. This will ensure a timely response.

Discover additional support services for the Google Maps Platform, including developer communities, technical guidance, and expert support at the Google Maps Platform support resources page.

If your bug or feature request is not related to this particular library, please visit the Google Maps Platform issue trackers.

Check for answers on StackOverflow with the google-maps tag.

---

Is your feature request related to a problem? Please describe.
Currently the library requires you to implement ClusterItem on your data classes. This can be problematic when you don't own those classes and/or producers of the data (or just don't wan't your model to depend on this particular library).

Describe alternatives you've considered
Creating wrapper implementing ClusterItem. This solution gets slow with large datasets/fast changing data. Additionaly it's just plain waste of memory.

Describe the solution you'd like
Remove the extends ClusterItem from interfaces like ClusterRenderer and Algorithm and thus in unopinionated manner enable other techniques of handling this problem.

[brezinajn]

ClusterManager is constrained to ClusterItem as well. But it seems it's only transitive dependency.

[arriolac]

Relaxing the APIs for clustering to accept any object, rather than an object conforming to an interface defined in the library (like ClusterItem), presents issues that changes the API in a drastic way. Specifically, the APIs could be used in ways that were not designed to (e.g. you can pass an object that contains no LatLng) and these issues would manifest as run-time errors. We can change the signatures to throw but I don't agree that this is a preferable interface.

I definitely understand the constraints you are working with but as a library, we want to strike a balance between ease-of-use, safety, and also designing something familiar.

[brezinajn]

Thanks for taking your time and considering my proposition.

We can change the signatures to throw but I don't agree that this is a preferable interface

Totally agree. Runtime error solution would be detrimental to quality of the library.

e.g. you can pass an object that contains no LatLng

This is manageable even without runtime errors though. My proposition would be to enforce LatLng, Title, Snippet by passing functions (or SAM interfaces) to ClusterManager's constructor, that would Accept T and return LatLng (and T -> Title, T -> Snippet respectively). There can be additional factory function with constraint to T extends ClusterItem. Functions from ClusterItem to LatLng are trivial. There would be only changes to the API of the constructor. You could easily migrate those by just using the above mentioned factory function.

This solution hides this new implementation detail behind the factory and thus should not have any negative effect on ease-of-use, safety or familiarity. It just optionally shifts the function implementation from the model.

Is this something you would be interested in? If you think this could be beneficial, or need some POC I can submit PR (or we can continue the debate here). If you are still convinced this isn't something the library would benefit from, please feel free to close this issue.

Edit: Fix incorrect quote

[arriolac]

Can you comment here what the API changes would look like so we can get a picture of what you intend? Doesn't need to be a full PR with implementation, just what the interface changes would look like.

[brezinajn]

Here is a gist with proposed changes.
The only changes in API were:

• removing the ClusterItem generic constraint from all interfaces
• adding getters to constructors
• providing a ClusterManager factory for T extends ClusterItem that has the same shape as current constructors.

Here is a example of using a getter instead of a function provided from an interface.

Basically the idea is to provide a proof of existence of T -> LagLng relationship by passing it as a value instead of forcing it to be implemented in the model.

[arriolac]

Thanks for the detailed gist you shared, I definitely have a clearer picture of what you mean now. To summarize, this API proposal enables clustering models that do not implement ClusterItem which is desirable when you don't own the model or when implementing the interface is undesirable for whatever reason. Creating a wrapper object is currently the workaround which has memory implications specially if you have lots of objects.

I'm certainly not against this change but my hesitation is that this is a breaking change and has implications (need to update our docs here on GitHub and on https://developers.google.com/maps/documentation/android-sdk/utility). Given that we just had a major version bump, this is something we can consider for the next major version change. If more people in the community feel strongly about this, we can reprioritize.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Please comment here if it is still valid so that we can reprioritize. Thank you!

[brezinajn]

Still valid

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Please comment here if it is still valid so that we can reprioritize. Thank you!

[brezinajn]

Still valid

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Please comment here if it is still valid so that we can reprioritize. Thank you!

[brezinajn]

Still valid

[ChainsChains]

Also valid feature request for me. Currently using wrapper

[brezinajn]

@barbeau @arriolac This feature request is slowly getting to be 2 years old. It'd be nice to have it either implemented or refused. I can provide a full PR if that'd help.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Please comment here if it is still valid so that we can reprioritize. Thank you!

[brezinajn]

Still valid.

[brezinajn]

@arriolac @barbeau There seems to be a new major release of the library without any activity on this issue. Is there anything I can do to help this situation?

[brezinajn]

There is currently a PR adding clustering capability to android-maps-compose . Under the hood, this library is used as the implementation.
It would be a good time to resolve this tech debt so that it does not affect other projects.
I can submit the proposed changes as a PR, if the change are deemed as beneficial for this library.

[DSteve595]

I'm curious to know more about these concerns:

Creating wrapper implementing ClusterItem. This solution gets slow with large datasets/fast changing data. Additionaly it's just plain waste of memory.

Have you seen evidence that this is actually slow? A wrapper object should be very lightweight, essentially just holding a reference to an object already on the heap. Fast to instantiate and takes very little memory.

I think that's worth verifying with data before making a change like that. For both Java and Kotlin, it's idiomatic to pass objects that fit an interface that provides necessary info. It's less common to pass an arbitrary object and separately pass a function for looking up that info. I personally would argue it's more surprising.

[brezinajn]

As a personal anecdote I've had a map with roughly 35000 markers. The app state was kept in a purely Kotlin module (multiplatform app), so it was not possible to persist the objects in the app state. We either had to recreate 35k of cluster items for each re-render, that was of course a huge performance hit, or keep a local state and synchronize it, making the whole process more complex and harder to maintain. It really felt like unnecessary complexity.

If needed we can of course profile both solutions. But I'd argue that performance is not the only factor here.

For both Java and Kotlin, it's idiomatic to pass objects that fit an interface that provides necessary info

For Java I agree.

It's less common to pass an arbitrary object and separately pass a function for looking up that info

For Kotlin I find this very common. As an example from stdlib you can see there are extension functions on Iterable sorted and sortedBy where sorted requires you to implement Comparable and sortedBy finds the behavior via provided lambda (additionally there is sortedWith that receives comparator). This pattern is very common in stdlib and from personal experience in Kotlin codebases overall.

I personally would argue it's more surprising.

I'm not arguing for removal of ClusterItem, or any functionality based on it. Only to enable developers not to implement a specific interface. In the gist I provided, you can see, the current behavior is kept via provided factory function, so there should be no surprise for developers.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Please comment here if it is still valid so that we can reprioritize. Thank you!

[brezinajn]

not stale

[brezinajn]

@wangela This issue is now more than 3 years old. Is there anything I can do to help this get moving?

[wangela]

We'll take a look and re-evaluate in the coming weeks.