leonard-palm / compose-state-events

A new way to implement One-Time-UI-Events (former SingleLiveEvent) in a Compose world.
Apache License 2.0
177 stars 13 forks source link

Action for EventEffect or NavigationEffect is called twice per trigger. #8

Closed TommyVisic closed 9 months ago

TommyVisic commented 10 months ago

Hi. Thank you for this library! Noticed a possible bug where the action for an EventEffect or NavigationEventEffect is called twice when triggered. This happens when the onConsumed parameter is set with a lambda. I think this is a valid way to use the effects, but I'm not sure. Can you let me know your thoughts? Thanks again

val screenState by viewModel.screenState.collectAsStateWithLifecycle()

// The action for this effect is called twice per trigger.
NavigationEventEffect(
    event = screenState.closeEvent,
    onConsumed = { viewModel.onConsumeCloseEvent() }
) {
    navController.popBackStack()
}

// The action for this version is called just once as expected.
NavigationEventEffect(
    event = screenState.closeEvent,
    onConsumed = viewModel::onConsumeCloseEvent
) {
    navController.popBackStack()
}
// View Model
private val _screenState = MutableStateFlow(initialState)
val screenState = _screenState.asStateFlow()

...

fun onCloseClick() {
    _screenState.update { it.copy(closeEvent = triggered) }
}

fun onConsumeCloseEvent() {
    _screenState.update { it.copy(closeEvent = consumed) }
}

A possible fix for this is to remove the onConsumed parameter from the keys on the LaunchedEffect within NavigationEventEffect and EventEffect.

leonard-palm commented 10 months ago

Hi @TommyVisic, unfortunately I was not able reproduce the described behaviour. Could you please send an example project where the bug happens? I would be really interested looking into that behaviour.

Setting the onConsumed as a reference or as a lambda definately makes a difference in terms of recomposition. But the library needs to be save to use even with a lambda I think.

Btw: Was the code snippet above just a random example? I think you dont need to trigger this close event via the views state if it is just a direct reaction to a users button click.

TommyVisic commented 10 months ago

StateEventExampleApp1.zip

Here's a sample that reproduces the double event. It looks like the key is 1) observe a MutableStateFlow and 2) call navController.popBackstack in the StateEvent action.

Yes, this is pretty much a random example. But in our case, we do indeed involve the view model in the close event because we have to do some work before we want to actually close the screen. Basically it's a "save and close" kind of action.

Thanks for your response!

leonard-palm commented 9 months ago

@TommyVisic Thank your for your example project. I was able to reproduce your described behaviour. Looking into it.

leonard-palm commented 9 months ago

Fixed in release 2.2.0