Skip to content

Commit 9e93574

Browse files
dzinaddzinad
committed
NAVSDK-777: meet code review
1 parent ad7ba6c commit 9e93574

9 files changed

Lines changed: 217 additions & 27 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,6 @@ This release depends on, and has been tested with, the following Mapbox dependen
300300
[Changes between v2.10.0-alpha.1 and v2.10.0-alpha.2](https://github.com/mapbox/mapbox-navigation-android/compare/v2.10.0-alpha.1...v2.10.0-alpha.2)
301301

302302
#### Features
303-
- Added `MapboxNavigation#refreshRoutesImmediately` to request an immediate refresh of current routes.
304303
#### Bug fixes and improvements
305304
- Fixed an issue where "silent waypoints" (not regular waypoints that define legs) had markers added on the map when route line was drawn with `MapboxRouteLineApi` and `MapboxRouteLineView`. [#6526](https://github.com/mapbox/mapbox-navigation-android/pull/6526)
306305
- 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)

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ class RouteRefreshOnDemandTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::
7474
)
7575
}
7676

77-
@Test
77+
@Test(timeout = 10_000)
7878
fun route_refresh_on_demand_executes_before_refresh_interval() = sdkTest {
7979
val routeRefreshOptions = RouteRefreshOptions.Builder()
8080
.intervalMillis(TimeUnit.MINUTES.toMillis(1))
@@ -113,7 +113,7 @@ class RouteRefreshOnDemandTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::
113113
.build()
114114
RouteRefreshOptions::class.java.getDeclaredField("intervalMillis").apply {
115115
isAccessible = true
116-
set(routeRefreshOptions, 10_000L)
116+
set(routeRefreshOptions, 5_000L)
117117
}
118118
refreshHandler.jsonResponseModifier = DynamicResponseModifier()
119119
createMapboxNavigation(routeRefreshOptions)
@@ -129,20 +129,20 @@ class RouteRefreshOnDemandTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::
129129
}
130130
}
131131
mapboxNavigation.setNavigationRoutesAndWaitForUpdate(requestedRoutes)
132-
delay(5000)
132+
delay(2500)
133133

134134
mapboxNavigation.refreshRoutesImmediately()
135135
mapboxNavigation.routesUpdates()
136136
.filter { it.reason == RoutesExtra.ROUTES_UPDATE_REASON_REFRESH }
137137
.first()
138138
assertEquals(1, routeRefreshes.size)
139139

140-
// no route refresh 6 seconds after refresh on demand
141-
delay(6000)
140+
// no route refresh 4 seconds after refresh on demand
141+
delay(4000)
142142
assertEquals(1, routeRefreshes.size)
143143

144-
delay(4000)
145-
// has new refresh 10 seconds after refresh on demand
144+
delay(1000)
145+
// has new refresh 5 seconds after refresh on demand
146146
mapboxNavigation.routesUpdates()
147147
.filter { it.reason == RoutesExtra.ROUTES_UPDATE_REASON_REFRESH }
148148
.take(2)

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

Lines changed: 110 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,41 @@ class RouteRefreshStateTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
173173
)
174174
}
175175

