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.

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

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

Closed
cryptrr opened this issue Oct 4, 2023 · 20 comments
Assignees
Labels
bug Something isn't working ios reproduced

Comments

@cryptrr
Copy link

cryptrr commented Oct 4, 2023

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:

  • iOS

Versions

  • Kotlin version*: 1.9.10
  • Compose Multiplatform version*: 1.5.2
  • OS version(s)* (required for Desktop and iOS issues): iOS 17.0
  • OS architecture (x86 or arm64): arm64 simulator

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.

@cryptrr cryptrr added bug Something isn't working submitted labels Oct 4, 2023
@cryptrr cryptrr changed the title UI state changes on iOS not reflecting when called from Kotlin Flow Collect methods. UI state changes on iOS not applied when called from Kotlin Flow Collect methods. Oct 4, 2023
@pjBooms
Copy link
Contributor

pjBooms commented Oct 4, 2023

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

@pjBooms pjBooms removed the submitted label Oct 4, 2023
@cryptrr
Copy link
Author

cryptrr commented Oct 4, 2023

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

Screen.Recording.2023-10-05.at.12.56.07.AM.mov
Screen.Recording.2023-10-05.at.12.58.34.AM.mov

@pablichjenkov
Copy link

pablichjenkov commented Oct 4, 2023

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
Copy link
Author

cryptrr commented Oct 4, 2023

I just tried putting viewModel as the key.

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

@cryptrr
Copy link
Author

cryptrr commented Oct 4, 2023

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

@pablichjenkov
Copy link

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

@cryptrr
Copy link
Author

cryptrr commented Oct 4, 2023

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
Copy link

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

@cryptrr
Copy link
Author

cryptrr commented Oct 4, 2023

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
Copy link
Author

cryptrr commented Oct 4, 2023

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

@cryptrr
Copy link
Author

cryptrr commented Oct 5, 2023

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
Copy link
Contributor

m-sasha commented Oct 5, 2023

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
Copy link
Author

cryptrr commented Oct 5, 2023

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
Copy link
Contributor

m-sasha commented Oct 5, 2023

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

@cryptrr
Copy link
Author

cryptrr commented Oct 5, 2023

Not working with DisposableEffect and SideEffect either.

@pjBooms pjBooms added the ios label Oct 5, 2023
@pjBooms pjBooms self-assigned this Oct 5, 2023
@m-sasha
Copy link
Contributor

m-sasha commented Oct 5, 2023

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
Copy link
Author

cryptrr commented Oct 5, 2023

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
Copy link

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
Copy link
Contributor

m-sasha commented Oct 5, 2023

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
Copy link

pablichjenkov commented Oct 5, 2023

Only if the lambda is using state that can change.

Correct, that is a good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ios reproduced
Projects
None yet
Development

No branches or pull requests

4 participants