-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
@ivovandongen, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @boundsj and @tmpsantos to be potential reviewers. |
43008a0
to
9f71100
Compare
cc @1ec5 @jfirebaugh |
@@ -190,7 +190,11 @@ - (void)addLayer:(MGLStyleLayer *)layer | |||
layer]; | |||
} | |||
|
|||
self.mapView.mbglMap->addLayer(std::unique_ptr<mbgl::style::Layer>(layer.layer)); | |||
try { | |||
self.mapView.mbglMap->addLayer(std::unique_ptr<mbgl::style::Layer>(layer.layer)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the other addLayer()
call in this class? (There’s also one in MGLMapView.mm, but that’ll go away as part of #7250.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added (tests) for MGLStyle#insertLayer
and and MGLMapView#insertCustomStyleLayerWithIdentifier
.
try { | ||
self.mapView.mbglMap->addLayer(std::unique_ptr<mbgl::style::Layer>(layer.layer)); | ||
} catch (std::runtime_error & err) { | ||
[NSException raise:@"Could not add layer" format:@"%s", err.what()]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me that we need to be more disciplined about exception names in the SDK: #7258. (Not required for this PR.)
@@ -0,0 +1,25 @@ | |||
#import "MGLMapViewTests.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this file isn’t specific to iOS, it should live in platform/darwin/test/ and also be included in macos.xcodeproj. (This is the theme of our contributing documentation, although we neglected to point out that tests should be shared too when possible.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the files and added to macos as well. Excluded the custom layer tests for macos as the header is ios specific. Also changed the source test according to #7203 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s fine; the custom style layer API is getting folded into the normal style layer API in #7250, so soon there won’t be anything extra to test.
1008bbe
to
89bfd97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Once we merge release-ios-v3.4.0 back to master, the particular call sites you’re wrapping in try-catch blocks will be replaced by new call sites in MGLStyle and the various MGLStyleLayer classes, due to #6793, #6793, #6097, and #7250. So we’ll need to be vigilant with that merge.
/cc @boundsj
MGLCustomStyleLayerPreparationHandler preparationHandler = ^{}; | ||
MGLCustomStyleLayerDrawingHandler drawingHandler = ^(CGSize size, CLLocationCoordinate2D centerCoordinate, double zoomLevel, CLLocationDirection direction, CGFloat pitch, CGFloat perspectiveSkew) { | ||
}; | ||
MGLCustomStyleLayerCompletionHandler completionHander = ^{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, feel free to inline these blocks into the method call. Long lines are par for the course in Objective-C. 😉
The iOS test failure appears to be what I fixed in #7228 on the release-ios-v3.4.0 branch. |
89bfd97
to
d0cb5fa
Compare
@@ -211,7 +215,11 @@ - (void)insertLayer:(MGLStyleLayer *)layer belowLayer:(MGLStyleLayer *)otherLaye | |||
} | |||
|
|||
const mbgl::optional<std::string> belowLayerId{otherLayer.identifier.UTF8String}; | |||
self.mapView.mbglMap->addLayer(std::unique_ptr<mbgl::style::Layer>(layer.layer), belowLayerId); | |||
try { | |||
self.mapView.mbglMap->addLayer(std::unique_ptr<mbgl::style::Layer>(layer.layer), belowLayerId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that #7282 has landed – including the changes in #6793, #6793, #6097, and #7250 – you’ll need to wrap any call to -[MGLStyleLayer addToMapView:]
in this try-catch block or alternatively move this try-catch block to every implementation of -addToMapView:
in the concrete subclasses of MGLStyleLayer.
04cbd5b
to
c005962
Compare
c005962
to
93b5ea0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once #7250 lands, we’ll want to push these try-catch blocks into -addToMapView:belowLayer:
and -removeFromMapView:
, and the separate code path for custom layers will go away, but this is the best we can do for now.
MGLDrawCustomStyleLayer, MGLFinishCustomStyleLayer, context), | ||
otherIdentifier ? mbgl::optional<std::string>(otherIdentifier.UTF8String) : mbgl::optional<std::string>()); | ||
} catch (std::runtime_error & err) { | ||
[NSException raise:@"MGLRedundantLayerIdentiferException" format:@"%s", err.what()]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized we’ve been saying “identifer” instead of “identifier”, both for layers and sources. I can fix that typo in a separate PR, since we also need to add a note to the documentation for the affected methods about MGLRedundantLayerIdentifierException
(prior art involving sources).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you’re in a good spot to do it, can you rename MGLRedundantLayerIdentiferException
to MGLRedundantLayerIdentiferException
? I don’t think we have any instances of it in the release branch, so this is a good branch to do it in. I can take care of MGLRedundantSourceIdentiferException
separately: #7314.
93b5ea0
to
617d3e4
Compare
617d3e4
to
1ed3ff1
Compare
[layer addToMapView:self.mapView]; | ||
try { | ||
[layer addToMapView:self.mapView]; | ||
} catch (std::runtime_error & err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're catching a generic exception here, but always forward a MGLRedundantLayerIdentifierException
. What we should do long term is to convert all of the generic std::runtime_error
into individual exception types, and to exception-specific catches that we can then forward.
@@ -198,6 +198,16 @@ Layer* Style::getLayer(const std::string& id) const { | |||
Layer* Style::addLayer(std::unique_ptr<Layer> layer, optional<std::string> before) { | |||
// TODO: verify source | |||
|
|||
//Guard against duplicate layer ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code style is to have a space after //
.
|
||
if (it != layers.end()) { | ||
std::string msg = "Layer " + layer->getID() + " already exists"; | ||
throw std::runtime_error(msg.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to to c_str()
here; std::runtime_error
also accepts std::string
. You could change this to something like this;
throw std::runtime_error(std::string{"Layer "} + layer->getID() + " already exists");
style.addLayer(std::make_unique<LineLayer>("line", "unusedsource")); | ||
FAIL() << "Should not have been allowed to add a duplicate layer id"; | ||
} catch (std::runtime_error) { | ||
//Expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check message here
try { | ||
style.addLayer(std::make_unique<LineLayer>("line", "unusedsource")); | ||
FAIL() << "Should not have been allowed to add a duplicate layer id"; | ||
} catch (std::runtime_error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rule for exceptions in C++ is to throw by value and catch a const
reference
[styleLayer addToMapView:self.mapView]; | ||
try { | ||
[styleLayer addToMapView:self.mapView]; | ||
} catch (std::runtime_error & err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
XCTAssertThrowsSpecificNamed([self.mapView.style insertLayer:[[MGLFillStyleLayer alloc] initWithIdentifier:@"my-layer" source:source] aboveLayer:initial], NSException, @"MGLRedundantLayerIdentifierException"); | ||
XCTAssertThrowsSpecificNamed([self.mapView.style insertLayer:[[MGLFillStyleLayer alloc] initWithIdentifier:@"my-layer" source:source] atIndex:0], NSException, @"MGLRedundantLayerIdentifierException"); | ||
|
||
#if TARGET_OS_IPHONE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Macros are not indented
1ed3ff1
to
57b0ba7
Compare
@kkaefer Thanks for the review, I've addressed your notes. |
Fixes #7254