Call onRelease callback in the same order as onForgotten SubcomposeLayout depends on this order to dispose nested subcompositions when parent exits the composition. In particular, we want to make sure that child can unpin itself (and cleanup e.g. focus) before parent onForgotten is dispatched. Deactivations were already dispatched in this order to account for a similar issue. Fixes: 322579799 Test: Added ordering test in SubcomposeLayoutTest and pinning test in LazyListPinnableContainerTest (cherry picked from https://android-review.googlesource.com/q/commit:01c9d9953b00e5111ff8a2075208ff78601a572e) (cherry picked from https://android-review.googlesource.com/q/commit:13a8f4633327392402d5282a254eabea94f8f526) Merged-In: I0429d56ae6d02406fbc6f062d1c1ca040030ab09 Change-Id: I0429d56ae6d02406fbc6f062d1c1ca040030ab09
diff --git a/compose/foundation/foundation/src/androidInstrumentedTest/kotlin/androidx/compose/foundation/lazy/list/LazyListPinnableContainerTest.kt b/compose/foundation/foundation/src/androidInstrumentedTest/kotlin/androidx/compose/foundation/lazy/list/LazyListPinnableContainerTest.kt index 82963da..30c436c 100644 --- a/compose/foundation/foundation/src/androidInstrumentedTest/kotlin/androidx/compose/foundation/lazy/list/LazyListPinnableContainerTest.kt +++ b/compose/foundation/foundation/src/androidInstrumentedTest/kotlin/androidx/compose/foundation/lazy/list/LazyListPinnableContainerTest.kt
@@ -20,6 +20,7 @@ import androidx.compose.foundation.layout.size import androidx.compose.foundation.lazy.LazyColumn import androidx.compose.foundation.lazy.LazyListState +import androidx.compose.foundation.lazy.LazyRow import androidx.compose.foundation.lazy.items import androidx.compose.runtime.Composable import androidx.compose.runtime.CompositionLocalProvider @@ -666,6 +667,42 @@ assertThat(parent2Pinned).isTrue() } } + + @Test + fun pinnedItemIsRemovedAfterContainerExitsComposition() { + var active by mutableStateOf(true) + // Arrange. + rule.setContentParameterized { + if (active) { + LazyColumn(Modifier.size(itemSize * 2)) { + items(3) { colIndex -> + LazyRow { + items(3) { rowIndex -> + if (colIndex == 1 && rowIndex == 1) { + pinnableContainer = LocalPinnableContainer.current + } + Box(Modifier.size(itemSize).testTag("$colIndex:$rowIndex")) + } + } + } + } + } + } + + rule.onNodeWithTag("1:1") + .assertIsPlaced() + + rule.runOnIdle { + requireNotNull(pinnableContainer).pin() + } + + rule.runOnIdle { + active = !active + } + + rule.onNodeWithTag("1:1") + .assertIsNotPlaced() + } } /**
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 83eaa14..7810951 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
@@ -17,6 +17,8 @@ @file:OptIn(InternalComposeApi::class) package androidx.compose.runtime +import androidx.collection.MutableScatterSet +import androidx.collection.mutableScatterSetOf import androidx.compose.runtime.changelist.ChangeList import androidx.compose.runtime.collection.IdentityArrayMap import androidx.compose.runtime.collection.IdentityArraySet @@ -1236,7 +1238,7 @@ private val remembering = mutableListOf<RememberObserver>() private val forgetting = mutableListOf<Any>() private val sideEffects = mutableListOf<() -> Unit>() - private var releasing: MutableList<ComposeNodeLifecycleCallback>? = null + private var releasing: MutableScatterSet<ComposeNodeLifecycleCallback>? = null override fun remembering(instance: RememberObserver) { remembering.add(instance) @@ -1255,15 +1257,18 @@ } override fun releasing(instance: ComposeNodeLifecycleCallback) { - (releasing ?: mutableListOf<ComposeNodeLifecycleCallback>().also { - releasing = it - }) += instance + val releasing = releasing + ?: mutableScatterSetOf<ComposeNodeLifecycleCallback>().also { releasing = it } + + releasing += instance + forgetting += instance } fun dispatchRememberObservers() { // Send forgets and node deactivations if (forgetting.isNotEmpty()) { trace("Compose:onForgotten") { + val releasing = releasing for (i in forgetting.size - 1 downTo 0) { val instance = forgetting[i] abandoning.remove(instance) @@ -1271,7 +1276,12 @@ instance.onForgotten() } if (instance is ComposeNodeLifecycleCallback) { - instance.onDeactivate() + // node callbacks are in the same queue as forgets to ensure ordering + if (releasing != null && instance in releasing) { + instance.onRelease() + } else { + instance.onDeactivate() + } } } } @@ -1286,17 +1296,6 @@ } } } - - // Send node releases - val releasing = releasing - if (!releasing.isNullOrEmpty()) { - trace("Compose:releases") { - for (i in releasing.size - 1 downTo 0) { - val instance = releasing[i] - instance.onRelease() - } - } - } } fun dispatchSideEffects() {
diff --git a/compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/layout/SubcomposeLayoutTest.kt b/compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/layout/SubcomposeLayoutTest.kt index e3bd0f6..ef2de8e 100644 --- a/compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/layout/SubcomposeLayoutTest.kt +++ b/compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/layout/SubcomposeLayoutTest.kt
@@ -52,6 +52,7 @@ import androidx.compose.ui.composed import androidx.compose.ui.draw.assertColor import androidx.compose.ui.draw.drawBehind +import androidx.compose.ui.focus.isExactly import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.ImageBitmap import androidx.compose.ui.graphics.asAndroidBitmap @@ -2694,6 +2695,47 @@ } } + @Test + fun nestedDisposeIsCalledInOrder() { + val disposeOrder = mutableListOf<String>() + var active by mutableStateOf(true) + rule.setContent { + if (active) { + BoxWithConstraints { + BoxWithConstraints { + DisposableEffect(Unit) { + onDispose { + disposeOrder += "inner 1" + } + } + } + + DisposableEffect(Unit) { + onDispose { + disposeOrder += "outer" + } + } + + BoxWithConstraints { + DisposableEffect(Unit) { + onDispose { + disposeOrder += "inner 2" + } + } + } + } + } + } + + rule.runOnIdle { + active = false + } + + rule.runOnIdle { + assertThat(disposeOrder).isExactly("inner 2", "outer", "inner 1") + } + } + private fun SubcomposeMeasureScope.measure( slotId: Any, constraints: Constraints,