Allow movable content to move into and out of SubcomposeLayout
Modified how movable content is tracked to allow movable content
to move into newly created SubcomposeLayout and out of
SubcomposeLayout that is being disposed. This is partically useful
when movable content is moving in an out of BoxWithConstraings that
is created and disposed as the content is moving.
The lifetime of when movable content is available to be moved was
extended from just after all composition completes to the end of
all scheduled work for the frame (just before the Recompose waits
for new work).
Fixes: 235398298
Test: ./gradlew :compose:r:r:tDUT
Change-Id: I1558232ce2133c5e55ca5065fc0e76f31d61b37c
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 4e06f54..44515859 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
@@ -1237,6 +1237,7 @@
private var writer: SlotWriter = insertTable.openWriter().also { it.close() }
private var writerHasAProvider = false
private var providerCache: CompositionLocalMap? = null
+ internal var deferredChanges: MutableList<Change>? = null
private var insertAnchor: Anchor = insertTable.read { it.anchor(0) }
private val insertFixups = mutableListOf<Change>()
@@ -1416,8 +1417,12 @@
entersStack.clear()
providersInvalidStack.clear()
providerUpdates.clear()
- if (!reader.closed) { reader.close() }
- if (!writer.closed) { writer.close() }
+ if (!reader.closed) {
+ reader.close()
+ }
+ if (!writer.closed) {
+ writer.close()
+ }
createFreshInsertTable()
compoundKeyHash = 0
childrenComposing = 0
@@ -1445,12 +1450,13 @@
* True if the composition should be checking if the composable functions can be skipped.
*/
@ComposeCompilerApi
- override val skipping: Boolean get() {
- return !inserting && !reusing &&
- !providersInvalid &&
- currentRecomposeScope?.requiresRecompose == false &&
- !forciblyRecompose
- }
+ override val skipping: Boolean
+ get() {
+ return !inserting && !reusing &&
+ !providersInvalid &&
+ currentRecomposeScope?.requiresRecompose == false &&
+ !forciblyRecompose
+ }
/**
* Returns the hash of the compound key calculated as a combination of the keys of all the
@@ -1485,7 +1491,7 @@
return if (!forceRecomposeScopes) {
forceRecomposeScopes = true
forciblyRecompose = true
- true
+ true
} else {
false
}
@@ -1947,21 +1953,23 @@
*/
override fun buildContext(): CompositionContext {
startGroup(referenceKey, reference)
+ if (inserting)
+ writer.markGroup()
- var ref = nextSlot() as? CompositionContextHolder
- if (ref == null) {
- ref = CompositionContextHolder(
+ var holder = nextSlot() as? CompositionContextHolder
+ if (holder == null) {
+ holder = CompositionContextHolder(
CompositionContextImpl(
compoundKeyHash,
forceRecomposeScopes
)
)
- updateValue(ref)
+ updateValue(holder)
}
- ref.ref.updateCompositionLocalScope(currentCompositionLocalScope())
+ holder.ref.updateCompositionLocalScope(currentCompositionLocalScope())
endGroup()
- return ref.ref
+ return holder.ref
}
private fun <T> resolveCompositionLocal(
@@ -2477,7 +2485,7 @@
// An early out if the group and anchor are the same
if (anchorGroup == group) return index
- // Walk down from the anchor group counting nodes of siblings in front of this group
+ // Walk down from the anc ghor group counting nodes of siblings in front of this group
var current = anchorGroup
val nodeIndexLimit = index + (updatedNodeCount(anchorGroup) - reader.nodeCount(group))
loop@ while (index < nodeIndexLimit) {
@@ -2575,8 +2583,9 @@
compoundKeyOf(
reader.parent(group),
recomposeGroup,
- recomposeKey) rol 3
- ) xor groupKey
+ recomposeKey
+ ) rol 3
+ ) xor groupKey
}
}
@@ -3049,10 +3058,10 @@
invalidations = from.invalidations
) {
invokeMovableContentLambda(
- to.content,
- to.locals,
- to.parameter,
- force = true
+ to.content,
+ to.locals,
+ to.parameter,
+ force = true
)
}
}
@@ -3173,6 +3182,7 @@
isComposing = false
}
}
+
/**
* Synchronously recompose all invalidated groups. This collects the changes which must be
* applied by [ControlledComposition.applyChanges] to have an effect.
@@ -3480,74 +3490,70 @@
*
* Returns the number of nodes left in place which is used to calculate the node index of
* any nested calls.
+ *
+ * @param groupBeingRemoved The group that is being removed from the table or 0 if the entire
+ * table is being removed.
*/
private fun reportFreeMovableContent(groupBeingRemoved: Int) {
fun reportGroup(group: Int, needsNodeDelete: Boolean, nodeIndex: Int): Int {
- // If the group has a mark (e.g. it is a movable content group), schedule it to be
- // removed and report that it is free to be moved to the parentContext. Nested
- // movable content is recomposed if necessary once the group has been claimed by
- // 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") // 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)
- val end = group + reader.groupSize(group)
- val invalidations = this.invalidations.filterToRange(group, end).fastMap {
- it.scope to it.instances
- }
- val reference = MovableContentStateReference(
- value,
- parameter,
- composition,
- slotTable,
- anchor,
- invalidations,
- currentCompositionLocalScope(group)
- )
- parentContext.deletedMovableContent(reference)
- recordSlotEditing()
- 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()
+ // If the group has a mark then it is either a movable content group or a
+ // composition context group
+ val key = reader.groupKey(group)
+ val objectKey = reader.groupObjectKey(group)
+ if (key == movableContentKey && objectKey is MovableContent<*>) {
+ // If the group is a movable content block schedule it to be removed and report
+ // that it is free to be moved to the parentContext. Nested movable content is
+ // recomposed if necessary once the group has been claimed by 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.
+ @Suppress("UNCHECKED_CAST")
+ val movableContent = objectKey as MovableContent<Any?>
+ val parameter = reader.groupGet(group, 0)
+ val anchor = reader.anchor(group)
+ val end = group + reader.groupSize(group)
+ val invalidations = this.invalidations.filterToRange(group, end).fastMap {
+ it.scope to it.instances
}
- val state = MovableContentState(slotTable)
- parentContext.movableContentStateReleased(reference, state)
- }
- if (needsNodeDelete) {
- realizeMovement()
- realizeUps()
- realizeDowns()
- val nodeCount = if (reader.isNode(group)) 1 else reader.nodeCount(group)
- if (nodeCount > 0) {
- recordRemoveNode(nodeIndex, nodeCount)
+ val reference = MovableContentStateReference(
+ movableContent,
+ parameter,
+ composition,
+ slotTable,
+ anchor,
+ invalidations,
+ currentCompositionLocalScope(group)
+ )
+ parentContext.deletedMovableContent(reference)
+ recordSlotEditing()
+ record { _, slots, _ -> releaseMovableGroupAtCurrent(reference, slots) }
+ if (needsNodeDelete) {
+ realizeMovement()
+ realizeUps()
+ realizeDowns()
+ val nodeCount = if (reader.isNode(group)) 1 else reader.nodeCount(group)
+ if (nodeCount > 0) {
+ recordRemoveNode(nodeIndex, nodeCount)
+ }
+ 0 // These nodes were deleted
+ } else reader.nodeCount(group)
+ } else if (key == referenceKey && objectKey == reference) {
+ // Group is a composition context reference. As this is being removed assume
+ // all movable groups in the composition that have this context will also be
+ // released whe the compositions are disposed.
+ val contextHolder = reader.groupGet(group, 0) as? CompositionContextHolder
+ if (contextHolder != null) {
+ // The contextHolder can be EMPTY in cases wher the content has been
+ // deactivated. Content is deactivated if the content is just being
+ // held onto for recycling and is not otherwise active. In this case
+ // the composers we are likely to find here have already been disposed.
+ val compositionContext = contextHolder.ref
+ compositionContext.composers.forEach { composer ->
+ composer.reportAllMovableContent()
+ }
}
- 0 // These nodes were deleted
+ reader.nodeCount(group)
} else reader.nodeCount(group)
} else if (reader.containsMark(group)) {
// Traverse the group freeing the child movable content. This group is known to
@@ -3589,6 +3595,66 @@
}
/**
+ * Release the reference the movable group stored in [slots] to the recomposer for to be used
+ * to insert to insert to other locations.
+ */
+ private fun releaseMovableGroupAtCurrent(
+ reference: MovableContentStateReference,
+ slots: SlotWriter
+ ) {
+ 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, reference.content)
+ writer.markGroup()
+ writer.update(reference.parameter)
+
+ // Move the content into current location
+ slots.moveTo(reference.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)
+ parentContext.movableContentStateReleased(reference, state)
+ }
+
+ /**
+ * Called during composition to report all the content of the composition will be released
+ * as this composition is to be disposed.
+ */
+ private fun reportAllMovableContent() {
+ if (slotTable.containsMark()) {
+ val changes = mutableListOf<Change>()
+ deferredChanges = changes
+ slotTable.read { reader ->
+ this.reader = reader
+ withChanges(changes) {
+ reportFreeMovableContent(0)
+ realizeUps()
+ if (startedGroup) {
+ record(skipToGroupEndInstance)
+ recordEndRoot()
+ }
+ }
+ }
+ }
+ }
+
+ /**
* Called when reader current is moved directly, such as when a group moves, to [location].
*/
private fun recordReaderMoving(location: Int) {
@@ -3605,15 +3671,17 @@
val reader = reader
val location = reader.parent
- if (startedGroups.peekOr(-1) != location) {
+ if (startedGroups.peekOr(invalidGroupLocation) != location) {
if (!startedGroup && implicitRootStart) {
// We need to ensure the root group is started.
recordSlotTableOperation(change = startRootGroup)
startedGroup = true
}
- val anchor = reader.anchor(location)
- startedGroups.push(location)
- recordSlotTableOperation { _, slots, _ -> slots.ensureStarted(anchor) }
+ if (location > 0) {
+ val anchor = reader.anchor(location)
+ startedGroups.push(location)
+ recordSlotTableOperation { _, slots, _ -> slots.ensureStarted(anchor) }
+ }
}
}
}
@@ -4298,6 +4366,8 @@
@PublishedApi
internal const val reuseKey = 207
+private const val invalidGroupLocation = -2
+
internal class ComposeRuntimeError(override val message: String) : IllegalStateException()
internal inline fun runtimeCheck(value: Boolean, lazyMessage: () -> Any) {
diff --git a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composition.kt b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composition.kt
index a32afee..0f27069 100644
--- a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composition.kt
+++ b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composition.kt
@@ -595,6 +595,24 @@
if (!disposed) {
disposed = true
composable = {}
+
+ // Changes are deferred if the composition contains movable content that needs
+ // to be released. NOTE: Applying these changes leaves the slot table in
+ // potentially invalid state. The routine use to produce this change list reuses
+ // code that extracts movable content from groups that are being deleted. This code
+ // does not bother to correctly maintain the node counts of a group nested groups
+ // that are going to be removed anyway so the node counts of the groups affected
+ // are might be incorrect after the changes have been applied.
+ val deferredChanges = composer.deferredChanges
+ if (deferredChanges != null) {
+ applyChangesInLocked(deferredChanges)
+ }
+
+ // Dispatch all the `onForgotten` events for object that are no longer part of a
+ // composition because this composition is being discarded. It is important that
+ // this is done after applying deferred changes above to avoid sending `
+ // onForgotten` notification to objects that are still part of movable content that
+ // will be moved to a new location.
val nonEmptySlotTable = slotTable.groupsSize > 0
if (nonEmptySlotTable || abandonSet.isNotEmpty()) {
val manager = RememberEventDispatcher(abandonSet)
@@ -791,7 +809,6 @@
}
changes.clear()
}
-
applier.onEndChanges()
}
diff --git a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Recomposer.kt b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Recomposer.kt
index c4b90f1..1189902 100644
--- a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Recomposer.kt
+++ b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Recomposer.kt
@@ -621,13 +621,13 @@
}
}
- discardUnusedValues()
-
synchronized(stateLock) {
deriveStateLocked()
}
}
}
+
+ discardUnusedValues()
}
}
diff --git a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/SlotTable.kt b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/SlotTable.kt
index c5a0450..41904d0 100644
--- a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/SlotTable.kt
+++ b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/SlotTable.kt
@@ -351,6 +351,13 @@
}
/**
+ * Turns true if the first group (considered the root group) contains a mark.
+ */
+ fun containsMark(): Boolean {
+ return groupsSize >= 0 && groups.containsMark(0)
+ }
+
+ /**
* Find the nearest recompose scope for [group] that, when invalidated, will cause [group]
* group to be recomposed.
*/
@@ -469,7 +476,7 @@
var lastLocation = -1
anchors.fastForEach { anchor ->
val location = anchor.toIndexFor(this)
- require(location in 0..groupsSize) { "Location out of bound" }
+ require(location in 0..groupsSize) { "Invalid anchor, location out of bound" }
require(lastLocation < location) { "Anchor is out of order" }
lastLocation = location
}
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 7ded476..24a3119 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,8 +1027,8 @@
expectUnused()
}
- @Test // Regression test for 230830644
- fun deferredSubcompose_conditional() = compositionTest {
+ @Test // Regression test for 230830644 and 235398298
+ fun deferredSubcompose_conditional_rootLevelChildren() = compositionTest {
var subcompose by mutableStateOf(false)
var lastPrivateState: State<Int> = mutableStateOf(0)
@@ -1072,7 +1072,71 @@
expectChanges()
revalidate()
- assertEquals(expectedState, lastPrivateState)
+ assertEquals(expectedState, lastPrivateState, "Movable content was unexpectedly recreated")
+
+ subcompose = false
+ expectChanges()
+ revalidate()
+
+ assertEquals(expectedState, lastPrivateState, "Movable content was unexpectedly recreated")
+ }
+
+ @Test // Regression test for 230830644 and 235398298
+ fun deferredSubcompose_conditional_nestedChildren() = 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 {
+ Column {
+ 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 {
+ Column {
+ Text("Sub-composed content start")
+ Text("Movable content")
+ Text("Sub-composed content end")
+ }
+ }
+ }
+ }
+
+ val expectedState = lastPrivateState
+ subcompose = true
+ expectChanges()
+ revalidate()
+
+ assertEquals(expectedState, lastPrivateState, "Movable content was unexpectedly recreated")
+
+ subcompose = false
+ expectChanges()
+ revalidate()
+
+ assertEquals(expectedState, lastPrivateState, "Movable content was unexpectedly recreated")
}
@Test // Regression test for 230830644
@@ -1525,10 +1589,11 @@
ComposeNode<View, ViewApplier>(factory = { host }, update = { })
val parent = rememberCompositionContext()
val composition = remember { Composition(ViewApplier(host), parent) }
- SideEffect {
+ LaunchedEffect(content as Any) {
composition.setContent(content)
}
DisposableEffect(Unit) {
+
onDispose { composition.dispose() }
}
}
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 4b63669..32cd5cf 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
@@ -44,9 +44,11 @@
}
fun addAt(index: Int, view: View) {
- if (view.parent != null) {
+ val parent = view.parent
+ if (parent != null) {
error(
- "Inserting a view named ${view.name} already has a parent into a view named $name"
+ "Inserting a view named ${view.name} into a view named $name which already has " +
+ "a parent named ${parent.name}"
)
}
view.parent = this
@@ -79,6 +81,11 @@
}
}
+ fun removeAllChildren() {
+ children.fastForEach { child -> child.parent = null }
+ children.clear()
+ }
+
fun attribute(name: String, value: Any) { attributes[name] = value }
var value: String?
diff --git a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/ViewApplier.kt b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/ViewApplier.kt
index 28ebae3..4b1b684 100644
--- a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/ViewApplier.kt
+++ b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/ViewApplier.kt
@@ -43,7 +43,7 @@
}
override fun onClear() {
- root.children.clear()
+ root.removeAllChildren()
}
override fun onBeginChanges() {
diff --git a/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/LayoutNode.kt b/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/LayoutNode.kt
index 5a4a960..e8c82f9 100644
--- a/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/LayoutNode.kt
+++ b/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/LayoutNode.kt
@@ -410,6 +410,7 @@
if (parent != null) {
parent.invalidateLayer()
parent.invalidateMeasurements()
+ measuredByParent = UsageByParent.NotUsed
}
layoutDelegate.resetAlignmentLines()
onDetach?.invoke(owner)
