From 987069d4777872957fde628a275af6e09c9b9d7b Mon Sep 17 00:00:00 2001 From: Niek Haarman Date: Tue, 10 Mar 2020 14:09:53 +0100 Subject: [PATCH 1/2] Invoke Scene.onStart after listener notification for StackNavigator A Scene (A) that _immediately_ causes another transition to another Scene (B) when A's `onStart` method is invoked results in the wrong order of scene notifications if the listener invocation happens after starting the scene: First B is reported and only then A. Ensuring listener invocation happens before starting the Scene resolves this issue. --- ext/acorn/integrationtests/build.gradle | 14 +++ ...t cause a transition in their onStart().kt | 111 +++++++++++++++++ .../acorn/navigation/StackNavigator.kt | 7 +- .../acorn/navigation/StackNavigatorTest.kt | 117 ++++++++++++++++++ settings.gradle | 2 + 5 files changed, 247 insertions(+), 4 deletions(-) create mode 100644 ext/acorn/integrationtests/build.gradle create mode 100644 ext/acorn/integrationtests/src/test/java/com/nhaarman/acorn/integrationtests/StackNavigator tests for scenes that cause a transition in their onStart().kt diff --git a/ext/acorn/integrationtests/build.gradle b/ext/acorn/integrationtests/build.gradle new file mode 100644 index 00000000..64e1310b --- /dev/null +++ b/ext/acorn/integrationtests/build.gradle @@ -0,0 +1,14 @@ +plugins { + id("org.jetbrains.kotlin.jvm") + id("org.gradle.java-library") +} + +dependencies { + implementation "org.jetbrains.kotlin:kotlin-stdlib" + + testImplementation project(":ext-acorn-testing") + testImplementation "com.nhaarman:expect.kt" + testImplementation "com.nhaarman.mockitokotlin2:mockito-kotlin" + testImplementation "org.junit.jupiter:junit-jupiter-api" + testRuntime "org.junit.jupiter:junit-jupiter-engine" +} diff --git a/ext/acorn/integrationtests/src/test/java/com/nhaarman/acorn/integrationtests/StackNavigator tests for scenes that cause a transition in their onStart().kt b/ext/acorn/integrationtests/src/test/java/com/nhaarman/acorn/integrationtests/StackNavigator tests for scenes that cause a transition in their onStart().kt new file mode 100644 index 00000000..91432d68 --- /dev/null +++ b/ext/acorn/integrationtests/src/test/java/com/nhaarman/acorn/integrationtests/StackNavigator tests for scenes that cause a transition in their onStart().kt @@ -0,0 +1,111 @@ +/* + * Copyright 2018 Niek Haarman + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.nhaarman.acorn.integrationtests + +import com.nhaarman.acorn.navigation.Navigator +import com.nhaarman.acorn.navigation.StackNavigator +import com.nhaarman.acorn.presentation.Container +import com.nhaarman.acorn.presentation.Scene +import com.nhaarman.acorn.state.SceneState +import com.nhaarman.acorn.testing.ContainerProvider +import com.nhaarman.acorn.testing.TestContext +import com.nhaarman.acorn.testing.TestContext.Companion.testWith +import com.nhaarman.mockitokotlin2.any +import com.nhaarman.mockitokotlin2.anyOrNull +import com.nhaarman.mockitokotlin2.inOrder +import com.nhaarman.mockitokotlin2.mock +import kotlin.reflect.KClass +import org.junit.jupiter.api.Test + +/** + * When a Scene (A) _immediately_ causes another transition to + * another Scene (B) in its `onStart` method, a wrong order of + * scene notifications can occur if the listener invocation happens + * after starting the scene: First B is reported and only then A. + * + * These tests form an integration test to ensure proper behavior. + */ +class `StackNavigator tests for scenes that cause a transition in their onStart()` { + + private val navigator = MyNavigator() + private val navigatorListener = mock() + + private val immediatelyFinishingScene = ImmediatelyFinishingScene { navigator.pop() } + + private val context = TestContext.create( + navigator, + object : ContainerProvider { + override fun containerFor(scene: Scene<*>): Container { + return mock() + } + } + ) + + @Test + fun `pushing an immediately finishing scene results in proper navigator notifications`() = testWith(context) { + /* Given */ + navigator.addNavigatorEventsListener(navigatorListener) + + /* When */ + navigator.push(immediatelyFinishingScene) + + /* Then */ + inOrder(navigatorListener) { + verify(navigatorListener).scene(any(), anyOrNull()) + verify(navigatorListener).scene(any(), anyOrNull()) + verify(navigatorListener).scene(any(), anyOrNull()) + verifyNoMoreInteractions() + } + } + + @Test + fun `replacing an immediately finishing scene results in proper navigator notifications`() = testWith(context) { + /* Given */ + navigator.addNavigatorEventsListener(navigatorListener) + + /* When */ + navigator.replace(immediatelyFinishingScene) + + /* Then */ + inOrder(navigatorListener) { + verify(navigatorListener).scene(any(), anyOrNull()) + verify(navigatorListener).scene(any(), anyOrNull()) + verify(navigatorListener).finished() + verifyNoMoreInteractions() + } + } + + private class InitialScene : Scene + + private class ImmediatelyFinishingScene(private val onFinish: () -> Unit) : Scene { + + override fun onStart() { + onFinish() + } + } + + private class MyNavigator : StackNavigator(null) { + + override fun initialStack(): List> { + return listOf(InitialScene()) + } + + override fun instantiateScene(sceneClass: KClass>, state: SceneState?): Scene { + error("Not used") + } + } +} diff --git a/ext/acorn/src/main/java/com/nhaarman/acorn/navigation/StackNavigator.kt b/ext/acorn/src/main/java/com/nhaarman/acorn/navigation/StackNavigator.kt index eecd2c38..2875000f 100644 --- a/ext/acorn/src/main/java/com/nhaarman/acorn/navigation/StackNavigator.kt +++ b/ext/acorn/src/main/java/com/nhaarman/acorn/navigation/StackNavigator.kt @@ -366,8 +366,8 @@ abstract class StackNavigator( override fun push(scene: Scene, data: TransitionData?): StateTransition { return StateTransition(Active(scenes + scene, listeners)) { scenes.last().onStop() - scene.onStart() listeners.forEach { it.scene(scene, data) } + scene.onStart() } } @@ -386,9 +386,8 @@ abstract class StackNavigator( poppedScene.onStop() poppedScene.onDestroy() - newScenes.last().onStart() - listeners.forEach { it.scene(newScenes.last(), TransitionData.backwards) } + newScenes.last().onStart() } } } @@ -404,8 +403,8 @@ abstract class StackNavigator( poppedScene.onStop() poppedScene.onDestroy() - scene.onStart() listeners.forEach { it.scene(scene, data) } + scene.onStart() } } diff --git a/ext/acorn/src/test/java/com/nhaarman/acorn/navigation/StackNavigatorTest.kt b/ext/acorn/src/test/java/com/nhaarman/acorn/navigation/StackNavigatorTest.kt index 8112ae7f..7d68776c 100644 --- a/ext/acorn/src/test/java/com/nhaarman/acorn/navigation/StackNavigatorTest.kt +++ b/ext/acorn/src/test/java/com/nhaarman/acorn/navigation/StackNavigatorTest.kt @@ -25,6 +25,7 @@ import com.nhaarman.acorn.state.navigatorState import com.nhaarman.expect.expect import com.nhaarman.mockitokotlin2.any import com.nhaarman.mockitokotlin2.anyOrNull +import com.nhaarman.mockitokotlin2.eq import com.nhaarman.mockitokotlin2.inOrder import com.nhaarman.mockitokotlin2.mock import com.nhaarman.mockitokotlin2.never @@ -414,6 +415,57 @@ internal class StackNavigatorTest { expect(listener.lastSavableScene).toBeNull() } + @Test + fun `replacing a scene for inactive navigator does not notify listeners`() { + /* Given */ + navigator.addNavigatorEventsListener(listener) + + /* When */ + navigator.replace(scene2) + + /* Then */ + expect(listener.lastSavableScene).toBeNull() + } + + @Test + fun `replacing a scene for destroyed navigator does not notify listeners`() { + /* Given */ + navigator.addNavigatorEventsListener(listener) + navigator.onDestroy() + + /* When */ + navigator.replace(scene2) + + /* Then */ + expect(listener.lastSavableScene).toBeNull() + } + + @Test + fun `replacing a scene for active navigator does notify listeners`() { + /* Given */ + navigator.addNavigatorEventsListener(listener) + navigator.onStart() + + /* When */ + navigator.replace(scene2) + + /* Then */ + expect(listener.lastSavableScene).toBe(scene2) + } + + @Test + fun `replacing a scene for active navigator - scene notification has forward transition data`() { + /* Given */ + navigator.addNavigatorEventsListener(listener) + navigator.onStart() + + /* When */ + navigator.replace(scene2) + + /* Then */ + expect(listener.lastTransitionData?.isBackwards).toBe(false) + } + @Test fun `onBackPressed for single scene stack notifies listeners of finished`() { /* Given */ @@ -1177,6 +1229,71 @@ internal class StackNavigatorTest { } } + @Nested + inner class `Order of scene start and listener invocation` { + + /** + * A Scene (A) that _immediately_ causes another transition to + * another Scene (B) when A's `onStart` method is invoked results + * in the wrong order of scene notifications if the listener + * invocation happens after starting the scene: First B is reported + * and only then A. + * + * Ensuring listener invocation happens before starting the Scene + * resolves this issue. + */ + + @Test + fun `pushing a scene invokes listeners before starting the new scene`() { + /* Given */ + navigator.addNavigatorEventsListener(listener) + navigator.onStart() + + /* When */ + navigator.push(scene2) + + /* Then */ + inOrder(listener, scene2) { + verify(listener).scene(eq(scene2), anyOrNull()) + verify(scene2).onStart() + } + } + + @Test + fun `replacing a scene invokes listeners before starting the new scene`() { + /* Given */ + navigator.addNavigatorEventsListener(listener) + navigator.onStart() + + /* When */ + navigator.replace(scene2) + + /* Then */ + inOrder(listener, scene2) { + verify(listener).scene(eq(scene2), anyOrNull()) + verify(scene2).onStart() + } + } + + @Test + fun `popping a scene invokes listeners before starting the new scene`() { + /* Given */ + navigator.addNavigatorEventsListener(listener) + navigator.onStart() + navigator.push(scene2) + + /* When */ + navigator.pop() + + /* Then */ + inOrder(listener, scene1) { + verify(scene1).onStop() + verify(listener).scene(eq(scene1), anyOrNull()) + verify(scene1).onStart() + } + } + } + class TestStackNavigator( private val initialStack: List ) : StackNavigator(null) { diff --git a/settings.gradle b/settings.gradle index 7241a705..807fb97c 100644 --- a/settings.gradle +++ b/settings.gradle @@ -7,6 +7,7 @@ include(":ext-acorn") include(":ext-acorn-testing") include(":ext-acorn-rx") include(":ext-acorn-experimental") +include(":ext-acorn-integrationtests") include(":ext-acorn-android") include(":ext-acorn-android-testing") @@ -39,6 +40,7 @@ project(":ext-acorn").projectDir = file("ext/acorn") project(":ext-acorn-testing").projectDir = file("ext/acorn/acorn-testing") project(":ext-acorn-rx").projectDir = file("ext/acorn/acorn-rx") project(":ext-acorn-experimental").projectDir = file("ext/acorn/acorn-experimental") +project(":ext-acorn-integrationtests").projectDir = file("ext/acorn/integrationtests") project(":ext-acorn-android").projectDir = file("ext/acorn-android") project(":ext-acorn-android-testing").projectDir = file("ext/acorn-android/acorn-android-testing") From cb62451d4bf3b72dec1d4fa33126443db30f7f1d Mon Sep 17 00:00:00 2001 From: Niek Haarman Date: Tue, 10 Mar 2020 14:30:51 +0100 Subject: [PATCH 2/2] Invoke Scene.onStart after listener notification for ReplacingNavigator A Scene (A) that _immediately_ causes another transition to another Scene (B) when A's `onStart` method is invoked results in the wrong order of scene notifications if the listener invocation happens after starting the scene: First B is reported and only then A. Ensuring listener invocation happens before starting the Scene resolves this issue. --- ...t cause a transition in their onStart().kt | 94 +++++++++++++++++++ .../acorn/navigation/ReplacingNavigator.kt | 15 ++- .../navigation/ReplacingNavigatorTest.kt | 32 +++++++ 3 files changed, 132 insertions(+), 9 deletions(-) create mode 100644 ext/acorn/integrationtests/src/test/java/com/nhaarman/acorn/integrationtests/ReplacingNavigator tests for scenes that cause a transition in their onStart().kt diff --git a/ext/acorn/integrationtests/src/test/java/com/nhaarman/acorn/integrationtests/ReplacingNavigator tests for scenes that cause a transition in their onStart().kt b/ext/acorn/integrationtests/src/test/java/com/nhaarman/acorn/integrationtests/ReplacingNavigator tests for scenes that cause a transition in their onStart().kt new file mode 100644 index 00000000..48d843e5 --- /dev/null +++ b/ext/acorn/integrationtests/src/test/java/com/nhaarman/acorn/integrationtests/ReplacingNavigator tests for scenes that cause a transition in their onStart().kt @@ -0,0 +1,94 @@ +/* + * Copyright 2018 Niek Haarman + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.nhaarman.acorn.integrationtests + +import com.nhaarman.acorn.navigation.Navigator +import com.nhaarman.acorn.navigation.ReplacingNavigator +import com.nhaarman.acorn.presentation.Container +import com.nhaarman.acorn.presentation.Scene +import com.nhaarman.acorn.state.SceneState +import com.nhaarman.acorn.testing.ContainerProvider +import com.nhaarman.acorn.testing.TestContext +import com.nhaarman.acorn.testing.TestContext.Companion.testWith +import com.nhaarman.mockitokotlin2.any +import com.nhaarman.mockitokotlin2.anyOrNull +import com.nhaarman.mockitokotlin2.inOrder +import com.nhaarman.mockitokotlin2.mock +import kotlin.reflect.KClass +import org.junit.jupiter.api.Test + +/** + * When a Scene (A) _immediately_ causes another transition to + * another Scene (B) in its `onStart` method, a wrong order of + * scene notifications can occur if the listener invocation happens + * after starting the scene: First B is reported and only then A. + * + * These tests form an integration test to ensure proper behavior. + */ +class `ReplacingNavigator tests for scenes that cause a transition in their onStart()` { + + private val navigator = MyNavigator() + private val navigatorListener = mock() + + private val immediatelyFinishingScene = ImmediatelyFinishingScene { navigator.finish() } + + private val context = TestContext.create( + navigator, + object : ContainerProvider { + override fun containerFor(scene: Scene<*>): Container { + return mock() + } + } + ) + + @Test + fun `replacing an immediately finishing scene results in proper navigator notifications`() = testWith(context) { + /* Given */ + navigator.addNavigatorEventsListener(navigatorListener) + + /* When */ + navigator.replace(immediatelyFinishingScene) + + /* Then */ + inOrder(navigatorListener) { + verify(navigatorListener).scene(any(), anyOrNull()) + verify(navigatorListener).scene(any(), anyOrNull()) + verify(navigatorListener).finished() + verifyNoMoreInteractions() + } + } + + private class InitialScene : Scene + + private class ImmediatelyFinishingScene(private val onFinish: () -> Unit) : Scene { + + override fun onStart() { + onFinish() + } + } + + private class MyNavigator : ReplacingNavigator(null) { + + override fun initialScene(): Scene { + return InitialScene() + } + + override fun instantiateScene(sceneClass: KClass>, state: SceneState?): Scene { + error("Not used") + } + } +} diff --git a/ext/acorn/src/main/java/com/nhaarman/acorn/navigation/ReplacingNavigator.kt b/ext/acorn/src/main/java/com/nhaarman/acorn/navigation/ReplacingNavigator.kt index 04f44b3f..d59b69b4 100644 --- a/ext/acorn/src/main/java/com/nhaarman/acorn/navigation/ReplacingNavigator.kt +++ b/ext/acorn/src/main/java/com/nhaarman/acorn/navigation/ReplacingNavigator.kt @@ -86,11 +86,7 @@ abstract class ReplacingNavigator( * @param data Any transition data for this transition. */ fun replace(newScene: Scene, data: TransitionData? = null) { - execute(state.replaceWith(newScene)) - - if (state is LifecycleState.Active) { - state.listeners.forEach { it.scene(state.scene, data) } - } + execute(state.replaceWith(newScene, data)) } /** @@ -201,7 +197,7 @@ abstract class ReplacingNavigator( abstract fun stop(): StateTransition abstract fun destroy(): StateTransition - abstract fun replaceWith(scene: Scene): StateTransition + abstract fun replaceWith(scene: Scene, data: TransitionData?): StateTransition abstract fun finish(): StateTransition companion object { @@ -241,7 +237,7 @@ abstract class ReplacingNavigator( } } - override fun replaceWith(scene: Scene): StateTransition { + override fun replaceWith(scene: Scene, data: TransitionData?): StateTransition { return StateTransition(Inactive(scene, listeners)) } @@ -283,10 +279,11 @@ abstract class ReplacingNavigator( } } - override fun replaceWith(scene: Scene): StateTransition { + override fun replaceWith(scene: Scene, data: TransitionData?): StateTransition { return StateTransition(Active(scene, listeners)) { this.scene.onStop() this.scene.onDestroy() + listeners.forEach { it.scene(scene, data) } scene.onStart() } } @@ -323,7 +320,7 @@ abstract class ReplacingNavigator( return StateTransition(this) } - override fun replaceWith(scene: Scene): StateTransition { + override fun replaceWith(scene: Scene, data: TransitionData?): StateTransition { w("LifecycleState", "Warning: Cannot replace scene after state is destroyed.") return StateTransition(this) } diff --git a/ext/acorn/src/test/java/com/nhaarman/acorn/navigation/ReplacingNavigatorTest.kt b/ext/acorn/src/test/java/com/nhaarman/acorn/navigation/ReplacingNavigatorTest.kt index 5373c53e..5a2c1c6d 100644 --- a/ext/acorn/src/test/java/com/nhaarman/acorn/navigation/ReplacingNavigatorTest.kt +++ b/ext/acorn/src/test/java/com/nhaarman/acorn/navigation/ReplacingNavigatorTest.kt @@ -24,6 +24,7 @@ import com.nhaarman.acorn.state.SceneState import com.nhaarman.expect.expect import com.nhaarman.mockitokotlin2.any import com.nhaarman.mockitokotlin2.anyOrNull +import com.nhaarman.mockitokotlin2.eq import com.nhaarman.mockitokotlin2.inOrder import com.nhaarman.mockitokotlin2.mock import com.nhaarman.mockitokotlin2.never @@ -669,6 +670,37 @@ internal class ReplacingNavigatorTest { } } + @Nested + inner class `Order of scene start and listener invocation` { + + /** + * A Scene (A) that _immediately_ causes another transition to + * another Scene (B) when A's `onStart` method is invoked results + * in the wrong order of scene notifications if the listener + * invocation happens after starting the scene: First B is reported + * and only then A. + * + * Ensuring listener invocation happens before starting the Scene + * resolves this issue. + */ + + @Test + fun `replacing a scene invokes listeners before starting the new scene`() { + /* Given */ + navigator.addNavigatorEventsListener(listener) + navigator.onStart() + + /* When */ + navigator.replace(scene1) + + /* Then */ + inOrder(listener, scene1) { + verify(listener).scene(eq(scene1), anyOrNull()) + verify(scene1).onStart() + } + } + } + private open class TestListener : Navigator.Events { val scenes = mutableListOf>()