Add route refresh

[kmadsen]

Description

Resolves https://github.com/mapbox/navigation-sdks/issues/222

Related refactors
Update route line #2581
Improve the MapboxTimer #2575
Modularize faster route #2577

Current status is, ready for review

• I have added any issue links
• I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
• I have added the appropriate milestone and project boards

Goal

Keep traffic fresh while driving viewing a route, and/or navigating a route

Implementation

1. Refresh the current route when the profile contains "traffic"
2. Use the directions call to request for a traffic update MapboxDirectionsRefresh
3. Update the LegAnnotations on the route (traffic data)

What changed from 0.43

1. Automatic refresh with "traffic" profiles. 0.43 was manually configured during initialization
2. Add refresh current route. 0.43 would only refresh during navigation
3. Remove implementation configuration. 0.43 allowed clients to make unnecessarily fast refresh rates

Example of the LegAnnotation json

{"distance":[39.82088283734119,11.943930596414795,11.741113723722966,51.342100705424095,5.849113092832654,6.034403160338987,44.25323230168093,5.526209358359064,13.899091719402477,43.49430010119116,11.195125050833482,11.459495476013347,82.25052046354502,4.0959504357663175,10.490508411438704,13.257044629366959,85.21705182045457,10.552608042831702,12.569519488314297,28.62658627157395,15.634516768831888,29.59563279009562,34.59872514885146],"congestion":["low","low","heavy","heavy","heavy","heavy","heavy","heavy","heavy","heavy","heavy","heavy","heavy","heavy","heavy","low","low","low","moderate","moderate","moderate","moderate","moderate"]}

Screenshots or Gifs

Video of route being refreshed. Notice that the route blinks.

Testing

Please describe the manual tests that you ran to verify your changes

• I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
• I have tested via a test drive, or a simulation/mock location app
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have updated the CHANGELOG including this PR

cc: @abhishek1508 @Guardiola31337 @olegzil

[kmadsen]

This is a pain to find. I had to ask in #directions to figure it out
https://mapbox.slack.com/archives/C03KW1JQD/p1583881618015100

[kmadsen]

Updating this feature to be enabled automatically with traffic profiles because that's the only place it's supported.
https://mapbox.slack.com/archives/C03KW1JQD/p1583883018020600?thread_ts=1583881618.015100&cid=C03KW1JQD

[codecov-io]

Codecov Report

Merging #2570 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #2570   +/-   ##
=========================================
  Coverage     29.04%   29.04%           
  Complexity     1062     1062           
=========================================
  Files           281      281           
  Lines         10832    10832           
  Branches        881      881           
=========================================
  Hits           3146     3146           
  Misses         7340     7340           
  Partials        346      346

[Guardiola31337]

This is looking good so far @kmadsen

I've left some comments / questions to discuss before merging here.

Also, it'd be 💯 if we can add an example under the Navigation Core SDK examples showcasing the integration and the client options available and how can customize those (although it seems as it's enabled by default internally there are none 😬 - see my comments below).

[Guardiola31337]

We're enabling refresh by default, is that a good idea? How a client can disable it? Currently (in the legacy) this can be customized

mapbox-navigation-android/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigationOptions.kt

Lines 105 to 112 in 0d4de74

	/**
	* This enables / disables refresh route. If not specified, it's enabled by default.
	*
	* @param enableRefreshRoute whether or not to enable route refresh
	* @return this builder for chaining options together
	*/
	fun enableRefreshRoute(enableRefreshRoute: Boolean) =
	apply { this.enableRefreshRoute = enableRefreshRoute }

[kmadsen]

yeah this was the other option considered
https://github.com/mapbox/mapbox-java/compare/km-add-route-refresh?expand=1

[kmadsen]

but those other options enable or disable route_refresh even if it's not supported

[kmadsen]

this way only enables route refresh if it's supported. the approaches have their pros/cons so I think the issue is picking one

[Guardiola31337]

We're not setting the interval anywhere which means that we're using the default value from MapboxTimer

mapbox-navigation-android/libnavigation-util/src/main/java/com/mapbox/navigation/utils/timer/MapboxTimer.kt

Line 20 in 1656229

var restartAfterMillis = TimeUnit.MINUTES.toMillis(1)

i.e. 1 minute. Currently (in the legacy) by default we try to refresh every 5 minutes (that's supposedly the optimal interval in which we can make sure the data is going to be fresh) https://github.com/mapbox/mapbox-navigation-android/blob/master/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationConstants.kt#L140 although both can be customized from MapboxNavigationOptions

mapbox-navigation-android/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigationOptions.kt

Lines 114 to 121 in 1656229

	/**
	* This sets the route refresh interval. If not specified, the interval is 5 minutes by default.
	*
	* @param intervalInMilliseconds for route refresh
	* @return this builder for chaining options together
	*/
	fun refreshIntervalInMilliseconds(intervalInMilliseconds: Long) =
	apply { this.refreshIntervalInMilliseconds = intervalInMilliseconds }

Also, how can a client can pass in an interval?

[kmadsen]

was considering options here still. but this class can stay the same with each option

• add a function to MapboxNavigation to update the value
• add an option to NavigationOptions
• get the refresh rate from DirectionsResponse

[Guardiola31337]

get the refresh rate from DirectionsResponse

Do we have this available? Is the plan to have it eventually? cc @mapbox/navigation-api

[Guardiola31337]

Is this a good idea? Wondering if we should implement something similar to faster route in which we expose the route to the client and it's up to them to use it or not, instead of us always doing it internally 🤔

[Guardiola31337]

Should we update the MapboxDirectionsSession#routes[0] too 🤔? Actually, updating the MapboxDirectionsSession#routes will end up updating MapboxTripSession#route 👀

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

Lines 440 to 448 in 1656229

	private fun createInternalRoutesObserver() = object : RoutesObserver {
	override fun onRoutesChanged(routes: List<DirectionsRoute>) {
	if (routes.isNotEmpty()) {
	tripSession.route = routes[0]
	} else {
	tripSession.route = null
	}
	}
	}

[Guardiola31337]

Also noticed that we're updating the whole route in NN

mapbox-navigation-android/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt

Line 45 in 1656229

navigator.setRoute(value)

instead of only the annotations as we do in the legacy 👀 RouteHandler. Do you know if this has a performance impact? Probably yes because it's not the same to serialize the whole route rather than only updating the annotations of the current route.

[kmadsen]

looking into update the annotations only now, i did try before but that was early on.

considered performance impacts and considered it minor since it's per minutes (opposed to per seconds)

[kmadsen]

Another note is that legacy doesn't only update the navigator annotations. It also updates the entire route
https://github.com/mapbox/mapbox-navigation-android/blob/master/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/internal/navigation/RouteRefresherCallback.kt#L17

[Guardiola31337]

Nah, that's not correct 👀

mapbox-navigation-android/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/internal/navigation/RouteHandler.kt

Lines 18 to 26 in cd4d864

	} else {
	route.legs()?.let { routeLegs ->
	for (i in routeLegs.indices) {
	routeLegs[i].annotation()?.toJson()?.let { annotationJson ->
	mapboxNavigator.updateAnnotations(annotationJson, INDEX_FIRST_ROUTE, i)
	}
	}
	}
	}

if it's not a NEW_ROUTE

mapbox-navigation-android/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/internal/navigation/RouteRefresherCallback.kt

Line 17 in cd4d864

mapboxNavigation.startNavigation(directionsRoute, DirectionsRouteType.FRESH_ROUTE)

we're only updating the annotations.

[kmadsen]

Yeah discussed offline, now see how fresh_route causes the condition in RouteHandler

[kmadsen]

When attempting to only update annotations in NN, it didn't update the the traffic on the map.

[Guardiola31337]

Thanks for addressing the feedback @kmadsen

We should still think about how clients will enable / disable route refresh feature and how they can tune the refresh interval but we can work on that in a separate PR.

Great work 🍍