fix: stale QuadItem removal after ClusterItem position updates (#1730)

* Fix stale QuadItem removal after ClusterItem position updates

* test(clustering): add regression tests and demo for stale QuadItem removal (#1730)

- Expand QuadItemTest with comprehensive unit tests for single removal, bulk removal, clearing, and coordinate-boundary updates after a mutable ClusterItem changes position.

- Add fallback to QuadItem(item) in NonHierarchicalDistanceBasedAlgorithm.kt removeItem/removeItems for subclass robustness if mItemMap is bypassed.

- Enhance ClusteringDiffDemoActivity to mutate ClusterItem coordinates in-place and include background filler markers to trigger PointQuadTree quadrant splitting, demonstrating the fix on-device.

---------

Co-authored-by: Dale Hawkins <107309+dkhawk@users.noreply.github.com>

Author: Aparnamohan1312

clustering/src/main/java/com/google/maps/android/clustering/algo/NonHierarchicalDistanceBasedAlgorithm.kt

@@ -47,6 +47,7 @@ open class NonHierarchicalDistanceBasedAlgorithm<T : ClusterItem> : AbstractAlgo
      */
     @JvmField
     protected val mItems: MutableCollection<QuadItem<T>> = LinkedHashSet()
+    protected val mItemMap = HashMap<T, QuadItem<T>>()
 
     /**
      * Any modifications should be synchronized on mQuadTree.
@@ -61,6 +62,7 @@ open class NonHierarchicalDistanceBasedAlgorithm<T : ClusterItem> : AbstractAlgo
         synchronized(mQuadTree) {
             val result = mItems.add(quadItem)
             if (result) {
+                mItemMap[item] = quadItem
                 mQuadTree.add(quadItem)
             }
             return result
@@ -81,15 +83,16 @@ open class NonHierarchicalDistanceBasedAlgorithm<T : ClusterItem> : AbstractAlgo
     override fun clearItems() {
         synchronized(mQuadTree) {
             mItems.clear()
+            mItemMap.clear()
             mQuadTree.clear()
         }
     }
 
     override fun removeItem(item: T): Boolean {
         // QuadItem delegates hashcode() and equals() to its item so,
         //   removing any QuadItem to that item will remove the item
-        val quadItem = QuadItem(item)
         synchronized(mQuadTree) {
+            val quadItem = mItemMap.remove(item) ?: QuadItem(item)
             val result = mItems.remove(quadItem)
             if (result) {
                 mQuadTree.remove(quadItem)
@@ -104,7 +107,7 @@ open class NonHierarchicalDistanceBasedAlgorithm<T : ClusterItem> : AbstractAlgo
             for (item in items) {
                 // QuadItem delegates hashcode() and equals() to its item so,
                 //   removing any QuadItem to that item will remove the item
-                val quadItem = QuadItem(item)
+                val quadItem = mItemMap.remove(item) ?: QuadItem(item)
                 val individualResult = mItems.remove(quadItem)
                 if (individualResult) {
                     mQuadTree.remove(quadItem)

clustering/src/test/java/com/google/maps/android/clustering/QuadItemTest.java

@@ -97,8 +97,117 @@ public void testInsertionOrder() {
     }
   }
 
+  @Test
+  public void testUpdateItemAfterPositionChange() {
+    NonHierarchicalDistanceBasedAlgorithm<TestingItem> algo =
+        new NonHierarchicalDistanceBasedAlgorithm<>();
+    TestingItem item = new TestingItem("title1", 0.0, 0.0);
+    algo.addItem(item);
+    assertEquals(1, algo.getItems().size());
+
+    // Update the position of the mutable item
+    item.setPosition(10.0, 10.0);
+
+    // Call updateItem
+    assertTrue("updateItem should return true after position change", algo.updateItem(item));
+    assertEquals(1, algo.getItems().size());
+
+    // Verify that the old QuadItem at (0, 0) was removed from the tree
+    // and only the new position (10, 10) is indexed
+    java.util.Set<? extends Cluster<TestingItem>> clusters = algo.getClusters(4.0f);
+    assertEquals(1, clusters.size());
+    Cluster<TestingItem> cluster = clusters.iterator().next();
+    assertEquals(10.0, cluster.getPosition().latitude, 0.001);
+    assertEquals(10.0, cluster.getPosition().longitude, 0.001);
+  }
+
+  @Test
+  public void testRemoveItemAfterPositionChange() {
+    NonHierarchicalDistanceBasedAlgorithm<TestingItem> algo =
+        new NonHierarchicalDistanceBasedAlgorithm<>();
+    TestingItem item = new TestingItem("title1", 0.0, 0.0);
+    algo.addItem(item);
+    assertEquals(1, algo.getItems().size());
+
+    // Update the position of the mutable item
+    item.setPosition(10.0, 10.0);
+
+    // Removing the item should succeed and remove it from the tree
+    assertTrue("removeItem should return true after position change", algo.removeItem(item));
+    assertEquals(0, algo.getItems().size());
+    assertEquals(0, algo.getClusters(4.0f).size());
+  }
+
+  @Test
+  public void testUpdateItemPreventsStaleQuadTreeEntries() {
+    TestAlgorithm<TestingItem> algo = new TestAlgorithm<>();
+
+    // Add 60 filler items to force PointQuadTree to split (MAX_ELEMENTS = 50)
+    for (int i = 0; i < 60; i++) {
+      algo.addItem(new TestingItem("filler" + i, 10.0 + i * 0.001, 10.0 + i * 0.001));
+    }
+
+    // Add item1 in top-left quadrant
+    TestingItem item1 = new TestingItem("item1", 1.0, 1.0);
+    algo.addItem(item1);
+
+    assertEquals("QuadTree should contain item1 at (1, 1)", 1, algo.getQuadTreeItemCount(1.0, 1.0, 0.001));
+
+    // Move item1 far across quadrant boundary to (50.0, 50.0) and update
+    item1.setPosition(50.0, 50.0);
+    algo.updateItem(item1);
+
+    // Without fix, old QuadItem remains at (1.0, 1.0) in mQuadTree because remove traversed the new coordinates
+    assertEquals("QuadTree should NOT contain stale entry at (1, 1) after update", 0, algo.getQuadTreeItemCount(1.0, 1.0, 0.001));
+    assertEquals("QuadTree should contain item1 at (50, 50)", 1, algo.getQuadTreeItemCount(50.0, 50.0, 0.001));
+  }
+
+  @Test
+  public void testRemoveItemsAfterPositionChange() {
+    NonHierarchicalDistanceBasedAlgorithm<TestingItem> algo =
+        new NonHierarchicalDistanceBasedAlgorithm<>();
+    TestingItem item1 = new TestingItem("title1", 0.0, 0.0);
+    TestingItem item2 = new TestingItem("title2", 1.0, 1.0);
+    algo.addItems(java.util.Arrays.asList(item1, item2));
+    assertEquals(2, algo.getItems().size());
+
+    // Update the position of both items
+    item1.setPosition(10.0, 10.0);
+    item2.setPosition(20.0, 20.0);
+
+    assertTrue("removeItems should return true after position change",
+        algo.removeItems(java.util.Arrays.asList(item1, item2)));
+    assertEquals(0, algo.getItems().size());
+    assertEquals(0, algo.getClusters(4.0f).size());
+  }
+
+  @Test
+  public void testClearItemsAfterPositionChange() {
+    NonHierarchicalDistanceBasedAlgorithm<TestingItem> algo =
+        new NonHierarchicalDistanceBasedAlgorithm<>();
+    TestingItem item1 = new TestingItem("title1", 0.0, 0.0);
+    algo.addItem(item1);
+    item1.setPosition(10.0, 10.0);
+
+    algo.clearItems();
+    assertEquals(0, algo.getItems().size());
+    assertEquals(0, algo.getClusters(4.0f).size());
+  }
+
+  private static class TestAlgorithm<T extends ClusterItem> extends NonHierarchicalDistanceBasedAlgorithm<T> {
+    private static final com.google.maps.android.projection.SphericalMercatorProjection PROJ =
+        new com.google.maps.android.projection.SphericalMercatorProjection(1.0);
+
+    public int getQuadTreeItemCount(double lat, double lng, double span) {
+      com.google.maps.android.geometry.Point p = PROJ.toPoint(new LatLng(lat, lng));
+      com.google.maps.android.geometry.Bounds bounds = new com.google.maps.android.geometry.Bounds(
+          p.x - span, p.x + span, p.y - span, p.y + span);
+      return mQuadTree.search(bounds).size();
+    }
+  }
+
   private static class TestingItem implements ClusterItem {
-    private final LatLng mPosition;
+    private LatLng mPosition;
     private String mTitle;
 
     TestingItem(String title, double lat, double lng) {
@@ -110,6 +219,9 @@ private static class TestingItem implements ClusterItem {
       mTitle = "";
       mPosition = new LatLng(lat, lng);
     }
+    public void setPosition(double lat, double lng) {
+    mPosition = new LatLng(lat, lng);
+    }
 
     @NonNull
     @Override

demo/src/main/java/com/google/maps/android/utils/demo/ClusteringDiffDemoActivity.java

@@ -161,6 +161,13 @@ protected void startDemo(boolean isRestore) {
   }
 
   private void addItems() {
+    // Add 60 filler background markers across Greater London to force PointQuadTree to split into quadrants (MAX_ELEMENTS = 50)
+    for (int i = 0; i < 60; i++) {
+      double lat = 51.3 + (i % 10) * 0.05;
+      double lng = -0.4 + (i / 10) * 0.08;
+      mClusterManager.addItem(new Person(new LatLng(lat, lng), "Citizen " + i, R.drawable.john));
+    }
+
     // Marker in Enfield
     mClusterManager.addItem(new Person(City.ENFIELD.latLng, "John", R.drawable.john));
 
@@ -181,7 +188,7 @@ private void rotateLocation() {
     Log.d("ClusterTest", "Item rotated to: " + newLocation.toString() + ", City: " + cityName);
 
     if (itemToUpdate != null) {
-      itemToUpdate = new Person(newLocation, "Teach", R.drawable.teacher);
+      itemToUpdate.setPosition(newLocation);
       mClusterManager.updateItem(itemToUpdate); // Update the marker
       mClusterManager.cluster();
     }

demo/src/main/java/com/google/maps/android/utils/demo/model/Person.java

@@ -24,14 +24,18 @@
 public class Person implements ClusterItem {
   public final String name;
   public final int profilePhoto;
-  private final LatLng mPosition;
+  private LatLng mPosition;
 
   public Person(LatLng position, String name, int pictureResource) {
     this.name = name;
     profilePhoto = pictureResource;
     mPosition = position;
   }
 
+  public void setPosition(LatLng position) {
+    mPosition = position;
+  }
+
   @NonNull
   @Override
   public LatLng getPosition() {