Remove automatic setRoutes from RouteAlternativesController

[kmadsen]

Description

Following discussion from here #4892
We also waited for the Native Router to merge #4791

There are a few known issues where we are not ready to automatically set the route inside the alternative route controller

• Route alternatives breaks route refresh if you set the route from nav-native
• Nav-native's RouteAlternativesObserver does not provide the RouterOrigin
• Taking an alternative route still triggers a reroute
• The alternative is not removed when the fork is at the origin

These need to be fixed by Nav-Native in order to be able to update the routes continuously.

If we remove the directionsSession.setRoutes( call. Nav-native will only be notifying the SDK of route alternative changes. And then the RouteAlternativesObserver will allow downstream developers decide how they want to select the routes suggested.

Changelog

<changelog>Remove automatic setRoute from route alternatives controller</changelog>

[LukasPaczos]

Could you also update the AlternativeRouteActivity to correctly set back the alternatives to MapboxNavigation when they become available?

[LukasPaczos]

This check does not seem correct to me. We would report ROUTES_UPDATE_REASON_ALTERNATIVE only if one of the newly set routes was generated by the RouteAlternativesController but why do new need to limit ourselves this way?

What about this check:

routes.isEmpty() -> {
  RoutesExtra.ROUTES_UPDATE_REASON_CLEAN_UP
}
routes.first() == directionsSession.routes.firstOrNull() -> {
  RoutesExtra.ROUTES_UPDATE_REASON_ALTERNATIVE
}
else -> {
  RoutesExtra.ROUTES_UPDATE_REASON_NEW
}

We would report alternatives are updated whenever the primary route has not changed.

[LukasPaczos]

cc @RingerJK

[kmadsen]

I wasn't creative here, this is bringing back the implementation which was in place before and shipped with 2.0.0
https://github.com/mapbox/mapbox-navigation-android/pull/4892/files#diff-fabf10fcd842f15a333f539689309f950d861096e5f313dbe2b61e771b69a8d6L638-L640

routes.first() == directionsSession.routes.firstOrNull() -> {
RoutesExtra.ROUTES_UPDATE_REASON_ALTERNATIVE
}

This does not work when you change routes.

1. set a route
2. set a new route, will have reason_alternative

I think this scenario should be ROUTES_UPDATE_REASON_NEW

[RingerJK]

• What if a routes' list is not changed at all -> looks like update should be skipped
• What if all routes are replaced by alternatives routes from AlternativeRoutesController -> should be ROUTES_UPDATE_REASON_ALTERNATIVE? According to your suggestion, it will be REASON_NEW

[kmadsen]

hmm but the point @LukasPaczos is making is not to capture the extra as a "source for the new routes". but as a way to capture if the primary route has changed, vs if alternative routes has changed

[LukasPaczos]

If we'd like to reintroduce this logic as is, we'd need to reintroduce telemetry changes discussed in #4975 (comment) as well which show that this approach might be too complex and ambiguous.

Since our reason names are so general, I'm proposing to introduce a simpler rule for MapboxNavigation#setRoutes:

if routes are cleared -> ROUTES_UPDATE_REASON_CLEAN_UP
if the primary route changes -> ROUTES_UPDATE_REASON_NEW
if primary does not change -> ROUTES_UPDATE_REASON_ALTERNATIVE
if nothing changes -> abort

Additionally, we should update the docs for each of these constants to make this relationship clear. It's not perfect by any means, ideally we should have even more states which we could consider introducing in a separate PR. This proposal is just to simplify the logic and not break telemetry impl.

[kmadsen]

The issue this introduces is, when the RouteAlternativesController would suggest to re-route on an alternative route. But I think it's fair to say that experience has not been handled

#5039

[korshaknn]

why we immediately request alternatives? shouldn't we just wait for a callback from NN after the interval?

[kmadsen]

Right now we always have 1 primary route and 1 alternative route. When you pass an alternative it is removed. If we wait for the interval, there will be ~minutes where you won't know any alternatives.

Triggering a request here, makes it so you always have an alternative showing while routing. But I do think NN will eventually trigger calls out of interval to create the desired experience.

So I'd like to leave it in because it tests the public function and it creates a desired experience.

[Guardiola31337]

Noting that Ktlint failed 👀

> Task :libnavigation-core:ktlint
/root/code/libnavigation-core/src/main/java/com/******/navigation/core/routealternatives/RouteAlternativesController.kt:4:1: Unused import
/root/code/libnavigation-core/src/main/java/com/******/navigation/core/routealternatives/RouteAlternativesController.kt:6:1: Unused import
/root/code/libnavigation-core/src/main/java/com/******/navigation/core/routealternatives/RouteAlternativesController.kt:11:1: Unused import
/root/code/libnavigation-core/src/main/java/com/******/navigation/core/routealternatives/RouteAlternativesController.kt:12:1: Unused import

> Task :libnavigation-core:ktlint FAILED

[korshaknn]

val alternatives = routeAlternatives.map { routeAlternative ->

should be parsed as DirectionsResponse

#5036 (comment)