Reuse PermanentNavModel if provided in the list of navModels

bumble-tech · Sep 29, 2023 · d6f40a5 · d6f40a5
1 parent 1c13ab4
commit d6f40a5
Show file tree

Hide file tree

Showing 5 changed files with 187 additions and 27 deletions.
diff --git a/libraries/core/src/androidTest/kotlin/com/bumble/appyx/core/node/PermanentChildTest.kt b/libraries/core/src/androidTest/kotlin/com/bumble/appyx/core/node/PermanentChildTest.kt
@@ -13,23 +13,30 @@ import com.bumble.appyx.core.AppyxTestScenario
 import com.bumble.appyx.core.children.nodeOrNull
 import com.bumble.appyx.core.modality.BuildContext
 import com.bumble.appyx.core.navigation.EmptyNavModel
+import com.bumble.appyx.core.navigation.model.permanent.PermanentNavModel
+import com.bumble.appyx.core.node.PermanentChildTest.TestParentNode.NavTarget
 import kotlinx.parcelize.Parcelize
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
 import org.junit.Rule
 import org.junit.Test
 
 class PermanentChildTest {
 
+    var nodeFactory: (buildContext: BuildContext) -> TestParentNode = {
+        TestParentNode(null, it)
+    }
+
     @get:Rule
     val rule = AppyxTestScenario { buildContext ->
-        TestParentNode(buildContext)
+        nodeFactory(buildContext)
     }
 
     @Test
     fun permanent_child_is_rendered() {
         rule.start()
 
-        rule.onNode(hasTestTag(TestParentNode.NavTarget::class.java.name)).assertExists()
+        rule.onNode(hasTestTag(NavTarget::class.java.name)).assertExists()
     }
 
     @Test
@@ -38,38 +45,80 @@ class PermanentChildTest {
         rule.node.renderPermanentChild = false
         val childNodes = rule.node.children.value.values.map { it.nodeOrNull }
 
-        rule.onNode(hasTestTag(TestParentNode.NavTarget::class.java.name)).assertDoesNotExist()
+        rule.onNode(hasTestTag(NavTarget::class.java.name)).assertDoesNotExist()
 
         rule.node.renderPermanentChild = true
 
-        rule.onNode(hasTestTag(TestParentNode.NavTarget::class.java.name)).assertExists()
+        rule.onNode(hasTestTag(NavTarget::class.java.name)).assertExists()
         assertEquals(childNodes, rule.node.children.value.values.map { it.nodeOrNull })
     }
 
+    @Test
+    fun given_permanent_model_with_key_When_PermanentChild_with_the_same_key_Then_has_one_child() {
+        val permanentNavModel = PermanentNavModel<NavTarget>(NavTarget.Child1, savedStateMap = null)
+        nodeFactory = { buildContext ->
+            TestParentNode(permanentNavModel, buildContext)
+        }
+
+        rule.start()
+
+        val childNodes = rule.node.children.value.values.map { it.nodeOrNull }
+        assertTrue(childNodes.count() == 1)
+    }
+
+    @Test
+    fun given_permanent_model_with_key_When_PermanentChild_add_new_key_Then_has_two_children() {
+        val permanentNavModel = PermanentNavModel<NavTarget>(NavTarget.Child2, savedStateMap = null)
+        nodeFactory = { buildContext ->
+            TestParentNode(permanentNavModel, buildContext)
+        }
+
+        rule.start()
+
+        val childNodes = rule.node.children.value.values.map { it.nodeOrNull }
+        assertTrue(childNodes.count() == 2)
+    }
+
     class TestParentNode(
+        private val permanentNavModel: PermanentNavModel<NavTarget>? = null,
         buildContext: BuildContext,
-    ) : ParentNode<TestParentNode.NavTarget>(
+    ) : ParentNode<NavTarget>(
         buildContext = buildContext,
-        navModel = EmptyNavModel<NavTarget, Any>(),
+        navModel = permanentNavModel ?: EmptyNavModel<NavTarget, Any>(),
     ) {
 
-        @Parcelize
-        object NavTarget : Parcelable
+        sealed class NavTarget : Parcelable {
+            @Parcelize
+            object Child1 : NavTarget()
+
+            @Parcelize
+            object Child2 : NavTarget()
+        }
 
         var renderPermanentChild by mutableStateOf(true)
 
-        override fun resolve(navTarget: NavTarget, buildContext: BuildContext): Node =
-            node(buildContext) { modifier ->
+
+        class ChildNode(
+            private val navTarget: NavTarget,
+            buildContext: BuildContext
+        ) : Node(buildContext) {
+
+            @Composable
+            override fun View(modifier: Modifier) {
                 BasicText(
                     text = navTarget.toString(),
                     modifier = modifier.testTag(NavTarget::class.java.name),
                 )
             }
+        }
+
+        override fun resolve(navTarget: NavTarget, buildContext: BuildContext): Node =
+            ChildNode(navTarget, buildContext)
 
         @Composable
         override fun View(modifier: Modifier) {
             if (renderPermanentChild) {
-                PermanentChild(NavTarget)
+                PermanentChild(NavTarget.Child1)
             }
         }
     }

diff --git a/.../core/src/main/kotlin/com/bumble/appyx/core/navigation/model/combined/CombinedNavModel.kt b/.../core/src/main/kotlin/com/bumble/appyx/core/navigation/model/combined/CombinedNavModel.kt
@@ -6,6 +6,7 @@ import com.bumble.appyx.core.navigation.NavElements
 import com.bumble.appyx.core.navigation.NavKey
 import com.bumble.appyx.core.navigation.NavModel
 import com.bumble.appyx.core.navigation.NavModelAdapter
+import com.bumble.appyx.core.navigation.model.permanent.PermanentNavModel
 import com.bumble.appyx.core.plugin.Destroyable
 import com.bumble.appyx.core.state.MutableSavedStateMap
 import kotlinx.coroutines.CoroutineScope
@@ -18,6 +19,13 @@ class CombinedNavModel<NavTarget>(
     val navModels: List<NavModel<NavTarget, *>>,
 ) : NavModel<NavTarget, Any?>, Destroyable {
 
+    init {
+        val permanentNavModelCount = navModels.filterIsInstance<PermanentNavModel<*>>().count()
+        check(permanentNavModelCount <= MAX_PERMANENT_NAV_MODEL_COUNT) {
+            "Do not provide more than one PermanentNavModel"
+        }
+    }
+
     constructor(vararg navModels: NavModel<NavTarget, *>) : this(navModels.toList())
 
     private val scope = CoroutineScope(EmptyCoroutineContext + Dispatchers.Unconfined)
@@ -61,4 +69,8 @@ class CombinedNavModel<NavTarget>(
         navModels.filterIsInstance<Destroyable>().forEach { it.destroy() }
     }
 
+    companion object {
+        private const val MAX_PERMANENT_NAV_MODEL_COUNT = 1
+    }
+
 }
diff --git a/libraries/core/src/main/kotlin/com/bumble/appyx/core/node/ParentNode.kt b/libraries/core/src/main/kotlin/com/bumble/appyx/core/node/ParentNode.kt
@@ -32,6 +32,7 @@ import com.bumble.appyx.core.navigation.NavKey
 import com.bumble.appyx.core.navigation.NavModel
 import com.bumble.appyx.core.navigation.Resolver
 import com.bumble.appyx.core.navigation.isTransitioning
+import com.bumble.appyx.core.navigation.model.combined.CombinedNavModel
 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.addUnique
@@ -62,11 +63,56 @@ abstract class ParentNode<NavTarget : Any>(
     plugins = plugins + navModel + childAware
 ), Resolver<NavTarget> {
 
-    private val permanentNavModel = PermanentNavModel<NavTarget>(
-        savedStateMap = buildContext.savedStateMap,
-        key = KEY_PERMANENT_NAV_MODEL,
-    )
-    val navModel: NavModel<NavTarget, *> = permanentNavModel + navModel
+    private lateinit var permanentNavModel: PermanentNavModel<NavTarget>
+    private var isPermanentNavModelProvided: Boolean? = null
+    val navModel: NavModel<NavTarget, *> = resolveNavModels(navModel, buildContext)
+
+    /**
+     * If PermanentNavModel is provided in the constructor, it will be retrieved and used. Original NavModel
+     * will not be modified. Otherwise, PermanentNavModel will be created internally, and combined with
+     * the original NavModel. In ths case it's not added to the list of plugins, and we'll have to call
+     * onSavedInstanceState manually.
+     */
+    private fun resolveNavModels(
+        navModel: NavModel<NavTarget, *>,
+        buildContext: BuildContext,
+    ): NavModel<NavTarget, *> {
+
+        val existingPermanentNavModel = retrievePermanentNavModel(navModel)
+        isPermanentNavModelProvided = existingPermanentNavModel != null
+
+        return if (existingPermanentNavModel != null) {
+            permanentNavModel = existingPermanentNavModel
+            navModel
+        } else {
+            permanentNavModel = PermanentNavModel(
+                savedStateMap = buildContext.savedStateMap,
+                key = KEY_PERMANENT_NAV_MODEL,
+            )
+            navModel + permanentNavModel
+        }
+    }
+
+    /**
+     * If PermanentNavModel is provided in the constructor, it will be retrieved and used.
+     * Otherwise, it will be created internally.
+     */
+    private fun retrievePermanentNavModel(navModel: NavModel<NavTarget, *>): PermanentNavModel<NavTarget>? =
+        when (navModel) {
+            is CombinedNavModel<NavTarget> -> {
+                navModel
+                    .navModels
+                    .find { it is PermanentNavModel<NavTarget> } as PermanentNavModel<NavTarget>?
+            }
+            is PermanentNavModel<NavTarget> -> navModel
+            else -> null
+        }
+
+    private fun createPermanentNavModel(buildContext: BuildContext): PermanentNavModel<NavTarget> =
+        PermanentNavModel(
+            savedStateMap = buildContext.savedStateMap,
+            key = KEY_PERMANENT_NAV_MODEL,
+        )
 
     private val childNodeCreationManager = ChildNodeCreationManager<NavTarget>(
         savedStateMap = buildContext.savedStateMap,
@@ -225,8 +271,10 @@ abstract class ParentNode<NavTarget : Any>(
     @CallSuper
     override fun onSaveInstanceState(state: MutableSavedStateMap) {
         super.onSaveInstanceState(state)
-        // permanentNavModel is not provided as a plugin, store manually
-        permanentNavModel.saveInstanceState(state)
+        if (isPermanentNavModelProvided != true) {
+            // permanentNavModel is not provided as a plugin, store manually
+            permanentNavModel.saveInstanceState(state)
+        }
         childNodeCreationManager.saveChildrenState(state)
     }
 

diff --git a/libraries/core/src/test/kotlin/com/bumble/appyx/core/node/ParentNodeTest.kt b/libraries/core/src/test/kotlin/com/bumble/appyx/core/node/ParentNodeTest.kt
@@ -1,11 +1,14 @@
 package com.bumble.appyx.core.node
 
 import androidx.arch.core.executor.testing.InstantTaskExecutorRule
+import com.bumble.appyx.core.children.nodeOrNull
 import com.bumble.appyx.core.modality.BuildContext
+import com.bumble.appyx.core.navigation.model.permanent.PermanentNavModel
+import com.bumble.appyx.core.navigation.model.permanent.operation.addUnique
+import com.bumble.appyx.core.node.ParentNodeTest.NavTarget.ChildA
+import com.bumble.appyx.core.node.ParentNodeTest.NavTarget.ChildB
 import com.bumble.appyx.core.node.ParentNodeTest.NodeB.Companion.StatusExecuted
-import com.bumble.appyx.core.node.ParentNodeTest.TestParentNode.NavTarget
-import com.bumble.appyx.core.node.ParentNodeTest.TestParentNode.NavTarget.ChildA
-import com.bumble.appyx.core.node.ParentNodeTest.TestParentNode.NavTarget.ChildB
+import com.bumble.appyx.core.state.SavedStateMap
 import com.bumble.appyx.navmodel.backstack.BackStack
 import com.bumble.appyx.navmodel.backstack.operation.push
 import com.bumble.appyx.testing.junit4.util.MainDispatcherRule
@@ -75,24 +78,67 @@ class ParentNodeTest {
             assertTrue(attachedNode is NodeB)
         }
 
+    @Test
+    fun `GIVEN node with PermanentNavModel WHEN saves state THEN restores state correctly`() =
+        testScope.runTest {
+            //given
+            val permanentNavModel: PermanentNavModel<NavTarget> =
+                PermanentNavModel(ChildA, savedStateMap = null)
+            val node = buildParentNode(permanentNavModel = permanentNavModel)
+            assertChildrenCount(node, 1)
+
+            // when
+            permanentNavModel.addUnique(ChildB)
+            assertChildrenCount(node, 2)
+            val state = node.saveInstanceState { true }
+            val restoredStateNode =
+                buildParentNode(permanentNavModel = permanentNavModel, savedStateMap = state)
+
+            //then
+            assertChildrenCount(restoredStateNode, 2)
+        }
+
+    private fun assertChildrenCount(node: ParentNode<*>, expectedCount: Int) {
+        val childrenCount = node.children.value.values.mapNotNull { it.nodeOrNull }.count()
+        assertTrue(childrenCount == expectedCount)
+    }
+
     private fun buildBackStack(initialElement: NavTarget = ChildA) =
         BackStack(initialElement = initialElement, savedStateMap = null)
 
     private fun buildParentNode(backStack: BackStack<NavTarget>) =
         TestParentNode(backStack).apply { onBuilt() }
 
+    private fun buildParentNode(
+        savedStateMap: SavedStateMap? = null,
+        permanentNavModel: PermanentNavModel<NavTarget>
+    ) =
+        TestPermanentModelParentNode(
+            savedStateMap = savedStateMap,
+            permanentNavModel = permanentNavModel
+        ).apply { onBuilt() }
+
+    private class TestPermanentModelParentNode(
+        savedStateMap: SavedStateMap? = null,
+        permanentNavModel: PermanentNavModel<NavTarget>
+    ) : ParentNode<NavTarget>(
+        buildContext = BuildContext.root(savedStateMap),
+        navModel = permanentNavModel
+    ) {
+
+        override fun resolve(navTarget: NavTarget, buildContext: BuildContext) = when (navTarget) {
+            ChildA -> node(buildContext) {}
+            ChildB -> NodeB(buildContext)
+        }
+    }
+
     private class TestParentNode(
         private val backStack: BackStack<NavTarget>
     ) : ParentNode<NavTarget>(
         buildContext = BuildContext.root(null),
         navModel = backStack
     ) {
 
-        sealed class NavTarget {
-            object ChildA : NavTarget()
-            object ChildB : NavTarget()
-        }
-
         suspend fun waitForBAttached(): NodeB {
             return waitForChildAttached()
         }
@@ -115,6 +161,11 @@ class ParentNodeTest {
         }
     }
 
+    private sealed class NavTarget {
+        object ChildA : NavTarget()
+        object ChildB : NavTarget()
+    }
+
     private class NodeB(buildContext: BuildContext) : Node(buildContext) {
 
         var status: String? = null

diff --git a/.../sandbox/src/main/kotlin/com/bumble/appyx/sandbox/client/combined/CombinedNavModelNode.kt b/.../sandbox/src/main/kotlin/com/bumble/appyx/sandbox/client/combined/CombinedNavModelNode.kt
@@ -23,12 +23,12 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.unit.dp
 import com.bumble.appyx.core.composable.Children
 import com.bumble.appyx.core.modality.BuildContext
+import com.bumble.appyx.core.navigation.model.combined.plus
 import com.bumble.appyx.core.node.Node
 import com.bumble.appyx.core.node.ParentNode
 import com.bumble.appyx.navmodel.backstack.BackStack
 import com.bumble.appyx.navmodel.backstack.operation.push
 import com.bumble.appyx.navmodel.backstack.transitionhandler.rememberBackstackFader
-import com.bumble.appyx.core.navigation.model.combined.plus
 import com.bumble.appyx.sandbox.client.child.ChildNode
 import com.bumble.appyx.sandbox.client.combined.CombinedNavModelNode.NavTarget.Dynamic.Child
 import kotlinx.parcelize.Parcelize