Reversed MGLStyle.layers #7484

frederoni · 2016-12-19T15:07:05Z

Fixed #7474 Reverse MGLStyle.layers.

mention-bot · 2016-12-19T15:07:07Z

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

1ec5

Some off-by-one errors remain because we need to account for the reversal in some special cases.

macosapp's Layers sidebar should continue to list the layers top to bottom, but I can take care of that in a separate PR.

1ec5 · 2016-12-19T16:07:43Z

platform/darwin/src/MGLStyle.mm

@@ -293,7 +293,7 @@ - (void)insertObject:(MGLStyleLayer *)styleLayer inLayersAtIndex:(NSUInteger)ind
        }


The cases above need to be adjusted. If you pass in an index equal to the number of layers, an exception should be raised. If you pass in an index of 0, the layer should be inserted below the bottommost layer, not above the topmost layer.

What if you pass in an index of 0 and the number of layers is 0? The layer should be inserted and no exception should be raised. -[NSMutableArray insertObject:atIndex:] allows index=size but + 1 is out of bounds. The second argument still holds but the first should be >.

I agree with you: #7484 (comment). Unfortunately it's this comment that stuck around. 😁

1ec5 · 2016-12-19T16:09:30Z

platform/darwin/src/MGLStyle.mm

@@ -308,7 +308,7 @@ - (void)removeObjectFromLayersAtIndex:(NSUInteger)index
        [NSException raise:NSRangeException


It's valid to remove the last (topmost) layer in the style.

These off-by-one errors are confusing but it's still possible to remove the topmost layer.
Perhaps a greater than or equal to operator would make it easier to reason about?

If you pass in the index of the last layer in the array, wouldn’t index be equal to layers.size() - 1, triggering this exception?

It would be equal but it wouldn't trigger because the operator is >

Doh, I’m blind! 😆 OK, I remember intentionally using > … - 1 here to parallel the subtraction below. Now that that line below has been simplified, let’s use >= instead.

1ec5 · 2016-12-19T22:06:36Z

Some exception messages still need to be adjusted: for example, this message should say “cannot be placed below”, not above.

I think we need to double-check all three insertion methods to make sure they insert before or after the sibling layer as appropriate.

frederoni · 2016-12-20T14:07:55Z

macosapp's Layers sidebar should continue to list the layers top to bottom, but I can take care of that in a separate PR.

Feel free to implement that in this PR if it makes it easier.

1ec5 · 2016-12-20T20:58:33Z

platform/darwin/src/MGLStyle.mm

@@ -282,18 +282,19 @@ - (void)insertObject:(MGLStyleLayer *)styleLayer inLayersAtIndex:(NSUInteger)ind
         styleLayer];
    }
    auto layers = self.mapView.mbglMap->getLayers();
-    if (index > layers.size()) {
+    if (index >= layers.size()) {
        [NSException raise:NSRangeException
                    format:@"Cannot insert style layer at out-of-bounds index %lu.", (unsigned long)index];
    } else if (index == 0) {


This should be a special case for when index == layers.size(), not for when index is 0, and it should continue to insert below nil. The else case below will crash for index == layers.size() because layers.at(layers.size()) is out of bounds, even if you make the change below to say layers.at(index).

index == layers.size() gets caught by the case above, which I think is incorrect: you can append a single object to NSMutableArray by passing its count into -insertObject:atIndex:; an exception only gets raised if the index is count + 1 or higher.

1ec5 · 2016-12-20T21:01:34Z

platform/darwin/src/MGLStyle.mm

        } catch (const std::runtime_error & err) {
            [NSException raise:@"MGLRedundantLayerIdentifierException" format:@"%s", err.what()];
        }
    } else {
        try {
-            MGLStyleLayer *sibling = [self layerFromMBGLLayer:layers.at(layers.size() - index)];
+            MGLStyleLayer *sibling = [self layerFromMBGLLayer:layers.at(index + 1)];


Inserting a layer at an index should put it below (before) the layer currently at that index. Therefore, this should be layers.at(index).

1ec5 · 2016-12-20T21:08:16Z

platform/darwin/src/MGLStyle.mm

@@ -448,7 +449,7 @@ - (void)insertLayer:(MGLStyleLayer *)layer aboveLayer:(MGLStyleLayer *)sibling {
    if (index + 1 > layers.size()) {
        [NSException raise:NSInvalidArgumentException
                    format:
-         @"A style layer cannot be placed above %@ in the style. "
+         @"A style layer cannot be placed below %@ in the style. "


Oops, I misread the diff when I suggested rewording this to say “below” instead of “above” in #7484 (comment). It should indeed say “above”, because that’s what this method is all about.

1ec5 · 2016-12-21T15:57:45Z

platform/darwin/src/MGLStyle.mm

@@ -293,7 +293,7 @@ - (void)insertObject:(MGLStyleLayer *)styleLayer inLayersAtIndex:(NSUInteger)ind
        }


I agree with you: #7484 (comment). Unfortunately it's this comment that stuck around. 😁

frederoni added bug iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS release blocker Blocks the next final release runtime styling labels Dec 19, 2016

frederoni added this to the ios-v3.4.0 milestone Dec 19, 2016

frederoni force-pushed the fred-reverse-layers-7474 branch from 6dd6fae to 81a728e Compare December 19, 2016 15:22

1ec5 suggested changes Dec 19, 2016

View reviewed changes

1ec5 mentioned this pull request Dec 20, 2016

Update and reformat documentation for runtime styling #7475

Merged

1ec5 suggested changes Dec 20, 2016

View reviewed changes

frederoni force-pushed the fred-reverse-layers-7474 branch 2 times, most recently from f2e6c08 to d885d64 Compare December 21, 2016 13:20

1ec5 approved these changes Dec 21, 2016

View reviewed changes

[ios, macos] reversed MGLStyle.layers

c8149bf

frederoni force-pushed the fred-reverse-layers-7474 branch from d885d64 to c8149bf Compare December 22, 2016 12:05

frederoni merged commit 916cd6c into release-ios-v3.4.0 Dec 22, 2016

frederoni deleted the fred-reverse-layers-7474 branch December 22, 2016 12:35

1ec5 mentioned this pull request Jan 1, 2017

Arrange layers top to bottom in sidebar #7564

Merged

boundsj mentioned this pull request Jan 3, 2017

Reverse MGLStyle.layers #7474

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reversed MGLStyle.layers #7484

Reversed MGLStyle.layers #7484

frederoni commented Dec 19, 2016 •

edited

Loading

mention-bot commented Dec 19, 2016

1ec5 left a comment

1ec5 Dec 19, 2016

frederoni Dec 21, 2016 •

edited

Loading

1ec5 Dec 21, 2016

1ec5 Dec 19, 2016

frederoni Dec 19, 2016

1ec5 Dec 19, 2016

frederoni Dec 19, 2016

1ec5 Dec 19, 2016

1ec5 commented Dec 19, 2016 •

edited

Loading

frederoni commented Dec 20, 2016

1ec5 Dec 20, 2016

1ec5 Dec 20, 2016

1ec5 Dec 20, 2016

1ec5 Dec 20, 2016

1ec5 Dec 21, 2016

		@@ -293,7 +293,7 @@ - (void)insertObject:(MGLStyleLayer *)styleLayer inLayersAtIndex:(NSUInteger)ind
		}

		@@ -308,7 +308,7 @@ - (void)removeObjectFromLayersAtIndex:(NSUInteger)index
		[NSException raise:NSRangeException

Reversed MGLStyle.layers #7484

Reversed MGLStyle.layers #7484

Conversation

frederoni commented Dec 19, 2016 • edited Loading

mention-bot commented Dec 19, 2016

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Dec 19, 2016 • edited Loading

frederoni commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni commented Dec 19, 2016 •

edited

Loading

frederoni Dec 21, 2016 •

edited

Loading

1ec5 commented Dec 19, 2016 •

edited

Loading