1/4 fun ComponentDialog.setContent() replaces ScreenOverlayDialogFactory
#1026
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
rjrjr
changed the title
fun ComponentDialog.setContent() replaces ScreenOverlayDialogFactoryfun ComponentDialog.setContent() replaces ScreenOverlayDialogFactory
rjrjr
changed the title
fun ComponentDialog.setContent() replaces ScreenOverlayDialogFactoryfun ComponentDialog.setContent() replaces ScreenOverlayDialogFactory
rjrjr
commented
Jun 14, 2023
workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/AndroidOverlay.kt
Outdated
Show resolved
Hide resolved
...ui/core-android/src/main/java/com/squareup/workflow1/ui/container/ContentDialogSetContent.kt
Outdated
Show resolved
Hide resolved
...-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/FullScreenModalFactory.kt
Show resolved
Hide resolved
...ow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/OverlayDialogFactory.kt
Outdated
Show resolved
Hide resolved
...low-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/OverlayDialogHolder.kt
Outdated
Show resolved
Hide resolved
...ui/core-android/src/main/java/com/squareup/workflow1/ui/container/RealOverlayDialogHolder.kt
Show resolved
Hide resolved
rjrjr
force-pushed
the
ray/1-component-set-dialog
branch
from
8ff77ce to
7154b36
Compare
|
LeakCanary failures. 😬 |
Oh -- actually espresso failures. Pretty sure fixing those will also fix the leaks. |
rjrjr
force-pushed
the
ray/1-component-set-dialog
branch
from
7154b36 to
7f258e5
Compare
helios175
reviewed
Jun 15, 2023
...ui/core-android/src/main/java/com/squareup/workflow1/ui/container/ContentDialogSetContent.kt
Outdated
Show resolved
Hide resolved
helios175
reviewed
Jun 15, 2023
...ose/src/androidTest/java/com/squareup/workflow1/ui/compose/ComposeViewTreeIntegrationTest.kt
Outdated
Show resolved
Hide resolved
helios175
reviewed
Jun 15, 2023
...-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/FullScreenModalFactory.kt
Outdated
Show resolved
Hide resolved
helios175
approved these changes
Jun 15, 2023
rjrjr
force-pushed
the
ray/1-component-set-dialog
branch
from
7f258e5 to
e4f4fe1
Compare
rjrjr
force-pushed
the
ray/1-component-set-dialog
branch
from
e4f4fe1 to
8d1228d
Compare
|
This loses the crucial call to |
rjrjr
force-pushed
the
ray/1-component-set-dialog
branch
from
8d1228d to
5973c5b
Compare
rjrjr
changed the title
fun ComponentDialog.setContent() replaces ScreenOverlayDialogFactoryfun ComponentDialog.setContent() replaces ScreenOverlayDialogFactory
Fixed. |
steve-the-edwards
approved these changes
Jun 16, 2023
...iners/android/src/main/java/com/squareup/sample/container/panel/PanelOverlayDialogFactory.kt
Show resolved
Hide resolved
.../core-android/src/main/java/com/squareup/workflow1/ui/container/ComponentDialogSetContent.kt
Outdated
Show resolved
Hide resolved
...-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/FullScreenModalFactory.kt
Outdated
Show resolved
Hide resolved
...core-android/src/main/java/com/squareup/workflow1/ui/container/ScreenOverlayDialogFactory.kt
Outdated
Show resolved
Hide resolved
.../core-android/src/main/java/com/squareup/workflow1/ui/container/ComponentDialogSetContent.kt
Outdated
Show resolved
Hide resolved
rjrjr
force-pushed
the
ray/1-component-set-dialog
branch
2 times, most recently
from
a9e7dda to
6f4712f
Compare
With AndroidX 1.6.0, `ComponentDialog` serves as its own `LifecycleOwner` and `OnBackPressedDispatcherOwner`. To take advantage of this we introduce a new `ComponentDialog.setContent` extension function, deprecate `ScreenOverlayDialogFactory`, and deprecate our hooks for customizing `Dialog` back press handling. Related kdoc is improved, and a factory function is bound to `OverlayDialogFactory.Companion`.
`fun View.findViewTreeOnBackPressedDispatcherOwner()` is AndroidX's preferred entry point to the exciting new world of `OnBackPressedDispatcherOwner`. With this PR we support it explicitly, in particular taking care to ensure that it can be called by newly constructed views before they have been attached to a parent. That is, we take care to make eager calls `View.setViewTreeSavedStateRegistryOwner` on every view we build. We accomplish this mainly by riding the rails previously laid down via `WorkflowLifecycleOwner.installOn`, which now requires an `OnBackPressedDispatcherOwner` parameter. (This is the method used by `WorkflowViewStub` _et al_ to ensure that we're managing the JetPack Lifecycle correctly, and `OnBackPressedDispatcherOwner` is just another piece of that puzzle.) In aid of that, we introduce a new `ViewEnvironmentKey`, `OnBackPressedDispatcherOwnerKey`. It is initialized by `WorkflowLayout`, our new `ComponentDialog.setContent` extension, and `@Composable fun WorkflowRendering()`. This key is not intended for use by feature code, it's more of an implementation detail that has to stay public to allow custom containers to be built. It ensures that `WorkflowViewStub` and friends will have access to the correct `OnBackPressedDispatcherOwner` before they have access to a parent view. TODO: add tests of `@Composable fun BackHandler()`, in both activity and dialog windows.
`val View.backPressedHandler` is overly complicated and kind of naive. The replacement, `fun View.setBackHandler()`, carefully echoes the API and behavior of [`@Composable fun BackHandler()`](https://developer.android.com/reference/kotlin/androidx/activity/compose/package-summary#BackHandler(kotlin.Boolean,kotlin.Function0)). We also provide a single argument overload that take a nullable handler function, and sets `enabled` to true for non-`null` handlers, false for `null`. The biggest change in implementation is that we now use the preferred `OnBackPressedDispatcher.addCallback(LifecycleOwner, OnBackPressedCallback)` overload, which should allow AndroidX to do all the lifecycle bookkeeping for us -- taking advantage of all the hard work we've done to make `WorkflowLifecycleOwner` behave correctly. This work revealed a problem where there was no `WorkflowLifecycleOwner` in place in time for the first update of the content view in `ScreenOverlayDialogFactory`, which prevented us from repeating that mistake in the new `ComponentDialog.setContent()` extension function introduced two PRs up. So that's nice.
1. `setContent` as a method name gave no clue that we were returning a new manager object, so now it's called `asDialogHolderWithContent` 2. Hard coding the call to `Dialog.setContent` interferes with code that needs to make that call itself. We now accept a `setContent: (ScreenViewHolder<C>) -> Unit` lambda to do that work, which defaults to the current behavior. 3. Decouple building and starting views. This allows us to ensure that a Dialog's content view is attached to its window before its first rendering -- before the first call to `ScreenViewHolder.show`. Necessary to ensure that the strictly view-tree-based AndroidX `findViewTreeOnBackPressedDispatcherOwner()` call works immediately. This was always the original intention, but never actually found a use case until now. 4. Moves most of that default behavior to a public `Dialog.fixBackgroundAndDimming` extension function to keep it available and more self documenting (hard learned lessons in there). Pretty similar to how `ScreenOverlayDialogFactory` acts. 5. Big kdoc update
rjrjr
force-pushed
the
ray/1-component-set-dialog
branch
from
408fafd to
0f909ea
Compare
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.
With AndroidX 1.6.0,
ComponentDialogserves as its ownLifecycleOwnerandOnBackPressedDispatcherOwner. To take advantage of this we introduce a newComponentDialog.setContentextension function, deprecateScreenOverlayDialogFactory, and deprecate our hooks for customizingDialogback press handling.Related kdoc is improved, and a factory function is bound to
OverlayDialogFactory.Companion.Update: this PR also collects three downstream ones, in prepration for merging them all into
main:View.findViewTreeOnBackPressedDispatcherOwner. #1027fun View.setBackHandler()replacesval View.backPressedHandler#1028asDialogHolderWithContent,ScreenViewHolder.startShowing#1031