NAVAND-1636: do not set routes to NN when session is already started

changelog/unreleased/bugfixes/7662.md

@@ -0,0 +1 @@
+- Fixed an issue where the first voice instruction might have been played thrice when switching between regular session and replay with route being set.

instrumentation-tests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/WaypointsTest.kt

@@ -20,6 +20,7 @@ import com.mapbox.navigation.core.internal.extensions.flowLocationMatcherResult
 import com.mapbox.navigation.instrumentation_tests.R
 import com.mapbox.navigation.instrumentation_tests.activity.EmptyTestActivity
 import com.mapbox.navigation.instrumentation_tests.utils.ApproximateCoordinates
+import com.mapbox.navigation.instrumentation_tests.utils.assertions.waitUntilHasSize
 import com.mapbox.navigation.instrumentation_tests.utils.http.MockDirectionsRequestHandler
 import com.mapbox.navigation.instrumentation_tests.utils.location.MockLocationReplayerRule
 import com.mapbox.navigation.instrumentation_tests.utils.readRawFileText
@@ -37,7 +38,6 @@ import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.filter
 import kotlinx.coroutines.flow.take
 import kotlinx.coroutines.flow.toList
-import kotlinx.coroutines.withTimeout
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNull
 import org.junit.Before
@@ -278,15 +278,6 @@ class WaypointsTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::class.java)
         assertEquals(expected.longitude(), actual.longitude(), 0.00001)
     }
 
