Make SharedApp backwards compatible and removable #6303

kmadsen · 2022-09-09T16:35:33Z

Description

Resolves #6253

There is an incompatibility with SharedApp where it can only be setup once and it is the only opportunity to specify custom services https://github.com/mapbox/mapbox-navigation-android/pull/6213/files#r964971123

Every time the SharedApp.setup function changes it will break compatibility with anything that uses it. Converting the SharedApp to a MapboxNavigationObserver to resolve it and removing customizability so it can stay backwards compatible.

On the topic of, creating custom services like MapboxAudioGuidance. We should be discussing public api options for how to do so, that way the SharedApp will not break compatibility with AA. Am also considering that custom services can be swapped with MapboxNavigationApp.unregisterObserver and MapboxNavigationApp.registerObserver, which are going to be forever backwards compatible and they are public.

There is no public interface for declaring custom services so it should be removed from this SharedApp.setup function.

CHANGELOG.md

codecov · 2022-09-09T17:21:58Z

Codecov Report

Merging #6303 (c9a1859) into main (c9a1859) will not change coverage.
The diff coverage is n/a.

❗ Current head c9a1859 differs from pull request most recent head 50ad972. Consider uploading reports for the commit 50ad972 to get more accurate results

@@            Coverage Diff            @@
##               main    #6303   +/-   ##
=========================================
  Coverage     68.89%   68.89%           
  Complexity     4454     4454           
=========================================
  Files           668      668           
  Lines         26688    26688           
  Branches       3147     3147           
=========================================
  Hits          18387    18387           
  Misses         7093     7093           
  Partials       1208     1208

kmadsen · 2022-09-09T17:32:26Z

EDIT: solution was to register the MapboxAudioGuidance before the AudioGuidanceStateController

instrumentation test ran into this

Process: com.mapbox.navigation.instrumentation_tests, PID: 4267
java.lang.IllegalStateException: Class MapboxAudioGuidance is not been registered to MapboxNavigationApp
	at com.mapbox.navigation.core.lifecycle.MapboxNavigationOwner.getObserver(MapboxNavigationOwner.kt:75)
	at com.mapbox.navigation.core.lifecycle.MapboxNavigationOwner.getObserver(MapboxNavigationOwner.kt:70)
	at com.mapbox.navigation.core.lifecycle.MapboxNavigationAppDelegate.getObserver(MapboxNavigationAppDelegate.kt:66)
	at com.mapbox.navigation.core.lifecycle.MapboxNavigationApp.getObserver(MapboxNavigationApp.kt:168)
	at com.mapbox.navigation.ui.app.internal.controller.AudioGuidanceStateController.onAttached(AudioGuidanceStateController.kt:50)
	at com.mapbox.navigation.core.lifecycle.MapboxNavigationOwner.register(MapboxNavigationOwner.kt:48)
	at com.mapbox.navigation.core.lifecycle.MapboxNavigationAppDelegate.registerObserver(MapboxNavigationAppDelegate.kt:53)
	at com.mapbox.navigation.core.lifecycle.MapboxNavigationApp.registerObserver(MapboxNavigationApp.kt:150)
	at

libnavui-androidauto/src/main/java/com/mapbox/androidauto/MapboxCarApp.kt

kmadsen · 2022-09-09T21:01:20Z

~~Back to draft on this one because I'm thinking it will create discussion.~~
~~It is also incomplete because it requires a merge, and a release and an update.~~

EDIT: actually i'm not finding more changes to make here so will leave it opn

tomaszrybakiewicz · 2022-09-12T20:09:53Z

libnavui-dropin/src/main/java/com/mapbox/navigation/dropin/NavigationView.kt

@@ -109,7 +109,7 @@ class NavigationView @JvmOverloads constructor(
 keepScreenOn = true
 captureSystemBarsInsets()

- SharedApp.setup(context.applicationContext as Application)
+ MapboxNavigationApp.registerObserver(SharedApp)


I don't think we should be changing how SharedApp is integrated with NavigationView. Instead, we should leave SharedApp.setup(Application) call here and move MapboxNavigationApp.registerObserver(SharedApp) call inside SharedApp.setup method.
Registering SharedApp as MapboxNavigationObserver is an implementation detail that should be hidden from the public.

Adding the point that SharedApp is still internal, but what we will want is a public version of SharedApp and then it can register an internal MapboxNavigationObserver object

Why should setup(Application) be required though? The application context is passed to NavigationOptions

I added the application context requirement to get early access to the Context before MapboxNavigation and NavigationOptions are initialized.

But why does SharedApp need the early access to the Context. Everything that uses the context would be better to get it from MapboxNavigationObserver

tomaszrybakiewicz · 2022-09-12T20:30:11Z

After applying the fix from this PR I am getting crash when opening "Default NavigationView" activity 2x in QA-Test-App:

E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.mapbox.navigation.qa_test_app, PID: 4971
    java.lang.IllegalStateException: There are multiple DataStores active for the same file: /data/user/0/com.mapbox.navigation.qa_test_app/files/datastore/mapbox_navigation_preferences.preferences_pb. You should either maintain your DataStore as a singleton or confirm that there is no two DataStore's active on the same file (by confirming that the scope is cancelled).
        at androidx.datastore.core.SingleProcessDataStore$file$2.invoke(SingleProcessDataStore.kt:168)
        at androidx.datastore.core.SingleProcessDataStore$file$2.invoke(SingleProcessDataStore.kt:163)
        at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74)
        at androidx.datastore.core.SingleProcessDataStore.getFile(SingleProcessDataStore.kt:163)
        at androidx.datastore.core.SingleProcessDataStore.readData(SingleProcessDataStore.kt:380)
        at androidx.datastore.core.SingleProcessDataStore.readDataOrHandleCorruption(SingleProcessDataStore.kt:359)
        at androidx.datastore.core.SingleProcessDataStore.readAndInit(SingleProcessDataStore.kt:322)
        at androidx.datastore.core.SingleProcessDataStore.readAndInitOrPropagateFailure(SingleProcessDataStore.kt:311)
        at androidx.datastore.core.SingleProcessDataStore.handleRead(SingleProcessDataStore.kt:261)
        at androidx.datastore.core.SingleProcessDataStore.access$handleRead(SingleProcessDataStore.kt:76)
        at androidx.datastore.core.SingleProcessDataStore$actor$3.invokeSuspend(SingleProcessDataStore.kt:239)
        at androidx.datastore.core.SingleProcessDataStore$actor$3.invoke(Unknown Source:8)
        at androidx.datastore.core.SingleProcessDataStore$actor$3.invoke(Unknown Source:4)
        at androidx.datastore.core.SimpleActor$offer$2.invokeSuspend(SimpleActor.kt:122)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

kmadsen · 2022-09-13T21:41:11Z

After some more discussion, SharedApp is not a class we really intend to make public. So making it backwards compatible is not the highest priority.

An initiative to install components with a public api is better served by the component installer

For android auto, we're looking for something along these lines #6304 where we can SharedApp from android auto.

kmadsen · 2022-09-15T17:11:39Z

Closing this one because we're planning on removing SharedApp from libnavui-androidauto entirely. Sharing public interfaces will be done as a separate effort. The approach will be more like this one #6304

kmadsen requested a review from a team as a code owner September 9, 2022 16:35

kmadsen mentioned this pull request Sep 9, 2022

Fixes alternatives and continuous alternatives in Drop-In #6213

Merged

kmadsen force-pushed the km-shared-app-backwards-compatible branch 2 times, most recently from 80d1dd4 to 1ecfb3f Compare September 9, 2022 16:41

kmadsen commented Sep 9, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kmadsen mentioned this pull request Sep 9, 2022

Unblock android auto releases #6304

Closed

kmadsen force-pushed the km-shared-app-backwards-compatible branch 2 times, most recently from f5a607e to d98d130 Compare September 9, 2022 17:44

kmadsen commented Sep 9, 2022

View reviewed changes

libnavui-androidauto/src/main/java/com/mapbox/androidauto/MapboxCarApp.kt Outdated Show resolved Hide resolved

kmadsen changed the title ~~Make SharedApp backwards compatible~~ Make SharedApp backwards compatible and removable Sep 9, 2022

kmadsen force-pushed the km-shared-app-backwards-compatible branch from d98d130 to 5b1eb0d Compare September 9, 2022 20:47

kmadsen marked this pull request as draft September 9, 2022 20:54

kmadsen marked this pull request as ready for review September 9, 2022 21:04

kmadsen force-pushed the km-shared-app-backwards-compatible branch from 5b1eb0d to 9c502e1 Compare September 9, 2022 21:06

Make SharedApp backwards compatible

50ad972

kmadsen force-pushed the km-shared-app-backwards-compatible branch from 9c502e1 to 50ad972 Compare September 9, 2022 21:13

tomaszrybakiewicz reviewed Sep 12, 2022

View reviewed changes

kmadsen mentioned this pull request Sep 12, 2022

Fix qa test app crash #6320

Closed

kmadsen closed this Sep 15, 2022

kmadsen deleted the km-shared-app-backwards-compatible branch September 15, 2022 17:11

kmadsen mentioned this pull request Sep 15, 2022

Update audio guidance for AA compatibility #6336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make SharedApp backwards compatible and removable #6303

Make SharedApp backwards compatible and removable #6303

kmadsen commented Sep 9, 2022 •

edited

Loading

codecov bot commented Sep 9, 2022 •

edited

Loading

kmadsen commented Sep 9, 2022 •

edited

Loading

kmadsen commented Sep 9, 2022 •

edited

Loading

tomaszrybakiewicz Sep 12, 2022

kmadsen Sep 12, 2022 •

edited

Loading

kmadsen Sep 12, 2022

tomaszrybakiewicz Sep 13, 2022

kmadsen Sep 13, 2022

tomaszrybakiewicz commented Sep 12, 2022

kmadsen commented Sep 13, 2022 •

edited

Loading

kmadsen commented Sep 15, 2022

Make SharedApp backwards compatible and removable #6303

Make SharedApp backwards compatible and removable #6303

Conversation

kmadsen commented Sep 9, 2022 • edited Loading

Description

codecov bot commented Sep 9, 2022 • edited Loading

Codecov Report

kmadsen commented Sep 9, 2022 • edited Loading

kmadsen commented Sep 9, 2022 • edited Loading

tomaszrybakiewicz Sep 12, 2022

Choose a reason for hiding this comment

kmadsen Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

kmadsen Sep 12, 2022

Choose a reason for hiding this comment

tomaszrybakiewicz Sep 13, 2022

Choose a reason for hiding this comment

kmadsen Sep 13, 2022

Choose a reason for hiding this comment

tomaszrybakiewicz commented Sep 12, 2022

kmadsen commented Sep 13, 2022 • edited Loading

kmadsen commented Sep 15, 2022

kmadsen commented Sep 9, 2022 •

edited

Loading

codecov bot commented Sep 9, 2022 •

edited

Loading

kmadsen commented Sep 9, 2022 •

edited

Loading

kmadsen commented Sep 9, 2022 •

edited

Loading

kmadsen Sep 12, 2022 •

edited

Loading

kmadsen commented Sep 13, 2022 •

edited

Loading