-
Notifications
You must be signed in to change notification settings - Fork 61
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
BackPressHandler plugin #32
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LachlanMcKee
reviewed
Aug 2, 2022
CherryPerry
commented
Aug 2, 2022
CherryPerry
commented
Aug 2, 2022
SamuPS
reviewed
Aug 2, 2022
CherryPerry
force-pushed
the
androidx-back-integration
branch
from
August 3, 2022 10:25
5ae37da
to
f02b84d
Compare
CherryPerry
force-pushed
the
androidx-back-integration
branch
2 times, most recently
from
August 10, 2022 13:46
86eb800
to
f6621df
Compare
CherryPerry
force-pushed
the
androidx-back-integration
branch
from
August 10, 2022 14:27
f6621df
to
2ae8636
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an example in the Sandbox? I can't figure out how it should be used in a real app/feature. The Android Test does not make it clear how straightforward it is.
core/src/androidTest/java/com/bumble/appyx/core/plugin/BackPressHandlerTest.kt
Show resolved
Hide resolved
CherryPerry
force-pushed
the
androidx-back-integration
branch
from
August 10, 2022 15:56
6c676a7
to
a97a9d0
Compare
LachlanMcKee
approved these changes
Aug 10, 2022
Thanks for updating the example! |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently
BackPressHandler
is not an actual plugin and is manually supported only for a routing source. In this PR I tried to implementBackPressHandler
that is available for any component in a node.Also in this PR i tried to support predictive back gesture feature of Android 13. As you can see on the attached video it works as expected.
Concerns
AndroidX
In this PR I implement
BackPressHandler
as a provider of AndroidXOnBackPressedCallback
. It allows more seamless integration with other AndroidX components but has the following issues:OnBackPressedCallback.isEnabled
is not reactive/observable, so we need to manually synchronise internal state of an owner component with this property likefeature.state.subscribe { callback.isEnabled = state.someCheck }
.StateFlow
in their internal components (like interactors) instead of RxJava for example. But we are already doing it inRoutingSource
.fun onBackPressed()
because we need to declare if we support going back or not first, so AndroidX and Android integration works fine. This change also allowed me to remove one property fromRoutingSource
.Registration order
In the current implementation and in this pull request we are relying on Compose to register
OnBackPressedCallback
. Invocation order ofOnBackPressedCallback
of different nodes in the same parent is defined by the order of Composable functions. If a node is out of a composition, it won't handle back presses. Implementation details is here.Most likely this problem is out of scope of this pull request
Check list
CHANGELOG.md
if required.predictive-support.webm