Use the pre-existing `mapView` location provider when simulation mode turned off. #3393

ShanMa1991 · 2021-09-22T22:20:40Z

Description

This pr is to fix #3392 and #3276 .

Implementation

To fix the #3392 , the pre-existing locationProvider of the mapView during active navigation is stored when simulating start or mapView set up. When the simulation mode turned off, we use it for location update and override the simulated location provider with it.

To fix #3276, the notification of navigationServiceSimulatingDidChange is added by the MapboxNavigationService to notify users of the simulating status change. And in CarPlay we observe this notification to listen to the simulating status change, and behave the same as mobile NavigationViewController to update thelocationProvider of the mapView based on the simulating status. In this way, the two background location updates would not result in the unmatched behavior or the Navigator rerouting during simulation mode.

Besides, the simulationSpeedMultiplier of the navigationService change also leads to the update of the simulatedLocationProvider for the mapView to allow the locationProvider of the mapView be synced with the locationManager during active navigation.

Removed the carPlayManager(_: didBeginNavigationWith:) delegate method in CarPlayManager and move it to CarPlayNavigationViewController. In fact the navigation service starts in CarPlay when the navigation view loaded in CarPlayNavigationViewController.

Sources/MapboxNavigation/NavigationMapView.swift

ShanMa1991 · 2021-10-06T20:05:53Z

Sources/MapboxNavigation/CarPlayManager.swift

