bindShowRendering no longer calls showRendering. #408
Merged
Conversation
This file contains hidden or 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
force-pushed
the
ray/decor-397-396
branch
6 times, most recently
from
e47cb6f to
a301ef5
Compare
rjrjr
commented
Apr 16, 2021
samples/containers/android/src/main/java/com/squareup/sample/container/panel/PanelContainer.kt
Outdated
Show resolved
Hide resolved
rjrjr
commented
Apr 16, 2021
workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/ViewRegistry.kt
Outdated
Show resolved
Hide resolved
|
Haven't started reading the actual PR yet, but just some comments from the description:
Minor implementation inconvenience isn't a great reason to make API changes.
Maybe, but I'd rather keep it locked down until we prove that, so we don't have to deprecated it later. I don't know if we will, either, because |
rjrjr
force-pushed
the
ray/decor-397-396
branch
from
a301ef5 to
e0a3ee4
Compare
rjrjr
commented
Apr 16, 2021
| ): DialogRef<Any> { | ||
| val view = initialViewEnvironment[ViewRegistry] | ||
| .buildView(initialModalRendering, initialViewEnvironment, this) | ||
| .buildView( |
There was a problem hiding this comment.
This change and a couple of others like it are because I deleted a convenience overload of buildView that caused nothing but trouble, and which in practice no one but the library will ever use.
There was a problem hiding this comment.
Yes! Figured that's what was going on, and I've wanted to do that in the past too. Thanks!
rjrjr
force-pushed
the
ray/decor-397-396
branch
3 times, most recently
from
b3b1725 to
a6807d9
Compare
Yeah, it was a bad call. Turns out I didn't even use it. PR is smaller now. |
zach-klippenstein
approved these changes
Apr 16, 2021
samples/containers/android/src/main/java/com/squareup/sample/container/panel/PanelContainer.kt
Outdated
Show resolved
Hide resolved
| ): DialogRef<Any> { | ||
| val view = initialViewEnvironment[ViewRegistry] | ||
| .buildView(initialModalRendering, initialViewEnvironment, this) | ||
| .buildView( |
There was a problem hiding this comment.
Yes! Figured that's what was going on, and I've wanted to do that in the past too. Thanks!
That was always a weird coupling, and it caused the nasty compounding double update behavior in `DecorativeViewFactory`. To prevent `ViewFactory` implementations from having to remember to call `showRendering`, we make `ViewRegistry.buildView` do so. And to allow more sophisticiated `ViewFactory` implementations to prevent that call from happening prematurely, we build it into the default value for a new `initializeView` method on `ViewRegistry.buildView`. `DecorativeViewFactory` takes advantage by passing a no-op function in for `initializeView` when the view is built, and then making its own explicit call to `showRendering` after it's done playing wrapping games. Closes #397
rjrjr
force-pushed
the
ray/decor-397-396
branch
from
a6807d9 to
487faf7
Compare
rjrjr
added a commit
that referenced
this pull request
Dec 9, 2021
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. The `initializeView` lambdas are gone, and so is `View.showFirstRendering`. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for calling a new method, `View.start`, before it makes any calls to `View.showRendering`. `View.start` takes over the `WorkflowLifecycleRunner.installOn` job that was previously done by `WorkflowViewStub` and a few others. Since it is called only after any `ViewFactory` wrapping of the rendering in hand has been peformed, we're certain it will only happen once. Of course we still need the ability to to customize view initialization via wrapping. To accomodate that, `View.start` is implemented by executing another new extension, `internal var View.starter: (View) -> Unit`. Because it is a `var`, `buildView` callers can wrap it before `start` is invoked. We provide a bit of hand holding to those who need this ability to wrap the starter via `DecorativeViewFactory.onStartWrapper`, which replaceds `initializeView`. Where `initializeView` lambdas were expected to make their own call to the static `View.showFirstRendering` function, `onStartWrapper` is passed a function to invoke -- the current contents of `View.starter`. We wrap and replace `var View.starter` for them, and keep it private to the library. This required a pretty thorough overhaul of `ViewShowRendering.kt`, which is renamed `WorkflowUiViewState.kt`. `WorkflowUiViewState` is a sealed class that hangs off of a view tag, with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
rjrjr
added a commit
that referenced
this pull request
Dec 9, 2021
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. The `initializeView` lambdas are gone, and so is `View.showFirstRendering`. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for calling a new method, `View.start`, before it makes any calls to `View.showRendering`. `View.start` takes over the `WorkflowLifecycleRunner.installOn` job that was previously done by `WorkflowViewStub` and a few others. Since it is called only after any `ViewFactory` wrapping of the rendering in hand has been peformed, we're certain it will only happen once. Of course we still need the ability to to customize view initialization via wrapping. To accomodate that, `View.start` is implemented by executing another new extension, `internal var View.starter: (View) -> Unit`. Because it is a `var`, `buildView` callers can wrap it before `start` is invoked. We provide a bit of hand holding to those who need this ability to wrap the starter via `DecorativeViewFactory.onStartWrapper`, which replaceds `initializeView`. Where `initializeView` lambdas were expected to make their own call to the static `View.showFirstRendering` function, `onStartWrapper` is passed a function to invoke -- the current contents of `View.starter`. We wrap and replace `var View.starter` for them, and keep it private to the library. This required a pretty thorough overhaul of `ViewShowRendering.kt`, which is renamed `WorkflowUiViewState.kt`. `WorkflowUiViewState` is a sealed class that hangs off of a view tag, with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
rjrjr
added a commit
that referenced
this pull request
Dec 10, 2021
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that (we thought) by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for immediately calling `View.start` on the new `View`. `View.start` makes the initial `showRendering` call that formerly was the job of `ViewFactory.buildView` -- the factory builds the view, and the container turns the key. Since `View.start` is called only after all wrapped `ViewFactory.buildView` functions have executed, we're certain it will only happen once. Of course we still need the ability to to customize view initialization via wrapping, especially to invoke `WorkflowLifecycleOwner.installOn`. To accomodate that, the function that `View.start` executes can be wrapped via the new `viewStarter` argument to `ViewRegistry.buildView` and `DecorativeViewFactory`, which replaces `initializeView`. This required a pretty thorough overhaul of `ViewShowRendering.kt`, which is renamed `WorkflowUiViewState.kt`. `WorkflowUiViewState` is a sealed class that hangs off of a view tag, with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
rjrjr
added a commit
that referenced
this pull request
Dec 10, 2021
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that (we thought) by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for immediately calling `View.start` on the new `View`. `View.start` makes the initial `showRendering` call that formerly was the job of `ViewFactory.buildView` -- the factory builds the view, and the container turns the key. Since `View.start` is called only after all wrapped `ViewFactory.buildView` functions have executed, we're certain it will only happen once. Of course we still need the ability to customize view initialization via wrapping, especially to invoke `WorkflowLifecycleOwner.installOn`. To accomodate that, the function that `View.start` executes can be wrapped via the new `viewStarter` argument to `ViewRegistry.buildView` and `DecorativeViewFactory`, which replaces `initializeView`. This required a pretty thorough overhaul of `ViewShowRendering.kt`, which is renamed `WorkflowUiViewState.kt`. `WorkflowUiViewState` is a sealed class that hangs off of a view tag, with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
rjrjr
added a commit
that referenced
this pull request
Dec 10, 2021
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that (we thought) by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for immediately calling `View.start` on the new `View`. `View.start` makes the initial `showRendering` call that formerly was the job of `ViewFactory.buildView` -- the factory builds the view, and the container turns the key. Since `View.start` is called only after all wrapped `ViewFactory.buildView` functions have executed, we're certain it will only happen once. Of course we still need the ability to customize view initialization via wrapping, especially to invoke `WorkflowLifecycleOwner.installOn`. To accomodate that, the function that `View.start` executes can be wrapped via the new `viewStarter` argument to `ViewRegistry.buildView` and `DecorativeViewFactory`, which replaces `initializeView`. This required a pretty thorough overhaul of `ViewShowRendering.kt`, which is renamed `WorkflowUiViewState.kt`. `WorkflowUiViewState` is a sealed class that hangs off of a view tag, with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
rjrjr
added a commit
that referenced
this pull request
Dec 10, 2021
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that (we thought) by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for immediately calling `View.start` on the new `View`. `View.start` makes the initial `showRendering` call that formerly was the job of `ViewFactory.buildView` -- the factory builds the view, and the container turns the key. Since `View.start` is called only after all wrapped `ViewFactory.buildView` functions have executed, we're certain it will only happen once. Of course we still need the ability to customize view initialization via wrapping, especially to invoke `WorkflowLifecycleOwner.installOn`. To accomodate that, the function that `View.start` executes can be wrapped via the new `viewStarter` argument to `ViewRegistry.buildView` and `DecorativeViewFactory`, which replaces `initializeView`. This required a pretty thorough overhaul of `ViewShowRendering.kt`, which is renamed `WorkflowUiViewState.kt`. `WorkflowUiViewState` is a sealed class that hangs off of a view tag, with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
rjrjr
added a commit
that referenced
this pull request
Dec 10, 2021
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that (we thought) by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for immediately calling `View.start` on the new `View`. `View.start` makes the initial `showRendering` call that formerly was the job of `ViewFactory.buildView` -- the factory builds the view, and the container turns the key. Since `View.start` is called only after all wrapped `ViewFactory.buildView` functions have executed, we're certain it will only happen once. Of course we still need the ability to customize view initialization via wrapping, especially to invoke `WorkflowLifecycleOwner.installOn`. To accomodate that, the function that `View.start` executes can be wrapped via the new `viewStarter` argument to `ViewRegistry.buildView` and `DecorativeViewFactory`, which replaces `initializeView`. This required a pretty thorough overhaul of `ViewShowRendering.kt`, which is renamed `WorkflowUiViewState.kt`. `WorkflowUiViewState` is a sealed class that hangs off of a view tag, with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
rjrjr
added a commit
that referenced
this pull request
Dec 10, 2021
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that (we thought) by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for immediately calling `View.start` on the new `View`. `View.start` makes the initial `showRendering` call that formerly was the job of `ViewFactory.buildView` -- the factory builds the view, and the container turns the key. Since `View.start` is called only after all wrapped `ViewFactory.buildView` functions have executed, we're certain it will only happen once. Of course we still need the ability to customize view initialization via wrapping, especially to invoke `WorkflowLifecycleOwner.installOn`. To accomodate that, the function that `View.start` executes can be wrapped via the new `viewStarter` argument to `ViewRegistry.buildView` and `DecorativeViewFactory`, which replaces `initializeView`. This required a pretty thorough overhaul of `ViewShowRendering.kt`, which is renamed `WorkflowUiViewState.kt`. `WorkflowUiViewState` is a sealed class that hangs off of a view tag, with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
rjrjr
added a commit
that referenced
this pull request
Dec 10, 2021
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that (we thought) by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for immediately calling `View.start` on the new `View`. `View.start` makes the initial `showRendering` call that formerly was the job of `ViewFactory.buildView` -- the factory builds the view, and the container turns the key. Since `View.start` is called only after all wrapped `ViewFactory.buildView` functions have executed, we're certain it will only happen once. Of course we still need the ability to customize view initialization via wrapping, especially to invoke `WorkflowLifecycleOwner.installOn`. To accomodate that, the function that `View.start` executes can be wrapped via the new `viewStarter` argument to `ViewRegistry.buildView` and `DecorativeViewFactory`, which replaces `initializeView`. This required a pretty thorough overhaul of `ViewShowRendering.kt` The `ViewShowRenderingTag` that it hangs off of a view tag is renamed `WorkflowViewState`, and extracted to a separate file. `WorkflowViewState` is a sealed class with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
rjrjr
added a commit
that referenced
this pull request
Dec 10, 2021
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that (we thought) by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for immediately calling `View.start` on the new `View`. `View.start` makes the initial `showRendering` call that formerly was the job of `ViewFactory.buildView` -- the factory builds the view, and the container turns the key. Since `View.start` is called only after all wrapped `ViewFactory.buildView` functions have executed, we're certain it will only happen once. Of course we still need the ability to customize view initialization via wrapping, especially to invoke `WorkflowLifecycleOwner.installOn`. To accomodate that, the function that `View.start` executes can be wrapped via the new `viewStarter` argument to `ViewRegistry.buildView` and `DecorativeViewFactory`, which replaces `initializeView`. This required a pretty thorough overhaul of `ViewShowRendering.kt` The `ViewShowRenderingTag` that it hangs off of a view tag is renamed `WorkflowViewState`, and extracted to a separate file. `WorkflowViewState` is a sealed class with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
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.
That was always a weird coupling, and it caused the nasty compounding double
update behavior in
DecorativeViewFactory.To prevent
ViewFactoryimplementations from having to remember to callshowRendering, we makeViewRegistry.buildViewdo so. And to allow moresophisticiated
ViewFactoryimplementations to prevent that call fromhappening prematurely, we build it into the default value for a new
initializeViewmethod onViewRegistry.buildView.DecorativeViewFactorytakes advantage by passing a no-op function in forinitializeViewwhen the view is built, and then making its own explicit callto
showRenderingafter it's done playing wrapping games.Closes #397