Add data-driven-styling support for `text-font` #10850

jfirebaugh · 2018-01-05T23:59:49Z

Before getting to the meat of the changes in the last commit, I needed to pay back some technical debt on our generated style code:

Special case heatmap-color so it doesn't crash make style-code (even though I'm then ignoring
-- manually reverting -- all hillshade and heatmap related changes it generates)
Commit accumulated make style-code updates. Some of these changes undo previous changes to generated files that didn't come with accompanying changes to the source templates (#10810 updated a generated file without changing the source #10846) -- they'll need to be reverted once that bug is fixed. cc @tobrun and @1ec5 for reviews of Android and Darwin changes respectively.
Deal with mapbox/mapbox-gl-js@fa47208.

1ec5 · 2018-01-06T00:03:02Z

platform/darwin/scripts/generate-style-code.js

@@ -404,7 +404,11 @@ global.describeValue = function (value, property, layerType) {
 };

 global.propertyDefault = function (property, layerType) {
-    return 'an `MGLStyleValue` object containing ' + describeValue(property.default, property, layerType);
+    if (property.name === 'heatmap-color') {
+        return 'a rainbow color scale from blue to red';


Nice. In 3c86571 for #10726, I wound up locally deleting the heatmap source type from the style specification object, but this is more future-proof.

1ec5 · 2018-01-06T00:04:46Z

platform/darwin/src/MGLFillExtrusionStyleLayer.h

@@ -8,7 +8,7 @@
 NS_ASSUME_NONNULL_BEGIN

 /**
- Controls the translation reference point.
+ Controls the frame of reference for `fillExtrusionTranslate`.


This is unfortunate, because the iOS/macOS SDKs rename fillExtrusionTranslate to fillExtrusionTranslation etc.

Fixed in a0cb1f2.

1ec5 · 2018-01-06T01:27:16Z

platform/darwin/src/MGLRasterStyleLayer.h

-
- This property corresponds to the `raster-fade-duration-transition` property in the style JSON file format.
-*/
-@property (nonatomic) MGLTransition rasterFadeDurationTransition;


Is the removal of this property intentional? It’s is present in iOS map SDK v3.7.0, so typically we’d deprecate it before removing it outright.

Yes, see mapbox/mapbox-gl-js@fa47208. I'd advocate for outright removing it rather than deprecating: it's very obscure, and this is for a semver major release anyway.

Makes sense. I guess in the future we can also subsume raster-fade-duration under a transition property somehow for consistency.

1ec5 · 2018-01-06T01:31:08Z

platform/darwin/src/MGLSymbolStyleLayer.h

- like {field_name}.  (Token replacement is only supported for literal
- `textField` values--not for property functions.)
+ like `{field_name}`. (`{token}` replacement is only supported for literal
+ `textField` values; not for property functions.)


Note that this implies support for {token} replacement in expressions. (Also, in the event that someone happens to update this documentation in the style specification, the semicolon should be a comma.)

mapbox/mapbox-gl-js#5959 mentions data expressions and fixes the typo.

I think we should just remove references to token replacement entirely. We want people to migrate to expressions (and we're not going to support token replacement within them: #10399).

The published style specification is the canonical documentation for runtime styling for both GL JS (which supports expressions) and Android (which doesn’t yet in v5.3.0). @tobrun, do you think it would be safe to remove the bit about the {token} syntax now, before the next Android release which has expression support?

1ec5 · 2018-01-06T01:33:58Z

platform/darwin/src/MGLSymbolStyleLayer.h

@@ -475,9 +477,13 @@ MGL_EXPORT
 /**
 Offset distance of icon from its anchor.

+ This property is measured in points multiplied by the value of "icon-size"s.


generate-style-code assumes units is a single plural word. The style specification should use backticks instead of double quotation marks around icon-size, and propertyDoc() should account for this case by not tacking an “s” onto the end.

mapbox/mapbox-gl-js#5959 changes the quotation marks to backticks.

1ec5 · 2018-01-06T01:35:39Z

platform/darwin/src/MGLSymbolStyleLayer.h

- `NSValue` object containing a `CGVector` struct set to 0 rightward and 0
- downward. Set this property to `nil` to reset it to the default value.
+ `NSValue` object containing a `CGVector` struct set to 0 points multiplied by
+ the value of "icon-size"s rightward and 0 points multiplied by the value of


This is why sticking a phrase inside units is problematic. I guess we can special-case this property to omit the unit (even though CGVector normally specifies two point values).

👍 I'm going to revert mapbox/mapbox-gl-js@67bce60 and explain this in the prose instead.

OK. For what it’s worth, 5374da2 adds support for resolving property references in backticks and also omits unwieldy units from struct descriptions. No harm in merging mapbox/mapbox-gl-js#5958 though.

1ec5 · 2018-01-08T23:42:46Z

platform/darwin/src/MGLSymbolStyleLayer.h

- {tokens} replaced, referencing the data property to pull from.
+ `{tokens}` replaced, referencing the data property to pull from. (`{token}`
+ replacement is only supported for literal `iconImageName` values; not for
+ property functions.)


mapbox/mapbox-gl-js#5964 adds some links to anchors on the style specification page. The iOS/macOS runtime styling codegen script doesn’t resolve these anchor links to an absolute URL, but we can override the documentation for that platform to talk about something more platform-appropriate. For now, that’d be:

for equivalent functionality in expressions, use concatenate strings and key paths together with the stringByAppendingString: function.

#8074 (comment) proposes a more intuitive join function, but I can update the override as part of #10726 when I get around to implementing that.

…-font`

anandthakker

Expressions & properties parts of this look good to me -- @jfirebaugh is validation the only thing remaining here?

jfirebaugh · 2018-01-10T20:58:21Z

Yeah, I'm on the fence about validation. Unlike gl-js, native doesn't have a built out framework for it, and generally just tries to cope with invalid portions of styles (e.g. by ignoring the property or falling back to a default value).

jfirebaugh requested review from 1ec5, tobrun and anandthakker January 5, 2018 23:59

1ec5 reviewed Jan 6, 2018

View reviewed changes

1ec5 self-assigned this Jan 6, 2018

jfirebaugh mentioned this pull request Jan 6, 2018

Revert 67bce6; explain in prose instead mapbox/mapbox-gl-js#5958

Merged

1ec5 mentioned this pull request Jan 6, 2018

Minor corrections in style spec documentation mapbox/mapbox-gl-js#5959

Closed

1ec5 removed their assignment Jan 6, 2018

jfirebaugh mentioned this pull request Jan 8, 2018

Update {token} replacement docs for expressions mapbox/mapbox-gl-js#5964

Merged

1ec5 reviewed Jan 8, 2018

View reviewed changes

jfirebaugh mentioned this pull request Jan 9, 2018

Commit accumulated make style-code updates #10874

Merged

jfirebaugh force-pushed the dds-text-font branch 2 times, most recently from 8232df9 to 6888af8 Compare January 9, 2018 19:14

jfirebaugh changed the base branch from master to make-style-code January 9, 2018 19:15

jfirebaugh force-pushed the make-style-code branch 2 times, most recently from d5333ae to 5cd6d9d Compare January 9, 2018 21:38

jfirebaugh force-pushed the dds-text-font branch from 6888af8 to 98afbd2 Compare January 9, 2018 21:41

tobrun approved these changes Jan 10, 2018

View reviewed changes

jfirebaugh force-pushed the dds-text-font branch from 98afbd2 to b31f9c2 Compare January 10, 2018 19:16

jfirebaugh changed the base branch from make-style-code to master January 10, 2018 19:16

[core, ios, macos, android] Add data-driven-styling support for `text…

5f61dd8

…-font`

jfirebaugh force-pushed the dds-text-font branch from b31f9c2 to 5f61dd8 Compare January 10, 2018 20:42

anandthakker reviewed Jan 10, 2018

View reviewed changes

[core] Add warning for invalid text-font values

2ddd492

jfirebaugh merged commit 542713f into master Jan 10, 2018

jfirebaugh deleted the dds-text-font branch January 10, 2018 23:31

jfirebaugh mentioned this pull request Jan 10, 2018

Master ticket for remaining data-driven style properties mapbox/mapbox-gl-js#4434

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add data-driven-styling support for `text-font` #10850

Add data-driven-styling support for `text-font` #10850

jfirebaugh commented Jan 5, 2018 •

edited

Loading

1ec5 Jan 6, 2018

1ec5 Jan 6, 2018

1ec5 Jan 6, 2018

1ec5 Jan 6, 2018

jfirebaugh Jan 6, 2018

1ec5 Jan 6, 2018

1ec5 Jan 6, 2018 •

edited

Loading

1ec5 Jan 6, 2018

jfirebaugh Jan 6, 2018

1ec5 Jan 6, 2018

1ec5 Jan 6, 2018

1ec5 Jan 6, 2018

1ec5 Jan 6, 2018

jfirebaugh Jan 6, 2018

1ec5 Jan 6, 2018

1ec5 Jan 8, 2018

anandthakker left a comment

jfirebaugh commented Jan 10, 2018

Add data-driven-styling support for text-font #10850

Add data-driven-styling support for text-font #10850

Conversation

jfirebaugh commented Jan 5, 2018 • 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

1ec5 Jan 6, 2018 • 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

anandthakker left a comment

Choose a reason for hiding this comment

jfirebaugh commented Jan 10, 2018

Add data-driven-styling support for `text-font` #10850

Add data-driven-styling support for `text-font` #10850

jfirebaugh commented Jan 5, 2018 •

edited

Loading

1ec5 Jan 6, 2018 •

edited

Loading