2.9 fix banner and voice instructions removed on navigator#update leg index

* [cp] NavigatorObserver: fix race condition on invoking TripSession#updateLegIndex

* [cp] NavigatorObserver: fix race condition on invoking TripSession#updateLegIndex for voice instructions

* [cp] MapboxTripSession: add logs voice and banner instructions

* [cp] minor fixes

Author: RingerJK

CHANGELOG.md

@@ -5,6 +5,8 @@ Mapbox welcomes participation and contributions from everyone.
 ## Unreleased
 #### Features
 #### Bug fixes and improvements
+- Fixed an issue where `RouteProgress#BannerInstructions` could've become `null` when `MapboxNavigation#updateLegIndex` was called. [#6717](https://github.com/mapbox/mapbox-navigation-android/pull/6717)
+- Fixed an issue where `RouteProgress#VoiceInstructions` could've become `null` when `MapboxNavigation#updateLegIndex` was called. [#6717](https://github.com/mapbox/mapbox-navigation-android/pull/6717)
 
 ## Mapbox Navigation SDK 2.9.4 - 08 December, 2022
 ### Changelog

libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/BannerInstructionEvent.kt

@@ -1,34 +1,58 @@
 package com.mapbox.navigation.core.trip.session
 
 import com.mapbox.api.directions.v5.models.BannerInstructions
+import com.mapbox.navigation.utils.internal.ifNonNull
 
