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

v10 default styles #6301

Merged
merged 4 commits into from
May 12, 2017
Merged

v10 default styles #6301

merged 4 commits into from
May 12, 2017

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Sep 9, 2016

Upgraded from v9 default styles to v10 wherever the developer expects to get the latest and greatest, as well as in a couple tests where it may be beneficial to ensure that we can handle a two-digit version number in the style URL.

Added the Traffic Day v2 and Traffic Night v2 styles, which are now documented as part of the styles API in Mapbox API Documentation.

This PR also partially reverses the decision in #4702 to require iOS and macOS developers to specify a style version number when calling one of MGLStyle’s style URL factory methods. The versioned methods are still there, but the unversioned methods are no longer deprecated. This change makes the iOS and macOS SDKs consistent with the Android and Qt SDKs, which never followed suit and kept their unversioned constants.

Hold until the v10 styles are live.

/cc @tobrun @friedbunny @tmpsantos @amyleew

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl Qt Mapbox Maps SDK for Qt, aka Qt Location Mapbox GL labels Sep 9, 2016
@1ec5 1ec5 added this to the ios-v3.4.0 milestone Sep 9, 2016
@1ec5 1ec5 self-assigned this Sep 9, 2016
@mention-bot
Copy link

@1ec5, thanks for your PR! By analyzing this pull request, we identified @bleege, @tmpsantos and @jfirebaugh to be potential reviewers.

@1ec5 1ec5 added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Sep 9, 2016
@tmpsantos
Copy link
Contributor

@1ec5 thanks!

Just food for thought, not exactly related to this PR, but if a style change also upgrades the tile source, we need to think about a way of not rendering tiles on the offline database useless.

@1ec5
Copy link
Contributor Author

1ec5 commented Sep 13, 2016

Can you elaborate? Is the concern that the user would have to redownload a region's tiles if the v10 Streets style uses a newer version of the Streets source (say, v8) than the v9 Streets style? Styles are somewhat tightly coupled to the sources they reference, so I'm not sure how we'd address that issue without adding custom tile migration logic for individual sources.

@tmpsantos
Copy link
Contributor

Styles are somewhat tightly coupled to the sources they reference

Makes sense. Yeah, in this case, if you wanna keep the offline database, better not upgrade the style. Thanks!

@amyleew
Copy link
Contributor

amyleew commented Oct 3, 2016

@1ec5 - v9 updates and limited v10 styles are now ready to roll in. Please note changelog for specifics.

@zugaldia
Copy link
Member

zugaldia commented Oct 3, 2016

@1ec5 Thanks for updating the Android versions as well, is there any tail work required on Android once this PR gets merged?

@1ec5
Copy link
Contributor Author

1ec5 commented Oct 6, 2016

v10 has been released for Streets, Outdoors, and Satellite Streets. Apparently the styles are versioned separately, so Light, Dark, and Satellite are still stuck on v9. This PR will need to be reworked to reflect that fact:

  • Remove mbgl::util::default_styles::currentVersion and figure out what to do about code that uses it
  • Tweak MGLStyleDefaultVersion’s documentation to say it’s the version of the default style, not “the default version of the suite of default styles”
  • Hard-code version numbers in tests, making it much more likely that versions will get out of sync in the future

@1ec5 1ec5 force-pushed the 1ec5-default-styles-v10 branch from 9f29c99 to 2df753a Compare October 17, 2016 11:14
@boundsj boundsj modified the milestones: ios-3.4.1, ios-v3.4.0 Oct 30, 2016
@1ec5 1ec5 modified the milestones: ios-future, ios-v3.4.1 Jan 24, 2017
@1ec5
Copy link
Contributor Author

1ec5 commented Jan 24, 2017

Not critical for v3.4.1, and no Mapbox-wide timetable for releasing the v10 styles. Moving off the v3.4.1 milestone.

