[ios, macos] Add format expression convenience methods support. #14094

fabian-guerra · 2019-03-12T23:20:03Z

I'm using MGLAttributedExpression as a custom structure that holds an expression and it's respective attributes. This structure is treated as a constant value but the underlaying expression property can be of any kind.

1ec5

Suggested changelog entry:

Added the mgl_attributed: and MGL_ATTRIBUTED expression operators, which concatenate MGLAttributedExpression objects for specifying rich text in the MGLSymbolStyleLayer.text property.

Also, the “Predicates and Expressions” guide should be updated to reflect the addition of these new operators.

1ec5 · 2019-03-13T00:42:29Z

platform/darwin/src/MGLAttributedExpression.h

+FOUNDATION_EXTERN MGL_EXPORT MGLAttributedExpressionKey const MGLFontSizeAttribute;
+
+MGL_EXPORT
+@interface MGLAttributedExpression : NSObject


This class seems to represent an attribute range (or attribute run) rather than an expression in its own right. Naming it MGLAttributedExpression could cause some confusion, since it looks like a subclass of NSExpression that can be set directly on NSExpression-typed properties. Acknowledging the difference with a name like MGLAttributedRun (akin to Core Text’s CTRun) would head off that confusion.

Ideally, we’d create a system akin to NSAttributedString, where you build up an NSAttributedExpression by applying attributes to ranges of the string and enumerate those ranges. But that’s potentially a large API surface area to implement, and it’s difficult to think in terms of ranges when the underlying content is of undefined length.

I'm using the same mental model between NSString and NSAttributedString where you can't assign the later to the first one and vice versa. I think MGLAttributedExpression keeps the concept of an object that contains an expression with attributes.

This is a little different though: NSString doesn’t hold an array of NSAttributedStrings.

I agree with @1ec5. How about something like MGLTextFormatAttributes? The MGLText prefix already exists as part of MGLSymbolStyleLayer...

@tobrun is there an equivalent to this in the Android SDK?

yes, we have:

textField( format( formatEntry("this is", formatFontScale(0.75)), formatEntry( concat(literal("\n"), get(TITLE_FEATURE_PROPERTY)), formatFontScale(1.25), formatTextFont(new String[] {"DIN Offc Pro Bold", "Arial Unicode MS Regular"}) ) ) ),

I think the concept that resembles the intent is MGLAttributedExpression, in regards to MGLTextFormatAttributes the underlaying object is not text per se (although it may contain a string constant).

1ec5 · 2019-03-13T00:52:15Z

platform/darwin/src/NSExpression+MGLAdditions.mm

@@ -1115,6 +1139,11 @@ - (id)mgl_jsonExpressionObject {
                }
                [NSException raise:NSInvalidArgumentException
                            format:@"Casting expression to %@ not yet implemented.", type];
