-
Notifications
You must be signed in to change notification settings - Fork 319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix route refresh not starting #4831
Conversation
libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt
Show resolved
Hide resolved
@@ -408,7 +408,6 @@ class MapboxNavigation( | |||
tripSession, | |||
logger | |||
) | |||
routeRefreshController.restart() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 like how this way can delete some interactions
?.takeIf { it.isUuidValidForRefresh() } | ||
if (route != null) { | ||
private fun refreshRoute(route: DirectionsRoute) { | ||
val isValid = route.routeOptions()?.enableRefresh() == true && route.isUuidValidForRefresh() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little on the fence of having it come from the external caller. The tripSession is still needed with route progress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should just call routeRefreshController.restart() without the route
RouteRefreshController depends on directionsSession so it can access the primary route on its own.
Also, that reduces the tests you need to add to the overpowered MapboxNavigation.
@@ -576,7 +575,6 @@ class MapboxNavigation( | |||
rerouteController?.interrupt() | |||
routeAlternativesController.interrupt() | |||
directionsSession.setRoutes(routes, initialLegIndex) | |||
routeRefreshController.restart() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
4155ade
to
a53a167
Compare
…nsSession the source of truth
a53a167
to
8b6b1d5
Compare
* fix route refresh not starting * removed route from the TripSession interface to clearly make DirectionsSession the source of truth
* fix route refresh not starting * removed route from the TripSession interface to clearly make DirectionsSession the source of truth
Description
Closes #4829. Fixes a regression potentially from #4755.
A route is not set synchronously in the
MapboxTripSession
anymore, so we cannot use a synchronous getter for it in theRouteRefreshController
.We could potentially use a synchronous getter from
MapboxDirectionsSession
instead as #4830 proposes but it's exposed to the same risk as the previousMapboxTripSession
getter - route setting is run on a callback (RoutesObserver
) and we don't guarantee that the route setting process is synchronous. We could changeMapboxDirectionsSession
to work asynchronously in the future and run into the same exact regression.This PR instead uses the
RoutesObserver
to restart the refresh timer.Changelog