4/4 asDialogHolderWithContent, ScreenViewHolder.startShowing
#1031
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
setContent to asDialogHolderWithContent, turnOffChromesetContent to asDialogHolderWithContent, turnOffChrome
rjrjr
force-pushed
the
ray/4-better-method-name-and-optional-chrome-wrangling
branch
from
3b0b714 to
ccc0bb0
Compare
steve-the-edwards
approved these changes
Jun 16, 2023
.../core-android/src/main/java/com/squareup/workflow1/ui/container/AsDialogHolderWithContent.kt
Show resolved
Hide resolved
.../core-android/src/main/java/com/squareup/workflow1/ui/container/AsDialogHolderWithContent.kt
Outdated
Show resolved
Hide resolved
|
Btw - I don't remember seeing naming update in the deprecation warning strings. Need to add that. |
rjrjr
force-pushed
the
ray/3-new-back-handler
branch
from
bb48afd to
af3caaf
Compare
rjrjr
force-pushed
the
ray/4-better-method-name-and-optional-chrome-wrangling
branch
from
c8d713d to
10b1c37
Compare
rjrjr
changed the title
setContent to asDialogHolderWithContent, turnOffChromeasDialogHolderWithContent, ScreenViewHolder.startShowing
rjrjr
force-pushed
the
ray/4-better-method-name-and-optional-chrome-wrangling
branch
2 times, most recently
from
6568f63 to
eebc089
Compare
|
@steve-the-edwards Good morning! I've smoked and canaried the heck out of this, and it's ready to go. The three upstream PRs haven't changed since you reviewed them, but this one could definitely use a new review. Be sure to check ignore whitespace! |
rjrjr
force-pushed
the
ray/3-new-back-handler
branch
from
af3caaf to
eb3de9c
Compare
rjrjr
force-pushed
the
ray/4-better-method-name-and-optional-chrome-wrangling
branch
from
eebc089 to
f686c15
Compare
steve-the-edwards
approved these changes
Jun 26, 2023
workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/ScreenViewFactory.kt
Outdated
Show resolved
Hide resolved
...ow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/OverlayDialogFactory.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+31
to
+37
| * +----+=============+------+ | ||
| * | My| | | | ||
| * | | MyEditModal | | | ||
| * | | | | | ||
| * +----+=============+------+ | ||
| * | MyTutorialScreen | | ||
| * +-------------------------+ |
There was a problem hiding this comment.
😮 . ASCII diagram skills. 🔥
Comment on lines
+64
to
+69
| * Whatever structure you settle on for your root rendering, it is important | ||
| * to render the same structure every time. If your app will ever want to show | ||
| * an [Overlay], it should always render [BodyAndOverlaysScreen], even when | ||
| * there is no [Overlay] to show. Otherwise your entire view tree will be rebuilt, | ||
| * since the view built for a `MyBodyAndBottomBarScreen` cannot be updated to show | ||
| * a [BodyAndOverlaysScreen] rendering. |
There was a problem hiding this comment.
Nice recent addition :)
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/4-better-method-name-and-optional-chrome-wrangling
branch
from
f686c15 to
834da85
Compare
Base automatically changed from
ray/3-new-back-handler
to
ray/1-component-set-dialog
June 26, 2023 21:41
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.
Hide whitespace while reviewing this, especially in
ScreenViewFactory.kt.setContentas a method name gave no clue that we were returning a new manager object, so now it's calledasDialogHolderWithContentHard coding the call to
Dialog.setContentinterferes with code that needs to make that call itself. We now accept asetContent: (ScreenViewHolder<C>) -> Unitlambda to do that work, which defaults to the current behavior.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 AndroidXfindViewTreeOnBackPressedDispatcherOwner()call works immediately. This was always the original intention, but never actually found a use case until now.Moves most of that default behavior to a public
Dialog.fixBackgroundAndDimmingextension function to keep it available and more self documenting (hard learned lessons in there). Pretty similar to howScreenOverlayDialogFactoryacts.Big kdoc update