Hey friends. I have a bit of a weird issue going o...

# compose-android

[Tim Malseed]

Hey friends. I have a bit of a weird issue going on with compose navigation. It’s a little bit contrived, but I’m trying to get to the bottom of it

[Tim Malseed]

Let’s say we have a composable, Screen A, that observes ViewState (a state flow)

[Tim Malseed]

sealed class ViewState {
    object ShowScreenB : ViewState()
    object ShowScreenC : ViewState()
}

@Composable
fun ScreenA(viewState: ViewState, onNavigateToScreenB: () -> Unit) {

    when (viewState) {
        is ShowScreenB -> {
            LaunchedEffect(viewState) {
                onNavigateToScreenB()
            }
        }
        is ShowScreenC -> {
            LaunchedEffect(viewState) {
                onNavigateToScreenC()
            }
        }
    }
}

@Composable
fun ScreenB(viewState: ViewState) {

    when (viewState) {
        is ShowScreenC -> {
            LaunchedEffect(viewState) {
                popBackStack()
            }
        }
    }
}

[Tim Malseed]

(1) If we’re on Screen A, and ViewState changes to ShowScreenB, we navigate to Screen B.

(2) If we’re on Screen A, and the ViewState changes to ShowScreenC, we navigate to Screen C.

(3) If we’re on Screen B, and ViewState changes to ShowScreenC, we navigate back (and ideally, (2) will occur)

[Tim Malseed]

There’s a subtle issue that can occur, if the ViewState emits new values while we’re transitioning from Screen A to Screen B.

So, we’re on Screen A, and the viewState changes to ShowScreenB, and we begin navigating to Screen B, but then the viewState quickly updates to ShowScreenC, Screen A might still attempt to navigate to Screen C (even though it’s currently navigating to Screen B). Then, Screen B tries to pop the backstack, and actually pops Screen C off the stack, and we end up back at Screen B.

[Tim Malseed]

Now, there is a possible solution for this. We can make sure that navigation events only occur if the originating screen is in the ‘resumed’ state:

private fun NavBackStackEntry.lifecycleIsResumed() =
    this.lifecycle.currentState == Lifecycle.State.RESUMED

private fun NavController.navigateSafely(from: NavBackStackEntry, route: String) {
    if (from.lifecycleIsResumed()) {
        navigate(route)
    } else {
        // Don't navigate
    }
}

[Tim Malseed]

If we use this approach, navigation event (1) will occur, but navigation event (2) will not - as Screen A is not ‘resumed’ at this time.

[Tim Malseed]

This works, but it introduces yet another issue.

We start at Screen A, navigate to Screen B. Correctly ignore navigation to Screen C. Screen B now pops the backstack, bringing us back to Screen A.

Now, the ViewState is ShowScreenC, but we’ve already launched the LaunchedEffect for ShowScreenC, and ignored that navigation event. So now we’re stuck on Screen A!

[Ian Lake]

Did you read https://medium.com/androiddevelopers/viewmodel-one-off-event-antipatterns-16a1da869b95

[Tim Malseed]

I was reading this yesterday. The thing is, it is ‘state’ that I am using to decide whether or not to navigate.

[Tim Malseed]

The tl;dr is:

I’m using a launched effect to navigate when state changes. But, if there’s already a navigation event being handled, then the launched effect is ignored. When I return to the original screen, it thinks I’ve already handled the current state, because the launched effect fired (and its effect was ignored)

[Tim Malseed]

I wish I could be more concise!

[Ian Lake]

Your navigateSafety is the whole problem. Your entire handling of the event needs to be in that if statement

[Ian Lake]

That means you don't mark the event as handled until you actually navigate

[Ian Lake]

(that's the whole reason we don't have a navigateSafety in the first place - your entire handling, including your business logic of analytics, marking event as processed, etc. all need to be idempotent and in that same if block)

[Ian Lake]

And no, having a UI state of 'ShowScreenB' isn't state, that's an event. A state would be 'ShowItemDetail' or something with an actual business case. What you do when that state is true (show a side panel on a larger screen or use AnimatedVisibility to expand something inline or show a dialog) is the whole separation between state and UI

[Tim Malseed]

The name showScreenB was just meant to help make this contrived example easier to understand. In my real life example, it is a ‘state’ of the application (like Unauthenticated), which causes the login screen to be displayed

[Tim Malseed]

I can see that the issued is navigateSafely doesn’t do anything in the ‘else’ block - if we can’t navigate, nothing happens and the event goes unhandled.

[Tim Malseed]

The trouble is, I’m relying on the mechanics of LaunchedEffect to ensure that a navigation event only occurs once per state change. But, in certain cases I’m intentionally not handling that navigation event.

