Skip to content
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

Reuse PermanentNavModel if provided in the list of navModels #605

Closed
wants to merge 6 commits into from
+216 −30
Closed

Reuse PermanentNavModel if provided in the list of navModels #605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d6f40a5
Reuse PermanentNavModel if provided in the list of navModels
KovalevAndrey Sep 29, 2023
951653b
Update CHANGELOG.md
KovalevAndrey Sep 29, 2023
a6bd3de
Remove redundant comment
KovalevAndrey Sep 29, 2023
4db8192
Add CombinedNavModel test
KovalevAndrey Sep 29, 2023
3b9acf8
Use createPermanentNavModel method
KovalevAndrey Sep 29, 2023
0230d63
Check that we're not nesting CombinedNavModel
KovalevAndrey Sep 29, 2023
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

## Pending changes

-
- [#605](https://github.com/bumble-tech/appyx/pull/605) – **Fixed**: Reuse PermanentNavModel if provided in the list of NavModels

---

Expand Down
71 changes: 60 additions & 11 deletions libraries/core/src/androidTest/kotlin/com/bumble/appyx/core/node/PermanentChildTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
}
}
}
Expand Down
17 changes: 17 additions & 0 deletions .../core/src/main/kotlin/com/bumble/appyx/core/navigation/model/combined/CombinedNavModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -18,6 +19,18 @@ class CombinedNavModel<NavTarget>(
val navModels: List<NavModel<NavTarget, *>>,
) : NavModel<NavTarget, Any?>, Destroyable {

init {
val permanentNavModelCount = navModels.count { it is PermanentNavModel<*> }
check(permanentNavModelCount <= MAX_PERMANENT_NAV_MODEL_COUNT) {
"CombinedNavModel does not support more than one PermanentNavModel"
}

val hasNoNestedCombinedNavModel = navModels.count { it is CombinedNavModel<*> } == 0
check(hasNoNestedCombinedNavModel) {
"CombinedNavModel does not support nested CombinedNavModel"
}
}

constructor(vararg navModels: NavModel<NavTarget, *>) : this(navModels.toList())

private val scope = CoroutineScope(EmptyCoroutineContext + Dispatchers.Unconfined)
Expand Down Expand Up @@ -61,4 +74,8 @@ class CombinedNavModel<NavTarget>(
navModels.filterIsInstance<Destroyable>().forEach { it.destroy() }
}

companion object {
private const val MAX_PERMANENT_NAV_MODEL_COUNT = 1
}

}
56 changes: 49 additions & 7 deletions libraries/core/src/main/kotlin/com/bumble/appyx/core/node/ParentNode.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -62,11 +63,50 @@ 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 isPermanentNavModelCreated: Boolean = false
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)

return if (existingPermanentNavModel != null) {
permanentNavModel = existingPermanentNavModel
navModel
} else {
permanentNavModel = createPermanentNavModel(buildContext)
navModel + permanentNavModel
}
}

