Skip to content

Commit 87965a6

Browse files
dzinaddzinad
committed
NAVAND-777: meet code review
1 parent 8615bab commit 87965a6

29 files changed

+626
-270
lines changed

‎instrumentation-tests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshOnDemandTest.kt‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import com.mapbox.navigation.core.directions.session.RoutesExtra
1717
import com.mapbox.navigation.core.directions.session.RoutesUpdatedResult
1818
import com.mapbox.navigation.instrumentation_tests.R
1919
import com.mapbox.navigation.instrumentation_tests.activity.EmptyTestActivity
20+
import com.mapbox.navigation.instrumentation_tests.utils.DelayedResponseModifier
2021
import com.mapbox.navigation.instrumentation_tests.utils.DynamicResponseModifier
2122
import com.mapbox.navigation.instrumentation_tests.utils.MapboxNavigationRule
2223
import com.mapbox.navigation.instrumentation_tests.utils.coroutines.getSuccessfulResultOrThrowException
@@ -198,10 +199,32 @@ class RouteRefreshOnDemandTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::
198199
mapboxNavigation.setNavigationRoutesAndWaitForUpdate(requestedRoutes)
199200

200201
delay(7000) // 2 failed planned attempts + accuracy
201-
mapboxNavigation.routeRefreshController.requestImmediateRouteRefresh() // fail and postpone next planned attempt
202+
// fail and postpone next planned attempt
203+
mapboxNavigation.routeRefreshController.requestImmediateRouteRefresh()
202204
delay(2000)
203-
mapboxNavigation.routeRefreshController.requestImmediateRouteRefresh() // dispatch new routes with REFRESH reason
205+
// dispatch new routes with REFRESH reason
206+
mapboxNavigation.routeRefreshController.requestImmediateRouteRefresh()
207+
208+
mapboxNavigation.routesUpdates()
209+
.filter { it.reason == RoutesExtra.ROUTES_UPDATE_REASON_REFRESH }
210+
.first()
211+
}
212+
213+
@Test
214+
fun route_refresh_on_demand_during_planned_request() = sdkTest {
215+
val routeRefreshOptions = createRouteRefreshOptionsWithInvalidInterval(5000)
216+
refreshHandler.jsonResponseModifier = DelayedResponseModifier(3000)
217+
createMapboxNavigation(routeRefreshOptions)
218+
val routeOptions = generateRouteOptions(twoCoordinates)
219+
val requestedRoutes = mapboxNavigation.requestRoutes(routeOptions)
220+
.getSuccessfulResultOrThrowException()
221+
.routes
222+
mapboxNavigation.startTripSession()
223+
stayOnInitialPosition()
224+
mapboxNavigation.setNavigationRoutesAndWaitForUpdate(requestedRoutes)
204225

226+
delay(6000) // refresh interval + accuracy
227+
mapboxNavigation.routeRefreshController.requestImmediateRouteRefresh()
205228
mapboxNavigation.routesUpdates()
206229
.filter { it.reason == RoutesExtra.ROUTES_UPDATE_REASON_REFRESH }
207230
.first()

‎instrumentation-tests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshStateTest.kt‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,13 @@ class RouteRefreshStateTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
8686
mapboxNavigation.routeRefreshController.registerRouteRefreshStateObserver(observer)
8787
mapboxNavigation.routeRefreshController.requestImmediateRouteRefresh()
8888

89-
assertEquals(emptyList<String>(), observer.getStatesSnapshot())
89+
assertEquals(
90+
listOf(
91+
RouteRefreshExtra.REFRESH_STATE_STARTED,
92+
RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED,
93+
),
94+
observer.getStatesSnapshot()
95+
)
9096
}
9197

9298
@Test
@@ -114,7 +120,7 @@ class RouteRefreshStateTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
114120
assertEquals(
115121
listOf(
116122
RouteRefreshExtra.REFRESH_STATE_STARTED,
117-
RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED
123+
RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED,
118124
),
119125
observer.getStatesSnapshot()
120126
)
@@ -140,7 +146,10 @@ class RouteRefreshStateTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
140146
delay(2000) // refresh interval + accuracy
141147

142148
assertEquals(
143-
listOf(RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED),
149+
listOf(
150+
RouteRefreshExtra.REFRESH_STATE_STARTED,
151+
RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED,
152+
),
144153
observer.getStatesSnapshot()
145154
)
146155
}
@@ -265,6 +274,7 @@ class RouteRefreshStateTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
265274
listOf(
266275
RouteRefreshExtra.REFRESH_STATE_STARTED,
267276
RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED,
277+
RouteRefreshExtra.REFRESH_STATE_CLEARED_EXPIRED,
268278
RouteRefreshExtra.REFRESH_STATE_STARTED,
269279
RouteRefreshExtra.REFRESH_STATE_FINISHED_SUCCESS
270280
),

