[ios, macos] rename style spec properties #7128
Conversation
frederoni
added
iOS
|
@frederoni, thanks for your PR! By analyzing the history of the files in this pull request, we identified @1ec5, @fabian-guerra and @incanus to be potential reviewers. |
frederoni
added
the
⚠️ DO NOT MERGE
frederoni
force-pushed
the
fred-rename-prop-6098
branch
from
b9e720a
to
aae1e77
Compare
frederoni
removed
the
⚠️ DO NOT MERGE
| "icon-image": "icon-image-name" | ||
| }, | ||
| "paint_raster": { | ||
| "raster-brightness-min": "raster-brightness-minimum", |
There was a problem hiding this comment.
minimumRasterBrightness reads better. We might as well make that change as long as we're renaming this property.
| _.forOwn(overrides, function (value, key) { | ||
| const keyPath = stack + '.'; | ||
| if (_.isObject(value)) { | ||
| renameProperties(obj, overrides[key], stack.length ? keyPath + key : key); |
There was a problem hiding this comment.
Does this function need to be recursive? Seems like we could assume a two-level-deep override object and use subscript notation instead of _.set and _.unset.
frederoni
force-pushed
the
fred-rename-prop-6098
branch
3 times, most recently
from
bd082bd
to
951c716
Compare
| _.forOwn(cocoaConventions, function (properties, kind) { | ||
| _.forOwn(properties, function (newName, oldName) { | ||
| spec[kind][newName] = spec[kind][oldName]; | ||
| spec[kind][newName]["_original"] = oldName; |
There was a problem hiding this comment.
Nit: normally, in JavaScript, you’d say .original instead of ["_original"].
There was a problem hiding this comment.
Ack, sorry, I meant ._original. You were right about trying to avoid a theoretical conflict with a property named original.
| @@ -212,6 +221,10 @@ global.propertyDefault = function (property, layerType) { | |||
| return 'an `MGLStyleValue` object containing ' + describeValue(property.default, property, layerType); | |||
| }; | |||
|
|
|||
| global.originalPropertyName = function (property) { | |||
| return property._original ? property._original : property.name; | |||
There was a problem hiding this comment.
Nit: this can be simplified as property._original || property.name.
| @@ -21,9 +21,9 @@ namespace mbgl { | |||
| <% if (layoutProperties.length) { -%> | |||
| <% for (const property of layoutProperties) { -%> | |||
There was a problem hiding this comment.
We’ve been relying on an implementation detail of v8 whereby an object’s properties are enumerated in the order that they’re added to the object. Unfortunately, all the renamed properties appear last in the generated source code, which is a little jarring.
To fix this issue, we’d need to sort layoutProperties’ keys before enumerating the object:
let sortedLayoutProperties = _.pick(layoutProperties, _.keysIn(layoutProperties).sort());There are probably more efficient ways of doing this, but this way involves less typing. 😉
There was a problem hiding this comment.
After the style spec has been processed, the key is removed and inserted into the object as name so I sorted with _.sortBy(layoutProperties, ['name']. Unfortunately, it wasn't very well sorted before so the diff increased quite a bit. Separate commits to make it easier to review.
There was a problem hiding this comment.
I mistakenly thought the properties were alphabetically sorted to begin with, but you’re right that the properties are far from sorted in the style specification’s JSON objects. The official documentation is generated using v8, just like our generated runtime styling files, so for example icon-rotate comes before icon-padding. @lucaswoj doesn’t know of any reason we need to keep the properties in that order, so alphabetizing the properties makes sense because it’ll make it easier to find them in our jazzy documentation.
frederoni
force-pushed
the
fred-rename-prop-6098
branch
from
a2b13a5
to
0c518ad
Compare
Fixes #6098
Started with the most obvious properties to avoid confusion and sloppiness.
icon-image=>icon-image-nameraster-brightness-min=>raster-brightness-minimumraster-brightness-max=>raster-brightness-maximumline-dasharray is another one that doesn't feel particularly idiomatic but I'm ok with it and the remaining circlePitchScale & rasterHueRotate that @1ec5 pointed out #6098 (comment).
@1ec5 @boundsj 👀