NAVAND-552: meet code review

Author: dzinad

examples/src/main/java/com/mapbox/navigation/examples/core/MapboxVoiceActivity.kt

@@ -81,6 +81,7 @@ import java.util.Locale
  * attention to its usage. Long press anywhere on the map to set a destination and trigger
  * navigation.
  */
+@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
 class MapboxVoiceActivity : AppCompatActivity(), OnMapLongClickListener {
 
     private var isMuted: Boolean = false
@@ -203,7 +204,6 @@ class MapboxVoiceActivity : AppCompatActivity(), OnMapLongClickListener {
         }
     }
 
-    @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
     private val voiceInstructionsPrefetcher by lazy {
         VoiceInstructionsPrefetcher(speechApi)
     }
@@ -389,43 +389,39 @@ class MapboxVoiceActivity : AppCompatActivity(), OnMapLongClickListener {
                 .build()
         )
         init()
+        voiceInstructionsPrefetcher.onAttached(mapboxNavigation)
     }
 
-    @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
     override fun onStart() {
         super.onStart()
         if (::mapboxNavigation.isInitialized) {
             mapboxNavigation.registerRoutesObserver(routesObserver)
             mapboxNavigation.registerLocationObserver(locationObserver)
-            voiceInstructionsPrefetcher.onAttached(mapboxNavigation)
             mapboxNavigation.registerRouteProgressObserver(routeProgressObserver)
             mapboxNavigation.registerRouteProgressObserver(replayProgressObserver)
             mapboxNavigation.registerVoiceInstructionsObserver(voiceInstructionsObserver)
         }
         ResourceLoaderFactory.getInstance().registerObserver(resourceLoadObserver)
     }
 
-    @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
     override fun onStop() {
         super.onStop()
         ResourceLoaderFactory.getInstance().unregisterObserver(resourceLoadObserver)
         mapboxNavigation.unregisterRoutesObserver(routesObserver)
-        voiceInstructionsPrefetcher.onDetached(mapboxNavigation)
         mapboxNavigation.unregisterLocationObserver(locationObserver)
         mapboxNavigation.unregisterRouteProgressObserver(routeProgressObserver)
         mapboxNavigation.unregisterRouteProgressObserver(replayProgressObserver)
         mapboxNavigation.unregisterVoiceInstructionsObserver(voiceInstructionsObserver)
     }
 
-    @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
     override fun onDestroy() {
         super.onDestroy()
         routeLineApi.cancel()
         routeLineView.cancel()
         mapboxReplayer.finish()
         mapboxNavigation.onDestroy()
         speechApi.cancel()
-        voiceInstructionsPrefetcher.destroy()
+        voiceInstructionsPrefetcher.onDetached(mapboxNavigation)
         voiceInstructionsPlayer.shutdown()
     }
 

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

@@ -2020,15 +2020,6 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
         }
     }
 