-    private suspend fun List<*>.waitUntilHasSize(size: Int) {
-        val list = this
-        withTimeout(10000) {
-            while (list.size < size) {
-                delay(50)
-            }
-        }
-    }
-
     private fun stayOnPosition(point: Point, bearing: Float = 0f) {
         mockLocationReplayerRule.loopUpdate(
             mockLocationUpdatesRule.generateLocationUpdate {

instrumentation-tests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/ui/VoiceInstructionsTest.kt

@@ -0,0 +1,97 @@
+package com.mapbox.navigation.instrumentation_tests.ui
+
+import android.location.Location
+import androidx.test.espresso.Espresso
+import com.mapbox.api.directions.v5.models.VoiceInstructions
+import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
+import com.mapbox.navigation.base.options.NavigationOptions
+import com.mapbox.navigation.base.options.RoutingTilesOptions
+import com.mapbox.navigation.core.MapboxNavigation
+import com.mapbox.navigation.core.MapboxNavigationProvider
+import com.mapbox.navigation.core.replay.route.ReplayRouteSession
+import com.mapbox.navigation.core.trip.session.VoiceInstructionsObserver
+import com.mapbox.navigation.instrumentation_tests.utils.assertions.waitUntilHasSize
+import com.mapbox.navigation.instrumentation_tests.utils.history.MapboxHistoryTestRule
+import com.mapbox.navigation.instrumentation_tests.utils.location.MockLocationReplayerRule
+import com.mapbox.navigation.instrumentation_tests.utils.location.stayOnPosition
+import com.mapbox.navigation.instrumentation_tests.utils.routes.RoutesProvider
+import com.mapbox.navigation.instrumentation_tests.utils.routes.RoutesProvider.toNavigationRoutes
+import com.mapbox.navigation.testing.ui.BaseCoreNoCleanUpTest
+import com.mapbox.navigation.testing.ui.utils.MapboxNavigationRule
+import com.mapbox.navigation.testing.ui.utils.coroutines.sdkTest
+import com.mapbox.navigation.testing.ui.utils.coroutines.setNavigationRoutesAsync
+import com.mapbox.navigation.testing.ui.utils.getMapboxAccessTokenFromResources
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotEquals
+import org.junit.Before
+import org.junit.Rule
+import org.junit.Test
+import java.net.URI
+
+class VoiceInstructionsTest : BaseCoreNoCleanUpTest() {
+
+    @get:Rule
+    val mapboxNavigationRule = MapboxNavigationRule()
+
+    @get:Rule
+    val mockLocationReplayerRule = MockLocationReplayerRule(mockLocationUpdatesRule)
+
+    @get:Rule
+    val mapboxHistoryTestRule = MapboxHistoryTestRule()
+
+    @Before
+    fun setup() {
+        Espresso.onIdle()
+    }
+
+    override fun setupMockLocation(): Location {
+        val mockRoute = RoutesProvider.dc_very_short(context)
+        return mockLocationUpdatesRule.generateLocationUpdate {
+            latitude = mockRoute.routeWaypoints.first().latitude()
+            longitude = mockRoute.routeWaypoints.first().longitude()
+        }
+    }
+
+    @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
+    @Test
+    fun voiceInstructionIsDuplicatedOnceWhenReplayIsStarted() = sdkTest {
+        val mockRoute = RoutesProvider.dc_very_short(context)
+        mockWebServerRule.requestHandlers.addAll(mockRoute.mockRequestHandlers)
+        val voiceInstructions = mutableListOf<VoiceInstructions>()
+        val voiceInstructionsObserver = VoiceInstructionsObserver {
+            voiceInstructions.add(it)
+        }
+        val mapboxNavigation = createMapboxNavigation()
+        mapboxNavigation.registerVoiceInstructionsObserver(voiceInstructionsObserver)
+        stayOnPosition(
+            mockRoute.routeWaypoints.first(),
+            0f,
+        ) {
+            mapboxNavigation.startTripSession()
+            mapboxNavigation.setNavigationRoutesAsync(mockRoute.toNavigationRoutes())
+            voiceInstructions.waitUntilHasSize(1)
+        }
+        val relayRouteSession = ReplayRouteSession()
+        relayRouteSession.onAttached(mapboxNavigation)
+        voiceInstructions.waitUntilHasSize(3, timeoutMillis = 15000)
+
+        // the first instruction is duplicated once as a result of starting replay session
+        assertEquals(voiceInstructions[0], voiceInstructions[1])
+        // the first instruction id not duplicated anymore
+        assertNotEquals(voiceInstructions[1], voiceInstructions[2])
+    }
+
+    private fun createMapboxNavigation(): MapboxNavigation {
+        val navigationOptions = NavigationOptions.Builder(context)
+            .accessToken(getMapboxAccessTokenFromResources(context))
+            .routingTilesOptions(
+                RoutingTilesOptions.Builder()
+                    .tilesBaseUri(URI(mockWebServerRule.baseUrl))
+                    .build()
+            )
+            .build()
+        return MapboxNavigationProvider.create(navigationOptions).also {
+            mapboxHistoryTestRule.historyRecorder = it.historyRecorder
+        }
+    }
+}

instrumentation-tests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/utils/assertions/ListAssertions.kt

@@ -0,0 +1,13 @@
+package com.mapbox.navigation.instrumentation_tests.utils.assertions
+
+import kotlinx.coroutines.delay
+import kotlinx.coroutines.withTimeout
+
+suspend fun List<*>.waitUntilHasSize(size: Int, timeoutMillis: Long = 10000) {
+    val list = this
+    withTimeout(timeoutMillis) {
+        while (list.size < size) {
+            delay(50)
+        }
+    }
+}

libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt

@@ -1995,11 +1995,23 @@ class MapboxNavigation @VisibleForTesting internal constructor(
 
     private fun startSession(withTripService: Boolean, withReplayEnabled: Boolean) {
         runIfNotDestroyed {
+            val previousState = tripSession.getState()
             tripSession.start(
                 withTripService = withTripService,
                 withReplayEnabled = withReplayEnabled
             )
-            resetTripSessionRoutes()
+            // It's possible that we are in a state when routes are set,
+            // but session is not started. In this case, as soon as routes are set,
+            // NN starts generating status updates (which results in our route progress updates,
+            // voice and banner instructions events, etc.).
+            // If we first set the routes and then start the session, we might lose all the events
+            // between these two actions (because native status observer is only registered
+            // when the session is started). To avoid that,
+            // we set the routes again on session start. Then NN will generate all the
+            // relevant events again, because it will treat the routes as completely new.
+            if (previousState == TripSessionState.STOPPED) {
+                resetTripSessionRoutes()
+            }
             notificationChannelField?.let {
                 monitorNotificationActionButton(it.get(null) as ReceiveChannel<NotificationAction>)
             }

libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationBaseTest.kt

@@ -38,6 +38,7 @@ import com.mapbox.navigation.core.trip.session.NavigationSession
 import com.mapbox.navigation.core.trip.session.NavigationSessionState
 import com.mapbox.navigation.core.trip.session.TripSession
 import com.mapbox.navigation.core.trip.session.TripSessionLocationEngine
+import com.mapbox.navigation.core.trip.session.TripSessionState
 import com.mapbox.navigation.core.trip.session.createSetRouteResult
 import com.mapbox.navigation.metrics.internal.EventsServiceProvider
 import com.mapbox.navigation.metrics.internal.TelemetryServiceProvider
@@ -84,7 +85,9 @@ internal open class MapboxNavigationBaseTest {
     val cache: CacheHandle = mockk(relaxUnitFun = true)
     val navigator: MapboxNativeNavigator = mockk(relaxUnitFun = true)
     val tripService: TripService = mockk(relaxUnitFun = true)
-    val tripSession: TripSession = mockk(relaxUnitFun = true)
+    val tripSession: TripSession = mockk(relaxUnitFun = true) {
+        every { getState() } returns TripSessionState.STOPPED
+    }
     val locationEngine: LocationEngine = mockk(relaxUnitFun = true)
     val distanceFormatterOptions: DistanceFormatterOptions = mockk(relaxed = true)
     val routingTilesOptions: RoutingTilesOptions = mockk(relaxed = true)

libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationTest.kt

@@ -122,7 +122,7 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
     }
 
     @Test
-    fun `trip session route is reset after trip session is restarted`() {
+    fun `trip session route is reset after trip session is started`() {
         createMapboxNavigation()
         val primary = mockk<NavigationRoute>()
         val routes = listOf(primary, mockk())
@@ -135,6 +135,7 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
             }
         }
         mapboxNavigation.stopTripSession()
+        every { tripSession.getState() } returns TripSessionState.STOPPED
         mapboxNavigation.startTripSession()
 
         coVerify(exactly = 1) {
@@ -145,6 +146,30 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
         }
     }
 
+    @Test
+    fun `trip session route is not reset after trip session is started twice`() {
+        createMapboxNavigation()
+        val primary = mockk<NavigationRoute>()
+        val routes = listOf(primary, mockk())
+        val currentLegIndex = 3
+        every { directionsSession.routes } returns routes
+        every { directionsSession.initialLegIndex } returns 2
+        every { tripSession.getRouteProgress() } returns mockk {
+            every { currentLegProgress } returns mockk {
+                every { legIndex } returns currentLegIndex
+            }
+        }
+        mapboxNavigation.stopTripSession()
+        mapboxNavigation.startTripSession()
+        clearAllMocks(answers = false)
+        every { tripSession.getState() } returns TripSessionState.STARTED
+        mapboxNavigation.startTripSession()
+
+        coVerify(exactly = 0) {
+            tripSession.setRoutes(any(), any())
+        }
+    }
+
     @Test
     fun `getZLevel returns current z level`() {
         createMapboxNavigation()
@@ -1794,6 +1819,7 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
                     directionsSession.routes
                 } returns (firstArg() as DirectionsSessionRoutes).acceptedRoutes
             }
+            every { tripSession.getState() } returns TripSessionState.STOPPED
 
             pauseDispatcher {
                 mapboxNavigation.setNavigationRoutes(inputRoutes)