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

Clearly document that any runtime styling API risks breaking changes #6616

Closed
1ec5 opened this issue Oct 6, 2016 · 7 comments
Closed

Clearly document that any runtime styling API risks breaking changes #6616

1ec5 opened this issue Oct 6, 2016 · 7 comments
Assignees
Labels
documentation good first issue Good for newcomers Qt Mapbox Maps SDK for Qt, aka Qt Location Mapbox GL runtime styling

Comments

@1ec5
Copy link
Contributor

1ec5 commented Oct 6, 2016

Documentation for the Android, iOS, macOS, and Qt SDKs should take every opportunity to warn the developer that using a particular layer, source, or sprite name risks a breaking change on any SDK release, even a patch release. The iOS and macOS SDKs’ documentation for MGLStyleDefaultVersion is very clear that referencing this style constant comes with risks, but the same risks apply if the developer doesn’t explicitly set the style at all, instead relying on the SDK to use the latest version of Streets. Meanwhile, the Android SDK continues to have unversioned style constants like style_outdoors that make it all too easy to use the latest Outdoors version with the runtime styling API, which is a bad idea.

APIs like -[MGLStyle layerWithIdentifier:], -[MGLStyle sourceWithIdentifier:], and -[MGLStyle insertLayer:belowLayer:] should clearly state that the developer must set the style URL to an explicitly versioned style, whether using +[MGLStyle outdoorsStyleURLWithVersion:], the “Style URL” inspectable, or a manually constructed NSURL. Similarly, the corresponding Android SDK runtime styling methods should warn that the style must be set first and should not be set to one of the style string constants.

We can also take the opportunity to plug Mapbox Studio as a tool for creating custom styles with better stability guarantees as a jumping-off point for runtime styling.

/ref mapbox/mapbox-gl-styles#267 #4759
/cc @mapbox/mobile @lucaswoj @ajashton

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS Android Mapbox Maps SDK for Android documentation Qt Mapbox Maps SDK for Qt, aka Qt Location Mapbox GL release blocker Blocks the next final release runtime styling labels Oct 6, 2016
@1ec5 1ec5 added this to the ios-v3.4.0 milestone Oct 6, 2016
@lucaswoj
Copy link
Contributor

lucaswoj commented Oct 7, 2016

Documentation for the Android, iOS, macOS, and Qt SDKs should take every opportunity to warn the developer that using a particular layer, source, or sprite name risks a breaking change on any SDK release, even a patch release.

That's an unfortunate constraint after we spent so much time developing a semver system for our styles. Is there an oppurtunity to remove, deprecate, or demphasize MGLStyleDefaultVersion?

@1ec5
Copy link
Contributor Author

1ec5 commented Oct 7, 2016

MGLStyleDefaultVersion is orthogonal to the issue here. For various reasons, unlike GL JS, the Android, iOS, and macOS SDKs choose a style by default. Currently that style is Streets v9. In #4759, we took pains to document MGLStyleDefaultVersion as a way for the developer to know the version of the style that the SDK uses by default, for example for testing purposes – hence the admonition against using it for any other purpose. In the HTML documentation, this constant is buried far away from the MGLStyle documentation.

This issue is about extending the same warning against pegging to the current version to runtime styling APIs that the developer is likely to use. Imagine the following scenario: a developer is satisfied with the default Mapbox Streets style but wants to recolor motorways blue. 🇬🇧 So they simply write:

let layer = mapView.style().layer(identifier: "motorway") as? MGLLineStyleLayer
layer?.lineColor = UIColor.blue

This may work for now, but if they upgrade to a new version of the SDK that defaults to a new version of Streets that calls the layer freeway, suddenly their customization has no effect. So the point of this issue is to call attention to the need to explicitly set a (versioned) style URL. The -[MGLMapView layerWithIdentifier:] method above is a prime candidate for this warning.

@jfirebaugh
Copy link
Contributor

Perhaps worth noting that it would be possible to design the API in such a way that this was enforced -- disabling methods such as layerByIdentifier if the style has been set by unversioned constant rather than URL. Though I believe the current API is such that setting by unversioned constant involves composition of URL builder and URL setter, so the style object itself is unaware of the provenance of the URL.

@1ec5
Copy link
Contributor Author

1ec5 commented Oct 7, 2016

Though I believe the current API is such that setting by unversioned constant involves composition of URL builder and URL setter, so the style object itself is unaware of the provenance of the URL.

Correct. This is something I hope to improve on iOS and macOS as part of #6386. However, that isn’t directly applicable to the Android SDK, where the runtime styling methods live directly on MapboxMap.

@boundsj
Copy link
Contributor

boundsj commented Nov 2, 2016

@1ec5 with #6886 proposed for iOS and macOS, how do you feel about removing the release blocker tag and removing this issue from the iOS milestone? It can stay open to track Qt and Android.

cc @ivovandongen @brunoabinader

@boundsj boundsj self-assigned this Nov 2, 2016
@1ec5 1ec5 removed iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS release blocker Blocks the next final release labels Nov 2, 2016
@1ec5 1ec5 removed this from the ios-v3.4.0 milestone Nov 2, 2016
@cammace cammace mentioned this issue Nov 8, 2016
@cammace
Copy link
Contributor

cammace commented Nov 8, 2016

I have added warnings in the Javadoc within the Android code base in #6969. Removing Android tag.

@cammace cammace removed the Android Mapbox Maps SDK for Android label Nov 8, 2016
@brunoabinader
Copy link
Member

We'll address this for Mapbox Qt SDK in #6834. We haven't done a tagged release of it yet, so this can come all together with a proper documentation of our Qt APIs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation good first issue Good for newcomers Qt Mapbox Maps SDK for Qt, aka Qt Location Mapbox GL runtime styling
Projects
None yet
Development

No branches or pull requests

6 participants