-
-
Notifications
You must be signed in to change notification settings - Fork 112
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
Add Layout
protocol for FiberReconciler
#498
Conversation
Here are the results of the TokamakCoreBenchmark with layout enabled:
|
Actually, seems like 2f018a2 made the biggest difference. I didn't realize I hadn't run the benchmarks against that commit. I reverted the Here is the new graph: |
I've added a new I only use grayscale colors in those tests because the RGB colors don't quite match between the images, and we really just want to test the actual view geometry. |
I've adjusted the reconciler to perform updates from the root node when layout is enabled, because otherwise the If anyone has a better solution to this, let me know. |
@@ -0,0 +1,339 @@ | |||
// Copyright 2022 Tokamak contributors |
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.
Should this file be named ReconcilerPass.swift
, not ReconcilePass.swift
?
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.
The filename matches the struct in this file. Unless you think the struct should also be called Reconcile**r**Pass
not ReconcilePass
.
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.
Yeah, I think it's a little bit confusing that it conforms to FiberReconcilerPass
, but the name of the struct uses verb form, not noun form. I also would expect the name of the struct to be more specific than the name of the struct, but I don't have a good suggestion for a new name right now.
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.
Seems legit, great work!
I only have formatting and doc comment nits.
public func render<A: App>(_ app: A) -> String { | ||
_ = FiberReconciler(self, app) | ||
return """ | ||
<!doctype html> | ||
<html> | ||
\(rootElement.description) | ||
</html> | ||
""" | ||
} | ||
|
||
public func render<V: View>(_ view: V) -> String { | ||
_ = FiberReconciler(self, view) | ||
return """ | ||
<!doctype html> | ||
<html> | ||
\(rootElement.description) | ||
</html> | ||
""" | ||
} |
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.
will the presence of doctype
here make #489 obsolete?
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.
I believe that PR deals with the stack reconciler StaticHTMLRenderer
, not this fiber reconciler one.
Although this could be enhanced to support meta tags in the head and such once preferences are supported in the fiber reconciler.
… slightly optimize stack usage
I made a few changes to (1) move the LayoutSubview logic into the type instead of in the I was hoping this could help with stack overflows when using dynamic layout. But now I think the only real way to solve this is to make the calls into |
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.
Great, thanks!
This adds support for the new
Layout
protocol introduced in iOS 16 and aligned releases.The
FiberReconciler
's layout code has been refactored to use this protocol in place of the previousLayoutComputer
implementation. In order to match the behavior of native SwiftUI, I split the reconciling and layout passes into two separate loops. The reason for this change is that all of theLayoutSubview
s must be collected prior to performing any layout computations. However, the previousLayoutComputer
approach built the size incrementally as theView
s appeared.VStack
/HStack
, and otherLayoutComputer
s have been rewritten to use theLayout
protocol. I specifically worked on getting the stacks to match SwiftUI as closely as possible, other layouts may need tweaking.TODOs:
LayoutComputer
Layout
protocol (possibly test against the computed frames)ViewSpacing
(for instance,Text
seems to prefer no spacing on the top/bottom, but default spacing on the leading/trailing edges)Layout.Cache
does not actually cache between runsLayoutValueKey