Skip to content

Commit 98f5542

Browse files
RingerJKdzinad
RingerJK
authored and
dzinad
committed
2.8 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
1 parent 1d29182 commit 98f5542

5 files changed

Lines changed: 140 additions & 18 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ Mapbox welcomes participation and contributions from everyone.
66
#### Features
77

88
#### Bug fixes and improvements
9+
- Fixed an issue where `RouteProgress#BannerInstructions` could've become `null` when `MapboxNavigation#updateLegIndex` was called. [#6716](https://github.com/mapbox/mapbox-navigation-android/pull/6716)
10+
- Fixed an issue where `RouteProgress#VoiceInstructions` could've become `null` when `MapboxNavigation#updateLegIndex` was called. [#6716](https://github.com/mapbox/mapbox-navigation-android/pull/6716)
911

1012
## Mapbox Navigation SDK 2.8.0 - 29 September, 2022
1113
### Changelog

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

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,58 @@
11
package com.mapbox.navigation.core.trip.session
22

33
import com.mapbox.api.directions.v5.models.BannerInstructions
4+
import com.mapbox.navigation.utils.internal.ifNonNull
45

5-
internal class BannerInstructionEvent {
6+
internal class BannerInstructionEvent private constructor() {
67

7-
var latestInstructionIndex: Int? = null
8+
var latestInstructionWrapper: LatestInstructionWrapper? = null
89
private set
910

11+
val latestInstructionIndex: Int?
12+
get() = latestInstructionWrapper?.latestInstructionIndex
13+
14+
val latestBannerInstructions: BannerInstructions?
15+
get() = latestInstructionWrapper?.latestBannerInstructions
16+
1017
var bannerInstructions: BannerInstructions? = null
1118
private set
1219

13-
var latestBannerInstructions: BannerInstructions? = null
14-
private set
20+
companion object {
21+
operator fun invoke(): BannerInstructionEvent = BannerInstructionEvent()
22+
}
1523

1624
fun isOccurring(bannerInstructions: BannerInstructions?, instructionIndex: Int?): Boolean {
1725
return updateCurrentBanner(bannerInstructions, instructionIndex)
1826
}
1927

20-
fun invalidateLatestBannerInstructions() {
21-
latestBannerInstructions = null
22-
latestInstructionIndex = null
28+
fun invalidateLatestBannerInstructions(latestInstructionWrapper: LatestInstructionWrapper?) {
29+
if (latestInstructionWrapper == this.latestInstructionWrapper) {
30+
this.latestInstructionWrapper = null
31+
}
2332
}
2433

2534
private fun updateCurrentBanner(banner: BannerInstructions?, instructionIndex: Int?): Boolean {
2635
bannerInstructions = banner
2736
if (bannerInstructions != null && bannerInstructions!! != latestBannerInstructions) {
28-
latestBannerInstructions = bannerInstructions
29-
latestInstructionIndex = instructionIndex
37+
latestInstructionWrapper =
38+
LatestInstructionWrapper.createOrNull(instructionIndex, bannerInstructions)
3039
return true
3140
}
3241
return false
3342
}
43+
44+
data class LatestInstructionWrapper(
45+
val latestInstructionIndex: Int,
46+
val latestBannerInstructions: BannerInstructions
47+
) {
48+
companion object {
49+
fun createOrNull(
50+
latestInstructionIndex: Int?,
51+
latestBannerInstructions: BannerInstructions?
52+
): LatestInstructionWrapper? =
53+
ifNonNull(latestInstructionIndex, latestBannerInstructions) { idx, instruction ->
54+
LatestInstructionWrapper(idx, instruction)
55+
}
56+
}
57+
}
3458
}

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

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,10 @@ internal class MapboxTripSession(
155155
this@MapboxTripSession.primaryRoute = newPrimaryRoute
156156
roadObjects = newPrimaryRoute?.upcomingRoadObjects ?: emptyList()
157157
isOffRoute = false
158-
invalidateLatestInstructions()
158+
invalidateLatestInstructions(
159+
bannerInstructionEvent.latestInstructionWrapper,
160+
lastVoiceInstruction
161+
)
159162
routeProgress = null
160163
}.mapValue {
161164
it.alternatives
@@ -180,7 +183,9 @@ internal class MapboxTripSession(
180183
private val fallbackVersionsObservers = CopyOnWriteArraySet<FallbackVersionsObserver>()
181184

182185
private val bannerInstructionEvent = BannerInstructionEvent()
183-
private var lastVoiceInstruction: VoiceInstructions? = null
186+
187+
@VisibleForTesting
188+
internal var lastVoiceInstruction: VoiceInstructions? = null
184189

185190
private var state: TripSessionState = TripSessionState.STOPPED
186191
set(value) {
@@ -316,6 +321,11 @@ internal class MapboxTripSession(
316321
"state: ${status.routeState}",
317322
LOG_CATEGORY
318323
)
324+
logD(
325+
"navigatorObserver#onStatus; banner instruction: [${status.bannerInstruction}]," +
326+
" voice instruction: [${status.voiceInstruction}]",
327+
LOG_CATEGORY
328+
)
319329

320330
val tripStatus = status.getTripStatusFrom(primaryRoute)
321331
val enhancedLocation = tripStatus.navigationStatus.location.toLocation()
@@ -344,12 +354,13 @@ internal class MapboxTripSession(
344354
)
345355
}
346356
val remainingWaypoints = calculateRemainingWaypoints(tripStatus)
357+
val latestBannerInstructionsWrapper = bannerInstructionEvent.latestInstructionWrapper
347358
val routeProgress = getRouteProgressFrom(
348359
tripStatus.route,
349360
tripStatus.navigationStatus,
350361
remainingWaypoints,
351-
bannerInstructionEvent.latestBannerInstructions,
352-
bannerInstructionEvent.latestInstructionIndex,
362+
latestBannerInstructionsWrapper?.latestBannerInstructions,
363+
latestBannerInstructionsWrapper?.latestInstructionIndex,
353364
lastVoiceInstruction
354365
).also {
355366
if (it == null) {
@@ -589,10 +600,26 @@ internal class MapboxTripSession(
589600
var legIndexUpdated = false
590601
updateLegIndexJob = mainJobController.scope.launch {
591602
try {
603+
fun msg(state: String, append: String = ""): String =
604+
"update to new leg $state. Leg index: $legIndex, route id: " +
605+
"${primaryRoute?.id} + $append"
606+
607+
logD(LOG_CATEGORY, msg("started"))
608+
val latestInstructionWrapper = bannerInstructionEvent.latestInstructionWrapper
609+
val lastVoiceInstruction = lastVoiceInstruction
592610
legIndexUpdated = navigator.updateLegIndex(legIndex)
593611
if (legIndexUpdated) {
594-
invalidateLatestInstructions()
612+
invalidateLatestInstructions(latestInstructionWrapper, lastVoiceInstruction)
595613
}
614+
logD(
615+
msg(
616+
"finished",
617+
"(is leg updated: $legIndexUpdated; " +
618+
"latestInstructionWrapper: [$latestInstructionWrapper]; " +
619+
"lastVoiceInstruction: [$lastVoiceInstruction])"
620+
),
621+
LOG_CATEGORY,
622+
)
596623
} finally {
597624
callback.onLegIndexUpdatedCallback(legIndexUpdated)
598625
}
@@ -699,9 +726,19 @@ internal class MapboxTripSession(
699726
}
700727
}
701728

702-
private fun invalidateLatestInstructions() {
703-
bannerInstructionEvent.invalidateLatestBannerInstructions()
704-
lastVoiceInstruction = null
729+
/**
730+
* Invalidate latest banner and voice instruction. To get the latest banner instruction wrapper call
731+
* [BannerInstructionEvent.latestInstructionWrapper], to get the latest voice instruction
732+
* call [lastVoiceInstruction]
733+
*/
734+
private fun invalidateLatestInstructions(
735+
latestInstructionWrapper: BannerInstructionEvent.LatestInstructionWrapper?,
736+
voiceInstruction: VoiceInstructions?,
737+
) {
738+
bannerInstructionEvent.invalidateLatestBannerInstructions(latestInstructionWrapper)
739+
if (lastVoiceInstruction == voiceInstruction) {
740+
lastVoiceInstruction = null
741+
}
705742
}
706743

707744
private fun checkBannerInstructionEvent(

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,31 @@ class BannerInstructionEventTest {
9696
val anyBannerInstructions: BannerInstructions = mockk()
9797
bannerInstructionEvent.isOccurring(anyBannerInstructions, 0)
9898

99-
bannerInstructionEvent.invalidateLatestBannerInstructions()
99+
bannerInstructionEvent
100+
.invalidateLatestBannerInstructions(bannerInstructionEvent.latestInstructionWrapper)
100101

101102
assertNull(bannerInstructionEvent.latestBannerInstructions)
102103
assertNull(bannerInstructionEvent.latestInstructionIndex)
104+
assertNull(bannerInstructionEvent.latestInstructionWrapper)
105+
}
106+
107+
@Test
108+
fun invalidateNonExistingLatestBannerInstructions() {
109+
val bannerInstructionEvent = BannerInstructionEvent()
110+
val anyBannerInstructions: BannerInstructions = mockk()
111+
bannerInstructionEvent.isOccurring(anyBannerInstructions, 0)
112+
113+
bannerInstructionEvent
114+
.invalidateLatestBannerInstructions(mockk())
115+
116+
assertEquals(
117+
BannerInstructionEvent.LatestInstructionWrapper(0, anyBannerInstructions),
118+
bannerInstructionEvent.latestInstructionWrapper
119+
)
120+
assertEquals(
121+
anyBannerInstructions,
122+
bannerInstructionEvent.latestBannerInstructions
123+
)
124+
assertEquals(0, bannerInstructionEvent.latestInstructionIndex,)
103125
}
104126
}

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ import io.mockk.Runs
4646
import io.mockk.clearMocks
4747
import io.mockk.coEvery
4848
import io.mockk.coVerify
49+
import io.mockk.coVerifyOrder
4950
import io.mockk.every
5051
import io.mockk.just
5152
import io.mockk.mockk
@@ -126,6 +127,7 @@ class MapboxTripSessionTest {
126127
private val navigationStatus: NavigationStatus = mockk(relaxed = true)
127128
private val routeProgress: RouteProgress = mockk()
128129
private val threadController = spyk<ThreadController>()
130+
private val bannerInstructionEvent = spyk(BannerInstructionEvent())
129131

130132
private val parentJob = SupervisorJob()
131133
private val testScope = CoroutineScope(parentJob + coroutineRule.testDispatcher)
@@ -144,11 +146,13 @@ class MapboxTripSessionTest {
144146
mockkObject(MapboxNativeNavigatorImpl)
145147
mockkStatic("com.mapbox.navigation.core.navigator.NavigatorMapper")
146148
mockkStatic("com.mapbox.navigation.core.navigator.LocationEx")
149+
mockkObject(BannerInstructionEvent.Companion)
147150
mockkObject(RoadObjectFactory)
148151
every { location.toFixLocation() } returns fixLocation
149152
every { fixLocation.toLocation() } returns location
150153
every { keyFixPoints.toLocations() } returns keyPoints
151154
every { threadController.getMainScopeAndRootJob() } returns JobControl(parentJob, testScope)
155+
every { BannerInstructionEvent.invoke() } returns bannerInstructionEvent
152156
navigationOptions = NavigationOptions.Builder(context).build()
153157
tripSession = buildTripSession()
154158

@@ -1495,12 +1499,45 @@ class MapboxTripSessionTest {
14951499
}
14961500
}
14971501

1502+
/*
1503+
navigator.updateLegIndex(legIndex) might force invoking NavigatorObserver and one adds
1504+
new latest banner instructions to BannerInstructionEvent. That's why requires pre-save
1505+
latest banner instructions to remove legacy instructions only
1506+
*/
1507+
@Test
1508+
fun `updateLegIndex latest banner and voice instructions pre-save to later clean up them`() =
1509+
coroutineRule.runBlockingTest {
1510+
val idx = -1
1511+
val mockLatestInstructionWrapper =
1512+
mockk<BannerInstructionEvent.LatestInstructionWrapper>()
1513+
every {
1514+
bannerInstructionEvent.latestInstructionWrapper
1515+
} returns mockLatestInstructionWrapper
1516+
coEvery { navigator.updateLegIndex(idx) } coAnswers {
1517+
true
1518+
}
1519+
val mockVoiceInstruction = mockk<VoiceInstructions>()
1520+
1521+
tripSession.lastVoiceInstruction = mockVoiceInstruction
1522+
tripSession.updateLegIndex(idx, mockk(relaxUnitFun = true))
1523+
1524+
coVerifyOrder {
1525+
bannerInstructionEvent.latestInstructionWrapper
1526+
navigator.updateLegIndex(idx)
1527+
bannerInstructionEvent
1528+
.invalidateLatestBannerInstructions(mockLatestInstructionWrapper)
1529+
tripSession.lastVoiceInstruction = null
1530+
}
1531+
assertNull(tripSession.lastVoiceInstruction)
1532+
}
1533+
14981534
@After
14991535
fun cleanUp() {
15001536
unmockkObject(MapboxNativeNavigatorImpl)
15011537
unmockkStatic("com.mapbox.navigation.core.navigator.NavigatorMapper")
15021538
unmockkStatic("com.mapbox.navigation.core.navigator.LocationEx")
15031539
unmockkObject(RoadObjectFactory)
1540+
unmockkObject(BannerInstructionEvent.Companion)
15041541
}
15051542

15061543
private fun mockLocation(): Location = mockk(relaxed = true)

0 commit comments

Comments
 (0)