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
16.26k stars 1.18k forks source link

UI state changes on iOS not applied when called from Kotlin Flow Collect methods. #3766

Closed cryptrr closed 1 year ago

cryptrr commented 1 year ago

Describe the bug UI state changes on iOS not reflecting when called from Kotlin Flow Collect methods.

I am collecting events from flows to show ModalBottomSheetLayout. When I call the ModalBottomSheetState.show() from a normal code(non flow code), it will work. When called from a flow collector, it will not update the UI or show the Bottomsheet. In fact the bottomsheet actually gets updated in the background underneath the screen.

I unsuccessfully tried wrapping it in withContext(Dispatchers.Main){}

It really seems like there is a threading problem somewhere.

It works perfectly on Android.

The global event state collector in viewmodel

private val _eventFlow = MutableSharedFlow<ViewModelEvent>(replay = 0)
val eventFlow = _eventFlow.asSharedFlow()

val commentsBottomSheetState = rememberModalBottomSheetState(
     initialValue = ModalBottomSheetValue.Hidden,
     skipHalfExpanded = true
)

val miscUtilsBottomSheetState = rememberModalBottomSheetState(
     initialValue = ModalBottomSheetValue.Hidden,
     skipHalfExpanded = true
 )

LaunchedEffect(key1 = true) {
        viewModel.main.eventFlow.collectLatest {
            when (it) {
                is ViewModelEvent.OpenComments -> {
                    withContext(Dispatchers.Main){     //Not working either.
                        Logger.d{"Opening Comments Sheet MainThread  : ${it.post.id}"}
                        currentCommentsSheetDetails.value = it.post
                        commentsBottomSheetState.show()
                    }
                }
                is ViewModelEvent.ShowUserOptions -> {
                    currentReportPost.value = it.currentItem
                    miscUtilsBottomSheetState.show()
                }
                else -> {}
            }
        }
    }

Affected platforms Select one of the platforms below:

Versions

To Reproduce Steps and/or the code snippet to reproduce the behavior: Create a MutableSharedFlow in a global ViewModel and try changing UI state from its collect or collectLatest method inside a composable.

Expected behavior The bottomSheet state should change and the sheet should be shown.

pjBooms commented 1 year ago

May you provide a reproducible example? It's a bit hard to make it from the code snippet you provided

cryptrr commented 1 year ago

I reproduced it in the template

https://github.com/cryptrr/compose-multiplatform-ios-flow-uistate-bug

It works perfectly on android but fails in iOS

https://github.com/JetBrains/compose-multiplatform/assets/135767626/e7c4e29d-2ffa-4f72-9975-5c9674d7df4f

https://github.com/JetBrains/compose-multiplatform/assets/135767626/322b9016-71d3-4fe7-bb39-3c60e195ac2b

pablichjenkov commented 1 year ago

What if you place the ViewModel as a key in the LaunchEffect? I believe what happens is your App recomposes and your LaunchEffect lambda keeps referencing the old, staled ViewModel instance, possibly leaking it too. In Android the App doesn't recompose the first time maybe because the internal compose machinery inside setContent {} is a bit more optimized. But if you trigger recomposition of App() by other means, you may see the same behavior. You can verify printing the ViewModel instance, perhaps is not what I am thinking.

cryptrr commented 1 year ago

I just tried putting viewModel as the key.

Unfortunately, that doesn't seem to be the problem either.

cryptrr commented 1 year ago

Weird thing is, I am actually receiving the Event in the collector. It's only the ModalBottomSheetState.show() that is not running.

pablichjenkov commented 1 year ago

Seems related to the coroutine scope, seems that it needs to be executed from the scope returned by rememberCoroutineScope , could you try that

cryptrr commented 1 year ago

I tried that some time ago. That ain't the problem either.

