Revamp MGLStyleLayer property types

[1ec5]

This PR reworks how style attribute values are represented in the runtime styling API. The intricate system of protocols adopted by Foundation classes via categories has been replaced with a simple umbrella value type, MGLStyleValue, that adopts lightweight generics. The parallel – and often incomplete – private protocols adopted by Foundation classes via categories has been replaced by a single templated C++ class, MGLStyleValueTransformer. C++ templates were chosen over C macros because they’re far easier to debug (once they compile, that is).

Everywhere the developer previously used a string literal, for instance, they must now wrap that string literal in an MGLStyleValue<NSString *>. That includes style functions: MGLStyleFunction, one of the classes under the MGLStyleValue umbrella, expects stop values to be wrapped in MGLStyleValues. See #6601 (comment) for a concrete example in both Swift and Objective-C. All this is necessary because the Objective-C language lacks variant support.

The goal is to add a veneer of type safety, even when working with style functions, yet avoid a proliferation of abstract classes or private protocols. This approach should also extend rather straightforwardly to property functions and future extensions to the style specification syntax. More importantly, this PR ensures internal type safety, avoiding what are in essence unchecked casts throughout the API implementation.

Fixes #5970. #6588 cleans up the style layer classes themselves, whereas this PR cleans up each style layer class’s properties.

Fixes #6078.

Lots to do:

• Implement value classes
• Prototype conversions between Objective-C and C++ types
• Get the changes to link
• Apply the changes to all the style attributes
• Get array getters/setters to compile
• Get tests to pass
• Remove unused -[MGLStyleFunction rawValueAtZoomLevel:]
• Update property documentation
• Document MGLStyleValue classes

/cc @frederoni @boundsj @incanus

[mention-bot]

@1ec5, thanks for your PR! By analyzing the history of the files in this pull request, we identified @frederoni, @incanus and @boundsj to be potential reviewers.

[1ec5]

Here are the current linker errors:

Apple Mach-O Linker (ld) Error Group
  "mbgl::style::PropertyValue<mbgl::style::LineJoinType> MBGLValueFromMGLStyleValue<NSValue* __strong, mbgl::style::LineJoinType>(MGLStyleValue<NSValue* __strong>*)", referenced from:

  "mbgl::style::PropertyValue<float> MBGLValueFromMGLStyleValue<NSNumber* __strong, float>(MGLStyleValue<NSNumber* __strong>*)", referenced from:

  "MGLStyleValue<NSValue* __strong>* MGLStyleValueFromMBGLValue<mbgl::style::LineJoinType, NSValue* __strong>(mbgl::style::PropertyValue<mbgl::style::LineJoinType>&)", referenced from:

  "MGLStyleValue<NSNumber* __strong>* MGLStyleValueFromMBGLValue<float, NSNumber* __strong>(mbgl::style::PropertyValue<float>&)", referenced from:

clang: error: linker command failed with exit code 1 (use -v to see invocation)

[1ec5]

The mangled version:

Undefined symbols for architecture x86_64:
  "__Z26MBGLValueFromMGLStyleValueIU8__strongP7NSValueN4mbgl5style12LineJoinTypeEENS4_13PropertyValueIT0_EEP13MGLStyleValueIT_E", referenced from:
      -[MGLLineStyleLayer setLineJoin:] in MGLLineStyleLayer.o
  "__Z26MBGLValueFromMGLStyleValueIU8__strongP8NSNumberfEN4mbgl5style13PropertyValueIT0_EEP13MGLStyleValueIT_E", referenced from:
      -[MGLLineStyleLayer setLineMiterLimit:] in MGLLineStyleLayer.o
  "__Z26MGLStyleValueFromMBGLValueIN4mbgl5style12LineJoinTypeEU8__strongP7NSValueEP13MGLStyleValueIT0_ERNS1_13PropertyValueIT_EE", referenced from:
      -[MGLLineStyleLayer lineJoin] in MGLLineStyleLayer.o
  "__Z26MGLStyleValueFromMBGLValueIfU8__strongP8NSNumberEP13MGLStyleValueIT0_ERN4mbgl5style13PropertyValueIT_EE", referenced from:
      -[MGLLineStyleLayer lineMiterLimit] in MGLLineStyleLayer.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Indeed, there’s no trace of MBGLValueFromMGLStyleValue at all in MGLStyleAttributeFunction.o.

[1ec5]

As I work through the linker issues, a design question for everyone: am I swinging the pendulum too far towards type safety at the expense of concision? Something like this:

// MGLMapViewDelegate.mapViewDidFinishLoadingMap(_:)
let layer = mapView.style().layer(identifier: "motorway") as? MGLLineStyleLayer
layer?.lineGapWidth = 5 as MGLStyleAttributeValue
layer?.lineColor = UIColor.blue
let fn = MGLStyleAttributeFunction()
fn.stops = [1: 5, 18: 3]
layer?.lineWidth = fn

will probably turn into this:

// MGLMapViewDelegate.mapViewDidFinishLoadingMap(_:)
let layer = mapView.style().layer(identifier: "motorway") as? MGLLineStyleLayer
layer?.lineGapWidth = MGLStyleValue(rawValue: 5)
layer?.lineColor = MGLStyleValue(rawValue: .blue)
layer?.lineWidth = MGLStyleValue(stops: [
    1: MGLStyleValue(rawValue: 5),
    18: MGLStyleValue(rawValue: 3),
])

The equivalent code in Objective-C:

// -[MGLMapViewDelegate mapViewDidFinishLoadingMap:]
MGLLineStyleLayer *layer = (MGLLineStyleLayer *)[mapView.style layerWithIdentifier:@"motorway"];
layer.lineGapWidth = @5;
layer.lineColor = UIColor.blueColor;
MGLStyleAttributeFunction *fn = [[MGLStyleAttributeFunction alloc] init];
fn.stops = @{@1: @5, @18: @3};
layer.lineWidth = fn;

will probably turn into this:

// -[MGLMapViewDelegate mapViewDidFinishLoadingMap:]
MGLLineStyleLayer *layer = (MGLLineStyleLayer *)[mapView.style layerWithIdentifier:@"motorway"];
layer.lineGapWidth = [MGLStyleValue<NSNumber *> valueWithRawValue:@5];
layer.lineColor = [MGLStyleValue<UIColor> valueWithRawValue:UIColor.blueColor];
MGLStyleAttributeFunction *fn = [[MGLStyleAttributeFunction alloc] init];
NSDictionary *stops = @{
    @1: [MGLStyleValue<NSNumber *> valueWithRawValue:@5],
    @18: [MGLStyleValue<NSNumber *> valueWithRawValue:@3],
};
layer.lineWidth = [MGLStyleValue<NSNumber *> valueWithStops:stops];

(Swift is able to infer the <UIColor> and <Float>s in this case, but Objective-C probably won’t.)