+            } else if ([function isEqualToString:@"mgl_attributed:"] ||
+                       [function isEqualToString:@"MGL_FORMAT:"] ||
+                       [function isEqualToString:@"MGL_FORMAT"]) {


MGL_FORMAT looks like something related to printf-style string formatting or number formatting (#13632) rather than rich text. Let’s stick to “attributed” terminology if possible.

Apart from that, MGL_FORMAT and MGL_FORMAT: need to be installed above in +installFunctions.

I forgot to remove this lines I just wanted to use mgl_attributed: do you think it would be useful have MGL_ATTRIBUTED as well?

No, no need.

1ec5 · 2019-03-13T00:53:08Z

platform/darwin/test/MGLExpressionTests.mm

+        MGLAttributedExpression *attribute3 = [MGLAttributedExpression initWithExpression:[NSExpression expressionForConstantValue:@"bar"]
+                                                                                fontNames:nil
+                                                                                 fontSize:@(0.8)];
+        MGLAttributedExpression *attribute4 = [MGLAttributedExpression initWithExpression:[NSExpression expressionForConstantValue:@"\r"]


Would it be possible to support the newline \n as well?

It does support '\n' I will add a test for this.

1ec5 · 2019-03-13T00:55:13Z

platform/darwin/test/MGLExpressionTests.mm

+        MGLAttributedExpression *attribute4 = [MGLAttributedExpression initWithExpression:[NSExpression expressionForConstantValue:@"\r"]
+                                                                                fontNames:@[]
+                                                                                 fontSize:nil];
+        NSExpression *expression = [NSExpression expressionWithFormat:@"mgl_attributed(%@)", @[attribute1, attribute4, attribute2, attribute3]];


#12739 also envisioned a conversion from NSAttributedString to the format expression operator as a convenience. (A conversion to MGLAttributedExpression might make more sense, on second thought.) Note that NSAttributedString isn’t only intended for rich text; it’s meant to be a general-purpose container for strings that have attributes associated with portions of the text.

That doesn’t necessarily happen to happen as part of this PR, but I’d hold off on closing #12739 until there is something like that. The use case for an NSAttributedString conversion is a constant expression with sophisticated formatting that would be tedious to express as an NSExpression format string.

1ec5 · 2019-03-13T00:58:20Z

platform/darwin/src/MGLAttributedExpression.m

+    NSMutableDictionary *attrs = [NSMutableDictionary dictionary];
+
+    if (fontNames && fontNames.count > 0) {
+        [attrs setObject:fontNames forKey:MGLFontNamesAttribute];


Nit: use subscript notation:

Suggested change

[attrs setObject:fontNames forKey:MGLFontNamesAttribute];

attrs[MGLFontNamesAttribute] = fontNames;

(×2)

1ec5 · 2019-03-13T01:12:12Z

platform/darwin/src/MGLAttributedExpression.h

+FOUNDATION_EXTERN MGL_EXPORT MGLAttributedExpressionKey const MGLFontSizeAttribute;
+
+MGL_EXPORT
+@interface MGLAttributedExpression : NSObject


Since this class is public, remember to give it a documentation comment.

1ec5 · 2019-03-13T01:17:50Z

platform/darwin/src/NSExpression+MGLAdditions.mm

+        formatParameter = self.operand;
+    }
+
+    if ([formatParameter respondsToSelector:@selector(constantValue)] && [formatParameter.constantValue isKindOfClass:[NSArray class]]) {


A developer might be tempted to create a constant-value NSExpression from a bare MGLAttributedExpression, which would be an error here. Maybe we could convert a standalone, constant MGLAttributedExpression into format expression too?

If we do that, then it starts to seem like we could combine this operator with mgl_join: so that joining MGLAttributedExpressions together creates a format expression instead of a concat expression.

1ec5

Not thrilled about the naming of MGLAttributedExpression (see that thread), but that’s all I’m concerned about here. The implementation looks good.

1ec5 · 2019-03-14T00:58:41Z

platform/darwin/docs/guides/Predicates and Expressions.md

+<dd><code>mgl_attributed({x, y, z})</code></dd>
+</dl>
+
+Returns formatted text containing annotations for use in mixed-format `text-field`  entries.


Concatenates and returns the array of MGLAttributedExpression objects, for use with the MGLSymbolStyleLayer.text property.

1ec5 · 2019-03-14T00:59:03Z

platform/darwin/docs/guides/Predicates and Expressions.md

+Returns formatted text containing annotations for use in mixed-format `text-field`  entries.
+
+This function corresponds to the
+[`mgl_attributed`](https://www.mapbox.com/mapbox-gl-js/style-spec/#expressions-types-format)


It’s called format in the style specification.

1ec5 · 2019-03-14T01:00:13Z

platform/darwin/src/MGLAttributedExpression.h

+FOUNDATION_EXTERN MGL_EXPORT MGLAttributedExpressionKey const MGLFontSizeAttribute;
+
+/**
+ An `NSExpression` that has associated text formatting attibutes (such as font size or


This isn’t an NSExpression. It’s something that can be inserted into a specific operator inside an NSExpression.

1ec5 · 2019-03-14T01:00:56Z

platform/darwin/docs/guides/For Style Authors.md.ejs

@@ -403,6 +403,7 @@ In style specification | Method, function, or predicate type | Format string syn
 `zoom`                 | `NSExpression.zoomLevelVariableExpression` | `$zoomLevel`
 `heatmap-density`      | `NSExpression.heatmapDensityVariableExpression` | `$heatmapDensity`
 `line-progress`        | `NSExpression.lineProgressVariableExpression` | `$lineProgress`
+`format`               | `mgl_attributed:` | `mgl_attributed({x, y, z})`


Should we extend NSExpression with an initializer corresponding to this operator?

I think makes sense.

julianrex · 2019-03-19T12:17:31Z

Thank you @fabian-guerra for getting this in 👍

fabian-guerra added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold needs changelog Indicates PR needs a changelog entry prior to merging. expressions labels Mar 12, 2019

fabian-guerra added this to the release-l milestone Mar 12, 2019

fabian-guerra self-assigned this Mar 12, 2019

fabian-guerra requested review from 1ec5, julianrex and a team March 12, 2019 23:20

1ec5 reviewed Mar 13, 2019

View reviewed changes

fabian-guerra removed needs changelog Indicates PR needs a changelog entry prior to merging. ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Mar 14, 2019

1ec5 approved these changes Mar 14, 2019

View reviewed changes

fabian-guerra added 9 commits March 14, 2019 12:59

[ios, macos] Add MGLAttributedExpression class.

a801237

[ios, macos] Make MGLAttributedExpression a public class.

f693a72

[ios, macos] Add format expression support.

a76a9c7

[ios, macos] Add MGLAttributedExpression class documentation.

5056c60

[ios, macos] Improve format expression tests.

89eca91

[ios, macos] Update predicates and expression documentation.

97268ca

[ios, macos] Update public headers.

9b3986d

[ios, macos] Update changelogs.

c9a363f

[ios, macos] Update format expression documentation.

e6faefe

fabian-guerra force-pushed the fabian-format-expression-12739 branch from 46adc67 to e6faefe Compare March 14, 2019 22:00

[ios, macos] Fix format expressions tests.

ee9b8ef

fabian-guerra merged commit 7939ff2 into master Mar 15, 2019

fabian-guerra deleted the fabian-format-expression-12739 branch March 15, 2019 23:10

fabian-guerra mentioned this pull request Mar 15, 2019

Add NSExpression operator for rich text (format expressions) #12739

Open

fabian-guerra mentioned this pull request Apr 11, 2019

[ios, macos] Fix MGLSymbolStyleLayer.text localization issue. #14405

Merged

1ec5 mentioned this pull request Apr 15, 2021

Error decoding symbol layer with text-field set to formatted expression mapbox/mapbox-maps-ios#268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios, macos] Add format expression convenience methods support. #14094

[ios, macos] Add format expression convenience methods support. #14094

fabian-guerra commented Mar 12, 2019

1ec5 left a comment

1ec5 Mar 13, 2019

fabian-guerra Mar 13, 2019

1ec5 Mar 14, 2019

julianrex Mar 14, 2019 •

edited

Loading

tobrun Mar 14, 2019

fabian-guerra Mar 14, 2019

1ec5 Mar 13, 2019

fabian-guerra Mar 13, 2019

1ec5 Mar 14, 2019

1ec5 Mar 13, 2019

fabian-guerra Mar 13, 2019

1ec5 Mar 13, 2019

1ec5 Mar 13, 2019

1ec5 Mar 13, 2019

1ec5 Mar 13, 2019

1ec5 Mar 13, 2019

1ec5 left a comment

1ec5 Mar 14, 2019

1ec5 Mar 14, 2019

1ec5 Mar 14, 2019

1ec5 Mar 14, 2019

fabian-guerra Mar 14, 2019

julianrex commented Mar 19, 2019

	[attrs setObject:fontNames forKey:MGLFontNamesAttribute];
	attrs[MGLFontNamesAttribute] = fontNames;

[ios, macos] Add format expression convenience methods support. #14094

[ios, macos] Add format expression convenience methods support. #14094

Conversation

fabian-guerra commented Mar 12, 2019

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianrex Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianrex commented Mar 19, 2019

julianrex Mar 14, 2019 •

edited

Loading