@1ec5 1ec5 force-pushed the 1ec5-default-styles-v10 branch from 2df753a to f7df5f7 Compare March 4, 2017 22:33
@1ec5 1ec5 modified the milestones: ios-v3.6.0, ios-future Mar 14, 2017
@1ec5
Copy link
Contributor Author

1ec5 commented Apr 26, 2017

Can we log that to the console for Debug builds only? I think disabling runtime styling for certain URLs could also lead to very weird behavior and increases the complexity.

Yes, a log message would certainly be doable. If our goal is to alert about programmer error, though, maybe it should be present in release builds as well, but once per session, like the warning about using floats in predicates.

As far as disabling runtime styling for unversioned URLs, I think this is something that MGLMapView would be able to handle: -[MGLMapView setStyleURL:] would detect the x-darwin-versioned parameter and potentially strip it, and -[MGLMapView style] would remain nil even after -[MGLMapViewDelegate mapView:styleDidFinishLoading:] is called. But you’re right that a log message would be friendlier. Disabling the runtime styling API would be rather draconian, only remotely desirable if we find that everyone is using unversioned style URLs with the runtime styling API.

@1ec5 1ec5 requested a review from langsmith May 1, 2017 19:16
@1ec5 1ec5 force-pushed the 1ec5-default-styles-v10 branch from 98890a1 to abde819 Compare May 1, 2017 22:19
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 👍

@1ec5 1ec5 force-pushed the 1ec5-default-styles-v10 branch from abde819 to 6213fb2 Compare May 8, 2017 22:29
@1ec5 1ec5 added GLFW and removed Qt Mapbox Maps SDK for Qt, aka Qt Location Mapbox GL ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels May 9, 2017
kkaefer
kkaefer previously requested changes May 9, 2017
@@ -26,7 +28,12 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
### Other changes

* Xcode 8.0 or higher is now recommended for using this SDK. ([#8775](https://github.com/mapbox/mapbox-gl-native/pull/8775))
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge conflict

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I missed that one – good catch! Fixed in 1d0e203.

1ec5 added 4 commits May 9, 2017 15:57
Upgraded from v9 default styles to v10 wherever the developer expects to get the latest and greatest, as well as in a couple tests where it may be beneficial to ensure that we can handle a two-digit version number in the style URL.
MGLStyleDefaultVersion is just for Streets now. Deleted style version documentation tests because not all styles are on the same version.
Undeprecated the unversioned style URL factory methods in MGLStyle for consistency with the Android and Qt SDKs. Added warnings about using them with the runtime styling API.

Refactored mbgl::util::default_styles to track different versions for different styles.
The Styles API section of the Mapbox API Documentation site now lists Traffic Day v2 and Traffic Night v2, so this change adds those styles to all the places where styles are listed.

Also switched iosapp and macosapp to unversioned style factory methods since MGLStyleDefaultVersion is no longer applicable for all styles.
@1ec5 1ec5 force-pushed the 1ec5-default-styles-v10 branch from 1b2f5f0 to 4387c75 Compare May 9, 2017 22:57
@kkaefer kkaefer dismissed their stale review May 11, 2017 07:41

fixed

@1ec5 1ec5 requested review from kkaefer and removed request for kkaefer May 12, 2017 01:43
@1ec5
Copy link
Contributor Author

1ec5 commented May 12, 2017

We could append a query parameter like ?x-darwin-versioned=false to the returned URL; MGLMapView could track the usage of such URLs and disable or booby-trap the runtime styling API.

Tracking as possible tail work in #8971.

@1ec5 1ec5 merged commit 4d6f545 into master May 12, 2017
@1ec5 1ec5 deleted the 1ec5-default-styles-v10 branch May 12, 2017 01:57
@1ec5 1ec5 mentioned this pull request May 12, 2017
@1ec5
Copy link
Contributor Author

1ec5 commented May 12, 2017

Cherry-picking into the release-ios-v3.6.0-android-v5.1.0 branch in #8972.

@1ec5 1ec5 mentioned this pull request May 12, 2017
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants