fix: Maintain insertion order in clusters

A clustering performance improvement was made in #368 by switching from using an ArrayList to a HashSet. However, ArrayLists maintain insertion order of the elements when iterating over them later, while HashSets do not. Because the cluster has the center of the first element, that center will change if the iteration order of the elements change, which results in clusters changing position when updating an item model and reclustering.

This changes to using a LinkedHashSet, which should give the same constant time performance benefits as the Set for add/remove over the List, but still maintains insertion order.

A unit test for insertion order is also added.

Fixes #615

Refs: 615

Author: barbeau

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

@@ -24,12 +24,11 @@
 import com.google.maps.android.projection.SphericalMercatorProjection;
 import com.google.maps.android.quadtree.PointQuadTree;
 
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.List;
+import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Set;
 
@@ -54,7 +53,7 @@ public class NonHierarchicalDistanceBasedAlgorithm<T extends ClusterItem> extend
     /**
      * Any modifications should be synchronized on mQuadTree.
      */
-    private final Collection<QuadItem<T>> mItems = new HashSet<>();
+    private final Collection<QuadItem<T>> mItems = new LinkedHashSet<>();
 
     /**
      * Any modifications should be synchronized on mQuadTree.
@@ -169,7 +168,7 @@ protected Collection<QuadItem<T>> getClusteringItems(PointQuadTree<QuadItem<T>>
 
     @Override
     public Collection<T> getItems() {
-        final List<T> items = new ArrayList<T>();
+        final Set<T> items = new LinkedHashSet<>();
         synchronized (mQuadTree) {
             for (QuadItem<T> quadItem : mItems) {
                 items.add(quadItem.mClusterItem);

library/src/main/java/com/google/maps/android/quadtree/PointQuadTree.java

@@ -21,7 +21,7 @@
 
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
 
@@ -118,7 +118,7 @@ private void insert(double x, double y, T item) {
             return;
         }
         if (mItems == null) {
-            mItems = new HashSet<>();
+            mItems = new LinkedHashSet<>();
         }
         mItems.add(item);
         if (mItems.size() > MAX_ELEMENTS && mDepth < MAX_DEPTH) {

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

@@ -21,11 +21,14 @@
 
 import org.junit.Test;
 
+import java.util.Collection;
+
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 public class QuadItemTest {
+
     @Test
     public void testRemoval() {
         TestingItem item_1_5 = new TestingItem(0.1, 0.5);
@@ -46,10 +49,39 @@ public void testRemoval() {
         assertTrue(algo.getItems().contains(item_2_3));
     }
 
+    /**
+     * Test if insertion order into the algorithm is the same as returned item order. This matters
+     * because we want repeatable clustering behavior when updating model values and re-clustering.
+     */
+    @Test
+    public void testInsertionOrder() {
+        NonHierarchicalDistanceBasedAlgorithm<ClusterItem> algo =
+                new NonHierarchicalDistanceBasedAlgorithm<>();
+        for (int i = 0; i < 100; i++) {
+            algo.addItem(new TestingItem(Integer.toString(i), 0.0, 0.0));
+        }
+
+        assertEquals(100, algo.getItems().size());
+
+        Collection<ClusterItem> items = algo.getItems();
+        int counter = 0;
+        for (ClusterItem item : items) {
+            assertEquals(Integer.toString(counter), item.getTitle());
+            counter++;
+        }
+    }
+
     private class TestingItem implements ClusterItem {
         private final LatLng mPosition;
+        private final String mTitle;
+
+        TestingItem(String title, double lat, double lng) {
+            mTitle = title;
+            mPosition = new LatLng(lat, lng);
+        }
 
         TestingItem(double lat, double lng) {
+            mTitle = "";
             mPosition = new LatLng(lat, lng);
         }
 
@@ -60,7 +92,7 @@ public LatLng getPosition() {
 
         @Override
         public String getTitle() {
-            return null;
+            return mTitle;
         }
 
         @Override