adrielcafe / voyager

🛸 A pragmatic navigation library for Jetpack Compose
https://voyager.adriel.cafe
MIT License
2.59k stars 138 forks source link

Double click causes crack #90

Open funyin opened 2 years ago

funyin commented 2 years ago

There is a bit of a delay between clicking on an item for navigation and the navigation actually occurring. So when I double-click on a button for navigation, the app crashes.

jollygreenegiant commented 1 year ago

This is by design - the library is designed to only allow unique screen keys on the stack, so to avoid the crash you'll either need to make your screen keys unique (instead of hard coded), or don't allow double clicks on your navigation actions

funyin commented 1 year ago

Hmm, this makes sense but the issue came up because there was some delay in navigation.

I think a good fix would be to reject any new requests to navigate if Voyager is currently processing a navigation request.

Syer10 commented 1 year ago

Thats not really possible, its not Voyager thats processing the navigation request, its Compose. Voyager just tells Compose the last screen in the stack and Compose has the responsibility to display it. If you want to protect from this crash I would suggest doing something like this

val navigator = LocalNavigator.currentOrThrow
lamba = {
    if (YourScreen in navigator.items) return@lamba
    navigator push YourScreen
}
funyin commented 1 year ago

This seems like a great idea but it did not work. I also tried checking if any item in the list contains the key of the screen I am about to push but that also did not work.

Also not that I was doing a right-to-left navigation animation

Kashif-E commented 1 year ago

I have made this function to handle this in voyager navigator, it works fine but occasionally breaks for some reason, haven't been able to find the perfect fix for this but looking for something better than this @DevSrSouza @adrielcafe can better comment on this

  public fun pushX(screen: Screen) {
        if (items.last().key != screen.key && items.last().uniqueScreenKey != screen.uniqueScreenKey) {
            push(screen)
        }
    }
Syer10 commented 1 year ago

uniqueScreenKey is a key generator, it doesn't actually reference anything, you're better off removing it from the if statement. My suggestion would basically use a debounce function, like https://github.com/mmolosay/debounce/ to make sure that clicks only happen once.

Kashif-E commented 1 year ago

Thanks for elaborating @Syer10