[Tim Malseed]

Perhaps navigateSafely could return a boolean to indicate whether navigation was handled, that boolean could be held as some state, and the launched effect could be keyed against that state, and then conditionally fire only if ‘handled’ is false

[Tim Malseed]

Gah! It seems like the LaunchedEffect(key, key2, ..) docs specifically mention not to use a mutable state as a key

This function should not be used to (re-)launch ongoing tasks in response to callback events by way of storing callback data in MutableState passed to key1.

[Stylianos Gakis]

Everything to do with navigation + state + navigating safely and so on would be insanely simpler if the NavController would simply be passed to the ViewModel. Then when “the thing” happens, you call navigate once in there without changing any transient state which you need to then clear, and it’d all be good. Also wouldn’t need to be careful about not clearing this state, and popping the backstack resulting in infinitely navigating forward again.

[curioustechizen]

I use a ScreenRouter abstraction that is injected into the ViewModel for the exact reason that @Stylianos Gakis mentioned above. The implementation of this abstraction uses NavController under the hood.

(I believe I have some mistakes in the way the the implementation of the ScreenRouter gains access to the NavController, but that's besides the point)

[Tim Malseed]

I feel like a ViewModel serves a particular view, but navigation lives at a higher level, so I’ve always tried to avoid interacting with navigation from the VM

[curioustechizen]

Fair enough.

You could treat the ScreenRouter abstraction as the thing that lives at a higher level though. I guess it also depends on how you structure the API of this ScreenRouter.

[Stylianos Gakis]

The only reason I am not doing it is because I am not confident that I would be able to provide it safely to the VM in a safe way, regarding what happens after configuration changes, process death and if in those scenarios I will be referencing the wrong NavController or something like that. Other than that I really don’t believe it’s the wrong layer to call your navigation from. The ViewModel contains all the business logic, of calling the right other repositories and so on for a screen, and I consider making a decision like “This network request succeeded, therefore now I want to go to the next step of this flow, so I will navigate to it” a valid decision for the VM to make. I really don’t quite see the benefit in many cases of the VM instead saying “This network request succeeded, here’s the next step you need to take, but take it as an object containing the data of it, so you can navigate for me and then tell me that you did it”.

[curioustechizen]

For example, my app is a companion app for a BLE peripheral. For some operations, the user is required to be connected to the peripheral.

When the user presses a button, the ViewModel has a check similar to if (isConnected) goToActualScreen() else goToConnectionScreen(). I feel this logic belongs in the ViewModel (otherwise your higher level navigation component will need to have access to the entire app state).

How the goToActualScreen() and goToConnectionScreen() are implemented - that the ViewModel is not concerned with. That's the job of the navigation implementation, and it does have access to the navigation state as opposed to entire app state to do its job.

[curioustechizen]

I am not confident that I would be able to provide it safely to the VM in a safe way

Yes this can be tricky indeed - and this is at the core of the mistakes I alluded to above. I've seen some example apps on GitHub that achieve this better than how I do it but I haven't had the time to experiment with it.

[Oleksandr Balan]

We do use one of (modified) anti-patterns which works pretty well in our case 🙈

We have buffered channel & emit events with non-suspend trySend call. On the Compose UI part we simply collect effects flow and call lambda, which then invokes appropriate navigation call.

private val _effects: Channel<Effect> = Channel(Channel.BUFFERED)

val effects: Flow<Effect> = _effects.receiveAsFlow()

fun saveIntroShown() {
    viewModelScope.launch(<http://dispatchers.io|dispatchers.io>) {
        preferences.featureIntroShown = true

        _effects.trySend(Effect.CloseIntro)
    }
}

sealed interface Effect {
    object CloseIntro : Effect
}

[Pablichjenkov]

Design a state machine per screen, events transform the state and you consume that state from the composable screen. Usually it is more than a state machine, is a nested state machine.
The design based on 'events' is not good for compose for the reason linked in the post above.
An MVI/redux/store Library can help with the design.

[Colton Idle]

Seems like a lot of people agree with @Stylianos Gakis suggesstion of passing the navController to VM.

I wonder why that pattern isn't reccomendded?

[Pablichjenkov]

I believe it is because if you reference a navCont from an Activity bound VM, then on Activity recreation it could be leaked.

[curioustechizen]

I've found that in activity/fragment bound VMs you don't need to "pass" the NC to the VMs. Your implementation of the ScreenRouter can use findNavController. So the need to pass an NC to the VM only exists when you use Compose + NC

[Ian Lake]

Anything you can findNavController on is also unsafe to pass to a VM - they all contain references back to the activity

[curioustechizen]

My point is you don't need to pass it to the VM. You can call findNavController just in time when you need to navigate.