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

osana · 2018-11-20T23:43:33Z

tobrun

Thank you for picking this up @osana. Seeing that we return the source object to the user instead of true/false feels a bit strange to me. Take for example following codeblock:

geoJsonSource = mapboxMap.removeSource(geoJsonSource);
if(geoJsonSource == null){
 // we couldn’t remove source
}

I think it makes more sense following the core setup to return true or false on the public api. If we would do this, we should also need to fix removeLayer as well.

thoughts @mapbox/maps-android ?

LukasPaczos · 2018-11-21T12:55:52Z

I agree with @tobrun, remove methods might be more readable if they return boolean values indicating whether removal has been successful. Users can always fetch the layer/source before removal with get methods which should cover current use cases.

osana · 2018-11-21T15:32:09Z

@tobrun @LukasPaczos Do you think it makes sense to change removeLayer return value
in this PR?

LukasPaczos · 2018-11-21T15:37:31Z

Sure, shouldn't be an issue.

LukasPaczos · 2018-11-28T18:23:14Z

platform/android/src/native_map_view.cpp

 }

 /**
 * 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) {


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.

osana · 2018-11-29T18:32:29Z

@tobrun @LukasPaczos Capturing offline conversation of what needs to happen in this PR

MapboxMap has three removeLayer methods. At the moment two of them invalidate existing jav areferences to the removed layer.

mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java

Line 354 in 1ceea72

* Removes the layer. Any references to the layer become invalid and should not be used anymore

mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java

Line 376 in 1ceea72

* Removes the layer. Any other references to the layer become invalid and should not be used anymore

We need the above two methods keep the reference alive ( transfer ownership from native to java peer) as it is done in

mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java

Line 365 in 1ceea72

* Removes the layer. The reference is re-usable after this and can be re-added

TODO: javadoc update & native_map_view.cpp change

Add test to verify the above is working, ie the reference to Layer in java is still valid after the Layer was removed
MapboxMap has two methods to removeSource. They result in the same native method call.
But Javadoc does not reflect that

mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java

Line 437 in 1ceea72

    
              * Removes the source. Any references to the source become invalid and should not be used anymore

mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java

Line 448 in 1ceea72

* Removes the source, preserving the reference for re-use

in both cases the reference should be kept alive

Add test to verify 2 is working, ie the reference to Source is still valid in java after the Source was removed

osana · 2018-12-03T15:16:31Z

platform/android/src/native_map_view.cpp

+        jni::Local<jni::Object<Layer>> layerObj =
+                LayerManagerAndroid::get()->createJavaLayerPeer(env, *map, std::move(coreLayer));
+        mbgl::android::Layer *layer = reinterpret_cast<mbgl::android::Layer *>(layerObj.get());
+        layer->setLayer(std::move(coreLayer));


@tobrun could you ask someone with more knowledge of c++ how this should be done . Lines 846 -848 are not correct.

LukasPaczos

Would be great to add test covering a scenario when removal fails. Otherwise LGTM 👍

LukasPaczos · 2018-12-06T15:35:50Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java

    }
    return nativeRemoveLayerAt(index);
+


NIT: new line

…boolean

LukasPaczos

🚀

…boolean (#13428)

osana added the Android Mapbox Maps SDK for Android label Nov 20, 2018

osana force-pushed the osana-return-fromRemoveSource branch 2 times, most recently from d2c060e to 881e405 Compare November 21, 2018 03:49

osana requested review from LukasPaczos and tobrun November 21, 2018 03:49

tobrun reviewed Nov 21, 2018

View reviewed changes

osana force-pushed the osana-return-fromRemoveSource branch 2 times, most recently from 86be772 to b95dd52 Compare November 21, 2018 15:31

LukasPaczos added the SEMVER-MAJOR Requires a major release according to Semantic Versioning rules label Nov 21, 2018

LukasPaczos added this to the android-v7.0.0-iowaska milestone Nov 21, 2018

osana force-pushed the osana-return-fromRemoveSource branch 3 times, most recently from 618f45f to 3281250 Compare November 28, 2018 05:38

LukasPaczos reviewed Nov 28, 2018

View reviewed changes

osana force-pushed the osana-return-fromRemoveSource branch 3 times, most recently from 9506127 to bcc671c Compare November 30, 2018 20:46

osana commented Dec 3, 2018

View reviewed changes

osana force-pushed the osana-return-fromRemoveSource branch from bcc671c to de18ed8 Compare December 5, 2018 18:58

LukasPaczos reviewed Dec 6, 2018

View reviewed changes

tobrun added the beta blocker Blocks the next beta release label Dec 7, 2018

osana force-pushed the osana-return-fromRemoveSource branch 2 times, most recently from 809d460 to 4630831 Compare December 7, 2018 15:21

nativeRemoveSource() and nativeRemoveLayer() methods should return a …

6c23514

…boolean

osana force-pushed the osana-return-fromRemoveSource branch from 4630831 to 6c23514 Compare December 7, 2018 15:22

LukasPaczos approved these changes Dec 7, 2018

View reviewed changes

osana merged commit ea775ae into master Dec 7, 2018

osana deleted the osana-return-fromRemoveSource branch December 7, 2018 18:10

osana removed the beta blocker Blocks the next beta release label Dec 7, 2018

tobrun pushed a commit that referenced this pull request Dec 10, 2018

nativeRemoveSource() and nativeRemoveLayer() methods should return a …

ed0ca39

…boolean (#13428)

tobrun mentioned this pull request Dec 10, 2018

Cherry pick style.java, return boolean from removeSource/Layer #13533

Merged

tobrun pushed a commit that referenced this pull request Dec 10, 2018

nativeRemoveSource() and nativeRemoveLayer() methods should return a …

aa9a647

…boolean (#13428)

LukasPaczos mentioned this pull request Jan 9, 2019

Unify behavior of removing sources and layers by the ID #12601

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

osana commented Nov 20, 2018

tobrun left a comment

LukasPaczos commented Nov 21, 2018

osana commented Nov 21, 2018

LukasPaczos commented Nov 21, 2018

LukasPaczos Nov 28, 2018

osana commented Nov 29, 2018 •

edited

Loading

osana Dec 3, 2018

LukasPaczos left a comment

LukasPaczos Dec 6, 2018

LukasPaczos left a comment

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

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

Conversation

osana commented Nov 20, 2018

tobrun left a comment

Choose a reason for hiding this comment

LukasPaczos commented Nov 21, 2018

osana commented Nov 21, 2018

LukasPaczos commented Nov 21, 2018

LukasPaczos Nov 28, 2018

Choose a reason for hiding this comment

osana commented Nov 29, 2018 • edited Loading

osana Dec 3, 2018

Choose a reason for hiding this comment

LukasPaczos left a comment

Choose a reason for hiding this comment

LukasPaczos Dec 6, 2018

Choose a reason for hiding this comment

LukasPaczos left a comment

Choose a reason for hiding this comment

osana commented Nov 29, 2018 •

edited

Loading