Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Install aftermarket expression functions #11472

Merged
merged 23 commits into from
Mar 29, 2018

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Mar 16, 2018

Install Mapbox-specific functions as first-class NSExpression functions, prefixed to avoid collisions with any additional functions that may be built into NSExpression in the future. Various functions need to be renamed because the aftermarket variant no longer takes an explicit operand.

The result of this PR is that nontrivial expression usage will be a lot more legible and easier to write by hand without frequently consulting a reference. For example, concatenating strings goes from:

[NSExpression expressionWithFormat:@"FUNCTION('Old', 'stringByAppendingString:', ' ', 'MacDonald')"]

to:

[NSExpression expressionWithFormat:@"mgl_join({'Old', ' ', 'MacDonald'})"]

Fixes #11015.

/cc @fabian-guerra @jmkiley @akitchen

@1ec5 1ec5 added feature iOS Mapbox Maps SDK for iOS refactor macOS Mapbox Maps SDK for macOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold runtime styling labels Mar 16, 2018
@1ec5 1ec5 added this to the ios-v4.0.0 milestone Mar 16, 2018
@1ec5 1ec5 self-assigned this Mar 16, 2018
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 19, 2018

f66b1ee converts length JSON expressions to length: NSExpressions for an expression that gets the length of a constant string, but not for an expression that gets the length of an expression that evaluates to a string.

Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that the following PRs are complementary to this PR?
#11464
#11450

To me seems like it and it would be good if we merge them.

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 23, 2018

Yes, they are complementary, but I don’t think they have to be coupled. We’re still going to need conventional functions, just in case the mechanism in this PR breaks for any reason. I’ve added items to the to-do list above for those PRs, in case I get to them first.

@1ec5 1ec5 force-pushed the 1ec5-freedom-of-nsexpression-11015 branch from e82177e to 15bf3d0 Compare March 26, 2018 21:37
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 27, 2018

Thinking about this for a moment:

Safeguard against changes to NSExpression that render this approach ineffective

+[NSExpression mgl_expressionWithJSONObject:] uses the aftermarket functions to create the NSExpression object. This way the developer isn’t forced to work with the clunkier custom function syntax when using the style layer properties’ getters, and we can easily verify that expressions round-trip. But if the aftermarket installation mechanism fails for any reason, this is the code that’ll fail. I suppose we could catch the NSException that arises and use the custom function syntax in that case? I’m not keen on using a @try-@catch block, but exception handling is the roughest edge of NSExpression.

@1ec5 1ec5 force-pushed the 1ec5-freedom-of-nsexpression-11015 branch from c989d11 to 8df1e98 Compare March 27, 2018 06:00
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 27, 2018

0203d2d819f3c335c84e7db932953b30ad5f773f implements a catch-all MGL_FUNCTION() function, replacing assertions that used to fail for unrecognized expression operators. Now it’s possible to represent the entire GL expression language within an NSExpression. (Not sure about the naming of this function; it could just as well be called NS_EXPRESSION().)


// Install functions that resemble control structures, taking arbitrary
// numbers of arguments. Vararg aftermarket functions need to be declared
// with an explicit and implicit first argument.
INSTALL_CONTROL_STRUCTURE(MGL_LET);
INSTALL_CONTROL_STRUCTURE(MGL_MATCH);
INSTALL_CONTROL_STRUCTURE(MGL_SWITCH);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested MGL_MATCH and MGL_SWITCH as counterparts to mgl_match: and mgl_case:, respectively, but I think I got mixed up. MGL_SWITCH implies the same semantics as MGL_MATCH. I do think “case” would be problematic, as it makes it impossible to refer to the cases inside the expression. “Ternary” is also problematic because there can be multiple cases. Do you think MGL_CONDITION or MGL_IF could work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think MGL_IF will do the work. It's more descriptive to what the function actually does.

@@ -792,19 +820,19 @@ - (id)mgl_jsonExpressionObject {
}

