Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make SharedApp backwards compatible and removable #6303

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Mapbox welcomes participation and contributions from everyone.
#### Features
#### Bug fixes and improvements
- Optimized calls to modify route arrow layer visibility. [#6308](https://github.com/mapbox/mapbox-navigation-android/pull/6308)
- Make `SharedApp` backwards compatible and removable. [#6303](https://github.com/mapbox/mapbox-navigation-android/pull/6303)

## Mapbox Navigation SDK 2.8.0-beta.3 - 09 September, 2022
### Changelog
Expand Down
1 change: 1 addition & 0 deletions libnavui-androidauto/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Mapbox welcomes participation and contributions from everyone.
## Unreleased
#### Features
#### Bug fixes and improvements
- Removed `MapboxCarApp.setup`, replace with `MapboxNavigationApp.registerObserver(MapboxCarApp)` so it can be disabled. [#6275](https://github.com/mapbox/mapbox-navigation-android/pull/6275)

## androidauto-v0.10.0 - Sep 9, 2022
### Changelog
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package com.mapbox.androidauto

import android.app.Application
import com.mapbox.androidauto.navigation.location.CarAppLocation
import com.mapbox.androidauto.navigation.location.impl.CarAppLocationImpl
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
import com.mapbox.navigation.core.MapboxNavigation
import com.mapbox.navigation.core.lifecycle.MapboxNavigationApp
import com.mapbox.navigation.core.lifecycle.MapboxNavigationObserver
import com.mapbox.navigation.ui.app.internal.SharedApp
import com.mapbox.navigation.ui.voice.internal.MapboxAudioGuidance
import kotlinx.coroutines.flow.MutableStateFlow
Expand All @@ -13,10 +14,10 @@ import kotlinx.coroutines.flow.StateFlow
/**
* The entry point for your Mapbox Android Auto app.
*/
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
object MapboxCarApp {

object MapboxCarApp : MapboxNavigationObserver {
private val carAppStateFlow = MutableStateFlow<CarAppState>(FreeDriveState)
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
private val carAppLocation: CarAppLocation = CarAppLocationImpl()

/**
* Attach observers to the CarAppState to determine which view to show.
Expand All @@ -26,8 +27,7 @@ object MapboxCarApp {
/**
* Location service available to the car and app.
*/
fun carAppLocationService(): CarAppLocation =
MapboxNavigationApp.getObserver(CarAppLocation::class)
fun carAppLocationService(): CarAppLocation = carAppLocation

/**
* Audio guidance service available to the car and app.
Expand All @@ -42,13 +42,13 @@ object MapboxCarApp {
carAppStateFlow.value = carAppState
}

/**
* Setup android auto from your [Application.onCreate]
*
* @param application used to detect when activities are foregrounded
*/
fun setup(application: Application) {
SharedApp.setup(application)
MapboxNavigationApp.registerObserver(CarAppLocationImpl())
override fun onAttached(mapboxNavigation: MapboxNavigation) {
// TODO add after 2.8.0-rc.1
// MapboxNavigationApp.registerObserver(SharedApp)
MapboxNavigationApp.registerObserver(carAppLocation)
}

override fun onDetached(mapboxNavigation: MapboxNavigation) {
MapboxNavigationApp.unregisterObserver(carAppLocation)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.mapbox.navigation.ui.app.internal

import android.content.Context
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
import com.mapbox.navigation.core.MapboxNavigation
import com.mapbox.navigation.core.internal.extensions.attachCreated
import com.mapbox.navigation.core.lifecycle.MapboxNavigationApp
import com.mapbox.navigation.core.lifecycle.MapboxNavigationObserver
Expand All @@ -23,9 +24,7 @@ import com.mapbox.navigation.ui.voice.internal.impl.MapboxAudioGuidanceImpl
import java.util.concurrent.atomic.AtomicBoolean

@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
object SharedApp {
private var isSetup = false

object SharedApp : MapboxNavigationObserver {
val store = Store()
val state get() = store.state.value

Expand All @@ -42,6 +41,10 @@ object SharedApp {
val destinationStateController = DestinationStateController(store)
val routeStateController = RouteStateController(store)
val routePreviewStateController = RoutePreviewStateController(store)
private val stateResetController = StateResetController(store, ignoreTripSessionUpdates)
private val routeAlternativeComponent = RouteAlternativeComponent {
RouteAlternativeComponentImpl(store)
}
private val navigationObservers: Array<MapboxNavigationObserver> = arrayOf(
routeStateController,
cameraStateController,
Expand All @@ -51,26 +54,14 @@ object SharedApp {
routePreviewStateController,
audioGuidanceStateController,
tripSessionStarterStateController,
stateResetController,
routeAlternativeComponent,
)

@JvmOverloads
fun setup(
context: Context,
audioGuidance: MapboxAudioGuidance? = null,
routeAlternativeContract: RouteAlternativeContract? = null
) {
if (isSetup) return
isSetup = true

MapboxNavigationApp.registerObserver(StateResetController(store, ignoreTripSessionUpdates))
MapboxNavigationApp.registerObserver(
RouteAlternativeComponent {
routeAlternativeContract ?: RouteAlternativeComponentImpl(store)
}
)
MapboxNavigationApp.lifecycleOwner.attachCreated(*navigationObservers)
MapboxNavigationApp.registerObserver(audioGuidance ?: defaultAudioGuidance(context))
}
/**
* Requires the [Context] so will be initialized when attached.
*/
private lateinit var mapboxAudioGuidance: MapboxAudioGuidance

fun tripSessionTransaction(updateSession: () -> Unit) {
// Any changes to MapboxNavigation TripSession should be done within `tripSessionTransaction { }` block.
Expand All @@ -87,5 +78,17 @@ object SharedApp {
}
}

override fun onAttached(mapboxNavigation: MapboxNavigation) {
val context = mapboxNavigation.navigationOptions.applicationContext
mapboxAudioGuidance = defaultAudioGuidance(context)
MapboxNavigationApp.registerObserver(mapboxAudioGuidance)
MapboxNavigationApp.lifecycleOwner.attachCreated(*navigationObservers)
}

override fun onDetached(mapboxNavigation: MapboxNavigation) {
navigationObservers.forEach { MapboxNavigationApp.unregisterObserver(it) }
MapboxNavigationApp.unregisterObserver(mapboxAudioGuidance)
}

private const val DEFAULT_DATA_STORE_NAME = "mapbox_navigation_preferences"
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package com.mapbox.navigation.dropin

import android.Manifest
import android.app.Activity
import android.app.Application
import android.content.Context
import android.util.AttributeSet
import android.view.LayoutInflater
Expand All @@ -20,6 +19,7 @@ import com.mapbox.maps.MapView
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
import com.mapbox.navigation.base.options.NavigationOptions
import com.mapbox.navigation.core.internal.extensions.attachCreated
import com.mapbox.navigation.core.internal.extensions.attachStarted
import com.mapbox.navigation.core.lifecycle.MapboxNavigationApp
import com.mapbox.navigation.core.replay.MapboxReplayer
import com.mapbox.navigation.dropin.component.analytics.AnalyticsComponent
Expand Down Expand Up @@ -109,7 +109,7 @@ class NavigationView @JvmOverloads constructor(
keepScreenOn = true
captureSystemBarsInsets()

SharedApp.setup(context.applicationContext as Application)
MapboxNavigationApp.registerObserver(SharedApp)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@kmadsen kmadsen Sep 12, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (!MapboxNavigationApp.isSetup()) {
MapboxNavigationApp.setup(
NavigationOptions.Builder(context)
Expand Down
3 changes: 3 additions & 0 deletions qa-test-app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ dependencies {
implementation project(':libnavui-androidauto')
implementation dependenciesList.mapboxSearchSdk

// TODO Remove after 2.8.0-rc.1
implementation project(':libnavui-app')

// test
androidTestImplementation project(':libtesting-ui')
androidTestImplementation dependenciesList.testRunner
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,5 @@
package com.mapbox.navigation.qa_test_app

import android.app.Application
import com.mapbox.androidauto.MapboxCarApp

class QaTestApplication : Application() {

override fun onCreate() {
super.onCreate()

MapboxCarApp.setup(this)
}
}
class QaTestApplication : Application()
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.lifecycleScope
import androidx.lifecycle.repeatOnLifecycle
import com.mapbox.android.core.permissions.PermissionsManager
import com.mapbox.androidauto.MapboxCarApp
import com.mapbox.androidauto.MapboxCarNavigationManager
import com.mapbox.androidauto.car.MainCarContext
import com.mapbox.androidauto.car.MainScreenManager
Expand All @@ -27,9 +28,11 @@ import com.mapbox.maps.MapboxExperimental
import com.mapbox.maps.extension.androidauto.MapboxCarMap
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
import com.mapbox.navigation.base.options.NavigationOptions
import com.mapbox.navigation.core.internal.extensions.attachCreated
import com.mapbox.navigation.core.lifecycle.MapboxNavigationApp
import com.mapbox.navigation.core.trip.session.TripSessionState
import com.mapbox.navigation.qa_test_app.utils.Utils
import com.mapbox.navigation.ui.app.internal.SharedApp
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch

Expand All @@ -47,6 +50,11 @@ class MainCarSession : Session() {

init {
logAndroidAuto("MainCarSession constructor")

// TODO remove after 2.8.0-rc.1
MapboxNavigationApp.registerObserver(SharedApp)
attachCreated(MapboxCarApp)

val logoSurfaceRenderer = CarLogoSurfaceRenderer()
val compassSurfaceRenderer = CarCompassSurfaceRenderer()
MapboxNavigationApp.attach(lifecycleOwner = this)
Expand Down