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

Replace bundled styles with mapbox: style URLs #2746

Merged
merged 3 commits into from
Oct 30, 2015
Merged

Conversation

ljbade
Copy link
Contributor

@ljbade ljbade commented Oct 23, 2015

Fixes #2239

@ljbade ljbade added iOS Mapbox Maps SDK for iOS build macOS Mapbox Maps SDK for macOS Android Mapbox Maps SDK for Android Linux tests ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Oct 23, 2015
@1ec5
Copy link
Contributor

1ec5 commented Oct 23, 2015

We also need to deprecate or remove the bundledStyleURLs property from MGLMapView, since that property always returns an empty array. We can deprecate it with a compile-time message instructing developers to look at some webpage for a list of always-available styles, but we need to have that webpage ready.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 23, 2015

@1ec5 can we make it return a list of mapbox:// URLs?

@1ec5
Copy link
Contributor

1ec5 commented Oct 23, 2015

That’s not a bad idea. We’d just base it off default_styles.cpp, which would enable us to nix the parallel list we’ve compiled in MBXViewController. We’d still need to rename the method, though: the styles aren’t “bundled”, just universally available, regardless of access token.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 23, 2015

@1ec5 I was thinking more from source compatibility perspective but next release is breaking anyway right?

@1ec5
Copy link
Contributor

1ec5 commented Oct 23, 2015

You’re right, we should mark -bundledStyleURLs as deprecated with a message to migrate to whatever we call it. -universalStyleURLs?

@ljbade
Copy link
Contributor Author

ljbade commented Oct 24, 2015

-mapboxStyleURLs?

@1ec5
Copy link
Contributor

1ec5 commented Oct 29, 2015

I’ve completed the iOS side of this change. There’s a new class MGLStyle that – for now – does nothing but vend style URLs in a manner similar to UIColor. It would make for a great entry point an iOS runtime styling API as part of #837. Unlike Style.java on the Android side, MGLStyle gets its values directly from a refactored default_styles.hpp and uses static assertions to stay in sync. So that takes care of #1462. Objective-C++, o happy day!

The upshot for other platforms is that there are now individual constants for each “default style” (that is, a style that’s available regardless of access token). There’s still a constant C array of them in case you need to iterate over them.

@jfirebaugh, does my C++ look reasonable? @incanus, can you forgive my macro shenanigans?

namespace default_styles {

struct DefaultStyle {
const char *url;
Copy link
Contributor

Choose a reason for hiding this comment

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

Our style for C++ is to space * and & with the rest of the type, so const char* url.

Copy link
Contributor

Choose a reason for hiding this comment

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

Old habits die hard: e13535eda83116ced526c6785e9596e25bdfaac2.

@jfirebaugh
Copy link
Contributor

LGTM other than the one nit.

@1ec5 1ec5 changed the title [core] Remove bundled styles. Replace bundled styles with mapbox: style URLs Oct 30, 2015
@ljbade
Copy link
Contributor Author

ljbade commented Oct 30, 2015

Excellent, when your happy @1ec5 I think we can squash, get Travis green, final set of eyes, then merge.

@1ec5
Copy link
Contributor

1ec5 commented Oct 30, 2015

Running into #2022.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 30, 2015

@1ec5 is that the right ticket number?

@1ec5
Copy link
Contributor

1ec5 commented Oct 30, 2015

Yes, that was precisely what I and Travis were both hitting, but now it’s green again. Go figure.

@1ec5
Copy link
Contributor

1ec5 commented Oct 30, 2015

I’ve come back around to the idea of deprecating and eventually removing -bundledStyleURLs from the iOS SDK (rather than just renaming it). I don’t see a real use case for host applications to iterate over these style URLs without any names that would be suitable for display in a UI. With the introduction of -[MGLStyle streetsStyleURL] et al., developers no longer need to rely on -bundledStyleURLs to discover available styles.

Leith Bade and others added 2 commits October 30, 2015 13:09
Also renamed styles to more common names.
Moved mbgl::util::default_styles to a more appropriate location, where iOS platform code can also find it. Moved -[MGLMapView bundledStyleURLs] (which is now deprecated) and the style switcher in iosapp to default_styles.

Added a collection of convenience methods for getting style URLs. It makes little sense to layer an enum atop this, as MapKit does, because MGLMapView styles aren’t limited to this set. A good analogy is UIColor. This also makes for a good entry point for future runtime styling APIs.

Introduced independent constants for each default style, because it’s more common to need access to a particular style than to iterate over them. This fact is apparent in the MGLStyle class, which now uses macros and assertions to ensure that it’s kept up-to-date with changes in default_styles.

/ref #1462
@1ec5 1ec5 merged commit 557b8af into master Oct 30, 2015
@1ec5 1ec5 removed the in progress label Oct 30, 2015
@1ec5 1ec5 deleted the 2239-no-bundled-styles branch October 30, 2015 20:37
@1ec5 1ec5 removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Oct 30, 2015
1ec5 added a commit that referenced this pull request Nov 20, 2015
As of #2746, we no longer bundle any styles with the SDK, so the asset: URL scheme is unused. Instead, point asset: to the application root for developer convenience and consistency with the Android and default asset roots. Also fixed an issue that prevented relative URLs from being treated as asset: URLs.

Fixes #1208, fixes #3050.
1ec5 added a commit that referenced this pull request Nov 20, 2015
As of #2746, we no longer bundle any styles with the SDK, so the asset: URL scheme is unused. Instead, point asset: to the application root for developer convenience and consistency with the Android and default asset roots. Also fixed an issue that prevented relative URLs from being treated as asset: URLs.

Fixes #1208, fixes #3050.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android build iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants