-
Notifications
You must be signed in to change notification settings - Fork 311
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
Conversation
var directionsService: Directions | ||
public var router: Router | ||
public var eventsManager: EventsManager | ||
public var delegate: NavigationServiceDelegate? |
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.
Should this be weak
?
} | ||
} | ||
// @objc public var route: Route! { |
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's happening here? I'd rather not bring commented-out code into master
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.
Apologies, that shouldn't of made it into the PR. 😅
|
||
let mapViewController = RouteMapViewController(routeController: self.routeController, delegate: self) | ||
// self.routeController = RouteController(along: route, directions: directions, locationManager: locationManager ?? NavigationLocationManager()) |
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.
Is this block of commented-out code still needed? Would rather not merge this in.
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.
Ugh, this shouldn't of made it in either. Sourcetree is being difficult today.
public func tunnelIntersectionManager(_ manager: TunnelIntersectionManager, willEnableAnimationAt location: CLLocation) { | ||
routeController.tunnelIntersectionManager(manager, willEnableAnimationAt: location) | ||
// routeController.tunnelIntersectionManager(manager, willEnableAnimationAt: location) |
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 seems like a TODO
styleManager.applyStyle(type: .night) | ||
} | ||
|
||
public func tunnelIntersectionManager(_ manager: TunnelIntersectionManager, willDisableAnimationAt location: CLLocation) { | ||
routeController.tunnelIntersectionManager(manager, willDisableAnimationAt: location) | ||
// routeController.tunnelIntersectionManager(manager, willDisableAnimationAt: location) |
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.
Another TODO ?
Please focus here next. Refactoring without test coverage is 👎 |
NavigationSettings.shared.distanceUnit = route.routeOptions.locale.usesMetric ? .kilometer : .mile | ||
mapViewController?.notifyDidReroute(route: route) | ||
mapViewController?.notifyDidReroute(route: route) |
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: funky indentation.
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.
- Fix this
navigationViewController.routeController.routeProgress.legIndex += 1 | ||
navigationViewController.routeController.resume() | ||
guard let navigationViewController = self.presentedViewController as? NavigationViewController, | ||
let navService = navigationViewController.navigationService else { return } |
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 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).
import CoreLocation | ||
import MapboxDirections | ||
|
||
@objc public protocol NavigationServiceDelegate: RouterDelegate, TunnelIntersectionManagerDelegate {} |
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.
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.
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.
@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.
ffcaf54
to
8f47670
Compare
|
||
navigation.reroute(from: CLLocation(latitude: 0, longitude: 0), along: navigation.routeProgress) | ||
directionsClientSpy.fireLastCalculateCompletion(with: nil, routes: nil, error: NSError()) | ||
let routeController = navigation.router as! RouteController |
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.
If reroute(from:along)
stays public, this forced-cast is unnecessary.
UIApplication.shared.delegate!.window!!.addSubview(navigationViewController.view) | ||
|
||
let service = dependencies.navigationService | ||
|
||
// Identify a location without a custom road name. | ||
let fultonStreetLocation = dependencies.poi[2] | ||
// let fultonStreetLocation = service.router.route.legs.first!.steps[5].intersections!.first!.location | ||
|
||
|
||
navigationViewController.mapViewController!.labelRoadNameCompletionHandler = { (defaultRaodNameAssigned) in |
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.
defaultRaodNameAssigned
locationEngine = nil | ||
locationManagerDesiredAccuracy = nil | ||
} | ||
locationEngine = dataSource.locationSource.description |
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.
Use the name of the class instead. LocationSource.description
only supports simulation or device but you can also simulate on a device, or replay using the simulator, or use a custom location source. This information is useful when debugging events.
@@ -15,6 +15,14 @@ import UIKit | |||
open class NavigationLocationManager: CLLocationManager { | |||
|
|||
var lastKnownLocation: CLLocation? | |||
override open var delegate: CLLocationManagerDelegate? { |
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: no need to override the default implementation for delegate with the same implementation as the superclass already provides.
…o use that delegate protocol. Also changed `RouteControllerDelegate` into `RouterDelegate`.
# Conflicts: # MapboxCoreNavigation/RouteController.swift
This happens because I changed Carthage dependecies to build mapbox-navigation-ios/circle.yml Line 56 in a139f89
|
Instead of directions being memoized within the NavigationViewController, it is now a computed property, based off of the navigationService. Also cleaning up some unused cruft.
# Conflicts: # Examples/Objective-C/ViewController.m # MapboxNavigation/NavigationViewController.swift # MapboxNavigationTests/LeaksSpec.swift # MapboxNavigationTests/NavigationViewControllerTests.swift
# Conflicts: # Cartfile.resolved # Examples/Objective-C/ViewController.m # Examples/Swift/CustomViewController.swift # MapboxCoreNavigation/EventDetails.swift # MapboxCoreNavigation/EventsManager.swift # MapboxCoreNavigation/RouteController.swift # MapboxCoreNavigationTests/MapboxCoreNavigationTests.swift # MapboxCoreNavigationTests/RouteControllerTests.swift # MapboxCoreNavigationTests/TunnelIntersectionManagerTests.swift # MapboxNavigation.xcodeproj/project.pbxproj # MapboxNavigation/FeedbackViewController.swift # MapboxNavigation/NavigationViewController.swift # MapboxNavigation/RouteMapViewController.swift # MapboxNavigationTests/LaneTests.swift # MapboxNavigationTests/LeaksSpec.swift # MapboxNavigationTests/NavigationViewControllerTests.swift # MapboxNavigationTests/StepsViewControllerTests.swift
…dn't get a progress update" related
…eing passed through on carplay
# Conflicts: # MapboxCoreNavigation/SimulatedLocationManager.swift
Part 1 of the "Nav To The Future" initiative.
To-Do:
EventsManager
TunnelIntersectionManager
(nowTunnelAuthority
)TunnelIntersectionManagerTests
CHANGELOG.md
/cc @mapbox/navigation-ios @mcwhittemore