Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[android] returning null instead of the Source object in the MapboxMap#removeSource when it fails #13428

Merged
merged 1 commit into from
Dec 7, 2018
Merged

[android] returning null instead of the Source object in the MapboxMap#removeSource when it fails #13428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 12 additions & 17 deletions platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,35 +351,32 @@ 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);
}

/**
* 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);
}

/**
* 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);
}

Expand Down Expand Up @@ -434,24 +431,22 @@ 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);
}

/**
* 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);
}

Expand Down
52 changes: 23 additions & 29 deletions ...orm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down
37 changes: 25 additions & 12 deletions ...SDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/RuntimeStyleTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
});
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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));
});
}

Expand Down Expand Up @@ -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
Expand Down
32 changes: 13 additions & 19 deletions platform/android/src/native_map_view.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every other remove method besides this one drops the reference to the layer, so the one kept in Java is probably going to become invalid. In order to make our intended logic possible, that a user fetches the layer with getLayer (or just keeps the reference from the start) and can keep after it's removed from the map (successfully or not), we need to keep the reference valid.

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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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"),
Expand Down
10 changes: 3 additions & 7 deletions platform/android/src/native_map_view.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,21 +216,17 @@ class NativeMapView : public MapObserver {

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&);

jni::Local<jni::Object<Source>> getSource(JNIEnv&, const jni::String&);

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);

Expand Down