Merge pull request #163 from nhaarman/listenerinvocationsordering

Invoke Scene.onStart after listener notification

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"
+}

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<Navigator.Events>()
+
+    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<InitialScene>(), anyOrNull())
+            verify(navigatorListener).scene(any<ImmediatelyFinishingScene>(), anyOrNull())
+            verify(navigatorListener).finished()
+            verifyNoMoreInteractions()
+        }
+    }
+
+    private class InitialScene : Scene<Container>
+
+    private class ImmediatelyFinishingScene(private val onFinish: () -> Unit) : Scene<Container> {
+
+        override fun onStart() {
+            onFinish()
+        }
+    }
+
+    private class MyNavigator : ReplacingNavigator(null) {
+
+        override fun initialScene(): Scene<out Container> {
+            return InitialScene()
+        }
+
+        override fun instantiateScene(sceneClass: KClass<out Scene<*>>, state: SceneState?): Scene<out Container> {
+            error("Not used")
+        }
+    }
+}

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<Navigator.Events>()
+
+    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<InitialScene>(), anyOrNull())
+            verify(navigatorListener).scene(any<ImmediatelyFinishingScene>(), anyOrNull())
+            verify(navigatorListener).scene(any<InitialScene>(), 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<InitialScene>(), anyOrNull())
+            verify(navigatorListener).scene(any<ImmediatelyFinishingScene>(), anyOrNull())
+            verify(navigatorListener).finished()
+            verifyNoMoreInteractions()
+        }
+    }
+
+    private class InitialScene : Scene<Container>
+
+    private class ImmediatelyFinishingScene(private val onFinish: () -> Unit) : Scene<Container> {
+
+        override fun onStart() {
+            onFinish()
+        }
+    }
+
+    private class MyNavigator : StackNavigator(null) {
+
+        override fun initialStack(): List<Scene<out Container>> {
+            return listOf(InitialScene())
+        }
+
+        override fun instantiateScene(sceneClass: KClass<out Scene<*>>, state: SceneState?): Scene<out Container> {
+            error("Not used")
+        }
+    }
+}

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<out Container>, 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<out Container>): StateTransition
+        abstract fun replaceWith(scene: Scene<out Container>, data: TransitionData?): StateTransition
         abstract fun finish(): StateTransition
 
         companion object {
@@ -241,7 +237,7 @@ abstract class ReplacingNavigator(
                 }
             }
 
-            override fun replaceWith(scene: Scene<out Container>): StateTransition {
+            override fun replaceWith(scene: Scene<out Container>, data: TransitionData?): StateTransition {
                 return StateTransition(Inactive(scene, listeners))
             }
 
@@ -283,10 +279,11 @@ abstract class ReplacingNavigator(
                 }
             }
 
-            override fun replaceWith(scene: Scene<out Container>): StateTransition {
+            override fun replaceWith(scene: Scene<out Container>, 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<out Container>): StateTransition {
+            override fun replaceWith(scene: Scene<out Container>, data: TransitionData?): StateTransition {
                 w("LifecycleState", "Warning: Cannot replace scene after state is destroyed.")
                 return StateTransition(this)
             }

ext/acorn/src/main/java/com/nhaarman/acorn/navigation/StackNavigator.kt

@@ -366,8 +366,8 @@ abstract class StackNavigator(
             override fun push(scene: Scene<out Container>, 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()
                 }
             }
 

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<Scene<out Container>>()