Silently ignore removal of nonexistent style layer or source

[eimantas]

In case of exception:

• log a warning to console;
• mark style as nonmutated;
• return nullptr;

[mention-bot]

@eimantas, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @1ec5 and @incanus to be potential reviewers.

[kkaefer]

I believe this is incorrect. It always sets to false, even when the style was mutated before the call to remove a layer.

[eimantas]

Should I remove this statement? Since a couple of lines above there's impl->styleMutated = true; assuming the layer will be removed. And since this is in a catch block - the layer is not removed and the style is not mutated.

[kkaefer]

@eimantas Thank you for your contribution! Are you able to make the changes?

[eimantas]

@kkaefer - change implemented.

[eimantas]

Since this is "Core GL" fix, should I also add the entry to the Android SDK changelog?

[kkaefer]

@eimantas The fix is to not mark the style as mutated until the change was completed successfully, so that a failure to remove still leaves the style in unmutated state

[eimantas]

Please see the change now.

[boundsj]

Noting that this addresses #7900

[1ec5]

Would this change officially sanction a pattern of removing potentially nonexistent layers? That’s reasonable, but then why issue a warning? The developer might want to remove layers indiscriminately without spewing onto the console.

On the other hand, if we still consider the removal of a nonexistent layer to be programmer error, we should continue to throw an assertion. However, -[MGLStyle removeLayer:] (in the iOS/macOS SDK) could catch the C++ exception and rethrow it as an NSException, offering pure Objective-C code the opportunity to catch the exception if the removal was intentional.

[eimantas]

That’s reasonable, but then why issue a warning?

So that a developer can fix his code the check whether the layer exists before removing it.

However if you decide to transfer this into Cocoa/Android land with NSException and related classes, I believe there should be a convenience function for checking whether a layer exists before removing it (something like bool hasLayer(const std::string& id) or bool layerExists(const std::string& id). This would encourage developers to use defensive style of programming. I understand there's findLayer function, but that is not really its purpose. Is it?

[1ec5]

I believe there should be a convenience function for checking whether a layer exists before removing it (something like bool hasLayer(const std::string& id) or bool layerExists(const std::string& id). This would encourage developers to use defensive style of programming. I understand there's findLayer function, but that is not really its purpose. Is it?

Sure, that's an intended use of -[MGLStyle layerWithIdentifier:] and its Android equivalent. In fact, on iOS and macOS, it's impossible to remove a layer without obtaining an MGLStyleLayer object first. So the only time this error comes up is when you obtain the MGLStyleLayer by creating one anew or hanging onto a stale MGLStyleLayer.

There should be negligible performance impact to always checking for the existence of a layer before removing it. On the other hand, there should also be negligible performance impact to attempting to remove a nonexistent layer. So if that's the only programming mistake we're trying to head off with the warning, I'd lean towards silencing the warning. What do others here think?

[jfirebaugh]

I think we should change the removal method in core and in SDK bindings to return a null pointer and not log any message.

[eimantas]

If everyone is fine with silently ignoring the removal of nonexistent layer, I can implement these changes.

[jfirebaugh]

Rather than throwing an exception which is caught by Map::removeLayer, Style::removeLayer itself should return nullptr.

[eimantas]

Updates are done.

[jfirebaugh]

Can you update the CHANGELOG entries now that it doesn't log a warning?

@ivovandongen Do you want to change the Android bindings to follow this behavior (no error, return null)?

[ivovandongen]

Do you want to change the Android bindings to follow this behavior (no error, return null)?

@jfirebaugh Sure, but don't we want to make this consistent for sources than as well? Eg here

[1ec5]

On iOS/macOS, -[MGL*StyleLayer removeFromMapView:] clears its internal state before telling mbgl to remove the layer from the style. If the layer isn’t part of the style, -removeFromMapView: will bail, resulting in an unusable MGLStyleLayer object. The following code would crash:

MGLBackgroundStyleLayer *fillLayer = [[MGLBackgroundStyleLayer alloc] initWithIdentifier:@"bg"];
[style removeLayer:fillLayer]; // no-op
// …
[style addLayer:fillLayer]; // MGLRedundantLayerException

I think the correct behavior would be to ensure that _pendingLayer ends up with whatever either _pendingLayer or rawLayer had at the beginning of this method.

/cc @boundsj

[boundsj]

@1ec5 I think the correct behavior would be to ensure that _pendingLayer ends up with whatever either _pendingLayer or rawLayer had at the beginning of this method.

I agree. Instead of setting this variable and property to nullptr in the first place, I think we can create a mbgl::style::Layer pointer and use it to set self.rawLayer just like we do in initWithIdentifier:source: but we can use self.identifier.UTF8String and self.sourceIdentifier.UTF8String to create the layer (since we will have those strings at this point).

I think we should do this in between the two early returns, resetting _pendingLayer and self.rawLayer if mbglMap->removeLayer() succeeds but before the dynamic_cast check. I think this would allow the layer to keep the original values if the remove operations fails and still have a valid value if somehow the wrong type of layer was removed and the cast check causes an early return.

I can create an issue for this and make a separate PR.

[boundsj]

@jfirebaugh @kkaefer can this change be merged now? Once this core change lands and the behavior is changed, I'd like to add Darwin platform tests for the change in #8055.

[jfirebaugh]

don't we want to make this consistent for sources than as well

Yes, we should.

The Android bindings also need to be adjusted (e.g. here), to either test for a nullptr result (if we want to keep the existing behavior), or to pass on a null pointer.

[boundsj]

@jfirebaugh @kkaefer @1ec5: @ivovandongen and I made the change to make nonexistent source and layer removal consistent and updated the platform implementations (including taking care of #8053). I think this is ready for re-review.

[1ec5]

Darwin/iOS/macOS changes look good.

outdated