JetBrains / compose-multiplatform

Compose Multiplatform, a modern UI framework for Kotlin that makes building performant and beautiful user interfaces easy and enjoyable.
https://jetbrains.com/lp/compose-multiplatform
Apache License 2.0
15.72k stars 1.14k forks source link

Change of `staticCompositionLocalOf` value is not propagated correctly in Voyager. #4685

Closed igorescodro closed 3 months ago

igorescodro commented 3 months ago

Describe the bug The Material Theme is no longer updating between Dark/Light anymore in the Compose Compiler 1.6.2. I have the following code to observe the theme change and recompose the theme based on the DataStore update.

@Composable
fun AlkaaMultiplatformApp(
    navigationAction: NavigationAction = NavigationAction.Home,
    modifier: Modifier = Modifier,
    onThemeUpdate: (isDarkTheme: Boolean) -> Unit = {},
) {
    val isDarkTheme = rememberIsDarkTheme()
    onThemeUpdate(isDarkTheme)
    AlkaaTheme(isDarkTheme = isDarkTheme) {
        AlkaaNavGraph(
            navigationAction = navigationAction,
            modifier = modifier,
        )
    }
}

@Composable
private fun rememberIsDarkTheme(viewModel: AppViewModel = koinInject()): Boolean {
    val isSystemDarkTheme = isSystemInDarkTheme()

    val theme by remember(viewModel) {
        viewModel.loadCurrentTheme()
    }.collectAsState(initial = AppThemeOptions.SYSTEM)

    val isDarkTheme = when (theme) {
        AppThemeOptions.LIGHT -> false
        AppThemeOptions.DARK -> true
        AppThemeOptions.SYSTEM -> isSystemDarkTheme
    }
    return isDarkTheme
}

This is the code change that is making the MaterialTheme not work properly: https://github.com/igorescodro/alkaa/compare/v3.0.0...v3.0.1 If I roll back the libs.versions.toml to Compiler 1.5.11, the issue is no longer reproducible. The issue is not reproducible in the Android app, only iOS.

Could this be a possible regression of this change? https://github.com/JetBrains/compose-multiplatform-core/pull/715

Please, let me know if I have a wrong understanding of how to use the API. Thank you!

Affected platforms

Versions

elijah-semyonov commented 3 months ago

Investigation of this particular sample is required. Platform specific API works correctly:

val modifier = if (isSystemInDarkTheme()) {
        Modifier.background(Color.Gray)
    } else {
        Modifier.background(Color.White)
    }
    Box(modifier.fillMaxSize()) {
        Text("Background color depends on current system theme")
 }
elijah-semyonov commented 3 months ago

Thanks for a nice sample, reproduced.

https://github.com/elijah-semyonov/alkaa/commits/main/

Launching the app and changing the theme produces this log:

AlkaaLightColorScheme.primary: Color(0.0, 0.37254903, 0.69411767, 1.0, sRGB IEC61966-2.1)
AlkaaDarkColorScheme.primary: Color(0.6431373, 0.78431374, 1.0, 1.0, sRGB IEC61966-2.1)

isDarkTheme: false
Resolved MaterialTheme.colors.primary: Color(0.0, 0.37254903, 0.69411767, 1.0, sRGB IEC61966-2.1)
AlkaaNavGraph, MaterialTheme.colors.primary: Color(0.0, 0.37254903, 0.69411767, 1.0, sRGB IEC61966-2.1)
AlkaaNavGraph:BottomSheetNavigator, MaterialTheme.colors.primary: Color(0.0, 0.37254903, 0.69411767, 1.0, sRGB IEC61966-2.1)
AlkaaNavGraph:BottomSheetNavigator:Navigator, MaterialTheme.colors.primary: Color(0.0, 0.37254903, 0.69411767, 1.0, sRGB IEC61966-2.1)

// Theme changes

