Updates to make it easier to avoid using DescribedViewController, allowing flattening of VC hierarchies.
#153
Conversation
WorkflowUI/Sources/ViewControllerDescription/UIViewController+Extensions.swift
Show resolved
Hide resolved
kyleve
force-pushed
the
kve/flatten-described-vcs
branch
from
a76c792 to
8907b6a
Compare
| /// - parameter environment: The `environment` to used when updating the view controller. | ||
| /// - parameter onChange: A callback called if the view controller instance changed. | ||
| /// | ||
| public func update<VC: UIViewController, ScreenType: Screen>( |
There was a problem hiding this comment.
Useful for doing one-liner updates for contained view controllers. Eg, in Market we do:
private var content : UIViewController
...
override func screenDidChange(from previousScreen: ModalHostContainer, previousEnvironment: ViewEnvironment) {
update(child: \.content, with: screen.content, in: environment)
...
}
| @@ -26,4 +26,10 @@ | |||
| func viewControllerDescription(environment: ViewEnvironment) -> ViewControllerDescription | |||
| } | |||
|
|
|||
| extension Screen { | |||
| public func update(viewController: UIViewController, with environment: ViewEnvironment) { | |||
There was a problem hiding this comment.
Is this used?
It seems easy enough to just call the logic this abstracts, I'd be fine leaving this convenience out 🤷
There was a problem hiding this comment.
It's used in Market somewhat commonly to avoid having to do the description dance which saves a line of code or so, which IMO is nice, but also fine to remove it.
| } | ||
| } | ||
|
|
||
| public protocol UpdateChildScreenViewController {} |
There was a problem hiding this comment.
This dance is needed so we can reference Self in the extension method.
| /// in a way that is `Equatable` and `Hashable`. When implementing view controller | ||
| /// updating and diffing, you can use this type to identify if the backing view controller | ||
| /// type changed. | ||
| public struct KindIdentifier: Hashable { |
There was a problem hiding this comment.
Since this type is Hashable, you can use it to salt your ID types, making handling diffs easier when the underlying type of the VC in the view description changes.
kyleve
changed the title
DescribedViewController, allowing flattening of VC hierarchies.
| @@ -26,4 +26,10 @@ | |||
| func viewControllerDescription(environment: ViewEnvironment) -> ViewControllerDescription | |||
| } | |||
|
|
|||
| extension Screen { | |||
| public func update(viewController: UIViewController, with environment: ViewEnvironment) { | |||
There was a problem hiding this comment.
Is this used?
It seems easy enough to just call the logic this abstracts, I'd be fine leaving this convenience out 🤷
|
|
||
| import UIKit | ||
|
|
||
| extension UpdateChildScreenViewController where Self: UIViewController { |
There was a problem hiding this comment.
Why not put this directly on extension UIViewController and get rid of the protocol?
There was a problem hiding this comment.
This is needed because the method references Self, which you can't do in an extension on a concrete type like UIViewController. Blueprint does the same dance, for example.
WorkflowUI/Sources/ViewControllerDescription/UIViewController+Extensions.swift
Outdated
Show resolved
Hide resolved
WorkflowUI/Sources/ViewControllerDescription/UIViewController+Extensions.swift
Show resolved
Hide resolved
| parent.isViewLoaded, | ||
| old.isViewLoaded, | ||
| let container = old.view.superview { |
There was a problem hiding this comment.
If these views were not loaded when this is called, how do we ensure we still insertSubview etc.?
There was a problem hiding this comment.
We don't – we assume the parent view controller will do its own setup in loadView/viewDidLoad when it is asked to load the view.
| /// Describes the `UIViewController` type that backs the `ViewControllerDescription` | ||
| /// in a way that is `Equatable` and `Hashable`. When implementing view controller | ||
| /// updating and diffing, you can use this type to identify if the backing view controller | ||
| /// type changed. | ||
| public let kind: KindIdentifier |
| @@ -69,8 +75,10 @@ | |||
| fatalError("init(coder:) has not been implemented") | |||
| } | |||
|
|
|||
| private func render(screen: ScreenType, environment: ViewEnvironment) { | |||
| rootViewController.update(screen: screen, environment: environment) | |||
| private func update(screen: ScreenType, environment: ViewEnvironment) { | |||
There was a problem hiding this comment.
Do we need to also call this from viewDidLoad to ensure the views get added etc.?
There was a problem hiding this comment.
We do not – when the container loads its view, it will add the rootVC as a child: https://github.com/square/workflow-swift/blob/main/WorkflowUI/Sources/Container/ContainerViewController.swift#L81
|
|
||
| if description.canUpdate(viewController: existing) { | ||
| // Easy path: Just update the existing view controller if we can do that. | ||
| description.update(viewController: existing) |
There was a problem hiding this comment.
Do we potentially need to add subviews etc. here if they're not already added (due to the isViewLoaded checks below)?
There was a problem hiding this comment.
No – the assumption is the implementation of the view controller will do this in loadView/viewDidLoad, as custom parent VCs are expected to do.
WorkflowUI/Sources/ViewControllerDescription/UIViewController+Extensions.swift
Outdated
Show resolved
Hide resolved
| import XCTest | ||
| @testable import WorkflowUI | ||
|
|
||
| class UIViewControllerExtensionTests: XCTestCase { |
There was a problem hiding this comment.
Tests have been added
kyleve
force-pushed
the
kve/flatten-described-vcs
branch
4 times, most recently
from
6771004 to
d6704cf
Compare
…ectly to remove DescribedViewController layering from VC hierarchies.
kyleve
force-pushed
the
kve/flatten-described-vcs
branch
from
eff6175 to
7b46aa4
Compare
This change makes it easier to avoid using
DescribedViewController, which adds a (largely needless) layer to view controller hierarchies. I'll leave comments inline on relevant additions with comments.For Square folks, I recommend seeing how this is used in context in https://github.com/squareup/market/pull/4559.
Before
After
Checklist