From ed0ca39130cb01e804710f211599bf1ee3c0f73f Mon Sep 17 00:00:00 2001 From: Osana Babayan <32496536+osana@users.noreply.github.com> Date: Fri, 7 Dec 2018 13:10:28 -0500 Subject: [PATCH] nativeRemoveSource() and nativeRemoveLayer() methods should return a boolean (#13428) --- .../com/mapbox/mapboxsdk/maps/MapboxMap.java | 29 +++++------ .../mapbox/mapboxsdk/maps/NativeMapView.java | 52 ++++++++----------- .../testapp/style/RuntimeStyleTests.java | 37 ++++++++----- platform/android/src/native_map_view.cpp | 32 +++++------- platform/android/src/native_map_view.hpp | 10 ++-- 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 58fc66407f9..7cc1d003df7 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 void addLayerAt(@NonNull Layer layer, @IntRange(from = 0) int index) { } /** - * 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 Layer removeLayer(@NonNull String layerId) { * 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 Layer removeLayer(@NonNull Layer layer) { * 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 void addSource(@NonNull Source source) { } /** - * 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 Source removeSource(@NonNull String sourceId) { * 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 532f311e900..0f5d4de5abf 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 @@ public void addLayerAt(@NonNull Layer layer, @IntRange(from = 0) int index) { 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 @@ public void addSource(@NonNull Source source) { 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 @@ private native void nativeFlyTo(double angle, double latitude, double longitude, @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 @@ private native void nativeFlyTo(double angle, double latitude, double longitude, 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 23a75d1642c..a1b1317a018 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.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 void testRemoveLayerAt() { 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 void testAddRemoveSource() { 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 void testRemoveNonExistingSource() { @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 void perform(UiController uiController, View view) { 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 1c744a6b572..d908b14d365 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> NativeMapView::removeLayerById(JNIEnv& env, const jni::String& id) { - std::unique_ptr coreLayer = map->getStyle().removeLayer(jni::Make(env, id)); - if (coreLayer) { - return LayerManagerAndroid::get()->createJavaLayerPeer(env, *map, std::move(coreLayer)); - } else { - return jni::Local>(); - } -} /** * Remove layer at index. */ -jni::Local> 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>(); + return jni::jni_false; } std::unique_ptr coreLayer = map->getStyle().removeLayer(layers.at(index)->getID()); if (coreLayer) { - return LayerManagerAndroid::get()->createJavaLayerPeer(env, *map, std::move(coreLayer)); - } else { - return jni::Local>(); + jni::Local> 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(layerPtr); std::unique_ptr coreLayer = map->getStyle().removeLayer(layer->get().getID()); if (coreLayer) { layer->setLayer(std::move(coreLayer)); + return jni::jni_true; } + return jni::jni_false; } jni::Local>> NativeMapView::getSources(JNIEnv& env) { @@ -908,13 +900,16 @@ void NativeMapView::addSource(JNIEnv& env, const jni::Object& obj, jlong } } -void NativeMapView::removeSource(JNIEnv& env, const jni::Object& obj, jlong sourcePtr) { +jni::jboolean NativeMapView::removeSource(JNIEnv& env, const jni::Object& obj, jlong sourcePtr) { assert(sourcePtr != 0); mbgl::android::Source *source = reinterpret_cast(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, 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 704331d8e27..57676d3edf0 100755 --- a/platform/android/src/native_map_view.hpp +++ b/platform/android/src/native_map_view.hpp @@ -216,11 +216,9 @@ class NativeMapView : public MapObserver { void addLayerAt(JNIEnv&, jni::jlong, jni::jint); - jni::Local> removeLayerById(JNIEnv&, const jni::String&); + jni::jboolean removeLayerAt(JNIEnv&, jni::jint); - jni::Local> removeLayerAt(JNIEnv&, jni::jint); - - void removeLayer(JNIEnv&, jlong); + jni::jboolean removeLayer(JNIEnv&, jlong); jni::Local>> getSources(JNIEnv&); @@ -228,9 +226,7 @@ class NativeMapView : public MapObserver { void addSource(JNIEnv&, const jni::Object&, jlong nativePtr); - jni::Local> removeSourceById(JNIEnv&, const jni::String&); - - void removeSource(JNIEnv&, const jni::Object&, jlong nativePtr); + jni::jboolean removeSource(JNIEnv&, const jni::Object&, jlong nativePtr); void addImage(JNIEnv&, const jni::String&, const jni::Object& bitmap, jni::jfloat, jni::jboolean);