NAVAND-777: meet code review

Author: dzinad

CHANGELOG.md

@@ -4,7 +4,6 @@ Mapbox welcomes participation and contributions from everyone.
 
 ## Unreleased
 #### Features
-- Added `MapboxNavigation#refreshRoutesImmediately` to trigger route refresh request immediately. [#6610](https://github.com/mapbox/mapbox-navigation-android/pull/6610)
 #### Bug fixes and improvements
 
 ## Mapbox Navigation SDK 2.11.0-alpha.1 - 13 January, 2023

changelog/unreleased/features/dd.md

@@ -0,0 +1 @@
+- Added `MapboxNavigation#refreshRoutesImmediately` to trigger route refresh request immediately.

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

@@ -6,12 +6,11 @@ import com.mapbox.navigation.base.route.NavigationRoute
 @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
 internal class ImmediateRouteRefreshController(
     private val routeRefresherExecutor: RouteRefresherExecutor,
-    private val plannedRefreshController: Pausable,
     private val stateHolder: RouteRefreshStateHolder,
     private val listener: RouteRefresherListener
 ) {
 
-    private val callback = object : RouteRefresherProgressCallback {
+    private val progressCallback = object : RouteRefresherProgressCallback {
 
         override fun onStarted() {
             stateHolder.onStarted()
@@ -22,17 +21,29 @@ internal class ImmediateRouteRefreshController(
                 stateHolder.onSuccess()
             } else {
                 stateHolder.onFailure(null)
-                plannedRefreshController.resume()
             }
             listener.onRoutesRefreshed(routeRefresherResult)
         }
     }
 
-    fun requestRoutesRefresh(routes: List<NavigationRoute>) {
+    fun requestRoutesRefresh(routes: List<NavigationRoute>, callback: (RouteRefresherResult) -> Unit) {
         if (routes.isEmpty()) {
             return
         }
-        plannedRefreshController.pause()
-        routeRefresherExecutor.postRoutesToRefresh(routes, callback)
+        routeRefresherExecutor.postRoutesToRefresh(routes, wrapCallback(callback))
+    }
+
+    private fun wrapCallback(
+        callback: (RouteRefresherResult) -> Unit
+    ) = object : RouteRefresherProgressCallback {
+
+        override fun onStarted() {
+            progressCallback.onStarted()
+        }
+
+        override fun onResult(routeRefresherResult: RouteRefresherResult) {
+            progressCallback.onResult(routeRefresherResult)
+            callback(routeRefresherResult)
+        }
     }
 }

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

@@ -15,7 +15,7 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
     private val listener: RouteRefresherListener,
     private val cancellableHandler: CancellableHandler,
     private val retryStrategy: RetryRouteRefreshStrategy,
-) : Pausable {
+) {
 
     constructor(
         routeRefresherExecutor: RouteRefresherExecutor,
@@ -63,14 +63,14 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
         }
     }
 
-    override fun pause() {
+    fun pause() {
         if (!paused) {
             paused = true
             cancellableHandler.cancelAll()
         }
     }
 
-    override fun resume() {
+    fun resume() {
         if (paused) {
             paused = false
             routesToRefresh?.let {

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

@@ -7,7 +7,7 @@ import com.mapbox.navigation.base.route.NavigationRouterRefreshError
 import com.mapbox.navigation.core.RouteProgressData
 import com.mapbox.navigation.core.RouteProgressDataProvider
 import com.mapbox.navigation.core.directions.session.RouteRefresh
-import com.mapbox.navigation.core.ev.EVDynamicDataHolder
+import com.mapbox.navigation.core.ev.EVRefreshDataProvider
 import com.mapbox.navigation.utils.internal.logE
 import com.mapbox.navigation.utils.internal.logI
 import kotlinx.coroutines.async
@@ -25,7 +25,7 @@ internal data class RouteRefresherResult(
 
 internal class RouteRefresher(
     private val routeProgressDataProvider: RouteProgressDataProvider,
-    private val evDataHolder: EVDynamicDataHolder,
+    private val evRefreshDataProvider: EVRefreshDataProvider,
     private val routeDiffProvider: DirectionsRouteDiffProvider,
     private val routeRefresh: RouteRefresh,
 ) {
@@ -85,7 +85,7 @@ internal class RouteRefresher(
             routeProgressData.legIndex,
             routeProgressData.routeGeometryIndex,
             routeProgressData.legGeometryIndex,
-            evDataHolder.currentData(route.routeOptions.unrecognizedJsonProperties ?: emptyMap())
+            evRefreshDataProvider.get(route.routeOptions)
         )
         return when (val result = requestRouteRefresh(route, routeRefreshRequestData)) {
             is RouteRefreshResult.Fail -> {

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

@@ -18,6 +18,30 @@ class CancellableHandlerTest {
     private val cancellation = mockk<() -> Unit>(relaxed = true)
     private val handler = CancellableHandler(coroutineRule.coroutineScope)
 
+    @Test
+    fun postExecutesWithZeroTimeout() = coroutineRule.runBlockingTest {
+        handler.postDelayed(0, runnable, cancellation)
+
+        verify(exactly = 1) {
+            runnable.run()
+        }
+        verify(exactly = 0) {
+            cancellation.invoke()
+        }
+    }
+
+    @Test
+    fun postExecutesWithNegativeTimeout() = coroutineRule.runBlockingTest {
+        handler.postDelayed(-10, runnable, cancellation)
+
+        verify(exactly = 1) {
+            runnable.run()
+        }
+        verify(exactly = 0) {
+            cancellation.invoke()
+        }
+    }
+
     @Test
     fun postExecutesAfterTimeout() = coroutineRule.runBlockingTest {
         val timeout = 7000L
@@ -50,6 +74,28 @@ class CancellableHandlerTest {
         }
     }
 
+    @Test
+    fun postWithZeroTimeoutRemovesBlockFromMap() = coroutineRule.runBlockingTest {
+        handler.postDelayed(0, runnable, cancellation)
+
+        handler.cancelAll()
+
+        verify(exactly = 0) {
+            cancellation.invoke()
+        }
+    }
+
+    @Test
+    fun postWithNegativeTimeoutRemovesBlockFromMap() = coroutineRule.runBlockingTest {
+        handler.postDelayed(-10, runnable, cancellation)
+
+        handler.cancelAll()
+
+        verify(exactly = 0) {
+            cancellation.invoke()
+        }
+    }
+
     @Test
     fun cancelAll_noJobs() = coroutineRule.runBlockingTest {
         handler.cancelAll()

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

@@ -11,45 +11,37 @@ import org.junit.Test
 class ImmediateRouteRefreshControllerTest {
 
     private val routeRefresherExecutor = mockk<RouteRefresherExecutor>(relaxed = true)
-    private val plannedRefreshController = mockk<Pausable>(relaxed = true)
     private val stateHolder = mockk<RouteRefreshStateHolder>(relaxed = true)
     private val listener = mockk<RouteRefresherListener>(relaxed = true)
+    private val clientCallback = mockk<(RouteRefresherResult) -> Unit>(relaxed = true)
     private val routes = listOf<NavigationRoute>(mockk())
 
     private val sut = ImmediateRouteRefreshController(
         routeRefresherExecutor,
-        plannedRefreshController,
         stateHolder,
         listener
     )
 
     @Test
     fun requestRoutesRefreshWithEmptyRoutes() {
-        sut.requestRoutesRefresh(emptyList())
+        sut.requestRoutesRefresh(emptyList(), clientCallback)
 
         verify(exactly = 0) {
-            plannedRefreshController.pause()
+            clientCallback(any())
             routeRefresherExecutor.postRoutesToRefresh(any(), any())
         }
     }
 
-    @Test
-    fun requestRoutesRefreshPausesPlannedController() {
-        sut.requestRoutesRefresh(routes)
-
-        verify(exactly = 1) { plannedRefreshController.pause() }
-    }
-
     @Test
     fun requestRoutesRefreshPostsRefreshRequest() {
-        sut.requestRoutesRefresh(routes)
+        sut.requestRoutesRefresh(routes, clientCallback)
 
         verify(exactly = 1) { routeRefresherExecutor.postRoutesToRefresh(routes, any()) }
     }
 
     @Test
     fun routesRefreshStarted() {
-        sut.requestRoutesRefresh(routes)
+        sut.requestRoutesRefresh(routes, clientCallback)
         val callback = interceptCallback()
 
         callback.onStarted()
@@ -59,7 +51,7 @@ class ImmediateRouteRefreshControllerTest {
 
     @Test
     fun routesRefreshFinishedSuccessfully() {
-        sut.requestRoutesRefresh(routes)
+        sut.requestRoutesRefresh(routes, clientCallback)
         val callback = interceptCallback()
         val result = RouteRefresherResult(
             true,
@@ -71,11 +63,12 @@ class ImmediateRouteRefreshControllerTest {
 
         verify(exactly = 1) { stateHolder.onSuccess() }
         verify(exactly = 1) { listener.onRoutesRefreshed(result) }
+        verify(exactly = 1) { clientCallback(result) }
     }
 
     @Test
     fun routesRefreshFinishedWithFailure() {
-        sut.requestRoutesRefresh(routes)
+        sut.requestRoutesRefresh(routes, clientCallback)
         val callback = interceptCallback()
         val result = RouteRefresherResult(
             false,
@@ -86,7 +79,7 @@ class ImmediateRouteRefreshControllerTest {
         callback.onResult(result)
 
         verify(exactly = 1) { stateHolder.onFailure(null) }
-        verify(exactly = 1) { plannedRefreshController.resume() }
+        verify(exactly = 1) { clientCallback(result) }
         verify(exactly = 1) { listener.onRoutesRefreshed(result) }
     }
 

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

@@ -6,7 +6,7 @@ import com.mapbox.navigation.base.route.NavigationRouterRefreshCallback
 import com.mapbox.navigation.core.RouteProgressData
 import com.mapbox.navigation.core.RouteProgressDataProvider
 import com.mapbox.navigation.core.directions.session.RouteRefresh
-import com.mapbox.navigation.core.ev.EVDynamicDataHolder
+import com.mapbox.navigation.core.ev.EVRefreshDataProvider
 import com.mapbox.navigation.testing.LoggingFrontendTestRule
 import com.mapbox.navigation.testing.MainCoroutineRule
 import com.mapbox.navigation.testing.factories.createDirectionsRoute
@@ -43,12 +43,12 @@ class RouteRefresherTest {
     private val routeProgressDataProvider = mockk<RouteProgressDataProvider>(relaxed = true) {
         coEvery { getRouteRefreshRequestDataOrWait() } returns routeProgressData
     }
-    private val evDataHolder = mockk<EVDynamicDataHolder>(relaxed = true)
+    private val evRefreshDataProvider = mockk<EVRefreshDataProvider>(relaxed = true)
     private val routeDiffProvider = mockk<DirectionsRouteDiffProvider>(relaxed = true)
     private val routeRefresh = mockk<RouteRefresh>(relaxed = true)
     private val sut = RouteRefresher(
         routeProgressDataProvider,
-        evDataHolder,
+        evRefreshDataProvider,
         routeDiffProvider,
         routeRefresh,
     )