Mapbox-gl-native: Unify behavior of removing sources and layers by the ID

[LukasPaczos]

mbgl::android::NativeMapView#removeLayerById invoked by MapboxMap#removeLayer(String) doesn't return control over the core layer to the peer, therefore, calling

mapboxMap.removeLayer(layerID);
mapboxMap.addLayer(layer);

results in a CannotAddLayerException.

This is documented as follows:

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

However, for Sources removed by ID we are first looking the source up and removing by reference, even though the javadoc states the same as for Layers:

@Nullable
public Source removeSource(@NonNull String sourceId) {
  if (checkState("removeSource")) {
    return null;
  }
  Source source = getSource(sourceId);
  if (source != null) {
    return removeSource(source);
  }
  return null;
}

We should unify this logic to express the same behavior for layers and sources or simply update the documentation for sources.

[LukasPaczos]

Additional observations when it comes to adding/removing sources and layers:

Difference between nativeGetLayer(String id) and nativeGetSource(String id)

Even though the changes in https://github.com/mapbox/mapbox-gl-native/pull/13428 (later ported to Style.java) unified the code paths of layer/source removal by ID to first fetch the source/layer and then remove it by reference, the calls to nativeGetLayer(String id) and nativeGetSource(String id) differ, which results in different outcomes.

Calling nativeGetSource(String id) returns the same java reference that was initially passed in nativeAddSource(Source source, long ptr), becuase the JNI Source object keeps the reference to the Java peer and can return it. Therefore, calling nativeRemoveSource(Source source) keeps the initial reference valid for reuse in this code path:

public boolean removeSource(@NonNull String sourceId) {
  if (checkState("removeSource")) {
    return false;
  }
  Source source = getSource(sourceId);
  if (source != null) {
    return removeSource(source);
  }
  return false;
}

On the other hand, calling nativeGetLayer(String id) creates an new Java peer out of core Layer and returns this new reference, therefore path like:

public boolean removeLayer(@NonNull String layerId) {
  if (checkState("removeLayer")) {
    return false;
  }
  Layer layer = getLayer(layerId);
  if (layer != null) {
    return removeLayer(layer);
  }
  return false;
}

makes the initial reference invalid, because the removeLayer(layer) is called with an effectively new reference, not the initial one as is the case with the removeSource(source).

Currently, this codepath will succeed:

source = new GeoJsonSource("source", Point.fromLngLat(0, 0));
style.addSource(source);
style.removeSource("source");
style.addSource(source);

while this one will fail:

layer = new SymbolLayer("layer", "source");
style.addLayer(layer);
style.removeLayer("layer");
style.addLayer(layer);

Solutions

1. Adjust the documentation to state that removeSource(String id) keeps the reference valid whereas removeLayer(Layer id) makes the initial reference invalid, or
2. make the reference invalid for the source removal, or
3. keep the java peer reference in the Layer object and return it when requested.

Style.java cavities

With introduction of the Style.java we are trying to return a local copy of the Layer instead of fetching it from core, if possible:

public Layer getLayer(@NonNull String id) {
  validateState("getLayer");
  Layer layer = layers.get(id);
  if (layer == null) {
    layer = nativeMapView.getLayer(id);
  }
  return layer;
}

This introduces an unexpected behaviour where runtime added layers can be removed and re-added by id:

Layer layer = new SymbolLayer("layer", "source");
style.addLayer(layer);

Layer fetchedLayer = style.getLayer("layer");
style.removeLayer(fetchedLayer);
style.addLayer(layer);

and style/default layers cannot and will throw an exception:

Layer layer = style.getLayer("com.mapbox.annotations.points");

Layer fetchedLayer = style.getLayer("com.mapbox.annotations.points");
style.removeLayer(fetchedLayer);
style.addLayer(layer);

Next steps

• [ ] Research whether we can keep a reference to the Layer's Java peer and return it when requested (@pozdnyakov would you able to help here? I've seen you working on that codebase recently.)
• [ ] Unify behaviour for fetching runtime added and style/default layers via the Style.java object. Might be automatically resolved if above is possible.
• [ ] Fix get and remove documention for layers and sources in the Style.java class.

/cc @mapbox/maps-android @samfader

[stale[bot]]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

[stale[bot]]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

[LukasPaczos]

I this ticket still relevant, especially in the context of Carbon @tarigo?

[tarigo]

For Carbon - no.