- Added Waypoints to NavigationRoute; - RouteOptionsUpdater refactored;

[RingerJK]

Description

DO NOT MERGE: targeting to v2.8.0-alpha.1

[codecov[bot]]

Codecov Report

Merging #6005 (0636da9) into main (5e27577) will decrease coverage by 0.12%. The diff coverage is 62.22%.

:exclamation: Current head 0636da9 differs from pull request most recent head 8270348. Consider uploading reports for the commit 8270348 to get more accurate results

@@             Coverage Diff              @@
##               main    #6005      +/-   ##
============================================
- Coverage     70.35%   70.22%   -0.13%     
- Complexity     4913     4914       +1     
============================================
  Files           718      722       +4     
  Lines         27980    28023      +43     
  Branches       3299     3303       +4     
============================================
- Hits          19685    19680       -5     
- Misses         7017     7068      +51     
+ Partials       1278     1275       -3     

Impacted Files	Coverage Δ
...avigation/base/internal/utils/DirectionsRouteEx.kt	40.00% <0.00%> (-36.93%)	:arrow_down:
.../mapbox/navigation/base/internal/utils/RouterEx.kt	45.45% <0.00%> (-4.55%)	:arrow_down:
.../navigation/base/internal/utils/WaypointFactory.kt	0.00% <0.00%> (ø)
...mapbox/navigation/base/trip/model/RouteProgress.kt	0.00% <ø> (ø)
...pbox/navigation/core/accounts/BillingController.kt	88.09% <ø> (ø)
...avigation/core/history/model/HistoryEventMapper.kt	0.00% <0.00%> (ø)
.../mapbox/navigation/base/internal/route/Waypoint.kt	11.53% <11.53%> (ø)
...x/navigation/ui/maps/building/BuildingProcessor.kt	68.00% <62.50%> (+2.37%)	:arrow_up:
...avigation/core/routeoptions/RouteOptionsUpdater.kt	83.33% <85.05%> (-4.44%)	:arrow_down:
.../navigation/base/internal/extensions/WaypointEx.kt	94.73% <94.73%> (ø)
... and 13 more

[dzinad]

Which ticket is it?

[RingerJK]

Which ticket is it?

Added ticket in the title

[dzinad]

A more general concern: RouteProgress is public API. And it has remainingWaypoints property.

1. Docs should be updated to specify which waypoints these are;
2. An idea: would it make sense to add another field for the other type of waypoints? My concern is that we might have to apply the logic that's in RouteOptionsUpdater somewhere else if we use RouteProgress#remainingWaypoints somewhere else. Also the end user can use this property. Now that the waypoints are of different types, it would make sense to allow the user to choose which ones they'd like to use. TBH I don't really understand what's happening in RouteOptionsUpdater. What's refactoring and what's logic change. IMO it would be nice to separate: the logic change would be in calculateRemainingWaypoints or somewhere near it. WDYT?

[dzinad]

BillingController uses RouteProgress#remainingWaypoints. Should its logic stay the same? I'd think yes, but want to clarify.

[RingerJK]

@dzinad

1. Docs should be updated to specify which waypoints these are;

if we are speaking about Waypoint.kt - I'll do it 👍

2. An idea: would it make sense to add another field for the other type of waypoints? My concern is that we might have to apply the logic that's in RouteOptionsUpdater somewhere else if we use RouteProgress#remainingWaypoints somewhere else. Also the end user can use this property. Now that the waypoints are of different types, it would make sense to allow the user to choose which ones they'd like to use. TBH I don't really understand what's happening in RouteOptionsUpdater. What's refactoring and what's logic change. IMO it would be nice to separate: the logic change would be in calculateRemainingWaypoints or somewhere near it. WDYT?

The Waypoint class is internal itself so we're good to experiment and change it.

• What fields would you have in mind to add for other types of waypoints?
• how do you see customers might choose the particular type of waypoints? For now, they are mostly like passive observer
• Yeah, I agree, the logic is a bit hard to get in RouteOptionsUpdater. Would it help if I move everything into separate functions?

BillingController uses RouteProgress#remainingWaypoints. Should its logic stay the same? I'd think yes, but want to clarify.

great catch actually. I would say it should be switched to the Waypoint as well (I see there's using DirecionsResponce#DirectionsWaypoint. @LukasPaczos do you have objections?

[LukasPaczos]

We can switch the billing controller to use Waypoint but it should ignore silent waypoints, it's only interested in leg waypoints.

[LukasPaczos]

great catch actually. I would say it should be switched to the Waypoint as well (I see there's using DirecionsResponce#DirectionsWaypoint. @LukasPaczos do you have objections?

We can switch the billing controller to use Waypoint but it should ignore silent waypoints, it's only interested in leg waypoints.

Sorry, this always surprises me time and time again that DirectionsResponse#waypoints contains silent waypoints. Since DirectionsResponse#waypoints contains all of regular, silent, and EV waypoints and RouteProgress#remainingWaypoints is already updated to account for EV waypoints, then we shouldn't need to make any changes in the billing controller. Unless I'm missing something @RingerJK?

[dzinad]

if we are speaking about Waypoint.kt - I'll do it 👍

No, I was talking about RouteProgress#remainingWaypoints doc.

The Waypoint class is internal itself so we're good to experiment and change it.

Again, I was talking about RouteProgress.

What fields would you have in mind to add for other types of waypoints? how do you see customers might choose the particular type of waypoints? For now, they are mostly like passive observer

Requested waypoints and all waypoints? I don't know how they are used now. As far as I understood from the ticket description, the problem is that now in the response we may have more waypoints than in the request. Who knows hot this data will be used by users, why not expose both numbers?

Yeah, I agree, the logic is a bit hard to get in RouteOptionsUpdater. Would it help if I move everything into separate functions?

Nah, not really. Let's leave as is.

[RingerJK]

Sorry, this always surprises me time and time again that DirectionsResponse#waypoints contains silent waypoints. Since DirectionsResponse#waypoints contains all of regular, silent, and EV waypoints and RouteProgress#remainingWaypoints is already updated to account for EV waypoints, then we shouldn't need to make any changes in the billing controller. Unless I'm missing something @RingerJK?

I would say that for now, we can keep it as is, and if the Waypoint experiment becomes a success we can switch to that as a single source of truth.

we also need utils methods for us and customers to work with waypoints, as it's always a challenge to handle them right (for instance getNextRequestedCoordinate might be hard to make on the application side)

@LukasPaczos I think it's a typo in the doc, shouldn't be silent, right? https://github.com/mapbox/mapbox-navigation-android/blob/934e668205bf7688734db9ff00b199edd5055044/libnavigation-core/src/main/java/com/mapbox/navigation/core/accounts/BillingController.kt#L395-L403

[LukasPaczos]

I think it's a typo in the doc, shouldn't be silent, right?

Yes :+1:

[RingerJK]

latest updates:

• cut NAVAND-825 to address https://github.com/mapbox/mapbox-navigation-android/pull/6005#issuecomment-1292548529. Let's discuss if we want to expose it in the ticket
• fixed docs in RouteProgress and BillingController;
• added skip changelog, as it fails for my changes.

[dzinad]

LGTM but it'd be great to add an instrumentation test that verifies a re-route with EV routes.

Oh, I was going to add the test in https://mapbox.atlassian.net/browse/NAVAND-775. It's about alternatives but there is no basic test. It would be real nice if it appeared. :)

[RingerJK]

We should be in a good place with the implementation (and unit tests) and this lands only in the alpha but let's prioritize the instrumentation tests right after this.

NAVAND-835