Replace DecorativeScreenViewFactory with a builder #626
Comments
rjrjr
added a commit
that referenced
this issue
Mar 21, 2022
The legacy workflow-ui world is built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This is overly clever, needlessly throws away a good bit of compile time safety, and makes it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` is required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wraps another, with the intention of delegating to its registered `ViewFactory`, that requires creating a wrapper `ViewFactory` that makes a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which is still pretty tricky to use. To solve all this, we do two things: - deprecate all the existing `View` extension methods and abandon `WorkflowViewState`, replacing it with `ScreenViewHolder` - add an `updateView(View)` method to `ScreenViewFactory` `ScreenViewHolder` is inverted from `WorkflowViewState` -- instead of belonging to the `View`, nestled in a tag, the `View` belongs to it. Most of the old `View` extension methods are replaced by methods on `ScreenViewHolder`. It is very similar to `DialogHolder`, but is part of the public API. The change to `ScreenViewFactory`, giving it an `update` method, makes it much more like `OverlayDialogFactory`. I toyed a bit with the idea of trying to unify `ScreenViewFactory` / `OverlayDialogFactory` and `ScreenViewHolder` / `DialogHolder`, but it didn't seem worth the added complexity, especially around parameter types. One of the deprecated `View` extensions did not move to `ScreenViewHolder`: `View.Start` is replaced by `ScreenViewFactory.start`. This extension method is what calls `ScreenViewFactory.buildView`. It creates and returns a `ScreenViewHolder` that wraps the new view and accept updates for it, which it does by calling `ScreenViewFactory.update`, as you'd expect. `ScreenViewFactory.start` is also the new home for the `ViewStarter` mechanism that is used by `WorkflowViewStub` and other containers to call `WorkflowLifecycleOwner.installOn`. We are not completely out of the `View.setTag` business. `ScreenViewFactory` implementations must be stateless -- it is common for separate instances to field calls to `buildView` and `updateView`, or for a single factory instance to build and update multiple views. `View.setTag` is the most practical way for `buildView` to set up state that `updateView` can rely on. This technique is used as an implementation detail of several standard `ScreenViewFactory` implementations, but is never a public concern. In particular, the factories built by the various `ScreenViewRunner.bind` methods rely on this technique to keep a `View` paired with its runner. And `ScreenViewRunner` is still very much needed under the new scheme, so that we don't force every implementation of `ScreenViewFactory` to solve this problem of maintaining per-view state themselves. But `ScreenViewRunner` is now a convenience, rather than a fundamental mechanism. By making `ScreenViewFactory` responsible for both building and updating, it becomes trivial for one factory to delegate to another -- the update method no longer lives in a fragile `View` tag, and so no longer needs to be replaced. A wrapper factory can just delegate to another one. This means there is no longer a problem to be solved by `DecorativeScreenViewFactory`, so that class is deleted. `ManualScreenViewFactory` is also gone, replaced by a `bindBuiltView()` function that returns `Pair<View, ScreenViewRunner>`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443.
rjrjr
added a commit
that referenced
this issue
Mar 21, 2022
The legacy workflow-ui world is built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This is overly clever, needlessly throws away a good bit of compile time safety, and makes it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` is required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wraps another, with the intention of delegating to its registered `ViewFactory`, that requires creating a wrapper `ViewFactory` that makes a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which is still pretty tricky to use. To solve all this, we do two things: - deprecate all the existing `View` extension methods and abandon `WorkflowViewState`, replacing it with `ScreenViewHolder` - add an `updateView(View)` method to `ScreenViewFactory` `ScreenViewHolder` is inverted from `WorkflowViewState` -- instead of belonging to the `View`, nestled in a tag, the `View` belongs to it. Most of the old `View` extension methods are replaced by methods on `ScreenViewHolder`. It is very similar to `DialogHolder`, but is part of the public API. The change to `ScreenViewFactory`, giving it an `update` method, makes it much more like `OverlayDialogFactory`. I toyed a bit with the idea of trying to unify `ScreenViewFactory` / `OverlayDialogFactory` and `ScreenViewHolder` / `DialogHolder`, but it didn't seem worth the added complexity, especially around parameter types. One of the deprecated `View` extensions did not move to `ScreenViewHolder`: `View.Start` is replaced by `ScreenViewFactory.start`. This extension method is what calls `ScreenViewFactory.buildView`. It creates and returns a `ScreenViewHolder` that wraps the new view and accept updates for it, which it does by calling `ScreenViewFactory.update`, as you'd expect. `ScreenViewFactory.start` is also the new home for the `ViewStarter` mechanism that is used by `WorkflowViewStub` and other containers to call `WorkflowLifecycleOwner.installOn`. We are not completely out of the `View.setTag` business. `ScreenViewFactory` implementations must be stateless -- it is common for separate instances to field calls to `buildView` and `updateView`, or for a single factory instance to build and update multiple views. `View.setTag` is the most practical way for `buildView` to set up state that `updateView` can rely on. This technique is used as an implementation detail of several standard `ScreenViewFactory` implementations, but is never a public concern. In particular, the factories built by the various `ScreenViewRunner.bind` methods rely on this technique to keep a `View` paired with its runner. And `ScreenViewRunner` is still very much needed under the new scheme, so that we don't force every implementation of `ScreenViewFactory` to solve this problem of maintaining per-view state themselves. But `ScreenViewRunner` is now a convenience, rather than a fundamental mechanism. By making `ScreenViewFactory` responsible for both building and updating, it becomes trivial for one factory to delegate to another -- the update method no longer lives in a fragile `View` tag, and so no longer needs to be replaced. A wrapper factory can just delegate to another one. This means there is no longer a problem to be solved by `DecorativeScreenViewFactory`, so that class is deleted. `ManualScreenViewFactory` is also gone, replaced by a `bindBuiltView()` function that returns `Pair<View, ScreenViewRunner>`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443.
rjrjr
added a commit
that referenced
this issue
Mar 21, 2022
The legacy workflow-ui world is built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This is overly clever, needlessly throws away a good bit of compile time safety, and makes it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` is required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wraps another, with the intention of delegating to its registered `ViewFactory`, that requires creating a wrapper `ViewFactory` that makes a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which is still pretty tricky to use. To solve all this, we do two things: - deprecate all the existing `View` extension methods and abandon `WorkflowViewState`, replacing it with `ScreenViewHolder` - add an `updateView(View)` method to `ScreenViewFactory` `ScreenViewHolder` is inverted from `WorkflowViewState` -- instead of belonging to the `View`, nestled in a tag, the `View` belongs to it. Most of the old `View` extension methods are replaced by methods on `ScreenViewHolder`. It is very similar to `DialogHolder`, but is part of the public API. The change to `ScreenViewFactory`, giving it an `update` method, makes it much more like `OverlayDialogFactory`. I toyed a bit with the idea of trying to unify `ScreenViewFactory` / `OverlayDialogFactory` and `ScreenViewHolder` / `DialogHolder`, but it didn't seem worth the added complexity, especially around parameter types. One of the deprecated `View` extensions did not move to `ScreenViewHolder`: `View.Start` is replaced by `ScreenViewFactory.start`. This extension method is what calls `ScreenViewFactory.buildView`. It creates and returns a `ScreenViewHolder` that wraps the new view and accept updates for it, which it does by calling `ScreenViewFactory.update`, as you'd expect. `ScreenViewFactory.start` is also the new home for the `ViewStarter` mechanism that is used by `WorkflowViewStub` and other containers to call `WorkflowLifecycleOwner.installOn`. We are not completely out of the `View.setTag` business. `ScreenViewFactory` implementations must be stateless -- it is common for separate instances to field calls to `buildView` and `updateView`, or for a single factory instance to build and update multiple views. `View.setTag` is the most practical way for `buildView` to set up state that `updateView` can rely on. This technique is used as an implementation detail of several standard `ScreenViewFactory` implementations, but is never a public concern. In particular, the factories built by the various `ScreenViewRunner.bind` methods rely on this technique to keep a `View` paired with its runner. And `ScreenViewRunner` is still very much needed under the new scheme, so that we don't force every implementation of `ScreenViewFactory` to solve this problem of maintaining per-view state themselves. But `ScreenViewRunner` is now a convenience, rather than a fundamental mechanism. By making `ScreenViewFactory` responsible for both building and updating, it becomes trivial for one factory to delegate to another -- the update method no longer lives in a fragile `View` tag, and so no longer needs to be replaced. A wrapper factory can just delegate to another one. This means there is no longer a problem to be solved by `DecorativeScreenViewFactory`, so that class is deleted. `ManualScreenViewFactory` is also gone, replaced by a `bindBuiltView()` function that returns `Pair<View, ScreenViewRunner>`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443.
rjrjr
added a commit
that referenced
this issue
Mar 21, 2022
The legacy workflow-ui world is built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This is overly clever, needlessly throws away a good bit of compile time safety, and makes it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` is required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wraps another, with the intention of delegating to its registered `ViewFactory`, that requires creating a wrapper `ViewFactory` that makes a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which is still pretty tricky to use. To solve all this, we do two things: - deprecate all the existing `View` extension methods and abandon `WorkflowViewState`, replacing it with `ScreenViewHolder` - add an `updateView(View)` method to `ScreenViewFactory` `ScreenViewHolder` is inverted from `WorkflowViewState` -- instead of belonging to the `View`, nestled in a tag, the `View` belongs to it. Most of the old `View` extension methods are replaced by methods on `ScreenViewHolder`. It is very similar to `DialogHolder`, but is part of the public API. The change to `ScreenViewFactory`, giving it an `update` method, makes it much more like `OverlayDialogFactory`. I toyed a bit with the idea of trying to unify `ScreenViewFactory` / `OverlayDialogFactory` and `ScreenViewHolder` / `DialogHolder`, but it didn't seem worth the added complexity, especially around parameter types. One of the deprecated `View` extensions did not move to `ScreenViewHolder`: `View.Start` is replaced by `ScreenViewFactory.start`. This extension method is what calls `ScreenViewFactory.buildView`. It creates and returns a `ScreenViewHolder` that wraps the new view and accept updates for it, which it does by calling `ScreenViewFactory.update`, as you'd expect. `ScreenViewFactory.start` is also the new home for the `ViewStarter` mechanism that is used by `WorkflowViewStub` and other containers to call `WorkflowLifecycleOwner.installOn`. We are not completely out of the `View.setTag` business. `ScreenViewFactory` implementations must be stateless -- it is common for separate instances to field calls to `buildView` and `updateView`, or for a single factory instance to build and update multiple views. `View.setTag` is the most practical way for `buildView` to set up state that `updateView` can rely on. This technique is used as an implementation detail of several standard `ScreenViewFactory` implementations, but is never a public concern. In particular, the factories built by the various `ScreenViewRunner.bind` methods rely on this technique to keep a `View` paired with its runner. And `ScreenViewRunner` is still very much needed under the new scheme, so that we don't force every implementation of `ScreenViewFactory` to solve this problem of maintaining per-view state themselves. But `ScreenViewRunner` is now a convenience, rather than a fundamental mechanism. By making `ScreenViewFactory` responsible for both building and updating, it becomes trivial for one factory to delegate to another -- the update method no longer lives in a fragile `View` tag, and so no longer needs to be replaced. A wrapper factory can just delegate to another one. This means there is no longer a problem to be solved by `DecorativeScreenViewFactory`, so that class is deleted. `ManualScreenViewFactory` is also gone, replaced by a `bindBuiltView()` function that returns `Pair<View, ScreenViewRunner>`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443.
rjrjr
added a commit
that referenced
this issue
Mar 21, 2022
The legacy workflow-ui world is built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This is overly clever, needlessly throws away a good bit of compile time safety, and makes it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` is required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wraps another, with the intention of delegating to its registered `ViewFactory`, that requires creating a wrapper `ViewFactory` that makes a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which is still pretty tricky to use. To solve all this, we do two things: - deprecate all the existing `View` extension methods and abandon `WorkflowViewState`, replacing it with `ScreenViewHolder` - add an `updateView(View)` method to `ScreenViewFactory` `ScreenViewHolder` is inverted from `WorkflowViewState` -- instead of belonging to the `View`, nestled in a tag, the `View` belongs to it. Most of the old `View` extension methods are replaced by methods on `ScreenViewHolder`. It is very similar to `DialogHolder`, but is part of the public API. The change to `ScreenViewFactory`, giving it an `update` method, makes it much more like `OverlayDialogFactory`. I toyed a bit with the idea of trying to unify `ScreenViewFactory` / `OverlayDialogFactory` and `ScreenViewHolder` / `DialogHolder`, but it didn't seem worth the added complexity, especially around parameter types. One of the deprecated `View` extensions did not move to `ScreenViewHolder`: `View.Start` is replaced by `ScreenViewFactory.start`. This extension method is what calls `ScreenViewFactory.buildView`. It creates and returns a `ScreenViewHolder` that wraps the new view and accept updates for it, which it does by calling `ScreenViewFactory.update`, as you'd expect. `ScreenViewFactory.start` is also the new home for the `ViewStarter` mechanism that is used by `WorkflowViewStub` and other containers to call `WorkflowLifecycleOwner.installOn`. We are not completely out of the `View.setTag` business. `ScreenViewFactory` implementations must be stateless -- it is common for separate instances to field calls to `buildView` and `updateView`, or for a single factory instance to build and update multiple views. `View.setTag` is the most practical way for `buildView` to set up state that `updateView` can rely on. This technique is used as an implementation detail of several standard `ScreenViewFactory` implementations, but is never a public concern. In particular, the factories built by the various `ScreenViewRunner.bind` methods rely on this technique to keep a `View` paired with its runner. And `ScreenViewRunner` is still very much needed under the new scheme, so that we don't force every implementation of `ScreenViewFactory` to solve this problem of maintaining per-view state themselves. But `ScreenViewRunner` is now a convenience, rather than a fundamental mechanism. By making `ScreenViewFactory` responsible for both building and updating, it becomes trivial for one factory to delegate to another -- the update method no longer lives in a fragile `View` tag, and so no longer needs to be replaced. A wrapper factory can just delegate to another one. This means there is no longer a problem to be solved by `DecorativeScreenViewFactory`, so that class is deleted. `ManualScreenViewFactory` is also gone, replaced by a `bindBuiltView()` function that returns `Pair<View, ScreenViewRunner>`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443.
rjrjr
added a commit
that referenced
this issue
Mar 22, 2022
The legacy workflow-ui world is built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This is overly clever, needlessly throws away a good bit of compile time safety, and makes it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` is required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wraps another, with the intention of delegating to its registered `ViewFactory`, that requires creating a wrapper `ViewFactory` that makes a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which is still pretty tricky to use. To solve all this, we do two things: - deprecate all the existing `View` extension methods and abandon `WorkflowViewState`, replacing it with `ScreenViewHolder` - add an `updateView(View)` method to `ScreenViewFactory` `ScreenViewHolder` is inverted from `WorkflowViewState` -- instead of belonging to the `View`, nestled in a tag, the `View` belongs to it. Most of the old `View` extension methods are replaced by methods on `ScreenViewHolder`. It is very similar to `DialogHolder`, but is part of the public API. The change to `ScreenViewFactory`, giving it an `update` method, makes it much more like `OverlayDialogFactory`. I toyed a bit with the idea of trying to unify `ScreenViewFactory` / `OverlayDialogFactory` and `ScreenViewHolder` / `DialogHolder`, but it didn't seem worth the added complexity, especially around parameter types. Reviewers should also note that the **`buildView` method no longer has access to the rendering**, a choice I'm still second guessing. So far I haven't hit a use case that required it, beyond a couple of changes to expected strings in `BackStackContainerPersistenceTest`. Perhaps that's reason enough to bring it back? One of the deprecated `View` extensions did not move to `ScreenViewHolder`: `View.Start` is replaced by `ScreenViewFactory.start`. This extension method is what calls `ScreenViewFactory.buildView`. It creates and returns a `ScreenViewHolder` that wraps the new view and accept updates for it, which it does by calling `ScreenViewFactory.update`, as you'd expect. `ScreenViewFactory.start` is also the new home for the `ViewStarter` mechanism that is used by `WorkflowViewStub` and other containers to call `WorkflowLifecycleOwner.installOn`. We are not completely out of the `View.setTag` business. `ScreenViewFactory` implementations must be stateless -- it is common for separate instances to field calls to `buildView` and `updateView`, or for a single factory instance to build and update multiple views. `View.setTag` is the most practical way for `buildView` to set up state that `updateView` can rely on. This technique is used as an implementation detail of several standard `ScreenViewFactory` implementations, but is never a public concern. In particular, the factories built by the various `ScreenViewRunner.bind` methods rely on this technique to keep a `View` paired with its runner. And `ScreenViewRunner` is still very much needed under the new scheme, so that we don't force every implementation of `ScreenViewFactory` to solve this problem of maintaining per-view state themselves. But `ScreenViewRunner` is now a convenience, rather than a fundamental mechanism. By making `ScreenViewFactory` responsible for both building and updating, it becomes trivial for one factory to delegate to another -- the update method no longer lives in a fragile `View` tag, and so no longer needs to be replaced. A wrapper factory can just delegate to another one. This means there is no longer a problem to be solved by `DecorativeScreenViewFactory`, so that class is deleted. `ManualScreenViewFactory` is also gone, replaced by a `bindBuiltView()` function that returns `Pair<View, ScreenViewRunner>`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443.
rjrjr
added a commit
that referenced
this issue
Mar 22, 2022
The legacy workflow-ui world is built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This is overly clever, needlessly throws away a good bit of compile time safety, and makes it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` is required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wraps another, with the intention of delegating to its registered `ViewFactory`, that requires creating a wrapper `ViewFactory` that makes a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which is still pretty tricky to use. To solve all this, we do two things: - deprecate all the existing `View` extension methods and abandon `WorkflowViewState`, replacing it with `ScreenViewHolder` - add an `updateView(View)` method to `ScreenViewFactory` `ScreenViewHolder` is inverted from `WorkflowViewState` -- instead of belonging to the `View`, nestled in a tag, the `View` belongs to it. Most of the old `View` extension methods are replaced by methods on `ScreenViewHolder`. It is very similar to `DialogHolder`, but is part of the public API. The change to `ScreenViewFactory`, giving it an `update` method, makes it much more like `OverlayDialogFactory`. I toyed a bit with the idea of trying to unify `ScreenViewFactory` / `OverlayDialogFactory` and `ScreenViewHolder` / `DialogHolder`, but it didn't seem worth the added complexity, especially around parameter types. Reviewers should also note that the **`buildView` method no longer has access to the rendering**, a choice I'm still second guessing. So far I haven't hit a use case that required it, beyond a couple of changes to expected strings in `BackStackContainerPersistenceTest`. Perhaps that's reason enough to bring it back? It is also worth calling out that `ScreenViewFactory` still is not involved in the `canShow` path. That was a very conscious choice. Making that call dynamic would add a lot of flexibility, but we've never needed that flexibility. The downside of adding it is that we would lose our 1:1 relationship between factories and rendering types, and suddenly would need a resolution mechanism. The cost benefit analysis there seems clear to me. One of the deprecated `View` extensions did not move to `ScreenViewHolder`: `View.Start` is replaced by `ScreenViewFactory.start`. This extension method is what calls `ScreenViewFactory.buildView`. It creates and returns a `ScreenViewHolder` that wraps the new view and accept updates for it, which it does by calling `ScreenViewFactory.update`, as you'd expect. `ScreenViewFactory.start` is also the new home for the `ViewStarter` mechanism that is used by `WorkflowViewStub` and other containers to call `WorkflowLifecycleOwner.installOn`. We are not completely out of the `View.setTag` business. `ScreenViewFactory` implementations must be stateless -- it is common for separate instances to field calls to `buildView` and `updateView`, or for a single factory instance to build and update multiple views. `View.setTag` is the most practical way for `buildView` to set up state that `updateView` can rely on. This technique is used as an implementation detail of several standard `ScreenViewFactory` implementations, but is never a public concern. In particular, the factories built by the various `ScreenViewRunner.bind` methods rely on this technique to keep a `View` paired with its runner. And `ScreenViewRunner` is still very much needed under the new scheme, so that we don't force every implementation of `ScreenViewFactory` to solve this problem of maintaining per-view state themselves. But `ScreenViewRunner` is now a convenience, rather than a fundamental mechanism. By making `ScreenViewFactory` responsible for both building and updating, it becomes trivial for one factory to delegate to another -- the update method no longer lives in a fragile `View` tag, and so no longer needs to be replaced. A wrapper factory can just delegate to another one. This means there is no longer a problem to be solved by `DecorativeScreenViewFactory`, so that class is deleted. `ManualScreenViewFactory` is also gone, replaced by a `bindBuiltView()` function that returns `Pair<View, ScreenViewRunner>`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443.
rjrjr
added a commit
that referenced
this issue
Mar 24, 2022
The legacy workflow-ui world is built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This is overly clever, needlessly throws away a good bit of compile time safety, and makes it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` is required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wraps another, with the intention of delegating to its registered `ViewFactory`, that requires creating a wrapper `ViewFactory` that makes a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which is still pretty tricky to use. To solve all this, we do two things: - deprecate all the existing `View` extension methods and abandon `WorkflowViewState`, replacing it with `ScreenViewHolder` - add an `updateView(View)` method to `ScreenViewFactory` `ScreenViewHolder` is inverted from `WorkflowViewState` -- instead of belonging to the `View`, nestled in a tag, the `View` belongs to it. Most of the old `View` extension methods are replaced by methods on `ScreenViewHolder`. It is very similar to `DialogHolder`, but is part of the public API. The change to `ScreenViewFactory`, giving it an `update` method, makes it much more like `OverlayDialogFactory`. I toyed a bit with the idea of trying to unify `ScreenViewFactory` / `OverlayDialogFactory` and `ScreenViewHolder` / `DialogHolder`, but it didn't seem worth the added complexity, especially around parameter types. Reviewers should also note that the **`buildView` method no longer has access to the rendering**, a choice I'm still second guessing. So far I haven't hit a use case that required it, beyond a couple of changes to expected strings in `BackStackContainerPersistenceTest`. Perhaps that's reason enough to bring it back? It is also worth calling out that `ScreenViewFactory` still is not involved in the `canShow` path. That was a very conscious choice. Making that call dynamic would add a lot of flexibility, but we've never needed that flexibility. The downside of adding it is that we would lose our 1:1 relationship between factories and rendering types, and suddenly would need a resolution mechanism. The cost benefit analysis there seems clear to me. One of the deprecated `View` extensions did not move to `ScreenViewHolder`: `View.Start` is replaced by `ScreenViewFactory.start`. This extension method is what calls `ScreenViewFactory.buildView`. It creates and returns a `ScreenViewHolder` that wraps the new view and accept updates for it, which it does by calling `ScreenViewFactory.update`, as you'd expect. `ScreenViewFactory.start` is also the new home for the `ViewStarter` mechanism that is used by `WorkflowViewStub` and other containers to call `WorkflowLifecycleOwner.installOn`. We are not completely out of the `View.setTag` business. `ScreenViewFactory` implementations must be stateless -- it is common for separate instances to field calls to `buildView` and `updateView`, or for a single factory instance to build and update multiple views. `View.setTag` is the most practical way for `buildView` to set up state that `updateView` can rely on. This technique is used as an implementation detail of several standard `ScreenViewFactory` implementations, but is never a public concern. In particular, the factories built by the various `ScreenViewRunner.bind` methods rely on this technique to keep a `View` paired with its runner. And `ScreenViewRunner` is still very much needed under the new scheme, so that we don't force every implementation of `ScreenViewFactory` to solve this problem of maintaining per-view state themselves. But `ScreenViewRunner` is now a convenience, rather than a fundamental mechanism. By making `ScreenViewFactory` responsible for both building and updating, it becomes trivial for one factory to delegate to another -- the update method no longer lives in a fragile `View` tag, and so no longer needs to be replaced. A wrapper factory can just delegate to another one. This means there is no longer a problem to be solved by `DecorativeScreenViewFactory`, so that class is deleted. `ManualScreenViewFactory` is also gone, replaced by a `bindBuiltView()` function that returns `Pair<View, ScreenViewRunner>`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443.
rjrjr
added a commit
that referenced
this issue
Mar 29, 2022
The legacy workflow-ui world was built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This was overly clever, needlessly threw away a good bit of compile time safety, and made it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` was required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wrapped another, with the intention of delegating to its registered `ViewFactory`, that required creating a wrapper `ViewFactory` that made a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which was still pretty tricky to use. To solve all this, we reverse the relationship between `View` and `WorkflowViewState`, with the latter being replaced by `ScreenViewHolder`. Instead of returing a raw `View`, `ScreenViewFactory.buildView` now returns the view in a `ScreenViewHolder`. The call to `ScreenViewHolder()` replaces the old call to `bindShowRendering`, and the compiler makes it impossible to forget. `ScreenViewHolder` is a box that holds: - a `View` - the `(Screen, ViewEnvironment) -> Unit` function that updates it (still `fun interface ScreenViewRunner` for ease of implementation) - and the latest `ViewEnvironment` that function received It is worth noting that `ScreenViewHolder.showing` `.canShow` and `.show()` are all static extensions, built around the new `Showing: ViewEnvironmentKey<Screen>`. `ScreenViewHolder` itself is "write only", with an `in ScreenT: Screen` type parameter -- you can push strongly typed `Screen` renderings into it, but you can't pull them out again. This minimalist `ScreenViewHolder` interface is trivial to wrap, so all the complexity that inspired `DecorativeViewFactory` is gone. It is replaced by the new `ScreenViewFactory.unwrapping` extension functions, whose use is demontrated in `NamedScreenViewFactory`, `EnvironmentScreenViewFactory` and `BackButtonScreen`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443.
rjrjr
added a commit
that referenced
this issue
Mar 29, 2022
The legacy workflow-ui world was built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This was overly clever, needlessly threw away a good bit of compile time safety, and made it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` was required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wrapped another, with the intention of delegating to its registered `ViewFactory`, that required creating a wrapper `ViewFactory` that made a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which was still pretty tricky to use. To solve all this, we reverse the relationship between `View` and `WorkflowViewState`, with the latter being replaced by `ScreenViewHolder`. Instead of returing a raw `View`, `ScreenViewFactory.buildView` now returns the view in a `ScreenViewHolder`. The call to `ScreenViewHolder()` replaces the old call to `bindShowRendering`, and the compiler makes it impossible to forget. `ScreenViewHolder` is a box that holds: - a `View` - the `(Screen, ViewEnvironment) -> Unit` function that updates it (still `fun interface ScreenViewRunner` for ease of implementation) - and the latest `ViewEnvironment` that function received It is worth noting that `ScreenViewHolder.showing` `.canShow` and `.show()` are all static extensions, built around the new `Showing: ViewEnvironmentKey<Screen>`. `ScreenViewHolder` itself is "write only", with an `in ScreenT: Screen` type parameter -- you can push strongly typed `Screen` renderings into it, but you can't pull them out again. This minimalist `ScreenViewHolder` interface is trivial to wrap, so all the complexity that inspired `DecorativeViewFactory` is gone. It is replaced by the new `ScreenViewFactory.unwrapping` extension functions, whose use is demontrated in `NamedScreenViewFactory`, `EnvironmentScreenViewFactory` and `BackButtonScreen`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443. ``` title WorkflowViewStub.show() participant "WorkflowViewStub" as stub participant Screen participant "ViewEnvironment" as env participant "ScreenViewFactoryFinder" as finder participant "ViewRegistry" as registry participant "ScreenViewFactory" as factory participant "ScreenViewHolder" as holder [->stub:show(screen, env) stub->Screen:buildView(env) activate Screen Screen->Screen:toViewFactory(env) activate Screen Screen->env:get(ScreenViewFactoryFinder) Screen<--env:finder Screen->finder:getViewFactoryForRendering(this) activate finder finder->registry:getEntryFor(screen) finder<--registry:factory Screen<--finder:factory deactivate finder deactivate Screen Screen->factory:start(this, env) activate factory factory->factory:buildView(screen, env) activate factory factory->holder:init(screen, env, runner) holder-->factory:holder deactivate factory factory->holder:show(screen, env) activate holder holder->holder:this.environment = env + (Showing to screen) holder->holder:this.runner.show(screen, environment) deactivate holder Screen<--factory:holder deactivate factory stub<--Screen:holder deactivate Screen [->stub:show(screen, env) activate stub stub->holder:canShow(screen) activate holder holder->holder: compatible(this.environment[Showing], screen) stub<--holder:true deactivate holder stub->holder:show(screen, env) activate holder holder->holder:this.environment = env + (Showing to screen) holder->holder:this.runner.show(screen, environment) deactivate holder deactivate stub ```
rjrjr
added a commit
that referenced
this issue
Mar 29, 2022
The legacy workflow-ui world was built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This was overly clever, needlessly threw away a good bit of compile time safety, and made it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` was required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wrapped another, with the intention of delegating to its registered `ViewFactory`, that required creating a wrapper `ViewFactory` that made a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which was still pretty tricky to use. To solve all this, we reverse the relationship between `View` and `WorkflowViewState`, with the latter being replaced by `ScreenViewHolder`. Instead of returing a raw `View`, `ScreenViewFactory.buildView` now returns the view in a `ScreenViewHolder`. The call to `ScreenViewHolder()` replaces the old call to `bindShowRendering`, and the compiler makes it impossible to forget. `ScreenViewHolder` is a box that holds: - a `View` - the `(Screen, ViewEnvironment) -> Unit` function that updates it (still `fun interface ScreenViewRunner` for ease of implementation) - and the latest `ViewEnvironment` that function received It is worth noting that `ScreenViewHolder.showing` `.canShow` and `.show()` are all static extensions, built around the new `Showing: ViewEnvironmentKey<Screen>`. `ScreenViewHolder` itself is "write only", with an `in ScreenT: Screen` type parameter -- you can push strongly typed `Screen` renderings into it, but you can't pull them out again. This minimalist `ScreenViewHolder` interface is trivial to wrap, so all the complexity that inspired `DecorativeViewFactory` is gone. It is replaced by the new `ScreenViewFactory.unwrapping` extension functions, whose use is demontrated in `NamedScreenViewFactory`, `EnvironmentScreenViewFactory` and `BackButtonScreen`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443. ``` title WorkflowViewStub.show() participant "WorkflowViewStub" as stub participant Screen participant "ViewEnvironment" as env participant "ScreenViewFactoryFinder" as finder participant "ViewRegistry" as registry participant "ScreenViewFactory" as factory participant "ScreenViewHolder" as holder [->stub:show(screen, env) stub->Screen:buildView(env) activate Screen Screen->Screen:toViewFactory(env) activate Screen Screen->env:get(ScreenViewFactoryFinder) Screen<--env:finder Screen->finder:getViewFactoryForRendering(this) activate finder finder->registry:getEntryFor(screen) finder<--registry:factory Screen<--finder:factory deactivate finder deactivate Screen Screen->factory:start(this, env) activate factory factory->factory:buildView(screen, env) activate factory factory->holder:init(screen, env, runner) holder-->factory:holder deactivate factory factory->holder:show(screen, env) activate holder holder->holder:this.environment = env + (Showing to screen) holder->holder:this.runner.show(screen, environment) deactivate holder Screen<--factory:holder deactivate factory stub<--Screen:holder deactivate Screen [->stub:show(screen, env) activate stub stub->holder:canShow(screen) activate holder holder->holder: compatible(this.environment[Showing], screen) stub<--holder:true deactivate holder stub->holder:show(screen, env) activate holder holder->holder:this.environment = env + (Showing to screen) holder->holder:this.runner.show(screen, environment) deactivate holder deactivate stub ```
rjrjr
added a commit
that referenced
this issue
Mar 29, 2022
The legacy workflow-ui world was built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This was overly clever, needlessly threw away a good bit of compile time safety, and made it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` was required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wrapped another, with the intention of delegating to its registered `ViewFactory`, that required creating a wrapper `ViewFactory` that made a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which was still pretty tricky to use. To solve all this, we reverse the relationship between `View` and `WorkflowViewState`, with the latter being replaced by `ScreenViewHolder`. Instead of returing a raw `View`, `ScreenViewFactory.buildView` now returns the view in a `ScreenViewHolder`. The call to `ScreenViewHolder()` replaces the old call to `bindShowRendering`, and the compiler makes it impossible to forget. `ScreenViewHolder` is a box that holds: - a `View` - the `(Screen, ViewEnvironment) -> Unit` function that updates it (still `fun interface ScreenViewRunner` for ease of implementation) - and the latest `ViewEnvironment` that function received It is worth noting that `ScreenViewHolder.showing` `.canShow` and `.show()` are all static extensions, built around the new `Showing: ViewEnvironmentKey<Screen>`. `ScreenViewHolder` itself is "write only", with an `in ScreenT: Screen` type parameter -- you can push strongly typed `Screen` renderings into it, but you can't pull them out again. This minimalist `ScreenViewHolder` interface is trivial to wrap, so all the complexity that inspired `DecorativeViewFactory` is gone. It is replaced by the new `ScreenViewFactory.unwrapping` extension functions, whose use is demontrated in `NamedScreenViewFactory`, `EnvironmentScreenViewFactory` and `BackButtonScreen`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443. ``` title WorkflowViewStub.show() participant "WorkflowViewStub" as stub participant Screen participant "ViewEnvironment" as env participant "ScreenViewFactoryFinder" as finder participant "ViewRegistry" as registry participant "ScreenViewFactory" as factory participant "ScreenViewHolder" as holder [->stub:show(screen, env) stub->Screen:buildView(env) activate Screen Screen->Screen:toViewFactory(env) activate Screen Screen->env:get(ScreenViewFactoryFinder) Screen<--env:finder Screen->finder:getViewFactoryForRendering(this) activate finder finder->registry:getEntryFor(screen) finder<--registry:factory Screen<--finder:factory deactivate finder deactivate Screen Screen->factory:start(this, env) activate factory factory->factory:buildView(screen, env) activate factory factory->holder:init(screen, env, runner) holder-->factory:holder deactivate factory factory->holder:show(screen, env) activate holder holder->holder:this.environment = env + (Showing to screen) holder->holder:this.runner.show(screen, environment) deactivate holder Screen<--factory:holder deactivate factory stub<--Screen:holder deactivate Screen [->stub:show(screen, env) activate stub stub->holder:canShow(screen) activate holder holder->holder: compatible(this.environment[Showing], screen) stub<--holder:true deactivate holder stub->holder:show(screen, env) activate holder holder->holder:this.environment = env + (Showing to screen) holder->holder:this.runner.show(screen, environment) deactivate holder deactivate stub ```
rjrjr
added a commit
that referenced
this issue
Mar 30, 2022
Replaces #703. The legacy workflow-ui world was built around a set of extension methods on `View`, backed by a `WorkflowViewState` object hanging off of a tag. This was overly clever, needlessly threw away a good bit of compile time safety, and made it much harder than it should be to implement wrapper renderings. In particular, every `ViewFactory` was required to call `View.bindShowRendering` to put all this machinery in place. When a screen rendering wrapped another, with the intention of delegating to its registered `ViewFactory`, that required creating a wrapper `ViewFactory` that made a new `bindShowRendering` call replacing and wrapping the `WorkflowViewState` put in place by the delegate. That's so tricky it required the introduction of `DecorativeViewFactory`, which was still pretty tricky to use. To solve all this, we reverse the relationship between `View` and `WorkflowViewState`, with the latter being replaced by `ScreenViewHolder`. Instead of returing a raw `View`, `ScreenViewFactory.buildView` now returns the view in a `ScreenViewHolder`. The call to `ScreenViewHolder()` replaces the old call to `bindShowRendering`, and the compiler makes it impossible to forget. `ScreenViewHolder` is a box that holds: - a `View` - the `(Screen, ViewEnvironment) -> Unit` function that updates it (still `fun interface ScreenViewRunner` for ease of implementation) - and the latest `ViewEnvironment` that function received It is worth noting that `ScreenViewHolder.showing` `.canShow` and `.show()` are all static extensions, built around the new `Showing: ViewEnvironmentKey<Screen>`. `ScreenViewHolder` itself is "write only", with an `in ScreenT: Screen` type parameter -- you can push strongly typed `Screen` renderings into it, but you can't pull them out again. This minimalist `ScreenViewHolder` interface is trivial to wrap, so all the complexity that inspired `DecorativeViewFactory` is gone. It is replaced by the new `ScreenViewFactory.unwrapping` extension functions, whose use is demontrated in `NamedScreenViewFactory`, `EnvironmentScreenViewFactory` and `BackButtonScreen`. The `ViewStarter` mechanism is also much simpler now, also no longer relying on any state stored in `View` tags. The implementation of `ScreenViewFactory.start()` remains a bit complex, but only because of the work it has to do to keep the legacy `View.start()` method working, in partnership with `AsScreenViewFactory()`. Once we delete the deprecated View code, most of the code in both of those can be deleted. Fixes #413, fixes #598, fixes #626, fixes #551, fixes #443. ``` title WorkflowViewStub.show() participant "WorkflowViewStub" as stub participant Screen participant "ViewEnvironment" as env participant "ScreenViewFactoryFinder" as finder participant "ViewRegistry" as registry participant "ScreenViewFactory" as factory participant "ScreenViewHolder" as holder [->stub:show(screen, env) stub->Screen:buildView(env) activate Screen Screen->Screen:toViewFactory(env) activate Screen Screen->env:get(ScreenViewFactoryFinder) Screen<--env:finder Screen->finder:getViewFactoryForRendering(this) activate finder finder->registry:getEntryFor(screen) finder<--registry:factory Screen<--finder:factory deactivate finder deactivate Screen Screen->factory:start(this, env) activate factory factory->factory:buildView(screen, env) activate factory factory->holder:init(screen, env, runner) holder-->factory:holder deactivate factory factory->holder:show(screen, env) activate holder holder->holder:this.environment = env + (Showing to screen) holder->holder:this.runner.show(screen, environment) deactivate holder Screen<--factory:holder deactivate factory stub<--Screen:holder deactivate Screen [->stub:show(screen, env) activate stub stub->holder:canShow(screen) activate holder holder->holder: compatible(this.environment[Showing], screen) stub<--holder:true deactivate holder stub->holder:show(screen, env) activate holder holder->holder:this.environment = env + (Showing to screen) holder->holder:this.runner.show(screen, environment) deactivate holder deactivate stub ```
|
Obviated by |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It should be possible to build something like transforms suggested by #606 (spiked in #621) as extensions on
ScreenViewFactory. Then we can either get rid of that thing or make it private.The text was updated successfully, but these errors were encountered: