-
Notifications
You must be signed in to change notification settings - Fork 314
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
Navigation Native, Part 2 #1618
Conversation
10189f1
to
7dd816c
Compare
Move getDirections out of extension to allow overriding it CLLocation<->MBFixLocation Bring in nav-native Add native route controller Support for alternate routes
766e1bb
to
72149e7
Compare
72149e7
to
4c40994
Compare
…`endNavigation(feedback:)` Navigation Service method.
@objc(MBNavigationService) | ||
public protocol NavigationService: CLLocationManagerDelegate, RouterDataSource, EventsManagerDataSource { | ||
/** | ||
The services' location manager. This will be the object responsible for notifying the service of GPS updates. |
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.
This is fine, but you can also say “the services object’s”, so it doesn’t sound like the manager has multiple owners.
MapboxCoreNavigation.podspec
Outdated
@@ -40,6 +40,7 @@ Pod::Spec.new do |s| | |||
s.requires_arc = true | |||
s.module_name = "MapboxCoreNavigation" | |||
|
|||
s.dependency "MapboxNavigationNative" |
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.
Pin to a specific version or use the tadpole operator. Don’t float on the latest version, because someone who installs this version of the SDK might face dependency hell down the line.
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.
yes, especially as it is under such active development
@@ -174,4 +175,14 @@ extension CLLocation { | |||
} | |||
return true | |||
} | |||
|
|||
func asMBFixLocation() -> MBFixLocation { |
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.
Write this as an initializer on an extension of MBFixLocation.
func asMBFixLocation() -> MBFixLocation { | ||
return MBFixLocation(location: coordinate, | ||
time: timestamp, | ||
speed: NSNumber(value: speed), |
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.
Nit: you can cast as NSNumber instead of using this initializer.
|
||
A setting of `.always` will simulate route progress at all times. | ||
A setting of `.onPoorGPS` will enable simulation when we do not recieve a location update after the `poorGPSPatience` threshold has elapsed. | ||
A setting of `.never` will never enable the location simulator, regardless of circumstances. |
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.
These settings are already documented on SimulationOption, right?
|
||
/** | ||
The simulation mode of the service. | ||
*/ | ||
var simulationMode: SimulationOption { get } |
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.
Why is the type SimulationOption
but the property simulationMode
?
- parameter route: The route to follow. | ||
- parameter directions: The Directions object that created `route`. | ||
- parameter locationSource: An optional override for the default `NaviationLocationManager`. | ||
- parameter eventsManagerType: A type argument used for overriding the events manager. |
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.
What is a “type argument”? I think this is descriptive enough:
An optional events manager to use while tracking the route.
@@ -11,6 +11,10 @@ public protocol RouterDataSource { | |||
@objc public protocol Router: class, CLLocationManagerDelegate { | |||
@objc unowned var dataSource: RouterDataSource { get } | |||
@objc var delegate: RouterDelegate? { get set } | |||
|
|||
@objc(initWithRoute:directions:locationManager:) | |||
init(along route: Route, directions: Directions, dataSource source: RouterDataSource) |
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.
Given that this initializer is exposed to Objective-C, does it need to be public as well?
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 Router
protocol is public so this initializer is public as well despite the missing access modifier.
@@ -5,4 +5,5 @@ target 'PodInstall' do | |||
# The branch of MapboxNavigation and MapboxNavigation will be replaced by Bitrise to use the current branch. | |||
pod 'MapboxNavigation', :path => '../../../' | |||
pod 'MapboxCoreNavigation', :path => '../../../' | |||
pod 'MapboxDirections.swift', :git => '[email protected]:mapbox/MapboxDirections.swift.git', :branch => 'return-response' |
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.
Remember to remove this explicit dependency once we’re back on an official release.
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.
In addition to the following feedback, I get the feeling we should say something in the changelog, even if it’s just waxing nostalgic about all the problems we think have gone away by migrating to the MapboxNavigationNative framework.
Speaking of that framework, we should list any new transitive dependencies here in the Carthage installation instructions:
mapbox-navigation-ios/README.md
Line 57 in 96c28d5
1. Follow the rest of [Carthage’s iOS integration instructions](https://github.com/Carthage/Carthage#if-youre-building-for-ios-tvos-or-watchos). Your application target’s Embedded Frameworks should include MapboxNavigation.framework and MapboxCoreNavigation.framework. |
and that might be worth a mention in the changelog too.
"benefits from a new off-route detection heuristic provided by Mapbox's cross-platform native navigation library" ? /cc @mcwhittemore |
nav-native is providing route following, location filtering, guidance triggering and location projection support. I'd call this logic move out in the changelog definitely. |
Let’s only call out the portions we’re using on iOS — as we move more functionality over we can call it out then |
Cartfile
Outdated
@@ -1,5 +1,5 @@ | |||
binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 4.3 | |||
binary "https://s3.amazonaws.com/mapbox/mapbox-navigation-native/ios/builds/Mapbox-iOS-Navigation-Native.json" | |||
binary "https://www.mapbox.com/ios-sdk/MapboxNavigationNative.json" |
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.
Pin to a specific version of this dependency.
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.
Looks good, other than the one nitpick about dependency versioning, which can always be loosened up later if need be.
Cartfile
Outdated
@@ -1,5 +1,5 @@ | |||
binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 4.3 | |||
binary "https://www.mapbox.com/ios-sdk/MapboxNavigationNative.json" | |||
binary "https://www.mapbox.com/ios-sdk/MapboxNavigationNative.json" ~> 3.1.4 |
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.
My understanding is that this library will respect semver, so it’s OK to float on the minor version as well as the patch version, just as we do in the map SDK.
/cc @kevinkreiser
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 podspec specifies ~> 3.0
, so let’s be consistent with that.
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.
Get this over with. 😛
cc @JThramer
This branch is targeting
epic/nav-native.jerrad/146-navigation-service