-
Notifications
You must be signed in to change notification settings - Fork 47
[feat]: add support for sizingOptions to SwiftUIScreen #277
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
Changes from all commits
dc5c9f0
9270a33
8120743
3199ad9
fce48e6
db7bc67
12619f4
b54de19
7cb26e6
d907e42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,14 +23,20 @@ import WorkflowUI | |
public protocol SwiftUIScreen: Screen { | ||
associatedtype Content: View | ||
|
||
var sizingOptions: SwiftUIScreenSizingOptions { get } | ||
|
||
@ViewBuilder | ||
static func makeView(model: ObservableValue<Self>) -> Content | ||
|
||
static var isEquivalent: ((Self, Self) -> Bool)? { get } | ||
} | ||
|
||
public extension SwiftUIScreen { | ||
static var isEquivalent: ((Self, Self) -> Bool)? { return nil } | ||
var sizingOptions: SwiftUIScreenSizingOptions { [] } | ||
} | ||
|
||
public extension SwiftUIScreen { | ||
static var isEquivalent: ((Self, Self) -> Bool)? { nil } | ||
} | ||
|
||
public extension SwiftUIScreen where Self: Equatable { | ||
|
@@ -53,17 +59,29 @@ public extension SwiftUIScreen { | |
viewEnvironment: viewEnvironment, | ||
content: Self.makeView(model: model) | ||
) | ||
}) | ||
}), | ||
swiftUIScreenSizingOptions: sizingOptions | ||
) | ||
}, | ||
update: { | ||
$0.modelSink.send(self) | ||
$0.viewEnvironmentSink.send(environment) | ||
$0.swiftUIScreenSizingOptions = sizingOptions | ||
} | ||
) | ||
} | ||
} | ||
|
||
public struct SwiftUIScreenSizingOptions: OptionSet { | ||
public let rawValue: Int | ||
|
||
public init(rawValue: Int) { | ||
self.rawValue = rawValue | ||
} | ||
|
||
public static let preferredContentSize: SwiftUIScreenSizingOptions = .init(rawValue: 1 << 0) | ||
} | ||
|
||
private struct EnvironmentInjectingView<Content: View>: View { | ||
@ObservedObject var viewEnvironment: ObservableValue<ViewEnvironment> | ||
let content: Content | ||
|
@@ -77,17 +95,104 @@ private struct EnvironmentInjectingView<Content: View>: View { | |
private final class ModeledHostingController<Model, Content: View>: UIHostingController<Content> { | ||
let modelSink: Sink<Model> | ||
let viewEnvironmentSink: Sink<ViewEnvironment> | ||
var swiftUIScreenSizingOptions: SwiftUIScreenSizingOptions { | ||
didSet { | ||
updateSizingOptionsIfNeeded() | ||
|
||
init(modelSink: Sink<Model>, viewEnvironmentSink: Sink<ViewEnvironment>, rootView: Content) { | ||
if !hasLaidOutOnce { | ||
setNeedsLayoutBeforeFirstLayoutIfNeeded() | ||
} | ||
} | ||
} | ||
|
||
private var hasLaidOutOnce = false | ||
|
||
init( | ||
modelSink: Sink<Model>, | ||
viewEnvironmentSink: Sink<ViewEnvironment>, | ||
rootView: Content, | ||
swiftUIScreenSizingOptions: SwiftUIScreenSizingOptions | ||
) { | ||
self.modelSink = modelSink | ||
self.viewEnvironmentSink = viewEnvironmentSink | ||
self.swiftUIScreenSizingOptions = swiftUIScreenSizingOptions | ||
|
||
super.init(rootView: rootView) | ||
|
||
updateSizingOptionsIfNeeded() | ||
} | ||
|
||
required init?(coder aDecoder: NSCoder) { | ||
fatalError("not implemented") | ||
} | ||
|
||
override func viewDidLoad() { | ||
super.viewDidLoad() | ||
|
||
view.backgroundColor = .clear | ||
|
||
setNeedsLayoutBeforeFirstLayoutIfNeeded() | ||
} | ||
|
||
override func viewDidLayoutSubviews() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hmm yeah this is an interesting one, since that stuff all lives in Modals right now. We'll definitely want to keep it, since avoiding the duplicative preferredContentSize calculations was a pretty big performance win. Trying to think if there's a way we could flow this into the SwiftUI environment at the Market layer... But I guess since this is a view controller, we don't have that available. One gross idea, but it would work, is do a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we may want to consider moving that associated object to a Let's move further discussion on this to UI-5801 so that the context is easier to discover once we start that work! |
||
super.viewDidLayoutSubviews() | ||
|
||
defer { hasLaidOutOnce = true } | ||
|
||
if #available(iOS 16.0, *) { | ||
// Handled in initializer, but set it on first layout to resolve a bug where the PCS is | ||
// not updated appropriately after the first layout. | ||
// UI-5797 | ||
if !hasLaidOutOnce, | ||
swiftUIScreenSizingOptions.contains(.preferredContentSize) { | ||
let size = view.sizeThatFits(view.frame.size) | ||
|
||
if preferredContentSize != size { | ||
preferredContentSize = size | ||
} | ||
} | ||
} else if swiftUIScreenSizingOptions.contains(.preferredContentSize) { | ||
let size = view.sizeThatFits(view.frame.size) | ||
|
||
if preferredContentSize != size { | ||
preferredContentSize = size | ||
} | ||
} | ||
} | ||
|
||
private func updateSizingOptionsIfNeeded() { | ||
if #available(iOS 16.0, *) { | ||
self.sizingOptions = swiftUIScreenSizingOptions.uiHostingControllerSizingOptions | ||
} | ||
|
||
if !swiftUIScreenSizingOptions.contains(.preferredContentSize), | ||
preferredContentSize != .zero { | ||
preferredContentSize = .zero | ||
} | ||
} | ||
|
||
private func setNeedsLayoutBeforeFirstLayoutIfNeeded() { | ||
if swiftUIScreenSizingOptions.contains(.preferredContentSize) { | ||
// Without manually calling setNeedsLayout here it was observed that a call to | ||
// layoutIfNeeded() immediately after loading the view would not perform a layout, and | ||
// therefore would not update the preferredContentSize in viewDidLayoutSubviews(). | ||
// UI-5797 | ||
view.setNeedsLayout() | ||
} | ||
} | ||
} | ||
|
||
extension SwiftUIScreenSizingOptions { | ||
@available(iOS 16.0, *) | ||
fileprivate var uiHostingControllerSizingOptions: UIHostingControllerSizingOptions { | ||
var options = UIHostingControllerSizingOptions() | ||
|
||
if contains(.preferredContentSize) { | ||
options.insert(.preferredContentSize) | ||
} | ||
|
||
return options | ||
} | ||
} | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
import SwiftUI | ||
import UIKit | ||
import WorkflowSwiftUIExperimental | ||
import XCTest | ||
|
||
final class SwiftUIScreenTests: XCTestCase { | ||
func test_noSizingOptions() { | ||
let viewController = ContentScreen(sizingOptions: []) | ||
.buildViewController(in: .empty) | ||
|
||
viewController.view.layoutIfNeeded() | ||
|
||
XCTAssertEqual(viewController.preferredContentSize, .zero) | ||
} | ||
|
||
func test_preferredContentSize() { | ||
let viewController = ContentScreen(sizingOptions: .preferredContentSize) | ||
.buildViewController(in: .empty) | ||
|
||
viewController.view.layoutIfNeeded() | ||
|
||
XCTAssertEqual( | ||
viewController.preferredContentSize, | ||
.init(width: 42, height: 42) | ||
) | ||
} | ||
|
||
func test_preferredContentSize_sizingOptionsChanges() { | ||
let viewController = ContentScreen(sizingOptions: []) | ||
.buildViewController(in: .empty) | ||
|
||
viewController.view.layoutIfNeeded() | ||
|
||
XCTAssertEqual(viewController.preferredContentSize, .zero) | ||
|
||
ContentScreen(sizingOptions: .preferredContentSize) | ||
.viewControllerDescription(environment: .empty) | ||
.update(viewController: viewController) | ||
|
||
viewController.view.layoutIfNeeded() | ||
|
||
XCTAssertEqual( | ||
viewController.preferredContentSize, | ||
.init(width: 42, height: 42) | ||
) | ||
|
||
ContentScreen(sizingOptions: []) | ||
.viewControllerDescription(environment: .empty) | ||
.update(viewController: viewController) | ||
|
||
viewController.view.layoutIfNeeded() | ||
|
||
XCTAssertEqual(viewController.preferredContentSize, .zero) | ||
} | ||
} | ||
|
||
private struct ContentScreen: SwiftUIScreen { | ||
let sizingOptions: SwiftUIScreenSizingOptions | ||
|
||
static func makeView(model: ObservableValue<ContentScreen>) -> some View { | ||
Color.clear | ||
.frame(width: 42, height: 42) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: is this new and necessary for the PCS behavior we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, whoops, meant to call this out or pull it into a separate PR...
It's not necessary to achieve the goal of this PR, but IMO it's generally expected, and annoying to work around if you want to have a transparent background when working with
Screens
like this. We also set this inWorkflowHostingController
.Epoxy sets this on their hosting controller and they included a comment describing their motivation:
Happy to move this to a separate PR if we'd prefer!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining. Makes sense to me, and not important to break out IMO 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's worth cribbing that comment into our implementation too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth adding a similar comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, maybe? I think it's probably pretty self-explanatory.
Happy to add one if anyone feels strongly though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 comments about it seems like a signal that I should add the comment—I'll do so in a follow up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#278