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

Change back handler behavior #273

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Change back handler behavior #273

wants to merge 3 commits into from

Conversation

DevNatan
Copy link
Collaborator

@DevNatan DevNatan commented Dec 10, 2023

Update back handler behavior to work according to docs

Fixes #154
Fixes #255
As well #72

Current behavior

Method How it works
onBackPressed = null stack size = 0: default platform behavior
stack size > 0: default platform behavior
Should pop backstack instead.
BackHandler is not being handled properly, app is being closed.
onBackPressed = { false } stack size = 0: default platform behavior
Back press being supplied means that the developer will take care of that.
According to docs "return false won't pop the current screen" is not actually true
stack size > 0: does nothing
onBackPressed = { true } stack size = 0: default platform behavior
stack size > 0: pop

New behavior

Method How it works
onBackPressed = null stack size = 0: default platform behavior
stack size > 0: pop
onBackPressed = { false } stack size = 0: does nothing
stack size > 0: does nothing
onBackPressed = { true } stack size = 0: default platform behavior
stack size > 0: pop

Tested using sample:multiplatform

@DevNatan DevNatan added the bug Something isn't working label Dec 10, 2023
@DevNatan DevNatan self-assigned this Dec 10, 2023
@DevSrSouza
Copy link
Collaborator

I don't think we should force BackHandler always. There are use cases where you make voyager backHandler nullable and use Compose BackHandler by hand.

Copy link
Collaborator

@DevSrSouza DevSrSouza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking the PR to we investigate a better approach for BackHandler without changing the currecnt behavior.

@dhng22
Copy link

dhng22 commented Feb 1, 2024

Any update for this PR?

@rom4ster
Copy link

rom4ster commented Feb 8, 2024

Why are you blocking a fix to a clear bug. You literally have to spam the back button to get it to work and that is in NO WAY a good user experience. This library is not coded by monkeys (devs overall did a great job) so I am 100% sure the intended behavior is not to have to do this to use the back button so why block a fix? Sure look at better ways but this should be merged in as a stopgap to prevent people from dealing with this honestly garbage back handling.

I get that code is not perfect and everyone makes mistakes. This is natural and normal. What is not normal is when you have a issue as problematic as this, and then are handed a fix on a silver platter, you just stall it for now close to 1.5 months

@rom4ster
Copy link

rom4ster commented Feb 8, 2024

I don't think we should force BackHandler always. There are use cases where you make voyager backHandler nullable and use Compose BackHandler by hand.

This is not a problem created by this PR, this is how it works by default. You cannot hold OP liable for issues that are already in the design of the library. All OP did was modify default behavior. You can re code the current behavior by forcing false and overriding onBackPressed so this should be a non issue.

@Vaorhd
Copy link

Vaorhd commented Feb 16, 2024

Will you update lib ?

@DevSrSouza
Copy link
Collaborator

@DevNatan can you have a change here? Not having any behavior when the BackPressed callback is null?
This is avoid Voyager emit a BackHandler when some use case requires to rewrite the BackHandler with custom approach and set the backPressed to null, for example, this is the actual currently recommendation for accessing navigator on the Back Pressed callback (see #333).

Comment on lines +176 to +179
internal fun popRecursively() {
if (pop()) return
parent?.popRecursively()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this function public annotated with @InternalVoyagerApi?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @DevNatan are you going to fix this ?

// `navigator.size == 1` covers onBackPressed = { false } and empty stack
// because `navigator.canPop` always returns `false` when stack min size is 1
BackHandler(
enabled = (navigator.size == 1 && !onBackPressed(navigator.lastItem)) ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should use the onBackPressed function here. The assumption is that the onBackPressed will be called when is actually poping the navigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OnBackPress not working properly onBackPressed override doesnt work
6 participants