NAVAND-552: predownload voice instructions

Author: dzinad

CHANGELOG.md

@@ -37,6 +37,8 @@ Mapbox welcomes participation and contributions from everyone.
 - Fixed an issue with `NavigationView` that caused road label position to not update in some cases. [#6531](https://github.com/mapbox/mapbox-navigation-android/pull/6531)
 - Fixed an issue where `DirectionsResponse#waypoints` list was cleared after a successful non-EV route refresh. [#6539](https://github.com/mapbox/mapbox-navigation-android/pull/6539)
 - Fixed an issue with EV route refresh failing in cases where EV data updates are not provided. Now, the initial parameters from a route request will be used as a fallback. [#6534](https://github.com/mapbox/mapbox-navigation-android/pull/6534)
+- Optimized voice instructions downloading. [#6547](https://github.com/mapbox/mapbox-navigation-android/pull/6547)
+- Fixed an issue where voice instruction was being played too late with low connectivity. [#6547](https://github.com/mapbox/mapbox-navigation-android/pull/6547)
 
 ## Mapbox Navigation SDK 2.10.0-alpha.1 - 28 October, 2022
 ### Changelog

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

@@ -456,6 +456,7 @@ class MapboxNavigationActivity : AppCompatActivity() {
         mapboxNavigation.onDestroy()
         maneuverApi.cancel()
         speechAPI.cancel()
+        speechAPI.destroy()
         voiceInstructionsPlayer.shutdown()
     }
 

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

@@ -410,6 +410,7 @@ class MapboxVoiceActivity : AppCompatActivity(), OnMapLongClickListener {
         mapboxReplayer.finish()
         mapboxNavigation.onDestroy()
         speechApi.cancel()
+        speechApi.destroy()
         voiceInstructionsPlayer.shutdown()
     }
 

libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt

@@ -54,6 +54,7 @@ import com.mapbox.navigation.core.directions.session.RoutesExtra
 import com.mapbox.navigation.core.directions.session.RoutesObserver
 import com.mapbox.navigation.core.history.MapboxHistoryReader
 import com.mapbox.navigation.core.history.MapboxHistoryRecorder
+import com.mapbox.navigation.core.internal.LegacyMapboxNavigationInstanceHolder
 import com.mapbox.navigation.core.internal.ReachabilityService
 import com.mapbox.navigation.core.internal.telemetry.CustomEvent
 import com.mapbox.navigation.core.internal.telemetry.UserFeedbackCallback
@@ -392,7 +393,7 @@ class MapboxNavigation @VisibleForTesting internal constructor(
         private set
 
     init {
-        if (hasInstance) {
+        if (LegacyMapboxNavigationInstanceHolder.peek() != null) {
             throw IllegalStateException(
                 """
                     A different MapboxNavigation instance already exists.
@@ -401,7 +402,6 @@ class MapboxNavigation @VisibleForTesting internal constructor(
                 """.trimIndent()
             )
         }
-        hasInstance = true
 
         val config = NavigatorLoader.createConfig(
             navigationOptions.deviceProfile,
@@ -567,6 +567,7 @@ class MapboxNavigation @VisibleForTesting internal constructor(
             navigator.cache
         )
         roadObjectMatcher = RoadObjectMatcher(navigator)
+        LegacyMapboxNavigationInstanceHolder.onCreated(this)
     }
 
     /**
@@ -1064,7 +1065,7 @@ class MapboxNavigation @VisibleForTesting internal constructor(
         routeRefreshController.unregisterAllRouteRefreshStateObservers()
 
         isDestroyed = true
-        hasInstance = false
+        LegacyMapboxNavigationInstanceHolder.onDestroyed()
     }
 
     /**
@@ -1910,9 +1911,6 @@ class MapboxNavigation @VisibleForTesting internal constructor(
 
     private companion object {
 
-        @Volatile
-        private var hasInstance = false
-
         private const val LOG_CATEGORY = "MapboxNavigation"
         private const val USER_AGENT: String = "MapboxNavigationNative"
         private const val THREADS_COUNT = 2

libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigationProvider.kt

@@ -2,6 +2,7 @@ package com.mapbox.navigation.core
 
 import androidx.annotation.UiThread
 import com.mapbox.navigation.base.options.NavigationOptions
+import com.mapbox.navigation.core.internal.LegacyMapboxNavigationInstanceHolder
 
 /**
  * Singleton responsible for ensuring there is only one MapboxNavigation instance.
@@ -11,8 +12,6 @@ import com.mapbox.navigation.base.options.NavigationOptions
     message = "Use MapboxNavigationApp to attach MapboxNavigation to lifecycles."
 )
 object MapboxNavigationProvider {
-    @Volatile
-    private var mapboxNavigation: MapboxNavigation? = null
 
     /**
      * Create MapboxNavigation with provided options.
@@ -25,12 +24,8 @@ object MapboxNavigationProvider {
         message = "Set the navigation options with MapboxNavigationApp.setup"
     )
     fun create(navigationOptions: NavigationOptions): MapboxNavigation {
-        mapboxNavigation?.onDestroy()
-        mapboxNavigation = MapboxNavigation(
-            navigationOptions
-        )
-
-        return mapboxNavigation!!
+        LegacyMapboxNavigationInstanceHolder.peek()?.onDestroy()
+        return MapboxNavigation(navigationOptions)
     }
 
     /**
@@ -48,7 +43,7 @@ object MapboxNavigationProvider {
             throw RuntimeException("Need to create MapboxNavigation before using it.")
         }
 
-        return mapboxNavigation!!
+        return LegacyMapboxNavigationInstanceHolder.peek()!!
     }
 
     /**
@@ -59,15 +54,14 @@ object MapboxNavigationProvider {
         message = "MapboxNavigationApp will determine when to destroy MapboxNavigation instances"
     )
     fun destroy() {
-        mapboxNavigation?.onDestroy()
-        mapboxNavigation = null
+        LegacyMapboxNavigationInstanceHolder.peek()?.onDestroy()
     }
 
     /**
      * Check if MapboxNavigation is created.
      */
     @JvmStatic
     fun isCreated(): Boolean {
-        return mapboxNavigation?.isDestroyed == false
+        return LegacyMapboxNavigationInstanceHolder.peek()?.isDestroyed == false
     }
 }

libnavigation-core/src/main/java/com/mapbox/navigation/core/internal/LegacyMapboxNavigationInstanceHolder.kt

@@ -0,0 +1,46 @@
+package com.mapbox.navigation.core.internal
+
+import androidx.annotation.UiThread
+import androidx.annotation.VisibleForTesting
+import com.mapbox.navigation.core.MapboxNavigation
+import java.util.concurrent.CopyOnWriteArraySet
+
+fun interface MapboxNavigationCreateObserver {
+
+    fun onCreated(mapboxNavigation: MapboxNavigation)
+}
+
+@Deprecated("Used to keep track of MapboxNavigation instances created via deprecated methods")
+@UiThread
+object LegacyMapboxNavigationInstanceHolder {
+
+    @Volatile
+    private var mapboxNavigation: MapboxNavigation? = null
+
+    private val createObservers = CopyOnWriteArraySet<MapboxNavigationCreateObserver>()
+
+    fun onCreated(instance: MapboxNavigation) {
+        mapboxNavigation = instance
+        createObservers.forEach { it.onCreated(instance) }
+    }
+
+    fun onDestroyed() {
+        mapboxNavigation = null
+    }
+
+    fun peek(): MapboxNavigation? = mapboxNavigation
+
+    fun registerCreateObserver(observer: MapboxNavigationCreateObserver) {
+        createObservers.add(observer)
+        mapboxNavigation?.let { observer.onCreated(it) }
+    }
+
+    fun unregisterCreateObserver(observer: MapboxNavigationCreateObserver) {
+        createObservers.remove(observer)
+    }
+
+    @VisibleForTesting(otherwise = VisibleForTesting.NONE)
+    fun unregisterAllCreateObservers() {
+        createObservers.clear()
+    }
+}

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

@@ -0,0 +1,128 @@
+package com.mapbox.navigation.core
+
+import com.mapbox.navigation.core.internal.LegacyMapboxNavigationInstanceHolder
+import com.mapbox.navigation.core.internal.MapboxNavigationCreateObserver
+import com.mapbox.navigation.testing.LoggingFrontendTestRule
+import io.mockk.clearMocks
+import io.mockk.mockk
+import io.mockk.verify
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
+import org.junit.Before
+import org.junit.Rule
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+
+@RunWith(RobolectricTestRunner::class)
+class LegacyMapboxNavigationInstanceHolderTest {
+
+    @get:Rule
+    val loggerRule = LoggingFrontendTestRule()
+    private val observer = mockk<MapboxNavigationCreateObserver>(relaxed = true)
+    private val mapboxNavigation = mockk<MapboxNavigation>(relaxed = true)
+
+    @Before
+    fun setUp() {
+        LegacyMapboxNavigationInstanceHolder.unregisterAllCreateObservers()
+        LegacyMapboxNavigationInstanceHolder.onDestroyed()
+    }
+
+    @After
+    fun tearDown() {
+        LegacyMapboxNavigationInstanceHolder.unregisterAllCreateObservers()
+        LegacyMapboxNavigationInstanceHolder.onDestroyed()
+    }
+
+    @Test
+    fun `observer is invoked on registration if has mapboxNavigation`() {
+        LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation)
+
+        LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer)
+
+        verify(exactly = 1) { observer.onCreated(mapboxNavigation) }
+    }
+
+    @Test
+    fun `observer is not invoked on registration if has no mapboxNavigation`() {
+        LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer)
+
+        verify(exactly = 0) { observer.onCreated(mapboxNavigation) }
+    }
+
+    @Test
+    fun `observer is not invoked on registration if has destroyed mapboxNavigation`() {
+        LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation)
+        LegacyMapboxNavigationInstanceHolder.onDestroyed()
+
+        LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer)
+
+        verify(exactly = 0) { observer.onCreated(mapboxNavigation) }
+    }
+
+    @Test
+    fun `observer is invoked when mapboxNavigation is created`() {
+        LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer)
+
+        LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation)
+
+        verify(exactly = 1) { observer.onCreated(mapboxNavigation) }
+    }
+
+    @Test
+    fun `observer is invoked when new mapboxNavigation is created`() {
+        LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer)
+        LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation)
+        clearMocks(observer, answers = false)
+
+        val newNavigation = mockk<MapboxNavigation>(relaxed = true)
+        LegacyMapboxNavigationInstanceHolder.onCreated(newNavigation)
+
+        verify(exactly = 1) { observer.onCreated(newNavigation) }
+    }
+
+    @Test
+    fun `removed observer is not invoked when mapboxNavigation is created`() {
+        LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer)
+        LegacyMapboxNavigationInstanceHolder.unregisterCreateObserver(observer)
+
+        LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation)
+
+        verify(exactly = 0) { observer.onCreated(any()) }
+    }
+
+    @Test
+    fun `multiple observers are invoked when new mapboxNavigation is created`() {
+        val secondObserver = mockk<MapboxNavigationCreateObserver>(relaxed = true)
+        LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer)
+        LegacyMapboxNavigationInstanceHolder.registerCreateObserver(secondObserver)
+
+        LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation)
+
+        verify(exactly = 1) {
+            observer.onCreated(mapboxNavigation)
+            secondObserver.onCreated(mapboxNavigation)
+        }
+    }
+
+    @Test
+    fun `peek with no instance`() {
+        assertNull(LegacyMapboxNavigationInstanceHolder.peek())
+    }
+
+    @Test
+    fun `peek with active instance`() {
+        LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation)
+
+        assertEquals(mapboxNavigation, LegacyMapboxNavigationInstanceHolder.peek())
+    }
+
+    @Test
+    fun `peek with destroyed instance`() {
+        LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation)
+        LegacyMapboxNavigationInstanceHolder.onDestroyed()
+
+        assertNull(LegacyMapboxNavigationInstanceHolder.peek())
+    }
+}

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