private fun retrievePermanentNavModel(navModel: NavModel<NavTarget, *>): PermanentNavModel<NavTarget>? =
when (navModel) {
is CombinedNavModel<NavTarget> -> {
navModel
.navModels
.find { it is PermanentNavModel<NavTarget> } as PermanentNavModel<NavTarget>?
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like it's possible (though maybe not advised?) that a developer could nest a CombinedNavModel within a CombinedNavModel.

Perhaps this case should be covered as well? (if so, then the init block for the CombinedNavModel might need to be updated).

If we don't want to allow nesting CombinedNavModel within CombinedNavModel perhaps the init block of CombinedNavModel could prevent this as well? (though this hypothetically could be a breaking change.

i.e.

CombinedNavModel(
    listOf(
         CombinedNavModel(
             listOf(
                 PermanentNavModel()
             )
         )
    )
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, I think we can prevent nesting of CombinedNavModel as it doesn't make much sense and harder to support

Copy link
Collaborator

@CherryPerry CherryPerry Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nesting makes sense at the current moment.
First you pass CombinedNavModel via ParentNode(navModel = backStack1 + backStack2) then this CombinedNavModel is merged with internal PermanentNavModel into another CombinedNavModel. So at the end we have CombinedNavModel inside CombinedNavModel is a pretty basic case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@KovalevAndrey it sounds like this might be wise to keep? otherwise you should definitely update the changelog to call this out as a breaking change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@LachlanMcKee Looking at the code I'm leaning towards removing the PermanentNavModel from parentNode at all and use it the same way as any other navModels

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nesting makes sense at the current moment. First you pass CombinedNavModel via ParentNode(navModel = backStack1 + backStack2) then this CombinedNavModel is merged with internal PermanentNavModel into another CombinedNavModel. So at the end we have CombinedNavModel inside CombinedNavModel is a pretty basic case.

If we're using the provided API it doesn't get nested

Screenshot 2023-09-30 at 10 48 19

}
is PermanentNavModel<NavTarget> -> navModel
else -> null
}

private fun createPermanentNavModel(buildContext: BuildContext): PermanentNavModel<NavTarget> {
isPermanentNavModelCreated = true
return PermanentNavModel(
savedStateMap = buildContext.savedStateMap,
key = KEY_PERMANENT_NAV_MODEL,
)
}

private val childNodeCreationManager = ChildNodeCreationManager<NavTarget>(
savedStateMap = buildContext.savedStateMap,
Expand Down Expand Up @@ -225,8 +265,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 (isPermanentNavModelCreated) {
// permanentNavModel is not provided as a plugin, store manually
permanentNavModel.saveInstanceState(state)
}
childNodeCreationManager.saveChildrenState(state)
}

Expand Down
98 changes: 88 additions & 10 deletions libraries/core/src/test/kotlin/com/bumble/appyx/core/node/ParentNodeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
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.NavModel
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
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
Expand Down Expand Up @@ -75,24 +80,92 @@ class ParentNodeTest {
assertTrue(attachedNode is NodeB)
}

private fun buildBackStack(initialElement: NavTarget = ChildA) =
BackStack(initialElement = initialElement, savedStateMap = null)
@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(navModel = permanentNavModel)
assertChildrenCount(node, 1)

// when
permanentNavModel.addUnique(ChildB)
assertChildrenCount(node, 2)
val state = node.saveInstanceState { true }
val restoredStateNode =
buildParentNode(navModel = permanentNavModel, savedStateMap = state)

//then
assertChildrenCount(restoredStateNode, 2)
}

@Test
fun `GIVEN node with CombinedNavModel with PermanentNavModel WHEN saves state THEN restores state correctly`() =
testScope.runTest {
//given
val permanentNavModel: PermanentNavModel<NavTarget> =
PermanentNavModel(ChildA, savedStateMap = null)

val backStack = buildBackStack(initialElement = ChildB)
val node = buildParentNode(navModel = permanentNavModel + backStack)
assertChildrenCount(node, 2)

// when
permanentNavModel.addUnique(ChildB)
assertChildrenCount(node, 3)
val state = node.saveInstanceState { true }
val restoredStateNode =
buildParentNode(navModel = permanentNavModel + backStack, savedStateMap = state)

//then
assertChildrenCount(restoredStateNode, 3)
}

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,
savedStateMap: SavedStateMap? = null
) =
BackStack(initialElement = initialElement, savedStateMap = savedStateMap)

private fun buildParentNode(backStack: BackStack<NavTarget>) =
TestParentNode(backStack).apply { onBuilt() }

private fun buildParentNode(
savedStateMap: SavedStateMap? = null,
navModel: NavModel<NavTarget, *>
) =
TestPermanentModelParentNode(
savedStateMap = savedStateMap,
navModel = navModel
).apply { onBuilt() }

private class TestPermanentModelParentNode(
savedStateMap: SavedStateMap? = null,
navModel: NavModel<NavTarget, *>
) : ParentNode<NavTarget>(
buildContext = BuildContext.root(savedStateMap),
navModel = navModel
) {

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()
}
Expand All @@ -115,6 +188,11 @@ class ParentNodeTest {
}
}

private sealed class NavTarget {
object ChildA : NavTarget()
object ChildB : NavTarget()
}

private class NodeB(buildContext: BuildContext) : Node(buildContext) {

var status: String? = null
Expand Down
2 changes: 1 addition & 1 deletion .../sandbox/src/main/kotlin/com/bumble/appyx/sandbox/client/combined/CombinedNavModelNode.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down