Add iOS bindings for cluster properties

[jmkiley]

Fixes #15399

Currently, MGLShapeSourceOptionsClusterProperties takes an NSDictionary of NSString keys and arrays containing two NSExpression objects as values.

Because the property takes a specific format, I am going to look a little more at making a method to return this dictionary.

• Add MGLShapeSourceOptionsClusterProperties.
• Convert the NSDictionary into an std::unordered_map so that it can be passed to core.
• Add documentation.
• Add tests.

Thanks to @fabian-guerra for helping!

[fabian-guerra]

@zmiao could you please add a test in https://github.com/mapbox/mapbox-gl-native/blob/master/platform/darwin/test/MGLExpressionTests.mm
for the new variable featureAccumulated and a sum case

e.g.

{
        NSExpression *expression = [NSExpression expressionForVariable:@"featureAccumulated"];
        XCTAssertEqualObjects(expression.mgl_jsonExpressionObject, @[@"accumulated"]);
        XCTAssertEqualObjects([NSExpression expressionWithFormat:@"$featureAccumulated"].mgl_jsonExpressionObject, @[@"accumulated"]);
        XCTAssertEqualObjects([NSExpression expressionWithMGLJSONObject:@[@"accumulated"]], expression);
        NSMutableDictionary *context = [@{@"featureAccumulated": @5} mutableCopy];
        XCTAssertEqualObjects([expression expressionValueWithObject:nil context:context], @5);
    }

expression = [NSExpression expressionWithFormat:@"sum({ $featureAccumulated, sumVal })"];
        NSArray *expt = expression.mgl_jsonExpressionObject;
        jsonExpression = @[@"+", @[@"accumulated"], @[@"get", @"sumVal"]];
        XCTAssertEqualObjects(expression.mgl_jsonExpressionObject, jsonExpression);
        XCTAssertEqualObjects([NSExpression expressionWithMGLJSONObject:jsonExpression], expression);

[zmiao]

@fabian-guerra Thank you very much for the hint of

expression = [NSExpression expressionWithFormat:@"sum({ $featureAccumulated, sumVal })"];

I changed the expression creation via using expressionWithFormat, instead of expressionForFunction, then everything works perfectly. No need to adapt the NSExpression any more! I think the root cause is via using expressionWithFormat, there will be only one argument with collections as subArguments. But if we use expressionForFunction, I created two arguments, then it crashed.

Except for that, I added the support for plain string as reduce operator. So right now, the binding is fully working. I tested locally, the cluster aggregation works very well. I added two ways of creating the clusterProperty in the example, please have a look.

@jmkiley As I mentioned above, there is no need to take extra efforts of adapting NSExpressions. As I am poor of knowledge about Objectiv-C, you might have to refine the codes. And please feel free to remove non-necessary codes(MBXViewController.m earthquarkes.geojson) in this pr, and adding changelogs.

cc: @tmpsantos @chloekraw

[fabian-guerra]

This paragraph seems redundant. It's clear if starts with:

The dictionary key is an NSString that will be an attribute key for the clusters. The dictionary value is an NSArray consisting of a reduce operator and a map expression.

[fabian-guerra]

Another question: Why the array also holds a string? shouldn't hold expression values only?

[zmiao]

This is the same behavior ported from GL-JS. For reduceExpression, either a string with a operand key word or a valid expression containing featureAccumulated are acceptable.

[fabian-guerra]

I see. In that case let's drop the term reduce as it is foreign to iOS expressions. The array should hold a sum/max function instead of an NSString or in opposite side a constant NSString expression. I would prefer the first option as this code is parsed back as an NSExpression in https://github.com/mapbox/mapbox-gl-native/pull/15515/files#diff-782f4462e342b786fea8a94199fb6225R115-R116

cc @zmiao @jmkiley

[zmiao]

@fabian-guerra, do you mean what @jmkiley mentioned here: #15515 (comment)

[chloekraw]

In that case let's drop the term reduce as it is foreign to iOS expressions.

Have we done this yet? I appreciate the benefit that changing a name might provide to platform-specific developers, but I'd like to present an alternate perspective: making decisions such as these makes Mapbox harder to use.

If I'm understanding correctly, reduceExpression is not a term of art: it's something that @mourner created and named for clusterProperties when designing it in GL-JS. Is there a different name for this expression that would allow an iOS developer to immediately grok what the function does? If so, let's consider it. If not, then that iOS developer will still have to rely on Mapbox documentation to learn about the expression, and we should reduce this friction as much as possible.

The documentation available isn't limited to a single platform's API reference. Ways for people to learn about Mapbox features include technical guides, examples, blog posts. It's unrealistic to adapt every resource we make to every platform, and I have a strong opinion that the more consistently we name our features across all platforms, the better of an experience our major customers will have building maps with Mapbox and porting features into all of the platforms they need to support.

[zmiao]

platform/ios/app/earthquakes.geojson could be removed.

[jmkiley]

Addressed feedback from @fabian-guerra!

[jmkiley]

Updated this to remove the option to use just an expression function as the first object in the cluster properties array. It may be confusing since it's not an option we offer for any other properties that accept NSExpressions.

[zmiao]

@jmkiley I am thinking about your last comments

Updated this to remove the option to use just an expression function as the first object in the cluster properties array. It may be confusing since it's not an option we offer for any other properties that accept NSExpressions.

Since it would not fully cover the style spec of the usage about reduce expression : https://docs.mapbox.com/mapbox-gl-js/style-spec/#sources-geojson-clusterProperties, or for ios, it is ok to work like this? Because for android, it supports both ways of creating the reduce expression functions.
cc: @chloekraw

[chloekraw]

Since it would not fully cover the style spec of the usage about reduce expression : https://docs.mapbox.com/mapbox-gl-js/style-spec/#sources-geojson-clusterProperties, or for ios, it is ok to work like this? Because for android, it supports both ways of creating the reduce expression functions.

Thank you for raising the question @zmiao. Our customers will absolutely expect that the functionality for clusterProperties is consistent across platforms if the SDK support section is filled out. Though the style specification docs live on GL-JS, developers on all platforms rely on them to understand the Mapbox Style Specification. @jmkiley @fabian-guerra let's voice on these proposed changes.

[fabian-guerra]

Thank you!

[aramsargsyan]

Thank you for this feature! Just a question, is there a possibility to accumulate string attributes from children? If not possible with accumulation/function, would just passing a string argument from one of children of the cluster to the cluster possible in any way?

[rfremont]

Thank you for this feature! Just a question, is there a possibility to accumulate string attributes from children? If not possible with accumulation/function, would just passing a string argument from one of children of the cluster to the cluster possible in any way?

let firstExpressionStrAttr = NSExpression(format: "mgl_join({$featureAccumulated,'_',strAttr})")
let secondExpressionStrAttr = NSExpression(format: "FUNCTION(childrenInt, 'stringValue')")
var clusterPropertiesDictionary = ["strAttr": [firstExpressionStrAttr, secondExpressionStrAttr]]

I have this that is OK to append string with '_' separator. Here the key in children is 'childrenInt'.
I don't know if it's possible to have an array of string with NSExpression.

I have a question also, with this complex string in cluster, how can I test if "strAttr" contains "childrenInt".
I have this implementation without success :(

clusterLayer.iconImageName = NSExpression(format: "TERNARY(FUNCTION(strAttr, 'contains','%@') = YES, 'selected', 'notSelected')", childrenIntSelectedStr)

Any advice would be appreciated ... thanks