adrielcafe / voyager

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

OnBackPress not working properly #255

Open dhng22 opened 12 months ago

dhng22 commented 12 months ago

This issue was stated in #154, i'm also facing the same issue, where override onBackPress when there's only one Screen inside Navigator will has no effect

DevSrSouza commented 8 months ago

I have tested on the Voyager samples, and the back press closes the app when the onBackPressed = { true }, can you share more examples of your issue?

DevSrSouza commented 8 months ago

Sorry, I previously did understand your use case, now I do. The idea is that when there is only on Screen on the Stack, you will never be notified on the onBackPressed.

This design is a side effect caused because we want, where there is only one Screen on the stack, fallback to the System back press callback (for example, closing the app)

dhng22 commented 8 months ago

@DevSrSouza Any reason for that inconsistency? All i want is to close the app's drawer when user go back, but my app only have one activity. Application with single activity is common now with Compose. So Voyager's BackHandler feature is literally useless with any application that has a single activity. I dont think this is a good idea since this "side effect" is not consistent. And instead of

the most cases, you want to user to close the app

I can decide whenever system should have the control over BackHandler. Isn't that much more easier than the current design which I can't do anything. Design should be consistent between activities, not disabled when there's one activity in the stack.

DevSrSouza commented 8 months ago

Can you try something like this:

class MyScreenWithDrawer : Screen {

   @Composable
   fun Content() {
        ...
        val coroutineScope = rememberCoroutineScope()
        val scaffoldState = rememberScaffoldState()
        BackHandler(enabled = scaffoldState.drawerState.isOpen) {
           coroutineScope.launch { scaffoldState.drawerState.close() }
        }
}
dhng22 commented 8 months ago

Yes I'm writing code in expect/actual, that works normally. But it's just uncomfortable

DevSrSouza commented 8 months ago

To be clear, Voyager implementation works like this:

BackHandler is only applied when there is more the one Screen in the Stack, if there is only one, then, it is disabled on the system back handler will be executed.

If I recall correctly, the Scaffold API does not hold a BackHandler on it implementation to Close the Drawer, you have to implement by hand.

I can decide whenever system should have the control over BackHandler. Isn't that much more easier than the current design which I can't do anything. Design should be consistent between activities, not disabled when there's one activity in the stack.

Yes, if you want to have your own Custom BackHandler, you can pass the onBackPressed = null and implement your self how will Pop Navigation and how you will handle the Scaffold state, but your use case, I would recommend using a new BackHandler.

Yes I'm writing code in expect/actual, that works normally. But it's just uncomfortable

Yes, I can see how it can be, but I how the Android implementation works for Compose, with Voyager or without Voyager, this will be the same I think.

DevSrSouza commented 8 months ago

I tested here and I was right, without the BackHandler and without Voyager it just close the app.

https://github.com/adrielcafe/voyager/assets/29736164/065a42d6-7fdd-473b-846d-bbb91f0bb172

So, the Voyager onBackPressed API is only for navigation purpose.

But still... Even that your use case is not actually a Voyager problem, is how things works in the Scaffold API, there actually is a use case that has a problem with Voyager onBackPressed, is that, when there is only one screen, the onBackPressed is never called. This would be useful if you want to prompt the user if they are sure they want to close the app for a example, but at same time, it can be implemented by Screen, by using the BackHandler API.

dhng22 commented 8 months ago

I tested here and I was right, without the BackHandler and without Voyager it just close the app.

Well sure thing that it will close the app. I'm building my app from scratch that even my Drawer is neither from the Material API nor the Scaffold API, it's customized, I'm not using the API you mentioned. So i need the control over the BackHandler to know when to close my Drawer.

So, the Voyager onBackPressed API is only for navigation purpose.

Right, It's not a big deal to me, I'm just discussing about the consistency of the BackHandler feature, since it's supported by Voyager but useless to me. Have to write expect/actual really lost the meaning of a multiplatform library and an API that it supports

DevSrSouza commented 8 months ago

How I mention before, the Voyager onBackPressed API is only meant for Navigation used. Is different from the BackHandler API.

This is actually something that I have being look into, Being truly honest with your, is not the first time I have complete disabled Voyager backPressed for doing a more specific use case, maybe provide the BackHandler API as Multiplatform only for Android used can help other uses cases, but I'm not sure this is the right move, because we are providing APIs for Navigation, this can out of the Scope of the library.

DevSrSouza commented 8 months ago

This is why I have blocked #273 , currently onBackPressed API does not cover alot of cases like yours, a new API has to be think here. The currently implementation for the scope of the library, is almost perfect and simple, out of the scope of the library, not much.