val scope = rememberCoroutineScope()

    LaunchedEffect(key1 = viewModel) {
        scope.launch {
            viewModel.eventFlow.collectLatest {

                when (it) {
                    is ViewModelEvent.OpenComments -> {
                        //scope.launch {  //Putting the launch here doesn't work either
                              currentCommentsSheetDetails.value = it.postId
                              commentsBottomSheetState.show()
                        //}            
                    }
                    else -> {}
                }
            }
        }
    }
pablichjenkov commented 1 year ago

Humm I see🤔. Internal BottomSheet implementation stuff then 🤷🏻. Would be interesting to see what the real cause is.

cryptrr commented 1 year ago

This same problem also happens where I show internal App Notifications. The app notifications are received on the event flow collector from the viewModel. But the the notification is not shown on ios.

So I'm not sure this is a problem with BottomSheet implementation.

cryptrr commented 1 year ago

Idk. This seems it could be some weird UI Thread issue.

cryptrr commented 1 year ago

This is reproduced on 1.4.3 . @pjBooms @eymar @igordmn @dima-avdeev-jb @SebastianAigner This is a serious problem for all event driven apps. Is there any workaround that we can use until this is fixed?

m-sasha commented 1 year ago

This doesn't seem to do anything with the shared flow. The bottom sheet doesn't show when asked from a LaunchedEffect:

@OptIn(ExperimentalMaterialApi::class)
@Composable
fun App() {
    MaterialTheme {
        val bottomSheetState = rememberModalBottomSheetState(
            initialValue = ModalBottomSheetValue.Hidden,
            skipHalfExpanded = true
        )

        LaunchedEffect(Unit) {
            delay(2000)
            println("Showing bottom sheet in LaunchedEffect")
            bottomSheetState.show()
            println("Done showing bottom sheet")
        }

        val coroutineScope = rememberCoroutineScope()
        ModalBottomSheetLayout(
            sheetState = bottomSheetState,
            sheetContent = {
                Box(
                    modifier = Modifier.fillMaxWidth().height(400.dp),
                    contentAlignment = Alignment.Center
                ){
                    Text("I am Bottom Sheet!")
                }
            }
        ) {
            Button(
                onClick = {
                    coroutineScope.launch {
                        bottomSheetState.show()
                    }
                }
            ) {
                Text("Open Bottom Sheet")
            }
        }
    }
}
cryptrr commented 1 year ago

Weird. Seems the BottomSheet is broken on Flows altogether. Have you tried opening similar UI elements like SnackbarHost? For me that's not working either.

m-sasha commented 1 year ago

Doesn't seem to be related to flows, but a problem with LaunchedEffect, or some machinery related to it.

cryptrr commented 1 year ago

Not working with DisposableEffect and SideEffect either.

m-sasha commented 1 year ago

So turns out there's a problem which causes an extra recomposition of App (which we'll fix), but the real issue is that you forgot to put the bottomSheetState as a key to LaunchedEffect. When App is recomposed and a new instance of bottomSheetState is created, the lambda in LaunchedEffect is not recreated, and is referencing the old bottomSheetState.

cryptrr commented 1 year ago

Great, that fixes it. It's just that now I need to pass in all the 15 bottomsheetstates as key to the LaunchedEffect 😩 . Thank you @m-sasha , close the issue if you may.

Edit: Just one BottomSheetState key is necessary in this case.

pablichjenkov commented 1 year ago

Oh great to know. In general LaunchEffect(Unit) shouldn't be used. I think there is a linting rule now that prohibits doing it.

m-sasha commented 1 year ago

Oh great to know. In general LaunchEffect(Unit) shouldn't be used. I think there is a linting rule now that prohibits doing it.

Only if the lambda is using state that can change. The most common use of LaunchedEffect(Unit) is to request initial focus with a remembered FocusRequester. It should be fine in that case (if the FocusRequester is remembered with no keys).

pablichjenkov commented 1 year ago

Only if the lambda is using state that can change.

Correct, that is a good point.

okushnikov commented 2 months ago

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