[swift] Remove view registry #974

bencochran · 2020-02-17T23:21:00Z

This removes the view registry in favor of having screens provide their own view controllers (via ViewControllerDescription).

This improves compile-time safety as well as allows for easier container types (we can now have generic screen types, and pass-through containers no longer must provide a wrapper view
controller).

Closes #815

swift/WorkflowUI/Sources/Screen/ScreenViewController.swift

bencochran · 2020-02-18T00:03:16Z

swift/Samples/SplitScreenContainer/Sources/SplitScreenContainerViewController.swift

    private lazy var trailingContainerView: ContainerView = .init()

-    private var currentRatio: CGFloat {


got rid of these redundant copies of the values while I was in here

bencochran · 2020-02-18T00:04:17Z

swift/Samples/TicTacToe/Sources/Authentication/LoginScreen.swift

-
-        update(with: screen)
-    }
+fileprivate final class LoginViewController: UIViewController {


Made this one not be a subclass of ScreenViewController just to show that is’s not necessary as a superclass

swift/Samples/TicTacToe/Sources/Authentication/LoginScreen.swift

bencochran · 2020-02-18T00:08:38Z

swift/WorkflowUI/Sources/ViewControllerDescription/DescribedViewController.swift

@@ -19,51 +19,60 @@
 import UIKit


-internal final class AnyScreenViewController: ScreenViewController<AnyScreen> {
+public final class DescribedViewController: UIViewController {


This isn’t a rename, but a rewrite. I suppose they do perform mostly the same job, though.

bencochran · 2020-02-18T00:09:21Z

swift/WorkflowUI/Sources/Screen/AnyScreen/AnyScreen.swift

@@ -19,24 +19,22 @@
 import UIKit

 public struct AnyScreen: Screen {


Happy to see this collapse into just a passthrough for the view controller description

bencochran · 2020-02-18T00:14:35Z

swift/WorkflowUI/Sources/ViewControllerDescription/ViewControllerDescription.swift

+    internal func buildViewController() -> UIViewController {
+        let viewController = build()
+        assert(canUpdate(viewController: viewController), "View controller description built a view controller it cannot update (\(viewController) is not exactly type \(viewControllerType))")
+        return viewController


I initially considered calling update with the view controller after building it. Technically that’s redundant for our ScreenViewController subclasses (since they all take the screen in in init), but I’m worried that omitting it will cause a lot of people to initially build a view controller that’s not fully populated without realizing it. Anyone opposed to calling update eagerly here?

could that cause an issue if an update implementation expects the view controller to be inside a window?

canUpdate is just the type check, the update implementation doesn’t play in here. Though, if someone wrote an update method that required being in a window, I’d be very sad at them.

Oh, I totally misattributed your question and thought it was about the assert(canUpdate…. So, yeah doing an update eagerly here would break a view controller’s ability to assume it’s in a window when it gets updated. But I strongly believe that’s not an assumption we should allow people to make.

This removes the view registry in favor of having screens provide their own view controllers (via `ViewControllerDescription`). This improves compile-time safety as well as allows for easier container types (we can now have generic screen types, and pass-through containers no longer must provide a wrapper view controller).

dhavalshreyas · 2020-02-18T15:33:03Z

swift/WorkflowUI/Sources/ViewControllerDescription/ViewControllerDescription.swift

+    ///   - build: Closure that produces a new instance of the view controller
+    ///   - update: Closure that updates the given view controller
+    public init<VC: UIViewController>(type: VC.Type = VC.self, build: @escaping () -> VC, update: @escaping (VC) -> Void) {
+        self.viewControllerType = type


Instead of doing this, what if we hold-on to the canUpdate block?

self.canUpdate = { untypedVC in return untypedVC is VC }

I played with that locally, but found it really useful for debugging to have the type around in a printable manner. I also don’t think we want is because we only want exact matches, not subclasses IMO.

curious why we wouldn't want to allow subclasses in this context (out of ignorance, I'm not familiar enough yet to know why or why not to allow it :) )

I could see a use case for something like subclasses altering layout of the same views, and using the same screen to update the super class and the subclass

Since a single view controller description is responsible for building and updating, I can’t imagine a scenario where you’d reasonably build a type of a superclass and then be passed a value of a subclass, except by mistake.

Do we actually want to be able to pass in something other than the default (VC.self) here?

In the case of the extension on ScreenViewController, it was inferring a type of ScreenViewController<FooScreen> instead of FooViewController with older Swift versions (where I couldn’t do Self(screen: screen) and instead had to do self.init(screen: screen). Adding this hinting allowed me to fix that.

dhavalshreyas

This is awesome, thank you!

swift/Samples/SplitScreenContainer/Sources/SplitScreenContainerViewController.swift

swift/WorkflowUI/Sources/Screen/Screen.swift

swift/WorkflowUI/Sources/ViewControllerDescription/DescribedViewController.swift

JustinDSN

This is really really great! Thank you! Just left a few comments.

AquaGeek · 2020-02-18T18:41:03Z

Cases where we want to change the resulting view controller type (e.g. adapt a Screen defined elsewhere to our use case) now require you to wrap that in your own Screen type (or pull off the various properties that you need/care about), right?

Also, do we care that this requires that we use view controllers as the currency globally (as opposed to e.g. views)?

bencochran · 2020-02-18T18:50:20Z

Cases where we want to change the resulting view controller type (e.g. adapt a Screen defined elsewhere to our use case) now require you to wrap that in your own Screen type (or pull off the various properties that you need/care about), right?

Yep, that’s the intended pattern should we need it. It would also be valid for a workflow to render a type that’s not a Screen then have a consumer conform it’s type to Screen to provide a view controller, though I like that approach less.

Also, do we care that this requires that we use view controllers as the currency globally (as opposed to e.g. views)?

Not personally. It’s already the case today with view registry. We could provide a pretty easy convenience for ViewControllerDescription that describes a UIView if we wanted.

timdonnelly

Thanks!

zach-klippenstein

This is awesome, but how bad is updating existing code to use this going to be? Should we leave the ViewRegistry parameters in for now and do a release with support for the new technique, so we can migrate existing code over incrementally?

dhavalshreyas · 2020-02-18T21:14:35Z

This is awesome, but how bad is updating existing code to use this going to be? Should we leave the ViewRegistry parameters in for now and do a release with support for the new technique, so we can migrate existing code over incrementally?

We tried a few approaches (#816) which would allow us to migrate incrementally, however there were some risks with this approach. Since we'll need the same ViewRegistry to get carried over to the leafs, if we use a non-ViewRegistry Screen in between, there will be a run-time crash.

The hard-removal approach is a lot more up-front work to do the migration, however we'll have compile-time safety and will be a cleaner migration.

…llerDescription

bencochran force-pushed the bc/view-description branch from 2b98700 to 3476fb7 Compare February 17, 2020 23:58

bencochran commented Feb 18, 2020

View reviewed changes

bencochran force-pushed the bc/view-description branch from 6d748b3 to c01b2a8 Compare February 18, 2020 02:51

bencochran changed the title ~~[WIP] [swift] Remove view registry~~ [swift] Remove view registry Feb 18, 2020

bencochran requested review from JustinDSN, timdonnelly, zach-klippenstein, rjrjr and dhavalshreyas February 18, 2020 04:59

dhavalshreyas reviewed Feb 18, 2020

View reviewed changes

dhavalshreyas approved these changes Feb 18, 2020

View reviewed changes

JustinDSN reviewed Feb 18, 2020

View reviewed changes

swift/Samples/SplitScreenContainer/Sources/SplitScreenContainerViewController.swift Show resolved Hide resolved

JustinDSN reviewed Feb 18, 2020

View reviewed changes

swift/WorkflowUI/Sources/Screen/Screen.swift Show resolved Hide resolved

JustinDSN reviewed Feb 18, 2020

View reviewed changes

swift/WorkflowUI/Sources/ViewControllerDescription/DescribedViewController.swift Show resolved Hide resolved

JustinDSN requested a review from amorde February 18, 2020 17:48

JustinDSN approved these changes Feb 18, 2020

View reviewed changes

timdonnelly approved these changes Feb 18, 2020

View reviewed changes

amorde approved these changes Feb 18, 2020

View reviewed changes

zach-klippenstein approved these changes Feb 18, 2020

View reviewed changes

Always perform initial update to view controllers built by ViewContro…

eed86f8

…llerDescription

bencochran merged commit cffc161 into master Feb 26, 2020

bencochran deleted the bc/view-description branch February 26, 2020 23:10

This was referenced Feb 28, 2020

Soft-deprecate ViewRegistry #816

Closed

Consider de-emphasizing ViewRegistry. #390

Closed

[Swift] View Registry to support Factory blocks for screen -> view controller #553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[swift] Remove view registry #974

[swift] Remove view registry #974

bencochran commented Feb 17, 2020 •

edited

bencochran Feb 18, 2020

bencochran Feb 18, 2020

bencochran Feb 18, 2020

bencochran Feb 18, 2020

bencochran Feb 18, 2020

amorde Feb 18, 2020

bencochran Feb 18, 2020

bencochran Feb 18, 2020

dhavalshreyas Feb 18, 2020

bencochran Feb 18, 2020

amorde Feb 18, 2020

bencochran Feb 18, 2020

AquaGeek Feb 18, 2020

bencochran Feb 18, 2020

dhavalshreyas left a comment

JustinDSN left a comment

AquaGeek commented Feb 18, 2020

bencochran commented Feb 18, 2020

timdonnelly left a comment

zach-klippenstein left a comment

dhavalshreyas commented Feb 18, 2020

		private lazy var trailingContainerView: ContainerView = .init()

		private var currentRatio: CGFloat {

		@@ -19,24 +19,22 @@
		import UIKit

		public struct AnyScreen: Screen {

[swift] Remove view registry #974

[swift] Remove view registry #974

Conversation

bencochran commented Feb 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhavalshreyas left a comment

Choose a reason for hiding this comment

JustinDSN left a comment

Choose a reason for hiding this comment

AquaGeek commented Feb 18, 2020

bencochran commented Feb 18, 2020

timdonnelly left a comment

Choose a reason for hiding this comment

zach-klippenstein left a comment

Choose a reason for hiding this comment

dhavalshreyas commented Feb 18, 2020

bencochran commented Feb 17, 2020 •

edited