Update audio guidance for AA compatibility

[kmadsen]

Description

Fixes NAVAND-628

AA and the nav-sdk work on different release channels. In order to support long term backwards compatibility, AA will only use public apis from the nav-sdk.

MapboxAudioGuidance is an internal api, so this moves it out into the public. Looking to make this part of the 2.9 release train, and the alpha release starts today.

Some related issues and pull requests that this change is addressing

• [Android Auto] QA-Test-App Crash Fix #6316
• Fix qa test app crash #6320
• Unblock android auto releases #6304
• Unblock android auto releases 2 #6305
• Make SharedApp backwards compatible and removable #6303

Code samples

Here are some samples of the interactions.

// If MapboxAudioGuidance is not available, create it and register it. This allows for
// different systems to share an instance of MapboxAudioGuidance
val audioGuidance = MapboxAudioGuidance.getInstance()

Lower level interactions are available, but you need to know what you're doing.

// 1. Get and/or enable audio guidance
if (MapboxNavigationApp.getObservers(MapboxAudioGuidance::class).isEmpty()) {. 
  MapboxNavigationApp.registerObserver(MapboxAudioGuidance.create())
}

// 2. Use the audio guidance service -- assumes it has been registered or else it will crash
MapboxNavigationApp.getObserver(MapboxAudioGuidance::class).mute()

// 3. Remove it - code like 1 may re-register. code like 2 will crash if there are components that assume it's registered
MapboxNavigationApp.getObservers(MapboxAudioGuidance::class)?.firstOrNull()?.let {
  MapboxNavigationApp.unregisterObserver(it)
}

[kmadsen]

EDIT: Solution here is to temporarily depend on the project rather than the version api project(":libnavigation-android")

Running into an issue from this approach #6316

It works fine while debugging but the incompatibility is still caught

./gradlew libnavui-androidauto:jacocoTestReport

> Task :libnavui-androidauto:compileReleaseKotlin FAILED
e: .../mapbox/androidauto/MapboxCarApp.kt: (7, 43): Unresolved reference: MapboxAudioGuidance
e: .../mapbox/androidauto/MapboxCarApp.kt: (32, 39): Unresolved reference: MapboxAudioGuidance
e: .../mapbox/androidauto/MapboxCarApp.kt: (33, 29): Not enough information to infer type variable T
e: .../mapbox/androidauto/MapboxCarApp.kt: (33, 41): Unresolved reference: MapboxAudioGuidance
e: .../mapbox/androidauto/MapboxCarApp.kt: (47, 33): Not enough information to infer type variable T
e: .../mapbox/androidauto/MapboxCarApp.kt: (47, 46): Unresolved reference: MapboxAudioGuidance
e: .../mapbox/androidauto/MapboxCarApp.kt: (48, 50): Unresolved reference: MapboxAudioGuidance
e: .../mapbox/androidauto/navigation/audioguidance/AppAudioGuidanceUi.kt: (11, 43): Unresolved reference: MapboxAudioGuidanceState
e: .../mapbox/androidauto/navigation/audioguidance/AppAudioGuidanceUi.kt: (43, 36): Unresolved reference: MapboxAudioGuidanceState
e: .../mapbox/androidauto/navigation/audioguidance/AppAudioGuidanceUi.kt: (45, 13): Not enough information to infer type variable R
e: .../mapbox/androidauto/navigation/audioguidance/AppAudioGuidanceUi.kt: (46, 17): Variable expected

[kmadsen]

Converting to a draft because we need to release a new version of andriodauto before we can proceed here

It looks like we also need to make an incompatible change in order to handle the nav-sdk changes. In other words, after this merges we will need a nav-sdk release so that AA can become compatible. Migrating to public apis should make it so this is the last time we need to do this.

Will open this one back up for review after this #6343. That way we can start working on 2.9.0-alpha

[codecov]

Codecov Report

Merging #6336 (298f80d) into main (f54719c) will increase coverage by 0.03%.
The diff coverage is 63.63%.

@@             Coverage Diff              @@
##               main    #6336      +/-   ##
============================================
+ Coverage     68.88%   68.91%   +0.03%     
- Complexity     4504     4517      +13     
============================================
  Files           681      682       +1     
  Lines         27002    27010       +8     
  Branches       3162     3168       +6     
============================================
+ Hits          18601    18615      +14     
+ Misses         7182     7176       -6     
  Partials       1219     1219              

Impacted Files	Coverage Δ
...com/mapbox/navigation/ui/app/internal/SharedApp.kt	0.00% <ø> (ø)
...ava/com/mapbox/navigation/dropin/NavigationView.kt	0.00% <0.00%> (ø)
...avigation/ui/voice/installer/ComponentInstaller.kt	0.00% <ø> (ø)
...voice/internal/impl/MapboxAudioGuidanceServices.kt	12.50% <50.00%> (ø)
.../voice/internal/ui/AudioGuidanceButtonComponent.kt	93.54% <50.00%> (-0.21%)	⬇️
...avigation/ui/voice/api/MapboxAudioGuidanceState.kt	54.16% <54.16%> (ø)
...box/navigation/ui/voice/api/MapboxAudioGuidance.kt	79.68% <91.66%> (ø)
...nternal/controller/AudioGuidanceStateController.kt	77.27% <100.00%> (ø)

[tomaszrybakiewicz]

Great initiative. Left a couple of comments.

While we are on the topic of the MapboxAudioGuidance interface, what do you think about simplifying it to just?

interface MapboxAudioGuidance : MapboxNavigationObserver {
    
    val state: StateFlow<State>
    
    fun setMuted(muted: Boolean)

    /**
     * Accessed through [state].
     */
    interface State {
        /**
         * When the trip session has started, and there is an active route.
         */
        val isPlayable: Boolean

        /**
         * Unrelated to the navigation state, provides state of
         * the controls [mute] [unmute] [toggle]
         */
        val isMuted: Boolean

        /**
         * Once a voice instruction becomes available this will not be null.
         * When the state [isPlayable] and this is null, it means there are no voice instructions
         * on the route at this time.
         */
        val voiceInstructions: VoiceInstructions?

        /**
         * After a [voiceInstructions] has been announced, this value will be emitted.
         * This will always be null when [isMuted] is true.
         */
        val speechAnnouncement: SpeechAnnouncement?
    }
}

[tomaszrybakiewicz]

Let's make all constructors internal and offer a factory to instantiate the default MapboxAudioGuidance implementation. Doing so will allow us to replace MapboxAudioGuidanceImpl when needed without breaking compatibility.

object MapboxAudioGuidanceProvider {
    fun create(): MapboxAudioGuidance =
        MapboxAudioGuidanceImpl(MapboxAudioGuidanceServices(), Dispatchers.Main)
}

[kmadsen]

that's an idea but i like the empty constructor for public api. this will still allow us to delegate the internals when/if we want. But the MapboxAudioGuidanceImpl is deleted after merging the interface. All for better backwards compatibility in the public api.

[kmadsen]

@tomaszrybakiewicz how about this, added a couple companion functions to make it clear what the intended usage is. Moved the constructor to private

MapboxAudioGuidance {
..
companion object {
    fun create(): MapboxAudioGuidance ...

    fun getInstance(): MapboxAudioGuidance = MapboxNavigationApp
            .getObservers(MapboxAudioGuidance::class)
            .firstOrNull() ?: create()
            .also { MapboxNavigationApp.registerObserver(it) }

[kmadsen]

While we are on the topic of the MapboxAudioGuidance interface, what do you think about simplifying it to just?

Removing the interface in favor of a class to be able to handle backwards compatibility. Customization will be handled by adding new functions rather than proposing inheritance. Which is more consistent with the rest of the sdk.

As for the change from fun setMuted(muted: Boolean) vs mute and unmute. These definitions were built to be consistent with other apis. Change the definition in one place we change the definition everywhere, so I really prefer leaving it as is in this pull request.

mapbox-navigation-android/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/view/MapboxAudioGuidanceButton.kt

Lines 117 to 130 in 8df22ed

	fun mute() {
	iconImage.setImageResource(muteIconResId)
	}
	/**
	* Update this button to represent audio guidance state in un-muted state.
	* This method does nothing if button is already in given state.
	*
	* @param state new camera state
	*/
	@UiThread
	fun unMute() {
	iconImage.setImageResource(unMuteIconResId)
	}

[abhishek1508]

When running this branch and starting MapboxNavigationViewCustomizedActivity, the QA-Test-App is crashing for me

 Caused by: java.lang.IllegalStateException: Class MapboxAudioGuidance is not been registered to MapboxNavigationApp
        at com.mapbox.navigation.core.lifecycle.MapboxNavigationOwner.getObserver(MapboxNavigationOwner.kt:78)
        at com.mapbox.navigation.core.lifecycle.MapboxNavigationOwner.getObserver(MapboxNavigationOwner.kt:72)
        at com.mapbox.navigation.core.lifecycle.MapboxNavigationAppDelegate.getObserver(MapboxNavigationAppDelegate.kt:69)
        at com.mapbox.navigation.core.lifecycle.MapboxNavigationApp.getObserver(MapboxNavigationApp.kt:181)
        at com.mapbox.navigation.ui.app.internal.controller.AudioGuidanceStateController.onAttached(AudioGuidanceStateController.kt:50)
        at com.mapbox.navigation.core.lifecycle.MapboxNavigationOwner$carAppLifecycleObserver$1.onStart(MapboxNavigationOwner.kt:31)
        at androidx.lifecycle.FullLifecycleObserverAdapter.onStateChanged(FullLifecycleObserverAdapter.java:39)
        at androidx.lifecycle.LifecycleRegistry$ObserverWithState.dispatchEvent(LifecycleRegistry.java:354)
        at androidx.lifecycle.LifecycleRegistry.addObserver(LifecycleRegistry.java:196)
        at com.mapbox.navigation.core.lifecycle.MapboxNavigationAppDelegate.setup(MapboxNavigationAppDelegate.kt:31)
        at com.mapbox.navigation.core.lifecycle.MapboxNavigationApp.setup(MapboxNavigationApp.kt:89)
        at com.mapbox.navigation.dropin.NavigationView.<init>(NavigationView.kt:113)
        at com.mapbox.navigation.dropin.NavigationView.<init>(NavigationView.kt:77)
        at com.mapbox.navigation.dropin.NavigationView.<init>(Unknown Source:13)

cc @kmadsen

[abhishek1508]

Out of curiosity, we are introducing MapboxAudioGuidanceState in libnavui-voice, but we also have AudioGuidanceState in libnavui-app. Why do we have 2 states for audio guidance?

@kmadsen @tomaszrybakiewicz

[kmadsen]

When running this branch and starting MapboxNavigationViewCustomizedActivity, the QA-Test-App is crashing for me

Thanks for catching! I didn't fix AudioGuidanceStateController

- MapboxNavigationApp.getObserver(MapboxAudioGuidance::class)
+ MapboxAudioGuidance.getInstance()

Using MapboxAudioGuidance.getInstance() will ensure MapboxAudioGuidance is available and subscribed to MapboxNavigationApp

[kmadsen]

Out of curiosity, we are introducing MapboxAudioGuidanceState in libnavui-voice, but we also have AudioGuidanceState in libnavui-app. Why do we have 2 states for audio guidance?

I'm not seeing a need for the AudioGuidanceState either.

[tomaszrybakiewicz]

Out of curiosity, we are introducing MapboxAudioGuidanceState in libnavui-voice, but we also have AudioGuidanceState in libnavui-app. Why do we have 2 states for audio guidance?

@kmadsen @tomaszrybakiewicz

@abhishek1508 Not entirely sure why AudioGuidanceState was introduced in libnavui-app in the first place. Maybe it was accidentally merged.
We used to have only one class to represent that state in libnavui-voice.

Refs to previous work:
[Drop-In UI]: Integrate AudioGuidanceComponent with AA (1/2)
[Android Auto] Integrate AudioGuidanceComponent with AA (2/2)