Skip to content

[Android auto] first instruction fix#5825

Merged
korshaknn merged 1 commit intomainfrom
knn_voice_instruction_fix
May 19, 2022
Merged

[Android auto] first instruction fix#5825
korshaknn merged 1 commit intomainfrom
knn_voice_instruction_fix

Conversation

@korshaknn
Copy link
Copy Markdown
Contributor

Description

This PR fixes an issue when the first instruction is not played.
It's related to libnavui-androidauto module only and can be tested with 1Tap.
Also it might be useful to have some activity (or a separate test app?) in examples to test libnavui-androidauto module.

issue details:

  • when routes observer triggered we cancel previous voice instructions flow, create a new flow with a new instructions observer
  • new instructions observer ^^ created too late and we lost the first instruction

changes:

  • start/stop listening for routes, instructions, tripSessionState on attach/detach
  • pass values ^^ to flows stored in MapboxAudioGuidanceServicesImpl

@korshaknn korshaknn self-assigned this May 18, 2022
@korshaknn korshaknn requested a review from a team as a code owner May 18, 2022 12:55
@korshaknn korshaknn force-pushed the knn_voice_instruction_fix branch from 7d8a8a7 to c885a2f Compare May 18, 2022 12:58
@korshaknn
Copy link
Copy Markdown
Contributor Author

not sure about the changelog, do we need it?

@korshaknn korshaknn force-pushed the knn_voice_instruction_fix branch from c885a2f to 184cd4e Compare May 18, 2022 13:00
@LukasPaczos
Copy link
Copy Markdown

not sure about the changelog, do we need it?

We have a separate file in libnavui-androidauto/CHANGELOG.md, could you move the note there?

@korshaknn korshaknn force-pushed the knn_voice_instruction_fix branch from 184cd4e to 7925958 Compare May 18, 2022 13:21
@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2022

Codecov Report

Merging #5825 (3a78af2) into main (4a2600b) will not change coverage.
The diff coverage is n/a.

❗ Current head 3a78af2 differs from pull request most recent head 22417e2. Consider uploading reports for the commit 22417e2 to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #5825   +/-   ##
=========================================
  Coverage     67.63%   67.63%           
  Complexity     3829     3829           
=========================================
  Files           586      586           
  Lines         23583    23583           
  Branches       2715     2715           
=========================================
  Hits          15951    15951           
  Misses         6614     6614           
  Partials       1018     1018           

@korshaknn korshaknn force-pushed the knn_voice_instruction_fix branch from 7925958 to 5fb1c2b Compare May 18, 2022 19:25
}

init {
MapboxNavigationApp.registerObserver(this)
Copy link
Copy Markdown
Contributor

@kmadsen kmadsen May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the fix to convert the underlying flowables into hot flowable? Meaning, they stay subscribed even after the top level is unsubscribed.

This also means that the MapboxAudioGuidance observer cannot be unsubscribed entirely. The developer would have no way to unregister this class besides the MapboxNavigationApp lifecycle ending

Copy link
Copy Markdown
Contributor

@kmadsen kmadsen May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MapboxNavigationApp.unregisterObserver(mapboxAudioGuidance) should also unregister all of the observers it is responsible for.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've considered this issue before but haven't added support for it. I'd like to make it difficult to create leaking subscriptions with MapboxNavigationApp #5829

With 5829, we could turn MapboxVoiceInstructions into an object and make it so it is the only subscriber. In case there are too many issues trying to unregister MapboxNavigation when MapboxNavigationApp.unregisterObserver(mapboxAudioGuidance) is called.

@kmadsen
Copy link
Copy Markdown
Contributor

kmadsen commented May 19, 2022

Also it might be useful to have some activity (or a separate test app?) in examples to test libnavui-androidauto module.

Side note, yeah this is happening. We're consolidating 1tap, drop-in-ui, and android-auto implementations for audio guidance. Essentially, all examples will use the same implementation. Right now it is in transition, sorry the qa-test-app support at the moment is lacking.

Copy link
Copy Markdown
Contributor

@kmadsen kmadsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/mapbox/mapbox-navigation-android/pull/5825/files#r876477888

Sorry it is a confusing because MapboxCarApp controls a single subscription by subscribing and never unsubscribing from MapboxAudioGuidance (at the moment).

MapboxAudioGuidance is not actually a singleton. Doing something like this should not leak subscriptions.


audioGuidance: MapboxAudioGuidance = MapboxAudioGuidanceImpl(...)
MapboxNavigationApp.registerObserver(audioGuidance)
MapboxNavigationApp.unregisterObserver(audioGuidance)
audioGuidance: MapboxAudioGuidance = MapboxAudioGuidanceImpl(...)
MapboxNavigationApp.registerObserver(audioGuidance)
MapboxNavigationApp.unregisterObserver(audioGuidance)

@korshaknn
Copy link
Copy Markdown
Contributor Author

@kmadsen thanks for the feedback!

Is this the fix to convert the underlying flowables into hot flowable? Meaning, they stay subscribed even after the top level is unsubscribed.

yes, we need it because with cold flows we miss the first instruction.

MapboxNavigationApp.unregisterObserver(mapboxAudioGuidance) should also unregister all of the observers it is responsible for.

fixed with

override fun onAttached(mapboxNavigation: MapboxNavigation) {
        mapboxVoiceInstructions.registerObservers(mapboxNavigation)
        job = scope.launch { audioGuidanceFlow(mapboxNavigation).collect() }
}

override fun onDetached(mapboxNavigation: MapboxNavigation) {
        mapboxVoiceInstructions.unregisterObservers(mapboxNavigation)
        job?.cancel()
        job = null
}

also MapboxVoiceInstructions is not a MapboxNavigationObserver anymore, so we need to manually register/unregister observers like we do it in MapboxAudioGuidanceImpl.
I think we should be good with it at least for now, MapboxVoiceInstructions is only used in MapboxAudioGuidanceImpl.

Copy link
Copy Markdown
Contributor

@kmadsen kmadsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@korshaknn korshaknn force-pushed the knn_voice_instruction_fix branch from 3a78af2 to 22417e2 Compare May 19, 2022 16:06
@korshaknn korshaknn merged commit 8472f8a into main May 19, 2022
@korshaknn korshaknn deleted the knn_voice_instruction_fix branch May 19, 2022 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants