Skip to content

Add ability to check configuration changing #6484

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add ability to check configuration changing #6484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,10 @@ This release depends on, and has been tested with, the following Mapbox dependen
- Mapbox Core Common `v23.1.0-rc.2`
- Mapbox Java `v6.8.0` ([release notes](https://github.com/mapbox/mapbox-java/releases/tag/v6.8.0))
- Mapbox Android Core `v5.0.2` ([release notes](https://github.com/mapbox/mapbox-events-android/releases/tag/core-5.0.2))
- Add ability to check when `NavigationOptions` are changing with `MapboxNavigationApp.isOptionsChanging`. [#6484](https://github.com/mapbox/mapbox-navigation-android/pull/6484)
- Add ability to check when Lifecycle events are triggered by configuration changes with `MapboxNavigationApp.isConfigurationChanging`. [#6484](https://github.com/mapbox/mapbox-navigation-android/pull/6484)
- Added ability to check when `NavigationOptions` are changing with `MapboxNavigationApp.isOptionsChanging`. [#6484](https://github.com/mapbox/mapbox-navigation-android/pull/6484)
- Added ability to check when Lifecycle events are triggered by configuration changes with `MapboxNavigationApp.isConfigurationChanging`. [#6484](https://github.com/mapbox/mapbox-navigation-android/pull/6484)

## Mapbox Navigation SDK 2.9.0-beta.2 - 14 October, 2022
### Changelog
Expand Down
1 change: 1 addition & 0 deletions libnavigation-core/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ dependencies {
testImplementation project(':libtesting-utils')
testImplementation project(':libtesting-navigation-base')
testImplementation project(':libtesting-navigation-util')
testImplementation(dependenciesList.androidXLifecycleTesting)
apply from: "${rootDir}/gradle/unit-testing-dependencies.gradle"
testImplementation dependenciesList.commonsIO

Expand Down
19 changes: 19 additions & 0 deletions libnavigation-core/src/main/java/com/mapbox/navigation/core/lifecycle/MapboxNavigationApp.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,25 @@ object MapboxNavigationApp {
@JvmStatic
fun isSetup(): Boolean = mapboxNavigationAppDelegate.isSetup

/**
* When calling [setup] multiple times, all registered [MapboxNavigationObserver] will be
* detached from the instance of [MapboxNavigation] that is being destroyed. This is needed for
* maintaining state across options changes. You do not need to clear the state when
* [isOptionsChanging] is true.
*/
@UiThread
@JvmStatic
fun isOptionsChanging(): Boolean = mapboxNavigationAppDelegate.isOptionsChanging

/**
* When observing [MapboxNavigation] from a [Lifecycle], the [Lifecycle.Event] may be triggered
* because of a configuration change. This will help you know if the lifecycle events are
* triggered events like mobile device orientation changes.
*/
@UiThread
@JvmStatic
fun isConfigurationChanging(): Boolean = mapboxNavigationAppDelegate.isConfigurationChanging()
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you envision using these methods? Do you have an example?
I can't say about options changing but regarding configuration changing I'm not sure if that's the way to go.
I'm worried that we might use this method somewhere and when it returns true we'll just ignore some actions, meanwhile the state will become inconsistent because some other components will be recreated.
I think it would be more neat and clear if the components that don't care about configuration changes just had a broader lifecycle. It's basically your first idea from #6766 (comment).

Copy link
Contributor Author

@kmadsen kmadsen Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I have the same concern. I've been debating whether the MapboxNavigationObserver implementation should ignore the detach->attach when configuration is changing. Or if MapboxNavigaitonApp should not detach observers when configuration is changing.

I think we need to support the case where, a MapboxNavigationObserver is only attached while in landscape mode. So I've been thinking it should be the MapboxNavigationObserver responsibility to create a definition and usage requirements

Copy link
Contributor Author

@kmadsen kmadsen Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There are issues with putting the logic in MapboxNavigationObserver implementations. For example, if we want the observer to be usable without MapboxNavigationApp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So another option am considering, is what if you can specify what you want during registration. For example, add a new way to register an observer

MapboxNavigationApp.registerStrongObserver(observer)

the strong (there is probably a better word) observer would not detach during configuration or option changes. another option is to add parameters to the registerObserver function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually like the strong observer idea. And I think that a new type of observer is more explicit than passing the options as an argument. It's also less flexible but I'm not sure if we're gonna need a lot of flexibility here.
And I guess we should also support the case when we want to do A on onAttached, B on configuration/options change and C on onDetached. We'd probably need two different observers to resolve this. To make it possible we should guarantee the order in which the observers are called. For example, if we register one MapboxNavigationObserver (O1) and MapboxNavigationStrongObserver (O2), then common onAttached/onDetached should be passed first to O1, then to O2 (or whatever order is more convenient). WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually like the strong observer idea.

👍

And I think that a new type of observer is more explicit than passing the options as an argument

Do you mean, preferring 2 over 1 like this? I can see it, but there are pros and cons 🤔

  1. MapboxNavigationApp.registerStrongObserver(object : MapboxNavigationObserver ..)
  2. MapboxNavigationApp.registerObserver(object : MapboxNavigationStrongObserver ..)
  3. Add MapboxNavigationObserver#isStrong() overridable configuration
  4. Add MapboxNavigationObserver#onConfigurationChanged() and then make all observers strong
  5. Add MapboxNavigationApp#isConfigurationChanging() so that onAttached/onDetached can figure it out

I'm not sure what I prefer yet.. it seems all of them technically work so we could list some pros/cons to decide.

And I guess we should also support the case when we want to do A on onAttached, B on configuration/options change and C on onDetached.

We can also consider, adding an overridable function to MapboxNavigationObserver (e.g., onConfigurationChanged..). The pull request as it is now, requires a condition to see if onAttached/onDetached is triggered by a configuration change.

MapboxNavigationObserver (O1) and MapboxNavigationStrongObserver (O2), then common onAttached/onDetached should be passed first to O1, then to O2 (or whatever order is more convenient). WDYT?

I'm not yet sold on MapboxNavigationStrongObserver because I bet if we listed the pros and cons, we may prefer solution 3 to add an overridable configuration to MapboxNavigationObserver, or we may decide on something else. I don't like that we would have to change the type of the observer. I would like to at least start with the flexibility to be able to make any MapboxNavigationObserver a strong observer.

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 like that we would have to change the type of the observer.

Why? We are intoducing a new type of observer with behaviour that was not present before. We could do with a new interface here.

Add MapboxNavigationObserver#isStrong() overridable configuration

How are you going to implement it without breaking the API? There were some problems with introducing an interface method with default implementation. Converting it to an abstract class will break java users for sure.

Copy link
Contributor Author

@kmadsen kmadsen Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't like that we would have to change the type of the observer.
Why? We are intoducing a new type of observer with behaviour that was not present before. We could do with a new interface here.

There is no difference with the implementation of the interface. Maybe we do this approach, just think we should consider other approaches first.

How are you going to implement it without breaking the API?

We can change the MapboxNavigationObserver implementation if we convert the interface to a java interface (an example here). Default functions are supported in java, default functions will have better support in kotlin after version 1.6.20 https://youtrack.jetbrains.com/issue/KT-54239


/**
* Call [MapboxNavigationApp.setup] to provide the application with [NavigationOptions].
*
Expand Down
11 changes: 10 additions & 1 deletion ...on-core/src/main/java/com/mapbox/navigation/core/lifecycle/MapboxNavigationAppDelegate.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,31 @@ internal class MapboxNavigationAppDelegate {

val lifecycleOwner: LifecycleOwner by lazy { carAppLifecycleOwner }

var isOptionsChanging = false
private set

var isSetup = false
private set

fun setup(navigationOptionsProvider: NavigationOptionsProvider) = apply {
if (carAppLifecycleOwner.isConfigurationChanging()) {
return this
}

if (isSetup) {
isOptionsChanging = true
disable()
}

mapboxNavigationOwner.setup(navigationOptionsProvider)
carAppLifecycleOwner.lifecycle.addObserver(mapboxNavigationOwner.carAppLifecycleObserver)
isOptionsChanging = false
isSetup = true
}

fun isConfigurationChanging(): Boolean {
return carAppLifecycleOwner.isConfigurationChanging()
}

fun attachAllActivities(application: Application) {
carAppLifecycleOwner.attachAllActivities(application)
}
Expand Down
32 changes: 12 additions & 20 deletions ...ation-core/src/test/java/com/mapbox/navigation/core/lifecycle/CarAppLifecycleOwnerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ import androidx.fragment.app.Fragment
import androidx.fragment.app.FragmentActivity
import androidx.lifecycle.DefaultLifecycleObserver
import androidx.lifecycle.Lifecycle
import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.LifecycleRegistry
import androidx.lifecycle.testing.TestLifecycleOwner
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
import com.mapbox.navigation.testing.LoggingFrontendTestRule
import io.mockk.every
Expand Down Expand Up @@ -405,8 +404,8 @@ class CarAppLifecycleOwnerTest {
val testLifecycleOwner = TestLifecycleOwner()
carAppLifecycleOwner.attach(testLifecycleOwner)

testLifecycleOwner.lifecycleRegistry.currentState = Lifecycle.State.RESUMED
testLifecycleOwner.lifecycleRegistry.currentState = Lifecycle.State.DESTROYED
testLifecycleOwner.handleLifecycleEvent(Lifecycle.Event.ON_RESUME)
testLifecycleOwner.handleLifecycleEvent(Lifecycle.Event.ON_DESTROY)

verify(exactly = 1) { testLifecycleObserver.onCreate(any()) }
verify(exactly = 1) { testLifecycleObserver.onStart(any()) }
Expand All @@ -422,10 +421,10 @@ class CarAppLifecycleOwnerTest {
carAppLifecycleOwner.attach(testLifecycleOwnerA)
carAppLifecycleOwner.attach(testLifecycleOwnerB)

testLifecycleOwnerA.lifecycleRegistry.currentState = Lifecycle.State.RESUMED
testLifecycleOwnerB.lifecycleRegistry.currentState = Lifecycle.State.RESUMED
testLifecycleOwnerA.lifecycleRegistry.currentState = Lifecycle.State.DESTROYED
testLifecycleOwnerB.lifecycleRegistry.currentState = Lifecycle.State.DESTROYED
testLifecycleOwnerA.handleLifecycleEvent(Lifecycle.Event.ON_RESUME)
testLifecycleOwnerB.handleLifecycleEvent(Lifecycle.Event.ON_RESUME)
testLifecycleOwnerA.handleLifecycleEvent(Lifecycle.Event.ON_DESTROY)
testLifecycleOwnerB.handleLifecycleEvent(Lifecycle.Event.ON_DESTROY)

verify(exactly = 1) { testLifecycleObserver.onCreate(any()) }
verify(exactly = 1) { testLifecycleObserver.onStart(any()) }
Expand All @@ -441,10 +440,10 @@ class CarAppLifecycleOwnerTest {
carAppLifecycleOwner.attach(testLifecycleOwnerA)
carAppLifecycleOwner.attach(testLifecycleOwnerB)

testLifecycleOwnerA.lifecycleRegistry.currentState = Lifecycle.State.RESUMED
testLifecycleOwnerB.lifecycleRegistry.currentState = Lifecycle.State.STARTED
testLifecycleOwnerA.lifecycleRegistry.currentState = Lifecycle.State.DESTROYED
testLifecycleOwnerB.lifecycleRegistry.currentState = Lifecycle.State.DESTROYED
testLifecycleOwnerA.handleLifecycleEvent(Lifecycle.Event.ON_RESUME)
testLifecycleOwnerB.handleLifecycleEvent(Lifecycle.Event.ON_RESUME)
testLifecycleOwnerA.handleLifecycleEvent(Lifecycle.Event.ON_DESTROY)
testLifecycleOwnerB.handleLifecycleEvent(Lifecycle.Event.ON_DESTROY)

verify(exactly = 1) { testLifecycleObserver.onCreate(any()) }
verify(exactly = 1) { testLifecycleObserver.onStart(any()) }
Expand All @@ -458,7 +457,7 @@ class CarAppLifecycleOwnerTest {
val testLifecycleOwner = TestLifecycleOwner()

carAppLifecycleOwner.attach(testLifecycleOwner)
testLifecycleOwner.lifecycleRegistry.currentState = Lifecycle.State.RESUMED
testLifecycleOwner.handleLifecycleEvent(Lifecycle.Event.ON_RESUME)
carAppLifecycleOwner.detach(testLifecycleOwner)

verify(exactly = 1) { testLifecycleObserver.onCreate(any()) }
Expand All @@ -468,13 +467,6 @@ class CarAppLifecycleOwnerTest {
verify(exactly = 0) { testLifecycleObserver.onDestroy(any()) }
}

class TestLifecycleOwner : LifecycleOwner {
val lifecycleRegistry = LifecycleRegistry(this)
.also { it.currentState = Lifecycle.State.INITIALIZED }

override fun getLifecycle(): Lifecycle = lifecycleRegistry
}

private fun mockActivity(isChangingConfig: Boolean = false): FragmentActivity = mockk {
every { isChangingConfigurations } returns isChangingConfig
}
Expand Down
Loading