From f50cc261a45242c36938322d50eae4c8b866c20c Mon Sep 17 00:00:00 2001 From: CherryPerry Date: Tue, 30 Aug 2022 10:02:48 +0100 Subject: [PATCH] Fix parent-child order of back press handling by fixing lifecycle issue --- CHANGELOG.md | 2 + core/src/androidTest/AndroidManifest.xml | 12 ++ .../appyx/core/plugin/AppyxTestScenario.kt | 54 +++++ .../appyx/core/plugin/BackPressHandlerTest.kt | 194 +++++++++++++++--- .../plugin/BackPressHandlerTestActivity.kt | 35 ++++ .../lifecycle/ChildNodeLifecycleManager.kt | 25 ++- .../com/bumble/appyx/core/node/ParentNode.kt | 18 +- .../core/lifecycle/ChildLifecycleTest.kt | 34 ++- .../testing/ui/rules/AppyxViewActivity.kt | 2 +- 9 files changed, 331 insertions(+), 45 deletions(-) create mode 100644 core/src/androidTest/AndroidManifest.xml create mode 100644 core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/AppyxTestScenario.kt create mode 100644 core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/BackPressHandlerTestActivity.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d062c3288..51ef1baed7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Pending changes +- [#119](https://github.com/bumble-tech/appyx/pull/119) - **Fixed**: Lifecycle observers are invoked in incorrect order (child before parent) + ## 1.0-alpha06 – 26 Aug 2022 - [#96](https://github.com/bumble-tech/appyx/pull/96) – **Breaking change**: Removed `InteractorTestHelper`. Please use Node tests instead of Interactor tests. diff --git a/core/src/androidTest/AndroidManifest.xml b/core/src/androidTest/AndroidManifest.xml new file mode 100644 index 0000000000..3e66e8d835 --- /dev/null +++ b/core/src/androidTest/AndroidManifest.xml @@ -0,0 +1,12 @@ + + + + + + + + + + diff --git a/core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/AppyxTestScenario.kt b/core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/AppyxTestScenario.kt new file mode 100644 index 0000000000..66cc4aa612 --- /dev/null +++ b/core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/AppyxTestScenario.kt @@ -0,0 +1,54 @@ +package com.bumble.appyx.core.plugin + +import androidx.annotation.WorkerThread +import androidx.compose.runtime.Composable +import androidx.compose.ui.test.junit4.ComposeTestRule +import androidx.compose.ui.test.junit4.createEmptyComposeRule +import androidx.test.core.app.ActivityScenario +import com.bumble.appyx.core.integration.NodeFactory +import com.bumble.appyx.core.integration.NodeHost +import com.bumble.appyx.core.node.Node +import com.bumble.appyx.testing.ui.rules.AppyxViewActivity +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit + +/** + * [com.bumble.appyx.testing.ui.rules.AppyxTestRule] based on [ActivityScenario] to support lifecycle tests. + * + * TODO: Consider merging with AppyxTestRule. + */ +class AppyxTestScenario( + private val composeTestRule: ComposeTestRule = createEmptyComposeRule(), + /** Add decorations like custom theme or CompositionLocalProvider. Do not forget to invoke `content()`. */ + private val decorator: (@Composable (content: @Composable () -> Unit) -> Unit) = { content -> content() }, + private val nodeFactory: NodeFactory, +) : ComposeTestRule by composeTestRule { + + @get:WorkerThread + val activityScenario: ActivityScenario by lazy { + val awaitNode = CountDownLatch(1) + AppyxViewActivity.composableView = { activity -> + decorator { + NodeHost(integrationPoint = activity.integrationPoint, factory = { buildContext -> + node = nodeFactory.create(buildContext) + awaitNode.countDown() + node + }) + } + } + val scenario = ActivityScenario.launch(BackPressHandlerTestActivity::class.java) + require(awaitNode.await(10, TimeUnit.SECONDS)) { + "Timeout while waiting for node initialization" + } + waitForIdle() + scenario + } + + lateinit var node: T + private set + + fun start() { + activityScenario + } + +} diff --git a/core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/BackPressHandlerTest.kt b/core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/BackPressHandlerTest.kt index 6860d15fe6..4ec69116a6 100644 --- a/core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/BackPressHandlerTest.kt +++ b/core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/BackPressHandlerTest.kt @@ -1,11 +1,22 @@ package com.bumble.appyx.core.plugin import android.app.Activity +import android.os.Parcelable import androidx.activity.OnBackPressedCallback +import androidx.compose.foundation.background +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.height import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier +import androidx.compose.ui.graphics.Color +import androidx.compose.ui.unit.dp +import androidx.lifecycle.Lifecycle import androidx.test.espresso.Espresso import androidx.test.platform.app.InstrumentationRegistry +import com.bumble.appyx.core.children.nodeOrNull import com.bumble.appyx.core.composable.Children import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.ParentNode @@ -14,7 +25,7 @@ import com.bumble.appyx.debug.Appyx import com.bumble.appyx.navmodel.backstack.BackStack import com.bumble.appyx.navmodel.backstack.activeRouting import com.bumble.appyx.navmodel.backstack.operation.push -import com.bumble.appyx.testing.ui.rules.AppyxTestRule +import kotlinx.parcelize.Parcelize import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers.equalTo import org.hamcrest.Matchers.instanceOf @@ -24,71 +35,68 @@ import org.junit.Test class BackPressHandlerTest { - private var onBackPressedHandled = false - private var backPressHandler: BackPressHandler = object : BackPressHandler { - override val onBackPressedCallback: OnBackPressedCallback = - object : OnBackPressedCallback(true) { - override fun handleOnBackPressed() { - onBackPressedHandled = true - } - } - } + private var backHandler = TestPlugin() + private var childBackHandler = TestPlugin() @get:Rule - val rule = AppyxTestRule(launchActivity = false) { buildContext -> - TestParentNode(buildContext = buildContext, plugin = backPressHandler) + val rule = AppyxTestScenario { buildContext -> + TestParentNode( + buildContext = buildContext, + plugin = backHandler, + childPlugin = childBackHandler, + ) } @After fun after() { Appyx.exceptionHandler = null + BackPressHandlerTestActivity.reset() } @Test fun routing_handles_back_press_when_plugin_has_disabled_listener() { rule.start() - runOnMainSync { - backPressHandler.onBackPressedCallback!!.isEnabled = false - rule.node.backStack.push(TestParentNode.Routing.ChildB) - } + pushChildB() + disablePlugin() Espresso.pressBack() Espresso.onIdle() + rule.waitForIdle() assertThat(rule.node.backStack.activeRouting, equalTo(TestParentNode.Routing.ChildA)) - assertThat(onBackPressedHandled, equalTo(false)) + assertThat(backHandler.onBackPressedHandled, equalTo(false)) } @Test fun custom_plugin_handles_back_press_before_routing() { rule.start() - runOnMainSync { rule.node.backStack.push(TestParentNode.Routing.ChildB) } + pushChildB() Espresso.pressBack() Espresso.onIdle() + rule.waitForIdle() assertThat(rule.node.backStack.activeRouting, equalTo(TestParentNode.Routing.ChildB)) - assertThat(onBackPressedHandled, equalTo(true)) + assertThat(backHandler.onBackPressedHandled, equalTo(true)) } @Test fun activity_is_closed_when_nobody_can_handle_back_press() { rule.start() - runOnMainSync { - backPressHandler.onBackPressedCallback!!.isEnabled = false - } + disablePlugin() Espresso.pressBackUnconditionally() Espresso.onIdle() + rule.waitForIdle() - assertThat(rule.activityResult.resultCode, equalTo(Activity.RESULT_CANCELED)) + assertThat(rule.activityScenario.result.resultCode, equalTo(Activity.RESULT_CANCELED)) } @Test fun reports_incorrect_handler() { var exception: Exception? = null Appyx.exceptionHandler = { exception = it } - backPressHandler = object : BackPressHandler { + backHandler = object : TestPlugin() { override val onBackPressedCallback: OnBackPressedCallback = object : OnBackPressedCallback(true) { override fun handleOnBackPressed() { @@ -105,41 +113,167 @@ class BackPressHandlerTest { rule.start() Espresso.onIdle() + rule.waitForIdle() assertThat(exception, instanceOf(IllegalStateException::class.java)) } + @Test + fun child_back_handler_works_before_parent() { + rule.start() + runOnMainSync { rule.node.backStack.push(TestParentNode.Routing.ChildWithPlugin) } + + Espresso.pressBack() + Espresso.onIdle() + rule.waitForIdle() + + assertThat(childBackHandler.onBackPressedHandled, equalTo(true)) + } + + @Test + fun appyx_handles_back_press_before_activity_handler() { + BackPressHandlerTestActivity.handleBackPress.value = true + rule.start() + pushChildB() + + Espresso.pressBack() + Espresso.onIdle() + rule.waitForIdle() + + assertThat(backHandler.onBackPressedHandled, equalTo(true)) + } + + @Test + fun activity_handles_back_press_if_appyx_cant() { + BackPressHandlerTestActivity.handleBackPress.value = true + rule.start() + disablePlugin() + + Espresso.pressBack() + Espresso.onIdle() + rule.waitForIdle() + + assertThat(BackPressHandlerTestActivity.onBackPressedHandled, equalTo(true)) + } + + @Test + // real case for https://github.com/bumble-tech/appyx/issues/118 + fun appyx_handles_back_press_after_activity_returns_from_background() { + fun TestParentNode.findChildNode() = + children.value.values.firstNotNullOf { value -> + value.nodeOrNull?.takeIf { value.key.routing == TestParentNode.Routing.ChildWithPlugin } + } as TestParentNode + + rule.start() + runOnMainSync { + rule.node.run { + backStack.push(TestParentNode.Routing.ChildWithPlugin) + val node = findChildNode() + node.backStack.push(TestParentNode.Routing.ChildB) + } + } + + rule.activityScenario.moveToState(Lifecycle.State.CREATED) + rule.waitForIdle() + rule.activityScenario.moveToState(Lifecycle.State.RESUMED) + rule.waitForIdle() + + Espresso.pressBack() + Espresso.onIdle() + rule.waitForIdle() + + assertThat(childBackHandler.onBackPressedHandled, equalTo(true)) + } + private fun runOnMainSync(runner: Runnable) { InstrumentationRegistry.getInstrumentation().runOnMainSync(runner) } + private fun pushChildB() { + runOnMainSync { rule.node.backStack.push(TestParentNode.Routing.ChildB) } + } + + private fun disablePlugin() { + backHandler.onBackPressedCallback.isEnabled = false + } + class TestParentNode( buildContext: BuildContext, val backStack: BackStack = BackStack( initialElement = Routing.ChildA, savedStateMap = null, ), - plugin: Plugin, + plugin: Plugin?, + private val childPlugin: Plugin?, ) : ParentNode( buildContext = buildContext, navModel = backStack, - plugins = listOf(plugin), + plugins = listOfNotNull(plugin), ) { - sealed class Routing { + sealed class Routing : Parcelable { + + @Parcelize object ChildA : Routing() + + @Parcelize object ChildB : Routing() + + @Parcelize + object ChildWithPlugin : Routing() + } override fun resolve(routing: Routing, buildContext: BuildContext) = when (routing) { - Routing.ChildA -> node(buildContext) {} - Routing.ChildB -> node(buildContext) {} + Routing.ChildA -> node(buildContext) { + Spacer( + modifier = Modifier + .fillMaxSize() + .background(Color.Green) + ) + } + Routing.ChildB -> node(buildContext) { + Spacer( + modifier = Modifier + .fillMaxSize() + .background(Color.Red) + ) + } + Routing.ChildWithPlugin -> TestParentNode( + buildContext = buildContext, + plugin = childPlugin, + childPlugin = null, + ) } @Composable override fun View(modifier: Modifier) { - Children(navModel = backStack, modifier = modifier) + Column(modifier) { + Spacer( + modifier = Modifier + .fillMaxWidth() + .background(Color.Black) + .height(26.dp) + ) + Spacer( + modifier = Modifier + .fillMaxWidth() + .height(10.dp) + ) + Children(navModel = backStack, modifier = Modifier.fillMaxSize()) + } } } + private open class TestPlugin : BackPressHandler { + var onBackPressedHandled = false + private set + override val onBackPressedCallback: OnBackPressedCallback = + object : OnBackPressedCallback(true) { + override fun handleOnBackPressed() { + onBackPressedHandled = true + } + } + } + } diff --git a/core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/BackPressHandlerTestActivity.kt b/core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/BackPressHandlerTestActivity.kt new file mode 100644 index 0000000000..bf9128f427 --- /dev/null +++ b/core/src/androidTest/kotlin/com/bumble/appyx/core/plugin/BackPressHandlerTestActivity.kt @@ -0,0 +1,35 @@ +package com.bumble.appyx.core.plugin + +import android.os.Bundle +import androidx.activity.OnBackPressedCallback +import androidx.lifecycle.lifecycleScope +import com.bumble.appyx.testing.ui.rules.AppyxViewActivity +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.launch + +class BackPressHandlerTestActivity : AppyxViewActivity() { + + private val callback = object : OnBackPressedCallback(handleBackPress.value) { + override fun handleOnBackPressed() { + onBackPressedHandled = true + } + } + + override fun onCreate(savedInstanceState: Bundle?) { + super.onCreate(savedInstanceState) + lifecycleScope.launch { handleBackPress.collect { callback.isEnabled = it } } + onBackPressedDispatcher.addCallback(this, callback) + } + + companion object { + val handleBackPress: MutableStateFlow = MutableStateFlow(false) + @Volatile + var onBackPressedHandled: Boolean = false + + fun reset() { + handleBackPress.value = false + onBackPressedHandled = false + } + } + +} \ No newline at end of file diff --git a/core/src/main/kotlin/com/bumble/appyx/core/lifecycle/ChildNodeLifecycleManager.kt b/core/src/main/kotlin/com/bumble/appyx/core/lifecycle/ChildNodeLifecycleManager.kt index 89054a104f..ecbf26a8bb 100644 --- a/core/src/main/kotlin/com/bumble/appyx/core/lifecycle/ChildNodeLifecycleManager.kt +++ b/core/src/main/kotlin/com/bumble/appyx/core/lifecycle/ChildNodeLifecycleManager.kt @@ -2,16 +2,16 @@ package com.bumble.appyx.core.lifecycle import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleRegistry -import androidx.lifecycle.coroutineScope import com.bumble.appyx.core.children.ChildEntry import com.bumble.appyx.core.children.ChildEntryMap import com.bumble.appyx.core.children.nodeOrNull -import com.bumble.appyx.core.navigation.RoutingKey import com.bumble.appyx.core.navigation.NavModel import com.bumble.appyx.core.navigation.NavModelAdapter +import com.bumble.appyx.core.navigation.RoutingKey import com.bumble.appyx.core.withPrevious import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged @@ -24,19 +24,34 @@ import kotlinx.coroutines.launch * and updates lifecycle of children nodes when updated. */ internal class ChildNodeLifecycleManager( - private val lifecycle: Lifecycle, private val navModel: NavModel, private val children: StateFlow>, - private val coroutineScope: CoroutineScope = lifecycle.coroutineScope, + private val coroutineScope: CoroutineScope, ) { + private val lifecycleState = MutableStateFlow(Lifecycle.State.INITIALIZED) + + /** + * Propagates the parent lifecycle to children. + * + * Child lifecycles should be updated only after all lifecycle callbacks are invoked in the current node. + * + * **DO NOT USE LIFECYCLE.OBSERVE HERE!** + * + * Otherwise a child node lifecycle might be updated before the parent. + * It leads to incorrect registration order of back handlers. + */ + fun propagateLifecycleToChildren(state: Lifecycle.State) { + lifecycleState.value = state + } + fun launch() { // previously state management was split into multiple jobs // but we found execution order issues when the main thread is busy (?) // so we merged it into single one coroutineScope.launch { combine( - lifecycle.asFlow(), + lifecycleState, navModel.screenState.keys(), children.withPrevious(), ::Triple diff --git a/core/src/main/kotlin/com/bumble/appyx/core/node/ParentNode.kt b/core/src/main/kotlin/com/bumble/appyx/core/node/ParentNode.kt index 65023f1f6f..811aae39e1 100644 --- a/core/src/main/kotlin/com/bumble/appyx/core/node/ParentNode.kt +++ b/core/src/main/kotlin/com/bumble/appyx/core/node/ParentNode.kt @@ -10,6 +10,7 @@ import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier import androidx.lifecycle.DefaultLifecycleObserver +import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.coroutineScope import androidx.lifecycle.lifecycleScope @@ -24,18 +25,16 @@ import com.bumble.appyx.core.composable.ChildRenderer import com.bumble.appyx.core.lifecycle.ChildNodeLifecycleManager import com.bumble.appyx.core.modality.AncestryInfo import com.bumble.appyx.core.modality.BuildContext -import com.bumble.appyx.core.plugin.Plugin +import com.bumble.appyx.core.navigation.NavModel import com.bumble.appyx.core.navigation.Resolver import com.bumble.appyx.core.navigation.RoutingKey -import com.bumble.appyx.core.navigation.NavModel import com.bumble.appyx.core.navigation.isTransitioning import com.bumble.appyx.core.navigation.model.combined.plus import com.bumble.appyx.core.navigation.model.permanent.PermanentNavModel import com.bumble.appyx.core.navigation.model.permanent.operation.add +import com.bumble.appyx.core.plugin.Plugin import com.bumble.appyx.core.state.MutableSavedStateMap import com.bumble.appyx.core.state.SavedStateMap -import kotlin.coroutines.resume -import kotlin.reflect.KClass import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -46,6 +45,8 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeoutOrNull +import kotlin.coroutines.resume +import kotlin.reflect.KClass abstract class ParentNode( navModel: NavModel, @@ -73,9 +74,9 @@ abstract class ParentNode( val children: StateFlow> = _children.asStateFlow() private val childNodeLifecycleManager = ChildNodeLifecycleManager( - lifecycle = lifecycle, navModel = this.navModel, children = children, + coroutineScope = lifecycleScope, ) private var transitionsInBackgroundJob: Job? = null @@ -177,6 +178,11 @@ abstract class ParentNode( PermanentChild(routing) { child -> child() } } + override fun updateLifecycleState(state: Lifecycle.State) { + super.updateLifecycleState(state) + childNodeLifecycleManager.propagateLifecycleToChildren(state) + } + /** * [NavModel.onTransitionFinished] is invoked by Composable function when animation is finished. * When application is in background, recomposition is paused, so we never invoke the callback. @@ -236,7 +242,7 @@ abstract class ParentNode( } result ?: throw IllegalStateException( "Expected child of type [${T::class.java}] was not found after executing action. " + - "Check that your action actually results in the expected child. " + "Check that your action actually results in the expected child. " ) } diff --git a/core/src/test/kotlin/com/bumble/appyx/core/lifecycle/ChildLifecycleTest.kt b/core/src/test/kotlin/com/bumble/appyx/core/lifecycle/ChildLifecycleTest.kt index 408fc12b9f..58d69d9e89 100644 --- a/core/src/test/kotlin/com/bumble/appyx/core/lifecycle/ChildLifecycleTest.kt +++ b/core/src/test/kotlin/com/bumble/appyx/core/lifecycle/ChildLifecycleTest.kt @@ -3,18 +3,20 @@ package com.bumble.appyx.core.lifecycle import androidx.arch.core.executor.testing.InstantTaskExecutorRule import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier +import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleOwner import com.bumble.appyx.core.children.nodeOrNull import com.bumble.appyx.core.modality.BuildContext -import com.bumble.appyx.core.node.Node -import com.bumble.appyx.core.node.ParentNode -import com.bumble.appyx.core.node.build import com.bumble.appyx.core.navigation.BaseNavModel import com.bumble.appyx.core.navigation.Operation import com.bumble.appyx.core.navigation.RoutingElement import com.bumble.appyx.core.navigation.RoutingElements import com.bumble.appyx.core.navigation.RoutingKey import com.bumble.appyx.core.navigation.onscreen.OnScreenStateResolver +import com.bumble.appyx.core.node.Node +import com.bumble.appyx.core.node.ParentNode +import com.bumble.appyx.core.node.build import com.bumble.appyx.testing.junit4.util.MainDispatcherRule import org.junit.Assert.assertEquals import org.junit.Rule @@ -43,6 +45,32 @@ class ChildLifecycleTest { ) } + @Test + fun `parent lifecycle callbacks is invoked before child`() { + var counter = 1 + + class TestResumedObserver : DefaultLifecycleObserver { + var isInvoked = 0 + override fun onResume(owner: LifecycleOwner) { + isInvoked = counter++ + } + } + + val parentObserver = TestResumedObserver() + val childObserver = TestResumedObserver() + + val parent = Parent(BuildContext.root(null)).build() + parent.lifecycle.addObserver(parentObserver) + parent.routing.add(key = "0", onScreen = true) + parent.updateLifecycleState(Lifecycle.State.STARTED) + parent.findChild()?.lifecycle?.addObserver(childObserver) + + parent.updateLifecycleState(Lifecycle.State.RESUMED) + + assertEquals(parentObserver.isInvoked, 1) + assertEquals(childObserver.isInvoked, 2) + } + @Test fun `off screen child is limited to created`() { val parent = Parent(BuildContext.root(null)).build() diff --git a/testing-ui/src/main/kotlin/com/bumble/appyx/testing/ui/rules/AppyxViewActivity.kt b/testing-ui/src/main/kotlin/com/bumble/appyx/testing/ui/rules/AppyxViewActivity.kt index 514c8176b1..47a2071a17 100644 --- a/testing-ui/src/main/kotlin/com/bumble/appyx/testing/ui/rules/AppyxViewActivity.kt +++ b/testing-ui/src/main/kotlin/com/bumble/appyx/testing/ui/rules/AppyxViewActivity.kt @@ -5,7 +5,7 @@ import androidx.activity.compose.setContent import androidx.compose.runtime.Composable import com.bumble.appyx.core.integrationpoint.NodeActivity -class AppyxViewActivity : NodeActivity() { +open class AppyxViewActivity : NodeActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState)