Add category convenience methods to NSNumber for style values (was: Runtime styling properties are too loosely typed)

[1ec5]

We currently use the MGLStyleAttributeValue protocol to allow any style attribute property to be set to the appropriate value type or an instance of MGLStyleAttributeFunction. While this approach is slightly more strongly typed than id, preventing the developer from setting a property to e.g. a UIButton, it doesn’t keep the developer from setting the property to an NSString where an NSNumber is expected or an NSArray where an NSValue is expected. The attribute properties’ types should be more specific, allowing the compiler to catch common developer errors.

/cc @frederoni @incanus @jfirebaugh

[1ec5]

Solution 1: More protocols

We’d need type-specific protocols that each conform to MGLStyleAttributeValue:

• MGLColorStyleAttributeValue (implemented by NSColor, UIColor, and MGLStyleAttributeFunction)
• MGLNumberStyleAttributeValue (implemented by NSNumber and MGLStyleAttributeFunction)
• MGLArrayStyleAttributeValue (implemented by NSArray and MGLStyleAttributeFunction)
• Etc.

This isn’t a huge departure from what we’re already doing. It’s just a matter of defining some additional protocols and having the existing categories and properties adopt them.

One downside is that people may see id <MGLStringStyleAttributeValue> and not realize they can treat it as an NSString, even after reading the definition of MGLStringStyleAttributeValue.

[1ec5]

Solution 2: Lightweight generics

All properties would be of type MGLStyleAttributeValue, which should be a class with a single type parameter that determines the primitive underlying type of the value. Perhaps something like this:

@interface MGLStyleAttributeValue<T> : NSObject

+ (instancetype)valueWithRawValue:(T)rawValue;
+ (instancetype)valueWithFunctionType:(MGLStyleAttributeFunctionType)functionType property:(NSString *)property base:(NSNumber *)base stops:(NSDictionary<NSNumber *, MGLStyleAttributeValue<T> *> *)stops;

// MGLStyleAttributeFunctionTypeNone if created using +valueWithRawValue:
@property (nonatomic) MGLStyleAttributeFunctionType functionType;

- (T)rawValueAtZoomLevel:(double)zoomLevel;

@end

The conversion from Objective-C types to C++ types would occur in each individual property, but we’d factor out almost everything into macros similar to the current MGLSetEnumProperty() and MGLGetEnumProperty(). (I think we should consider doing that anyways.)

In Swift 2.2, type parameters in lightweight generics are ignored, making the properties just as loosely typed as we have now. So we’d need to clarify the expected type in documentation and enforce it at runtime via assertions. In Swift 3, type parameters are fully bridged from Objective-C. I’m not sure about Swift 2.3.

This approach greatly simplifies the runtime styling API implementation. It allows us to centralize the logic for converting between Objective-C and C++ types and remove the disparate public and private categories. It strengthens the type system, shifting most type checking to compile time.

Dealing with attributes would get a bit more verbose. Instead of setting a paint attribute property to @"camera-16", you’d set it to [MGLStyleAttributeValue valueWithRawValue:@"camera-16"] in Objective-C, or MGLStyleAttributeValue(rawValue: "camera-16") in Swift.

[frederoni]

Solution 3: More setters and getters

We’d need to generate at least one additional setter and getter per property.
This approach would make the API as strongly typed as iOS/macOS developers are used to. The downside is bloated layer implementation and excessive code completion which is debatable. I'd also expect the ETA of this solution to exceed the aforementioned.

// Idiomatic primitive types, structs and objects:
waterLayer.fillColor = [UIColor redColor];
waterLayer.fillTranslate = CGVectorMake(10, 10);
stateLayer.iconPadding = UIEdgeInsetsMake(0, 0, 0, 0);
stateLayer.iconHaloWidth = 1.0f;

// Primitive types and structs still needs to be wrapped one way or
// another in containers when using functions:
MGLStyleAttributeFunction *paddingFunction = [[MGLStyleAttributeFunction alloc] init];
paddingFunction.stops = @{@6.0: NSStringFromUIEdgeInsets(UIEdgeInsetsMake(0, 0, 0, 0))};
stateLayer.iconPaddingFunction = paddingFunction;

What about undefined?
A function will always be an object so setting it to nil could set the underlying property to undefined. It would do the trick but in a lazy way. Another way to tackle the problem could be adding an unset method to the layer: [MGLStyleLayer unsetProperty:theProperty]

[1ec5]

I’ve never been a huge fan of having properties that can be set or unset depending on whether some sibling property is set. Solution 3 is a cleaner solution than what we have now, but I’m not convinced that it’s better than solution 2 on balance.

What is the type of MGLStyleAttributeFunction.stops in solution 3? If it’s NSDictionary<NSString *, id>, then we’re back to weak typing. If NSDictionary<NSString *, NSNumber *>, then we’ve basically arrived at solution 2 (lightweight generics), but with twice the properties. (Granted, those properties are convenient when working with constant values.)

Additionally, solution 3 won’t scale well beyond zoom functions. For #5948, we’ll already need to add -iconPaddingForStyleClass: and -setIconPadding:forStyleClass:; now we’ll need to multiply that out, with -iconPaddingFunctionForStyleClass: and -setIconPaddingFunction:forStyleClass:.

A function will always be an object so setting it to nil could set the underlying property to undefined. It would do the trick but in a lazy way. Another way to tackle the problem could be adding an unset method to the layer: [MGLStyleLayer unsetProperty:theProperty]

An explicit “unset” method for a one-to-one relationship would be un-Cocoa and make it more difficult to be KVO compliant in the future. Unsetting an object-typed property should always entail setting the property to nil.

[frederoni]

I’ve never been a huge fan of having properties that can be set or unset depending on whether
some sibling property is set.

They're actually independent in that sense, setting a function will override the constant and vice versa as excepted.

What is the type of MGLStyleAttributeFunction.stops in solution 3?
If it’s NSDictionary<NSString *, id>, then we’re back to weak typing.

Function stops would still be weakly typed, unfortunately. No improvements there.
Perhaps we can solve that by splitting up functions into MGLStyleZoomFunction or even into different types for padding, offset etc.

I agree about the “unset” method, it's not something I’d prefer.

[1ec5]

They're actually independent in that sense, setting a function will override the constant and vice versa as excepted.

Yes, that’s what we’d have to do, but it still isn’t as straightforward as having one property for each attribute. It will be possible to make this approach KVO compliant, but it means more work on the part of the developer: if the icon padding is undefined, you need to check two different properties to determine that.

Function stops would still be weakly typed, unfortunately. No improvements there.
Perhaps we can solve that by splitting up functions into MGLStyleZoomFunction or even into different types for padding, offset etc.

That’s exactly what the lightweight generics in solution 2 are meant to address. One class can handle every type, be it an NSNumber, NSString, or NSValue.

[1ec5]

Another problem with the current architecture is that it crashes hard if you try to use the wrong type: the MGLStyleAttributeValue_Private category is very broad, encompassing conversion methods for all the value types that could be held in an attribute value. All these methods are optional, but we don’t check that (for example) the attribute value responds to -mbgl_stringWithPropertyValueString: before calling it. So if the developer happens to pass an NSString into a numeric property, they’ll hit an uncaught exception that NSString doesn’t respond to -mbgl_stringWithPropertyValueNumber:, with no other information to go on.

[1ec5]