-    private class TestVoiceInstructionsTriggerObserver : RoutesObserver, RouteProgressObserver {
-
-        override fun onRouteProgressChanged(routeProgress: RouteProgress) {
-        }
-
-        override fun onRoutesChanged(result: RoutesUpdatedResult) {
-        }
-    }
-
     private fun alternativeWithId(mockId: String): RouteAlternative {
         val mockedRoute = mockk<RouteInterface> {
             every { routeId } returns mockId

libnavigation-util/src/test/java/com/mapbox/navigation/utils/internal/TimeTest.kt

@@ -64,7 +64,7 @@ package com.mapbox.navigation.ui.voice.api {
     method public void cancel();
     method public void clean(com.mapbox.navigation.ui.voice.model.SpeechAnnouncement announcement);
     method public void generate(com.mapbox.api.directions.v5.models.VoiceInstructions voiceInstruction, com.mapbox.navigation.ui.base.util.MapboxNavigationConsumer<com.mapbox.bindgen.Expected<com.mapbox.navigation.ui.voice.model.SpeechError,com.mapbox.navigation.ui.voice.model.SpeechValue>> consumer);
-    method public void generatePredownloaded(com.mapbox.api.directions.v5.models.VoiceInstructions voiceInstruction, com.mapbox.navigation.ui.base.util.MapboxNavigationConsumer<com.mapbox.bindgen.Expected<com.mapbox.navigation.ui.voice.model.SpeechError,com.mapbox.navigation.ui.voice.model.SpeechValue>> consumer);
+    method @com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI public void generatePredownloaded(com.mapbox.api.directions.v5.models.VoiceInstructions voiceInstruction, com.mapbox.navigation.ui.base.util.MapboxNavigationConsumer<com.mapbox.bindgen.Expected<com.mapbox.navigation.ui.voice.model.SpeechError,com.mapbox.navigation.ui.voice.model.SpeechValue>> consumer);
   }
 
   @UiThread public final class MapboxVoiceInstructionsPlayer {
@@ -95,8 +95,6 @@ package com.mapbox.navigation.ui.voice.api {
 
   @com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI public final class VoiceInstructionsPrefetcher implements com.mapbox.navigation.core.lifecycle.MapboxNavigationObserver {
     ctor public VoiceInstructionsPrefetcher(com.mapbox.navigation.ui.voice.api.MapboxSpeechApi speechApi);
-    ctor public VoiceInstructionsPrefetcher(int observableTime, double timePercentageToTriggerAfter, com.mapbox.navigation.ui.voice.api.MapboxSpeechApi speechApi);
-    method public void destroy();
     method public void onAttached(com.mapbox.navigation.core.MapboxNavigation mapboxNavigation);
     method public void onDetached(com.mapbox.navigation.core.MapboxNavigation mapboxNavigation);
     field public static final com.mapbox.navigation.ui.voice.api.VoiceInstructionsPrefetcher.Companion Companion;

libnavui-voice/api/current.txt

@@ -42,7 +42,8 @@ internal constructor(
 
     private var dataStoreOwner: NavigationDataStoreOwner? = null
     private var configOwner: NavigationConfigOwner? = null
-    private var audioGuidanceVoice: MapboxAudioGuidanceVoice? = null
+    @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
+    private var trigger: VoiceInstructionsPrefetcher? = null
     private var mutedStateFlow = MutableStateFlow(false)
     private val internalStateFlow = MutableStateFlow(MapboxAudioGuidanceState())
     private val scope = CoroutineScope(SupervisorJob() + dispatcher)
@@ -72,9 +73,10 @@ internal constructor(
     /**
      * @see [MapboxNavigationApp]
      */
+    @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
     override fun onDetached(mapboxNavigation: MapboxNavigation) {
         mapboxVoiceInstructions.unregisterObservers(mapboxNavigation)
-        audioGuidanceVoice?.destroy()
+        trigger?.onDetached(mapboxNavigation)
         job?.cancel()
         job = null
     }
@@ -165,17 +167,14 @@ internal constructor(
 
     @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
     private fun MapboxNavigation.audioGuidanceVoice(): Flow<MapboxAudioGuidanceVoice> {
-        var trigger: VoiceInstructionsPrefetcher? = null
         return combine(
             mapboxVoiceInstructions.voiceLanguage(),
             configOwner!!.language(),
         ) { voiceLanguage, deviceLanguage -> voiceLanguage ?: deviceLanguage }
             .distinctUntilChanged()
             .map { language ->
-                audioGuidanceVoice?.destroy()
                 trigger?.onDetached(this)
                 audioGuidanceServices.mapboxAudioGuidanceVoice(this, language).also {
-                    audioGuidanceVoice = it
                     trigger = VoiceInstructionsPrefetcher(it.mapboxSpeechApi).also { trigger ->
                         trigger.onAttached(this)
                     }

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

@@ -1,9 +1,11 @@
 package com.mapbox.navigation.ui.voice.api
 
 import android.content.Context
+import androidx.annotation.UiThread
 import com.mapbox.api.directions.v5.models.VoiceInstructions
 import com.mapbox.bindgen.Expected
 import com.mapbox.bindgen.ExpectedFactory
+import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
 import com.mapbox.navigation.core.trip.session.VoiceInstructionsObserver
 import com.mapbox.navigation.ui.base.util.MapboxNavigationConsumer
 import com.mapbox.navigation.ui.voice.model.SpeechAnnouncement
@@ -13,6 +15,7 @@ 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
@@ -31,11 +34,10 @@ class MapboxSpeechApi @JvmOverloads constructor(
     private val options: MapboxSpeechApiOptions = MapboxSpeechApiOptions.Builder().build()
 ) {
 
-    private val cachedFilesLock = Any()
     private val cachedFiles = mutableMapOf<TypeAndAnnouncement, SpeechValue>()
     private val mainJobController by lazy { InternalJobControlFactory.createMainScopeJobControl() }
-    private val predownloadScope by lazy {
-        InternalJobControlFactory.createDefaultScopeJobControl().scope
+    private val predownloadJobController by lazy {
+        InternalJobControlFactory.createDefaultScopeJobControl()
     }
     private val voiceAPI = VoiceApiProvider.retrieveMapboxVoiceApi(
         context,
@@ -80,6 +82,7 @@ class MapboxSpeechApi @JvmOverloads constructor(
      * with the raw announcement (without file) that can be played with a text-to-speech engine.
      * @see [cancel]
      */
+    @ExperimentalPreviewMapboxNavigationAPI
     fun generatePredownloaded(
         voiceInstruction: VoiceInstructions,
         consumer: MapboxNavigationConsumer<Expected<SpeechError, SpeechValue>>
@@ -119,29 +122,26 @@ class MapboxSpeechApi @JvmOverloads constructor(
     fun clean(announcement: SpeechAnnouncement) {
         voiceAPI.clean(announcement)
         VoiceInstructionsParser.parse(announcement).onValue {
-            synchronized(cachedFilesLock) {
-                val value = cachedFiles[it]
-                // when we clear fallback announcement, there is a chance we will remove the key
-                // from map and not remove the file itself
-                // since for fallback SpeechAnnouncement file is null
-                if (value?.announcement == announcement) {
-                    cachedFiles.remove(it)
-                }
+            val value = cachedFiles[it]
+            // when we clear fallback announcement, there is a chance we will remove the key
+            // from map and not remove the file itself
+            // since for fallback SpeechAnnouncement file is null
+            if (value?.announcement == announcement) {
+                cachedFiles.remove(it)
             }
         }
     }
 
+    @UiThread
     internal fun predownload(instructions: List<VoiceInstructions>) {
-        mainJobController.scope.launch {
-            instructions.forEach { instruction ->
-                val typeAndAnnouncement = VoiceInstructionsParser.parse(instruction).value
-                if (typeAndAnnouncement != null && !hasTypeAndAnnouncement(typeAndAnnouncement)) {
-                    predownloadScope.launch {
-                        retrieveVoiceFile(instruction) {
+        instructions.forEach { instruction ->
+            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 ->
-                                synchronized(cachedFilesLock) {
-                                    cachedFiles[typeAndAnnouncement] = speechValue
-                                }
+                                cachedFiles[typeAndAnnouncement] = speechValue
                             }
                         }
                     }
@@ -151,11 +151,9 @@ class MapboxSpeechApi @JvmOverloads constructor(
     }
 
     internal fun destroy() {
-        predownloadScope.cancel()
-        synchronized(cachedFilesLock) {
-            val announcements = cachedFiles.map { it.value.announcement }
-            announcements.forEach { clean(it) }
-        }
+        predownloadJobController.job.children.forEach { it.cancel() }
+        val announcements = cachedFiles.map { it.value.announcement }
+        announcements.forEach { clean(it) }
     }
 
     @Throws(IllegalStateException::class)
@@ -202,15 +200,11 @@ class MapboxSpeechApi @JvmOverloads constructor(
     }
 
     private fun hasTypeAndAnnouncement(typeAndAnnouncement: TypeAndAnnouncement): Boolean {
-        synchronized(cachedFilesLock) {
-            return typeAndAnnouncement in cachedFiles
-        }
+        return typeAndAnnouncement in cachedFiles
     }
 
     private fun getFromCache(voiceInstruction: VoiceInstructions): SpeechValue? {
         val key = VoiceInstructionsParser.parse(voiceInstruction).value
-        synchronized(cachedFilesLock) {
-            return key?.let { cachedFiles[it] }
-        }
+        return key?.let { cachedFiles[it] }
     }
 }

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

@@ -21,46 +21,23 @@ import com.mapbox.navigation.utils.internal.ifNonNull
  */
 @ExperimentalPreviewMapboxNavigationAPI
 class VoiceInstructionsPrefetcher internal constructor(
+    private val speechApi: MapboxSpeechApi,
     private val observableTime: Int,
     private val timePercentageToTriggerAfter: Double,
-    private val speechApi: MapboxSpeechApi,
-    private val nextVoiceInstructionsProvider: NextVoiceInstructionsProvider,
-    private val timeProvider: Time,
+    private val nextVoiceInstructionsProvider: NextVoiceInstructionsProvider =
+        TimeBasedNextVoiceInstructionsProvider(observableTime),
+    private val timeProvider: Time = Time.SystemImpl,
 ) : MapboxNavigationObserver {
 
     /**
-     * Creates [VoiceInstructionsPrefetcher] with default
-     * observableTime and timePercentageToTriggerAfter.
-     * See [DEFAULT_OBSERVABLE_TIME_SECONDS] and [DEFAULT_TIME_PERCENTAGE_TO_TRIGGER_AFTER].
+     * Creates [VoiceInstructionsPrefetcher.
      *
      * @param speechApi [MapboxSpeechApi] instances that's used to generate instructions
      */
     constructor(speechApi: MapboxSpeechApi) : this(
-        DEFAULT_OBSERVABLE_TIME_SECONDS,
-        DEFAULT_TIME_PERCENTAGE_TO_TRIGGER_AFTER,
-        speechApi
-    )
-
-    /**
-     * Creates [VoiceInstructionsPrefetcher] with custom
-     * observableTime and timePercentageToTriggerAfter.
-     *
-     * @param observableTime voice instructions will be predownloaded for `observableTime` seconds
-     *  of route ahead
-     * @param timePercentageToTriggerAfter voice instructions predownloading will not be triggered
-     *  if `timePercentageToTriggerAfter` seconds did not pass since last predownloading session
-     * @param speechApi [MapboxSpeechApi] instances that's used to generate instructions
-     */
-    constructor(
-        observableTime: Int,
-        timePercentageToTriggerAfter: Double,
-        speechApi: MapboxSpeechApi,
-    ) : this(
-        observableTime,
-        timePercentageToTriggerAfter,
         speechApi,
-        TimeBasedNextVoiceInstructionsProvider(observableTime),
-        Time.SystemImpl
+        DEFAULT_OBSERVABLE_TIME_SECONDS,
+        DEFAULT_TIME_PERCENTAGE_TO_TRIGGER_AFTER
     )
 
     private val routesObserver = RoutesObserver { onRoutesChanged(it) }
@@ -88,6 +65,7 @@ class VoiceInstructionsPrefetcher internal constructor(
     override fun onDetached(mapboxNavigation: MapboxNavigation) {
         mapboxNavigation.unregisterRoutesObserver(routesObserver)
         mapboxNavigation.unregisterRouteProgressObserver(routeProgressObserver)
+        speechApi.destroy()
     }
 
     private fun onRoutesChanged(result: RoutesUpdatedResult) {
@@ -134,14 +112,6 @@ class VoiceInstructionsPrefetcher internal constructor(
         }
     }
 
-    /**
-     * The method stops all work related to pre-downloading voice instructions and unregisters
-     * all related callbacks. It should be invoked from `Activity#onDestroy`.
-     */
-    fun destroy() {
-        speechApi.destroy()
-    }
-
     private fun shouldDownloadBasedOnTime(): Boolean {
         return timeProvider.seconds() >=
             lastDownloadTime + observableTime * timePercentageToTriggerAfter

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

@@ -1,6 +1,7 @@
 package com.mapbox.navigation.ui.voice.internal
 
 import com.mapbox.api.directions.v5.models.VoiceInstructions
+import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
 import com.mapbox.navigation.ui.voice.api.MapboxSpeechApi
 import com.mapbox.navigation.ui.voice.api.MapboxVoiceInstructionsPlayer
 import com.mapbox.navigation.ui.voice.model.SpeechAnnouncement
@@ -41,10 +42,7 @@ class MapboxAudioGuidanceVoice(
         }
     }
 
-    fun destroy() {
-        mapboxSpeechApi.destroy()
-    }
-
+    @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
     private suspend fun MapboxSpeechApi.generate(
         instructions: VoiceInstructions
     ): SpeechAnnouncement = suspendCancellableCoroutine { cont ->

libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/internal/MapboxAudioGuidanceVoice.kt

@@ -36,7 +36,7 @@ class TestMapboxAudioGuidanceServices(
 
     private val mapboxSpeechApi = mockk<MapboxSpeechApi>(relaxed = true)
 
-    val mapboxAudioGuidanceVoice = mockk<MapboxAudioGuidanceVoice> {
+    val mapboxAudioGuidanceVoice = mockk<MapboxAudioGuidanceVoice>(relaxed = true) {
         coEvery { speak(any()) } coAnswers {
             val voiceInstructions = firstArg<VoiceInstructions?>()
             val speechAnnouncement: SpeechAnnouncement? = voiceInstructions?.let {