Skip to content
This repository was archived by the owner on Aug 8, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions platform/darwin/resources/Base.lproj/Foundation.strings
Original file line number Diff line number Diff line change
Expand Up @@ -298,3 +298,9 @@
/* OpenStreetMap short name attribution */
"OSM_SHORT_NAME" = "OSM";

/* User-friendly error description */
"REMOVE_SRC_FAIL_IN_USE_FMT" = "Source '%@' is in use, cannot remove.";

/* User-friendly error description */
"REMOVE_SRC_FAIL_MISMATCH_FMT" = "Identifier '%1$@' does not match source identifier '%2$s'";

35 changes: 32 additions & 3 deletions platform/darwin/src/MGLSource.mm
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#import "MGLSource_Private.h"
#import "MGLStyle_Private.h"
#import "MGLMapView_Private.h"
#import "NSBundle+MGLAdditions.h"

#include <mbgl/style/style.hpp>
#include <mbgl/map/map.hpp>
Expand Down Expand Up @@ -57,11 +58,39 @@ - (void)addToMapView:(MGLMapView *)mapView {
_mapView.style.rawStyle->addSource(std::move(_pendingSource));
}

- (void)removeFromMapView:(MGLMapView *)mapView {
- (BOOL)removeFromMapView:(MGLMapView *)mapView error:(NSError * __nullable * __nullable)outError {
BOOL removed = NO;

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

auto removedSource = mapView.style.rawStyle->removeSource(self.identifier.UTF8String);

if (removedSource) {
removed = YES;
_pendingSource = std::move(removedSource);
_mapView = nil;
} else if (outError) {
NSString *format = NSLocalizedStringWithDefaultValue(@"REMOVE_SRC_FAIL_IN_USE_FMT", @"Foundation", nil, @"Source '%@' is in use, cannot remove.", @"User-friendly error description");
NSString *localizedDescription = [NSString stringWithFormat:format, self.identifier];

*outError = [NSError errorWithDomain:MGLErrorDomain
code:MGLErrorCodeSourceIsInUseCannotRemove
userInfo:@{ NSLocalizedDescriptionKey : localizedDescription }];
}
}
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");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

NSString *localizedDescription = [NSString stringWithFormat:format,
self.identifier,
self.rawSource ? self.rawSource->getID().c_str() : "(null)"];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


*outError = [NSError errorWithDomain:MGLErrorDomain
code:MGLErrorCodeSourceIdentifierMismatch
userInfo:@{ NSLocalizedDescriptionKey : localizedDescription }];
}

return removed;
}

