-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Description
Is your feature request related to a problem? Please describe.
DefaultClusterRenderer currently contains two protected methods that can be overridden by applications to change the default rendering of cluster items and clusters:
- Item -
onBeforeClusterItemRendered()- Default implementation is empty (no-op) - Cluster -
onBeforeClusterRendered()- Default implementation draws a circle with a rough count of the number of items and sets it as the icon
Looking at these methods, you would think that items are rendered as markers without titles or snippets by default. However, the default item rendering actually occurs in the private method DefaultClusterRenderer.CreateMarkerTask.perform():
if (!(item.getTitle() == null) && !(item.getSnippet() == null)) {
markerOptions.title(item.getTitle());
markerOptions.snippet(item.getSnippet());
} else if (!(item.getSnippet() == null)) {
markerOptions.title(item.getSnippet());
} else if (!(item.getTitle() == null)) {
markerOptions.title(item.getTitle());
}This is strange to me for the following reasons:
- The default rendering implementation for clusters lives in a protected method while the default for items does not
- I would expect if I overrode
onBeforeClusterItemRendered()with a custom icon or ano-opthat a marker wouldn't have a title or snippet, even if that data existed in the model. However, that's not what happens - the title gets set anyway. And again, this design doesn't match the behavior for clusters.
For example, in the demo app CustomMarkerClusteringDemoActivity, onBeforeClusterItemRendered() is overridden with a custom icon and title, with the title being from another model field (the Person's name). The only reason the marker title doesn't get set twice is that the demo app is returning null from the item's getTitle() and getSnippet() methods - but that seems a bit arbitrary.
Describe the solution you'd like
Refactor the default implementation for item rendering currently in DefaultClusterRenderer.CreateMarkerTask.perform() into onBeforeClusterItemRendered().
- Pros:
- This would match the implementation of
onBeforeClusterRendered()for clusters, with the default implementation living in the protected method - This would allow applications extending
DefaultClusterRendererto inherit this default behavior along with a custom implementation via a call tosuper.onBeforeClusterItemRendered(), or eliminate it by not callingsuper.onBeforeClusterItemRendered().
- This would match the implementation of
- Cons:
- Moving this code would change the behavior of the library for apps already overriding
onBeforeClusterItemRendered()that aren't callingsuper.onBeforeClusterItemRendered()- the marker title and snippet would no longer be updated.
- Moving this code would change the behavior of the library for apps already overriding
Describe alternatives you've considered
- Leave the code as-is. Better document that default item rendering happens outside of the protected method
onBeforeClusterItemRendered(). - Move default cluster implementation out of protected method and into private method to match the default item implementation
Additional context
Discussion for this started in #627 (review), but was moved here to be handled independent of
PR #627.