How do I avoid multiple fast clicks on the same co...

# compose

[Tgo1014]

How do I avoid multiple fast clicks on the same composable?

[Stylianos Gakis]

What you probably want is for the click to not re-trigger some business logic right?
Either disable the button while that logic is running (maybe not so nice) or just make the logic not do anything while it’s already processing the previous event?
What’s your use case exactly, maybe more details can help find a solution.

[Tgo1014]

It’s navigating to another screen, so if you click fast it can open the screen multiple times. For a normal view I could use debounce, but not sure how can I do with compose

[Stylianos Gakis]

I figured as much, I had the exact same problem yesterday. I simply solved it by changing my navigating method from this:

val goToDetailsScreen: (Long) -> Unit = { id ->
    navController.navigate(Screen.DetailsScreen.createRoute(id))
}

To this:

val goToDetailsScreen: (Long) -> Unit = { id ->
    if (navController.currentDestination?.route != Screen.DetailsScreen.route) {
        navController.navigate(Screen.DetailsScreen.createRoute(id))
    }
}

This is probably the easiest way to fix it without having to handle complex logic of if the thing should be clickable or not and confusing the user tapping it and not getting any feedback from it.

I am not sure if it’s the best way though, I’d love to hear about better options or why this may be non-optimal.

[Tgo1014]

I guess it makes sense, I’ll see if I can reuse this logic 🤔

[Ian Lake]

You don't have to do all of that, just check if the Lifecycle is RESUMED (it'll instantly be changed when you navigate): if (navBackStackEntry.lifecycle.currentState == Lifecycle.State.RESUMED)

[Stylianos Gakis]

Interesting, so you mean change what I wrote above to this?

if (navController.currentBackStackEntry?.lifecycle?.currentState == Lifecycle.State.RESUMED) {
    navController.navigate(Screen.DetailsScreen.createRoute(id))
}

Does this mean that the state is basically not at RESUMED while the process of navigation is happening? (While the new view is animating in for example)

But this would not prevent going to the same destination and opening it on top of itself right? (If this method was called after the navigation animation is over and we are actually on that screen)

[Ian Lake]

During animations (either in or out), you won't be RESUMED, that is correct.

[Ian Lake]

I don't know why you'd have a button navigating to yourself unless you meant to do that

[Stylianos Gakis]

Yeah I was mostly curious about if it’s possible or not, which I guess it is. But anyway, this RESUMED check is interesting, is it documented anywhere as a suggestion to avoid multiple navigations or just something you thought of now to fix this issue?

[Ian Lake]

That's just how Navigation has always worked. The Jetsnack and Owl samples do that check, FWIW: https://github.com/android/compose-samples/blob/49c0477dd877453483d0436fedf56e63030811a9/Jetsnack/app/src/main/java/com/example/jetsnack/ui/JetsnackNavGraph.kt#L62

[Stylianos Gakis]

Awesome, thank you for pointing these out to me!

[Stylianos Gakis]

I don’t know why you’d have a button navigating to yourself unless you meant to do that

Actually there is this case of having destinations in a bottom navigation. In this case clicking on the item again does navigate to the same screen again. Is this expected to be handled manually at the BottomNavigation declaration like this?

onNavigation = { selectedScreen: Screen ->
    if (selectedScreen != currentSelectedScreen) {
        navActions.navigateToTopScreen(selectedScreen)
    }
},

[Stylianos Gakis]

Nevermind, just followed the official documentation on this and did this instead

navController.navigate(item.screen.route) {
    popUpTo(navController.graph.findStartDestination().id) {
        saveState = true
    }
    launchSingleTop = true
    restoreState = true
}

Which also saves the multiple backstacks which is awesome, but launchSingleTop is what I was looking for I guess

[Jason Ankers]

Any reason this simple check couldn’t be part of the navigation library?

[Tgo1014]

@Jason Ankers would be really useful indeed

[Stylianos Gakis]

Because it also has to work in the case where you want to somehow navigate to another screen while you’re in the middle of another navigation (which is a very valid use case IMO). This really feels like a per-case problem to solve, not something for the navigation library to do without asking.

[Jason Ankers]

