Fix route refresh not starting (#4831)

* fix route refresh not starting * removed route from the TripSession interface to clearly make DirectionsSession the source of truth
mapbox · Sep 16, 2021 · d41c5ad · d41c5ad
1 parent 16e7fad
commit d41c5ad
Show file tree

Hide file tree

Showing 7 changed files with 43 additions and 88 deletions.
diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt
@@ -408,7 +408,6 @@ class MapboxNavigation(
             tripSession,
             logger
         )
-        routeRefreshController.restart()
 
         defaultRerouteController = MapboxRerouteController(
             directionsSession,
@@ -569,7 +568,6 @@ class MapboxNavigation(
         rerouteController?.interrupt()
         routeAlternativesController.interrupt()
         directionsSession.setRoutes(routes, initialLegIndex)
-        routeRefreshController.restart()
     }
 
     /**
@@ -993,6 +991,11 @@ class MapboxNavigation(
 
     private fun createInternalRoutesObserver() = RoutesObserver { routes ->
         tripSession.setRoute(routes.firstOrNull(), directionsSession.initialLegIndex)
+        if (routes.isNotEmpty()) {
+            routeRefreshController.restart(routes.first())
+        } else {
+            routeRefreshController.stop()
+        }
     }
 
     private fun createInternalOffRouteObserver() = OffRouteObserver { offRoute ->
@@ -1052,7 +1055,7 @@ class MapboxNavigation(
                 logger
             )
             historyRecorder.historyRecorderHandle = navigator.getHistoryRecorderHandle()
-            tripSession.route?.let {
+            directionsSession.routes.firstOrNull()?.let {
                 navigator.setRoute(
                     it,
                     tripSession.getRouteProgress()?.currentLegProgress?.legIndex ?: 0

diff --git a/...tion-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt b/...tion-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt
@@ -16,7 +16,7 @@ import com.mapbox.navigation.utils.internal.MapboxTimer
  * This class is responsible for refreshing the current direction route's traffic.
  * This does not support alternative routes.
  *
- * If the route is successfully refreshed, this class will update the [TripSession.route]
+ * If the route is successfully refreshed, this class will update the [DirectionsSession].
  */
 internal class RouteRefreshController(
     private val routeRefreshOptions: RouteRefreshOptions,
@@ -40,14 +40,13 @@ internal class RouteRefreshController(
      * If route refresh is enabled, attach a refresh poller.
      * Cancel old timers, and cancel pending requests.
      */
-    fun restart() {
+    fun restart(route: DirectionsRoute) {
         stop()
-        val route = tripSession.route
-        if (route?.routeOptions()?.enableRefresh() == true) {
+        if (route.routeOptions()?.enableRefresh() == true) {
             routerRefreshTimer.startTimer {
-                refreshRoute()
+                refreshRoute(route)
             }
-        } else if (route != null && route.routeOptions() == null) {
+        } else if (route.routeOptions() == null) {
             logger.w(
                 TAG,
                 Message(
@@ -72,11 +71,9 @@ internal class RouteRefreshController(
         }
     }
 
-    private fun refreshRoute() {
-        val route = tripSession.route
-            ?.takeIf { it.routeOptions()?.enableRefresh() == true }
-            ?.takeIf { it.isUuidValidForRefresh() }
-        if (route != null) {
+    private fun refreshRoute(route: DirectionsRoute) {
+        val isValid = route.routeOptions()?.enableRefresh() == true && route.isUuidValidForRefresh()
+        if (isValid) {
             val legIndex = tripSession.getRouteProgress()?.currentLegProgress?.legIndex ?: 0
             currentRequestId?.let { directionsSession.cancelRouteRefreshRequest(it) }
             currentRequestId = directionsSession.requestRouteRefresh(
@@ -92,7 +89,7 @@ internal class RouteRefreshController(
                         The route is not qualified for route refresh feature.
                         See com.mapbox.navigation.base.extensions.supportsRouteRefresh
                         extension for details.
-                        ${route?.routeOptions()}
+                        ${route.routeOptions()}
                     """.trimIndent()
                 )
             )

diff --git a/...avigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt b/...avigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt
@@ -4,6 +4,7 @@ import android.annotation.SuppressLint
 import android.hardware.SensorEvent
 import android.location.Location
 import android.os.Looper
+import androidx.annotation.VisibleForTesting
 import com.mapbox.android.core.location.LocationEngineCallback
 import com.mapbox.android.core.location.LocationEngineResult
 import com.mapbox.api.directions.v5.models.BannerInstructions
@@ -54,8 +55,6 @@ import java.util.concurrent.CopyOnWriteArraySet
  * @param navigator Native navigator
  * @param threadController controller for main/io jobs
  * @param logger interface for logging any events
- *
- * @property route should be set to start routing
  */
 internal class MapboxTripSession(
     override val tripService: TripService,
@@ -68,8 +67,8 @@ internal class MapboxTripSession(
 
     private var updateRouteJob: Job? = null
 
-    override var route: DirectionsRoute? = null
-        private set
+    @VisibleForTesting
+    internal var route: DirectionsRoute? = null
 
     override fun setRoute(route: DirectionsRoute?, legIndex: Int) {
         val isSameUuid = route?.isSameUuid(this.route) ?: false

diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/TripSession.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/TripSession.kt
@@ -11,7 +11,6 @@ import com.mapbox.navigator.FallbackVersionsObserver
 internal interface TripSession {
 
     val tripService: TripService
-    val route: DirectionsRoute?
     fun setRoute(route: DirectionsRoute?, legIndex: Int)
 
     fun getRawLocation(): Location?

diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationTest.kt
@@ -19,7 +19,6 @@ import com.mapbox.navigation.base.internal.extensions.inferDeviceLocale
 import com.mapbox.navigation.base.options.IncidentsOptions
 import com.mapbox.navigation.base.options.NavigationOptions
 import com.mapbox.navigation.base.options.RoutingTilesOptions
-import com.mapbox.navigation.base.route.RouteRefreshOptions
 import com.mapbox.navigation.base.route.Router
 import com.mapbox.navigation.base.route.RouterCallback
 import com.mapbox.navigation.base.route.RouterOrigin
@@ -291,43 +290,6 @@ class MapboxNavigationTest {
         verify(exactly = 2) { tripSession.unregisterAllOffRouteObservers() }
     }
 
-    @Test
-    fun `restart the routeRefreshController during initialization`() {
-        createMapboxNavigation()
-        ThreadController.cancelAllUICoroutines()
-        verify(exactly = 1) { routeRefreshController.restart() }
-    }
-
-    @Test
-    fun `restart the routeRefreshController if new route is set`() {
-        ThreadController.cancelAllUICoroutines()
-
-        mapboxNavigation = MapboxNavigation(
-            provideNavigationOptions()
-                .routeRefreshOptions(
-                    RouteRefreshOptions.Builder()
-                        .build()
-                )
-                .build()
-        )
-        val route: DirectionsRoute = mockk()
-        val routeOptions: RouteOptions = mockk()
-        every { route.routeOptions() } returns routeOptions
-        every { route.geometry() } returns "geometry"
-        every { route.legs() } returns emptyList()
-        every { routeOptions.overview() } returns "full"
-        every { routeOptions.annotationsList() } returns emptyList()
-        every { routeOptions.enableRefresh() } returns true
-        mapboxNavigation.setRoutes(listOf(route))
-
-        // the sequence needs to be kept because routeRefreshController uses the route stored
-        // in the trip session, which has to be set first via directionsSession
-        verifyOrder {
-            directionsSession.setRoutes(listOf(route))
-            routeRefreshController.restart()
-        }
-    }
-
     @Test
     fun registerMapMatcherResultObserver() {
         createMapboxNavigation()
@@ -563,6 +525,7 @@ class MapboxNavigationTest {
         }
 
         verify { tripSession.setRoute(primary, initialLegIndex) }
+        verify { routeRefreshController.restart(primary) }
     }
 
     @Test
@@ -579,6 +542,7 @@ class MapboxNavigationTest {
         }
 
         verify { tripSession.setRoute(null, initialLegIndex) }
+        verify { routeRefreshController.stop() }
     }
 
     @Test
@@ -892,7 +856,7 @@ class MapboxNavigationTest {
         every {
             tripSession.registerFallbackVersionsObserver(capture(fallbackObserverSlot))
         } just Runs
-        every { tripSession.route } returns null
+        every { directionsSession.routes } returns emptyList()
         every { tripSession.getRouteProgress() } returns mockk()
 
         mapboxNavigation = MapboxNavigation(navigationOptions)
@@ -926,7 +890,7 @@ class MapboxNavigationTest {
         every {
             tripSession.registerFallbackVersionsObserver(capture(fallbackObserverSlot))
         } just Runs
-        every { tripSession.route } returns null
+        every { directionsSession.routes } returns emptyList()
         every { tripSession.getRouteProgress() } returns mockk()
 
         mapboxNavigation = MapboxNavigation(navigationOptions)
@@ -960,7 +924,7 @@ class MapboxNavigationTest {
         val routeProgress: RouteProgress = mockk()
         val legProgress: RouteLegProgress = mockk()
         val index = 137
-        every { tripSession.route } returns route
+        every { directionsSession.routes } returns listOf(route)
         every { tripSession.getRouteProgress() } returns routeProgress
         every { routeProgress.currentLegProgress } returns legProgress
         every { legProgress.legIndex } returns index

diff --git a/...-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt b/...-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt
@@ -79,7 +79,6 @@ class RouteRefreshControllerTest {
                 every { legIndex } returns 0
             }
         }
-        every { tripSession.route } returns validRoute
         every {
             directionsSession.requestRouteRefresh(any(), any(), capture(routeRefreshCallbackSlot))
         } returns requestId
@@ -89,7 +88,7 @@ class RouteRefreshControllerTest {
     fun `should refresh route every 5 minutes by default`() = coroutineRule.runBlockingTest {
         every { routeOptions.enableRefresh() } returns true
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(TimeUnit.MINUTES.toMillis(15))
         routeRefreshController.stop()
 
@@ -108,7 +107,7 @@ class RouteRefreshControllerTest {
         )
         every { routeOptions.enableRefresh() } returns true
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(TimeUnit.MINUTES.toMillis(15))
         routeRefreshController.stop()
 
@@ -119,7 +118,7 @@ class RouteRefreshControllerTest {
     fun `should refresh route with correct properties`() = coroutineRule.runBlockingTest {
         every { routeOptions.enableRefresh() } returns true
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(TimeUnit.MINUTES.toMillis(6))
         routeRefreshController.stop()
 
@@ -131,7 +130,7 @@ class RouteRefreshControllerTest {
         coroutineRule.runBlockingTest {
             every { routeOptions.enableRefresh() } returns true
 
-            routeRefreshController.restart()
+            routeRefreshController.restart(validRoute)
             coroutineRule.testDispatcher.advanceTimeBy(TimeUnit.MINUTES.toMillis(6))
             routeRefreshController.stop()
 
@@ -143,7 +142,7 @@ class RouteRefreshControllerTest {
         every { routeOptions.enableRefresh() } returns true
         every { validRoute.requestUuid() } returns null
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(TimeUnit.MINUTES.toMillis(6))
         routeRefreshController.stop()
 
@@ -164,7 +163,7 @@ class RouteRefreshControllerTest {
     fun `cancel request when stopped`() = coroutineRule.runBlockingTest {
         every { routeOptions.enableRefresh() } returns true
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(TimeUnit.MINUTES.toMillis(6))
         routeRefreshController.stop()
 
@@ -175,7 +174,7 @@ class RouteRefreshControllerTest {
     fun `do not send a request when route options is null`() {
         every { validRoute.routeOptions() } returns null
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(routeRefreshOptions.intervalMillis * 2)
         routeRefreshController.stop()
 
@@ -187,7 +186,7 @@ class RouteRefreshControllerTest {
         every { routeOptions.enableRefresh() } returns true
         every { validRoute.requestUuid() } returns ""
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(routeRefreshOptions.intervalMillis * 2)
         routeRefreshController.stop()
 
@@ -199,7 +198,7 @@ class RouteRefreshControllerTest {
         every { routeOptions.enableRefresh() } returns true
         every { validRoute.requestUuid() } returns OFFLINE_UUID
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(routeRefreshOptions.intervalMillis * 2)
         routeRefreshController.stop()
 
@@ -216,13 +215,13 @@ class RouteRefreshControllerTest {
         val countDownLatch = CountDownLatch(2)
         every { directionsSession.requestRouteRefresh(any(), any(), any()) } answers {
             if (countDownLatch.count == 1L) {
-                routeRefreshController.restart()
+                routeRefreshController.restart(validRoute)
             }
             countDownLatch.countDown()
             countDownLatch.count
         }
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(routeRefreshOptions.intervalMillis * 2)
         countDownLatch.await()
         routeRefreshController.stop()
@@ -243,11 +242,11 @@ class RouteRefreshControllerTest {
             countDownLatch.count
         }
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(
             (routeRefreshOptions.intervalMillis * 1.9).toLong()
         )
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(
             (routeRefreshOptions.intervalMillis * 1.9).toLong()
         )
@@ -266,7 +265,7 @@ class RouteRefreshControllerTest {
         every { routeOptions.enableRefresh() } returns true
         every { routeDiffProvider.buildRouteDiffs(validRoute, any(), 0) } returns emptyList()
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(routeRefreshOptions.intervalMillis)
         routeRefreshCallbackSlot.captured.onRefresh(
             mockk {
@@ -283,7 +282,7 @@ class RouteRefreshControllerTest {
     fun `clear the request when there is a failure response`() {
         every { routeOptions.enableRefresh() } returns true
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(routeRefreshOptions.intervalMillis)
         routeRefreshCallbackSlot.captured.onError(
             mockk {
@@ -301,7 +300,7 @@ class RouteRefreshControllerTest {
     fun `should log warning when route options are null`() = coroutineRule.runBlockingTest {
         every { validRoute.routeOptions() } returns null
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(TimeUnit.MINUTES.toMillis(6))
         routeRefreshController.stop()
 
@@ -328,7 +327,7 @@ class RouteRefreshControllerTest {
         every { routeOptions.enableRefresh() } returns true
         every { routeDiffProvider.buildRouteDiffs(validRoute, newRoute, 0) } returns routeDiffs
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(routeRefreshOptions.intervalMillis)
         routeRefreshCallbackSlot.captured.onRefresh(newRoute)
         routeRefreshController.stop()
@@ -346,7 +345,7 @@ class RouteRefreshControllerTest {
         every { routeOptions.enableRefresh() } returns true
         every { routeDiffProvider.buildRouteDiffs(validRoute, newRoute, 0) } returns emptyList()
 
-        routeRefreshController.restart()
+        routeRefreshController.restart(validRoute)
         coroutineRule.testDispatcher.advanceTimeBy(routeRefreshOptions.intervalMillis)
         routeRefreshCallbackSlot.captured.onRefresh(newRoute)
         routeRefreshController.stop()