- (NSString *)description {
Expand Down
2 changes: 1 addition & 1 deletion platform/darwin/src/MGLSource_Private.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ struct SourceWrapper {
to the `MGLSource` instance and the unique_ptr reference is valid again. It is
safe to add the source back to the style after it is removed.
*/
- (void)removeFromMapView:(MGLMapView *)mapView;
- (BOOL)removeFromMapView:(MGLMapView *)mapView error:(NSError * __nullable * __nullable)outError;

@end

Expand Down
21 changes: 21 additions & 0 deletions platform/darwin/src/MGLStyle.h
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,27 @@ MGL_EXPORT
*/
- (void)removeSource:(MGLSource *)source;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
Removes a source from the current style.

@note Source identifiers are not guaranteed to exist across styles or different
versions of the same style. Applications that use this API must first set the
style URL to an explicitly versioned style using a convenience method like
`+[MGLStyle outdoorsStyleURLWithVersion:]`, `MGLMapView`’s “Style URL”
inspectable in Interface Builder, or a manually constructed `NSURL`. This
approach also avoids source identifer name changes that will occur in the default
style’s sources over time.

@param source The source to remove from the current style.
@param outError Upon return, if an error has occurred, a pointer to an `NSError`
object describing the error. Pass in `NULL` to ignore any error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: nil

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@return `YES` if `source` was removed successfully. If `NO`, `outError` contains
an `NSError` object describing the problem.
*/
- (BOOL)removeSource:(MGLSource *)source error:(NSError * __nullable * __nullable)outError;


#pragma mark Managing Style Layers

/**
Expand Down
9 changes: 8 additions & 1 deletion platform/darwin/src/MGLStyle.mm
Original file line number Diff line number Diff line change
Expand Up @@ -219,16 +219,23 @@ - (void)addSource:(MGLSource *)source

- (void)removeSource:(MGLSource *)source
{
[self removeSource:source error:nil];
}

- (BOOL)removeSource:(MGLSource *)source error:(NSError * __nullable * __nullable)outError {
MGLLogDebug(@"Removing source: %@", source);

if (!source.rawSource) {
[NSException raise:NSInvalidArgumentException format:
@"The source %@ cannot be removed from the style. "
@"Make sure the source was created as a member of a concrete subclass of MGLSource.",
source];
}
[source removeFromMapView:self.mapView];

return [source removeFromMapView:self.mapView error:outError];
}


- (nullable NSArray<MGLAttributionInfo *> *)attributionInfosWithFontSize:(CGFloat)fontSize linkColor:(nullable MGLColor *)linkColor {
// It’d be incredibly convenient to use -sources here, but this operation
// depends on the sources being sorted in ascending order by creation, as
Expand Down
6 changes: 5 additions & 1 deletion platform/darwin/src/MGLTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ typedef NS_ENUM(NSInteger, MGLErrorCode) {
/** An attempt to load the style failed. */
MGLErrorCodeLoadStyleFailed = 5,
/** An error occurred while snapshotting the map. */
MGLErrorCodeSnapshotFailed = 6
MGLErrorCodeSnapshotFailed = 6,
/** Source is in use and cannot be removed */
MGLErrorCodeSourceIsInUseCannotRemove = 7,
/** Source is in use and cannot be removed */
MGLErrorCodeSourceIdentifierMismatch = 8
};

/** Options for enabling debugging features in an `MGLMapView` instance. */
Expand Down
7 changes: 6 additions & 1 deletion platform/darwin/test/MGLStyleTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,12 @@ - (void)testRemovingSourceInUse {
[self.style addLayer:fillLayer];

// Attempt to remove the raster tile source
[self.style removeSource:rasterTileSource];
NSError *error;
BOOL result = [self.style removeSource:rasterTileSource error:&error];

XCTAssertFalse(result);
XCTAssertEqualObjects(error.domain, MGLErrorDomain);
XCTAssertEqual(error.code, MGLErrorCodeSourceIsInUseCannotRemove);

// Ensure it is still there
XCTAssertTrue([[self.style sourceWithIdentifier:rasterTileSource.identifier] isMemberOfClass:[MGLRasterTileSource class]]);
Expand Down
1 change: 1 addition & 0 deletions platform/ios/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
* Added `MGLLoggingConfiguration` and `MGLLoggingBlockHandler` that handle error and fault events produced by the SDK. ([#13235](https://github.com/mapbox/mapbox-gl-native/pull/13235))
* This SDK’s dynamic framework now has a bundle identifier of `com.mapbox.Mapbox`. ([#12857](https://github.com/mapbox/mapbox-gl-native/pull/12857))
* Modified the behavior of the map view so that programmatic camera transitions can no longer be interrupted by user interaction when `MGLMapView.zoomEnabled`, `MGLMapView.rotateEnabled`, `MGLMapView.scrollEnabled`, and `MGLMapView.pitchEnabled` are set to false. ([#13362](https://github.com/mapbox/mapbox-gl-native/pull/13362))
* Added `-[MGLStyle removeSource:error:]` that returns a `BOOL` indicating success (and an optional `NSError` in case of failure). ([#13399](https://github.com/mapbox/mapbox-gl-native/pull/13399))

## 4.6.0 - November 7, 2018

Expand Down
6 changes: 4 additions & 2 deletions platform/macos/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
# Changelog for Mapbox Maps SDK for macOS

## master

* Added `-[MGLStyle removeSource:error:]` that returns a `BOOL` indicating success (and an optional `NSError` in case of failure). ([#13399](https://github.com/mapbox/mapbox-gl-native/pull/13399))

## 0.12.0 - November 8, 2018

* Renamed `-[MGLOfflineStorage putResourceWithUrl:data:modified:expires:etag:mustRevalidate:]` to `-[MGLOfflineStorage preloadData:forURL:modificationDate:expirationDate:eTag:mustRevalidate:]`. ([#13318](https://github.com/mapbox/mapbox-gl-native/pull/13318))
* This SDK’s dynamic framework now has a bundle identifier of `com.mapbox.Mapbox`. ([#12857](https://github.com/mapbox/mapbox-gl-native/pull/12857))

## master

### Styles and rendering

* `MGLSymbolStyleLayer.text` can now be set to rich text with varying fonts and text sizes. ([#12624](https://github.com/mapbox/mapbox-gl-native/pull/12624))
Expand Down