[android] return layer ownership on remove

6 files changed, 121 insertions, 39 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 3408757bd0..db4601724e 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
@@ -210,7 +210,7 @@ public class MapboxMap {
      *
      * @param layerId the layer to remove
      * @throws NoSuchLayerException
-     * 
+     *
      */
     @UiThread
     public void removeLayer(@NonNull String layerId) throws NoSuchLayerException {
@@ -218,6 +218,17 @@ public class MapboxMap {
     }
 
     /**
+     * Removes the layer. The reference is re-usable after this and can be re-added
+     *
+     * @param layer the layer to remove
+     * @throws NoSuchLayerException
+     */
+    @UiThread
+    public void removeLayer(@NonNull Layer layer) throws NoSuchLayerException {
+        getMapView().getNativeMapView().removeLayer(layer);
+    }
+
+    /**
      * Returns a {@link Source} if any source with the given identifier was found.
      * <p>
      * Source identifiers are not guaranteed to exist across styles or different versions of the
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 cfdedfb494..24a56f1004 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
@@ -499,7 +499,11 @@ final class NativeMapView {
     }
 
     public void removeLayer(@NonNull String layerId) throws NoSuchLayerException {
-        nativeRemoveLayer(nativeMapViewPtr, layerId);
+        nativeRemoveLayerById(nativeMapViewPtr, layerId);
+    }
+
+    public void removeLayer(@NonNull Layer layer) throws NoSuchLayerException {
+        nativeRemoveLayer(nativeMapViewPtr, layer.getNativePtr());
     }
 
     public Source getSource(@NonNull String sourceId) {
@@ -743,7 +747,9 @@ final class NativeMapView {
 
     private native void nativeAddLayer(long nativeMapViewPtr, long layerPtr, String before);
 
-    private native void nativeRemoveLayer(long nativeMapViewPtr, String layerId) throws NoSuchLayerException;
+    private native void nativeRemoveLayerById(long nativeMapViewPtr, String layerId) throws NoSuchLayerException;
+
+    private native void nativeRemoveLayer(long nativeMapViewPtr, long layerId) throws NoSuchLayerException;
 
     private native Source nativeGetSource(long nativeMapViewPtr, String sourceId);
 
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 a6882874bf..1191963feb 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
@@ -1,5 +1,6 @@
 package com.mapbox.mapboxsdk.testapp.style;
 
+import android.graphics.Color;
 import android.support.test.espresso.Espresso;
 import android.support.test.espresso.UiController;
 import android.support.test.espresso.ViewAction;
@@ -17,6 +18,9 @@ import com.mapbox.mapboxsdk.style.sources.VectorSource;
 import com.mapbox.mapboxsdk.testapp.R;
 import com.mapbox.mapboxsdk.testapp.activity.style.RuntimeStyleTestActivity;
 import com.mapbox.mapboxsdk.testapp.utils.OnMapReadyIdlingResource;
+import com.mapbox.mapboxsdk.testapp.utils.ViewUtils;
+
+import junit.framework.Assert;
 
 import org.hamcrest.Matcher;
 import org.junit.After;
@@ -26,6 +30,10 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
+import static android.support.test.espresso.Espresso.onView;
+import static android.support.test.espresso.matcher.ViewMatchers.isDisplayed;
+import static android.support.test.espresso.matcher.ViewMatchers.withId;
+import static junit.framework.Assert.fail;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
@@ -35,7 +43,7 @@ import static org.junit.Assert.assertTrue;
  * Basic smoke tests for Layer and Source
  */
 @RunWith(AndroidJUnit4.class)
-public class RuntimeStyleTests extends BaseStyleTest {
+public class RuntimeStyleTests {
 
     @Rule
     public final ActivityTestRule<RuntimeStyleTestActivity> rule = new ActivityTestRule<>(RuntimeStyleTestActivity.class);
@@ -48,45 +56,13 @@ public class RuntimeStyleTests extends BaseStyleTest {
         Espresso.registerIdlingResources(idlingResource);
     }
 
-    /**
-     * TODO fix failing test
-     */
     @Test
-    @Ignore
     public void testGetAddRemoveLayer() {
-        checkViewIsDisplayed(R.id.mapView);
-
-        MapboxMap mapboxMap = rule.getActivity().getMapboxMap();
-
-        //Get initial
-        assertNotNull(mapboxMap.getLayer("building"));
-
-        //Remove
-        try {
-            mapboxMap.removeLayer("building");
-        } catch (NoSuchLayerException e) {
-            assertFalse(true);
-        }
-        assertNull(mapboxMap.getLayer("building"));
-
-        //Add
-        FillLayer layer = new FillLayer("building", "composite");
-        layer.setSourceLayer("building");
-        mapboxMap.addLayer(layer);
-
-        assertNotNull(mapboxMap.getLayer("building"));
-
-        try {
-            layer.setProperties(PropertyFactory.visibility(Property.VISIBLE));
-            assertTrue("Never reached as the reference is invalid after adding", false);
-        } catch (Exception e) {
-            //Expected, reference is no longer valid
-        }
+        onView(withId(R.id.mapView)).perform(new AddRemoveLayerAction());
     }
 
     @Test
     public void testAddRemoveSource() {
-        checkViewIsDisplayed(R.id.mapView);
 
         MapboxMap mapboxMap = rule.getActivity().getMapboxMap();
         mapboxMap.addSource(new VectorSource("my-source", "mapbox://mapbox.mapbox-terrain-v2"));
@@ -97,6 +73,60 @@ public class RuntimeStyleTests extends BaseStyleTest {
         }
     }
 
+    private class AddRemoveLayerAction implements ViewAction {
+
+        @Override
+        public Matcher<View> getConstraints() {
+            return isDisplayed();
+        }
+
+        @Override
+        public String getDescription() {
+            return getClass().getSimpleName();
+        }
+
+        @Override
+        public void perform(UiController uiController, View view) {
+            MapboxMap mapboxMap = rule.getActivity().getMapboxMap();
+
+            //Get initial
+            assertNotNull(mapboxMap.getLayer("building"));
+
+            //Remove
+            try {
+                mapboxMap.removeLayer("building");
+            } catch (NoSuchLayerException e) {
+                fail("Definitively exists: " + e.getMessage());
+            }
+            assertNull(mapboxMap.getLayer("building"));
+
+            //Add
+            FillLayer layer = new FillLayer("building", "composite");
+            layer.setSourceLayer("building");
+            mapboxMap.addLayer(layer);
+            assertNotNull(mapboxMap.getLayer("building"));
+
+            //Assure the reference still works
+            layer.setProperties(PropertyFactory.visibility(Property.VISIBLE));
+
+            //Remove, preserving the reference
+            try {
+                mapboxMap.removeLayer(layer);
+            } catch (NoSuchLayerException e) {
+                fail("Definitively exists: " + e.getMessage());
+            }
+
+            //Property setters should still work
+            layer.setProperties(PropertyFactory.fillColor(Color.RED));
+
+            //Re-add the reference...
+            mapboxMap.addLayer(layer);
+
+            //Ensure it's there
+            Assert.assertNotNull(mapboxMap.getLayer(layer.getId()));
+        }
+    }
+
     @After
     public void unregisterIntentServiceIdlingResource() {
         Espresso.unregisterIdlingResources(idlingResource);
diff --git a/platform/android/src/jni.cpp b/platform/android/src/jni.cpp
index 1d4849dcfb..6c346ec931 100755
--- a/platform/android/src/jni.cpp
+++ b/platform/android/src/jni.cpp
@@ -1109,7 +1109,10 @@ void nativeAddLayer(JNIEnv *env, jni::jobject* obj, jlong nativeMapViewPtr, jlon
     }
 }
 
-void nativeRemoveLayer(JNIEnv *env, jni::jobject* obj, jlong nativeMapViewPtr, jni::jstring* id) {
+/**
+ * Remove by layer id. Ownership is not transferred back
+ */
+void nativeRemoveLayerById(JNIEnv *env, jni::jobject* obj, jlong nativeMapViewPtr, jni::jstring* id) {
     assert(nativeMapViewPtr != 0);
     NativeMapView *nativeMapView = reinterpret_cast<NativeMapView *>(nativeMapViewPtr);
     try {
@@ -1119,6 +1122,22 @@ void nativeRemoveLayer(JNIEnv *env, jni::jobject* obj, jlong nativeMapViewPtr, j
     }
 }
 
+/**
+ * Remove with wrapper object id. Ownership is transferred back to the wrapper
+ */
+void nativeRemoveLayer(JNIEnv *env, jni::jobject* obj, jlong nativeMapViewPtr, jlong layerPtr) {
+    assert(nativeMapViewPtr != 0);
+    NativeMapView *nativeMapView = reinterpret_cast<NativeMapView *>(nativeMapViewPtr);
+    mbgl::android::Layer *layer = reinterpret_cast<mbgl::android::Layer *>(layerPtr);
+    try {
+        std::unique_ptr<mbgl::style::Layer> coreLayer = nativeMapView->getMap().removeLayer(layer->get().getID());
+        layer->setLayer(std::move(coreLayer));
+    } catch (const std::runtime_error& error) {
+        jni::ThrowNew(*env, jni::FindClass(*env, "com/mapbox/mapboxsdk/style/layers/NoSuchLayerException"), error.what());
+    }
+}
+
+
 jni::jobject* nativeGetSource(JNIEnv *env, jni::jobject* obj, jni::jlong nativeMapViewPtr, jni::jstring* sourceId) {
     assert(env);
     assert(nativeMapViewPtr != 0);
@@ -1838,7 +1857,8 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM *vm, void *reserved) {
         MAKE_NATIVE_METHOD(nativeFlyTo, "(JDDDJDD)V"),
         MAKE_NATIVE_METHOD(nativeGetLayer, "(JLjava/lang/String;)Lcom/mapbox/mapboxsdk/style/layers/Layer;"),
         MAKE_NATIVE_METHOD(nativeAddLayer, "(JJLjava/lang/String;)V"),
-        MAKE_NATIVE_METHOD(nativeRemoveLayer, "(JLjava/lang/String;)V"),
+        MAKE_NATIVE_METHOD(nativeRemoveLayerById, "(JLjava/lang/String;)V"),
+        MAKE_NATIVE_METHOD(nativeRemoveLayer, "(JJ)V"),
         MAKE_NATIVE_METHOD(nativeGetSource, "(JLjava/lang/String;)Lcom/mapbox/mapboxsdk/style/sources/Source;"),
         MAKE_NATIVE_METHOD(nativeAddSource, "(JJ)V"),
         MAKE_NATIVE_METHOD(nativeRemoveSource, "(JLjava/lang/String;)V"),
diff --git a/platform/android/src/style/layers/layer.cpp b/platform/android/src/style/layers/layer.cpp
index aa6df40470..00a52147b8 100644
--- a/platform/android/src/style/layers/layer.cpp
+++ b/platform/android/src/style/layers/layer.cpp
@@ -45,6 +45,10 @@ namespace android {
         this->map = &_map;
     }
 
+    void Layer::setLayer(std::unique_ptr<mbgl::style::Layer> sourceLayer) {
+        this->ownedLayer = std::move(sourceLayer);
+    }
+
     std::unique_ptr<mbgl::style::Layer> Layer::releaseCoreLayer() {
         assert(ownedLayer != nullptr);
         return std::move(ownedLayer);
@@ -54,6 +58,10 @@ namespace android {
         return jni::Make<jni::String>(env, layer.getID());
     }
 
+    style::Layer& Layer::get() {
+        return layer;
+    }
+
     void Layer::setLayoutProperty(jni::JNIEnv& env, jni::String jname, jni::Object<> jvalue) {
         Value value(env, jvalue);
 
diff --git a/platform/android/src/style/layers/layer.hpp b/platform/android/src/style/layers/layer.hpp
index 01eac2280b..f3cd073552 100644
--- a/platform/android/src/style/layers/layer.hpp
+++ b/platform/android/src/style/layers/layer.hpp
@@ -34,10 +34,17 @@ public:
 
     virtual jni::jobject* createJavaPeer(jni::JNIEnv&) = 0;
 
+    /**
+     * Set core layer (ie return ownership after remove)
+     */
+    void setLayer(std::unique_ptr<mbgl::style::Layer>);
+
     void addToMap(mbgl::Map&, mbgl::optional<std::string>);
 
     jni::String getId(jni::JNIEnv&);
 
+    style::Layer& get();
+
     void setLayoutProperty(jni::JNIEnv&, jni::String, jni::Object<> value);
 
     void setPaintProperty(jni::JNIEnv&, jni::String, jni::Object<> value);