nativeRemoveSource() and nativeRemoveLayer() methods should return a boolean (#13428)

5 files changed, 76 insertions, 84 deletions

diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java
index 58fc66407f..7cc1d003df 100644
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java
@@ -351,13 +351,12 @@ public final class MapboxMap {
   }
 
   /**
-   * Removes the layer. Any references to the layer become invalid and should not be used anymore
+   * Removes the layer. The reference is re-usable after this and can be re-added
    *
    * @param layerId the layer to remove
-   * @return the removed layer or null if not found
+   * @return true if the layer was successfully removed, false - otherwise
    */
-  @Nullable
-  public Layer removeLayer(@NonNull String layerId) {
+  public boolean removeLayer(@NonNull String layerId) {
     return nativeMapView.removeLayer(layerId);
   }
 
@@ -365,10 +364,9 @@ public final class MapboxMap {
    * Removes the layer. The reference is re-usable after this and can be re-added
    *
    * @param layer the layer to remove
-   * @return the layer
+   * @return true if the layer was successfully removed, false - otherwise
    */
-  @Nullable
-  public Layer removeLayer(@NonNull Layer layer) {
+  public boolean removeLayer(@NonNull Layer layer) {
     return nativeMapView.removeLayer(layer);
   }
 
@@ -376,10 +374,9 @@ public final class MapboxMap {
    * Removes the layer. Any other references to the layer become invalid and should not be used anymore
    *
    * @param index the layer index
-   * @return the removed layer or null if not found
+   * @return true if the layer was successfully removed, false - otherwise
    */
-  @Nullable
-  public Layer removeLayerAt(@IntRange(from = 0) int index) {
+  public boolean removeLayerAt(@IntRange(from = 0) int index) {
     return nativeMapView.removeLayerAt(index);
   }
 
@@ -434,13 +431,12 @@ public final class MapboxMap {
   }
 
   /**
-   * Removes the source. Any references to the source become invalid and should not be used anymore
+   * Removes the source, preserving the reference for re-use
    *
    * @param sourceId the source to remove
-   * @return the source handle or null if the source was not present
+   * @return true if the source was successfully removed, false - otherwise
    */
-  @Nullable
-  public Source removeSource(@NonNull String sourceId) {
+  public boolean removeSource(@NonNull String sourceId) {
     return nativeMapView.removeSource(sourceId);
   }
 
@@ -448,10 +444,9 @@ public final class MapboxMap {
    * Removes the source, preserving the reference for re-use
    *
    * @param source the source to remove
-   * @return the source
+   * @return true if the source was successfully removed, false - otherwise
    */
-  @Nullable
-  public Source removeSource(@NonNull Source source) {
+  public boolean removeSource(@NonNull Source source) {
     return nativeMapView.removeSource(source);
   }
 
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 532f311e90..0f5d4de5ab 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
@@ -748,27 +748,29 @@ final class NativeMapView {
     nativeAddLayerAt(layer.getNativePtr(), index);
   }
 
-  @Nullable
-  public Layer removeLayer(@NonNull String layerId) {
+  public boolean removeLayer(@NonNull String layerId) {
     if (checkState("removeLayer")) {
-      return null;
+      return false;
     }
-    return nativeRemoveLayerById(layerId);
+
+    Layer layer = getLayer(layerId);
+    if (layer != null) {
+      return removeLayer(layer);
+    }
+    return false;
   }
 
-  @Nullable
-  public Layer removeLayer(@NonNull Layer layer) {
+
+  public boolean removeLayer(@NonNull Layer layer) {
     if (checkState("removeLayer")) {
-      return null;
+      return false;
     }
-    nativeRemoveLayer(layer.getNativePtr());
-    return layer;
+    return nativeRemoveLayer(layer.getNativePtr());
   }
 
-  @Nullable
-  public Layer removeLayerAt(@IntRange(from = 0) int index) {
+  public boolean removeLayerAt(@IntRange(from = 0) int index) {
     if (checkState("removeLayerAt")) {
-      return null;
+      return false;
     }
     return nativeRemoveLayerAt(index);
   }
@@ -794,25 +796,22 @@ final class NativeMapView {
     nativeAddSource(source, source.getNativePtr());
   }
 
-  @Nullable
-  public Source removeSource(@NonNull String sourceId) {
+  public boolean removeSource(@NonNull String sourceId) {
     if (checkState("removeSource")) {
-      return null;
+      return false;
     }
     Source source = getSource(sourceId);
     if (source != null) {
       return removeSource(source);
     }
-    return null;
+    return false;
   }
 
-  @Nullable
-  public Source removeSource(@NonNull Source source) {
+  public boolean removeSource(@NonNull Source source) {
     if (checkState("removeSource")) {
-      return null;
+      return false;
     }
-    nativeRemoveSource(source, source.getNativePtr());
-    return source;
+    return nativeRemoveSource(source, source.getNativePtr());
   }
 
   public void addImage(@NonNull String name, @NonNull Bitmap image, boolean sdf) {
@@ -1217,16 +1216,11 @@ final class NativeMapView {
   @Keep
   private native void nativeAddLayerAt(long layerPtr, int index) throws CannotAddLayerException;
 
-  @NonNull
   @Keep
-  private native Layer nativeRemoveLayerById(String layerId);
+  private native boolean nativeRemoveLayer(long layerId);
 
   @Keep
-  private native void nativeRemoveLayer(long layerId);
-
-  @NonNull
-  @Keep
-  private native Layer nativeRemoveLayerAt(int index);
+  private native boolean nativeRemoveLayerAt(int index);
 
   @NonNull
   @Keep
@@ -1240,7 +1234,7 @@ final class NativeMapView {
   private native void nativeAddSource(Source source, long sourcePtr) throws CannotAddSourceException;
 
   @Keep
-  private native void nativeRemoveSource(Source source, long sourcePtr);
+  private native boolean nativeRemoveSource(Source source, long sourcePtr);
 
   @Keep
   private native void nativeAddImage(String name, Bitmap bitmap, float pixelRatio, boolean sdf);
diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/RuntimeStyleTests.java b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/RuntimeStyleTests.java
index 23a75d1642..a1b1317a01 100644
--- a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/RuntimeStyleTests.java
+++ b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/RuntimeStyleTests.java
@@ -42,6 +42,7 @@ import static android.support.test.espresso.matcher.ViewMatchers.isDisplayed;
 import static android.support.test.espresso.matcher.ViewMatchers.withId;
 import static com.mapbox.mapboxsdk.testapp.action.MapboxMapAction.invoke;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -124,14 +125,13 @@ public class RuntimeStyleTests extends BaseActivityTest {
       public void perform(UiController uiController, View view) {
         // Remove by index
         Layer firstLayer = mapboxMap.getLayers().get(0);
-        Layer removed = mapboxMap.removeLayerAt(0);
-        assertNotNull(removed);
-        assertNotNull(removed.getId());
-        assertEquals(firstLayer.getId(), removed.getId());
+        boolean removed = mapboxMap.removeLayerAt(0);
+        assertTrue(removed);
+        assertNotNull(firstLayer);
 
         // Test remove by index bounds checks
         Timber.i("Remove layer at index > size");
-        assertNull(mapboxMap.removeLayerAt(Integer.MAX_VALUE));
+        assertFalse(mapboxMap.removeLayerAt(Integer.MAX_VALUE));
       }
     });
   }
@@ -198,8 +198,8 @@ public class RuntimeStyleTests extends BaseActivityTest {
       mapboxMap.addSource(new VectorSource("my-source", "mapbox://mapbox.mapbox-terrain-v2"));
 
       // Remove
-      Source mySource = mapboxMap.removeSource("my-source");
-      assertNotNull(mySource);
+      boolean removeOk = mapboxMap.removeSource("my-source");
+      assertTrue(removeOk);
       assertNull(mapboxMap.getLayer("my-source"));
 
       // Add
@@ -288,9 +288,22 @@ public class RuntimeStyleTests extends BaseActivityTest {
   @Test
   public void testRemoveNonExistingLayer() {
     invoke(mapboxMap, (uiController, mapboxMap) -> {
-      mapboxMap.removeLayer("layer");
-      mapboxMap.removeLayerAt(mapboxMap.getLayers().size() + 1);
-      mapboxMap.removeLayerAt(-1);
+      assertFalse(mapboxMap.removeLayer("layer"));
+      assertFalse(mapboxMap.removeLayerAt(mapboxMap.getLayers().size() + 1));
+      assertFalse(mapboxMap.removeLayerAt(-1));
+    });
+  }
+
+  @Test
+  public void testRemoveExistingLayer() {
+    invoke(mapboxMap, (uiController, mapboxMap) -> {
+      Layer firstLayer = mapboxMap.getLayers().get(0);
+      assertTrue(mapboxMap.removeLayer(firstLayer));
+
+      firstLayer = mapboxMap.getLayers().get(0);
+      assertTrue(mapboxMap.removeLayer(firstLayer.getId()));
+
+      assertTrue(mapboxMap.removeLayerAt(0));
     });
   }
 
@@ -322,8 +335,8 @@ public class RuntimeStyleTests extends BaseActivityTest {
       assertNotNull(mapboxMap.getLayer("building"));
 
       // Remove
-      Layer building = mapboxMap.removeLayer("building");
-      assertNotNull(building);
+      boolean removed = mapboxMap.removeLayer("building");
+      assertTrue(removed);
       assertNull(mapboxMap.getLayer("building"));
 
       // Add
diff --git a/platform/android/src/native_map_view.cpp b/platform/android/src/native_map_view.cpp
index 1c744a6b57..d908b14d36 100755
--- a/platform/android/src/native_map_view.cpp
+++ b/platform/android/src/native_map_view.cpp
@@ -824,50 +824,42 @@ void NativeMapView::addLayerAt(JNIEnv& env, jlong nativeLayerPtr, jni::jint inde
     }
 }
 
-/**
- * Remove by layer id.
- */
-jni::Local<jni::Object<Layer>> NativeMapView::removeLayerById(JNIEnv& env, const jni::String& id) {
-    std::unique_ptr<mbgl::style::Layer> coreLayer = map->getStyle().removeLayer(jni::Make<std::string>(env, id));
-    if (coreLayer) {
-        return LayerManagerAndroid::get()->createJavaLayerPeer(env, *map, std::move(coreLayer));
-    } else {
-        return jni::Local<jni::Object<Layer>>();
-    }
-}
 
 /**
  * Remove layer at index.
  */
-jni::Local<jni::Object<Layer>> NativeMapView::removeLayerAt(JNIEnv& env, jni::jint index) {
+jni::jboolean NativeMapView::removeLayerAt(JNIEnv& env, jni::jint index) {
     auto layers = map->getStyle().getLayers();
 
     // Check index
     int numLayers = layers.size() - 1;
     if (index > numLayers || index < 0) {
         Log::Warning(Event::JNI, "Index out of range: %i", index);
-        return jni::Local<jni::Object<Layer>>();
+        return jni::jni_false;
     }
 
     std::unique_ptr<mbgl::style::Layer> coreLayer = map->getStyle().removeLayer(layers.at(index)->getID());
     if (coreLayer) {
-        return LayerManagerAndroid::get()->createJavaLayerPeer(env, *map, std::move(coreLayer));
-    } else {
-        return jni::Local<jni::Object<Layer>>();
+        jni::Local<jni::Object<Layer>> layerObj =
+                LayerManagerAndroid::get()->createJavaLayerPeer(env, *map, std::move(coreLayer));
+        return jni::jni_true;
     }
+    return jni::jni_false;
 }
 
 /**
  * Remove with wrapper object id. Ownership is transferred back to the wrapper
  */
-void NativeMapView::removeLayer(JNIEnv&, jlong layerPtr) {
+jni::jboolean NativeMapView::removeLayer(JNIEnv&, jlong layerPtr) {
     assert(layerPtr != 0);
 
     mbgl::android::Layer *layer = reinterpret_cast<mbgl::android::Layer *>(layerPtr);
     std::unique_ptr<mbgl::style::Layer> coreLayer = map->getStyle().removeLayer(layer->get().getID());
     if (coreLayer) {
         layer->setLayer(std::move(coreLayer));
+        return jni::jni_true;
     }
+    return jni::jni_false;
 }
 
 jni::Local<jni::Array<jni::Object<Source>>> NativeMapView::getSources(JNIEnv& env) {
@@ -908,13 +900,16 @@ void NativeMapView::addSource(JNIEnv& env, const jni::Object<Source>& obj, jlong
     }
 }
 
-void NativeMapView::removeSource(JNIEnv& env, const jni::Object<Source>& obj, jlong sourcePtr) {
+jni::jboolean NativeMapView::removeSource(JNIEnv& env, const jni::Object<Source>& obj, jlong sourcePtr) {
     assert(sourcePtr != 0);
 
     mbgl::android::Source *source = reinterpret_cast<mbgl::android::Source *>(sourcePtr);
     if (source->removeFromMap(env, obj, *map)) {
         source->releaseJavaPeer();
+        return jni::jni_true;
     }
+
+    return jni::jni_false;
 }
 
 void NativeMapView::addImage(JNIEnv& env, const jni::String& name, const jni::Object<Bitmap>& bitmap, jni::jfloat scale, jni::jboolean sdf) {
@@ -1046,7 +1041,6 @@ void NativeMapView::registerNative(jni::JNIEnv& env) {
             METHOD(&NativeMapView::addLayer, "nativeAddLayer"),
             METHOD(&NativeMapView::addLayerAbove, "nativeAddLayerAbove"),
             METHOD(&NativeMapView::addLayerAt, "nativeAddLayerAt"),
-            METHOD(&NativeMapView::removeLayerById, "nativeRemoveLayerById"),
             METHOD(&NativeMapView::removeLayerAt, "nativeRemoveLayerAt"),
             METHOD(&NativeMapView::removeLayer, "nativeRemoveLayer"),
             METHOD(&NativeMapView::getSources, "nativeGetSources"),
diff --git a/platform/android/src/native_map_view.hpp b/platform/android/src/native_map_view.hpp
index 704331d8e2..57676d3edf 100755
--- a/platform/android/src/native_map_view.hpp
+++ b/platform/android/src/native_map_view.hpp
@@ -216,11 +216,9 @@ public:
 
     void addLayerAt(JNIEnv&, jni::jlong, jni::jint);
 
-    jni::Local<jni::Object<Layer>> removeLayerById(JNIEnv&, const jni::String&);
+    jni::jboolean removeLayerAt(JNIEnv&, jni::jint);
 
-    jni::Local<jni::Object<Layer>> removeLayerAt(JNIEnv&, jni::jint);
-
-    void removeLayer(JNIEnv&, jlong);
+    jni::jboolean removeLayer(JNIEnv&, jlong);
 
     jni::Local<jni::Array<jni::Object<Source>>> getSources(JNIEnv&);
 
@@ -228,9 +226,7 @@ public:
 
     void addSource(JNIEnv&, const jni::Object<Source>&, jlong nativePtr);
 
-    jni::Local<jni::Object<Source>> removeSourceById(JNIEnv&, const jni::String&);
-
-    void removeSource(JNIEnv&, const jni::Object<Source>&, jlong nativePtr);
+    jni::jboolean removeSource(JNIEnv&, const jni::Object<Source>&, jlong nativePtr);
 
     void addImage(JNIEnv&, const jni::String&, const jni::Object<Bitmap>& bitmap, jni::jfloat, jni::jboolean);