‎instrumentation-tests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshTest.kt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import com.mapbox.api.directions.v5.models.DirectionsResponse
99
import com.mapbox.api.directions.v5.models.Incident
1010
import com.mapbox.api.directions.v5.models.RouteOptions
1111
import com.mapbox.geojson.Point
12-
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
1312
import com.mapbox.navigation.base.extensions.applyDefaultNavigationOptions
1413
import com.mapbox.navigation.base.options.NavigationOptions
1514
import com.mapbox.navigation.base.options.RoutingTilesOptions
@@ -60,7 +59,6 @@ import java.net.URI
6059
import java.util.concurrent.TimeUnit
6160
import kotlin.math.absoluteValue
6261

63-
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
6462
class RouteRefreshTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::class.java) {
6563

6664
@get:Rule

‎libnavigation-core/api/current.txt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -891,12 +891,13 @@ package com.mapbox.navigation.core.routerefresh {
891891
@com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI public final class RouteRefreshExtra {
892892
field public static final com.mapbox.navigation.core.routerefresh.RouteRefreshExtra INSTANCE;
893893
field public static final String REFRESH_STATE_CANCELED = "CANCELED";
894+
field public static final String REFRESH_STATE_CLEARED_EXPIRED = "CLEARED_EXPIRED";
894895
field public static final String REFRESH_STATE_FINISHED_FAILED = "FINISHED_FAILED";
895896
field public static final String REFRESH_STATE_FINISHED_SUCCESS = "FINISHED_SUCCESS";
896897
field public static final String REFRESH_STATE_STARTED = "STARTED";
897898
}
898899

899-
@StringDef({com.mapbox.navigation.core.routerefresh.RouteRefreshExtra.REFRESH_STATE_STARTED, com.mapbox.navigation.core.routerefresh.RouteRefreshExtra.REFRESH_STATE_FINISHED_SUCCESS, com.mapbox.navigation.core.routerefresh.RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED, com.mapbox.navigation.core.routerefresh.RouteRefreshExtra.REFRESH_STATE_CANCELED}) @kotlin.annotation.Retention(kotlin.annotation.AnnotationRetention.BINARY) public static @interface RouteRefreshExtra.RouteRefreshState {
900+
@StringDef({com.mapbox.navigation.core.routerefresh.RouteRefreshExtra.REFRESH_STATE_STARTED, com.mapbox.navigation.core.routerefresh.RouteRefreshExtra.REFRESH_STATE_FINISHED_SUCCESS, com.mapbox.navigation.core.routerefresh.RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED, com.mapbox.navigation.core.routerefresh.RouteRefreshExtra.REFRESH_STATE_CLEARED_EXPIRED, com.mapbox.navigation.core.routerefresh.RouteRefreshExtra.REFRESH_STATE_CANCELED}) @kotlin.annotation.Retention(kotlin.annotation.AnnotationRetention.BINARY) public static @interface RouteRefreshExtra.RouteRefreshState {
900901
}
901902

902903
@com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI public final class RouteRefreshStateResult {

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -396,7 +396,6 @@ class MapboxNavigation @VisibleForTesting internal constructor(
396396
val experimental: com.mapbox.navigator.Experimental
397397
get() = navigator.experimental
398398

399-
400399
private var reachabilityObserverId: Long? = null
401400

402401
private var latestLegIndex: Int? = null
@@ -1222,17 +1221,18 @@ class MapboxNavigation @VisibleForTesting internal constructor(
12221221
historyRecordingStateHandler.unregisterAllStateChangeObservers()
12231222
historyRecordingStateHandler.unregisterAllCopilotSessionObservers()
12241223
developerMetadataAggregator.unregisterAllObservers()
1224+
routeRefreshControllerImpl.destroy()
1225+
routesPreviewController.unregisterAllRoutesPreviewObservers()
12251226
runInTelemetryContext { telemetry ->
12261227
telemetry.destroy(this@MapboxNavigation)
12271228
}
1229+
// first unregister all observers, then cancel the coroutines to avoid getting cancelled state
12281230
threadController.cancelAllNonUICoroutines()
12291231
threadController.cancelAllUICoroutines()
12301232
ifNonNull(reachabilityObserverId) {
12311233
ReachabilityService.removeReachabilityObserver(it)
12321234
reachabilityObserverId = null
12331235
}
1234-
routeRefreshControllerImpl.destroy()
1235-
routesPreviewController.unregisterAllRoutesPreviewObservers()
12361236

12371237
isDestroyed = true
12381238
hasInstance = false

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ internal class RoutesProgressDataProvider(
2121
/**
2222
* Retrieved progress data for passed routes.
2323
*
24-
* @throws IllegalArgumentException if routes re empty
24+
* @throws IllegalArgumentException if routes are empty
2525
*/
2626
@Throws(IllegalArgumentException::class)
2727
suspend fun getRoutesProgressData(

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

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package com.mapbox.navigation.core.routerefresh
22

3+
import kotlinx.coroutines.CancellationException
34
import kotlinx.coroutines.CoroutineScope
5+
import kotlinx.coroutines.Dispatchers
46
import kotlinx.coroutines.Job
57
import kotlinx.coroutines.delay
68
import kotlinx.coroutines.launch
@@ -9,21 +11,25 @@ internal class CancellableHandler(
911
private val scope: CoroutineScope
1012
) {
1113

12-
private val jobs = linkedMapOf<Job, () -> Unit>()
14+
private val jobs = linkedSetOf<Job>()
1315

14-
fun postDelayed(timeout: Long, block: Runnable, cancellationCallback: () -> Unit) {
15-
val job = scope.launch {
16-
delay(timeout)
17-
block.run()
16+
fun postDelayed(timeout: Long, block: suspend () -> Unit, cancellationCallback: () -> Unit) {
17+
val job = scope.launch(Dispatchers.Main.immediate) {
18+
try {
19+
delay(timeout)
20+
block()
21+
} catch (ex: CancellationException) {
22+
cancellationCallback()
23+
throw ex
24+
}
1825
}
19-
jobs[job] = cancellationCallback
26+
jobs.add(job)
2027
job.invokeOnCompletion { jobs.remove(job) }
2128
}
2229

2330
fun cancelAll() {
24-
HashMap(jobs).forEach {
25-
it.value.invoke()
26-
it.key.cancel()
31+
HashSet(jobs).forEach {
32+
it.cancel()
2733
}
2834
}
2935
}

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

Lines changed: 25 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,52 +1,45 @@
11
package com.mapbox.navigation.core.routerefresh
22

3+
import com.mapbox.bindgen.Expected
34
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
45
import com.mapbox.navigation.base.route.NavigationRoute
6+
import com.mapbox.navigation.utils.internal.logW
7+
import kotlinx.coroutines.CoroutineScope
8+
import kotlinx.coroutines.launch
59

610
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
711
internal class ImmediateRouteRefreshController(
812
private val routeRefresherExecutor: RouteRefresherExecutor,
913
private val stateHolder: RouteRefreshStateHolder,
14+
private val scope: CoroutineScope,
1015
private val listener: RouteRefresherListener
1116
) {
1217

13-
private val progressCallback = object : RouteRefresherProgressCallback {
14-
15-
override fun onStarted() {
16-
stateHolder.onStarted()
17-
}
18-
19-
override fun onResult(routeRefresherResult: RouteRefresherResult) {
20-
if (routeRefresherResult.success) {
21-
stateHolder.onSuccess()
22-
} else {
23-
stateHolder.onFailure(null)
24-
}
25-
listener.onRoutesRefreshed(routeRefresherResult)
26-
}
27-
}
28-
18+
@Throws(IllegalArgumentException::class)
2919
fun requestRoutesRefresh(
3020
routes: List<NavigationRoute>,
31-
callback: (RouteRefresherResult) -> Unit
21+
callback: (Expected<String, RouteRefresherResult>) -> Unit
3222
) {
3323
if (routes.isEmpty()) {
34-
return
24+
throw IllegalArgumentException("Routes to refresh should not be empty")
3525
}
36-
routeRefresherExecutor.postRoutesToRefresh(routes, wrapCallback(callback))
37-
}
38-
39-
private fun wrapCallback(
40-
callback: (RouteRefresherResult) -> Unit
41-
) = object : RouteRefresherProgressCallback {
42-
43-
override fun onStarted() {
44-
progressCallback.onStarted()
45-
}
46-
47-
override fun onResult(routeRefresherResult: RouteRefresherResult) {
48-
progressCallback.onResult(routeRefresherResult)
49-
callback(routeRefresherResult)
26+
scope.launch {
27+
val result = routeRefresherExecutor.executeRoutesRefresh(
28+
routes,
29+
startCallback = { stateHolder.onStarted() }
30+
)
31+
callback(result)
32+
result.fold(
33+
{ logW("Route refresh on-demand error: $it", RouteRefreshLog.LOG_CATEGORY) },
34+
{
35+
if (it.success) {
36+
stateHolder.onSuccess()
37+
} else {
38+
stateHolder.onFailure(null)
39+
}
40+
listener.onRoutesRefreshed(it)
41+
}
42+
)
5043
}
5144
}
5245
}

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

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
55
import com.mapbox.navigation.base.route.NavigationRoute
66
import com.mapbox.navigation.base.route.RouteRefreshOptions
77
import com.mapbox.navigation.utils.internal.logI
8+
import com.mapbox.navigation.utils.internal.logW
89
import kotlinx.coroutines.CoroutineScope
910

1011
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
@@ -29,7 +30,7 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
2930
stateHolder,
3031
listener,
3132
CancellableHandler(scope),
32-
RetryRouteRefreshStrategy(maxRetryCount = MAX_RETRY_COUNT)
33+
RetryRouteRefreshStrategy(maxAttemptsCount = MAX_RETRY_COUNT)
3334
)
3435

3536
private var paused = false
@@ -40,7 +41,7 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
4041
cancellableHandler.cancelAll()
4142
routesToRefresh = null
4243
if (routes.isEmpty()) {
43-
logI("Routes are empty", RouteRefreshLog.LOG_CATEGORY)
44+
logI("Routes are empty, nothing to refresh", RouteRefreshLog.LOG_CATEGORY)
4445
stateHolder.reset()
4546
return
4647
}
@@ -59,6 +60,7 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
5960
)
6061
val logMessage = "No routes which could be refreshed. $message"
6162
logI(logMessage, RouteRefreshLog.LOG_CATEGORY)
63+
stateHolder.onStarted()
6264
stateHolder.onFailure(logMessage)
6365
stateHolder.reset()
6466
}
@@ -90,55 +92,49 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
9092
}
9193

9294
private fun scheduleUpdateRetry(routes: List<NavigationRoute>, shouldNotifyOnStart: Boolean) {
93-
postAttempt { executePlannedRefresh(routes, shouldNotifyOnStart = shouldNotifyOnStart) }
95+
postAttempt {
96+
retryStrategy.onNextAttempt()
97+
executePlannedRefresh(routes, shouldNotifyOnStart = shouldNotifyOnStart)
98+
}
9499
}
95100

96-
private fun postAttempt(attemptBlock: () -> Unit) {
101+
private fun postAttempt(attemptBlock: suspend () -> Unit) {
97102
cancellableHandler.postDelayed(
98103
timeout = routeRefreshOptions.intervalMillis,
99104
block = attemptBlock,
100105
cancellationCallback = { stateHolder.onCancel() }
101106
)
102107
}
103108

104-
private fun executePlannedRefresh(
109+
private suspend fun executePlannedRefresh(
105110
routes: List<NavigationRoute>,
106111
shouldNotifyOnStart: Boolean
107112
) {
108-
routeRefresherExecutor.postRoutesToRefresh(
113+
val routeRefresherResult = routeRefresherExecutor.executeRoutesRefresh(
109114
routes,
110-
createCallback(routes, shouldNotifyOnStart)
111-
)
112-
}
113-
114-
private fun createCallback(
115-
routes: List<NavigationRoute>,
116-
shouldNotifyOnStart: Boolean
117-
): RouteRefresherProgressCallback {
118-
return object : RouteRefresherProgressCallback {
119-
120-
override fun onStarted() {
115+
startCallback = {
121116
if (shouldNotifyOnStart) {
122117
stateHolder.onStarted()
123118
}
124119
}
125-
126-
override fun onResult(routeRefresherResult: RouteRefresherResult) {
127-
retryStrategy.onNextAttempt()
128-
if (routeRefresherResult.success) {
120+
)
121+
routeRefresherResult.fold(
122+
{ logW("Planned route refresh error: $it", RouteRefreshLog.LOG_CATEGORY) },
123+
{
124+
if (it.success) {
129125
stateHolder.onSuccess()
130-
listener.onRoutesRefreshed(routeRefresherResult)
126+
listener.onRoutesRefreshed(it)
131127
} else {
132128
if (retryStrategy.shouldRetry()) {
133129
scheduleUpdateRetry(routes, shouldNotifyOnStart = false)
134130
} else {
135131
stateHolder.onFailure(null)
136-
listener.onRoutesRefreshed(routeRefresherResult)
132+
listener.onRoutesRefreshed(it)
137133
scheduleNewUpdate(routes)
138134
}
139135
}
140136
}
141-
}
137+
)
142138
}
143139

144140
companion object {

0 commit comments

Comments
 (0)