-
Notifications
You must be signed in to change notification settings - Fork 101
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
Simpler, better back button handling. #843
Simpler, better back button handling. #843
Conversation
9b676fb
to
7ad3116
Compare
7ad3116
to
b5b1f00
Compare
b5b1f00
to
76422f5
Compare
76422f5
to
3c394cf
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.
I'm not sure I've gotten to coming around to how this is simpler, but I trust you and I think I need to do a more systematic learning of the modal/dialog/overlay handling anyway.
I do have a few questions about this particular PR though - noted in comments.
workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/DialogSession.kt
Show resolved
Hide resolved
workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/DialogSession.kt
Outdated
Show resolved
Hide resolved
...w-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/LayeredDialogSessions.kt
Show resolved
Hide resolved
* work, and provides default bindings for [AlertOverlay] and [FullScreenOverlay]. | ||
* | ||
* Here is how this hook could be used to provide a custom dialog to handle [FullScreenOverlay]: |
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.
This example is no longer applicable because of back press handling?
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.
It was already inaccurate, and I'm kind of exhausted, TBH.
workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/DialogSession.kt
Outdated
Show resolved
Hide resolved
workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/DialogSession.kt
Show resolved
Hide resolved
Less complex, anyway. The old scheme had a strategy object in the ViewEnvironment that was providing a one-size-fits-all-dialogs scheme for back button handling, and that scheme was only used by the ScreenViewOverlay subtype, not for Overlay in general. It also was applied only for modals, which was just wrong. With this change, the OverlayDialogHolder that wraps the Dialog is directly responsible for defining its back button behavior; the bizarre subtle I'll add this to the commit message. |
3c394cf
to
2120968
Compare
`OverlayDialogHolder` now can provide an `onBackPressed: () -> Unit` function to be called in place of `Dialog.onBackPressed`, and we provide a default implementation that hooks in `OnBackPressedDispatcher`. This works for all kinds of dialogs, not just modal ones, and replaces `ModalScreenOverlayBackButtonHelper`. The other method that was on `ModalScreenOverlayBackButtonHelper`, which put the no-op `backPressedHandler` on the content view to ensure those on layer layers would not fire, is now built into the default implementation of `ScreenOverlayDialogFactory.buildDialogWithContent`, which is designed to be customized anyway. Also locks down `AlertOverlayDialogFactory`, but moves all the interesting parts into a public function. Should be easier and safer for re-use. So how is this simpler? Or at least less complex? The old scheme had: - a strategy object in the `ViewEnvironment`, `ModalScreenOverlayBackButtonHelper`, that was providing a one-size-fits-all-dialogs scheme for back button handling - that scheme was only used by the `ScreenViewOverlay` subtype, not for `Overlay` in general - and it was applied only for modals, which was very wrong With this change: - The individual `OverlayDialogHolder` that wraps a `Dialog` is directly responsible for defining its back button behavior, and can be changed from the default behavior directly by the feature developer - The bizarre subtle `onBackPressed = {}` hack is moved to the factory class responsible for building the dialog, again in a spot that is naturally open for customization - and we eliminate the boolean return value, meaning one less decision point for developers
2120968
to
ba43dab
Compare
OverlayDialogHolder
now can provide anonBackPressed: () -> Unit
function to be called in place ofDialog.onBackPressed
, and we provide a default implementation that hooks inOnBackPressedDispatcher
. This works for all kinds of dialogs, not just modal ones, and replacesModalScreenOverlayBackButtonHelper
. The other method that was onModalScreenOverlayBackButtonHelper
, which put the no-opbackPressedHandler
on the content view to ensure those on layer layers would not fire, is now built into the default implementation ofScreenOverlayDialogFactory.buildDialogWithContent
, which is designed to be customized anyway.Also locks down
AlertOverlayDialogFactory
, but moves all the interesting parts into a public function. Should be easier and safer for re-use.So how is this simpler? Or at least less complex?
The old scheme had:
ViewEnvironment
,ModalScreenOverlayBackButtonHelper
, that was providing a one-size-fits-all-dialogs scheme for back button handlingScreenViewOverlay
subtype, not forOverlay
in generalWith this change:
OverlayDialogHolder
that wraps aDialog
is directly responsible for defining its back button behavior, and can be changed from the default behavior directly by the feature developeronBackPressed = {}
hack is moved to the factory class responsible for building the dialog, again in a spot that is naturally open for customization