NAVAND-552: meet code review

dzinad · dzinad · commit f071a471821c · 2023-01-31T14:20:10.000+03:00
diff --git a/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxSpeechApi.kt b/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxSpeechApi.kt
@@ -64,7 +64,7 @@ class MapboxSpeechApi @JvmOverloads constructor(
         consumer: MapboxNavigationConsumer<Expected<SpeechError, SpeechValue>>
     ) {
         mainJobController.scope.launch {
-            retrieveVoiceFile(voiceInstruction, consumer)
+            consumer.accept(retrieveVoiceFile(voiceInstruction))
         }
     }
 
@@ -138,19 +138,18 @@ class MapboxSpeechApi @JvmOverloads constructor(
             val typeAndAnnouncement = VoiceInstructionsParser.parse(instruction).value
             if (typeAndAnnouncement != null && !hasTypeAndAnnouncement(typeAndAnnouncement)) {
                 predownloadJobController.scope.launch {
-                    retrieveVoiceFile(instruction) {
-                        mainJobController.scope.launch(Dispatchers.Main.immediate) {
-                            it.onValue { speechValue ->
-                                cachedFiles[typeAndAnnouncement] = speechValue
-                            }
+                    val voiceFile = retrieveVoiceFile(instruction)
+                    mainJobController.scope.launch {
+                        voiceFile.onValue { speechValue ->
+                            cachedFiles[typeAndAnnouncement] = speechValue
                         }
                     }
                 }
             }
         }
     }
 
-    internal fun destroy() {
+    internal fun cancelPredownload() {
         predownloadJobController.job.children.forEach { it.cancel() }
         val announcements = cachedFiles.map { it.value.announcement }
         announcements.forEach { clean(it) }
@@ -159,28 +158,25 @@ class MapboxSpeechApi @JvmOverloads constructor(
     @Throws(IllegalStateException::class)
     private suspend fun retrieveVoiceFile(
         voiceInstruction: VoiceInstructions,
-        consumer: MapboxNavigationConsumer<Expected<SpeechError, SpeechValue>>,
-    ) {
+    ): Expected<SpeechError, SpeechValue> {
         when (val result = voiceAPI.retrieveVoiceFile(voiceInstruction)) {
             is VoiceState.VoiceFile -> {
                 val announcement = voiceInstruction.announcement()
                 val ssmlAnnouncement = voiceInstruction.ssmlAnnouncement()
-                consumer.accept(
-                    ExpectedFactory.createValue(
-                        SpeechValue(
-                            // Can't be null as it's checked in retrieveVoiceFile
-                            SpeechAnnouncement.Builder(announcement!!)
-                                .ssmlAnnouncement(ssmlAnnouncement)
-                                .file(result.instructionFile)
-                                .build()
-                        )
+                return ExpectedFactory.createValue(
+                    SpeechValue(
+                        // Can't be null as it's checked in retrieveVoiceFile
+                        SpeechAnnouncement.Builder(announcement!!)
+                            .ssmlAnnouncement(ssmlAnnouncement)
+                            .file(result.instructionFile)
+                            .build()
                     )
                 )
             }
             is VoiceState.VoiceError -> {
                 val fallback = getFallbackAnnouncement(voiceInstruction)
                 val speechError = SpeechError(result.exception, null, fallback)
-                consumer.accept(ExpectedFactory.createError(speechError))
+                return ExpectedFactory.createError(speechError)
             }
         }
     }
diff --git a/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/TimeBasedNextVoiceInstructionsProvider.kt b/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/TimeBasedNextVoiceInstructionsProvider.kt
@@ -1,5 +1,7 @@
 package com.mapbox.navigation.ui.voice.api
 
+import com.mapbox.api.directions.v5.models.LegStep
+import com.mapbox.api.directions.v5.models.RouteLeg
 import com.mapbox.api.directions.v5.models.VoiceInstructions
 import com.mapbox.navigation.utils.internal.logW
 
@@ -25,42 +27,63 @@ internal class TimeBasedNextVoiceInstructionsProvider(
             }
 
         val voiceInstructions = mutableListOf<VoiceInstructions>()
+        fillCurrentStepVoiceInstructions(
+            currentStep,
+            progress.stepDistanceRemaining,
+            voiceInstructions
+        )
         var cumulatedTime = progress.stepDurationRemaining
-        val currentStepInstructions = currentStep.voiceInstructions()?.filter { instruction ->
-            val distanceAlongGeometry = instruction.distanceAlongGeometry()
-            distanceAlongGeometry != null &&
-                distanceAlongGeometry <= progress.stepDistanceRemaining
-        }
-        if (currentStepInstructions != null) {
-            voiceInstructions.addAll(currentStepInstructions)
-        }
 
+        // fill next steps
         var currentStepIndex = progress.stepIndex
         var currentLegIndex = progress.legIndex
         while (cumulatedTime < observableTimeSeconds) {
-            if (currentStepIndex + 1 < (legSteps?.size ?: 0)) {
-                currentStep = legSteps!![currentStepIndex + 1]
-                currentStepIndex++
-            } else {
+            if (isLastStep(currentStepIndex, legSteps)) {
                 currentStepIndex = 0
-                if (currentLegIndex + 1 < legs.size) {
+                if (isLastLeg(currentLegIndex, legs)) {
+                    break
+                } else {
                     legSteps = legs[currentLegIndex + 1].steps()
                     currentLegIndex++
                     if (legSteps.isNullOrEmpty()) {
                         continue
                     } else {
                         currentStep = legSteps.first()
                     }
-                } else {
-                    break
                 }
+            } else {
+                currentStep = legSteps!![currentStepIndex + 1]
+                currentStepIndex++
             }
             currentStep.voiceInstructions()?.let { voiceInstructions.addAll(it) }
             cumulatedTime += currentStep.duration()
         }
         return voiceInstructions
     }
 
+    private fun fillCurrentStepVoiceInstructions(
+        currentStep: LegStep,
+        stepDistanceRemaining: Double,
+        voiceInstructions: MutableList<VoiceInstructions>
+    ) {
+        val currentStepInstructions = currentStep.voiceInstructions()?.filter { instruction ->
+            val distanceAlongGeometry = instruction.distanceAlongGeometry()
+            distanceAlongGeometry != null &&
+                distanceAlongGeometry <= stepDistanceRemaining
+        }
+        if (currentStepInstructions != null) {
+            voiceInstructions.addAll(currentStepInstructions)
+        }
+    }
+
+    private fun isLastStep(currentStepIndex: Int, legSteps: List<LegStep>?): Boolean {
+        return currentStepIndex + 1 >= (legSteps?.size ?: 0)
+    }
+
+    private fun isLastLeg(currentLegIndex: Int, legs: List<RouteLeg>): Boolean {
+        return currentLegIndex + 1 >= legs.size
+    }
+
     private companion object {
         private const val LOG_CATEGORY = "TimeBasedNextVoiceInstructionsProvider"
     }
diff --git a/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/VoiceInstructionsPrefetcher.kt b/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/VoiceInstructionsPrefetcher.kt
@@ -66,7 +66,7 @@ class VoiceInstructionsPrefetcher internal constructor(
         lastDownloadTime = 0
         mapboxNavigation.unregisterRoutesObserver(routesObserver)
         mapboxNavigation.unregisterRouteProgressObserver(routeProgressObserver)
-        speechApi.destroy()
+        speechApi.cancelPredownload()
     }
 
     private fun onRoutesChanged(result: RoutesUpdatedResult) {
diff --git a/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidanceTest.kt b/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidanceTest.kt
@@ -321,7 +321,7 @@ class MapboxAudioGuidanceTest {
         carAppAudioGuidance.onDetached(mapboxNavigation)
 
         verify(exactly = 1) {
-            speechApi.destroy()
+            speechApi.cancelPredownload()
         }
     }
 
diff --git a/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/api/MapboxSpeechApiTest.kt b/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/api/MapboxSpeechApiTest.kt
@@ -275,7 +275,7 @@ class MapboxSpeechApiTest {
         coEvery { voiceAPI.retrieveVoiceFile(instruction) } returns VoiceState.VoiceFile(file)
         val sut = MapboxSpeechApi(mockk(relaxed = true), "pk.123", Locale.US.language)
         sut.predownload(listOf(instruction))
-        sut.destroy()
+        sut.cancelPredownload()
 
         sut.generatePredownloaded(instruction, consumer)
 
@@ -505,7 +505,7 @@ class MapboxSpeechApiTest {
     fun `destroy with no instructions`() {
         val sut = MapboxSpeechApi(mockk(relaxed = true), "pk.123", Locale.US.language)
 
-        sut.destroy()
+        sut.cancelPredownload()
 
         coVerify(exactly = 0) { voiceAPI.clean(any()) }
     }
@@ -524,7 +524,7 @@ class MapboxSpeechApiTest {
         sut.predownload(listOf(instruction1, instruction2))
         clearMocks(voiceAPI, answers = false)
 
-        sut.destroy()
+        sut.cancelPredownload()
 
         coVerify(exactly = 1) {
             voiceAPI.clean(announcement1)
diff --git a/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/api/VoiceInstructionsPrefetcherTest.kt b/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/api/VoiceInstructionsPrefetcherTest.kt
@@ -85,7 +85,7 @@ class VoiceInstructionsPrefetcherTest {
         verify(exactly = 1) {
             mapboxNavigation.registerRouteProgressObserver(routeProgressObserverSlot.first())
             mapboxNavigation.registerRoutesObserver(routesObserverSlot.first())
-            speechAPI.destroy()
+            speechAPI.cancelPredownload()
         }
     }
 

Original file line number	Diff line number	Diff line change
`@@ -64,7 +64,7 @@ class MapboxSpeechApi @JvmOverloads constructor(`
`64`	`64`	`consumer: MapboxNavigationConsumer<Expected<SpeechError, SpeechValue>>`
`65`	`65`	`) {`
`66`	`66`	`mainJobController.scope.launch {`
`67`		`- retrieveVoiceFile(voiceInstruction, consumer)`
	`67`	`+ consumer.accept(retrieveVoiceFile(voiceInstruction))`
`68`	`68`	`}`
`69`	`69`	`}`
`70`	`70`
`@@ -138,19 +138,18 @@ class MapboxSpeechApi @JvmOverloads constructor(`
`138`	`138`	`val typeAndAnnouncement = VoiceInstructionsParser.parse(instruction).value`
`139`	`139`	`if (typeAndAnnouncement != null && !hasTypeAndAnnouncement(typeAndAnnouncement)) {`
`140`	`140`	`predownloadJobController.scope.launch {`
`141`		`- retrieveVoiceFile(instruction) {`
`142`		`- mainJobController.scope.launch(Dispatchers.Main.immediate) {`
`143`		`- it.onValue { speechValue ->`
`144`		`- cachedFiles[typeAndAnnouncement] = speechValue`
`145`		`- }`
	`141`	`+ val voiceFile = retrieveVoiceFile(instruction)`
	`142`	`+ mainJobController.scope.launch {`
	`143`	`+ voiceFile.onValue { speechValue ->`
	`144`	`+ cachedFiles[typeAndAnnouncement] = speechValue`
`146`	`145`	`}`
`147`	`146`	`}`
`148`	`147`	`}`
`149`	`148`	`}`
`150`	`149`	`}`
`151`	`150`	`}`
`152`	`151`
`153`		`- internal fun destroy() {`
	`152`	`+ internal fun cancelPredownload() {`
`154`	`153`	`predownloadJobController.job.children.forEach { it.cancel() }`
`155`	`154`	`val announcements = cachedFiles.map { it.value.announcement }`
`156`	`155`	`announcements.forEach { clean(it) }`
`@@ -159,28 +158,25 @@ class MapboxSpeechApi @JvmOverloads constructor(`
`159`	`158`	`@Throws(IllegalStateException::class)`
`160`	`159`	`private suspend fun retrieveVoiceFile(`
`161`	`160`	`voiceInstruction: VoiceInstructions,`
`162`		`- consumer: MapboxNavigationConsumer<Expected<SpeechError, SpeechValue>>,`
`163`		`- ) {`
	`161`	`+ ): Expected<SpeechError, SpeechValue> {`
`164`	`162`	`when (val result = voiceAPI.retrieveVoiceFile(voiceInstruction)) {`
`165`	`163`	`is VoiceState.VoiceFile -> {`
`166`	`164`	`val announcement = voiceInstruction.announcement()`
`167`	`165`	`val ssmlAnnouncement = voiceInstruction.ssmlAnnouncement()`
`168`		`- consumer.accept(`
`169`		`- ExpectedFactory.createValue(`
`170`		`- SpeechValue(`
`171`		`- // Can't be null as it's checked in retrieveVoiceFile`
`172`		`- SpeechAnnouncement.Builder(announcement!!)`
`173`		`- .ssmlAnnouncement(ssmlAnnouncement)`
`174`		`- .file(result.instructionFile)`
`175`		`- .build()`
`176`		`- )`
	`166`	`+ return ExpectedFactory.createValue(`
	`167`	`+ SpeechValue(`
	`168`	`+ // Can't be null as it's checked in retrieveVoiceFile`
	`169`	`+ SpeechAnnouncement.Builder(announcement!!)`
	`170`	`+ .ssmlAnnouncement(ssmlAnnouncement)`
	`171`	`+ .file(result.instructionFile)`
	`172`	`+ .build()`
`177`	`173`	`)`
`178`	`174`	`)`
`179`	`175`	`}`
`180`	`176`	`is VoiceState.VoiceError -> {`
`181`	`177`	`val fallback = getFallbackAnnouncement(voiceInstruction)`
`182`	`178`	`val speechError = SpeechError(result.exception, null, fallback)`
`183`		`- consumer.accept(ExpectedFactory.createError(speechError))`
	`179`	`+ return ExpectedFactory.createError(speechError)`
`184`	`180`	`}`
`185`	`181`	`}`
`186`	`182`	`}`
Original file line number	Diff line number	Diff line change
`@@ -66,7 +66,7 @@ class VoiceInstructionsPrefetcher internal constructor(`
`66`	`66`	`lastDownloadTime = 0`
`67`	`67`	`mapboxNavigation.unregisterRoutesObserver(routesObserver)`
`68`	`68`	`mapboxNavigation.unregisterRouteProgressObserver(routeProgressObserver)`
`69`		`- speechApi.destroy()`
	`69`	`+ speechApi.cancelPredownload()`
`70`	`70`	`}`
`71`	`71`
`72`	`72`	`private fun onRoutesChanged(result: RoutesUpdatedResult) {`
Original file line number	Diff line number	Diff line change
`@@ -321,7 +321,7 @@ class MapboxAudioGuidanceTest {`
`321`	`321`	`carAppAudioGuidance.onDetached(mapboxNavigation)`
`322`	`322`
`323`	`323`	`verify(exactly = 1) {`
`324`		`- speechApi.destroy()`
	`324`	`+ speechApi.cancelPredownload()`
`325`	`325`	`}`
`326`	`326`	`}`
`327`	`327`
Original file line number	Diff line number	Diff line change
`@@ -85,7 +85,7 @@ class VoiceInstructionsPrefetcherTest {`
`85`	`85`	`verify(exactly = 1) {`
`86`	`86`	`mapboxNavigation.registerRouteProgressObserver(routeProgressObserverSlot.first())`
`87`	`87`	`mapboxNavigation.registerRoutesObserver(routesObserverSlot.first())`
`88`		`- speechAPI.destroy()`
	`88`	`+ speechAPI.cancelPredownload()`
`89`	`89`	`}`
`90`	`90`	`}`
`91`	`91`