return expressionObject;
} else if ([function isEqualToString:@"mgl_match:"]) {
NSMutableArray *expressionObject = [NSMutableArray arrayWithObjects:@"match", self.operand.mgl_jsonExpressionObject, nil];
} else if ([function isEqualToString:@"MGL_MATCH"]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes support for the conventional custom function mgl_match:. In this PR, I’ve left in support for those functions, just in case aftermarket expressions break with a new iOS SDK version. (I’ve equivocated about whether to remove public documentation about the conventional functions, but the implementations remain.) See the methods at the bottom of this file for an example of how to support both versions of the method simultaneously.

@@ -352,11 +357,11 @@ In style specification | Method, function, or predicate type | Format string syn
`case` | `+[NSExpression expressionForConditional:trueExpression:falseExpression:]` | `TERNARY(condition, trueExpression, falseExpression)`
`coalesce` | |
`match` | |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These rows need to be updated.

`mgl_interpolate:withCurveType:parameters:stops:` | `mgl_interpolate:withCurveType:parameters:stops:(x, 'linear', nil, %@)` | Produces continuous, smooth results by interpolating between pairs of input and output values (“stops”). Compared to the `mgl_interpolateWithCurveType:parameters:stops:` custom function, the input expression (that function’s target) is instead passed in as the first argument to this function.
`mgl_step:from:stops:` | `mgl_step:from:stops:(x, 11, %@)` |Produces discrete, stepped results by evaluating a piecewise-constant function defined by pairs of input and output values ("stops"). Compared to the `mgl_stepWithMinimum:stops:` custom function, the input expression (that function’s target) is instead passed in as the first argument to this function.
`mgl_join:` | `mgl_join({'Old', 'MacDonald'})` | Returns the result of concatenating together all the elements of an array in order. Compared to the `stringByAppendingString:` custom function, this function takes only one argument, which is an aggregate expression containing the strings to concatenate.
`MGL_LET` | `MGL_LET('age', uppercase('old'), 'name', uppercase('MacDonald'), mgl_join({$age, $name}))` | Any number of variable names interspersed with their assigned `NSExpression` values, followed by an `NSExpression` that may contain references to those variables. Compared to the `mgl_expressionWithContext:` custom function, this function takes the variable names and values inline before the expression that contains references to those variables.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rows need to be added for MGL_MATCH, MGL_SWITCH, and mgl_coalesce:.

@@ -111,7 +111,7 @@ let stops: [Float: UIColor] = [
10: .white,
]

layer.circleColor = NSExpression(format: "FUNCTION(mag, 'mgl_stepWithMinimum:stops:', %@, %@)",
layer.circleColor = NSExpression(format: "mgl_step:from:stops:(mag, %@, %@)",
UIColor.green, stops)
```

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right below here is an example that could potentially use MGL_MATCH. On the other hand, I do find the use of a dictionary with valueForKeyPath: to be somewhat elegant, so perhaps all it needs is a call to mgl_coalesce: to avoid the redundant valueForKeyPath: call.

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 28, 2018

But if the aftermarket installation mechanism fails for any reason, this is the code that’ll fail.

Per chat with @fabian-guerra, we’re going to consider this defensive approach for a followup release, possibly on hold until we know there’s going to be a problem with an upcoming version of iOS or macOS. The existing code is already resilient to minor changes like the private class being renamed, just not to more substantial, public-facing changes to NSExpression.

@fabian-guerra fabian-guerra force-pushed the 1ec5-freedom-of-nsexpression-11015 branch from 8874184 to a51ae90 Compare March 29, 2018 01:40
Renamed mgl_hasProperty:properties: to mgl_does:have: for better readability and consistency with the conventional mgl_has: function. Documented both forms of mgl_has:.
This is the preferred syntax for simple conditionals on iOS 9 and above, because you can inline the predicate instead of wrapping it in a constant value expression, which means you can write a conditional in a single format string.
@@ -343,7 +343,7 @@ In style specification | Method, function, or predicate type | Format string syn
`properties` | |`$mgl_featureProperties`
`at` | `objectFrom:withIndex:` | `array[n]`
`get` | `+[NSExpression expressionForKeyPath:]` | Key path
`has` | `mgl_hasProperty:properties:` |
`has` | `mgl_does:have:` | `mgl_does:have:(self, 'key')`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed and reordered this function. Instead of calling mgl_hasProperty:properties:('key', self), you call mgl_does:have:(self, 'key'). I think this reads better: “Does self have ‘key’?” It’s also consistent with the order when you call the conventional function: FUNCTION(self, 'mgl_has:', 'key').

@@ -354,7 +354,7 @@ In style specification | Method, function, or predicate type | Format string syn
`>=` | `NSGreaterThanOrEqualToPredicateOperatorType` | `key >= value`
`all` | `NSAndPredicateType` | `p0 AND … AND pn`
`any` | `NSOrPredicateType` | `p0 OR … OR pn`
`case` | `MGL_IF` | `MGL_IF(1 = 2, YES, 2 = 2, YES, NO)`
`case` | `+[NSExpression expressionForConditional:trueExpression:falseExpression:]` or `MGL_IF` | `TERNARY(1 = 2, YES, NO)` or `MGL_IF(1 = 2, YES, 2 = 2, YES, NO)`
Copy link
Contributor Author

@1ec5 1ec5 Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I restored full support for +[NSExpression expressionForConditional:trueExpression:falseExpression:] where available, along with the tests for it. On iOS 9.0 and above, we convert case to TERNARY() when it has only one conditional. This is the preferred syntax, because you can inline the predicate instead of wrapping it in a constant value expression, which means you can write a conditional in a single format string.

/cc @friedbunny

@1ec5 1ec5 removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Mar 29, 2018
@1ec5 1ec5 merged commit 8c5eb6c into release-boba Mar 29, 2018
@1ec5 1ec5 deleted the 1ec5-freedom-of-nsexpression-11015 branch March 29, 2018 17:25
@friedbunny
Copy link
Contributor

The code ticks in the table at platform/darwin/docs/guides/Predicates and Expressions.md#L195 aren’t converted automatically to <code>, so we’ll need to do some follow-up.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor runtime styling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants