-
-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate some hardcoded mapbox styles, and replace those that remain with maplibre styles. (+docs!) #45
Conversation
@@ -50,8 +50,8 @@ class CarPlayMapViewController: UIViewController, MLNMapViewDelegate { | |||
super.viewDidLoad() | |||
|
|||
self.styleManager = StyleManager(self) | |||
self.styleManager.styles = [DayStyle(), NightStyle()] | |||
|
|||
self.styleManager.styles = [DayStyle(demoStyle: ()), NightStyle(demoStyle: ())] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The demo styles are now being used in some places that were previously defaulting to mapbox styles. This is probably wrong, but (I think) no worse than things being completely broken before.
Probably these should somehow be surfaced or inferred as well. I somewhat arbitrarily decided this was out of scope for this PR, but I could follow up in this PR or another depending on how people are feeling about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't mind your approach here 👍
Whoops, converting to a draft while I address a missed TODO. |
71b505a
to
8ff89c1
Compare
|
||
extension Style: NSCopying { | ||
public func copy(with zone: NSZone? = nil) -> Any { | ||
let copy = Self(mapStyleURL: self.mapStyleURL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Living in Swift land for a while, I forgot about NSCopying!
Re-marking this as ready for review. I brushed up against some things that could use more attention (CarPlay and weird NavigationViewController tests). I didn't fix everything, but I think everything changed is either neutral or an improvement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. Could you add a Changelog entry explaining what you changed?
This reverts commit 7d907bf.
ebd8937
to
44f15c2
Compare
@@ -12,6 +12,13 @@ | |||
- Updated MapLibre Native dependency to ios-v6.0.0 (https://github.com/maplibre/maplibre-native/releases/tag/ios-v6.0.0). Implementers need to change the prefix MGL to MLN for all MapLibre Native classes that are referenced. | |||
- Only snap location to route if the location is within the `RouteControllerUserLocationSnappingDistance` | |||
- Add support for Swift Package Manager while dropping Carthage and Cocoapods. | |||
- Removed implicit default dependencies on MapBox tileservers by requiring explicit styles URLs in more places. | |||
- Merged in <https://github.com/maplibre/maplibre-navigation-ios/pull/45>. | |||
- BREAKING: Removed `MLNStyle` extensions referencing non-functioning MapBox styles, e.g. `MLNStyle.navigationGuidanceDayStyleURL`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically breaking from an API standpoint, though it seems like it would not have been working for anyone before.
Alternatively, we could keep the old methods around, deprecate them, and have them all return DayStyle(demoStyle: ())
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking is fine for me 👍
Ok, Changelog entry added. Curious if people care about mitigating the breaking change, but otherwise I think it's good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this change 👍
Fixes #42
Description
Following up on #42 (comment)
In the mapbox days, it arguably made sense to have hardcoded defaults that referenced mapbox servers. I think we can agree that in the maplibre world it no longer makes sense to reference these mapbox services.
Perhaps more controversially: Post maplibre, I think it makes less sense to have any default service provider at all. Instead, we should surface API's that require the implementor to pass in an explicit backend tile provider. I've introduced some new API's to that end, and deprecated some of the existing API's which utilize a default tile source.
Screenshot of previous API along with the newly proposed deprecation messages
For example:
We still do make available some "demo" styles that use https://demotiles.maplibre.org, but I've intentionally made it explicit that it's a demo -
DayStyle(demoStyle: ())
. See below for the alternatives rationale.Alternatives considered
Swap out mapbox tile server for malibre tile server
I could have maintained the current APIs where specifying styles is optional, and simply swapped the mapbox tiles out for maplibre demo tiles. However, my understanding of the maplibre project is, unlike mapbox, maplibre doesn't (currently anyway) intend to host production map services, and so maplibre libraries should direct people towards finding their own service providers, even at some cost to the "everything just works" experience. I think providing an easy way to use the demo services for POC, while trying to nudge users to proper external tile hosts is the right balance. But I'm open to learning more about this if people feel differently.
Another alternative that would be a smaller code change would be to keep the existing init, but make
styles
inNavigationViewController.init(styles:)
non-optional. This would still be a semver breaking change, but an arguably simpler change for users to adapt to. My understanding is that, in practice, the user would pass in 0, 1, or 2 styles. Accepting an array of 3 or more styles is not useful as far as I know. Plus I think the old API where "the second style should be the night style" is a bit obscure. Since we were breaking the API anyway, I decided to codify the expectations.Info for reviewers
I'm very new to the maplibre native ecosystem, so consider this PR a proposal open for comment and changes. I look forward to your feedback.