Reversed MGLStyle.layers

[frederoni]

Fixed #7474 Reverse MGLStyle.layers.

[mention-bot]

@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]

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.

[frederoni]

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 >.

[1ec5]

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

[1ec5]

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

[frederoni]

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?

[1ec5]

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?

[frederoni]

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

[1ec5]

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]

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]

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]

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).

[1ec5]

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]

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]

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]

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