Fix missing invalidations while movable content is moving
If a recompose socpe was explicitly invalidated while the
content is owned by the recomposer the invaliation was ignored.
This the second part of a two part fix for 262514238
Fixes: 262514238
Test: ./gradlew :compose:r:r:tDUT
Change-Id: Ic487b8f797a2862effc07d2ef020d8461717940e
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 3c0412e..9968148 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
@@ -320,7 +320,7 @@
internal val composition: ControlledComposition,
internal val slotTable: SlotTable,
internal val anchor: Anchor,
- internal val invalidations: List<Pair<RecomposeScopeImpl, IdentityArraySet<Any>?>>,
+ internal var invalidations: List<Pair<RecomposeScopeImpl, IdentityArraySet<Any>?>>,
internal val locals: PersistentCompositionLocalMap
)
@@ -1865,13 +1865,7 @@
when (val previous = slots.set(groupSlotIndex, value)) {
is RememberObserver ->
rememberManager.forgetting(previous)
- is RecomposeScopeImpl -> {
- val composition = previous.composition
- if (composition != null) {
- previous.release()
- composition.pendingInvalidScopes = true
- }
- }
+ is RecomposeScopeImpl -> previous.release()
}
}
}
@@ -2770,11 +2764,7 @@
}
}
is RecomposeScopeImpl -> {
- val composition = data.composition
- if (composition != null) {
- composition.pendingInvalidScopes = true
- data.release()
- }
+ data.release()
reader.reposition(group)
recordSlotTableOperation { _, slots, _ ->
runtimeCheck(data == slots.slot(group, index)) {
@@ -3109,15 +3099,11 @@
// For all the anchors that moved, if the anchor is tracking a recompose
// scope, update it to reference its new composer.
- if (anchors.isNotEmpty()) {
- val toComposition = to.composition as CompositionImpl
- anchors.fastForEach { anchor ->
- // The recompose scope is always at slot 0 of a restart group.
- val recomposeScope = slots.slot(anchor, 0) as? RecomposeScopeImpl
- // Check for null as the anchor might not be for a recompose scope
- recomposeScope?.adoptedBy(toComposition)
- }
- }
+ RecomposeScopeImpl.adoptAnchoredScopes(
+ slots = slots,
+ anchors = anchors,
+ newOwner = to.composition as RecomposeScopeOwner
+ )
}
fromTable.read { reader ->
@@ -3700,7 +3686,7 @@
// 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 ->
+ val anchors = slotTable.write { writer ->
writer.beginInsert()
// This is the prefix created by invokeMovableContentLambda
@@ -3709,7 +3695,7 @@
writer.update(reference.parameter)
// Move the content into current location
- slots.moveTo(reference.anchor, 1, writer)
+ val anchors = slots.moveTo(reference.anchor, 1, writer)
// skip the group that was just inserted.
writer.skipGroup()
@@ -3718,8 +3704,63 @@
writer.endGroup()
writer.endInsert()
+
+ anchors
}
+
val state = MovableContentState(slotTable)
+ if (RecomposeScopeImpl.hasAnchoredRecomposeScopes(slotTable, anchors)) {
+ // Copy this into a local so the `movableContentRecomposeScopeOwner` captures
+ // only the value of `composition` not the composer not the composer's reference
+ // to the composition, which might change if we move to a composer per thread model
+ // instead of the current composer per composition, and the owner of the recompose
+ // scope is the composition, not the composer.
+ val composition = composition
+
+ // If any recompose scopes are invalidated while the movable content is outside
+ // a composition, ensure the reference is updated to contain the invalidation.
+ val movableContentRecomposeScopeOwner = object : RecomposeScopeOwner {
+ override fun invalidate(
+ scope: RecomposeScopeImpl,
+ instance: Any?
+ ): InvalidationResult {
+ // Try sending this to the original owner first.
+ val result = (composition as? RecomposeScopeOwner)?.invalidate(scope, instance)
+ ?: InvalidationResult.IGNORED
+
+ // If the original owner ignores this then we need to record it in the
+ // reference
+ if (result == InvalidationResult.IGNORED) {
+ reference.invalidations += scope to instance?.let {
+ IdentityArraySet<Any>().also { it.add(it) }
+ }
+ return InvalidationResult.SCHEDULED
+ }
+ return result
+ }
+
+ // The only reason [recomposeScopeReleased] is called is when the recompose scope is
+ // removed from the table. First, this never happens for content that is moving, and
+ // 2) even if it did the only reason we tell the composer is to clear tracking
+ // tables that contain this information which is not relevant here.
+ override fun recomposeScopeReleased(scope: RecomposeScopeImpl) {
+ // Nothing to do
+ }
+
+ // [recordReadOf] this is also something that would happen only during active
+ // recomposition which doesn't happened to a slot table that is moving.
+ override fun recordReadOf(value: Any) {
+ // Nothing to do
+ }
+ }
+ slotTable.write { writer ->
+ RecomposeScopeImpl.adoptAnchoredScopes(
+ slots = writer,
+ anchors = anchors,
+ newOwner = movableContentRecomposeScopeOwner
+ )
+ }
+ }
parentContext.movableContentStateReleased(reference, state)
}
@@ -4203,11 +4244,7 @@
rememberManager.forgetting(slot)
}
if (slot is RecomposeScopeImpl) {
- val composition = slot.composition
- if (composition != null) {
- composition.pendingInvalidScopes = true
- slot.release()
- }
+ slot.release()
}
}
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 83dc817e..bc7d310 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
@@ -348,7 +348,7 @@
private val applier: Applier<*>,
recomposeContext: CoroutineContext? = null
-) : ControlledComposition {
+) : ControlledComposition, RecomposeScopeOwner {
/**
* `null` if a composition isn't pending to apply.
* `Set<Any>` or `Array<Set<Any>>` if there are modifications to record
@@ -928,7 +928,7 @@
} else block()
}
- fun invalidate(scope: RecomposeScopeImpl, instance: Any?): InvalidationResult {
+ override fun invalidate(scope: RecomposeScopeImpl, instance: Any?): InvalidationResult {
if (scope.defaultsInScope) {
scope.defaultsInvalid = true
}
@@ -948,6 +948,10 @@
return invalidateChecked(scope, anchor, instance)
}
+ override fun recomposeScopeReleased(scope: RecomposeScopeImpl) {
+ pendingInvalidScopes = true
+ }
+
private fun tryImminentInvalidation(scope: RecomposeScopeImpl, instance: Any?): Boolean =
isComposing && composer.tryImminentInvalidation(scope, instance)
diff --git a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/RecomposeScopeImpl.kt b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/RecomposeScopeImpl.kt
index b5b8aac..8f72d6ad8 100644
--- a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/RecomposeScopeImpl.kt
+++ b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/RecomposeScopeImpl.kt
@@ -19,6 +19,8 @@
import androidx.compose.runtime.collection.IdentityArrayIntMap
import androidx.compose.runtime.collection.IdentityArrayMap
import androidx.compose.runtime.collection.IdentityArraySet
+import androidx.compose.runtime.snapshots.fastAny
+import androidx.compose.runtime.snapshots.fastForEach
/**
* Represents a recomposable scope or section of the composition hierarchy. Can be used to
@@ -57,6 +59,12 @@
private const val SkippedFlag = 0x10
private const val RereadingFlag = 0x20
+internal interface RecomposeScopeOwner {
+ fun invalidate(scope: RecomposeScopeImpl, instance: Any?): InvalidationResult
+ fun recomposeScopeReleased(scope: RecomposeScopeImpl)
+ fun recordReadOf(value: Any)
+}
+
/**
* A RecomposeScope is created for a region of the composition that can be recomposed independently
* of the rest of the composition. The composer will position the slot table to the location
@@ -64,13 +72,12 @@
* [Composer.startRestartGroup] and is used to track how to restart the group.
*/
internal class RecomposeScopeImpl(
- composition: CompositionImpl?
+ owner: RecomposeScopeOwner?
) : ScopeUpdateScope, RecomposeScope {
private var flags: Int = 0
- var composition: CompositionImpl? = composition
- private set
+ private var owner: RecomposeScopeOwner? = owner
/**
* An anchor to the location in the slot table that start the group associated with this
@@ -83,7 +90,7 @@
* removed from the slot table. For example, if the scope is in the then clause of an if
* statement that later becomes false.
*/
- val valid: Boolean get() = composition != null && anchor?.valid ?: false
+ val valid: Boolean get() = owner != null && anchor?.valid ?: false
val canRecompose: Boolean get() = block != null
@@ -163,18 +170,19 @@
}
/**
- * Invalidate the group which will cause [composition] to request this scope be recomposed,
+ * Invalidate the group which will cause [owner] to request this scope be recomposed,
* and an [InvalidationResult] will be returned.
*/
fun invalidateForResult(value: Any?): InvalidationResult =
- composition?.invalidate(this, value) ?: InvalidationResult.IGNORED
+ owner?.invalidate(this, value) ?: InvalidationResult.IGNORED
/**
* Release the recompose scope. This is called when the recompose scope has been removed by the
* compostion because the part of the composition it was tracking was removed.
*/
fun release() {
- composition = null
+ owner?.recomposeScopeReleased(this)
+ owner = null
trackedInstances = null
trackedDependencies = null
}
@@ -183,18 +191,18 @@
* Called when the data tracked by this recompose scope moves to a different composition when
* for example, the movable content it is part of has moved.
*/
- fun adoptedBy(composition: CompositionImpl) {
- this.composition = composition
+ fun adoptedBy(owner: RecomposeScopeOwner) {
+ this.owner = owner
}
/**
- * Invalidate the group which will cause [composition] to request this scope be recomposed.
+ * Invalidate the group which will cause [owner] to request this scope be recomposed.
*
* Unlike [invalidateForResult], this method is thread safe and calls the thread safe
* invalidate on the composer.
*/
override fun invalidate() {
- composition?.invalidate(this, null)
+ owner?.invalidate(this, null)
}
/**
@@ -291,12 +299,12 @@
}
fun rereadTrackedInstances() {
- composition?.let { composition ->
+ owner?.let { owner ->
trackedInstances?.let { trackedInstances ->
rereading = true
try {
trackedInstances.forEach { value, _ ->
- composition.recordReadOf(value)
+ owner.recordReadOf(value)
}
} finally {
rereading = false
@@ -345,4 +353,26 @@
} else null
}
}
+
+ companion object {
+ internal fun adoptAnchoredScopes(
+ slots: SlotWriter,
+ anchors: List<Anchor>,
+ newOwner: RecomposeScopeOwner
+ ) {
+ if (anchors.isNotEmpty()) {
+ anchors.fastForEach { anchor ->
+ // The recompose scope is always at slot 0 of a restart group.
+ val recomposeScope = slots.slot(anchor, 0) as? RecomposeScopeImpl
+ // Check for null as the anchor might not be for a recompose scope
+ recomposeScope?.adoptedBy(newOwner)
+ }
+ }
+ }
+
+ internal fun hasAnchoredRecomposeScopes(slots: SlotTable, anchors: List<Anchor>) =
+ anchors.isNotEmpty() && anchors.fastAny {
+ slots.ownsAnchor(it) && slots.slot(slots.anchorIndex(it), 0) is RecomposeScopeImpl
+ }
+ }
}
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 1a46dc9..624d885e 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
@@ -599,6 +599,13 @@
return slots.toList().subList(start, end)
}
+ internal fun slot(group: Int, slotIndex: Int): Any? {
+ val start = groups.slotAnchor(group)
+ val end = if (group + 1 < groupsSize) groups.dataAnchor(group + 1) else slots.size
+ val len = end - start
+ return if (slotIndex in 0 until len) return slots[start + slotIndex] else Composer.Empty
+ }
+
override val compositionGroups: Iterable<CompositionGroup> get() = this
override fun iterator(): Iterator<CompositionGroup> =
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 d5ad0d4..d3d1a87 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
@@ -32,10 +32,8 @@
import kotlin.test.assertNotEquals
import kotlin.test.assertSame
import kotlin.test.assertTrue
-import kotlinx.coroutines.ExperimentalCoroutinesApi
@Stable
-@OptIn(ExperimentalCoroutinesApi::class)
class MovableContentTests {
@Test
@@ -779,7 +777,6 @@
}
@Test
- @OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)
fun validateRecomposeScopesDoNotGetLost() = compositionTest {
var isHorizontal by mutableStateOf(false)
val displayValue = mutableStateOf(0)
@@ -809,7 +806,7 @@
isHorizontal = true
Snapshot.sendApplyNotifications()
- testCoroutineScheduler.advanceTimeBy(10)
+ advanceTimeBy(10)
displayValue.value++
expectChanges()
@@ -1493,7 +1490,7 @@
}
}
- testCoroutineScheduler.advanceTimeBy(5_000)
+ advanceTimeBy(5_000)
assertEquals(state, lastSeen)
@@ -1505,6 +1502,45 @@
assertEquals(state, lastSeen, "Failed in iteration $it")
}
}
+
+ @Test
+ fun stateChangesWhilePendingMove() = compositionTest {
+ var state = 0
+ var lastSeen: Int? = null
+ var deferred by mutableStateOf(false)
+ var scope: RecomposeScope? = null
+
+ val content = movableContentOf {
+ Container {
+ lastSeen = state
+ scope = currentRecomposeScope
+ }
+ }
+
+ compose {
+ if (deferred) {
+ DeferredSubcompose {
+ content()
+ }
+ SideEffect {
+ state++
+ scope?.invalidate()
+ }
+ } else {
+ content()
+ }
+ }
+
+ advanceTimeBy(5_000)
+
+ assertEquals(state, lastSeen)
+
+ deferred = true
+
+ advance()
+
+ assertEquals(state, lastSeen)
+ }
}
@Composable
diff --git a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/CompositionTest.kt b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/CompositionTest.kt
index 0cd8df6..15e98c8 100644
--- a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/CompositionTest.kt
+++ b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/CompositionTest.kt
@@ -62,7 +62,7 @@
val changeCount = recomposer.changeCount
Snapshot.sendApplyNotifications()
if (recomposer.hasPendingWork) {
- testScheduler.advanceTimeBy(5_000)
+ advanceTimeBy(5_000)
check(ignorePendingWork || !recomposer.hasPendingWork) {
"Potentially infinite recomposition, still recomposing after advancing"
}
@@ -70,6 +70,8 @@
return recomposer.changeCount - changeCount
}
+ override fun advanceTimeBy(amount: Long) = testScheduler.advanceTimeBy(amount)
+
override fun advance(ignorePendingWork: Boolean) = advanceCount(ignorePendingWork) != 0L
override fun verifyConsistent() {
@@ -113,6 +115,11 @@
fun advanceCount(ignorePendingWork: Boolean = false): Long
/**
+ * Advance the clock by [amount] ms
+ */
+ fun advanceTimeBy(amount: Long)
+
+ /**
* Verify the composition is well-formed.
*/
fun verifyConsistent()
