-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Either hide ShowRenderingTag or don't make it a data class #598
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 ```
Gone. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
No description provided.
The text was updated successfully, but these errors were encountered: