Fix "SetRoutesReason" when alternative route is chosen as a primary. #7464
Conversation
ChangelogFeaturesBug fixes and improvements
Known issues
|
Codecov Report
@@ Coverage Diff @@
## main #7464 +/- ##
============================================
- Coverage 74.13% 74.12% -0.01%
- Complexity 6141 6143 +2
============================================
Files 829 829
Lines 32883 32894 +11
Branches 3916 3921 +5
============================================
+ Hits 24377 24383 +6
- Misses 6973 6977 +4
- Partials 1533 1534 +1
|
| routes.first() == directionsSession.routes.firstOrNull() -> { | ||
| SetRoutes.Alternatives(initialLegIndex, isPrimaryRouteChanged = false) | ||
| } | ||
| routes.map { it.id }.sorted() == directionsSession.routes.map { it.id }.sorted() -> { |
There was a problem hiding this comment.
That's not reason "alternatives". This reason is not only used when setting routes to Native Navigator, but also:
- In telemetry;
- As reason in RoutesUpdatedResult (public API);
- In some internal logic (but looks like it is adjusted).
But now you're trying to set reason alternatives where from our perspective it's not actually "alternatives". The main problem is the misunderstanding between us and NN, do we know where that came from?
Anyway, even if we leave this misalignment, I'd suggest introducing a new internal reason (like "Reorder"). And it would get mapped to native "alternative" and our "new" reasons correspondingly.
BTW, what if we had routes [A, B, C] and now we have [B, A]? For us it's reason "new". What about NN? What do they expect?
There was a problem hiding this comment.
As reason in RoutesUpdatedResult (public API)
It affects only mapping to RoutesUpdatedResult.RoutesUpdateReason (we haven't introduced any new value for RoutesUpdateReason), public api is not broken.
The main problem is the misunderstanding between us and NN, do we know where that came from?
This behavior is expected by the NN, and now it's consistent with the iOS Navigation SDK (I'll send you a link to a discussion).
Anyway, even if we leave this misalignment, I'd suggest introducing a new internal reason (like "Reorder"). And it would get mapped to native "alternative" and our "new" reasons correspondingly.
Makes sense, thank you!. I've updated the PR.
BTW, what if we had routes [A, B, C] and now we have [B, A]? For us it's reason "new". What about NN? What do they expect?
It's seems that the reason new is expected, but we can make sure with the NN team.
DzmitryFomchyn
force-pushed
the
NAVAND-1338-route-set-reason-fix
branch
from
40b3bed to
cb78522
Compare
libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/SetRoutes.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/internal/utils/SetRoutesEx.kt
Outdated
Show resolved
Hide resolved
| /** | ||
| * Triggered when routes list remain the same but **alternative route has been chosen as a primary**. | ||
| */ | ||
| internal data class Reorder(val legIndex: Int) : SetRoutes() |
There was a problem hiding this comment.
[NIT] As I understood it's an internal name which will be mapped to a different name for NN and SDK users. Maybe we can change it to be the same as comments above says:
| internal data class Reorder(val legIndex: Int) : SetRoutes() | |
| internal data class SwitchToAlternativeRoute(val legIndex: Int) : SetRoutes() |
?
There was a problem hiding this comment.
After new discussion with NN team, this type is used when primary route changed and previous primary became alternative. As for me, both names Reorder and SwitchToAlternativeRoute can be used here, but the second option sounds very similar to Alternatives. What do you think?
There was a problem hiding this comment.
Reorder doesn't reflect the fact that primary route changed. You could have had [A, B, C] and then [A, C, B]. That sounds like a reorder to me, but it's not.
So IMO SwitchToAlternative suits better here.
There was a problem hiding this comment.
ok, let's leave it as is for now in order not to wait for PR checks before release. I'll fix it as a follow-up PR
DzmitryFomchyn
force-pushed
the
NAVAND-1338-route-set-reason-fix
branch
2 times, most recently
from
187b365 to
7a2e956
Compare
DzmitryFomchyn
added
the
skip changelog
DzmitryFomchyn
force-pushed
the
NAVAND-1338-route-set-reason-fix
branch
from
7a2e956 to
c600232
Compare
| SetRoutes.Alternatives(initialLegIndex) | ||
| } | ||
| directionsSession.routes.map { it.id }.contains(routes.first().id) -> { |
There was a problem hiding this comment.
The result of execution of this branch relies on the fact that if it's just the alternatives being reordered, we won't go to this branch (because we'll stop on the previous one).
I'm not a fan of this approach, because the result here depends on the branch order, and here it's not super obvious that the order matters. With if-else it's more clear than with when, although that could be just a personal preference.
Anyway, if anyone else from you finds this point valid, let's maybe make it more explicit here. @DzmitryFomchyn @VysotskiVadim
Description
This PR fixes
SetRoutesReasonwhen alternative route is chosen as a primary.