Skip to content
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 Service, Part 1 #1602

Merged
merged 41 commits into from
Sep 26, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
57ed105
WIP: Building out scaffolding for `MapboxNavigationService`
Aug 9, 2018
ef09aac
WIP: It compiles! (Completed building out scaffolding around Navigati…
Aug 13, 2018
23818fc
WIP: It's Aliiiiive! (Hooking NavigationService up to consumer, movin…
Aug 13, 2018
a9611d1
WIP: PR Comments
Aug 13, 2018
8f47670
Update all the tests!
Aug 14, 2018
099ddc4
Cleaning up after myself
Aug 15, 2018
b24c8bf
Merge branch 'master' into jerrad/146-navigation-service
Aug 15, 2018
518a89e
Decoupling EventsManager from RouteController
Aug 16, 2018
8e44849
Merge branch 'master' into jerrad/146-navigation-service
Aug 16, 2018
d169b06
WIP: Temporarily disabling TunnelIntersectionTests for refactor
Aug 21, 2018
3a80e14
Decoupling the TunnelIntersectionManager (now TunnelAuthority) from t…
Aug 21, 2018
cedabcb
Gah, forgot about the OBJ-C interface!
Aug 21, 2018
230351f
Prevent yo-yoing by only passing synthetic location updates onto the …
Aug 21, 2018
8c04240
Fix naming conventions and coding style
frederoni Aug 21, 2018
7f5aa7f
Simplified TunnelAuthority and refactor accompanying tests
frederoni Aug 21, 2018
089a39a
Completely decoupling tunnel detection from route simulation. This si…
Aug 22, 2018
9c5b70d
Fixing issue where countdown timer payload was being executed on back…
Aug 22, 2018
950cd68
Fixing issue where we were using timer.cancel() (which is permanent) …
Aug 22, 2018
d49ebdc
Fixing Tests. Gah.
Aug 22, 2018
3e8edde
PR Comments, and preliminary polish.
Aug 22, 2018
d6f8498
Merge branch 'master' into jerrad/146-navigation-service
Aug 24, 2018
8a8fc79
Adding new `NavigationServiceDelegate` type and plumbing everything t…
Aug 27, 2018
7b8ed23
Merge branch 'master' into jerrad/146-navigation-service
Aug 27, 2018
e454202
Hooking up example
Aug 27, 2018
45795e4
cleanup, random comment fix
Aug 27, 2018
a139f89
Merge branch 'master' into jerrad/146-navigation-service
Aug 29, 2018
4fc2e2a
Fixing tests.
Aug 29, 2018
297f7b4
Consolidating directions reference into a single-point-of-truth.
Aug 29, 2018
ee3cd30
Merge branch 'master' into jerrad/146-navigation-service
Sep 7, 2018
cec16b6
Fixing merge clobber
Sep 7, 2018
b5c0c03
Merge branch 'epic/nav-native' into jerrad/146-navigation-service
Sep 21, 2018
fdcb350
WIP: Trying to fix test failures
Sep 24, 2018
9c7c9f9
WIP: Fixed crash, now working on the actual failures, which is "we di…
Sep 24, 2018
2049a30
Fixed issue where wrong type of location manager was used for core na…
Sep 24, 2018
41265b1
Fixing remaining tests
Sep 26, 2018
89c73cf
Final polish -- fixing issue where speed multiplier changes weren't b…
Sep 26, 2018
2e161df
Fixing test cruft where testing type is no longer extant on stubbed e…
Sep 26, 2018
6bccfd6
Reverting removal of ostensibly redundant XCTest availability macros
Sep 26, 2018
edbb56d
Merge branch 'epic/nav-native' into jerrad/146-navigation-service
Sep 26, 2018
0994b2b
Forgot this one
Sep 26, 2018
e9a7781
Fixing some post-merge testing issues
Sep 26, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions Examples/Objective-C/ViewController.m
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ @interface ViewController () <AVSpeechSynthesizerDelegate>
@property (nonatomic, assign) CLLocationCoordinate2D destination;
@property (nonatomic) MBDirections *directions;
@property (nonatomic) MBRoute *route;
@property (nonatomic) MBRouteController *navigation;
@property (nonatomic) MBNavigationService *navigation;
JThramer marked this conversation as resolved.
Show resolved Hide resolved
@property (nonatomic) NSLengthFormatter *lengthFormatter;
@property (nonatomic) AVSpeechSynthesizer *speechSynth;
@end
Expand All @@ -37,7 +37,7 @@ - (void)viewDidDisappear:(BOOL)animated {
[super viewDidDisappear:animated];

[self suspendNotifications];
[self.navigation suspendLocationUpdates];
[self.navigation stop];
}

- (IBAction)didLongPress:(UILongPressGestureRecognizer *)sender {
Expand Down Expand Up @@ -121,10 +121,15 @@ -(void)prepareForSegue:(UIStoryboardSegue *)segue sender:(id)sender {
if ([segue.identifier isEqualToString:@"StartNavigation"]) {
MBNavigationViewController *controller = (MBNavigationViewController *)[segue destinationViewController];

MBSimulatedLocationManager *locationManager = [[MBSimulatedLocationManager alloc] initWithRoute:self.route];

MBEventsManager *events = [[MBEventsManager alloc] initWithAccessToken:self.route.accessToken];

controller.directions = [MBDirections sharedDirections];
controller.route = self.route;
controller.navigationService = [[MBNavigationService alloc] initWithRoute:self.route directions:controller.directions locationSource:locationManager eventsManager:events];

controller.routeController.locationManager = [[MBSimulatedLocationManager alloc] initWithRoute:self.route];

}
}

Expand Down
13 changes: 7 additions & 6 deletions Examples/Swift/CustomViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class CustomViewController: UIViewController, MGLMapViewDelegate {

var destination: MGLPointAnnotation!
let directions = Directions.shared
var routeController: RouteController!
var navigationService: NavigationService!
var simulateLocation = false

var userRoute: Route?
Expand All @@ -27,7 +27,8 @@ class CustomViewController: UIViewController, MGLMapViewDelegate {
super.viewDidLoad()

let locationManager = simulateLocation ? SimulatedLocationManager(route: userRoute!) : NavigationLocationManager()
routeController = RouteController(along: userRoute!, locationManager: locationManager)
navigationService = MapboxNavigationService(route: userRoute!, locationSource: locationManager)


mapView.delegate = self
mapView.compassView.isHidden = true
Expand All @@ -36,7 +37,7 @@ class CustomViewController: UIViewController, MGLMapViewDelegate {
resumeNotifications()

// Start navigation
routeController.resume()
navigationService.start()

// Center map on user
mapView.recenterMap()
Expand All @@ -55,7 +56,7 @@ class CustomViewController: UIViewController, MGLMapViewDelegate {
func resumeNotifications() {
NotificationCenter.default.addObserver(self, selector: #selector(progressDidChange(_ :)), name: .routeControllerProgressDidChange, object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(rerouted(_:)), name: .routeControllerDidReroute, object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(updateInstructionsBanner(notification:)), name: .routeControllerDidPassVisualInstructionPoint, object: routeController)
NotificationCenter.default.addObserver(self, selector: #selector(updateInstructionsBanner(notification:)), name: .routeControllerDidPassVisualInstructionPoint, object: navigationService.router)
}

func suspendNotifications() {
Expand All @@ -65,7 +66,7 @@ class CustomViewController: UIViewController, MGLMapViewDelegate {
}

func mapView(_ mapView: MGLMapView, didFinishLoading style: MGLStyle) {
self.mapView.showRoutes([routeController.routeProgress.route])
self.mapView.showRoutes([navigationService.route])
}

// Notifications sent on all location updates
Expand Down Expand Up @@ -96,7 +97,7 @@ class CustomViewController: UIViewController, MGLMapViewDelegate {
// Fired when the user is no longer on the route.
// Update the route on the map.
@objc func rerouted(_ notification: NSNotification) {
self.mapView.showRoutes([routeController.routeProgress.route])
self.mapView.showRoutes([navigationService.route])
}

@IBAction func cancelButtonPressed(_ sender: Any) {
Expand Down
15 changes: 9 additions & 6 deletions Examples/Swift/ViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -329,11 +329,14 @@ extension ViewController: VoiceControllerDelegate {
extension ViewController: WaypointConfirmationViewControllerDelegate {
func confirmationControllerDidConfirm(_ confirmationController: WaypointConfirmationViewController) {
confirmationController.dismiss(animated: true, completion: {
guard let navigationViewController = self.presentedViewController as? NavigationViewController else { return }

guard navigationViewController.routeController.routeProgress.route.legs.count > navigationViewController.routeController.routeProgress.legIndex + 1 else { return }
navigationViewController.routeController.routeProgress.legIndex += 1
navigationViewController.routeController.resume()
guard let navigationViewController = self.presentedViewController as? NavigationViewController,
let navService = navigationViewController.navigationService else { return }
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR introduces some significant new concepts that will affect most non-trivial usage of the navigation SDK. So we’ll need an update to the changelog that enumerates the publicly visible changes (or links to a PR description that does so).


let router = navService.router
guard router.route.legs.count > router.routeProgress.legIndex + 1 else { return }

router.routeProgress.legIndex += 1
navService.start()
})
}
}
Expand All @@ -349,7 +352,7 @@ extension ViewController: NavigationViewControllerDelegate {
// This type of screen could show information about a destination, pickup/dropoff confirmation, instructions upon arrival, etc.

//If we're not in a "Multiple Stops" demo, show the normal EORVC
if navigationViewController.routeController.routeProgress.isFinalLeg {
if navigationViewController.navigationService.router.routeProgress.isFinalLeg {
return true
}

Expand Down
8 changes: 8 additions & 0 deletions MapboxCoreNavigation/NavigationLocationManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ import UIKit
open class NavigationLocationManager: CLLocationManager {

var lastKnownLocation: CLLocation?
override open var delegate: CLLocationManagerDelegate? {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need to override the default implementation for delegate with the same implementation as the superclass already provides.

get {
return super.delegate
}
set {
super.delegate = newValue
}
}

override public init() {
super.init()
Expand Down
110 changes: 110 additions & 0 deletions MapboxCoreNavigation/NavigationService.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import Foundation
import CoreLocation
import MapboxDirections

@objc public protocol NavigationServiceDelegate: RouterDelegate, TunnelIntersectionManagerDelegate {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Using protocol composition as a substitute for explicitly plumbing through delegate methods normally has one major flaw: since the umbrella protocol is just a passthrough for other protocols, it can’t give the delegate any additional context in the form of method arguments.

In this case, the delegate has no clue which NavigationService is calling the delegate methods, only the router or tunnel intersection manager. This makes a many-to-one (delegate-to-delegator) relationship impractical. The delegate would have to compare routeController to navigationService.router if it already has a reference to navigationService. Otherwise, it would have to rely on a backpointer (!) from RouteController to NavigationService, which itself could violate a many-to-one (navigation service–to–router) relationship.

In other words, suppose I registered to be the delegate for (at least one) navigation service. When I get delegate methods back, I should know which of the navigation services is messaging me. I shouldn’t have to infer it based on the identity of some navigation service’s router.

The classical approach would be to plumb delegate methods through the implementer of this protocol, adding a navigationService argument. If that is undesirable, then make NavigationServiceDelegate a type alias, remove NavigationService.delegate, and make application code talk directly to NavigationService’s router and tunnel intersection manager. I took the latter approach in #1601 in order to more loosely couple the map and navigation SDKs, but it does come at the cost of exposing implementation details.

This may not be a practical concern. For all I know, there may be only one relevant navigation service and the application may never need to talk to both simultaneously. But there’s nothing in the public interface that allows the developer to rely on that fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1ec5 You're totally right here, and NavigationService is only composed as-is for purposes of convenience. I am planning on doing exactly that you suggested as part of the refactor delegation protocols task.

@objc(MBNavigationService)
public protocol NavigationService: class, CLLocationManagerDelegate {
var locationSource: NavigationLocationManager { get }
var router: Router { get }
var route: Route { get set }
var eventsManager: EventsManager { get }
weak var delegate: NavigationServiceDelegate? { get set }

func start()
func stop()
}

@objc(MBNavigationService)
public class MapboxNavigationService: NSObject, NavigationService {
public var locationSource: NavigationLocationManager
var directionsService: Directions
public var router: Router
public var eventsManager: EventsManager
public weak var delegate: NavigationServiceDelegate?

@objc convenience init(route: Route) {
self.init(route: route, directions: nil, locationSource: nil, eventsManager: nil)
}

@objc required public init(route: Route,
directions directionsOverride: Directions? = nil,
locationSource locationOverride: NavigationLocationManager? = nil,
eventsManager eventsOverride: EventsManager? = nil)
{
eventsManager = eventsOverride ?? EventsManager(accessToken: route.accessToken)
directionsService = directionsOverride ?? Directions.shared
locationSource = locationOverride ?? NavigationLocationManager()
locationSource.activityType = route.routeOptions.activityType

router = RouteController(along: route, directions: directionsService, locationManager: locationSource, eventsManager: eventsManager)
super.init()

router.delegate = self
locationSource.delegate = self
}

public var route: Route {
get {
return router.route
}
set {
router.route = newValue
}
}
public func start() {
locationSource.startUpdatingHeading()
locationSource.startUpdatingLocation()
}

public func stop() {
locationSource.stopUpdatingHeading()
locationSource.stopUpdatingLocation()
}

}

extension MapboxNavigationService: CLLocationManagerDelegate {
public func locationManager(_ manager: CLLocationManager, didUpdateHeading newHeading: CLHeading) {
router.locationManager?(manager, didUpdateHeading: newHeading)
}
public func locationManager(_ manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) {
router.locationManager?(manager, didUpdateLocations: locations)
}
}

//MARK: - RouteControllerDelegate
extension MapboxNavigationService: RouteControllerDelegate {
typealias Default = RouteController.DefaultBehavior

public func routeController(_ routeController: RouteController, willRerouteFrom location: CLLocation) {
delegate?.routeController?(routeController, willRerouteFrom: location)
}
public func routeController(_ routeController: RouteController, didRerouteAlong route: Route) {
delegate?.routeController?(routeController, didRerouteAlong: route)
}
public func routeController(_ routeController: RouteController, didFailToRerouteWith error: Error) {
delegate?.routeController?(routeController, didFailToRerouteWith: error)
}
public func routeController(_ routeController: RouteController, didUpdate locations: [CLLocation]) {
delegate?.routeController?(routeController, didUpdate: locations)
}

//MARK: Questions
public func routeController(_ routeController: RouteController, shouldRerouteFrom location: CLLocation) -> Bool {
return delegate?.routeController?(routeController, shouldRerouteFrom: location) ?? Default.shouldRerouteFromLocation
}
public func routeController(_ routeController: RouteController, shouldDiscard location: CLLocation) -> Bool {
return delegate?.routeController?(routeController,shouldDiscard: location) ?? Default.shouldDiscardLocation
}
public func routeController(_ routeController: RouteController, didArriveAt waypoint: Waypoint) -> Bool {
return delegate?.routeController?(routeController, didArriveAt: waypoint) ?? Default.didArriveAtWaypoint
}
public func routeController(_ routeController: RouteController, shouldPreventReroutesWhenArrivingAt waypoint: Waypoint) -> Bool {
return delegate?.routeController?(routeController, shouldPreventReroutesWhenArrivingAt: waypoint) ?? Default.shouldPreventReroutesWhenArrivingAtWaypoint
}
public func routeControllerShouldDisableBatteryMonitoring(_ routeController: RouteController) -> Bool {
return delegate?.routeControllerShouldDisableBatteryMonitoring?(routeController) ?? Default.shouldDisableBatteryMonitoring
}
}