RouteController should get navigation statuses based on a consistent timestamp

[1ec5]

RouteController currently calls Navigator.status(at:) (a wrapper around Navigator.getStatusForTimestamp(_:)) with inconsistent or user-defined timestamps. Historically, the expectation was that the navigator would be able to march to and fro along the timeline of the route. However, this has never quite been the case in MapboxNavigationNative. A forthcoming version of MapboxNavigationNative will remove the ability to ask for the navigation status at an arbitrary time, to avoid misleading clients into thinking it has a capability that doesn’t quite exist.

Projected location

For forward compatibility, we need to remove RouteController.projectedLocation(for:), which allows applications to request the location at an arbitrary time:

mapbox-navigation-ios/MapboxCoreNavigation/RouteController.swift

Lines 340 to 346 in ebab7d0

	/**
	Returns an estimated location at a given timestamp. The timestamp must be
	a future timestamp compared to the last location received by the location manager.
	*/
	public func projectedLocation(for timestamp: Date) -> CLLocation {
	return CLLocation(navigator.status(at:timestamp).location)
	}

Current location

RouteController internally gets navigation statuses based on the time of the last Core Location update, which accounts for potential interruptions in location updates:

mapbox-navigation-ios/MapboxCoreNavigation/RouteController.swift

Line 196 in ebab7d0

let status = navigator.status(at: location.timestamp)

However, a very commonly used property of RouteController gets the status as of the current time, which can easily fall after the last Core Location update:

https://github.com/mapbox/mapbox-navigation-ios/blob/893f57f0bbb5d6f03fac0fe8ed1408519bb150fe/MapboxCoreNavigation/RouteController.swift#L89-L90

This is likely the usual trigger of #2113. A straightforward fix would be to remember the last location’s timestamp and get the status based on that timestamp.

Leg advancing

RouteController.advanceLegIndex(location:) is also problematic because it can take an arbitrary location and passes its timestamp to the navigator:

mapbox-navigation-ios/MapboxCoreNavigation/RouteController.swift

Lines 348 to 351 in ebab7d0

	public func advanceLegIndex(location: CLLocation) {
	let status = navigator.status(at: location.timestamp)
	routeProgress.legIndex = Int(status.legIndex)
	}

This method is required by the Router protocol, but it’s only used internally by LegacyRouteController to increment RouteProgress.legIndex. The example application used to call this method, too, to demonstrate manually advancing to the next leg after presenting an arrival interstitial at an intermediate waypoint. However, nowadays, it increments the leg index directly and restarts the navigation service:

mapbox-navigation-ios/Example/ViewController.swift

Lines 508 to 511 in ebab7d0

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

I’m unsure if that works quite as it’s supposed to. It would be better to encapsulate those details in RouteController.advanceLegIndex(location:), except that the method should take no parameters and always increment the leg index by one. RouteController can easily implement that functionality by calling RouteController.updateRouteLeg(to:).

/cc @mapbox/navigation-ios @mapbox/navnative @d-prukop

[1ec5]

I proposed this issue as a release blocker for v1.0.0 because it entails a backwards-incompatible change to the public API, albeit to a relatively little-used portion of it. One v1.0.0 goes out the door, our only option would be to ignore the passed-in timestamp and location, which would be misleading.

[SiarheiFedartsou]

Navigator.getStatus doesn't increment legIndex on its own, but just always returns current legIndex. It is SDK responsibility to call changeRouteLeg or setRoute when we send complete event in order to advance leg index. So this method has no sense:

mapbox-navigation-ios/MapboxCoreNavigation/RouteController.swift

Lines 348 to 351 in ebab7d0

	public func advanceLegIndex(location: CLLocation) {
	let status = navigator.status(at: location.timestamp)
	routeProgress.legIndex = Int(status.legIndex)
	}

[1ec5]

Agreed, I think this method should instead call RouteController.updateRouteLeg(to:), which correctly calls Navigator.changeRouteLeg(forRoute:leg:) and synchronizes the route progress model. We were just getting lucky because nothing was calling this code, only the corresponding implementation in LegacyRouteController that did work.