Added `-(BOOL)[MGLStyle removeSource:error:]` that provides an NSError. by julianrex · Pull Request #13399 · mapbox/mapbox-gl-native

julianrex · 2018-11-16T19:50:45Z

Addresses #9692 - adds a new method that returns YES if the source was removed successfully, or NO if not, in which case an NSError will be returned by reference (if an NSError** was provided).

/cc @jmkiley

fabian-guerra

Thanks for fixing this. I added a question and minor tasks.

fabian-guerra · 2018-11-17T00:07:09Z

You will have to add the above and this localized string: https://github.com/mapbox/mapbox-gl-native/blob/master/platform/ios/DEVELOPING.md#adding-user-facing-text

Since this isn't end-user-facing text (or perhaps shouldn't be) - what are the conventions here? Still use NSLocalizedStringWithDefaultValue?

Perhaps it should be:

Use a generic, localized, error message for NSLocalizedDescriptionKey, then move the existing, unlocalized, message to NSLocalizedFailureReasonErrorKey.

EDIT: I updated the strings file as @fabian-guerra details above.

fabian-guerra · 2018-11-17T00:07:52Z

Are we going to live with both methods or should we deprecate this?

I think we can hold off on deprecation for the moment.

fabian-guerra · 2018-11-17T00:08:15Z

Not that is makes a difference in practice, but since nil is used with objects (id), and NSError** isn't a pointer-to-object (rather a pointer-to-pointer-to-object), it should be NULL in this case.

Right, but it's a " pointer-to-pointer-to-ObjC" object thus following the apple's convention on using nil would be consistent with the platform and our SDK.

fabian-guerra

I added a comment and this branch needs to fix conflicts before merging.

fabian-guerra · 2018-11-19T20:59:38Z

Right, but it's a " pointer-to-pointer-to-ObjC" object thus following the apple's convention on using nil would be consistent with the platform and our SDK.

… in case of failure. Updated existing test.

1ec5 · 2018-11-20T20:02:44Z

    }
+    else if (outError) {
+        // Consider raising an exception here
+        NSString *format = NSLocalizedStringWithDefaultValue(@"REMOVE_SRC_FAIL_MISMATCH_FMT", @"Foundation", nil, @"Identifier '%@' does not match source identifier '%s'", @"User-friendly error description");


This is a pretty specific error to use as the localized description. Note that localized descriptions are meant to be displayed to the end user, at least in theory. Other fields, such as the failure reason, can be used for technical details for the developer. In practice, it’s unlikely this error message would be displayed in an alert, but translators are going to find this string a bit more difficult to comprehend than the usual fare. By the way, I wonder if the Transifex and NSLocalizedString will correctly handle %s.

Yes, I thought I had add a comment about this somewhere: I'm tempted to change this before release so that the description is generic, and we just use a raw (not translated) message as the failure reason.

Do we have a generic error message that can be reused here?

On second thought, I guess it’s OK for these error messages to contain some technical details. -[MGLStyle removeSource:error:] is likely to be called as part of some larger task, not directly in response to a user action. (Mapbox Studio Preview and macosapp are the only applications that would likely contain a user-facing Remove Source command.) ADD_FILE_CONTENTS_FAILED_DESC is another localizable error description that wouldn’t be presented to the user, since the user has no awareness about file paths.

The documentation about underlying errors implies that some subsystem (in this case, the map SDK) would produce an error with less than user-friendly text. Examples include errors that originate from NSURLSession or NSFileManager. Such errors are sometimes localized, so it’s also OK to keep these strings in the localization files, but we should keep them to a minimum to avoid unnecessary translator effort.

Nonetheless, I’ve opened #13492 to revise these messages.

1ec5 · 2018-12-01T23:56:09Z

+        NSString *format = NSLocalizedStringWithDefaultValue(@"REMOVE_SRC_FAIL_MISMATCH_FMT", @"Foundation", nil, @"Identifier '%@' does not match source identifier '%s'", @"User-friendly error description");
+        NSString *localizedDescription = [NSString stringWithFormat:format,
+                                          self.identifier,
+                                          self.rawSource ? self.rawSource->getID().c_str() : "(null)"];


By using an Objective-C %@ placeholder, we can avoid hard-coding a (null):

self.rawSource ? @(self.rawSource->getID().c_str()) : nil

1ec5 · 2018-12-02T05:29:17Z

    }
+    else if (outError) {
+        // Consider raising an exception here
+        NSString *format = NSLocalizedStringWithDefaultValue(@"REMOVE_SRC_FAIL_MISMATCH_FMT", @"Foundation", nil, @"Identifier '%@' does not match source identifier '%s'", @"User-friendly error description");


On second thought, I guess it’s OK for these error messages to contain some technical details. -[MGLStyle removeSource:error:] is likely to be called as part of some larger task, not directly in response to a user action. (Mapbox Studio Preview and macosapp are the only applications that would likely contain a user-facing Remove Source command.) ADD_FILE_CONTENTS_FAILED_DESC is another localizable error description that wouldn’t be presented to the user, since the user has no awareness about file paths.

The documentation about underlying errors implies that some subsystem (in this case, the map SDK) would produce an error with less than user-friendly text. Examples include errors that originate from NSURLSession or NSFileManager. Such errors are sometimes localized, so it’s also OK to keep these strings in the localization files, but we should keep them to a minimum to avoid unnecessary translator effort.

Nonetheless, I’ve opened #13492 to revise these messages.

julianrex added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Nov 16, 2018

julianrex added this to the release-iowaska milestone Nov 16, 2018

julianrex self-assigned this Nov 16, 2018

julianrex requested review from fabian-guerra and jmkiley and removed request for jmkiley November 16, 2018 19:51

fabian-guerra reviewed Nov 17, 2018

View reviewed changes

fabian-guerra approved these changes Nov 19, 2018

View reviewed changes

Julian Rex added 4 commits November 19, 2018 18:20

Added -(BOOL)[MGLStyle removeSource:error:] that returns an NSError…

4cba691

… in case of failure. Updated existing test.

Updated change logs.

a8970cc

Only set _pendingSource & clear _mapView if the source was removed.

4696a8d

Updated strings file.

61dd7de

julianrex force-pushed the jrex/9692-remove-in-use-source branch from 865078d to 61dd7de Compare November 19, 2018 23:24

julianrex requested a review from 1ec5 as a code owner November 19, 2018 23:24

julianrex merged commit 90f609c into master Nov 20, 2018

julianrex deleted the jrex/9692-remove-in-use-source branch November 20, 2018 15:26

1ec5 reviewed Nov 20, 2018

View reviewed changes

1ec5 mentioned this pull request Dec 2, 2018

Refine source removal errors #13492

Merged

1ec5 reviewed Dec 2, 2018

View reviewed changes

julianrex mentioned this pull request May 14, 2019

Return an error object removeSource: is called on an in use MGLSource #9692

Closed

Conversation

julianrex commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabian-guerra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianrex Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabian-guerra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

julianrex commented Nov 16, 2018 •

edited

Loading

julianrex Nov 19, 2018 •

edited

Loading