Fix moving content to new content of a subcomposition
When creating new content of a subcomposition the content
might be created after the movable content it references
has been removed from it current composition.
Previously it was assumed that all inserts of movable content
will occur before the content is removed from its slot table.
Fixes: 230830644
Test: ./gradlew :compose:r:r:tDUT
Change-Id: I0bf4f06e0aaeebe7ede53d4dcd50d6d8e03bfcff
diff --git a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composer.kt b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composer.kt
index a5719c8..d7d4ddd 100644
--- a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composer.kt
+++ b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composer.kt
@@ -2953,7 +2953,14 @@
}
}
} else {
- val nodesToInsert = from.slotTable.collectNodesFrom(from.anchor)
+ // If the state was already removed from the from table then it will have a
+ // state recorded in the recomposer, retrieve that now if we can. If not the
+ // state is still in its original location, recompose over it there.
+ val resolvedState = parentContext.movableContentStateResolve(from)
+ val fromTable = resolvedState?.slotTable ?: from.slotTable
+ val fromAnchor = resolvedState?.slotTable?.anchor(0) ?: from.anchor
+ val nodesToInsert = fromTable.collectNodesFrom(fromAnchor)
+
// Insert nodes if necessary
if (nodesToInsert.isNotEmpty()) {
record { applier, _, _ ->
@@ -2965,24 +2972,30 @@
applier.insertTopDown(base + i, node)
}
}
- val group = slotTable.anchorIndex(anchor)
- updateNodeCount(
- group,
- updatedNodeCount(group) + nodesToInsert.size
- )
+ if (to.slotTable == slotTable) {
+ // Inserting the content into the current slot table then we need to
+ // update the virtual node counts. Otherwise, we are inserting into
+ // a new slot table which is being created, not updated, so the virtual
+ // node counts do not need to be updated.
+ val group = slotTable.anchorIndex(anchor)
+ updateNodeCount(
+ group,
+ updatedNodeCount(group) + nodesToInsert.size
+ )
+ }
}
// Copy the slot table into the anchor location
record { _, slots, _ ->
- val state = parentContext.movableContentStateResolve(from)
+ val state = resolvedState ?: parentContext.movableContentStateResolve(from)
?: composeRuntimeError("Could not resolve state for movable content")
// The slot table contains the movable content group plus the group
// containing the movable content's table which then contains the actual
// state to be inserted. The state is at index 2 in the table (for the
- // to groups) and is inserted into the provider group at offset 1 from the
+ // two groups) and is inserted into the provider group at offset 1 from the
// current location.
- val anchors = slots.moveIntoGroupFrom(1, state.slotTable, 1)
+ val anchors = slots.moveIntoGroupFrom(1, state.slotTable, 2)
// For all the anchors that moved, if the anchor is tracking a recompose
// scope, update it to reference its new composer.
@@ -2997,12 +3010,9 @@
}
}
- // Recompose over the moved content.
- val fromTable = from.slotTable
-
fromTable.read { reader ->
withReader(reader) {
- val newLocation = fromTable.anchorIndex(from.anchor)
+ val newLocation = fromTable.anchorIndex(fromAnchor)
reader.reposition(newLocation)
writersReaderDelta = newLocation
val offsetChanges = mutableListOf<Change>()
@@ -3457,7 +3467,7 @@
// another insert. If the nested movable content ends up being removed this is reported
// during that recomposition so there is no need to look at child movable content here.
return if (reader.hasMark(group)) {
- @Suppress("UNCHECKED_CAST")
+ @Suppress("UNCHECKED_CAST") // The mark is only used when this cast is valid.
val value = reader.groupObjectKey(group) as MovableContent<Any?>
val parameter = reader.groupGet(group, 0)
val anchor = reader.anchor(group)
@@ -3479,9 +3489,28 @@
record { _, slots, _ ->
val slotTable = SlotTable()
+ // Write a table that as if it was written by a calling
+ // invokeMovableContentLambda because this might be removed from the
+ // composition before the new composition can be composed to receive it. When
+ // the new composition receives the state it must recompose over the state by
+ // calling invokeMovableContentLambda.
slotTable.write { writer ->
writer.beginInsert()
+
+ // This is the prefix created by invokeMovableContentLambda
+ writer.startGroup(movableContentKey, value)
+ writer.markGroup()
+ writer.update(parameter)
+
+ // Move the content into current location
slots.moveTo(anchor, 1, writer)
+
+ // skip the group that was just inserted.
+ writer.skipGroup()
+
+ // End the group that represents the call to invokeMovableContentLambda
+ writer.endGroup()
+
writer.endInsert()
}
val state = MovableContentState(slotTable)
diff --git a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/MovableContentTests.kt b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/MovableContentTests.kt
index accff5a..7ded476 100644
--- a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/MovableContentTests.kt
+++ b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/MovableContentTests.kt
@@ -1027,6 +1027,104 @@
expectUnused()
}
+ @Test // Regression test for 230830644
+ fun deferredSubcompose_conditional() = compositionTest {
+ var subcompose by mutableStateOf(false)
+ var lastPrivateState: State<Int> = mutableStateOf(0)
+
+ val content = movableContentOf {
+ lastPrivateState = remember { mutableStateOf(0) }
+ Text("Movable content")
+ }
+
+ compose {
+ Text("Main content start")
+ if (!subcompose) {
+ content()
+ }
+ Text("Main content end")
+ if (subcompose) {
+ DeferredSubcompose {
+ Text("Sub-composed content start")
+ content()
+ Text("Sub-composed content end")
+ }
+ }
+ }
+
+ validate {
+ Text("Main content start")
+ if (!subcompose) {
+ Text("Movable content")
+ }
+ Text("Main content end")
+ if (subcompose) {
+ DeferredSubcompose {
+ Text("Sub-composed content start")
+ Text("Movable content")
+ Text("Sub-composed content end")
+ }
+ }
+ }
+
+ val expectedState = lastPrivateState
+ subcompose = true
+ expectChanges()
+ revalidate()
+
+ assertEquals(expectedState, lastPrivateState)
+ }
+
+ @Test // Regression test for 230830644
+ fun deferredSubcompose_conditional_and_invalid() = compositionTest {
+ var subcompose by mutableStateOf(false)
+ var lastPrivateState: State<Int> = mutableStateOf(0)
+ var state by mutableStateOf("one")
+
+ val content = movableContentOf {
+ lastPrivateState = remember { mutableStateOf(0) }
+ Text("Movable content state: $state")
+ }
+
+ compose {
+ Text("Main content start")
+ if (!subcompose) {
+ content()
+ }
+ Text("Main content end")
+ if (subcompose) {
+ DeferredSubcompose {
+ Text("Sub-composed content start")
+ content()
+ Text("Sub-composed content end")
+ }
+ }
+ }
+
+ validate {
+ Text("Main content start")
+ if (!subcompose) {
+ Text("Movable content state: $state")
+ }
+ Text("Main content end")
+ if (subcompose) {
+ DeferredSubcompose {
+ Text("Sub-composed content start")
+ Text("Movable content state: $state")
+ Text("Sub-composed content end")
+ }
+ }
+ }
+
+ val expectedState = lastPrivateState
+ subcompose = true
+ state = "two"
+ expectChanges()
+ revalidate()
+
+ assertEquals(expectedState, lastPrivateState)
+ }
+
@Test
fun movableContentParameters_One() = compositionTest {
val data = mutableStateOf(0)
@@ -1421,10 +1519,28 @@
}
}
+@Composable
+private fun DeferredSubcompose(content: @Composable () -> Unit) {
+ val host = View().also { it.name = "DeferredSubcompose" }
+ ComposeNode<View, ViewApplier>(factory = { host }, update = { })
+ val parent = rememberCompositionContext()
+ val composition = remember { Composition(ViewApplier(host), parent) }
+ SideEffect {
+ composition.setContent(content)
+ }
+ DisposableEffect(Unit) {
+ onDispose { composition.dispose() }
+ }
+}
+
private fun MockViewValidator.Subcompose(content: MockViewValidator.() -> Unit) {
view("SubcomposeHost", content)
}
+private fun MockViewValidator.DeferredSubcompose(content: MockViewValidator.() -> Unit) {
+ view("DeferredSubcompose", content)
+}
+
class RememberedObject : RememberObserver {
var count: Int = 0
val isLive: Boolean get() = count > 0
diff --git a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/View.kt b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/View.kt
index 3aabba5..4b63669 100644
--- a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/View.kt
+++ b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/View.kt
@@ -45,7 +45,9 @@
fun addAt(index: Int, view: View) {
if (view.parent != null) {
- error("View named $name already has a parent")
+ error(
+ "Inserting a view named ${view.name} already has a parent into a view named $name"
+ )
}
view.parent = this
children.add(index, view)
