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