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

Rename some layer and source properties #6098

Closed
6 tasks done
1ec5 opened this issue Aug 19, 2016 · 15 comments
Closed
6 tasks done

Rename some layer and source properties #6098

1ec5 opened this issue Aug 19, 2016 · 15 comments
Assignees
Labels
iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Aug 19, 2016

The runtime styling API’s property names come directly from the style specification, which follows more of a CSS naming tradition that differs in many ways from the Cocoa naming convention iOS and macOS developers are accustomed to. Combined with #5970, this means developers really have no clue what to set many properties to unless they read the SDK’s source code or inspect similar properties’ values in the debugger.

Some examples of poorly named properties, starting with the most severe issues and ending with the most benign ones:

  • iconImage – The developer would expect to set this property to a CGImage, NSImage, or UIImage, but definitely not a string. It should be called iconImageName. Even if we fix Add category convenience methods to NSNumber for style values (was: Runtime styling properties are too loosely typed) #5970, developers might expect to pass in a path or URL string to an image, which would fail.
  • textFont – The developer would expect to set this property to an NSFont or UIFont, or perhaps an NSFontDescriptor or UIFontDescriptor, but definitely not an array of strings. It should be called textFontNames.
  • rasterBrightnessMin and rasterBrightnessMax – Minimum and maximum are always written out, so abbreviating them looks sloppy in Objective-C and Swift.
  • circlePitchScale – The developer would expect to set this property to a CGFloat. Maybe this would get better once we fix Add category convenience methods to NSNumber for style values (was: Runtime styling properties are too loosely typed) #5970, but another fix would be to rename this property circlePitchScaleStategy or somesuch.
  • lineDasharray – This is called a “dash pattern” in many Cocoa APIs. Swift in particular has moved away from putting types inside symbol names.
  • rasterHueRotate – This is a verb, whereas properties should be nouns. This can be a problem in Objective-C, where you can use the same syntax to access a property getter or call a parameter-less method.

On the other hand, as important as it is to ensure consistency with the rest of the SDK and with Cocoa APIs, there’s a strong case to be made for consistency with the style specification and GL JS when it comes to the runtime styling API. We should at least make a few conservative changes (like the first two above) and leave the rest of the API intact.

To rename any property, we’ll need an override mechanism like the one envisioned for documentation in #5954.

/cc @frederoni @jfirebaugh @incanus

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling labels Aug 19, 2016
@1ec5 1ec5 added this to the ios-v3.4.0 milestone Aug 19, 2016
@jfirebaugh
Copy link
Contributor

Seems to me that this should be an all-or-nothing sort of thing -- either we keep a predictable mapping directly from the style specification, or we conform as closely as possible to Cocoa naming conventions in every case. Mixing and matching conventions sounds confusing.

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 22, 2016

I’m mainly concerned about iconImage, rasterBrightnessMin, and rasterBrightnessMax. The rest falls into a gray area where there aren’t clear precedents in Apple frameworks but we could do better. If we were to rename just a few properties, it would just be a happy coincidence that the rest of the properties happen to match the style specification, right?

Whatever the case, each property’s documentation should refer to the underlying style specification property (or even link to it) for discoverability.

@incanus
Copy link
Contributor

incanus commented Aug 22, 2016

I share @1ec5's views here about severity and reasoning (particularly with iconImage and min/max, which are indeed confusing/error-prone and sloppy-looking, respectively). I don't think mixing and matching is as detrimental, providing that we do indeed indicate in the docs the underlying name and, ideally, spec definition link. It'd be more confusing if we weren't starting the property names the same way (icon..., raster...), which also helps with autocomplete discoverability ("What's that icon... property... ah yes, iconImageName!)

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 5, 2016

The most critical properties were renamed in #7128 on the v3.4.0 release branch. Here are the next batch of important properties to rename:

  • lineDasharray – Even if we decide not to rename this property to lineDashPattern, we should at least make it conform to camelCase as lineDashArray.
  • iconSize – The developer almost certainly expects to put a CGSize/NSSize into this property, but it’s actually a number indicating a scale, not an absolute size. (The first time I ever used this property, I blew up an icon to take up the entire screen on accident.) We should rename it iconScale.

@boundsj
Copy link
Contributor

boundsj commented Dec 7, 2016

#7310 took care of #6098 (comment). Can this be closed @frederoni @1ec5?

@boundsj boundsj removed the release blocker Blocks the next final release label Dec 7, 2016
@boundsj
Copy link
Contributor

boundsj commented Dec 7, 2016

@1ec5 @frederoni I removed the release blocker since it seems like all the critical changes are complete. I'll keep this open since it seems like there may be a few properties we still want to consider renaming.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 8, 2016

@frederoni and I aren't too keen on renaming circlePitchScale to circlePitchScalingStrategy. Alternatives include circlePitchScaling and circleScalesWithViewingDistance, for consistency with MGLAnnotationView.scalesWithViewingDistance`.

We agree on rasterHueRotation though.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 9, 2016

#7344 renamed rasterHueRotate to rasterHueRotation.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 12, 2016

#7391 updates documentation to reflect the above changes.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 16, 2016

#7457 renames a bevy of properties to use the appropriate parts of speech for getters and setters.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 5, 2017

#7603 takes care of the last batch of renames for v3.4.0. Some (but not all) of these renamings should be applied upstream as well:

See also mapbox/mapbox-gl-style-spec#218.

@jfirebaugh
Copy link
Contributor

As mentioned in mapbox/mapbox-gl-style-spec#201, what about *-translate vs *-translation and text-justify vs text-justification?

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 5, 2017

We already rename "justify" to "justification" as of #7457. We could rename "translate" to "translation" here, but Apple frameworks are less consistent about this word, and to be honest both are considerably less idiomatic than "offset".

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 6, 2017

@jfirebaugh, on second thought, I renamed translate to translation in #7603 as well. If we end up renaming those properties in mapbox/mapbox-gl-style-spec#201, there would be no reason for us to keep the verb form in these SDKs. I still think the distinction between translation and offset properties is completely opaque from a developer standpoint, but we can address that through better documentation without diverging further from the style specification.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 8, 2017

Fixed in #7603 on the release-ios-v3.4.0 branch.

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

No branches or pull requests

5 participants