android-maps-utils Refactor ClusterManager to call map.setOnCameraIdleListener internally

Is your feature request related to a problem? Please describe. Mentioned in https://github.com/googlemaps/android-maps-ktx/pull/29#discussion_r402459201.

All of the clustering demos currently look like this:

clusterManager = new ClusterManager<>(this, map);
map.setOnCameraIdleListener(clusterManager);

...which I believe aligns with the majority of use cases of ClusterManager for dynamic clustering based on the map viewport. If a developer doesn't include the 2nd line of code, the clusters won't be re-clustered when the map viewport changes and settles.

Describe the solution you'd like To make it easier for developers, we could move the registration of GoogleMap.OnCameraIdleListener within ClusterManager to avoid apps having to add this code themselves.

However, I do see use cases for static clustering, where an app wants to show markers and clustering, but not have this change based on the map viewport. If we do change to registering the GoogleMap.OnCameraIdleListener by default, this would be a breaking change for anyone doing static clustering.

So, a new implementation would need to:

Support both dynamic (listening to the camera) and static (not listening to the camera) clustering
Be a major version release due to breaking behavior

Feedback from developers on how they use ClusterManager is welcome! Especially whether you always call map.setOnCameraIdleListener(clusterManager);.

Describe alternatives you've considered Do nothing - leave code as-is

Additional context Mentioned in https://github.com/googlemaps/android-maps-ktx/pull/29#discussion_r402459201

Apr 03 '20 16:04 barbeau

Some thoughts on the approaches to take:

ClusterManager internally sets itself as OnCameraIdleListener If ClusterManager remains to implement OnCameraIdleListener, it should set itself as a the OnCameraIdleListener to the provided GoogleMap. Otherwise, it's a leaky abstraction because the caller needs to know that ClusterManager implements OnCameraIdleListener.

One improvement that can be made related to this is to add an OnCameraIdleListener property within ClusterManager that can be set via setOnCameraIdleListener. So, the implementation of onCameraIdle() with ClusterManager would change to:

@Override public void onCameraIdle() {
    if (mRenderer instanceof GoogleMap.OnCameraIdleListener) {
        ((GoogleMap.OnCameraIdleListener) mRenderer).onCameraIdle();
    }
    mAlgorithm.onCameraChange(mMap.getCameraPosition());
    
    // delegate clustering to the algorithm
    if (mAlgorithm.shouldReclusterOnMapMovement()) {
        cluster();
        // Don't re-compute clusters if the map has just been panned/tilted/rotated.
    } else if (mPreviousCameraPosition == null || mPreviousCameraPosition.zoom != mMap.getCameraPosition().zoom) {
        mPreviousCameraPosition = mMap.getCameraPosition();
        cluster();
    }

    // This will be added
    if (onCameraIdleListener != null) {
        onCameraIdleListener.onCameraIdle();
    }
}

This allows clients to have additional custom behavior when the camera goes idle. Currently, if you want to add a custom handler for when the camera goes idle you will have to set a listener on the GoogleMap object and then call the onCameraIdle() method on ClusterManager.

map.setOnCameraIdleListener(new GoogleMap.OnCameraIdleListener() {
    @Override
    public void onCameraIdle() { 
        // Add your custom handler here
        clusterManager.onCameraIdle();           
    }
});

Remove conformance to OnCameraIdleListener: The other approach is to remove conformance to OnCameraIdleListener on ClusterManager and rename the method onCameraIdle() to something else. This defaults ClusterManager to static clustering and reclustering on a camera move needs to be handled by the user.

My take: We should probably bias towards the common use-case which you alluded to—dynamic clustering. If static clustering is desired, we can document that or even provide another example in the demo app for it.

Apr 09 '20 23:04 arriolac

@barbeau any thoughts on the suggestion above?

Apr 27 '20 15:04 arriolac

One improvement that can be made related to this is to add an OnCameraIdleListener property within ClusterManager that can be set via setOnCameraIdleListener.

I agree with this improvement 👍

We should probably bias towards the common use-case which you alluded to—dynamic clustering. If static clustering is desired, we can document that or even provide another example in the demo app for it.

I agree. This would be considered breaking behavior, though, for those that currently have static clustering implemented.

Apr 27 '20 16:04 barbeau