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.

Sign up for GitHub

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

feat: flexible SwiftUI preferredContentSize calc [UI-7665] #320

Merged
merged 3 commits into from
Jan 31, 2025
Merged

feat: flexible SwiftUI preferredContentSize calc [UI-7665] #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c1bf3c8
feat: flexible swiftui preferredContentSize calc
watt Jan 28, 2025
7ee2295
test: swiftui preferredcontentsize
watt Jan 30, 2025
47cc3b6
chore: update comment about background color
watt Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 32 additions & 39 deletions WorkflowSwiftUI/Sources/ObservableScreen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,15 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
var swiftUIScreenSizingOptions: SwiftUIScreenSizingOptions {
didSet {
updateSizingOptionsIfNeeded()

if !hasLaidOutOnce {
if isViewLoaded {
setNeedsLayoutBeforeFirstLayoutIfNeeded()
}
}
}

private var hasLaidOutOnce = false
private var maxFrameWidth: CGFloat = 0
private var maxFrameHeight: CGFloat = 0

init(
setModel: @escaping (Model) -> Void,
Expand Down Expand Up @@ -132,10 +133,9 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
override func viewDidLoad() {
super.viewDidLoad()

// `UIHostingController`'s provides a system background color by default. In order to
// support `ObervableModelScreen`s being composed in contexts where it is composed within another
// view controller where a transparent background is more desirable, we set the background
// to clear to allow this kind of flexibility.
// `UIHostingController` provides a system background color by default. We set the
// background to clear to support contexts where it is composed within another view
// controller.
view.backgroundColor = .clear

setNeedsLayoutBeforeFirstLayoutIfNeeded()
Expand All @@ -146,21 +146,31 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon

defer { hasLaidOutOnce = true }

if #available(iOS 16.0, *) {
Copy link
Contributor

Choose a reason for hiding this comment

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

was the availability change intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. Previously we were letting UIHostingController handle PCS on iOS 16+, except for the first layout. This if handled the first layout case, and the else was for ≤15. Now we're handling PCS in all cases.

// 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 swiftUIScreenSizingOptions.contains(.preferredContentSize) {
// Use the largest frame ever laid out in as a constraint for preferredContentSize
// measurements.
let width = max(view.frame.width, maxFrameWidth)
let height = max(view.frame.height, maxFrameHeight)

maxFrameWidth = width
maxFrameHeight = height

let fixedSize = CGSize(width: width, height: height)

// Measure a few different ways to account for ScrollView behavior. ScrollViews will
// always greedily fill the space available, but will report the natural content size
// when given an infinite size. By combining the results of these measurements we can
// deduce the natural size of content that scrolls in either direction, or both, or
// neither.

let fixedResult = view.sizeThatFits(fixedSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea what the perf cost is for these repeated measurements? I wonder if we could introduce our own sizing options that would allow us to do fewer calls based on more specific sizing configurations.

Copy link
Contributor

Choose a reason for hiding this comment

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

also, is the new logic specifically for scroll views? if so, do we want to do some of these extra calculations conditionally? or does it have to account for them being embedded as descendant views?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any idea what the perf cost is for these repeated measurements

Unknown, but my suspicion is it's fine due to SwiftUI layout being much better than what we're used to.

also, is the new logic specifically for scroll views? if so, do we want to do some of these extra calculations conditionally? or does it have to account for them being embedded as descendant views?

We do need to calculate PCS regardless. The multiple measurements are to account for scroll views. Unfortunately we have to account for there being a scroll view anywhere in the content, and there's just no way to know.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately we have to account for there being a scroll view anywhere in the content, and there's just no way to know.

It'd be a science experiment, but I wonder how feasible it'd be to push the measurements onto the SwiftUI side with a custom Layout and then bubble up context like 'hasScrollView'/'hasMultilineText' via preferences so we could be slightly smarter with our measurements.

I think I'd rather spend the time submitting feedback to Apple to hopefully make this better directly out of the box with the hosting controller though!

let unboundedHorizontalResult = view.sizeThatFits(CGSize(width: .infinity, height: fixedSize.height))
let unboundedVerticalResult = view.sizeThatFits(CGSize(width: fixedSize.width, height: .infinity))

let size = CGSize(
width: min(fixedResult.width, unboundedHorizontalResult.width),
height: min(fixedResult.height, unboundedVerticalResult.height)
)
Comment on lines +170 to +173
Copy link
Contributor

Choose a reason for hiding this comment

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

any chance we need to (or should) handle weird/unexpected values here? is sizeThatFits guaranteed to return sensical results? i guess the concern would be if we end up assigning a preferred content size that doesn't 'make sense' – would that matter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

any chance we need to (or should) handle weird/unexpected values here? is sizeThatFits guaranteed to return sensical results? i guess the concern would be if we end up assigning a preferred content size that doesn't 'make sense' – would that matter?

It's not guaranteed to do anything by API contract, but based on observation it seems like SwiftUI will return sensible values.


if preferredContentSize != size {
preferredContentSize = size
Expand All @@ -175,10 +185,6 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
}

private func updateSizingOptionsIfNeeded() {
if #available(iOS 16.0, *) {
self.sizingOptions = swiftUIScreenSizingOptions.uiHostingControllerSizingOptions
}

if !swiftUIScreenSizingOptions.contains(.preferredContentSize),
preferredContentSize != .zero
{
Expand All @@ -187,7 +193,7 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
}

private func setNeedsLayoutBeforeFirstLayoutIfNeeded() {
if swiftUIScreenSizingOptions.contains(.preferredContentSize) {
if swiftUIScreenSizingOptions.contains(.preferredContentSize), !hasLaidOutOnce {
// 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().
Expand All @@ -203,17 +209,4 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
}
}

extension SwiftUIScreenSizingOptions {
@available(iOS 16.0, *)
fileprivate var uiHostingControllerSizingOptions: UIHostingControllerSizingOptions {
var options = UIHostingControllerSizingOptions()

if contains(.preferredContentSize) {
options.insert(.preferredContentSize)
}

return options
}
}

#endif
123 changes: 123 additions & 0 deletions WorkflowSwiftUI/Tests/PreferredContentSizeTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
#if canImport(UIKit)

import SwiftUI
import Workflow
import WorkflowSwiftUI
import XCTest

final class PreferredContentSizeTests: XCTestCase {
func test_preferredContentSize() {
let maxWidth: CGFloat = 50
let maxHeight: CGFloat = 50

func assertPreferredContentSize(in axes: Axis.Set) {
let screen = TestScreen(model: .constant(state: State(axes: axes)))
let viewController = screen.buildViewController(in: .empty)
viewController.view.frame = CGRect(x: 0, y: 0, width: maxWidth, height: maxHeight)
viewController.view.layoutIfNeeded()

func assertContentSize(
_ contentSize: CGSize,
expected: CGSize? = nil,
file: StaticString = #filePath,
line: UInt = #line
) {
let state = State(width: contentSize.width, height: contentSize.height, axes: axes)
let screen = TestScreen(model: .constant(state: state))
screen.update(viewController: viewController, with: .empty)

viewController.view.layoutIfNeeded()
let pcs = viewController.preferredContentSize

XCTAssertEqual(
pcs,
expected ?? contentSize,
"Axes: \(axes.testDescription)",
file: file,
line: line
)
}

assertContentSize(CGSize(width: 20, height: 20))
assertContentSize(CGSize(width: 40, height: 20))
assertContentSize(CGSize(width: 20, height: 40))
assertContentSize(
CGSize(width: 100, height: 100),
expected: CGSize(
width: axes.contains(.horizontal) ? maxWidth : 100,
height: axes.contains(.vertical) ? maxHeight : 100
)
)
}

assertPreferredContentSize(in: [])
assertPreferredContentSize(in: .horizontal)
assertPreferredContentSize(in: .vertical)
assertPreferredContentSize(in: [.horizontal, .vertical])
}
}

extension Axis.Set {
var testDescription: String {
switch self {
case .horizontal: "[horizontal]"
case .vertical: "[vertical]"
case [.horizontal, .vertical]: "[horizontal, vertical]"
default: "[]"
}
}
}

@ObservableState
private struct State {
var width: CGFloat = 0
var height: CGFloat = 0
var axes: Axis.Set = []
}

private struct TestWorkflow: Workflow {
typealias Rendering = StateAccessor<State>

func makeInitialState() -> State {
State()
}

func render(state: State, context: RenderContext<TestWorkflow>) -> Rendering {
context.makeStateAccessor(state: state)
}
}

private struct TestScreen: ObservableScreen {
typealias Model = StateAccessor<State>

var model: Model

var sizingOptions: SwiftUIScreenSizingOptions = .preferredContentSize

@ViewBuilder
static func makeView(store: Store<Model>) -> some View {
TestView(store: store)
}
}

private struct TestView: View {
var store: Store<StateAccessor<State>>

var body: some View {
WithPerceptionTracking {
if store.axes.isEmpty {
box
} else {
ScrollView(store.axes) {
box
}
}
}
}
Comment on lines +103 to +116
Copy link
Member

Choose a reason for hiding this comment

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

I'd love if multiline text was also included in the test cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would be hard to do without making the test fragile.


var box: some View {
Color.red.frame(width: store.width, height: store.height)
}
}

#endif
71 changes: 32 additions & 39 deletions WorkflowSwiftUIExperimental/Sources/SwiftUIScreen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,15 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
var swiftUIScreenSizingOptions: SwiftUIScreenSizingOptions {
didSet {
updateSizingOptionsIfNeeded()

if !hasLaidOutOnce {
if isViewLoaded {
setNeedsLayoutBeforeFirstLayoutIfNeeded()
}
}
}

private var hasLaidOutOnce = false
private var maxFrameWidth: CGFloat = 0
private var maxFrameHeight: CGFloat = 0

init(
modelSink: Sink<Model>,
Expand All @@ -130,10 +131,9 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
override func viewDidLoad() {
super.viewDidLoad()

// `UIHostingController`'s provides a system background color by default. In order to
// support `SwiftUIScreen`s being composed in contexts where it is composed within another
// view controller where a transparent background is more desirable, we set the background
// to clear to allow this kind of flexibility.
// `UIHostingController` provides a system background color by default. We set the
// background to clear to support contexts where it is composed within another view
// controller.
view.backgroundColor = .clear

setNeedsLayoutBeforeFirstLayoutIfNeeded()
Expand All @@ -144,21 +144,31 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon

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 swiftUIScreenSizingOptions.contains(.preferredContentSize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is the path we want to go, shall we extract the duplicate calculation logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm following the rule of 3 here, since these two don't currently share any common place to extract to. Plus we'll probably deprecate SwiftUIScreen once ObservableScreen is more established.

// Use the largest frame ever laid out in as a constraint for preferredContentSize
// measurements.
let width = max(view.frame.width, maxFrameWidth)
let height = max(view.frame.height, maxFrameHeight)

maxFrameWidth = width
maxFrameHeight = height

let fixedSize = CGSize(width: width, height: height)

// Measure a few different ways to account for ScrollView behavior. ScrollViews will
// always greedily fill the space available, but will report the natural content size
// when given an infinite size. By combining the results of these measurements we can
// deduce the natural size of content that scrolls in either direction, or both, or
// neither.

let fixedResult = view.sizeThatFits(fixedSize)
let unboundedHorizontalResult = view.sizeThatFits(CGSize(width: .infinity, height: fixedSize.height))
let unboundedVerticalResult = view.sizeThatFits(CGSize(width: fixedSize.width, height: .infinity))

let size = CGSize(
width: min(fixedResult.width, unboundedHorizontalResult.width),
height: min(fixedResult.height, unboundedVerticalResult.height)
)

if preferredContentSize != size {
preferredContentSize = size
Expand All @@ -173,10 +183,6 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
}

private func updateSizingOptionsIfNeeded() {
if #available(iOS 16.0, *) {
self.sizingOptions = swiftUIScreenSizingOptions.uiHostingControllerSizingOptions
}

if !swiftUIScreenSizingOptions.contains(.preferredContentSize),
preferredContentSize != .zero
{
Expand All @@ -185,7 +191,7 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
}

private func setNeedsLayoutBeforeFirstLayoutIfNeeded() {
if swiftUIScreenSizingOptions.contains(.preferredContentSize) {
if swiftUIScreenSizingOptions.contains(.preferredContentSize), !hasLaidOutOnce {
// 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().
Expand All @@ -201,17 +207,4 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
}
}

extension SwiftUIScreenSizingOptions {
@available(iOS 16.0, *)
fileprivate var uiHostingControllerSizingOptions: UIHostingControllerSizingOptions {
var options = UIHostingControllerSizingOptions()

if contains(.preferredContentSize) {
options.insert(.preferredContentSize)
}

return options
}
}

#endif
Loading