Conversation
…egIndex for voice instructions
RingerJK
force-pushed
the
kyv-NAVAND-918-fix-voice-instructions
branch
from
4df8130 to
d505c6c
Compare
Codecov Report
@@ Coverage Diff @@
## main #6689 +/- ##
=========================================
Coverage 72.41% 72.41%
- Complexity 5404 5405 +1
=========================================
Files 764 764
Lines 29386 29391 +5
Branches 3487 3488 +1
=========================================
+ Hits 21280 21284 +4
Misses 6715 6715
- Partials 1391 1392 +1
|
| ) { | ||
| bannerInstructionEvent.invalidateLatestBannerInstructions(latestInstructionWrapper) | ||
| lastVoiceInstruction = null | ||
| if (lastVoiceInstruction == voiceInstruction) { |
There was a problem hiding this comment.
I think we now have a case when the index won't be set to null but the voice instruction will.
The index will be null if latestInstructionWrapper is equal to saved one. But latestInstructionWrapper is about banner instruction and index. So can there be the following case:
- banner instruction is updated;
- index is not updated;
- voice instruction is not updated;
- we invoke
invalidateLatestInstructions; - result: banner instruction is not null, voice instruction is null?
I'm not sure, maybe this just can't happen. just double-checking.
There was a problem hiding this comment.
that actually a good point. It seems not critical for the voice instruction as it's pronounced once.
Let's cut a ticket to address the issue separately. It looks like refactoring task, along with https://github.com/mapbox/mapbox-navigation-android/pull/6689/files#r1041952357
| private fun invalidateLatestInstructions( | ||
| latestInstructionWrapper: BannerInstructionEvent.LatestInstructionWrapper? | ||
| latestInstructionWrapper: BannerInstructionEvent.LatestInstructionWrapper?, | ||
| voiceInstruction: VoiceInstructions?, |
There was a problem hiding this comment.
Is it right for the index to be wrapped in banner instruction event if it's responsible both for banner and voice instruction?
I'd keep them all together.
There was a problem hiding this comment.
The index is taken from BannerInstruction#getIndex and later the SDK re-uses that index value for voice instruction as well. Not sure why it's done so. (the index of voice and banner instruction must be the same, but why it's taken from BannerInstruction#getIndex is not clear)
@LukasPaczos do you have the context?
We can address the issue separately
3 participants
Description
Addressing same fix #6684 but for
VoiceInstructionsthanks to @VysotskiVadim