176+
@Test
177+
fun notStartedUntilTimeElapses() = sdkTest {
178+
setupMockRequestHandlers(
179+
coordinates,
180+
R.raw.route_response_single_route_refresh,
181+
createRefreshHandler(
182+
R.raw.route_response_route_refresh_annotations,
183+
"route_response_single_route_refresh"
184+
)
185+
)
186+
187+
createMapboxNavigation(createRouteRefreshOptionsWithInvalidInterval(5000))
188+
mapboxNavigation.registerRouteRefreshStateObserver(observer)
189+
mapboxNavigation.startTripSession()
190+
val requestedRoutes = requestRoutes()
191+
mapboxNavigation.setNavigationRoutes(requestedRoutes)
192+
193+
delay(4000)
194+
195+
assertEquals(
196+
emptyList<String>(),
197+
observer.getStatesSnapshot()
198+
)
199+
200+
waitForRefresh()
201+
202+
assertEquals(
203+
listOf(
204+
RouteRefreshExtra.REFRESH_STATE_STARTED,
205+
RouteRefreshExtra.REFRESH_STATE_FINISHED_SUCCESS
206+
),
207+
observer.getStatesSnapshot()
208+
)
209+
}
210+
176211
@Test
177212
fun successfulFromSecondAttemptRefreshTest() = sdkTest {
178213
setupMockRequestHandlers(
@@ -238,7 +273,7 @@ class RouteRefreshStateTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
238273
}
239274

240275
@Test
241-
fun routeRefreshIsNotCancelledOnDestroyTest() = sdkTest {
276+
fun routeRefreshDoesNotDispatchCancelledStateOnDestroyTest() = sdkTest {
242277
setupMockRequestHandlers(
243278
coordinates,
244279
R.raw.route_response_single_route_refresh,
@@ -267,7 +302,7 @@ class RouteRefreshStateTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
267302
)
268303
}
269304

270-
@Test
305+
@Test(timeout = 5_000)
271306
fun successfulRouteRefreshOnDemandTest() = sdkTest {
272307
setupMockRequestHandlers(
273308
coordinates,
@@ -296,7 +331,7 @@ class RouteRefreshStateTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
296331
)
297332
}
298333

299-
@Test
334+
@Test(timeout = 5_000)
300335
fun failedRouteRefreshOnDemandTest() = sdkTest {
301336
setupMockRequestHandlers(
302337
coordinates,
@@ -405,6 +440,41 @@ class RouteRefreshStateTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
405440
)
406441
}
407442

443+
@Test
444+
fun routeRefreshOnDemandFailsThenPlannedTest() = sdkTest {
445+
val refreshHandler = createRefreshHandler(
446+
R.raw.route_response_route_refresh_annotations,
447+
"route_response_single_route_refresh"
448+
)
449+
refreshHandler.jsonResponseModifier = DynamicResponseModifier()
450+
setupMockRequestHandlers(
451+
coordinates,
452+
R.raw.route_response_single_route_refresh,
453+
NthAttemptHandler(refreshHandler, 1)
454+
)
455+
456+
createMapboxNavigation(createRouteRefreshOptionsWithInvalidInterval(5_000))
457+
mapboxNavigation.registerRouteRefreshStateObserver(observer)
458+
mapboxNavigation.startTripSession()
459+
val requestedRoutes = requestRoutes()
460+
mapboxNavigation.setNavigationRoutesAndWaitForUpdate(requestedRoutes)
461+
462+
mapboxNavigation.refreshRoutesImmediately()
463+
delay(5000)
464+
465+
waitForRefreshes(2) // immediate + planned
466+
467+
assertEquals(
468+
listOf(
469+
RouteRefreshExtra.REFRESH_STATE_STARTED,
470+
RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED,
471+
RouteRefreshExtra.REFRESH_STATE_STARTED,
472+
RouteRefreshExtra.REFRESH_STATE_FINISHED_SUCCESS,
473+
),
474+
observer.getStatesSnapshot()
475+
)
476+
}
477+
408478
@Test
409479
fun routeRefreshOnDemandBetweenPlannedAttemptsTest() = sdkTest {
410480
val refreshHandler = createRefreshHandler(
@@ -442,6 +512,43 @@ class RouteRefreshStateTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
442512
)
443513
}
444514

515+
@Test
516+
fun routeRefreshOnDemandFailsBetweenPlannedAttemptsTest() = sdkTest {
517+
val refreshHandler = createRefreshHandler(
518+
R.raw.route_response_route_refresh_annotations,
519+
"route_response_single_route_refresh"
520+
)
521+
refreshHandler.jsonResponseModifier = DynamicResponseModifier()
522+
setupMockRequestHandlers(
523+
coordinates,
524+
R.raw.route_response_single_route_refresh,
525+
NthAttemptHandler(refreshHandler, 2)
526+
)
527+
528+
createMapboxNavigation(createRouteRefreshOptionsWithInvalidInterval(5_000))
529+
mapboxNavigation.registerRouteRefreshStateObserver(observer)
530+
mapboxNavigation.startTripSession()
531+
val requestedRoutes = requestRoutes()
532+
mapboxNavigation.setNavigationRoutesAndWaitForUpdate(requestedRoutes)
533+
delay(8000) // refresh interval + accuracy
534+
535+
mapboxNavigation.refreshRoutesImmediately()
536+
537+
waitForRefreshes(2) // one from immediate and the next planned
538+
539+
assertEquals(
540+
listOf(
541+
RouteRefreshExtra.REFRESH_STATE_STARTED,
542+
RouteRefreshExtra.REFRESH_STATE_CANCELED,
543+
RouteRefreshExtra.REFRESH_STATE_STARTED,
544+
RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED,
545+
RouteRefreshExtra.REFRESH_STATE_STARTED,
546+
RouteRefreshExtra.REFRESH_STATE_FINISHED_SUCCESS,
547+
),
548+
observer.getStatesSnapshot()
549+
)
550+
}
551+
445552
private fun createMapboxNavigation(routeRefreshOptions: RouteRefreshOptions) {
446553
mapboxNavigation = MapboxNavigationProvider.create(
447554
NavigationOptions.Builder(activity)

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -978,6 +978,11 @@ class MapboxNavigation @VisibleForTesting internal constructor(
978978
/**
979979
* Immediately refresh current navigation routes.
980980
* Listen for refreshed routes using [RoutesObserver].
981+
*
982+
* The on-demand refresh request is not guaranteed to succeed and call the [RoutesObserver],
983+
* [refreshRoutesImmediately] invocations cannot be coupled with
984+
* [RoutesObserver.onRoutesChanged] callbacks for state management.
985+
* You can use [registerRouteRefreshStateObserver] to monitor refresh statuses independently.
981986
*/
982987
fun refreshRoutesImmediately() {
983988
routeRefreshController.requestImmediateRouteRefresh(getNavigationRoutes())

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

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
3232
RetryRouteRefreshStrategy(maxRetryCount = 2)
3333
)
3434

35+
private var paused = false
3536
private var routesToRefresh: List<NavigationRoute>? = null
3637

3738
fun startRoutesRefreshing(routes: List<NavigationRoute>) {
@@ -63,13 +64,19 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
6364
}
6465

6566
override fun pause() {
66-
cancellableHandler.cancelAll()
67+
if (!paused) {
68+
paused = true
69+
cancellableHandler.cancelAll()
70+
}
6771
}
6872

6973
override fun resume() {
70-
routesToRefresh?.let {
71-
if (retryStrategy.shouldRetry()) {
72-
scheduleUpdateRetry(it)
74+
if (paused) {
75+
paused = false
76+
routesToRefresh?.let {
77+
if (retryStrategy.shouldRetry()) {
78+
scheduleUpdateRetry(it, shouldNotifyOnStart = true)
79+
}
7380
}
7481
}
7582
}
@@ -81,8 +88,8 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
8188
}
8289
}
8390

84-
private fun scheduleUpdateRetry(routes: List<NavigationRoute>) {
85-
postAttempt { executePlannedRefresh(routes, shouldNotifyOnStart = false) }
91+
private fun scheduleUpdateRetry(routes: List<NavigationRoute>, shouldNotifyOnStart: Boolean) {
92+
postAttempt { executePlannedRefresh(routes, shouldNotifyOnStart = shouldNotifyOnStart) }
8693
}
8794

8895
private fun postAttempt(attemptBlock: () -> Unit) {
@@ -122,7 +129,7 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
122129
listener.onRoutesRefreshed(routeRefresherResult)
123130
} else {
124131
if (retryStrategy.shouldRetry()) {
125-
scheduleUpdateRetry(routes)
132+
scheduleUpdateRetry(routes, shouldNotifyOnStart = false)
126133
} else {
127134
stateHolder.onFailure(null)
128135
if (routeRefresherResult.refreshedRoutes != routes) {

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ internal object RouteRefreshStateChanger {
1818
),
1919
RouteRefreshExtra.REFRESH_STATE_CANCELED to listOf(
2020
RouteRefreshExtra.REFRESH_STATE_STARTED,
21+
RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED,
2122
null,
2223
),
2324
RouteRefreshExtra.REFRESH_STATE_FINISHED_FAILED to listOf(

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,15 +44,15 @@ internal class RouteRefresher(
4444
val refreshedRoutes = refreshRoutesOrNull(routes, routeProgressData, routeRefreshTimeout)
4545
return if (refreshedRoutes.any { it != null }) {
4646
RouteRefresherResult(
47-
true,
47+
success = true,
4848
refreshedRoutes.mapIndexed { index, navigationRoute ->
4949
navigationRoute ?: routes[index]
5050
},
5151
routeProgressData
5252
)
5353
} else {
5454
RouteRefresherResult(
55-
false,
55+
success = false,
5656
routes.map { removeExpiringDataFromRoute(it, routeProgressData.legIndex) },
5757
routeProgressData
5858
)

0 commit comments

Comments
 (0)