[android] - optimise icon management

7 files changed, 248 insertions, 16 deletions

diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java
index edf448ab4f..c09c926eb5 100644
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java
@@ -118,6 +118,9 @@ class AnnotationManager {
 
       if (marker instanceof MarkerView) {
         markerViewManager.removeMarkerView((MarkerView) marker);
+      } else {
+        // do icon cleanup
+        iconManager.iconCleanup(marker.getIcon());
       }
     } else {
       // instanceOf Polygon/Polyline
@@ -137,6 +140,8 @@ class AnnotationManager {
 
         if (marker instanceof MarkerView) {
           markerViewManager.removeMarkerView((MarkerView) marker);
+        } else {
+          iconManager.iconCleanup(marker.getIcon());
         }
       } else {
         // instanceOf Polygon/Polyline
@@ -159,6 +164,8 @@ class AnnotationManager {
         marker.hideInfoWindow();
         if (marker instanceof MarkerView) {
           markerViewManager.removeMarkerView((MarkerView) marker);
+        } else {
+          iconManager.iconCleanup(marker.getIcon());
         }
       } else {
         // instanceOf Polygon/Polyline
diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/IconManager.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/IconManager.java
index 18eecfd9c3..affbf48267 100644
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/IconManager.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/IconManager.java
@@ -1,16 +1,17 @@
 package com.mapbox.mapboxsdk.maps;
 
 import android.graphics.Bitmap;
+import android.os.Build;
 
 import com.mapbox.mapboxsdk.Mapbox;
 import com.mapbox.mapboxsdk.annotations.Icon;
 import com.mapbox.mapboxsdk.annotations.IconFactory;
 import com.mapbox.mapboxsdk.annotations.Marker;
 import com.mapbox.mapboxsdk.annotations.MarkerView;
-import com.mapbox.mapboxsdk.exceptions.IconBitmapChangedException;
 
-import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 /**
  * Responsible for managing icons added to the Map.
@@ -25,15 +26,14 @@ import java.util.List;
  */
 class IconManager {
 
-  private NativeMapView nativeMapView;
-  private List<Icon> icons;
+  private final Map<Icon, Integer> iconMap = new HashMap<>();
 
+  private NativeMapView nativeMapView;
   private int highestIconWidth;
   private int highestIconHeight;
 
   IconManager(NativeMapView nativeMapView) {
     this.nativeMapView = nativeMapView;
-    this.icons = new ArrayList<>();
     // load transparent icon for MarkerView to trace actual markers, see #6352
     loadIcon(IconFactory.recreate(IconFactory.ICON_MARKERVIEW_ID, IconFactory.ICON_MARKERVIEW_BITMAP));
   }
@@ -83,13 +83,13 @@ class IconManager {
   }
 
   private void addIcon(Icon icon, boolean addIconToMap) {
-    if (!icons.contains(icon)) {
-      icons.add(icon);
+    if (!iconMap.keySet().contains(icon)) {
+      iconMap.put(icon, 1);
       if (addIconToMap) {
         loadIcon(icon);
       }
     } else {
-      validateIconChanged(icon);
+      iconMap.put(icon, iconMap.get(icon) + 1);
     }
   }
 
@@ -121,18 +121,11 @@ class IconManager {
   }
 
   void reloadIcons() {
-    for (Icon icon : icons) {
+    for (Icon icon : iconMap.keySet()) {
       loadIcon(icon);
     }
   }
 
-  private void validateIconChanged(Icon icon) {
-    Icon oldIcon = icons.get(icons.indexOf(icon));
-    if (!oldIcon.getBitmap().sameAs(icon.getBitmap())) {
-      throw new IconBitmapChangedException();
-    }
-  }
-
   void ensureIconLoaded(Marker marker, MapboxMap mapboxMap) {
     Icon icon = marker.getIcon();
     if (icon == null) {
@@ -149,4 +142,31 @@ class IconManager {
       marker.setTopOffsetPixels(getTopOffsetPixelsForIcon(icon));
     }
   }
+
+  public void iconCleanup(Icon icon) {
+    int refCounter = iconMap.get(icon) - 1;
+    if (refCounter == 0) {
+      remove(icon);
+    } else {
+      updateIconRefCounter(icon, refCounter);
+    }
+  }
+
+  private void remove(Icon icon) {
+    nativeMapView.removeAnnotationIcon(icon.getId());
+    iconMap.remove(icon);
+    if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
+      recycleBitmap(icon.getBitmap());
+    }
+  }
+
+  private void updateIconRefCounter(Icon icon, int refCounter) {
+    iconMap.put(icon, refCounter);
+  }
+
+  private void recycleBitmap(Bitmap bitmap) {
+    if (!bitmap.isRecycled()) {
+      bitmap.recycle();
+    }
+  }
 }
diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
index ddeb4b326e..b2d49bdbf8 100755
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
@@ -507,6 +507,13 @@ final class NativeMapView {
     nativeAddAnnotationIcon(symbol, width, height, scale, pixels);
   }
 
+  public void removeAnnotationIcon(String symbol) {
+    if (isDestroyedOn("removeAnnotationIcon")) {
+      return;
+    }
+    nativeRemoveAnnotationIcon(symbol);
+  }
+
   public void setVisibleCoordinateBounds(LatLng[] coordinates, RectF padding, double direction, long duration) {
     if (isDestroyedOn("setVisibleCoordinateBounds")) {
       return;
@@ -990,6 +997,8 @@ final class NativeMapView {
 
   private native void nativeAddAnnotationIcon(String symbol, int width, int height, float scale, byte[] pixels);
 
+  private native void nativeRemoveAnnotationIcon(String symbol);
+
   private native void nativeSetVisibleCoordinateBounds(LatLng[] coordinates, RectF padding,
                                                        double direction, long duration);
 
diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/maps/IconManagerResolver.java b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/maps/IconManagerResolver.java
new file mode 100644
index 0000000000..3e226a7ec5
--- /dev/null
+++ b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/maps/IconManagerResolver.java
@@ -0,0 +1,42 @@
+package com.mapbox.mapboxsdk.maps;
+
+import com.mapbox.mapboxsdk.annotations.Icon;
+
+import java.lang.reflect.Field;
+import java.util.HashMap;
+import java.util.Map;
+
+import timber.log.Timber;
+
+public class IconManagerResolver {
+
+  private IconManager iconManager;
+
+  public IconManagerResolver(MapboxMap mapboxMap) {
+    try {
+      Field annotationManagerField = MapboxMap.class.getDeclaredField("annotationManager");
+      annotationManagerField.setAccessible(true);
+      AnnotationManager annotationManager = (AnnotationManager) annotationManagerField.get(mapboxMap);
+
+      Field iconManagerField = AnnotationManager.class.getDeclaredField("iconManager");
+      iconManagerField.setAccessible(true);
+      iconManager = (IconManager) iconManagerField.get(annotationManager);
+    } catch (Exception exception) {
+      Timber.e(exception, "Could not create IconManagerResolver, unable to reflect.");
+    }
+  }
+
+  @SuppressWarnings("unchecked")
+  public Map<Icon, Integer> getIconMap() {
+    try {
+      Field field = IconManager.class.getDeclaredField("iconMap");
+      field.setAccessible(true);
+      return (Map<Icon, Integer>) field.get(iconManager);
+    } catch (NoSuchFieldException exception) {
+      Timber.e(exception, "Could not getIconMap, unable to reflect.");
+    } catch (IllegalAccessException exception) {
+      Timber.e(exception, "Could not getIconMap, unable to reflect.");
+    }
+    return new HashMap<>();
+  }
+}
diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/annotations/IconTest.java b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/annotations/IconTest.java
new file mode 100644
index 0000000000..33a946d0a1
--- /dev/null
+++ b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/annotations/IconTest.java
@@ -0,0 +1,146 @@
+package com.mapbox.mapboxsdk.testapp.annotations;
+
+import android.app.Activity;
+import android.support.v4.content.res.ResourcesCompat;
+
+import com.mapbox.mapboxsdk.annotations.Icon;
+import com.mapbox.mapboxsdk.annotations.IconFactory;
+import com.mapbox.mapboxsdk.annotations.Marker;
+import com.mapbox.mapboxsdk.annotations.MarkerOptions;
+import com.mapbox.mapboxsdk.geometry.LatLng;
+import com.mapbox.mapboxsdk.maps.IconManagerResolver;
+import com.mapbox.mapboxsdk.maps.MapboxMap;
+import com.mapbox.mapboxsdk.testapp.R;
+import com.mapbox.mapboxsdk.testapp.activity.BaseActivityTest;
+import com.mapbox.mapboxsdk.testapp.activity.espresso.EspressoTestActivity;
+import com.mapbox.mapboxsdk.testapp.utils.IconUtils;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.Map;
+
+import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.assertFalse;
+import static junit.framework.Assert.assertNull;
+import static junit.framework.Assert.assertTrue;
+
+/**
+ * Tests integration between Icons and Markers
+ */
+public class IconTest extends BaseActivityTest {
+
+  private Map<Icon, Integer> iconMap;
+
+  @Before
+  public void beforeTest() {
+    super.beforeTest();
+    iconMap = new IconManagerResolver(getMapboxMap()).getIconMap();
+  }
+
+  @Test
+  public void testEmpty() throws Exception {
+    assertTrue(iconMap.isEmpty());
+  }
+
+  @Test
+  public void testAddSameIconMarker() throws Exception {
+    Icon defaultMarker = IconFactory.getInstance(rule.getActivity()).defaultMarker();
+    getMapboxMap().addMarker(new MarkerOptions().position(new LatLng()));
+    getMapboxMap().addMarker(new MarkerOptions().position(new LatLng(1, 1)));
+    assertEquals(1, iconMap.size());
+    assertEquals(2, iconMap.get(defaultMarker), 0);
+  }
+
+  @Test
+  public void testAddDifferentIconMarker() throws Exception {
+    Icon icon = IconFactory.getInstance(rule.getActivity()).fromResource(R.drawable.mapbox_logo_icon);
+    getMapboxMap().addMarker(new MarkerOptions().icon(icon).position(new LatLng()));
+    getMapboxMap().addMarker(new MarkerOptions().position(new LatLng(1, 1)));
+    assertEquals(iconMap.size(), 2);
+    assertTrue(iconMap.containsKey(icon));
+    assertTrue(iconMap.get(icon) == 1);
+  }
+
+  @Test
+  public void testAddRemoveIconMarker() throws Exception {
+    MapboxMap mapboxMap = getMapboxMap();
+
+    Icon icon = IconFactory.getInstance(rule.getActivity()).fromResource(R.drawable.mapbox_logo_icon);
+    Marker marker = mapboxMap.addMarker(new MarkerOptions().icon(icon).position(new LatLng()));
+    mapboxMap.addMarker(new MarkerOptions().position(new LatLng(1, 1)));
+    assertEquals(iconMap.size(), 2);
+    assertTrue(iconMap.containsKey(icon));
+    assertTrue(iconMap.get(icon) == 1);
+
+    mapboxMap.removeMarker(marker);
+    assertEquals(iconMap.size(), 1);
+    assertFalse(iconMap.containsKey(icon));
+  }
+
+  @Test
+  public void testAddRemoveDefaultMarker() throws Exception {
+    MapboxMap mapboxMap = getMapboxMap();
+
+    Marker marker = mapboxMap.addMarker(new MarkerOptions().position(new LatLng(1, 1)));
+    assertEquals(iconMap.size(), 1);
+
+    mapboxMap.removeMarker(marker);
+    assertEquals(iconMap.size(), 0);
+
+    mapboxMap.addMarker(new MarkerOptions().position(new LatLng()));
+    assertEquals(iconMap.size(), 1);
+  }
+
+  @Test
+  public void testAddRemoveMany() throws Exception {
+    Activity activity = rule.getActivity();
+    MapboxMap mapboxMap = getMapboxMap();
+    IconFactory iconFactory = IconFactory.getInstance(activity);
+
+    // add 2 default icon markers
+    Marker defaultMarkerOne = mapboxMap.addMarker(new MarkerOptions().position(new LatLng(1, 1)));
+    Marker defaultMarkerTwo = mapboxMap.addMarker(new MarkerOptions().position(new LatLng(2, 1)));
+
+    // add 4 unique icon markers
+    mapboxMap.addMarker(new MarkerOptions()
+      .icon(iconFactory.fromResource(R.drawable.mapbox_logo_icon))
+      .position(new LatLng(3, 1))
+    );
+    mapboxMap.addMarker(new MarkerOptions()
+      .icon(iconFactory.fromResource(R.drawable.mapbox_compass_icon))
+      .position(new LatLng(4, 1))
+    );
+    mapboxMap.addMarker(new MarkerOptions()
+      .icon(IconUtils.drawableToIcon(activity, R.drawable.ic_stars,
+        ResourcesCompat.getColor(activity.getResources(),
+          R.color.blueAccent, activity.getTheme())))
+      .position(new LatLng(5, 1))
+    );
+    mapboxMap.addMarker(new MarkerOptions()
+      .icon(iconFactory.fromResource(R.drawable.ic_android))
+      .position(new LatLng(6, 1))
+    );
+
+    assertEquals("Amount of icons should match 5", 5, iconMap.size());
+    assertEquals("Refcounter of default marker should match 2", 2, iconMap.get(iconFactory.defaultMarker()), 0);
+
+    mapboxMap.removeMarker(defaultMarkerOne);
+
+    assertEquals("Amount of icons should match 5", 5, iconMap.size());
+    assertEquals("Refcounter of default marker should match 1", 1, iconMap.get(iconFactory.defaultMarker()), 0);
+
+    mapboxMap.removeMarker(defaultMarkerTwo);
+
+    assertEquals("Amount of icons should match 4", 4, iconMap.size());
+    assertNull("DefaultMarker shouldn't exist anymore", iconMap.get(iconFactory.defaultMarker()));
+
+    mapboxMap.clear();
+    assertEquals("Amount of icons should match 0", 0, iconMap.size());
+  }
+
+  @Override
+  protected Class getActivityClass() {
+    return EspressoTestActivity.class;
+  }
+}
diff --git a/platform/android/src/native_map_view.cpp b/platform/android/src/native_map_view.cpp
index fa9e90ddc6..58c39355d1 100755
--- a/platform/android/src/native_map_view.cpp
+++ b/platform/android/src/native_map_view.cpp
@@ -716,6 +716,11 @@ void NativeMapView::addAnnotationIcon(JNIEnv& env, jni::String symbol, jint w, j
         symbolName, std::move(premultipliedImage), float(scale)));
 }
 
+void NativeMapView::removeAnnotationIcon(JNIEnv& env, jni::String symbol) {
+    const std::string symbolName = jni::Make<std::string>(env, symbol);
+    map->removeAnnotationImage(symbolName);
+}
+
 jdouble NativeMapView::getTopOffsetPixelsForAnnotationSymbol(JNIEnv& env, jni::String symbolName) {
     return map->getTopOffsetPixelsForAnnotationImage(jni::Make<std::string>(env, symbolName));
 }
@@ -1523,6 +1528,7 @@ void NativeMapView::registerNative(jni::JNIEnv& env) {
             METHOD(&NativeMapView::updatePolygon, "nativeUpdatePolygon"),
             METHOD(&NativeMapView::removeAnnotations, "nativeRemoveAnnotations"),
             METHOD(&NativeMapView::addAnnotationIcon, "nativeAddAnnotationIcon"),
+            METHOD(&NativeMapView::removeAnnotationIcon, "nativeRemoveAnnotationIcon"),
             METHOD(&NativeMapView::getTopOffsetPixelsForAnnotationSymbol, "nativeGetTopOffsetPixelsForAnnotationSymbol"),
             METHOD(&NativeMapView::getTransitionDuration, "nativeGetTransitionDuration"),
             METHOD(&NativeMapView::setTransitionDuration, "nativeSetTransitionDuration"),
diff --git a/platform/android/src/native_map_view.hpp b/platform/android/src/native_map_view.hpp
index ba1545728a..ec82a36a5c 100755
--- a/platform/android/src/native_map_view.hpp
+++ b/platform/android/src/native_map_view.hpp
@@ -199,6 +199,8 @@ public:
 
     void addAnnotationIcon(JNIEnv&, jni::String, jint, jint, jfloat, jni::Array<jbyte>);
 
+    void removeAnnotationIcon(JNIEnv&, jni::String);
+
     jni::jdouble getTopOffsetPixelsForAnnotationSymbol(JNIEnv&, jni::String);
 
     jni::jlong getTransitionDuration(JNIEnv&);