diff --git a/changelog/unreleased/bugfixes/6817.md b/changelog/unreleased/bugfixes/6817.md new file mode 100644 index 00000000000..1f4bdeac69a --- /dev/null +++ b/changelog/unreleased/bugfixes/6817.md @@ -0,0 +1 @@ +- Improved `MapboxNavigation#startReplayTripSession` and `ReplayRouteSession` so that the previous trip session does not need to be stopped. :warning: `ReplayRouteSession#onDetached` removed the call to `stopTripSession`. diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayRouteSession.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayRouteSession.kt index f8d649d6646..b612bc9a8d9 100644 --- a/libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayRouteSession.kt +++ b/libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayRouteSession.kt @@ -106,7 +106,6 @@ class ReplayRouteSession : MapboxNavigationObserver { override fun onAttached(mapboxNavigation: MapboxNavigation) { this.replayRouteMapper = ReplayRouteMapper(options.replayRouteOptions) this.mapboxNavigation = mapboxNavigation - mapboxNavigation.stopTripSession() mapboxNavigation.startReplayTripSession() mapboxNavigation.registerRouteProgressObserver(routeProgressObserver) mapboxNavigation.registerRoutesObserver(routesObserver) @@ -139,7 +138,6 @@ class ReplayRouteSession : MapboxNavigationObserver { mapboxNavigation.mapboxReplayer.unregisterObserver(replayEventsObserver) mapboxNavigation.mapboxReplayer.stop() mapboxNavigation.mapboxReplayer.clearEvents() - mapboxNavigation.stopTripSession() this.mapboxNavigation = null this.currentRoute = null } diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt index b6b4a0a461c..9275af0640f 100644 --- a/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt +++ b/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt @@ -267,6 +267,28 @@ internal class MapboxTripSession( } } + private val onRawLocationUpdate: (Location) -> Unit = { rawLocation -> + val locationHash = rawLocation.hashCode() + logD( + "updateRawLocation; system elapsed time: ${System.nanoTime()}; " + + "location ($locationHash) elapsed time: ${rawLocation.elapsedRealtimeNanos}", + LOG_CATEGORY + ) + this.rawLocation = rawLocation + locationObservers.forEach { it.onNewRawLocation(rawLocation) } + mainJobController.scope.launch(start = CoroutineStart.UNDISPATCHED) { + logD( + "updateRawLocation; notify navigator for ($locationHash) - start", + LOG_CATEGORY + ) + navigator.updateLocation(rawLocation.toFixLocation()) + logD( + "updateRawLocation; notify navigator for ($locationHash) - end", + LOG_CATEGORY + ) + } + } + init { navigator.setNativeNavigatorRecreationObserver { if (fallbackVersionsObservers.isNotEmpty()) { @@ -297,39 +319,14 @@ internal class MapboxTripSession( * Start MapboxTripSession */ override fun start(withTripService: Boolean, withReplayEnabled: Boolean) { - if (state == TripSessionState.STARTED) { - return - } - navigator.addNavigatorObserver(navigatorObserver) - if (withTripService) { - tripService.startService() - } - tripSessionLocationEngine.startLocationUpdates(withReplayEnabled) { - updateRawLocation(it) - } - state = TripSessionState.STARTED - } - - private fun updateRawLocation(rawLocation: Location) { - val locationHash = rawLocation.hashCode() - logD( - "updateRawLocation; system elapsed time: ${System.nanoTime()}; " + - "location ($locationHash) elapsed time: ${rawLocation.elapsedRealtimeNanos}", - LOG_CATEGORY - ) - this.rawLocation = rawLocation - locationObservers.forEach { it.onNewRawLocation(rawLocation) } - mainJobController.scope.launch(start = CoroutineStart.UNDISPATCHED) { - logD( - "updateRawLocation; notify navigator for ($locationHash) - start", - LOG_CATEGORY - ) - navigator.updateLocation(rawLocation.toFixLocation()) - logD( - "updateRawLocation; notify navigator for ($locationHash) - end", - LOG_CATEGORY - ) + if (state != TripSessionState.STARTED) { + navigator.addNavigatorObserver(navigatorObserver) + if (withTripService) { + tripService.startService() + } + state = TripSessionState.STARTED } + tripSessionLocationEngine.startLocationUpdates(withReplayEnabled, onRawLocationUpdate) } /** diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/TripSessionLocationEngine.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/TripSessionLocationEngine.kt index f1fec248ae4..9dec05a609c 100644 --- a/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/TripSessionLocationEngine.kt +++ b/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/TripSessionLocationEngine.kt @@ -24,55 +24,60 @@ import java.util.concurrent.TimeUnit * trip session is not using replay, use the [NavigationOptions.locationEngine]. */ internal class TripSessionLocationEngine constructor( - val navigationOptions: NavigationOptions + private val navigationOptions: NavigationOptions, + private val replayLocationEngineProvider: (MapboxReplayer) -> LocationEngine = { + ReplayLocationEngine(it) + } ) { val mapboxReplayer: MapboxReplayer by lazy { MapboxReplayer() } - private val replayLocationEngine: ReplayLocationEngine by lazy { - ReplayLocationEngine(mapboxReplayer) + private val replayLocationEngine: LocationEngine by lazy { + replayLocationEngineProvider.invoke(mapboxReplayer) } - private var locationEngine: LocationEngine = navigationOptions.locationEngine + private var activeLocationEngine: LocationEngine? = null private var onRawLocationUpdate: (Location) -> Unit = { } + private val locationEngineCallback = object : LocationEngineCallback { + override fun onSuccess(result: LocationEngineResult?) { + logD(LOG_CATEGORY) { + "successful location engine callback $result" + } + result?.locations?.lastOrNull()?.let { + logIfLocationIsNotFreshEnough(it) + onRawLocationUpdate(it) + } + } + + override fun onFailure(exception: Exception) { + logD("location on failure exception=$exception", LOG_CATEGORY) + } + } + @SuppressLint("MissingPermission") fun startLocationUpdates(isReplayEnabled: Boolean, onRawLocationUpdate: (Location) -> Unit) { logD(LOG_CATEGORY) { "starting location updates for ${if (isReplayEnabled) "replay " else ""}location engine" } + stopLocationUpdates() this.onRawLocationUpdate = onRawLocationUpdate - val locationEngine = if (isReplayEnabled) { + activeLocationEngine = if (isReplayEnabled) { replayLocationEngine } else { navigationOptions.locationEngine } - locationEngine.requestLocationUpdates( + activeLocationEngine?.requestLocationUpdates( navigationOptions.locationEngineRequest, locationEngineCallback, Looper.getMainLooper() ) - locationEngine.getLastLocation(locationEngineCallback) + activeLocationEngine?.getLastLocation(locationEngineCallback) } fun stopLocationUpdates() { onRawLocationUpdate = { } - locationEngine.removeLocationUpdates(locationEngineCallback) - } - - private var locationEngineCallback = object : LocationEngineCallback { - override fun onSuccess(result: LocationEngineResult?) { - logD(LOG_CATEGORY) { - "successful location engine callback $result" - } - result?.locations?.lastOrNull()?.let { - logIfLocationIsNotFreshEnough(it) - onRawLocationUpdate(it) - } - } - - override fun onFailure(exception: Exception) { - logD("location on failure exception=$exception", LOG_CATEGORY) - } + activeLocationEngine?.removeLocationUpdates(locationEngineCallback) + activeLocationEngine = null } private fun logIfLocationIsNotFreshEnough(location: Location) { @@ -88,7 +93,6 @@ internal class TripSessionLocationEngine constructor( } private companion object { - private const val DELAYED_LOCATION_WARNING_THRESHOLD_MS = 500 // 0.5s private const val LOG_CATEGORY = "TripSessionLocationEngine" } diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/replay/route/ReplayRouteSessionTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/replay/route/ReplayRouteSessionTest.kt index 8c44ee8a0bf..976eaedac72 100644 --- a/libnavigation-core/src/test/java/com/mapbox/navigation/core/replay/route/ReplayRouteSessionTest.kt +++ b/libnavigation-core/src/test/java/com/mapbox/navigation/core/replay/route/ReplayRouteSessionTest.kt @@ -84,17 +84,6 @@ class ReplayRouteSessionTest { unmockkAll() } - @Test - fun `onAttached - should stop trip session and start replay session`() { - sut.onAttached(mapboxNavigation) - - verifyOrder { - mapboxNavigation.stopTripSession() - mapboxNavigation.startReplayTripSession() - replayer.play() - } - } - @Test fun `onAttached - should reset trip session and replayer when navigation routes are cleared`() { val routesObserver = slot() @@ -139,13 +128,13 @@ class ReplayRouteSessionTest { } @Test - fun `onDetached - should stop trip session and replayer`() { + fun `onDetached - should stop and clear the replayer`() { sut.onDetached(mapboxNavigation) verifyOrder { + replayer.unregisterObserver(any()) replayer.stop() replayer.clearEvents() - mapboxNavigation.stopTripSession() } } @@ -251,7 +240,6 @@ class ReplayRouteSessionTest { ) verifyOrder { - mapboxNavigation.stopTripSession() mapboxNavigation.startReplayTripSession() replayer.play() replayer.pushEvents(any()) diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/trip/session/MapboxTripSessionTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/trip/session/MapboxTripSessionTest.kt index 15f74d9dc10..6e1c36fa19d 100644 --- a/libnavigation-core/src/test/java/com/mapbox/navigation/core/trip/session/MapboxTripSessionTest.kt +++ b/libnavigation-core/src/test/java/com/mapbox/navigation/core/trip/session/MapboxTripSessionTest.kt @@ -238,16 +238,39 @@ class MapboxTripSessionTest { @Test fun startSessionWithTripServiceCallStartAgain() { tripSession.start(true) - tripSession.start(false) - every { tripService.hasServiceStarted() } returns true - assertTrue(tripSession.isRunningWithForegroundService()) - verify(exactly = 1) { tripService.startService() } - verify(exactly = 1) { tripSessionLocationEngine.startLocationUpdates(any(), any()) } + tripSession.stop() + + verifyOrder { + navigator.addNavigatorObserver(any()) + tripService.startService() + tripSessionLocationEngine.startLocationUpdates(any(), any()) + tripSessionLocationEngine.startLocationUpdates(any(), any()) + navigator.removeNavigatorObserver(any()) + tripSession.stop() + tripSessionLocationEngine.stopLocationUpdates() + } + } + @Test + fun startSessionWithReplayCallStartAgain() { + tripSession.start(true, withReplayEnabled = false) + tripSession.start(true, withReplayEnabled = true) + tripSession.start(true, withReplayEnabled = false) tripSession.stop() + + verifyOrder { + navigator.addNavigatorObserver(any()) + tripService.startService() + tripSessionLocationEngine.startLocationUpdates(false, any()) + tripSessionLocationEngine.startLocationUpdates(true, any()) + tripSessionLocationEngine.startLocationUpdates(false, any()) + navigator.removeNavigatorObserver(any()) + tripSession.stop() + tripSessionLocationEngine.stopLocationUpdates() + } } @Test diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/trip/session/TripSessionLocationEngineTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/trip/session/TripSessionLocationEngineTest.kt index 35ef9136108..abbfc99224d 100644 --- a/libnavigation-core/src/test/java/com/mapbox/navigation/core/trip/session/TripSessionLocationEngineTest.kt +++ b/libnavigation-core/src/test/java/com/mapbox/navigation/core/trip/session/TripSessionLocationEngineTest.kt @@ -13,7 +13,7 @@ import io.mockk.every import io.mockk.mockk import io.mockk.slot import io.mockk.verify -import org.junit.Before +import io.mockk.verifyOrder import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -27,34 +27,21 @@ class TripSessionLocationEngineTest { @get:Rule val loggerRule = LoggingFrontendTestRule() - private val locationCallbackSlot = slot>() - private val locationEngine: LocationEngine = mockk(relaxUnitFun = true) - private val locationEngineResult: LocationEngineResult = mockk(relaxUnitFun = true) - private val location: Location = mockk(relaxed = true) - private val context: Context = ApplicationProvider.getApplicationContext() + private val deviceLocationEngine = mockLocationEngine() + private val replayLocationEngine = mockLocationEngine() private val navigationOptions = NavigationOptions.Builder(context) - .locationEngine(locationEngine) + .locationEngine(deviceLocationEngine) .build() - private val tripSessionLocationEngine = TripSessionLocationEngine(navigationOptions) - @Before - fun setup() { - every { - locationEngine.requestLocationUpdates( - any(), - capture(locationCallbackSlot), - any() - ) - } answers {} - every { locationEngineResult.locations } returns listOf(location) + private val sut = TripSessionLocationEngine(navigationOptions) { + replayLocationEngine } @Test fun `should request location updates from navigation options when replay is disabled`() { - tripSessionLocationEngine.startLocationUpdates(false) { - // This test is not verifying the callback - } + val onRawLocationUpdate: (Location) -> Unit = mockk() + sut.startLocationUpdates(false, onRawLocationUpdate) verify(exactly = 1) { navigationOptions.locationEngine.requestLocationUpdates( @@ -67,13 +54,51 @@ class TripSessionLocationEngineTest { @Test fun `should stop location updates from navigation options when replay is disabled`() { - tripSessionLocationEngine.startLocationUpdates(false) { - // This test is not verifying the callback + val onRawLocationUpdate: (Location) -> Unit = mockk() + sut.startLocationUpdates(false, onRawLocationUpdate) + sut.stopLocationUpdates() + + verifyOrder { + navigationOptions.locationEngine.requestLocationUpdates(any(), any(), any()) + navigationOptions.locationEngine.removeLocationUpdates( + any>() + ) } - tripSessionLocationEngine.stopLocationUpdates() + } + + @Test + fun `should not request location updates from replay engine when replay is disabled`() { + val onRawLocationUpdate: (Location) -> Unit = mockk() + sut.startLocationUpdates(false, onRawLocationUpdate) + + verify(exactly = 0) { + replayLocationEngine.requestLocationUpdates(any(), any(), any()) + } + } + + @Test + fun `should request location updates from replay engine when replay is enable`() { + val onRawLocationUpdate: (Location) -> Unit = mockk() + sut.startLocationUpdates(true, onRawLocationUpdate) verify(exactly = 1) { - navigationOptions.locationEngine.removeLocationUpdates( + replayLocationEngine.requestLocationUpdates( + any(), + any(), + Looper.getMainLooper() + ) + } + } + + @Test + fun `should stop location updates from replay engine when replay is enabled`() { + val onRawLocationUpdate: (Location) -> Unit = mockk() + sut.startLocationUpdates(true, onRawLocationUpdate) + sut.stopLocationUpdates() + + verifyOrder { + replayLocationEngine.requestLocationUpdates(any(), any(), any()) + replayLocationEngine.removeLocationUpdates( any>() ) } @@ -81,12 +106,57 @@ class TripSessionLocationEngineTest { @Test fun `should not request location updates from navigation options when replay is enabled`() { - tripSessionLocationEngine.startLocationUpdates(true) { - // This test is not verifying the callback + val onRawLocationUpdate: (Location) -> Unit = mockk() + sut.startLocationUpdates(true, onRawLocationUpdate) + + verify(exactly = 0) { + navigationOptions.locationEngine.requestLocationUpdates(any(), any(), any()) } + } + + @Test + fun `should remove location updates from previous location engine`() { + val onRawLocationUpdate: (Location) -> Unit = mockk() + sut.startLocationUpdates(true, onRawLocationUpdate) verify(exactly = 0) { navigationOptions.locationEngine.requestLocationUpdates(any(), any(), any()) } } + + @Test + fun `startLocationUpdates should remove updates from previous location engine`() { + val onRawLocationUpdate: (Location) -> Unit = mockk() + sut.startLocationUpdates(true, onRawLocationUpdate) + sut.startLocationUpdates(false, onRawLocationUpdate) + sut.startLocationUpdates(true, onRawLocationUpdate) + + verifyOrder { + replayLocationEngine.requestLocationUpdates(any(), any(), any()) + replayLocationEngine.removeLocationUpdates( + any>() + ) + navigationOptions.locationEngine.requestLocationUpdates(any(), any(), any()) + navigationOptions.locationEngine.removeLocationUpdates( + any>() + ) + replayLocationEngine.requestLocationUpdates(any(), any(), any()) + } + } + + private fun mockLocationEngine(): LocationEngine { + val locationEngine: LocationEngine = mockk(relaxUnitFun = true) + val locationCallbackSlot = slot>() + val locationEngineResult: LocationEngineResult = mockk(relaxUnitFun = true) + val location: Location = mockk(relaxed = true) + every { + locationEngine.requestLocationUpdates( + any(), + capture(locationCallbackSlot), + any() + ) + } answers {} + every { locationEngineResult.locations } returns listOf(location) + return locationEngine + } } diff --git a/libnavui-app/src/main/java/com/mapbox/navigation/ui/app/internal/controller/TripSessionStarterStateController.kt b/libnavui-app/src/main/java/com/mapbox/navigation/ui/app/internal/controller/TripSessionStarterStateController.kt index 29a2bf2c406..f3e6b246bfa 100644 --- a/libnavui-app/src/main/java/com/mapbox/navigation/ui/app/internal/controller/TripSessionStarterStateController.kt +++ b/libnavui-app/src/main/java/com/mapbox/navigation/ui/app/internal/controller/TripSessionStarterStateController.kt @@ -1,7 +1,6 @@ package com.mapbox.navigation.ui.app.internal.controller import android.annotation.SuppressLint -import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI import com.mapbox.navigation.ui.app.internal.Action import com.mapbox.navigation.ui.app.internal.State import com.mapbox.navigation.ui.app.internal.Store @@ -12,7 +11,6 @@ import com.mapbox.navigation.ui.app.internal.tripsession.TripSessionStarterState * The class is responsible to start and stop the `TripSession` for NavigationView. * @param store defines the current screen state */ -@OptIn(ExperimentalPreviewMapboxNavigationAPI::class) @SuppressLint("MissingPermission") class TripSessionStarterStateController(store: Store) : StateController() { init { diff --git a/libnavui-dropin/src/main/java/com/mapbox/navigation/dropin/tripsession/TripSessionComponent.kt b/libnavui-dropin/src/main/java/com/mapbox/navigation/dropin/tripsession/TripSessionComponent.kt index 209d386b01a..9b97fa2ab6e 100644 --- a/libnavui-dropin/src/main/java/com/mapbox/navigation/dropin/tripsession/TripSessionComponent.kt +++ b/libnavui-dropin/src/main/java/com/mapbox/navigation/dropin/tripsession/TripSessionComponent.kt @@ -79,7 +79,7 @@ internal class TripSessionComponent( private fun startTripSession(mapboxNavigation: MapboxNavigation) { replayRouteTripSession?.onDetached(mapboxNavigation) replayRouteTripSession = null - mapboxNavigation.ensureTripSessionStarted() + mapboxNavigation.startTripSession() } private fun startReplayTripSession(mapboxNavigation: MapboxNavigation) { @@ -88,12 +88,6 @@ internal class TripSessionComponent( replayRouteTripSession?.onAttached(mapboxNavigation) } - private fun MapboxNavigation.ensureTripSessionStarted() { - if (getTripSessionState() != TripSessionState.STARTED) { - startTripSession() - } - } - private fun MapboxNavigation.ensureTripSessionStopped() { if (getTripSessionState() != TripSessionState.STOPPED) { stopTripSession() diff --git a/libnavui-dropin/src/test/java/com/mapbox/navigation/dropin/tripsession/TripSessionComponentTest.kt b/libnavui-dropin/src/test/java/com/mapbox/navigation/dropin/tripsession/TripSessionComponentTest.kt index b9cd85f6939..0b77e1ff4a2 100644 --- a/libnavui-dropin/src/test/java/com/mapbox/navigation/dropin/tripsession/TripSessionComponentTest.kt +++ b/libnavui-dropin/src/test/java/com/mapbox/navigation/dropin/tripsession/TripSessionComponentTest.kt @@ -114,7 +114,7 @@ class TripSessionComponentTest { } @Test - fun `EnableTripSession will restart a trip session when replay is enabled`() = + fun `EnableTripSession will not stopTripSession`() = runBlockingTest { testStore.setState( State( @@ -139,9 +139,9 @@ class TripSessionComponentTest { verifyOrder { mapboxNavigation.startReplayTripSession() - mapboxNavigation.stopTripSession() mapboxNavigation.startTripSession() } + verify(exactly = 0) { mapboxNavigation.stopTripSession() } } @Test diff --git a/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/QaTestApplication.kt b/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/QaTestApplication.kt index fa5501d96ef..e5e10d37c71 100644 --- a/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/QaTestApplication.kt +++ b/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/QaTestApplication.kt @@ -1,5 +1,12 @@ package com.mapbox.navigation.qa_test_app import android.app.Application +import com.mapbox.common.LogConfiguration +import com.mapbox.common.LoggingLevel -class QaTestApplication : Application() +class QaTestApplication : Application() { + override fun onCreate() { + super.onCreate() + LogConfiguration.setLoggingLevel(LoggingLevel.DEBUG) + } +}