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

Do not create permanentNavModel inside ParentNode. Provide it via constructor to ParentNode #606

Merged
merged 4 commits into from
Oct 3, 2023
Merged

Do not create permanentNavModel inside ParentNode. Provide it via constructor to ParentNode #606

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b82daa5
Do not create permanentNavModel inside ParentNode. Provide it via con…
KovalevAndrey Oct 2, 2023
66ad121
Update CHANGELOG.md
KovalevAndrey Oct 2, 2023
c9de405
Remember permanentNavModel and add fix StatefulNode1
KovalevAndrey Oct 2, 2023
60eb43f
Mention breaking change in the CHANGELOG.md and allow to add keys to …
KovalevAndrey Oct 2, 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

-
- [#606](https://github.com/bumble-tech/appyx/pull/606) – **Fixed**: Do not create permanentNavModel inside ParentNode. Provide it via constructor to ParentNode
KovalevAndrey marked this conversation as resolved.
Show resolved Hide resolved

---

Expand Down
35 changes: 29 additions & 6 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 @@ -11,29 +11,36 @@ import androidx.compose.ui.platform.testTag
import androidx.compose.ui.test.hasTestTag
import com.bumble.appyx.core.AppyxTestScenario
import com.bumble.appyx.core.children.nodeOrNull
import com.bumble.appyx.core.composable.PermanentChild
import com.bumble.appyx.core.modality.BuildContext
import com.bumble.appyx.core.navigation.EmptyNavModel
import com.bumble.appyx.core.navigation.model.permanent.PermanentNavModel
import kotlinx.parcelize.Parcelize
import org.junit.Assert.assertEquals
import org.junit.Rule
import org.junit.Test

class PermanentChildTest {

var nodeFactory: (buildContext: BuildContext) -> TestParentNode = {
TestParentNode(buildContext = it)
}

@get:Rule
val rule = AppyxTestScenario { buildContext ->
TestParentNode(buildContext)
nodeFactory(buildContext)
}

@Test
fun permanent_child_is_rendered() {
fun `WHEN_permanent_model_contains_relevant_nav_key_THEN_permanent_child_is_rendered`() {
createPermanentNavModelWithNavKey()
rule.start()

rule.onNode(hasTestTag(TestParentNode.NavTarget::class.java.name)).assertExists()
}

@Test
fun permanent_child_is_reused_when_visibility_switched() {
LachlanMcKee marked this conversation as resolved.
Show resolved Hide resolved
fun `WHEN_visibility_switched_THEN_permanent_child_is_reused`() {
createPermanentNavModelWithNavKey()
rule.start()
rule.node.renderPermanentChild = false
val childNodes = rule.node.children.value.values.map { it.nodeOrNull }
Expand All @@ -46,11 +53,27 @@ class PermanentChildTest {
assertEquals(childNodes, rule.node.children.value.values.map { it.nodeOrNull })
}

private fun createPermanentNavModelWithNavKey() {
nodeFactory = {
TestParentNode(
buildContext = it,
permanentNavModel = PermanentNavModel(
TestParentNode.NavTarget,
savedStateMap = null,
)
)
}

}

class TestParentNode(
buildContext: BuildContext,
private val permanentNavModel: PermanentNavModel<NavTarget> = PermanentNavModel(
savedStateMap = buildContext.savedStateMap
),
) : ParentNode<TestParentNode.NavTarget>(
buildContext = buildContext,
navModel = EmptyNavModel<NavTarget, Any>(),
navModel = permanentNavModel
) {

@Parcelize
Expand All @@ -69,7 +92,7 @@ class PermanentChildTest {
@Composable
override fun View(modifier: Modifier) {
if (renderPermanentChild) {
PermanentChild(NavTarget)
PermanentChild(permanentNavModel, NavTarget)
}
}
}
Expand Down
56 changes: 56 additions & 0 deletions libraries/core/src/main/kotlin/com/bumble/appyx/core/composable/PermanentChild.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package com.bumble.appyx.core.composable

import androidx.compose.runtime.Composable
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import com.bumble.appyx.core.navigation.model.permanent.PermanentNavModel
import com.bumble.appyx.core.node.Node
import com.bumble.appyx.core.node.ParentNode

@Composable
fun <NavTarget : Any> ParentNode<NavTarget>.PermanentChild(
permanentNavModel: PermanentNavModel<NavTarget>,
navTarget: NavTarget,
decorator: @Composable (child: ChildRenderer) -> Unit
) {
val child = remember(navTarget) {
KovalevAndrey marked this conversation as resolved.
Show resolved Hide resolved
permanentNavModel
.elements
.value
.find { it.key.navTarget == navTarget }
?.let { childOrCreate(it.key) }
?: throw IllegalStateException(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this break the code within StatefulNode1.kt?

Just so I understand, it's now mandatory to pass in a NavTarget to the PermanentNavModel constructor?

Perhaps we should update PermanentNavModel to throw an IllegalStateException if there are no NavTarget's provided? Could we also consider removing the vararg then, as this will definitely lead to issues

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should just ignore if there is no child of the specific type. Otherwise we break functionality where you add a child later. Or maybe provide some optional renderer function.

Just so I understand, it's now mandatory to pass in a NavTarget to the PermanentNavModel constructor?

Why it should be so? We still have add/addUnique operations to add nodes later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel like if you have a logic of adding a child later at some point it shouldn't be a part of PermanentNavModel

Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps leave it as is for now, and consider changing this behaviour in 2.0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@CherryPerry my usecase is that I have a PermanentChild that is added immediately (via the constructor) and is only shown once the data is loaded (unfortunately in my usecase the ParentNode is responsible for when to show the PermanentChild based on Output of whether the state is loaded or not)

Without this change, the PermanentChild would not correctly start the business logic when added via the PermanentChild constructor (well actually it can, but it renders the wrong one at the moment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway, we are breaking existing use case without providing alternative, when you want to add your permanent child later after some flag is triggered. Now you can via if (myCheck) PermanentChild(NavKey.AddMe).

Copy link
Collaborator Author

@KovalevAndrey KovalevAndrey Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I reverted it back so we can add keys later

Copy link
Collaborator Author

@KovalevAndrey KovalevAndrey Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Or we can observe permanentNavModel in composition to render permanent children

val isAddMeExists = permanentNavModel
    .elements.
    .mapState(scope, SharingStarted.WhileSubscribed()) { navElements ->
         navElements
             .find { it.key.navTarget == navTarget } ?: false
     }
     .collectAsState()

if (isAddMeExists) { PermanentChild() }

otherwise it's difficult to track issues where you forgot to add a key

Copy link
Collaborator Author

@KovalevAndrey KovalevAndrey Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps, can be added to an overloaded PermanentChild while default one fails

"No child found for $navTarget in $permanentNavModel. " +
"Add $navTarget to $permanentNavModel before calling PermanentChild."
)
}

decorator(PermanentChildRender(child.node))
}

@Composable
fun <NavTarget : Any> ParentNode<NavTarget>.PermanentChild(
permanentNavModel: PermanentNavModel<NavTarget>,
navTarget: NavTarget,
) {
PermanentChild(permanentNavModel, navTarget) { child -> child() }
}

private class PermanentChildRender(private val node: Node) : ChildRenderer {

@Suppress(
"ComposableNaming" // This wants to be 'Invoke' but that won't work with 'operator'.
)
@Composable
override operator fun invoke(modifier: Modifier) {
node.Compose(modifier)
}

@Suppress(
"ComposableNaming" // This wants to be 'Invoke' but that won't work with 'operator'.
)
@Composable
override operator fun invoke() {
invoke(modifier = Modifier)
}
}
70 changes: 1 addition & 69 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 @@ -2,14 +2,7 @@ package com.bumble.appyx.core.node

import androidx.annotation.CallSuper
import androidx.annotation.VisibleForTesting
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.Stable
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.ui.Modifier
import androidx.lifecycle.DefaultLifecycleObserver
import androidx.lifecycle.Lifecycle
import androidx.lifecycle.LifecycleOwner
Expand All @@ -24,21 +17,15 @@ import com.bumble.appyx.core.children.ChildEntryMap
import com.bumble.appyx.core.children.ChildNodeCreationManager
import com.bumble.appyx.core.children.ChildrenCallback
import com.bumble.appyx.core.children.nodeOrNull
import com.bumble.appyx.core.composable.ChildRenderer
import com.bumble.appyx.core.lifecycle.ChildNodeLifecycleManager
import com.bumble.appyx.core.mapState
import com.bumble.appyx.core.modality.BuildContext
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.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.plugin.Plugin
import com.bumble.appyx.core.state.MutableSavedStateMap
import kotlinx.coroutines.Job
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.launch
import kotlinx.coroutines.suspendCancellableCoroutine
Expand All @@ -50,7 +37,7 @@ import kotlin.reflect.KClass
@Suppress("TooManyFunctions")
@Stable
abstract class ParentNode<NavTarget : Any>(
navModel: NavModel<NavTarget, *>,
val navModel: NavModel<NavTarget, *>,
buildContext: BuildContext,
view: ParentNodeView<NavTarget> = EmptyParentNodeView(),
childKeepMode: ChildEntry.KeepMode = Appyx.defaultChildKeepMode,
Expand All @@ -62,12 +49,6 @@ 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 val childNodeCreationManager = ChildNodeCreationManager<NavTarget>(
savedStateMap = buildContext.savedStateMap,
customisations = buildContext.customisations,
Expand Down Expand Up @@ -96,35 +77,6 @@ abstract class ParentNode<NavTarget : Any>(
fun childOrCreate(navKey: NavKey<NavTarget>): ChildEntry.Initialized<NavTarget> =
childNodeCreationManager.childOrCreate(navKey)

@Composable
fun PermanentChild(
navTarget: NavTarget,
decorator: @Composable (child: ChildRenderer) -> Unit
) {
LaunchedEffect(navTarget) {
permanentNavModel.addUnique(navTarget)
}
val scope = rememberCoroutineScope()
val child by remember(navTarget) {
permanentNavModel
.elements
// use WhileSubscribed or Lazy otherwise desynchronisation issue
.mapState(scope, SharingStarted.WhileSubscribed()) { navElements ->
navElements
.find { it.key.navTarget == navTarget }
?.let { childOrCreate(it.key) }
}
}.collectAsState()
child?.let {
decorator(child = PermanentChildRender(it.node))
}
}

@Composable
fun PermanentChild(navTarget: NavTarget) {
PermanentChild(navTarget) { child -> child() }
}

override fun updateLifecycleState(state: Lifecycle.State) {
super.updateLifecycleState(state)
childNodeLifecycleManager.propagateLifecycleToChildren(state)
Expand Down Expand Up @@ -225,8 +177,6 @@ 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)
childNodeCreationManager.saveChildrenState(state)
}

Expand Down Expand Up @@ -266,26 +216,8 @@ abstract class ParentNode<NavTarget : Any>(

companion object {
const val ATTACH_WORKFLOW_SYNC_TIMEOUT = 5000L
const val KEY_PERMANENT_NAV_MODEL = "PermanentNavModel"
}

private class PermanentChildRender(private val node: Node) : ChildRenderer {

@Suppress(
"ComposableNaming" // This wants to be 'Invoke' but that won't work with 'operator'.
)
@Composable
override operator fun invoke(modifier: Modifier) {
node.Compose(modifier)
}

@Suppress(
"ComposableNaming" // This wants to be 'Invoke' but that won't work with 'operator'.
)
@Composable
override operator fun invoke() {
invoke(modifier = Modifier)
}
}

}
Loading