@@ -857,6 +856,8 @@ extension CarPlayManager: CarPlayNavigationViewControllerDelegate {
        if let passiveLocationProvider = navigationMapView?.mapView.location.locationProvider as? PassiveLocationProvider {
            passiveLocationProvider.locationManager.resumeTripSession()
        }
+
+        self.carPlayNavigationViewController = nil


It's to nil out the CarPlayNavigationViewController in CarPlayManager when the CarPlayNavigationViewController dismissed and exit navigation

ShanMa1991 · 2021-10-06T20:08:32Z

Sources/MapboxCoreNavigation/NavigationService.swift

        let progress = router.routeProgress
        delegate?.navigationService(self, willBeginSimulating: progress, becauseOf: intent)
+        announceSimulatingDidChange(simulatingUpdate: .willBeginSimulating)
+        guard !isSimulating else {


Because when CarPlay is used and under simulated mode, the navigationService.simulate(intent:) would be called twice. When we choose to start navigation from mobile, then the previous guard !isSimulating else { return } before the notification would block sending notification to the CarPlay.

@1ec5, could you please take a look at this one when you have a chance? Since NavigationService is shared between CarPlay and iOS, when CarPlay starts active-guidance navigation we pass it to iOS. This means that CarPlay will not really know when navigation starts or finishes as delegate object will be overwritten by iOS.

I wonder if there are any pitfalls, which we might've missed.

Because when CarPlay is used and under simulated mode, the navigationService.simulate(intent:) would be called twice.

I’m not sure I understand… if there’s only one NavigationService, then shouldn’t simulate(intent:) only be called once, since there’s only one poorGPSTimer?

mapbox-navigation-ios/Sources/MapboxCoreNavigation/NavigationService.swift

Lines 309 to 314 in 95d73db

_poorGPSTimer = DispatchTimer(countdown: poorGPSPatience.dispatchInterval) { [weak self] in

guard let self = self,

self.simulationMode == .onPoorGPS ||

(self.simulationMode == .inTunnels && self.isInTunnel(at: self.router.location!, along: self.routeProgress)) else { return }

self.simulate(intent: .poorGPS)

}

Would a more straightforward fix be for this didSet to bail if the new value is equal to the current value?

mapbox-navigation-ios/Sources/MapboxCoreNavigation/NavigationService.swift

Lines 150 to 162 in 95d73db

public var simulationMode: SimulationMode {

didSet {

switch simulationMode {

case .always:

simulate()

case .onPoorGPS, .inTunnels:

poorGPSTimer.arm()

case .never:

poorGPSTimer.disarm()

endSimulation(intent: .manual)

}

}

}

So the issue is that when the navigation starts in mobile with simulation mode, then the mobile would trigger the navigationService to start in simulation mode once. And then the CarPlay would also call the navigationService to start when the viewDidLoad, but this time it would not notify the simulating starts because it's already in simulation mode (the mobile and carPlay share the same service).

So the didSet is triggered when the navigationService property change. But in the test, the CarPlay only starts the navigation in simulation only when the viewDidLoad, and then the navigation service start.

In principle, CarPlayNavigationViewController should be sharing the location manager with RouteController, just like the two classes share a NavigationService. There shouldn’t be two independent location managers to keep in sync; NavigationService doesn’t have the ability to respond to two different location managers at the same time. I don’t disagree with the notification being added, but it seems like a band-aid atop a more fundamental architectural issue that we should prioritize post-v2.0.0.

Sources/MapboxNavigation/NavigationViewportDataSource.swift

Sources/MapboxCoreNavigation/SimulationType.swift

MaximAlien · 2021-10-13T22:59:56Z

Sources/MapboxCoreNavigation/CoreConstants.swift

+
+     The user info dictionary contains the key `MapboxNavigationService.NotificationUserInfoKey.simulatingUpdateKey` and `MapboxNavigationService.NotificationUserInfoKey.simulatedSpeedMultiplierKey`.
+     */
+    static let navigationServiceSimulatingDidChange: Notification.Name = .init(rawValue: "NavigationServiceSimulatingDidChange")


I guess func carPlayManager(_ carPlayManager: CarPlayManager, didBeginNavigationWith service: NavigationService) is no longer needed and can be removed as it was called not when navigation actually started, but when CarPlayNavigationViewController was actually presented?

Yes, the func carPlayManager(_ carPlayManager: CarPlayManager, didBeginNavigationWith service: NavigationService) should be replaced by carPlayManager(self, didPresent: carPlayNavigationViewController). Because the navigation really begin after the navigation service start. But because right now it's associated with the billing. I think it's safer to separate it into another ticket.

Can you elaborate regarding billing? I can see that we only handle func carPlayManager(_ carPlayManager: CarPlayManager, didBeginNavigationWith service: NavigationService) delegate method once on client side, and this delegate handling code can be moved to func carPlayManager(_ carPlayManager: CarPlayManager, didPresent navigationViewController: CarPlayNavigationViewController), but for this we'll have to make CarPlayManager.navigationService public.

In fact the carPlayManager(_: didBeginNavigationWith:) should be called in carPlayNavigationViewController after the navigation service start and view loaded. And the carPlayManager(_: didPresent: ) should also be called here because it's the place when the CarPlay really present the navigation view. And it doesn't need to make CarPlayManager.navigationService public, we could reach this through carPlayManager.carPlayNavigationViewController?.navigationService. And then the billing should start when the view is presented, which requires some change in tests.

Sources/MapboxCoreNavigation/CoreConstants.swift

MaximAlien · 2021-10-13T23:08:08Z

Sources/MapboxCoreNavigation/NavigationService.swift

        let progress = router.routeProgress
        delegate?.navigationService(self, willBeginSimulating: progress, becauseOf: intent)
+        announceSimulatingDidChange(simulatingUpdate: .willBeginSimulating)
+        guard !isSimulating else {


@1ec5, could you please take a look at this one when you have a chance? Since NavigationService is shared between CarPlay and iOS, when CarPlay starts active-guidance navigation we pass it to iOS. This means that CarPlay will not really know when navigation starts or finishes as delegate object will be overwritten by iOS.

I wonder if there are any pitfalls, which we might've missed.

Sources/MapboxNavigation/NavigationViewportDataSource.swift

Sources/MapboxCoreNavigation/CoreConstants.swift

1ec5 · 2021-10-19T01:09:13Z

Sources/MapboxCoreNavigation/NavigationService.swift

        let progress = router.routeProgress
        delegate?.navigationService(self, willBeginSimulating: progress, becauseOf: intent)
+        announceSimulatingDidChange(simulatingUpdate: .willBeginSimulating)
+        guard !isSimulating else {


Because when CarPlay is used and under simulated mode, the navigationService.simulate(intent:) would be called twice.

I’m not sure I understand… if there’s only one NavigationService, then shouldn’t simulate(intent:) only be called once, since there’s only one poorGPSTimer?

mapbox-navigation-ios/Sources/MapboxCoreNavigation/NavigationService.swift

Lines 309 to 314 in 95d73db

_poorGPSTimer = DispatchTimer(countdown: poorGPSPatience.dispatchInterval) { [weak self] in

guard let self = self,

self.simulationMode == .onPoorGPS ||

(self.simulationMode == .inTunnels && self.isInTunnel(at: self.router.location!, along: self.routeProgress)) else { return }

self.simulate(intent: .poorGPS)

}

Would a more straightforward fix be for this didSet to bail if the new value is equal to the current value?

mapbox-navigation-ios/Sources/MapboxCoreNavigation/NavigationService.swift

Lines 150 to 162 in 95d73db

public var simulationMode: SimulationMode {

didSet {

switch simulationMode {

case .always:

simulate()

case .onPoorGPS, .inTunnels:

poorGPSTimer.arm()

case .never:

poorGPSTimer.disarm()

endSimulation(intent: .manual)

}

}

}

azarovalex · 2021-10-19T12:38:23Z

Sources/MapboxCoreNavigation/NavigationService.swift

+    private func announceSimulationDidChange(simulationState: SimulationState) {
+        let userInfo: [NotificationUserInfoKey: Any] = [
+            NotificationUserInfoKey.simulationStateKey: simulationState,
+            NotificationUserInfoKey.simulatedSpeedMultiplierKey: _simulationSpeedMultiplier


Should we send this value if state is notInSimulation?

Yes, we do in the NavigationService.endSimulation(intent:) to send this value if state is notInSimulation, incase the carPlay could not be notified during the active navigation, such as when the user drives off the tunnel.

1ec5

Make sure to test basic interactions in the following scenarios:

Route simulation enabled
Route simulation disabled
Start navigation on the phone, then connect to CarPlay
Connect to CarPlay, then start navigation on the phone
Connect to CarPlay, then start navigation on CarPlay
Start navigation on CarPlay with the application in the background on the phone

1ec5 · 2021-10-19T17:24:49Z

Sources/MapboxCoreNavigation/NavigationService.swift

        let progress = router.routeProgress
        delegate?.navigationService(self, willBeginSimulating: progress, becauseOf: intent)
+        announceSimulatingDidChange(simulatingUpdate: .willBeginSimulating)
+        guard !isSimulating else {


In principle, CarPlayNavigationViewController should be sharing the location manager with RouteController, just like the two classes share a NavigationService. There shouldn’t be two independent location managers to keep in sync; NavigationService doesn’t have the ability to respond to two different location managers at the same time. I don’t disagree with the notification being added, but it seems like a band-aid atop a more fundamental architectural issue that we should prioritize post-v2.0.0.

Sources/MapboxCoreNavigation/CoreConstants.swift

Sources/MapboxCoreNavigation/NavigationService.swift

Sources/MapboxNavigation/CarPlayNavigationViewController.swift

…urned off.

…CarPlayManager.

Example/AppDelegate+CarPlay.swift

Sources/MapboxNavigation/CarPlayNavigationViewController.swift

…iewController.

… turned off. (#3393) Cherry-picked from 29687ef.

1ec5 · 2021-10-20T00:55:35Z

Cherry-picked to the release-v2.0 branch in 9fe469e.

ShanMa1991 added the bug Something isn’t working label Sep 22, 2021

ShanMa1991 added this to the v2.0.0 (GA) milestone Sep 22, 2021

ShanMa1991 requested review from 1ec5 and a team September 22, 2021 22:20

ShanMa1991 self-assigned this Sep 22, 2021

bamx23 reviewed Sep 23, 2021

View reviewed changes

Sources/MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved

ShanMa1991 requested a review from bamx23 September 23, 2021 18:23

bamx23 approved these changes Sep 24, 2021

View reviewed changes

ShanMa1991 force-pushed the shan/location-provider-3392 branch from d0b700f to b0f1939 Compare September 24, 2021 16:59

ShanMa1991 marked this pull request as draft September 24, 2021 23:24

ShanMa1991 force-pushed the shan/location-provider-3392 branch 5 times, most recently from 71ba424 to dc6ca74 Compare October 4, 2021 20:41

ShanMa1991 force-pushed the shan/location-provider-3392 branch from 9fd3495 to 3d75d0a Compare October 6, 2021 20:04

ShanMa1991 commented Oct 6, 2021

View reviewed changes

ShanMa1991 force-pushed the shan/location-provider-3392 branch from d038735 to 258aa6d Compare October 6, 2021 21:48

ShanMa1991 marked this pull request as ready for review October 6, 2021 21:56

ShanMa1991 requested a review from a team October 6, 2021 21:57

ShanMa1991 force-pushed the shan/location-provider-3392 branch 2 times, most recently from b0b7981 to d22f87b Compare October 8, 2021 19:15

ShanMa1991 commented Oct 8, 2021

View reviewed changes

Sources/MapboxNavigation/NavigationViewportDataSource.swift Outdated Show resolved Hide resolved

ShanMa1991 force-pushed the shan/location-provider-3392 branch 3 times, most recently from 45bc86b to 0d881bc Compare October 11, 2021 23:13

MaximAlien reviewed Oct 13, 2021

View reviewed changes

ShanMa1991 force-pushed the shan/location-provider-3392 branch 2 times, most recently from a5944f8 to e0a133d Compare October 14, 2021 23:44

ShanMa1991 force-pushed the shan/location-provider-3392 branch from e0a133d to 84cf2cd Compare October 14, 2021 23:56

ShanMa1991 requested a review from MaximAlien October 15, 2021 00:14

ShanMa1991 force-pushed the shan/location-provider-3392 branch from 84cf2cd to bbddc4f Compare October 18, 2021 18:34

1ec5 reviewed Oct 19, 2021

View reviewed changes

azarovalex reviewed Oct 19, 2021

View reviewed changes

1ec5 approved these changes Oct 19, 2021

View reviewed changes

MaximAlien reviewed Oct 19, 2021

View reviewed changes

Sources/MapboxCoreNavigation/CoreConstants.swift Outdated Show resolved Hide resolved

Sources/MapboxCoreNavigation/NavigationService.swift Outdated Show resolved Hide resolved

Sources/MapboxNavigation/CarPlayNavigationViewController.swift Outdated Show resolved Hide resolved

ShanMa1991 added 4 commits October 19, 2021 12:11

Use the pre-existing mapView location provider when simulation mode t…

11620f7

…urned off.

Update the documentation and the name.

9958264

Update the changeLog.

cdf69ab

Resolve the review comments and clean the code.

9499133

ShanMa1991 force-pushed the shan/location-provider-3392 branch from 9c3f142 to 58eb289 Compare October 19, 2021 19:11

Remove carPlayManager(_: didBeginNavigationWith:) delegate method in …

149fb97

…CarPlayManager.

ShanMa1991 force-pushed the shan/location-provider-3392 branch from 58eb289 to 149fb97 Compare October 19, 2021 20:10

MaximAlien reviewed Oct 19, 2021

View reviewed changes

Example/AppDelegate+CarPlay.swift Outdated Show resolved Hide resolved

ShanMa1991 requested a review from MaximAlien October 19, 2021 22:00

MaximAlien reviewed Oct 19, 2021

View reviewed changes

Sources/MapboxNavigation/CarPlayNavigationViewController.swift Outdated Show resolved Hide resolved

MaximAlien approved these changes Oct 19, 2021

View reviewed changes

Remove the optional check for navigationService of CarPlayNavigationV…

23d3d53

…iewController.

ShanMa1991 force-pushed the shan/location-provider-3392 branch from 269f9b5 to 23d3d53 Compare October 19, 2021 23:14

ShanMa1991 merged commit 29687ef into main Oct 19, 2021

ShanMa1991 deleted the shan/location-provider-3392 branch October 19, 2021 23:52

1ec5 pushed a commit that referenced this pull request Oct 20, 2021

Use the pre-existing mapView location provider when simulation mode…

9fe469e

… turned off. (#3393) Cherry-picked from 29687ef.

1ec5 mentioned this pull request Oct 27, 2021

CarPlayNavigationViewController and RouteController should share location manager #3522

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the pre-existing `mapView` location provider when simulation mode turned off. #3393

Use the pre-existing `mapView` location provider when simulation mode turned off. #3393

ShanMa1991 commented Sep 22, 2021 •

edited

Loading

ShanMa1991 Oct 6, 2021

ShanMa1991 Oct 6, 2021

MaximAlien Oct 13, 2021

1ec5 Oct 19, 2021

ShanMa1991 Oct 19, 2021

1ec5 Oct 19, 2021

MaximAlien Oct 13, 2021

ShanMa1991 Oct 14, 2021

MaximAlien Oct 19, 2021

ShanMa1991 Oct 19, 2021

MaximAlien Oct 13, 2021

1ec5 Oct 19, 2021

azarovalex Oct 19, 2021

ShanMa1991 Oct 19, 2021

1ec5 left a comment

1ec5 Oct 19, 2021

1ec5 commented Oct 20, 2021

	_poorGPSTimer = DispatchTimer(countdown: poorGPSPatience.dispatchInterval) { [weak self] in
	guard let self = self,
	self.simulationMode == .onPoorGPS \|\|
	(self.simulationMode == .inTunnels && self.isInTunnel(at: self.router.location!, along: self.routeProgress)) else { return }
	self.simulate(intent: .poorGPS)
	}

	public var simulationMode: SimulationMode {
	didSet {
	switch simulationMode {
	case .always:
	simulate()
	case .onPoorGPS, .inTunnels:
	poorGPSTimer.arm()
	case .never:
	poorGPSTimer.disarm()
	endSimulation(intent: .manual)
	}
	}
	}

Use the pre-existing mapView location provider when simulation mode turned off. #3393

Use the pre-existing mapView location provider when simulation mode turned off. #3393

Conversation

ShanMa1991 commented Sep 22, 2021 • edited Loading

Description

Implementation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Oct 20, 2021

Use the pre-existing `mapView` location provider when simulation mode turned off. #3393

Use the pre-existing `mapView` location provider when simulation mode turned off. #3393

ShanMa1991 commented Sep 22, 2021 •

edited

Loading