This recursive use of MGLStyleValue is pretty ugly, although it’s flexible enough to accommodate property functions (#4860) and future extensions to the style specification syntax, such as token defaults (mapbox/mapbox-gl-style-spec#104) and conditionals (mapbox/mapbox-gl-style-spec#402). As a compromise, we could allow the stops dictionary to contain values of type e.g. Float or NSNumber * rather than MGLStyleValue. But any extensions to the style specification syntax will almost certainly be backwards-incompatible.

[incanus]

The new Swift implementation above is slightly more awkward. Things like this, mentioning raw, rub me a little funny:

layer?.lineColor = MGLStyleValue(rawValue: .blue)

The new Objective-C implementation feels a lot more awkward.

layer.lineColor = [MGLStyleValue<UIColor> valueWithRawValue:UIColor.blueColor];

This is merely setting a property (a short one at that) to a color, but it's verbose even for Objective-C. The ideal here is:

layer.lineColor = [UIColor blueColor];

Something I've been working with lately is SceneKit, which features a number of types that can be passed as contents on SCNMaterialProperty objects.

Notably:

Discussion

For details on each visual property and the ways their contents affect a material’s appearance, see SCNMaterial.

You can set a value for this property using any of the following types:

• A color (NSColor/UIColor or CGColorRef), specifying a constant color across the material’s surface
• An image (NSImage/UIImage or CGImageRef), specifying a texture to be mapped across the material’s surface
• An NSString or NSURL object specifying the location of an image file
• A Core Animation layer (CALayer)
• A texture (SKTexture, MDLTexture, MTLTexture, or GLKTextureInfo)
• A SpriteKit scene (SKScene)
• A specially formatted image or array of six images, specifying the faces of a cube map

This is even broader than our property APIs, yet still relies on an untyped id for the contents property.

These are presumably meant to be used dynamically at runtime, else SCNMaterialProperty would only feature initializers that take contents at instantiation and then make them immutable in future.

If you use a wrong type, everything compiles but the bad setter is just silently ignored. I don't feel like this is user-hostile and really keeps things simple, no?

override func viewDidLoad() {
    super.viewDidLoad()

    let ball = SCNSphere(radius: 0.5)
    let node = SCNNode(geometry: ball)
    let sceneView = SCNView(frame: view.bounds)
    sceneView.scene = SCNScene()
    sceneView.scene?.rootNode.addChildNode(node)
    view.addSubview(sceneView)

    ball.firstMaterial?.diffuse.contents = UIColor.red // good
    ball.firstMaterial?.specular.contents = UIView(frame: view.bounds) // bad, ignored
}

Are we overthinking this API?

[1ec5]

Thank you for this example @incanus. It certainly speaks to a concern I’ve had as we’ve been building out this API. There’s a lot to be said for the simplicity of just using id or even putting all the attributes into an untyped NSDictionary, as do many Cocoa APIs like NSAttributedString. That said, SceneKit’s API was originally released in 2012, long before Swift launched with associated types (variants) and Objective-C gained support for lightweight generics. I wonder if the API would’ve been this fast and loose with types if Swift had been out by then, given that it runs counter to Swift’s preference for a strict type system.

One alternative I proposed awhile back, in #5970 (comment), was to rely just as heavily on protocols as we do now, but make the protocols more specific. If we go that route, I think we need to avoid the mess of private categories that we currently rely on: it’s too error-prone on our end and so easy to misuse on the developer’s.

[1ec5]

Things like this, mentioning raw, rub me a little funny

I’m not wedded to the “raw value” terminology, although it’s consistent with raw values of Swift enumerations. For this PR, I was considering some helper classes like MGLStyleNumber, the same way NSNumber is a convenience subclass for NSValue.

[incanus]

FWIW SCNMaterialProperty is from iOS 8+, meaning 2014. So, a bit newer and probably simultaneously / caught by surprise with Swift. Still, a data point 😄

I'll dig around a bit to see if Apple is introducing any new APIs like this, just as more data.

Still thinking on this a bit.

[frederoni]

Great rework @1ec5
It's way more robust and future proof than before but indeed, not very concise.
Why not add a few convenient instance methods on UIColor, NSNumber etc in order to turn this

layer.lineGapWidth = [MGLStyleValue<NSNumber *> valueWithRawValue:@5];
layer.lineColor = [MGLStyleValue<UIColor> valueWithRawValue:UIColor.blueColor];
MGLStyleAttributeFunction *fn = [[MGLStyleAttributeFunction alloc] init];
NSDictionary *stops = @{
    @1: [MGLStyleValue<NSNumber *> valueWithRawValue:@5],
    @18: [MGLStyleValue<NSNumber *> valueWithRawValue:@3],
};

into this:

layer.lineGapWidth = @5.mglValue;
layer.lineColor = UIColor.blueColor.mglColor;
MGLStyleAttributeFunction *fn = [[MGLStyleAttributeFunction alloc] init];
NSDictionary *stops = @{
    @1: @5.mglValue,
    @18: @3.mglValue,
};

[1ec5]

Why not add a few convenient instance methods on UIColor, NSNumber

That’s a clever idea. -mgl_styleConstant wouldn’t be especially discoverable, but that’s OK because it’s just a shortcut for the more verbose syntax that would be discoverable. On the other hand, such a method has the potential to muddle the developer’s idea of what exactly is being passed in. It’s worth exploring, certainly.

[1ec5]

With @boundsj’s help, I was able to get past the linker issues over the weekend. I also fixed a number of silly mistakes related to switching a method to use an inout parameter instead of a return value.

This has been the sole remaining beta blocker for awhile now. The criteria for a beta blocker is that it prevents us from being able to encourage developers to start developing against a new feature, because we expect to break their code significantly in a future prerelease. Given the problems we’ve already identified with the current API, like #5970 (comment), I believe #5970 still deserves to be a beta blocker – we need to change something.

How about we land these changes, verbosity and all, and release the beta? Feedback we get from the beta can inform whether we:

1. Introduce the Foundation category conveniences proposed in Revamp MGLStyleLayer property types #6601 (comment);
2. Dial down to unqualified ids as proposed in Revamp MGLStyleLayer property types #6601 (comment); or
3. Like (2), but qualified with type-specific protocols like MGLBoolRepresentable, as proposed in Add category convenience methods to NSNumber for style values (was: Runtime styling properties are too loosely typed) #5970 (comment).

We can layer all three approaches atop the changes in this PR. MGLStyleValue will continue to be accepted by the API in future betas, so any changes to client code will be optional. After all, the bulk of this PR is about improving internal type safety, which we want regardless of external ergonomics.

[boundsj]

👍

[1ec5]

Thanks everyone for your feedback. I’m going to finish the documentation and land this PR for the beta. Then we’ll continue the discussion in #5970.

[1ec5]

Filed #6712 about the Qt5 build bot failure.

[boundsj]

🚢 🚀