@boundsj pointed out that solution 2 would break KVO compliance (radar://problem/28053584) if we rely on typedefs.

[1ec5]

Working on solution 2 on the 1ec5-style-types-generics-5970 branch. Stuck on some linker issues at the moment.

[1ec5]

Took a bit of a detour cleaning up the containing classes too: #6588.

[1ec5]

Solution 2 – #5970 (comment) – has been implemented in #6601. In the course of landing #6601, we realized how verbose this approach would become in Swift but especially in Objective-C.

Before:

// MGLLineStyleLayer.swift
open var lineGapWidth: MGLStyleValue!

// MGLMapViewDelegate.mapViewDidFinishLoadingMap(_:)
let layer = mapView.style().layer(identifier: "motorway") as? MGLLineStyleLayer
layer?.lineGapWidth = 5 as MGLStyleAttributeValue
layer?.lineJoin = MGLLineJoin.bevel
layer?.lineColor = UIColor.blue
let lineColor = layer?.lineColor as? UIColor

let fn = MGLStyleAttributeFunction()
fn.stops = [1: 5, 18: 3]
layer?.lineWidth = fn

// MGLLineStyleLayer.h
@property (nonatomic, null_resettable) id <MGLStyleValue> lineGapWidth;

// -[MGLMapViewDelegate mapViewDidFinishLoadingMap:]
MGLLineStyleLayer *layer = (MGLLineStyleLayer *)[mapView.style layerWithIdentifier:@"motorway"];
layer.lineGapWidth = @5;
layer.lineJoin = @(MGLLineJoinBevel);
layer.lineColor = UIColor.blueColor;
UIColor *lineColor = [layer.lineColor isKindOfClass:[UIColor class]] ? layer.lineColor : nil;

MGLStyleAttributeFunction *fn = [[MGLStyleAttributeFunction alloc] init];
fn.stops = @{@1: @5, @18: @3};
layer.lineWidth = fn;

After:

// MGLLineStyleLayer.swift
open var lineGapWidth: MGLStyleValue<NSNumber>!

// MGLMapViewDelegate.mapViewDidFinishLoadingMap(_:)
let layer = mapView.style().layer(identifier: "motorway") as? MGLLineStyleLayer
layer?.lineGapWidth = MGLStyleValue(rawValue: 5)
layer?.lineJoin = MGLStyleValue(rawValue: MGLLineJoin.bevel as NSValue)
layer?.lineColor = MGLStyleValue(rawValue: .blue)
let lineColor = (lineLayer.lineGapWidth as? MGLStyleConstantValue<NSNumber>)?.rawValue

layer?.lineWidth = MGLStyleValue(stops: [
    1: MGLStyleValue(rawValue: 5),
    18: MGLStyleValue(rawValue: 3),
])

// MGLLineStyleLayer.h
@property (nonatomic, null_resettable) MGLStyleValue<NSNumber *> *lineGapWidth;

// -[MGLMapViewDelegate mapViewDidFinishLoadingMap:]
MGLLineStyleLayer *layer = (MGLLineStyleLayer *)[mapView.style layerWithIdentifier:@"motorway"];
layer.lineGapWidth = [MGLStyleValue<NSNumber *> valueWithRawValue:@5];
layer.lineJoin = [MGLStyleValue<NSValue *> valueWithRawValue:@(MGLLineJoinBevel)];
layer.lineColor = [MGLStyleValue<UIColor> valueWithRawValue:UIColor.blueColor];
UIColor *lineColor = [layer.lineColor isKindOfClass:[MGLStyleConstantValue class]] ? [(MGLStyleConstantValue<NSNumber *> *)layer.lineColor rawValue] : nil;

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

Some additional approaches have been proposed, all of which can be layered atop the work in that PR. The following comments detail those proposals, which we’ll revisit following the first beta.

[1ec5]

Reopening in case folks want to continue discussing alternative approaches before the release.

[nitrag]

My vote is for simplicity. So option 4? If there is something unrecognized skip it and throw a warning.

The current implementation is "OK" but I can't figure out LineJoin for the life of me:
lineStyle?.lineJoin = MGLStyleConstantValue(rawValue: MGLLineJoin.round)
nor
.lineJoin = MGLStyleConstantValue(rawValue: MGLLineJoin.round as NSValue) >>> cannot cast value type MGLLineJoin to type NSValue in coercion

Tried every combo I can think of...

[1ec5]

The current implementation is "OK" but I can't figure out LineJoin for the life of me:
lineStyle?.lineJoin = MGLStyleConstantValue(rawValue: MGLLineJoin.round)
nor
.lineJoin = MGLStyleConstantValue(rawValue: MGLLineJoin.round as NSValue) >>> cannot cast value type MGLLineJoin to type NSValue in coercion

Tried every combo I can think of...

Believe it or not, this is what it takes to put an enumeration value into an NSValue in Swift 3:

var bevel = MGLLineJoin.bevel
// @encode(MGLLineJoin) in Objective-C produces "Q"
lineLayer.lineJoin = MGLStyleValue(rawValue: NSValue(&bevel, withObjCType: "Q"))

The only proposal above that skirts the issue is #5970 (comment), because it allows the developer to work with primitives instead of objects. But that proposal has other drawbacks, such as needing to consult multiple methods to “get” a value.

What we can do instead is to provide NSValue initializers and getters, as we do for enumerations like CLLocationCoordinate2D and structs like MGLOfflinePackProgress. That’ll let you say:

lineLayer.lineJoin = MGLStyleValue(rawValue: NSValue(mglLineJoin: .round))

I’m marking this issue as a release blocker specifically for the NSValue initializers and getters.

[danpat]

Just chiming in that I hit this trying to set lineLayer.symbolPlacement which supposedly needs a value from the MGLSymbolPlacement enum. @1ec5 got me around my immediate problem, but I agree that this is a blocker, the API usability is terrible for these types of properties.

[boundsj]

this is a blocker, the API usability is terrible for these types of properties

This is being addressed as part of #7061 (75617cf)

[boundsj]

Closing again since #7061 should take care of the issues noted above in #5970 (comment) and #5970 (comment)

[boundsj]

Moving out of the 3.4.0 milestone and reopening as a placeholder for future work (i.e. #5970 (comment))

cc @1ec5 @frederoni @ericrwolfe

[1ec5]

#5970 (comment):

We could replace MGLStyleValue with NSExpression. NSExpression already comes with loosely typed but convenient constructors for constants and conditionals (handy for mapbox/mapbox-gl-style-spec#402), plus a format string parser that understands unary and binary operators (handy for mapbox/mapbox-gl-style-spec#47). A novel but obscure custom function syntax would be needed to implement zoom and property functions, but we could provide some helpers for that syntax.

We’re working towards a generalized expression syntax in the style specification in mapbox/mapbox-gl-js#4715. I’ve added lots of notes in mapbox/mapbox-gl-js#4715 (review) about how we’d bridge expressions to NSExpression.

On the plus side, it doesn’t sound like style functions (aka ramps) would be part of that proposal. Unfortunately, it sounds like we’ll end up needing to support style functions and expressions simultaneously for backwards compatibility with existing v8 styles. For the runtime styling API, that would mean MGLStyleValue would gain another initializer, MGLStyleValue(expression:), and another subclass, MGLStyleExpression. Ultimately, though, it looks like we have a path to removing style functions entirely in the next major version in favor of expressions, which would hopefully make style values easier to grok.

Another casualty with the upcoming expression work is compile-time type checking: NSExpression is usually created from a format string, and its evaluated type isn’t known until runtime. But given the awkwardness expressed in #6601 (comment), coupled with the huge gains in expressiveness that come with NSExpression, I think the scales tip in favor of NSExpression despite being loosely typed.

/cc @anandthakker

[anandthakker]

On the plus side, it doesn’t sound like style functions (aka ramps) would be part of that proposal.

Note that the proposal now does include stops for zoom (or zoom-and-property) functions.

[1ec5]

It seems likely that we’ll move away from compile-time type checking in favor of runtime type checking, contrary to the original intent of this issue. Expressions are extremely cumbersome to implement using structured data types. This is the same consideration that led us to adopt NSPredicate (with its format strings) for filtering.

Closing this issue to focus on #8074, which tracks the work to reimplement MGLStyleValue atop NSExpression (or replace MGLStyleValue with NSExpression).