Pass std::exception_ptr in MapObserver::onDidFailLoadingMap

[brunoabinader]

Forwards the error type and message std::exception_ptr from style::Observer::onStyleError as well as non-style related errors.

As per @1ec5 comments in #8398:

This would allow all the SDKs – not only the iOS and macOS SDKs – to provide more information when the map fails to load for some reason.

Fixes #8398.

[mention-bot]

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

[brunoabinader]

@1ec5 does the new entries in MGLErrorCode requires updating CHANGELOG.md?

[tmpsantos]

Qt looks good.

[jfirebaugh]

What do you think about using std::exception_ptr as the error type rather than ErrorType + std::string? This would match our convention in other places, e.g. style::Observer::on{Glyphs,Sprite,Source,Tile,Resource}Error.

[1ec5]

This is a welcome improvement! I do agree with @jfirebaugh that passing an std::exception_ptr all the way through would be preferable. Would that means we’d have different subclasses of std::exception_ptr for each failure mode, or would everything be an std::runtime_error?

does the new entries in MGLErrorCode requires updating CHANGELOG.md?

The new error types themselves probably aren’t noteworthy, but let’s add a blurb noting that we’ve made the error that’s passed into -[MGLMapViewDelegate mapViewDidFailLoadingMap:withError:] more informative. Thanks!

[1ec5]

You can declare error on this line; no need for a separate declaration statement.

[1ec5]

Would either MGLErrorCodeBadServerResponse or MGLErrorCodeConnectionFailed describe this failure, or is MGLErrorCodeLoadStyleFailed a distinct case?

[brunoabinader]

I'd prefer for map loading failure to have its own error type. However, we're no longer distinctively separating these in three different error types because std::runtime_error only provides a string.

[1ec5]

Thank you so much for this! 👍 It turns out the error user info dictionary is called that because its contents are often displayed to the user with a localized description and sometimes a recovery suggestion. Here are some suggestions for improvements, taking advantage of the error type and assuming message comes directly from an std::exception_ptr, as suggested in #8418 (comment):

• ParseStyle:
  • NSLocalizedDescriptionKey: “The map failed to load because the style is corrupted.”
  • NSLocalizedFailureReasonErrorKey: @(message.c_str())
• NotFound:
  • NSLocalizedDescriptionKey: “The map failed to load because the style can’t be found or is incompatible.”
  • NSLocalizedFailureReasonErrorKey: @(message.c_str())
• LoadingStyle:
  • NSLocalizedDescriptionKey: @(message.c_str())

For the localized descriptions, we can use the NSLocalizedStringWithDefaultValue() macro (as described here) to potentially localize the error message.

Let me know if you’d prefer that I make these changes myself.

[brunoabinader]

Would that means we’d have different subclasses of std::exception_ptr for each failure mode, or would everything be an std::runtime_error?

Good question - it seems we're using only plain std::runtime_error everywhere in GL native code, so I'd say let's keep the same pattern.

As explained above, we'll no longer be able to distinct between the three error types - however I don't think that would make much difference anyways - the error message is the most important debugging information we need.

[1ec5]

We do have a couple subclasses of mbgl::util::Exception meant to provide more distinguishable errors. Currently, the only subclasses appear to be SpriteImageException and MisuseException; could we add more subclasses for these error types?

[brunoabinader]

could we add more subclasses for these error types?

I don't have a strong opinion on this, though for this approach to work we'd need to deduct the exception type via catch - see the exception juggling in https://github.com/mapbox/mapbox-gl-native/blob/master/include/mbgl/util/string.hpp#L44-L51 - for all platform-specific codes that needed to handle this.

[1ec5]

For the iOS and macOS implementations, the catch statements would replace the existing switch statement. For other implementations that don't need to distinguish individual failure reasons, catching a runtime_error would be sufficient.

[1ec5]

This fixes #8086, which I previously missed before filing #8398 as a duplicate.

[tobrun]

Android 👍, added a comment to #8389 (comment) to take this new error handling in account.

[kkaefer]

related: #7890