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

TargetSdkVersion 34 replaces onBackPressed() with OnBackInvokedCallback #259

Closed
Zhuinden opened this issue May 13, 2022 · 4 comments · Fixed by #271
Closed

TargetSdkVersion 34 replaces onBackPressed() with OnBackInvokedCallback #259

Zhuinden opened this issue May 13, 2022 · 4 comments · Fixed by #271

Comments

@Zhuinden
Copy link
Owner

Zhuinden commented May 13, 2022

With the "advent" of OnBackPressedDispatcher, we already had to do this:

class MainActivity : AppCompatActivity(), SimpleStateChanger.NavigationHandler {
    private val backPressedCallback = object: OnBackPressedCallback(true) {
        override fun handleOnBackPressed() {
            if (!Navigator.onBackPressed(this@MainActivity)) {
                this.remove() // this is the only safe way to invoke onBackPressed while cancelling the execution of this callback
                onBackPressed() // this is the only safe way to invoke onBackPressed while cancelling the execution of this callback
                this@MainActivity.onBackPressedDispatcher.addCallback(this) // this is the only safe way to invoke onBackPressed while cancelling the execution of this callback
            }
        }
    }

    override fun onCreate(savedInstanceState: Bundle?) {
        // ...
        onBackPressedDispatcher.addCallback(backPressedCallback) // this is required for `onBackPressedDispatcher` to work correctly
        // ...
    }

    @Suppress("RedundantModalityModifier")
    override final fun onBackPressed() { // you cannot use `onBackPressed()` if you use `OnBackPressedDispatcher`
        super.onBackPressed() // `OnBackPressedDispatcher` by Google effectively breaks all usages of `onBackPressed()` because they do not respect the original contract of `onBackPressed()`
    }
}

However, now, onBackPressed() is deprecated in Android Tiramisu, replaced with onBackPressedCallback and onBackInvokedCallback, which seem to have the same design problems as OnBackPressedDispatcher: namely, that once you registered to receive a callback and you have received said callback, you can't cancel the request.

Clearly, the people at Google writing this code have never heard of event bubbling, which has worked in most UI frameworks in the past 15+ years. (they did, they just want to animate BACK if it's not handled by the app, so this change actually makes sense...)

However, this means we might need to bite the bullet and replace the current event-bubbling approach of ScopedServices.HandlesBack with tinkering with exposing if a service on top is currently handling back or not.

This is quite a heavy change and will require Android T to come out.

@matejdro
Copy link

It seems like ScopedServices.HandlesBack has now been superseded by the OnBackPressedCallback (any service can just use Google's callback instead of simple stack's one). Maybe it's time to deprecate that functionality and suggest to use the official callback solution instead?

That gets rid of the scoped services issue. For the rest, extra boolean canGoBack() method could be added to the Navigator. Then OnBackInvokedCallback can be implemented by installing a CompletionListener and enabling/disabling callback based on the result of the canGoBack method.

@Zhuinden
Copy link
Owner Author

Zhuinden commented Feb 2, 2023

I have a sincere dislike for the "new" notifications UI in Github. Back in the day, I never had to bulk-process 5 pages of notifications before I get important ones...

Anyways, the official callback alone wouldn't work, as a scoped service lives within the non-config scope. To intercept back in a scoped service, Simple-Stack will need to intercept it and implement the same dispatch behavior in ScopeNodes by default so that if either ScopeNode has an active back handler that wants to handle back, it would get it in "VM layer".

I had been trying to think of a way to introduce a new "back mode" to opt-in into the ahead-of-time model, so that there is a migration path and transition process + the original Back processing made perfect sense until suddenly Google changed the way it works with targetSdkVersion 34.

But I admit, this is getting more and more important to handle. Considering we use Simple-Stack in our apps, it will be done eventually.

@matejdro
Copy link

matejdro commented Feb 3, 2023

Huh, is there a use case where a non-config-scope service would need to intercept back? It seems like these two shouldn't mix.

Zhuinden added a commit that referenced this issue Mar 31, 2023
Zhuinden added a commit that referenced this issue Mar 31, 2023
Zhuinden added a commit that referenced this issue Mar 31, 2023
)

* #259 Add support for Backstack.setBackHandlingModel(AHEAD_OF_TIME)

* #259 Update (most) samples to use android:enableOnBackInvokedCallback

* #259 Update some dependencies in samples

* #259 Update readme and changelog

* #259 Remove accidental "res/res" folder

* #259 formatting
@Zhuinden
Copy link
Owner Author

I actually did it with only "breaking" ScopedServices.HandlesBack and it's opt-in.

Eheheheh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants