Route preview iteration 2

[VysotskiVadim]

The second iteration of #6107.

This PR introduces a "routes preview" state which is independent from active guidance. You can independently use two states in parallel. If you want to start from preview and then switch to AG, you have to update both states manually. After all the discussions we had, it's the best approach I can think of. Anyway it's an experimental, so if we find out that API isn't convenient after integration to 1Tap, Drop-In, AA, I can change it laster based on the feedback.

I wanted to address a few goals in this PR:

1. Provide a convenient way for Drop-In UI to sync preview state with AA. Really want to hear feedback from @abhishek1508 , @kmadsen
2. Provide a clear API for the SDK users to implement route preview state, which will replace this example, really want to hear feedback from @dzinad , @RingerJK , @LukasPaczos
3. Provide alternative metadata for preview state, really want to hear feedback from @Zayankovsky , @korshaknn , @Guardiola31337 as it should be useful for displaying labels in 1Tap.

[codecov]

Codecov Report

Merging #6495 (0558e06) into main (166964d) will decrease coverage by 0.00%.
The diff coverage is 69.71%.

@@             Coverage Diff              @@
##               main    #6495      +/-   ##
============================================
- Coverage     70.95%   70.95%   -0.01%     
- Complexity     5084     5117      +33     
============================================
  Files           727      731       +4     
  Lines         28378    28520     +142     
  Branches       3376     3392      +16     
============================================
+ Hits          20136    20235      +99     
- Misses         6972     7008      +36     
- Partials       1270     1277       +7     

Impacted Files	Coverage Δ
...box/navigation/core/NavigationComponentProvider.kt	46.34% <0.00%> (-5.01%)	⬇️
.../navigation/core/preview/NativeRoutesDataParser.kt	0.00% <0.00%> (ø)
...e/routealternatives/RouteAlternativesController.kt	75.37% <ø> (ø)
...ava/com/mapbox/navigation/core/MapboxNavigation.kt	68.87% <23.07%> (-0.99%)	⬇️
...om/mapbox/navigation/core/preview/RoutesPreview.kt	44.00% <44.00%> (ø)
...x/navigation/core/preview/RoutesPreviewObserver.kt	56.25% <56.25%> (ø)
...navigation/core/preview/RoutesPreviewController.kt	97.43% <97.43%> (ø)

[LukasPaczos]

This approach makes sense to me. If we move forward. we might want to consider deprecating MapboxNavigation#getAlternativeMetadataFor and add the metadata to RoutesUpdatedResult for parity with the preview experience, and maybe provide synchronous getters for RoutesUpdatedResult so that the metadata can be obtained on demand (registering an observer and immediately unregistering it works as well but the ergonomics of that are poor).

Additional consideration should be made for interop between setPreviewRoutes and setNavigationRoutes, should they clear each other out when called? What's the general interaction between setPreviewRoutes and the rest of the MapboxNavigation and its related controllers? As things stand, the RoutesPreviewController looks entirely decoupled from MapboxNavigation, so it could live on its own.

In my opinion, the preview experience should not always start with just setting routes, it should start with the route request (fetching the current raw location even when session is not started, map-matching it, dispatching and consuming results). Have you thought about that and how it ties to this controller (and if it even should)?

[VysotskiVadim]

@LukasPaczos , thank you for the feedback ❤️

Additional consideration should be made for interop between setPreviewRoutes and setNavigationRoutes, should they clear each other out when called? What's the general interaction between setPreviewRoutes and the rest of the MapboxNavigation and its related controllers?

I did it in the first version. Mapbox navigation used to be in 1 of two states: preview or active guidance. But it appeared that that we already want to display preview in parallel to active guidance. So current API aims to support 2 flows:

1. Route request -> route preview -> active guidance
2. Route request -> route preview -> active guidance -> active guidance + route preview

To support both flow I implemented route preview to be independent from active guidance, so users can control what and when they want to preview. It maybe too generic, but I would rather release it and wait for some feedback from Drop-In, AA, 1Tap. The first version of the feature is experimental, so we have freedom to adjust API as we go.

In my opinion, the preview experience should not always start with just setting routes, it should start with the route request (fetching the current raw location even when session is not started, map-matching it, dispatching and consuming results). Have you thought about that and how it ties to this controller (and if it even should)?

That's a fair point. But I don't have good ideas of route fetching right now. Instead of discussing and gathering problems/wishes related to route request support I'd rather release what we have and solve the problem of synchronisation between Drop-In and AA. Maybe later somebody from the team get an insight of how to implement a better version of route request.

[kmadsen]

most important is to maintain a constant order for the route preview list, and then have a index. otherwise android auto still needs to build its own route preview state.

Can we please start with a index and a list of routes?

RoutePreview(
  val selectedIndex: Int,
  val routes: List<NavigationRoute>
)

Alternative route metadata is actually arguable for this object, because the metadata can come from another source. The route request part of this is also arguable in this step, it may be necessary in the future - but it is not a requirement from the first iteration because other apis exist (how exactly to handle loading state, how do you bring your own route)

[VysotskiVadim]

most important is to maintain a constant order for the route preview list, and then have a index. otherwise android auto still needs to build its own route preview state.

It's already available but as a secondary option, i.e. routesList are ordered so that primary route is always the first and originalRoutesList saves order and primaryRouteIndex points on the primary route there. I'm not sure if I need to rename that because the other part of the SDK uses ordered list as primary data structure, so I tend to thing that for consistency it's better to keep current naming. @kmadsen , WDYT?

[dzinad]

Is it important what's done first: navigation routes are set or preview routes are removed?

[VysotskiVadim]

that's a good question! I can imagine that it could be important for UI to receive route line update in such an order that it doesn't need to redraw existing route line.
For now it's the SDK's client's responsibility to care about it as it up to the clients how to switch between preview and AG. I will add to PR description which specific cases the route preview covers so that we could think about taking more responsibilities from clients to the SDK without loosing flexibility.

[dzinad]

If you don't like my suggestion about keeping just one list, can we at least remove some properties? For example, do we need to expose primaryRouteIndex if we have primaryRoute?

[kmadsen]

we need to expose primaryRouteIndex

otherwise we'll need to lookup the index with equality compare

[kmadsen]

mapbox-navigation-android/libnavui-androidauto/src/main/java/com/mapbox/androidauto/car/preview/CarRoutePreviewScreen.kt

Line 54 in 27a84ac

private var selectedIndex = 0

example of how the index is needed for list views

[dzinad]

I still think we can only have one list where the first route will always be the primary one.
More details here: #6495 (comment)

[dzinad]

OK, it's also not very clear that the passed route should already be among the routes preview... I'd vote for removing this method at all if there isn't some use case that I'm not seeing.

[dzinad]

Shouldn't the mutex be used here? What if you invoke setRoutesPreviewPrimaryRoute twice in a short time? Or setRoutesPreviewPrimaryRoute and previewNavigationRoutes ? Is the order of updates guaranteed to be the same as the order of invocations?

[VysotskiVadim]

yes, it should! will add that, thank you!

[VysotskiVadim]

actually it shouldn't. controller won't be able to throw exception if it's run asynchronously. I ended up mentioning that this method updates only current state and schedules update in the queue.

[dzinad]

BTW, yes, these methods are asynchronous. Is it OK? I think that the caller would expect to get the updated previewed routes right after they are set.
Maybe yes, since we provide the data via an observer and getRoutesPreview() is just a snapshot of current state. We could also introduce a callback for the method but I'm not sure it's necessary as we already have an observer.

[kmadsen]

This is looking good to me! Thanks for addressing feedback. Let's integrate this into android auto and dropin with separate pull requests.

Please rebase as there are some conflicts and static-analysis to resolve, and then this is should be good to go ✅

[VysotskiVadim]

@DenisPryt , I noticed that metadata which is created using RouteParser.createRoutesData has different alternative id than the one which is created via setRoutes. Do I understand correctly that RouteParser.createRoutesData always creates new id as it probably doesn't track routes from the current navigation session?

[DenisPryt]

As far I know RouteParser.createRoutesData do not assign identifier to the alternatives. It should be 0.

[VysotskiVadim]

thanks. Added this to the docs and instrumentation tests.

[dzinad]

Don't you think that routes preview should be a separate example?

[VysotskiVadim]

no, do you think?

[dzinad]

It's an experimental optional feature and if you add it to "Turn by turn" we won't have the example without it. That's my concern, I'd vote for having 2 different examples for these 2 cases.

[dzinad]

Is it OK that by default you have index 0 while you can pass an empty list?
I mean aren't you trying to solve 2 tasks (set and clear) with 1 method? You don't need an index for an empty list but you provide one.
I know that the same approach is used for legIndex when setting navigation routes. But still I'm not very fond of it.
Not a strong objection, but if you agree we could come up with a better approach and introduce it here.

[VysotskiVadim]

added this feedback to the round 3

[dzinad]

But the user can request routes, remember the list, set it as data source for UI element and continue using this list as data source. I really don't see why SDK should support this, by that significantly complicating the logic.
Moreover, one may think that you can pass any route to this method while you can only pass the one that's present in MapboxNavigation#getRoutesPreview(). I know that it's described in the docs but can't we make the API more strict to reflect that? For example, accept the index, not the route.

[dzinad]

What about the value from getRoutesPreview()?

[dzinad]

You can also check what happens when you invoke changeRoutesPreviewPrimaryRoute after clearing routesPreview.

[dzinad]

What if we call it replaceRoutesPreviewPrimaryRoute? "replace" is used in String and Collection methods to indicate that you replace an existing value.
it's not perfect but slightly better I think.

[VysotskiVadim]

replace in strings are used to replace current value by some external value, so I'm not sure that new option prevent misleading.
We need to somehow highlight the fact, the route should be from the same list.

[dzinad]

Well, yes, but here the first argument is kind of in the method name ("RoutesPreviewPrimaryRoute"). But OK, agreed that one may think that the old value should be passed here. What about replaceRoutesPreviewPrimaryRouteWith?

[VysotskiVadim]

TBH "change" seems less misleading for me than "replace"

[dzinad]

OK, let's leave as-is.

[kmadsen]

making a list of options is helpful sometimes

• changeRoutesPreviewPrimaryRoute
• replaceRoutesPreviewPrimaryRoute
• setRoutesPreviewPrimaryRoute
• selectRoutesPreviewRoute
• setRoutesPreviewRoute
• setRoutePreview (no, too close to setRoutesPreview)

so i'll throw in a vote for setRoutesPreviewRoute, it feels easiest to read and find. it's similar but different enough from setRoutesPreview.

[kmadsen]

Thanks for addressing feedback! Excited to integrate this!