-internal class BannerInstructionEvent {
+internal class BannerInstructionEvent private constructor() {
 
-    var latestInstructionIndex: Int? = null
+    var latestInstructionWrapper: LatestInstructionWrapper? = null
         private set
 
+    val latestInstructionIndex: Int?
+        get() = latestInstructionWrapper?.latestInstructionIndex
+
+    val latestBannerInstructions: BannerInstructions?
+        get() = latestInstructionWrapper?.latestBannerInstructions
+
     var bannerInstructions: BannerInstructions? = null
         private set
 
-    var latestBannerInstructions: BannerInstructions? = null
-        private set
+    companion object {
+        operator fun invoke(): BannerInstructionEvent = BannerInstructionEvent()
+    }
 
     fun isOccurring(bannerInstructions: BannerInstructions?, instructionIndex: Int?): Boolean {
         return updateCurrentBanner(bannerInstructions, instructionIndex)
     }
 
-    fun invalidateLatestBannerInstructions() {
-        latestBannerInstructions = null
-        latestInstructionIndex = null
+    fun invalidateLatestBannerInstructions(latestInstructionWrapper: LatestInstructionWrapper?) {
+        if (latestInstructionWrapper == this.latestInstructionWrapper) {
+            this.latestInstructionWrapper = null
+        }
     }
 
     private fun updateCurrentBanner(banner: BannerInstructions?, instructionIndex: Int?): Boolean {
         bannerInstructions = banner
         if (bannerInstructions != null && bannerInstructions!! != latestBannerInstructions) {
-            latestBannerInstructions = bannerInstructions
-            latestInstructionIndex = instructionIndex
+            latestInstructionWrapper =
+                LatestInstructionWrapper.createOrNull(instructionIndex, bannerInstructions)
             return true
         }
         return false
     }
+
+    data class LatestInstructionWrapper(
+        val latestInstructionIndex: Int,
+        val latestBannerInstructions: BannerInstructions
+    ) {
+        companion object {
+            fun createOrNull(
+                latestInstructionIndex: Int?,
+                latestBannerInstructions: BannerInstructions?
+            ): LatestInstructionWrapper? =
+                ifNonNull(latestInstructionIndex, latestBannerInstructions) { idx, instruction ->
+                    LatestInstructionWrapper(idx, instruction)
+                }
+        }
+    }
 }

libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt

@@ -167,7 +167,10 @@ internal class MapboxTripSession(
             this@MapboxTripSession.primaryRoute = newPrimaryRoute
             roadObjects = newPrimaryRoute?.upcomingRoadObjects ?: emptyList()
             isOffRoute = false
-            invalidateLatestInstructions()
+            invalidateLatestInstructions(
+                bannerInstructionEvent.latestInstructionWrapper,
+                lastVoiceInstruction
+            )
             routeProgress = null
         }.mapValue {
             it.alternatives
@@ -192,7 +195,9 @@ internal class MapboxTripSession(
     private val fallbackVersionsObservers = CopyOnWriteArraySet<FallbackVersionsObserver>()
 
     private val bannerInstructionEvent = BannerInstructionEvent()
-    private var lastVoiceInstruction: VoiceInstructions? = null
+
+    @VisibleForTesting
+    internal var lastVoiceInstruction: VoiceInstructions? = null
 
     private var state: TripSessionState = TripSessionState.STOPPED
         set(value) {
@@ -328,6 +333,11 @@ internal class MapboxTripSession(
                     "state: ${status.routeState}",
                 LOG_CATEGORY
             )
+            logD(
+                "navigatorObserver#onStatus; banner instruction: [${status.bannerInstruction}]," +
+                    " voice instruction: [${status.voiceInstruction}]",
+                LOG_CATEGORY
+            )
 
             val tripStatus = status.getTripStatusFrom(primaryRoute)
             val enhancedLocation = tripStatus.navigationStatus.location.toLocation()
@@ -356,12 +366,13 @@ internal class MapboxTripSession(
                 )
             }
             val remainingWaypoints = calculateRemainingWaypoints(tripStatus)
+            val latestBannerInstructionsWrapper = bannerInstructionEvent.latestInstructionWrapper
             val routeProgress = getRouteProgressFrom(
                 tripStatus.route,
                 tripStatus.navigationStatus,
                 remainingWaypoints,
-                bannerInstructionEvent.latestBannerInstructions,
-                bannerInstructionEvent.latestInstructionIndex,
+                latestBannerInstructionsWrapper?.latestBannerInstructions,
+                latestBannerInstructionsWrapper?.latestInstructionIndex,
                 lastVoiceInstruction
             ).also {
                 if (it == null) {
@@ -603,10 +614,26 @@ internal class MapboxTripSession(
         var legIndexUpdated = false
         updateLegIndexJob = mainJobController.scope.launch {
             try {
+                fun msg(state: String, append: String = ""): String =
+                    "update to new leg $state. Leg index: $legIndex, route id: " +
+                        "${primaryRoute?.id} + $append"
+
+                logD(LOG_CATEGORY, msg("started"))
+                val latestInstructionWrapper = bannerInstructionEvent.latestInstructionWrapper
+                val lastVoiceInstruction = lastVoiceInstruction
                 legIndexUpdated = navigator.updateLegIndex(legIndex)
                 if (legIndexUpdated) {
-                    invalidateLatestInstructions()
+                    invalidateLatestInstructions(latestInstructionWrapper, lastVoiceInstruction)
                 }
+                logD(
+                    msg(
+                        "finished",
+                        "(is leg updated: $legIndexUpdated; " +
+                            "latestInstructionWrapper: [$latestInstructionWrapper]; " +
+                            "lastVoiceInstruction: [$lastVoiceInstruction])"
+                    ),
+                    LOG_CATEGORY,
+                )
             } finally {
                 callback.onLegIndexUpdatedCallback(legIndexUpdated)
             }
@@ -713,9 +740,19 @@ internal class MapboxTripSession(
         }
     }
 
-    private fun invalidateLatestInstructions() {
-        bannerInstructionEvent.invalidateLatestBannerInstructions()
-        lastVoiceInstruction = null
+    /**
+     * Invalidate latest banner and voice instruction. To get the latest banner instruction wrapper call
+     * [BannerInstructionEvent.latestInstructionWrapper], to get the latest voice instruction
+     * call [lastVoiceInstruction]
+     */
+    private fun invalidateLatestInstructions(
+        latestInstructionWrapper: BannerInstructionEvent.LatestInstructionWrapper?,
+        voiceInstruction: VoiceInstructions?,
+    ) {
+        bannerInstructionEvent.invalidateLatestBannerInstructions(latestInstructionWrapper)
+        if (lastVoiceInstruction == voiceInstruction) {
+            lastVoiceInstruction = null
+        }
     }
 
     private fun checkBannerInstructionEvent(

libnavigation-core/src/test/java/com/mapbox/navigation/core/trip/session/BannerInstructionEventTest.kt

@@ -96,9 +96,31 @@ class BannerInstructionEventTest {
         val anyBannerInstructions: BannerInstructions = mockk()
         bannerInstructionEvent.isOccurring(anyBannerInstructions, 0)
 
-        bannerInstructionEvent.invalidateLatestBannerInstructions()
+        bannerInstructionEvent
+            .invalidateLatestBannerInstructions(bannerInstructionEvent.latestInstructionWrapper)
 
         assertNull(bannerInstructionEvent.latestBannerInstructions)
         assertNull(bannerInstructionEvent.latestInstructionIndex)
+        assertNull(bannerInstructionEvent.latestInstructionWrapper)
+    }
+
+    @Test
+    fun invalidateNonExistingLatestBannerInstructions() {
+        val bannerInstructionEvent = BannerInstructionEvent()
+        val anyBannerInstructions: BannerInstructions = mockk()
+        bannerInstructionEvent.isOccurring(anyBannerInstructions, 0)
+
+        bannerInstructionEvent
+            .invalidateLatestBannerInstructions(mockk())
+
+        assertEquals(
+            BannerInstructionEvent.LatestInstructionWrapper(0, anyBannerInstructions),
+            bannerInstructionEvent.latestInstructionWrapper
+        )
+        assertEquals(
+            anyBannerInstructions,
+            bannerInstructionEvent.latestBannerInstructions
+        )
+        assertEquals(0, bannerInstructionEvent.latestInstructionIndex,)
     }
 }

libnavigation-core/src/test/java/com/mapbox/navigation/core/trip/session/MapboxTripSessionTest.kt

@@ -47,6 +47,7 @@ import io.mockk.Runs
 import io.mockk.clearMocks
 import io.mockk.coEvery
 import io.mockk.coVerify
+import io.mockk.coVerifyOrder
 import io.mockk.every
 import io.mockk.just
 import io.mockk.mockk
@@ -130,6 +131,7 @@ class MapboxTripSessionTest {
     private val navigationStatus: NavigationStatus = mockk(relaxed = true)
     private val routeProgress: RouteProgress = mockk()
     private val threadController = spyk<ThreadController>()
+    private val bannerInstructionEvent = spyk(BannerInstructionEvent())
 
     private val parentJob = SupervisorJob()
     private val testScope = CoroutineScope(parentJob + coroutineRule.testDispatcher)
@@ -148,11 +150,13 @@ class MapboxTripSessionTest {
         mockkObject(MapboxNativeNavigatorImpl)
         mockkStatic("com.mapbox.navigation.core.navigator.NavigatorMapper")
         mockkStatic("com.mapbox.navigation.core.navigator.LocationEx")
+        mockkObject(BannerInstructionEvent.Companion)
         mockkObject(RoadObjectFactory)
         every { location.toFixLocation() } returns fixLocation
         every { fixLocation.toLocation() } returns location
         every { keyFixPoints.toLocations() } returns keyPoints
         every { threadController.getMainScopeAndRootJob() } returns JobControl(parentJob, testScope)
+        every { BannerInstructionEvent.invoke() } returns bannerInstructionEvent
         navigationOptions = NavigationOptions.Builder(context).build()
         tripSession = buildTripSession()
 
@@ -1634,12 +1638,45 @@ class MapboxTripSessionTest {
             }
         }
 
+    /*
+        navigator.updateLegIndex(legIndex) might force invoking NavigatorObserver and one adds
+        new latest banner instructions to BannerInstructionEvent. That's why requires pre-save
+        latest banner instructions to remove legacy instructions only
+     */
+    @Test
+    fun `updateLegIndex latest banner and voice instructions pre-save to later clean up them`() =
+        coroutineRule.runBlockingTest {
+            val idx = -1
+            val mockLatestInstructionWrapper =
+                mockk<BannerInstructionEvent.LatestInstructionWrapper>()
+            every {
+                bannerInstructionEvent.latestInstructionWrapper
+            } returns mockLatestInstructionWrapper
+            coEvery { navigator.updateLegIndex(idx) } coAnswers {
+                true
+            }
+            val mockVoiceInstruction = mockk<VoiceInstructions>()
+
+            tripSession.lastVoiceInstruction = mockVoiceInstruction
+            tripSession.updateLegIndex(idx, mockk(relaxUnitFun = true))
+
+            coVerifyOrder {
+                bannerInstructionEvent.latestInstructionWrapper
+                navigator.updateLegIndex(idx)
+                bannerInstructionEvent
+                    .invalidateLatestBannerInstructions(mockLatestInstructionWrapper)
+                tripSession.lastVoiceInstruction = null
+            }
+            assertNull(tripSession.lastVoiceInstruction)
+        }
+
     @After
     fun cleanUp() {
         unmockkObject(MapboxNativeNavigatorImpl)
         unmockkStatic("com.mapbox.navigation.core.navigator.NavigatorMapper")
         unmockkStatic("com.mapbox.navigation.core.navigator.LocationEx")
         unmockkObject(RoadObjectFactory)
+        unmockkObject(BannerInstructionEvent.Companion)
     }
 
     private fun mockLocation(): Location = mockk(relaxed = true)