Skip to content
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

All kinds of version updates, esp. agp, androidx and kotlin. #443

Merged
merged 1 commit into from
Jun 25, 2021

Conversation

rjrjr
Copy link
Contributor

@rjrjr rjrjr commented Jun 25, 2021

No description provided.

@rjrjr rjrjr requested review from zach-klippenstein and a team as code owners June 25, 2021 18:12
Copy link
Contributor

@steve-the-edwards steve-the-edwards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onwards and upwards.

@rjrjr rjrjr merged commit 1d41f69 into main Jun 25, 2021
@rjrjr rjrjr deleted the ray/deps-update branch June 25, 2021 19:05
rjrjr added a commit that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants