Closed Omico closed 5 months ago
Can you post a short reproducer?
Hi @m-sasha, please take a look at https://github.com/Omico/issue-compose-multiplatform-4558. The issue only exists if we change the typography.
Can you post a short reproducer?
Reproducer:
val TestLocal = staticCompositionLocalOf { 0 }
fun main() = singleWindowApplication {
var value by remember { mutableStateOf(1) }
CompositionLocalProvider(TestLocal provides value) {
println("Outside Value: ${TestLocal.current}")
Popup(
onDismissRequest = { },
) {
println("Inside Value: ${TestLocal.current}")
Row(verticalAlignment = Alignment.CenterVertically) {
Button(
onClick = {
value = 2
},
content = { Text("2") },
)
Spacer(modifier = Modifier.width(8.dp))
Button(
onClick = {
value = 1
},
content = { Text("1") },
)
Text(TestLocal.current.toString(), Modifier.padding(8.dp))
}
}
}
}
Clicking the buttons, the value seen inside the Popup is always the previous value of the local.
If we change TestLocal
to be a compositionLocalOf
instead of staticCompositionLocalOf
, the issue isn't seen.
The problem seems to be with a change introduced in https://github.com/JetBrains/compose-multiplatform-core/pull/1086
In rememberComposeSceneLayer
we update the layer's compositionLocalContext
in a SideEffect
, but this is too late because composition happens before SideEffect
is executed.
Also, I don't entirely understand why a layer needs compositionLocalContext
. Aren't the locals already visible through the parent composition (passed via compositionContext
in scene.createLayer
)? In Android, a popup only passes the parent composition when creating a popup. And if I remove this passing of currentCompositionLocalContext
, the bug goes away, and composition locals still seem to be correctly passed into Popup
.
@igordmn @MatkovIvan
I don't entirely understand why a layer needs
compositionLocalContext
There is no public (outside of runtime
) way to extract it from compositionContext
, I wrote about it in mentioned PR
Aren't the locals already visible through the parent composition (passed via compositionContext in scene.createLayer)?
Only if it IS parent composition (MultiLayerComposeScene
) - it's not always a case for multiplatform, and cannot be for a general case (due to requirement of different coroutine context for different windows)
And if I remove this passing of
currentCompositionLocalContext
, the bug goes away, and composition locals still seem to be correctly passed intoPopup
.
Same as above - it will work ONLY for drawing Popup
inslde same canvas (MultiLayerComposeScene
)
Why does composition depend on where the composables are drawn (canvas)?
Because in case of single canvas it reuses already existing environment/contexts, but in other cases it cannot do that
In Android, Popup
creates a new (Android) View and in it a new Composition
with the parent composition set to rememberCompositionContext()
. See ComposeView.ensureCompositionCreated()
.
We, on the other hand, don't pass the parent composition; the new Composition
is created with (a new) Recomposer
as its parent. RootNodeOwner.setContent
is called with parent
set to BaseComposeScene.compositionContext
, which is a newly created Recomposer
.
That seems to be why the composition locals aren't propagated automatically. I imagine it has other implications, and possibly bugs, too.
I'd recommend
layoutDirection
, density
and compositionLocalContext
) immediately when rememberComposeSceneLayer
is composed.compositionLocalContext
.Long term: correctly set up the composition hierarchy, and get rid of compositionLocalContext.
We can't do that, because popup/dialog can have its recomposition cycle. For example, in a case when they are separate windows showed on another screen with a different refresh rate.
The short term solution seems the right solution. Even if the popup content is recomposed not in sync, it should see the most recent composition locals.
We can't do that, because popup/dialog can have its recomposition cycle. For example, in a case when they are separate windows showed on another screen with a different refresh rate.
I'm not convinced this necessarily means that we can't have a correct composition hierarchy. Why isn't it possible for each ComposeScene to have its own Recomposer
, and the Composition
have its parent set to the parent CompositionContext? It may require a few changes in compose.runtime
, but I'm not even sure that it will.
It may be possible in theory if we allow somehow compositions to be recomposed at different pace. How we do that - having one or multiple Recomposer
's - is implementation details. But that can require a lot of work.
Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.
Compose Multiplatform:
1.6.1
https://github.com/JetBrains/compose-multiplatform/assets/14316223/c8f58d11-3139-425e-9b3b-37a7aabdc744