NAVAND-1001: use immediate dispatcher to guarantee that language will change before voice instructions

[dzinad]

No description provided.

[codecov]

Codecov Report

Merging #6766 (6674259) into main (6b92019) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 6674259 differs from pull request most recent head 22804e6. Consider uploading reports for the commit 22804e6 to get more accurate results

@@             Coverage Diff              @@
##               main    #6766      +/-   ##
============================================
+ Coverage     72.50%   72.52%   +0.02%     
+ Complexity     5550     5542       -8     
============================================
  Files           779      777       -2     
  Lines         30069    30042      -27     
  Branches       3547     3548       +1     
============================================
- Hits          21801    21788      -13     
+ Misses         6843     6830      -13     
+ Partials       1425     1424       -1     

Impacted Files	Coverage Δ
...box/navigation/ui/voice/api/MapboxAudioGuidance.kt	84.74% <100.00%> (ø)
...tion/ui/voice/internal/MapboxAudioGuidanceVoice.kt	100.00% <100.00%> (+7.69%)	⬆️
...n/navigationview/NavigationViewListenerRegistry.kt	79.03% <0.00%> (-0.97%)	⬇️
...on/ui/maps/route/arrow/api/MapboxRouteArrowView.kt	83.56% <0.00%> (-0.39%)	⬇️
...ation/ui/maps/route/line/api/VanishingRouteLine.kt	87.12% <0.00%> (ø)
...ation/dropin/map/RouteLineComponentContractImpl.kt	40.00% <0.00%> (ø)
...on/dropin/navigationview/NavigationViewListener.kt	100.00% <0.00%> (ø)
...java/com/mapbox/navigation/dropin/ClickBehavior.kt
...on/dropin/navigationview/NavigationViewBehavior.kt
... and 8 more

[Guardiola31337]

As mentioned in #6758 (comment)

Looking good to me, I've tested downstream in 1TAP with a SNAPSHOT from #6766 and it's working as expected - same first voice instruction is not played twice anymore.

[dzinad]

The first commit is a duplicate of #6758.
@abhishek1508 @kmadsen could you please take a look at this one? Any test cases I can run to check I didn't break anything are also appreciated.

[Zayankovsky]

Looks like the route language is ignored with this PR. I set my device language to Russian, but specified English language when requesting a route, listen to the result:

Screenrecorder-2022-12-21-15-06-54-86.mp4

It tries to speak most of the instructions (which are in English) using Russian voice. This doesn't happen in main or in Tomasz's branch.

[dzinad]

Looks like the route language is ignored with this PR.

That must be because we start pronouncing the instruction in FreeDrive. Then the best way to go would be to guarantee the order.

[dzinad]

Sorry, wrong test case. It works, thanks!

[abhishek1508]

Try rotating the device, you will see the last instruction is being repeated upon device rotation. This doesn't happen on the main branch. Use qa-test-app/Default NavigationView as an example to test it.

@dzinad

[dzinad]

Try rotating the device, you will see the last instruction is being repeated upon device rotation. This doesn't happen on the main branch. Use qa-test-app/Default NavigationView as an example to test it.

Are you sure you've tested this branch? I can't reproduce it neither with qa-test-app nor with 1TAP.

[abhishek1508]

Are you sure you've tested this branch? I can't reproduce it neither with qa-test-app nor with 1TAP.

Yes.

Video from this PR:

PR.mp4

Video from main:

main.mp4

[Zayankovsky]

Try rotating the device, you will see the last instruction is being repeated upon device rotation. This doesn't happen on the main branch. Use qa-test-app/Default NavigationView as an example to test it.

I see the same behavior. Looks like it only happens when replay is enabled.

[kmadsen]

I see the same behavior. Looks like it only happens when replay is enabled.

Yeah this was an issue I was bringing up here https://mapbox.atlassian.net/browse/NAVAND-958?focusedCommentId=71075. Essentially, during device rotation the TripSessionComponent will detach/attach the ReplayRouteSession. ReplayRouteSession stops/starts the trip session

This is my opinion on the approach we should take https://mapbox.atlassian.net/browse/NAVAND-652. Essentially, we should have a service that controls the trip session state (if it is replay, or not). It should have a lifecycle similar to MapboxAudioGuidance, that does not get destroyed during configuration changes.

There are other approaches, but that is out of scope of this change. The issue is specific to drop in ui and the TripSessionComponent/ReplayRouteSession (here is another idea for handling these issues)

EDIT: Here is another solution to the problem I'm discussing here. Right now, replay requires a trip session restart. I wonder if voice instructions would be fixed if we did not have to restart the trip session for replay. There is a public and internal tracker for that idea #6056 https://mapbox.atlassian.net/browse/NAVAND-320. I still think TripSessionComponent should have a longer lifecycle, but trip session issue may improve the experience.

[dzinad]

@abhishek1508 I see the issue with replay enabled. However, I see the same issue on main. Idk why you didn't see it. The video below is from fresh main.

main_duplicate_instruction.mp4

[abhishek1508]

Considering this comment and that issue observed is out of scope of this PR, the changes LGTM.

[kmadsen]

That being said, why not refactor the above classes and either:

We considered changing the constructor because the language is not needed, but changing it is not backwards compatible #4686. So the approaches are limited at the moment.

[tomaszrybakiewicz]

That being said, why not refactor the above classes and either:

We considered changing the constructor because the language is not needed, but changing it is not backwards compatible #4686. So the approaches are limited at the moment.

@kmadsen It all depends on how we introduce those changes. For example, removing the argument for the public constructors will be a breaking change. However, we won't break compatibility if we introduce new primary constructors and refactor (and deprecate) old to secondary.

[kmadsen]

However, we won't break compatibility if we introduce new primary constructors and refactor (and deprecate) old to secondary.

Yeah maybe we should do that to remove the language from the constructor. Another option could be to make new objects and deprecate the old ones.

Thinking about this more, wasn't this issue solved with the introduction of "updateLanguage" functions #6491? We don't need to construct new objects to update the language anymore.

[dzinad]

Thinking about this more, wasn't this issue solved with the introduction of "updateLanguage" functions #6491? We don't need to construct new objects to update the language anymore.

It fixes the issue for non Drop-In users. While for Drop-In users it only avoids recreating some of the objects. Other ones are still being recreated and I think that's what Tomasz means.
I think we can have a deeper look at whether it would be possible to refactor: NAVAND-1041.
But I'm gonna go ahead and merge this one for now.