[UIKitBackend] UIViewControllerRepresentable and iPad-only split views
#104
Conversation
Sources/UIKitBackend/Widget.swift
Outdated
| func updateLeftConstraint() { | ||
| leftConstraint?.isActive = false | ||
| guard let superview = view.superview else { return } | ||
| leftConstraint = view.leftAnchor.constraint( | ||
| equalTo: superview.safeAreaLayoutGuide.leftAnchor, constant: CGFloat(x)) | ||
| // Set the constraint priority for leftConstraint (and topConstraint) to just under | ||
| // "required" so that we don't get warnings about unsatisfiable constraints from | ||
| // scroll views, which position relative to their contentLayoutGuide instead. | ||
| // This *should* be high enough that it won't cause any problems unless there was | ||
| // a constraint conflict anyways. | ||
| leftConstraint!.priority = .init(UILayoutPriority.required.rawValue - 1.0) | ||
| leftConstraint!.isActive = true | ||
| } |
There was a problem hiding this comment.
I reckon it'd be better to update the existing constraint if it already exists, rather than just making it inactive and creating a new one. Over time the view will probably build up a lot of inactive constraints which would be taking up memory and probably time in skipping over them too. And if UIKit stores the constraints in some sort of weak reference array thing then the overhead is in allocations and shuffling constraints in and out rather than memory.
There was a problem hiding this comment.
The documentation backs this up:
Setting the constant on an existing constraint performs much better than removing the constraint and adding a new one that's exactly like the old except that it has a different constant.
I will update this accordingly, though it's slightly more annoying than it seems at first as I still have to deactivate the old constraint (and possibly create a new one) in some cases.
Sources/UIKitBackend/Widget.swift
Outdated
| // scroll views, which position relative to their contentLayoutGuide instead. | ||
| // This *should* be high enough that it won't cause any problems unless there was | ||
| // a constraint conflict anyways. | ||
| leftConstraint!.priority = .init(UILayoutPriority.required.rawValue - 1.0) |
There was a problem hiding this comment.
I generally use an intermediate non-optional variable in situations like this to avoid the force unwraps (cause the unwraps make it look scary and like it needs extra attention, when in this case it's actually never gonna be nil). I know it probably seems silly given how many force unwraps are scattered throughout the backends lol, but generally I try to limit the force unwraps in backend implementations to situations where the alternative would be a bunch of error handling code that would probably just exit the program anyway (someday it'd probably make sense to get rid of all the force unwraps and replace them with code that displays a dialog and then exits, but that'd be a future thing).
There was a problem hiding this comment.
Did this as part of the refactor mentioned in the previous comment.
| /// | ||
| /// The default implementation does nothing. |
There was a problem hiding this comment.
I generally put the discussion above the parameters (with a blank line between the discussion and the summary). It's possible that DocC supports this too, but best to keep things consistent.
There was a problem hiding this comment.
I did it in this order because this is the order the compiled documentation renders in, but yeah I can definitely change it.
| /// property is what frame the view will actually be rendered with if the current layout | ||
| /// pass is not a dry run, while the other properties are used to inform the layout engine | ||
| /// how big or small the view can be. The ``SwiftCrossUI/ViewSize/idealSize`` property | ||
| /// should not vary with the `proposal`, and should only depend on the view's contents. | ||
| /// Pass `nil` for the maximum width/height if the view has no maximum size (and therefore | ||
| /// may occupy the entire screen). |
There was a problem hiding this comment.
Indent the wrapped lines by 2 spaces
There was a problem hiding this comment.
Not 100% sure what you meant by this, did I do the correct thing here?
| innerChild.rightAnchor.constraint(equalTo: child.contentLayoutGuide.rightAnchor), | ||
| ]) | ||
| override func viewWillLayoutSubviews() { | ||
| NSLayoutConstraint.activate(contentLayoutGuideConstraints) |
There was a problem hiding this comment.
activate probably only has to be called once at initialisation, and I'm not entirely certain on this but viewWillLayoutSubviews probably runs more than once. Could maybe move this to loadView?
There was a problem hiding this comment.
From what I remember, I tried putting it in loadView, and it crashed because the view hierarchy was insufficiently assembled at that point. I can double-check if it works in viewDidLoad, but ultimately activating the same instance of NSLayoutConstraint multiple times is harmless.
There was a problem hiding this comment.
Confirmed. Moving this to viewDidLoad causes the app to crash, with
Thread 1: "Unable to activate constraint with anchors <NSLayoutXAxisAnchor:0x60000178c7c0 \"_UIScrollViewLayoutGuide:0x600003b38e00'UIScrollView-contentLayoutGuide'.leading\"> and <NSLayoutXAxisAnchor:0x60000178c800 \"UIKitBackend.BaseViewWidget:0x10f912fd0.leading\"> because they have no common ancestor. Does the constraint or its anchors reference items in different view hierarchies? That's illegal."
However, updateViewConstraints also works, is called less frequently, and seems more appropriate, so I'll move it to there.
This PR contains a significant restructuring to allow widgets to be
UIViews orUIViewControllers. I used this to implement two features:UIViewControllerRepresentable, and split views. While implementingUIViewControllerRepresentableI discovered a way to improve the sizing algorithm forUIViewRepresentableso that is also in this PR.Split views are currently only supported on iPad, where they actually work.
To avoid these issues for now,
createSplitViewcrashes if it detects it's running on an unsupported system. We may revisit split views on mobile & TV in the future.Since the scroll widget is now a view controller, I debated making it the
UIScollView's delegate, but I decided against that as we wouldn't have any need to implement any of the delegate methods yet.