Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Root BackHandler possible bug #1580

Open
TepesLucian opened this issue Aug 14, 2024 · 9 comments
Open

Root BackHandler possible bug #1580

TepesLucian opened this issue Aug 14, 2024 · 9 comments
Labels
bug Something isn't working discussion Open discussions!
Milestone

Comments

@TepesLucian
Copy link

TepesLucian commented Aug 14, 2024

Having an issue in the following scenario. The root screen sometimes has a BackHandler like so:

@Composable
fun RootUi(
  state: TestState,
  modifier: Modifier,
) {
  RootContent()
  BackHandler {
    // don't let the user out of the app just yet
  }
}

At some point ScreenA is pushed to the backstack which has the following logic in its presenter:

navigator.apply {
   pop() // or resetRoot(ScreenRoot)
   goTo(ScreenB)
}

Effectively I want ScreenA to pop and to push ScreenB. Tapping android back button in ScreenB calls BackHandler on RootUi from my testing even thought it's not visible on the screen. Removing BackHandler navigation logic works as expected and navigates back to ScreenRoot.

@TepesLucian
Copy link
Author

Managed to write a test case that fails : 0e7467e

@saket
Copy link
Contributor

saket commented Aug 19, 2024

This is such a weird bug. The code enters an infinite loop between the BackHandler calling the navigator and the navigator calling the system back dispatcher. I wish the framework protected us from re-entrant callbacks. For now, is the only way to solve this by calling Activity#finish() directly from onRootPop?

@TepesLucian
Copy link
Author

Tried the workaround but it doesn't seem to fix anything. My issue is not when root ui is present on screen but when ScreenA pops itself and pushes ScreenB. Pressing back in ScreenB should pop to RootUi but instead it calls BackHandler from RootUi even though it is not present on the screen. BackHandler from RootUi should be disabled since its ui is not on the screen. It's probably a race condition due to ScreenA popping itself and immediately pushing ScreenB.

@aschulz90
Copy link
Contributor

If you check the layout inspector, do you see both screens in the hierarchy?
If you use AndroidPredictiveBackNavigationDecoration, then the two topmost screens are part of the layout hierarchy, to be able to peek at it.

@TepesLucian
Copy link
Author

Looked into layout inspector and not seeing the RootUi in there, only once you start swiping back. Not sure why its BackHandler still lives on. I've tried commenting out decoration from NavigableCircuitContent and everything works as expected : navigating back makes ScreenB go to RootUi instead of calling BackHandler in RootUi.

@ZacSweers ZacSweers added bug Something isn't working discussion Open discussions! labels Oct 23, 2024
@ZacSweers
Copy link
Collaborator

Is this only specific to gesture nav?

@TepesLucian
Copy link
Author

Yes. CupertinoGestureNavigationDecoration also seems to have issue.

@Skaldebane
Copy link

Skaldebane commented Nov 7, 2024

Hi, I think I'm facing the same issue?

I'm new to Circuit, just followed the tutorial (multiplatform) with no extras. On Android, after navigating from the root screen to another one, pressing system back doesn't pop it, and instead runs the onRootPop handler, where I exit the app, contradicting what the docs say.

I haven't changed the default decoration or anything. The device I test on is an Android 10 emulator (no predictive back there, nor do I have it enabled in AndroidManifest.xml).

Hope this gets fixed soon!

@Skaldebane
Copy link

Skaldebane commented Nov 8, 2024

I put a println inside the onRootPop and... it seems like it's not being called at all?
It appears like Circuit isn't handling anything, whether I'm on the root screen or a second screen, so the default system handler kicks in? No clue.

Another note: The navigation-specific docs also seem to be outdated, still using a BackHandler before calling rememberCircuitNavigator to handle root pop (that didn't work either, Circuit doesn't take precedence nor handle anything so that BackHandler gets executed). The tutorial uses the updated version though.

I guess I'll just create BackHandlers inside each screen to manually pop the current screen using the navigator for now.

@ZacSweers ZacSweers added this to the 1.0.0 milestone Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discussion Open discussions!
Projects
None yet
Development

No branches or pull requests

5 participants