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

Jump to bottom

Propagate composition locals to layers in the (re)composition phase #1233

Merged
merged 2 commits into from
Apr 4, 2024
Merged

Propagate composition locals to layers in the (re)composition phase #1233

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6315660
Apply compositionLocalContext, density and layoutDirection to layer i…
m-sasha Apr 3, 2024
1924208
Remove usage of `MultiLayerComposeScene.compositionLocalContext`
MatkovIvan Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 6 additions & 12 deletions compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneLayer.skiko.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,9 @@ package androidx.compose.ui.scene
import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalContext
import androidx.compose.runtime.DisposableEffect
import androidx.compose.runtime.SideEffect
import androidx.compose.runtime.currentCompositionLocalContext
import androidx.compose.runtime.getValue
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCompositionContext
import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.ui.InternalComposeUiApi
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.input.key.KeyEvent
Expand Down Expand Up @@ -172,27 +169,24 @@ internal fun rememberComposeSceneLayer(
val density = LocalDensity.current
val layoutDirection = LocalLayoutDirection.current
val parentComposition = rememberCompositionContext()
val compositionLocalContext by rememberUpdatedState(currentCompositionLocalContext)
val compositionLocalContext = currentCompositionLocalContext
val layer = remember {
scene.createLayer(
density = density,
layoutDirection = layoutDirection,
focusable = focusable,
compositionContext = parentComposition,
).also {
it.compositionLocalContext = compositionLocalContext
}
)
}
layer.focusable = focusable
layer.compositionLocalContext = compositionLocalContext
igordmn marked this conversation as resolved.
Show resolved Hide resolved
layer.density = density
layer.layoutDirection = layoutDirection

DisposableEffect(Unit) {
onDispose {
layer.close()
}
}
SideEffect {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like SideEffect was required because of rememberUpdatedState. The current state works fine

Copy link
Member

Choose a reason for hiding this comment

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

SideEffect was introduced in #562

layer.density = density
layer.layoutDirection = layoutDirection
layer.compositionLocalContext = compositionLocalContext
}
return layer
}
37 changes: 37 additions & 0 deletions compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/window/PopupTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.offset
import androidx.compose.foundation.layout.size
import androidx.compose.material.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.DisposableEffect
Expand Down Expand Up @@ -58,6 +59,7 @@ import androidx.compose.ui.test.assertIsDisplayed
import androidx.compose.ui.test.assertPositionInRootIsEqualTo
import androidx.compose.ui.test.assertWidthIsEqualTo
import androidx.compose.ui.test.onNodeWithTag
import androidx.compose.ui.test.runComposeUiTest
import androidx.compose.ui.test.runSkikoComposeUiTest
import androidx.compose.ui.touch
import androidx.compose.ui.unit.Density
Expand Down Expand Up @@ -125,6 +127,41 @@ class PopupTest {
assertThat(actualLocalValue).isEqualTo(3)
}

// https://github.com/JetBrains/compose-multiplatform/issues/4558
@Test
fun changeInStaticCompositionLocalVisibleImmediatelyInPopup() = runComposeUiTest {
// Test that when the provided value of a staticCompositionLocalOf changes, the change is
// seen correctly in the content of a popup.
// The `text` variable is needed in order to cause a recomposition of the popup content in
// the same frame as CompositionLocalProvider is recomposed. Without this the bug doesn't
// reproduce because first CompositionLocalProvider is recomposed and that triggers
// recomposition of the popup content, which happens afterward.

val compositionLocal = staticCompositionLocalOf<Int> {
error("not set")
}
var providedValue by mutableStateOf(1)
var text by mutableStateOf("1")
var actualLocalValue = 0

setContent {
CompositionLocalProvider(compositionLocal provides providedValue) {
Popup {
actualLocalValue = compositionLocal.current.also {
println(it)
}
Text(text)
}
}
}

assertThat(actualLocalValue).isEqualTo(providedValue)
text = "2"
providedValue = 2
waitForIdle()
assertThat(actualLocalValue).isEqualTo(providedValue)
}

// https://github.com/JetBrains/compose-multiplatform/issues/3142
@Test
fun passLayoutDirectionToPopup() = runSkikoComposeUiTest {
Expand Down