feat(cluster): Support updating existing items #627
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to these changes, when an item instance was removed, updated, and re-added to the ClusterManager, DefaultClusterRenderer.CreateMarkerTask.perform() would use a marker from its own mMarkerCache associated with that item to display that item on the map (even if ClusterManager.clearItems() was previously called). This resulted in the updated item contents not being reflected on the map, because the cached marker was not updated. This changeset does the following: * Add new protected methods in DefaultClusterRenderer - onClusterItemUpdated() and onClusterUpdated() - which are called when an existing cached marker is found for an item/cluster when that item/cluster is being added back into the ClusterManager. The default implementation of these methods update the marker contents on the map, but then can also be overridden by applications for custom behavior (similar to onBeforeClusterItemRendered() and onBeforeClusterRendered()). Following this change, you can now do ClusterManager.clearItems() and re-add the same list instance (or remove() and then add() the same item) and then call cluster() and the map markers will be updated with the new item model contents. * Add a new ClusterManager.updateItem() helper method for updating the ClusterManager and algorithms with an existing item. The current implementation in NonHierarchicalDistanceBasedAlgorithm is a wrapper for remove() and then add(). * Add more descriptive Javadocs to ClusterManager, DefaultClusterRenderer, and NonHierarchicalDistanceBasedAlgorithm to describe expected behavior, especially that cluster() should be invoked after any changes to ClusterManager to see changes on the map. One new TODO that could potentially be addressed - it seems like the code to set the marker text when creating the item marker should be refactored into onBeforeClusterItemRendered() to match the implementation of clusters (onBeforeClusterRendered()), and to allow applications extending DefaultClusterRenderer to inherit this default behavior via a call to super.onBeforeClusterItemRendered(). However, moving this code would change the behavior of the library for apps already extending DefaultClusterRenderer that aren't calling super.onBeforeClusterItemRendered() - the marker title and snippet would no longer be updated.
googlebot
added
the
cla: yes
arriolac
reviewed
Feb 19, 2020
library/src/main/java/com/google/maps/android/clustering/ClusterManager.java
Outdated
Show resolved
Hide resolved
library/src/main/java/com/google/maps/android/clustering/ClusterManager.java
Outdated
Show resolved
Hide resolved
library/src/main/java/com/google/maps/android/clustering/algo/GridBasedAlgorithm.java
Outdated
Show resolved
Hide resolved
library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java
Outdated
Show resolved
Hide resolved
library/src/main/java/com/google/maps/android/clustering/ClusterManager.java
Outdated
Show resolved
Hide resolved
barbeau
commented
Feb 20, 2020
library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #627 +/- ##
==========================================
+ Coverage 18.44% 19.00% +0.55%
==========================================
Files 71 71
Lines 4011 4056 +45
Branches 591 610 +19
==========================================
+ Hits 740 771 +31
- Misses 3237 3251 +14
Partials 34 34
Continue to review full report at Codecov.
|
* Implementation follows the Java Collection definitions of the matching methods * Synchronize within update() implementations to ensure they are atomic operations * Add unit tests for all Algorithm operations
|
Merging master branch to see if that enables CodeCov on this PR... |
arriolac
approved these changes
Feb 20, 2020
There was a problem hiding this comment.
LGTM. Before we merge, please follow-up on our discussion https://github.com/googlemaps/android-maps-utils/pull/627/files#r382051446 If we refactor, let's file an issue and tackle in a separate PR, otherwise, we can remove the TODO
library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java
Outdated
Show resolved
Hide resolved
library/src/main/java/com/google/maps/android/clustering/ClusterManager.java
Outdated
Show resolved
Hide resolved
"finally" clause will always be invoked (even after return), so we can just return the result of mAlgorithm method directly
See discussion at https://github.com/googlemaps/android-maps-utils/pull/627/files#r382258252. We'll open a new issue to discuss this specifically.
barbeau
added a commit
that referenced
this pull request
Apr 2, 2020
See #655 (comment) - we need to update the custom clustering demo for the best practices surrounding marker caching, following the implementation of #627 in v1. This means overriding onClusterUpdated() and onClusterItemUpdated() with the same implementations as onBeforeClusterRendered() and onBeforeClusterItemRendered(), respectively.
barbeau
added a commit
that referenced
this pull request
Apr 8, 2020
See #655 (comment) - we need to update the custom clustering demo for the best practices surrounding marker caching, following the implementation of #627 in v1. This means overriding onClusterUpdated() and onClusterItemUpdated() with the same implementations as onBeforeClusterRendered() and onBeforeClusterItemRendered(), respectively.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #90, when an item instance is removed, updated, and re-added the
ClusterManager, the updated item state wouldn't be reflected on the map.The reason for this is that
DefaultClusterRenderer.CreateMarkerTask.perform()was using a marker from its ownmMarkerCacheassociated with that item to display that item on the map (even ifClusterManager.clearItems()was previously called). This resulted in the updated item contents not being reflected on the map, because the cached marker was not updated.This PR does the following:
DefaultClusterRenderer-onClusterItemUpdated()andonClusterUpdated()- which are called when an existing cached marker is found for an item/cluster when that item/cluster is being added back into theClusterManager. The default implementation of these methods update the marker contents on the map, but then can also be overridden by applications for custom behavior (similar toonBeforeClusterItemRendered()andonBeforeClusterRendered()). Following this change, you can now doClusterManager.clearItems()and re-add the same list instance (orremove()and thenadd()the same item) and then callcluster()and the map markers will be updated with the new item model contents.ClusterManager.updateItem()helper method for updating theClusterManagerand algorithms with an existing item. The current implementation inNonHierarchicalDistanceBasedAlgorithmis a wrapper forremove()and thenadd().ClusterManager,DefaultClusterRenderer, andNonHierarchicalDistanceBasedAlgorithmto describe expected behavior, especially thatcluster()should be invoked after any changes toClusterManagerto see changes on the map.One new TODO I added that could potentially be addressed - it seems like the code to set the marker text when creating the item marker should be refactored into
onBeforeClusterItemRendered()to match the implementation of clusters (onBeforeClusterRendered()), and to allow applications extendingDefaultClusterRendererto inherit this default behavior via a call tosuper.onBeforeClusterItemRendered(). However, moving this code would change the behavior of the library for apps already extendingDefaultClusterRendererthat aren't callingsuper.onBeforeClusterItemRendered()- the marker title and snippet would no longer be updated. Thoughts on this are welcome!Fixes #90