Then it should be configurable, otherwise users need to wrap almost every navigate call in this ‘safe navigate’ alternative. The 99.99% use case is to navigate safely

[Kevin Worth]

Zombie thread, but… here’s an extension function

fun NavController.navigateSafely(safely: Boolean = true, navigate: NavController.() -> Unit) {
    val state = currentBackStackEntry?.lifecycle?.currentState ?: Lifecycle.State.RESUMED
    if (!safely || state == Lifecycle.State.RESUMED) {
        navigate()
    }
}

// Usage
navController.navigateSafely { navigate("some-route/$someVar") }

🤷

[Stylianos Gakis]

Your NavBackStackEntry has a non-null reference to a lifecycle so you can easily make something like this https://github.com/HedvigInsurance/android/blob/3b331fb73762e6b157dc211c271f397733[…]/main/kotlin/com/hedvig/android/app/navigation/HedvigNavHost.kt

[Stylianos Gakis]

And I do prefer the “safe” one to be the default, and the unsafe as an opt-in for those special places which might need it. Which are a handfull if any in my case at least.

[Kevin Worth]

@Stylianos Gakis I was looking to allow all the overloaded navigate methods (there are about a dozen or so).

[Ian Lake]

Keep in mind that you need to add that if statement around the entire code of your onClick listener - e.g., if you track analytics, do other processing, etc. Having a safe navigate call is really handling it at the wrong level if you actually want something that handles every case. You'd instead want it as a wrapper around your clickable lambda

[Stylianos Gakis]

The risk is just that the analytics events triggers, but the navigation just does not happen at all right?
For nav events themselves, we send nav events as a NavController.OnDestinationChangedListener anyway, like here. But I definitely understand how if someone is doing more analytics events it’d be a problem.
Would it not be quite tricky to make that as a wrapper to the entire click listener? Suddenly your onClick modifier would need to know about the NavBackStackEntry, or at least about the lifecycle of it, even if it’s in a deep composable.
I might be lacking some imagination here, but it does feel tricky.

[Ian Lake]

Keep in mind that LocalLifecycleOwner.current is available at every level and is already set to the NavBackStackEntry

[Ian Lake]

But if you're passing lambdas down your hierarchy, rather than passing the entire NavController, like our docs say you should, then you already have a central place to do that wrapping behavior

[Stylianos Gakis]

Aha right, LocalLifecycleOwner.current would correctly point at the right NavBackStackEntry since Navigation sets all that up for us right?

Yeah so in some place like this, where you got access to the lifecycle and you just received your lambda to navigate, you can I suppose check for lifecycle and navigate + then do the event whatever that would be.
The screen itself still just sees a lambda that it’s triggering, so no special logic there. As long as you hoist the analytics event up there too.

[Kevin Worth]

Ok, ok, so we could throw together a function that would enable something like

SomeItemList(
    ...
    onClickItem: safelyClick<Item>(backStackEntry = currentEntry) {
        navController.navigate("some-route/${it.someProperty}")
    }
)

// Need to fill in "..."
fun <T : Any> safelyClick(safely: Boolean = true, backStackEntry: NavBackStackEntry?, onClick: (foo: T) -> Unit): (foo: T) -> Unit { ... }

…but if we’re going to have to go through all that trouble, then we might as well go with what’s been suggested:

SomeItemList(
    ...
    onClickItem: {
        if (navBackStackEntry.lifecycle.currentState == Lifecycle.State.RESUMED) {
            // do any number of things (maybe stuff like analytics, etc.)
            navController.navigate("some-route/${it.someProperty}")
        }
    }
)

😎

[Stylianos Gakis]

At the level where your SomeItemList is, I’d expect the lambda to just be a simple (SomeProperty) -> Unit, and you can handle all that at the level where your composable screen is setup, no?

[Kevin Worth]

What I had in mind was:

NavHost(navController = navController, startDestination = "some-destination") {
    composable("some-destination") {
        SomeItemList(
            ...
        )
    }
    ...
}

[Ian Lake]

Extracting out the contents of your screen into its own composable method that you can write a preview for, pass fake data to, unit test separately from Navigation, etc. should definitely be on your to-do list, but yes, having it there as you're prototyping makes a lot of sense as a first step 👍