isDarkTheme: true
Resolved MaterialTheme.colors.primary: Color(0.6431373, 0.78431374, 1.0, 1.0, sRGB IEC61966-2.1)
AlkaaNavGraph, MaterialTheme.colors.primary: Color(0.6431373, 0.78431374, 1.0, 1.0, sRGB IEC61966-2.1)
AlkaaNavGraph:BottomSheetNavigator, MaterialTheme.colors.primary: Color(0.6431373, 0.78431374, 1.0, 1.0, sRGB IEC61966-2.1)
// LocalColorScheme.current, accessed through MaterialTheme.colorScheme is not updated
AlkaaNavGraph:BottomSheetNavigator:Navigator, MaterialTheme.colors.primary: Color(0.0, 0.37254903, 0.69411767, 1.0, sRGB IEC61966-2.1)
elijah-semyonov commented 3 months ago

Related issue: https://github.com/adrielcafe/voyager/issues/391

elijah-semyonov commented 3 months ago

On iOS Navigator Composable is recomposed with wrong(first non-default) staticCompositionLocal value. On Desktop, the Navigator Composable is not recomposed at all.

enum class DummyTheme {
    DARK, LIGHT, UNKNOWN
}

val DummyThemeLocal = staticCompositionLocalOf {
    DummyTheme.UNKNOWN
}

@OptIn(ExperimentalMaterialApi::class)
@Composable
fun AlkaaMultiplatformApp(
    navigationAction: NavigationAction = NavigationAction.Home,
    modifier: Modifier = Modifier,
    onThemeUpdate: (isDarkTheme: Boolean) -> Unit = {},
) {
    println("App")
    var isDark by remember { mutableStateOf(true) }

    val theme = if (isDark) {
        DummyTheme.DARK
    } else {
        DummyTheme.LIGHT
    }

    Button(onClick = {
        isDark = !isDark
    }) {
        Text("Toggle")
    }

    CompositionLocalProvider(
        DummyThemeLocal provides theme
    ) {
        println("CompositionLocalProvider")
        BottomSheetNavigator {
            println("BottomSheetNavigator")
            println(DummyThemeLocal.current)
            Navigator(screen = object : Screen {
                @Composable
                override fun Content() = Unit
            }) { navigator ->
                println("Navigator")
                println(DummyThemeLocal.current)
            }
        }
    }
}

Confirmed, that the problem is not present on Android. The issue doesn't seem to be related to PersistentCompositionLocalHashMap, there are no recent code changes in there. Voyager uses currentCompositeKeyHash, which is explicitly marked as InternalComposeApi, which looks very suspicious, but doesn't explain why Android behaves correctly.

m-sasha commented 3 months ago

Standalone reproducer:

import androidx.compose.material.Button
import androidx.compose.material.Text
import androidx.compose.runtime.*
import androidx.compose.ui.window.singleWindowApplication

val MyLocal1 = staticCompositionLocalOf<Int> { error("Not provided") }
val MyLocal2 = staticCompositionLocalOf<Int> { error("Not provided") }

fun main() = singleWindowApplication {
    var number by remember { mutableStateOf(1) }

    Button(
        onClick = { number += 1 }
    ) {
        Text("Toggle")
    }

    CompositionLocalProvider(MyLocal1 provides number) {
        CompositionLocalProvider(MyLocal2 providesDefault 5) {
            println("1: ${MyLocal1.current}")
            CompositionLocalProvider(MyLocal2 providesDefault 5) {
                println("2: ${MyLocal1.current}")
                Text(MyLocal1.current.toString())
            }
        }
    }
}

The issue is either https://issuetracker.google.com/issues/330036209 or another problem with providesDefault. I'll see that it's fixed on the Google side, but in the meanwhile, Voyager can work around this problem by using

CompositionLocalProvider(values = arrayOf(MyLocal2 providesDefault 5)) {
m-sasha commented 3 months ago

Ok, whether it's that issue or something else, it no longer reproduces in 1.6.10.

okushnikov commented 1 month ago

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