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

Removing source processed by the layer #12526

Closed
LukasPaczos opened this issue Aug 1, 2018 · 5 comments · Fixed by #13428
Closed

Removing source processed by the layer #12526

LukasPaczos opened this issue Aug 1, 2018 · 5 comments · Fixed by #13428
Assignees
Labels
Android Mapbox Maps SDK for Android SEMVER-MAJOR Requires a major release according to Semantic Versioning rules

Comments

@LukasPaczos
Copy link
Member

When removing a Source that's being currently processed by a Layer we are failing silently and printing a log.

We should improve the experience during this unusual condition by maybe printing a higher level log, and on the Android side updating docs and returning null instead of the Source object in the MapboxMap#removeSource when it fails.

Example that will fail:

mapView.getMapAsync(mapboxMap -> {
  GeoJsonSource source = new GeoJsonSource("source", Point.fromLngLat(0, 0));
  mapboxMap.addSource(source);
  mapboxMap.addLayer(new CircleLayer("layer", "source"));
  mapboxMap.removeSource("source");
});

How's @mapbox/maps-ios handling this scenario?

@LukasPaczos LukasPaczos added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels Aug 1, 2018
@LukasPaczos
Copy link
Member Author

Whoops, obviously we first need to remove a Layer to be able to remove a Source... That said, we still can improve the returned values and logs.

@tobrun
Copy link
Member

tobrun commented Aug 13, 2018

My idea was to patch this up as part of #10947

@1ec5
Copy link
Contributor

1ec5 commented Aug 14, 2018

How's @mapbox/maps-ios handling this scenario?

MGLStyle doesn’t explicitly document the fact that the developer shouldn’t remove sources or images that are currently being used by layers. If they try to do that, mbgl logs a warning to the console and fails silently (or gracefully, depending on your point of view).

Log::Warning(Event::General, "Source '%s' is in use, cannot remove", id.c_str());

Since we subsequently nil out the source’s map view backpointer and pending source pointer, we’re unable to prevent the developer from adding the same source to a different map view’s style:

_pendingSource = mapView.style.rawStyle->removeSource(self.identifier.UTF8String);
_mapView = nil;

[NSException raise:MGLRedundantSourceException
format:@"This instance %@ was already added to %@. Adding the same source instance " \
"to the style more than once is invalid.", self, mapView.style];

@lilykaiser
Copy link

Can we move forward in 10947 as @tobrun suggests?

@tobrun
Copy link
Member

tobrun commented Nov 15, 2018

android equivalent of #9692

@tobrun tobrun added SEMVER-MAJOR Requires a major release according to Semantic Versioning rules and removed Core The cross-platform C++ core, aka mbgl labels Nov 15, 2018
@tobrun tobrun added this to the android-v7.0.0-iowaska milestone Nov 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android SEMVER-MAJOR Requires a major release according to Semantic Versioning rules
Projects
None yet
5 participants