Popup content sees previous value of staticCompositionLocal

[Omico]

Compose Multiplatform: 1.6.1

2024-03-29_09-19-19.mp4

[m-sasha]

Can you post a short reproducer?

[Omico]

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.

[m-sasha]

Can you post a short reproducer?

[Omico]

Can you post a short reproducer?

See https://github.com/Omico/issue-compose-multiplatform-4558/blob/ce10cc732b75c7757bc9c1088db72663e8d84161/gpi/shared/src/commonMain/kotlin/me/omico/gpi/shared/App.kt

[m-sasha]

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.

[m-sasha]

The problem seems to be with a change introduced in JetBrains/compose-multiplatform-core#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

[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 into Popup.

Same as above - it will work ONLY for drawing Popup inslde same canvas (MultiLayerComposeScene)

[m-sasha]

Why does composition depend on where the composables are drawn (canvas)?

[MatkovIvan]

Because in case of single canvas it reuses already existing environment/contexts, but in other cases it cannot do that

[m-sasha]

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.

[m-sasha]

I'd recommend

• Short term: set the layer properties (layoutDirection, density and compositionLocalContext) immediately when rememberComposeSceneLayer is composed.
• Long term: correctly set up the composition hierarchy, and get rid of compositionLocalContext.

[igordmn]

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.

[m-sasha]

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.

[igordmn]

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.

[okushnikov]

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.