https://kotlinlang.org logo
Channels
Powered by
l

Lilly

12/04/2020, 1:17 AM
Hi I have 3 questions regarding state. I have the following composable:
Copy code
@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?
s

Sean McQuillan [G]

12/04/2020, 10:56 PM
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

12/05/2020, 1:06 AM
@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:
Copy code
@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.
s

Sean McQuillan [G]

12/05/2020, 1:08 AM
👀
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

12/05/2020, 1:16 AM
The code does not have to be in the 
BluetoothDeviceListAdapterComponent
composable. It could be also in the highest level
👍 1
s

Sean McQuillan [G]

12/05/2020, 1:16 AM
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

12/05/2020, 1:20 AM
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
s

Sean McQuillan [G]

12/05/2020, 1:20 AM
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 as`Initial` or by using 
null
)
l

Lilly

12/05/2020, 1:24 AM
What's happening is that 
onConnected
 as an event changes state that's hoisted somewhere higher in the tree
What state does 
onConnected
change?
s

Sean McQuillan [G]

12/05/2020, 1:24 AM
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

12/05/2020, 1:25 AM
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?
s

Sean McQuillan [G]

12/05/2020, 1:27 AM
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

12/05/2020, 1:28 AM
So far so good
s

Sean McQuillan [G]

12/05/2020, 1:28 AM
calling onConnected is this composables request to modify that state (it's an event)
l

Lilly

12/05/2020, 1:29 AM
True
s

Sean McQuillan [G]

12/05/2020, 1:29 AM
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

12/05/2020, 1:32 AM
ok got you
s

Sean McQuillan [G]

12/05/2020, 1:33 AM
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

12/05/2020, 1:35 AM
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
s

Sean McQuillan [G]

12/05/2020, 1:54 AM
So the desired result is that calling the event handler (
viewModel.connect
in the code you pasted) eventually calls 
onConnected
l

Lilly

12/05/2020, 1:54 AM
Thats the VM part:
Copy code
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
            }
        }
    }
s

Sean McQuillan [G]

12/05/2020, 1:55 AM
ah perfect
l

Lilly

12/05/2020, 1:55 AM
Sorry for the inconvenience 🙈
s

Sean McQuillan [G]

12/05/2020, 1:55 AM
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

12/05/2020, 1:58 AM
I have updated my code snippet for the missing 
_state.value
part. 
onConnected
is this:
Copy code
@Composable
fun DcInternEntryPoint(defaultRouting: Routing) {
    Router(defaultRouting) { backStack ->
        when (backStack.last()) {
            is Routing.Scanner -> {
                ScannerScreen(onConnected =  {
                    backStack.push(Routing.Test)
                })
            }
        }
    }
}
s

Sean McQuillan [G]

12/05/2020, 1:59 AM
This looks correct
So now everything should be working except the header state?
l

Lilly

12/05/2020, 2:00 AM
I have really problems with your terms ^^ What do you mean with header state. And I'm also not aware of an UDF diagram
s

Sean McQuillan [G]

12/05/2020, 2:00 AM
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

12/05/2020, 2:13 AM
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:
Copy code
ScannerScreen(onConnected = {
		backStack.push(Routing.Test)
	})
s

Sean McQuillan [G]

12/05/2020, 2:28 AM
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

12/05/2020, 3:22 AM
@Sean McQuillan [G] I refactored screen implementation and cleaned up the code. So thats what I have so far for the VM:
Copy code
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
UI
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?
s

Sean McQuillan [G]

12/07/2020, 4:57 PM
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

12/07/2020, 6:28 PM
@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?
s

Sean McQuillan [G]

12/07/2020, 8:10 PM
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

12/07/2020, 8:51 PM
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
s

Sean McQuillan [G]

12/09/2020, 6:12 AM
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
👍 1
l

Lilly

12/10/2020, 10:57 AM
@Sean McQuillan [G] Thanks for your answer
2 Views
Powered by