l

    Lilly

    1 year ago
    Hi I have 3 questions regarding state. I have the following composable:
    @Composable
    fun BluetoothDeviceListAdapterComponent(
        items: List<BluetoothDeviceWrapper>,
        connectVM: ConnectViewModel,
        onConnected: () -> Unit
    ) {
    
        // initial state is a problem here
        val connectState: BluetoothConnectState by connectVM.state.collectAsState()
    
        onCommit(connectState) {
            Timber.w("connectState changed")
            when(connectState) {
                is BluetoothConnectState.Loading -> {
                    Timber.w("Loading")
                }
                is BluetoothConnectState.Success -> {
                    Timber.w("GoTo next screen.")
                    onConnected()
                }
                is BluetoothConnectState.Failure -> {
                    // how to call toast from here?
                }
            }
        }
    state
    is a
    MutableStateFlow
    and can be Loading, Success or Failure(val errorMessage). First problem is that
    MutableStateFlow
    needs an initial value which triggers
    onCommit
    on first start but it should only be triggered when viewModel changes its state explicitly. Second problem is when state is Success and next screen is started: When I get back to the screen,
    onCommit
    is trigged with value Success and starts the next screen again, although the state hasn't changed. Last question would be how to call a Toast from
    onCommit
    ? Is this approach even optimal for my use case?
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    First pass – I'd suggest taking a read through https://developer.android.com/jetpack/compose/state. This code has some side-effects from recomposition (e.g. onConnected()) which will be hard to manage in recomposition. It appears you're treating
    connectedState
    as an event in this code and trying to modify other states in response to it changing. This route will make UDF hard to implement. There's a few bits of state here that I see, that you may find easier if you model them separately: 1. The current screen 2. A toast message Then, in compose consume the states that describe the UI at the present time.
    For snackbar you can use a snackbar host
    l

    Lilly

    1 year ago
    @Sean McQuillan [G] Thanks for your answer.
    It appears you're treating 
    connectedState
     as an event in this code and trying to modify other states in response to it changing.
    The scenario is a little bit different:
    @Composable
    fun BluetoothDeviceListAdapterComponent(
        items: List<BluetoothDeviceWrapper>,
        viewModel: ConnectViewModel,
        onConnected: () -> Unit
    ) {
        val uiState: BluetoothConnectState by viewModel.state.collectAsState() /* state */
    
        when (uiState) {
            is BluetoothConnectState.Loading -> Text("Loading...")
            is BluetoothConnectState.Success -> onConnected() // --> next screen
            is BluetoothConnectState.Failure -> // TODO
        }
    
        LazyColumnFor(items) { item ->
            BluetoothDeviceListItemComponent(item, onActionClick = { device ->
                viewModel.connect(device) /* event */
            }
        })
    }
    Every
    BluetoothDeviceListItemComponent
    has a Button which triggers the
    onActionClick
    event which in turn triggers a non-suspending function in the VM that is consuming a flow. The results are stored in a
    StateFlow<BluetoothConnectState>
    which is observed by the
    BluetoothDeviceListAdapterComponent
    . The first result is Loading, when bt device is connected, state changes to Success otherwise Failure. I can't figure out how to handle the initial state. A Loading composable should only be displayed after event
    onActionClick
    is triggered because the flow itself already exposes a Loading state at the right time.
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    👀
    Immediately jumping out at me is that
    onConnected
    is called by recomposition instead of an event handler. Trying to figure out how to advise an alternative
    Ah figured it out – so the underhoisting stems from passing both the
    viewModel
    and
    onConnected
    What's happening is that
    onConnected
    as an event changes state that's hoisted somewhere higher in the tree. This leads you to build a composition -> event bridge like here where you call
    onConnected
    by recomposition
    First thing to note about this code style is (1)
    onConnected
    may be called multiple times due to recomposition and (2)
    onConnected
    may be called during a composition that is later discarded when it shouldn't
    l

    Lilly

    1 year ago
    The code does not have to be in the
    BluetoothDeviceListAdapterComponent
    composable. It could be also in the highest level
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    This is the third part of the hoisting advice in the state in compose page (it's a bit subtle)
    "When two states are modified in response to the same events, they should be hoisted together"
    In this case, the "current screen" state should be determined at the same place as the connected status state
    which is in the ViewModel
    That will make this end up working a bit closer to expectations – as another cleanup you can also remove the direct dependency on the viewModel here and pass
    onConnectDevice: (Device) -> Unit
    as a parameter to this composable
    l

    Lilly

    1 year ago
    Your are too fast for me. Can you please explain what you mean with
    What's happening is that 
    onConnected
     as an event changes state that's hoisted somewhere higher in the tree
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    Ok got the general gist of everything. I'd suggest replacing this with composable that takes
    items: ..., uiState: LoadingState, onConnectDevice: ...)
    and simply displays stuff
    then the caller is responsible for managing all this state
    For the initial loading screen for collectAsState – if you intend for there to be a pre-loading state I'd suggest modeling it (either explicitly asInitial or by using
    null
    )
    l

    Lilly

    1 year ago
    What's happening is that 
    onConnected
     as an event changes state that's hoisted somewhere higher in the tree
    What state does
    onConnected
    change?
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    To get to the next screen it has to modify some state higher in the tree?
    (I don't have code so I had to fill in some details 😉 )
    l

    Lilly

    1 year ago
    Yes thats right. I'm using the router-compose library from zsoltk
    In this case, the "current screen" state should be determined at the same place as the connected status state -> which is in the ViewModel
    What is the current screen state here?
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    Ok so taking a step back – to display this composable there is a state in the router library that says "this is the current screen"
    Then, there's some code between there and here that produces a ViewModel
    And then this composable?
    l

    Lilly

    1 year ago
    So far so good
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    calling onConnected is this composables request to modify that state (it's an event)
    l

    Lilly

    1 year ago
    True
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    This code here (which pretty much everyone including me has written once in Compose) is a state to event bridge
    The
    uiState
    is driving recomposition of this composable, then as a side-effect of recomposition it's calling
    onConnected
    the fundamental reason for this is that
    onConnected
    and
    uiState
    are hoisted to different levels
    There's two basic solutions to this: 1. Hoist
    uiState
    to the same place as the state
    onConnected
    modifies 2. Call
    onConnected
    from an event handler (instead of in response to state changes)
    l

    Lilly

    1 year ago
    ok got you
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    Given the existence of a viewModel – it seems likely that it's this viewModels responsibility to dispatch the follow on event (just taking a guess at the rest of the code)
    Then, if you encapsulate that, then this composable doesn't need to know about anything except a header state,
    items
    , and the event to notify the viewModel about an item click
    (which, makes this composable a lot easier to write 🙂 )
    l

    Lilly

    1 year ago
    Give me a moment
    @Sean McQuillan [G] ok I can reduce the composable to
    items
    and a
    onConnectDevice: (Device) -> Unit
    parameter. This will hoist the state up to a higher level but how to go further from here. Fact is that
    onConnected
    is a reaction to a state change not? or what do you mean with
    it seems likely that it's this viewModels responsibility to dispatch the follow on event
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    So the desired result is that calling the event handler (
    viewModel.connect
    in the code you pasted) eventually calls
    onConnected
    l

    Lilly

    1 year ago
    Thats the VM part:
    private val _state: MutableStateFlow<ConnectBluetoothDeviceUseCase.BluetoothConnectState> =
            MutableStateFlow(
                ConnectBluetoothDeviceUseCase.BluetoothConnectState.Loading
            )    
    
        val state: StateFlow<ConnectBluetoothDeviceUseCase.BluetoothConnectState> = _state.asStateFlow()
    
        fun connect(device: BluetoothDevice) {
            viewModelScope.launch {
                connectBluetoothDevicesUseCase.connect(device).collect {
                    _state.value = it
                }
            }
        }
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    ah perfect
    l

    Lilly

    1 year ago
    Sorry for the inconvenience 🙈
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    So, you already have the continuation where you're doing
    _state.value = it
    Nah this is more fun than the docs I'm putting off 🙂
    Right there you can call
    onConnected
    safely (though since it's an event I'd recommend ensuring it's on Main dispatcher before calling it)
    The big insight here is that this ViewModel fits itself into a UDF system, and is not the "top" of the UDF diagram – the navigation composable above it is above this ViewModel in a UDF diagram
    so you can call an event "up" here while still doing UDF
    l

    Lilly

    1 year ago
    I have updated my code snippet for the missing
    _state.value
    part.
    onConnected
    is this:
    @Composable
    fun DcInternEntryPoint(defaultRouting: Routing) {
        Router(defaultRouting) { backStack ->
            when (backStack.last()) {
                is Routing.Scanner -> {
                    ScannerScreen(onConnected =  {
                        backStack.push(Routing.Test)
                    })
                }
            }
        }
    }
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    This looks correct
    So now everything should be working except the header state?
    l

    Lilly

    1 year ago
    I have really problems with your terms ^^ What do you mean with header state. And I'm also not aware of an UDF diagram
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    Moment
    Er I forgot one
    practically you can consider the VM an "implementation detail" of ScannerScreen and not draw it
    But this is a full UDF diagram
    "Header state" is the state that tells this composable what to display in the header (or if it should be hidden)
    In the code we started with it was derived from
    uiState
    (er sorry defining acronym: UDF = Unidirectional Data Flow)
    l

    Lilly

    1 year ago
    Ah ok. Thanks for the diagram! So you said I should call
    onConnect
    in VM but then I'm aksing how should this look like. I have no reference to the
    backStack
    in VM???
    onConnected:
    ScannerScreen(onConnected = {
    		backStack.push(Routing.Test)
    	})
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    Oh it's just a lambda – you can pass it to the VM
    You wouldn't actually deal with the back stack in the
    ScannerScreenViewModel
    , but you can call
    onConnected()
    from it
    l

    Lilly

    1 year ago
    @Sean McQuillan [G] I refactored screen implementation and cleaned up the code. So thats what I have so far for the VM:
    fun connect(device: BluetoothDevice, onConnected: () -> Unit) {
            viewModelScope.launch {
                connectBluetoothDevicesUseCase.connect(device).collect { connectState ->
                    when (connectState) {
                        BluetoothConnectState.Loading -> _state.value = connectState
                        BluetoothConnectState.Success -> onConnected()
                        is BluetoothConnectState.Failure -> _state.value = connectState
                    }
    
                }
            }
        }
    And the UI stuff
    It's working and now its not affected from recomposition. But I guess it's still not optimal. I also have a question regarding hoisting. The more I hoist state to the higher level composable the more levels have to recompose. Isn't this a circumstance which should be avoided? For example lets assume my
    ScreenContent
    function would display additional elements like a Button, Text or whatever. Now with the current implementation, every time
    discoveryState
    is changed, not only the
    BluetoothDeviceListAdapterComponent
    composable recomposes but also the additional elements like Button, Text would recompose. --> So wouldn't it be better to keep the
    discoveryState
    in
    BluetoothDeviceListAdapterComponent
    so that this is the only composable that is recomposed?
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    Compose skips recomposition aggressively for both parents and children of a composable. So I wouldn't motivate under hoisting for these performance reasons
    There's two major problems that arise from underhoisting: 1. The state->event trampoline always introduces one recomposition delay to trigger the event. This may be as long as a frame. 2. It leads to duplicated state
    Iff you profiled and found that this state was a trigger for unacceptable levels of work after skipping there's a few solutions: 1. Break up the composable into smaller sibling components that have a better skipping story (while leaving state hoisted). 2. Work with Compose to make this object more skippable by improving it's stability 3. Pass a state-lambda parameter (a lambda that returns the current value of the state) instead of the raw value, this has the effect of making a box that allows recomposition to skip until it's unboxed
    But, taking a step back, the compose compiler/runtime is designed intentionally to produce efficient programs in the presence of hoisting like this.
    Took a look at the code, looks good!
    l

    Lilly

    1 year ago
    @Sean McQuillan [G] Thank you so much for your help. Really appreciated! 🙂 ❤️
    So in some scenarios, it wouldn't be a problem to hoist the state to the level below the navigation-composable, that says the composable where I define the
    Scaffold
    for instance?
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    Yea – it seems like that will be pretty common thinking about it
    The big thing to avoid is reading that state in composition and triggering navigation events from inside the execution of recomposition. As long as the navigation events are called by event handlers it'll work great
    l

    Lilly

    1 year ago
    ok got it. Just 2 things I don't understand fully. What do you mean with:
    The state->event trampoline always introduces one recomposition delay to trigger the event.
    and what skipping do you mean here?
    Iff you profiled and found that this state was a trigger for unacceptable levels of work after skipping there's a few solutions
    Sean McQuillan [G]

    Sean McQuillan [G]

    1 year ago
    1: When you read a state from the execution of a composable, that happens the next time recomposition is scheduled (the recomposition was scheduled by the state change). This scheduling is roughly coordinated with frame times so it tends to introduce 1frame delays. 2. The compose compiler and runtime skip the execution of all composables they can prove can't have changed since the last recomposition due to their inputs
    l

    Lilly

    1 year ago
    @Sean McQuillan [G] Thanks for your answer