@@ -19,6 +19,7 @@ import com.mapbox.navigation.base.trip.notification.TripNotification
 import com.mapbox.navigation.core.accounts.BillingController
 import com.mapbox.navigation.core.arrival.ArrivalProgressObserver
 import com.mapbox.navigation.core.directions.session.DirectionsSession
+import com.mapbox.navigation.core.internal.LegacyMapboxNavigationInstanceHolder
 import com.mapbox.navigation.core.navigator.CacheHandleWrapper
 import com.mapbox.navigation.core.reroute.RerouteController
 import com.mapbox.navigation.core.reroute.RerouteState
@@ -124,6 +125,7 @@ internal open class MapboxNavigationBaseTest {
 
     @Before
     open fun setUp() {
+        LegacyMapboxNavigationInstanceHolder.onDestroyed()
         every { threadController.getMainScopeAndRootJob() } answers {
             JobControl(mockk(), coroutineRule.createTestScope())
         }
@@ -234,6 +236,7 @@ internal open class MapboxNavigationBaseTest {
         unmockkObject(EventsServiceProvider)
         unmockkObject(TelemetryServiceProvider)
         unmockkObject(TelemetryUtilsDelegate)
+        LegacyMapboxNavigationInstanceHolder.onDestroyed()
     }
 
     fun createMapboxNavigation() {

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

@@ -18,6 +18,7 @@ import com.mapbox.navigation.core.directions.session.RoutesExtra
 import com.mapbox.navigation.core.directions.session.RoutesObserver
 import com.mapbox.navigation.core.directions.session.RoutesUpdatedResult
 import com.mapbox.navigation.core.internal.HistoryRecordingStateChangeObserver
+import com.mapbox.navigation.core.internal.LegacyMapboxNavigationInstanceHolder
 import com.mapbox.navigation.core.internal.extensions.registerHistoryRecordingStateChangeObserver
 import com.mapbox.navigation.core.internal.extensions.unregisterHistoryRecordingStateChangeObserver
 import com.mapbox.navigation.core.internal.telemetry.NavigationCustomEventType
@@ -55,6 +56,7 @@ import io.mockk.coVerifyOrder
 import io.mockk.every
 import io.mockk.just
 import io.mockk.mockk
+import io.mockk.mockkObject
 import io.mockk.slot
 import io.mockk.verify
 import io.mockk.verifyOrder
@@ -1713,4 +1715,32 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
             routeRefreshRequestDataProvider.onEVDataUpdated(data)
         }
     }
+
+    @Test
+    fun mapboxNavigationCreationSetsInstanceToHolder() {
+        mockkObject(LegacyMapboxNavigationInstanceHolder) {
+            createMapboxNavigation()
+            verify { LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation) }
+        }
+    }
+
+    @Test(expected = IllegalStateException::class)
+    fun mapboxNavigationIsNotCreatedIfHolderHasInstance() {
+        mockkObject(LegacyMapboxNavigationInstanceHolder) {
+            every { LegacyMapboxNavigationInstanceHolder.peek() } returns mockk(relaxed = true)
+            createMapboxNavigation()
+            verify(exactly = 0) {
+                LegacyMapboxNavigationInstanceHolder.onCreated(any())
+            }
+        }
+    }
+
+    @Test
+    fun mapboxNavigationDestructionRemovesInstanceFromHolder() {
+        mockkObject(LegacyMapboxNavigationInstanceHolder) {
+            createMapboxNavigation()
+            mapboxNavigation.onDestroy()
+            verify { LegacyMapboxNavigationInstanceHolder.onDestroyed() }
+        }
+    }
 }