NAVAND-552: meet code review

Author: dzinad

libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxSpeechApi.kt

@@ -15,7 +15,6 @@ import com.mapbox.navigation.ui.voice.model.TypeAndAnnouncement
 import com.mapbox.navigation.ui.voice.model.VoiceState
 import com.mapbox.navigation.ui.voice.options.MapboxSpeechApiOptions
 import com.mapbox.navigation.utils.internal.InternalJobControlFactory
-import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.cancel
 import kotlinx.coroutines.launch
 import java.util.Locale
@@ -64,7 +63,7 @@ class MapboxSpeechApi @JvmOverloads constructor(
         consumer: MapboxNavigationConsumer<Expected<SpeechError, SpeechValue>>
     ) {
         mainJobController.scope.launch {
-            retrieveVoiceFile(voiceInstruction, consumer)
+            consumer.accept(retrieveVoiceFile(voiceInstruction))
         }
     }
 
@@ -138,19 +137,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 +157,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)
             }
         }
     }

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"
     }

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) {

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()
         }
     }
 

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)

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()
         }
     }