Is it a good practice passing `ModalBottomSheetSta...

# compose

[Tin Tran]

Is it a good practice passing ModalBottomSheetState to ViewModel?

[oianmol]

The ViewModel doesn't need to know if you have a bottom sheet or not 🤷 so i would choose not to provide the sheet state to VM.

[Tin Tran]

So if i want to control the sheet state from VM I’ll have to add a boolean to the view model and emit it to compose and have something like this in compose

LaunchedEffect(key1 = state.showBottomSheet) {
        if (state.showBottomSheet) {
            bottomSheetState.animateTo(ModalBottomSheetValue.Expanded)
        } else {
            bottomSheetState.animateTo(ModalBottomSheetValue.Hidden)
        }
    }

But if the user touch the scrim or swipe the bottom sheet down i’ll have to do this to notify the VM which I think is kind of cumbersome

val bottomSheetState = rememberModalBottomSheetState(
        initialValue = ModalBottomSheetValue.Hidden,
        confirmStateChange = {
            if (!state.bottomSheetCancelable) {
                return@rememberModalBottomSheetState false
            }
            if (it == ModalBottomSheetValue.Hidden) {
                Timber.d("test test test")
                keyboardController?.hide()
                viewModel.reduce(LoginEvent.HideBottomSheet)
            }
            true
        }
    )

[Albert Chang]

It’s perfectly valid. View models hold UI states and ModalBottomSheetState is a UI state.

[oianmol]

The sealed class of UIState which you construct for your views is less complex, but bottomSheetState is a complex object.

If in future you would want to move away from Bottom sheet's you would end up refactoring viewModels also,

I think VM's should be simple enough and should have minimum android dependencies. It's like method injection when you tell your VM about the bottomSheetState which is also a composable and not just some kotlin data class for UIstate.

[Albert Chang]

the bottomSheetState which is also a composable

No, ModalBottomSheetState is not a composable. It’s a UI state class.

If in future you would want to move away from Bottom sheet’s you would end up refactoring viewModels

What’s the problem of refactoring view models? Do you design your view model so that any future UI change doesn’t need any modification of view model? Is that possible anyway?

Creating this kind of unnecessary restriction for yourself only makes your code more verbose and more complicated. We usually call this over-engineering.

[oianmol]

It's a composable function form what I see, I would not write the bottom sheet interaction logic within the viewModel, it's like adding onClickListener in the VM

[Albert Chang]

But we are talking about ModalBottomSheetState instead of rememberModalBottomSheetState() 😄

[oianmol]

It's a rememberSaveable, and if config changes happen then it will reconstruct the ModalBottomSheetState for you! So what you essentially passed to your VM will not be the same instance. Does that make sense ? lmk if something is not clear or if I am wrong?

[oianmol]

Your VM's ModalBottomSheetState will be different after config change. that's why you should not pass it to your VM. It's an anti pattern. We are repeating the same mistakes again, it's like passing Activities to your AsyncTasks! 🤦

[Albert Chang]

Oh maybe you didn’t notice that ModalBottomSheetState has a public constructor which you can directly use to initialize the class in the view model.

[oianmol]

Why would you deal with all that when compose gives you rememberModalBottomSheetState() and take care of config changes with rememberSaveable ?

[oianmol]

@Ian Lake or @Manuel Vivo might help here for best practices, or @theapache64 🙂

[Albert Chang]

I don’t understand your question. What do I need to deal with when I can directly create a ModalBottomSheetState as a member of view model and the view model survives configuration changes?

[oianmol]

I see of course you can write a ModalBottomSheetState and use in the VM, but by moving it there you give your VM the responsibility to deal with UI interactions, for me I think of it as if I am writing click listeners within the VM, but you are right we just end up calling expand and hide so it works. I am just thinking out loud, if I follow what you are mentioning here would it lead to memory leaks or some unexpected behaviour 🤷

[fengdai]

Albert is right. All state objects are fine to be hoisted to view model. And no memory leak or unexpected behavior will happen.

[fengdai]

Simple state hoisting can be managed in the composable functions itself. However, if the amount of state to keep track of increases, or the logic to perform in composable functions arises, it’s a good practice to delegate the logic and state responsibilities to other classes: state holders.

&

a ViewModel is a special type of state holder

[Colton Idle]

Yeah. I do what Albert says as per @Manuel Vivos help on twitter.

[jossiwolf]

Using rememberModalBottomSheetState means that you get saving for free, yes. But it's perfectly fine for the state to live in the VM as part of your UI state🙂 Follow the same state saving best practices for that as with the rest of your UI state.

[Tin Tran]

Thanks guys

[Manuel Vivo]

Our recommendation is to keep the state closest to where it’s needed. If the bottom sheet state isn’t required in the VM (i.e. it isn’t necessary to derive other UI state nor requires business logic), then don’t hoist it in the VM. It’d be better placed in the UI close to its component.

As Jossi was saying, rememberMBSS saves the state across process and activity recreation. If you hoist it in the VM, you’d need to handle that yourself using the SavedStateHandle