Improve separation of code for layout method types #1305
Conversation
Source/ASDisplayNode+Yoga.mm
Outdated
| } | ||
|
|
||
| // Manual size calculation via calculateSizeThatFits: | ||
| CGSize size = [self calculateSizeThatFits:constrainedSize.max]; |
There was a problem hiding this comment.
@appleguy Is this the right logic to fallback to calculateSizeThatFits: or should we call through to the layout spec implementation: calculateLayoutLayoutSpec?
There was a problem hiding this comment.
We should definitely fall back to calculateLayoutThatFits:, as there are some subclasses that implement that directly. By contrast it can also fall back to calculateSizeThatFits: when appropriate, but the reverse is not true.
There was a problem hiding this comment.
Yep what I thought. Should be resolved. Hopefully in the future we can drop this though.
maicki
force-pushed
the
MSLayoutTypesSeparation
branch
2 times, most recently
from
ecf51ba
to
9d1234a
Compare
Source/ASDisplayNode+Layout.mm
Outdated
| @@ -143,19 +144,19 @@ - (NSString *)asciiArtName | |||
|
|
|||
| @implementation ASDisplayNode (ASLayout) | |||
|
|
|||
| - (void)setLayoutSpecBlock:(ASLayoutSpecBlock)layoutSpecBlock | |||
| - (ASLayoutType)layoutType | |||
There was a problem hiding this comment.
Good idea! What do you think about ASLayoutEngineType instead?
ASLayoutType sounds a bit too close to describing the actually-produced ASLayout objects.
There was a problem hiding this comment.
Agree, changed the name.
Source/ASDisplayNode+Yoga.mm
Outdated
| } | ||
|
|
||
| // Manual size calculation via calculateSizeThatFits: | ||
| CGSize size = [self calculateSizeThatFits:constrainedSize.max]; |
There was a problem hiding this comment.
We should definitely fall back to calculateLayoutThatFits:, as there are some subclasses that implement that directly. By contrast it can also fall back to calculateSizeThatFits: when appropriate, but the reverse is not true.
| layout = [layout filteredNodeLayoutTree]; | ||
|
|
||
| return layout; | ||
| return nil; |
There was a problem hiding this comment.
Rather than have default: and the nil fallthrough, what do you think about having an early-return check for Yoga and falling through to ASLayoutSpec in all other cases?
Although that wouldn't be quite as nice if we supported 3 or 4 layout engines, for the time being, we support 2 with the possibility of compiling out one of them. So, it would be especially clean to #if gate the YOGA condition and also avoid the semi-unusual possibility of falling through to this point.
There was a problem hiding this comment.
If you look into the switch statement we will always fall through to the default case. Before I had an assertion in here but now we will break to hit the assertion I added at the end and to point developers to the comment about the supported layout engine.
In theory the nil case at the end should never be reached in the first place as a node's layout engine should always be supported.
I don't think we should change the logic in here. I further added a comment to make this clear and an assertion at the end.
If you think the switch will add some overhead for the non yoga case we could compile out the whole switch statement, but I think for complexity purposes we should keep it as it is.
| NSTimeInterval _layoutComputationTotalTime; | ||
| NSInteger _layoutComputationNumberOfPasses; | ||
|
|
||
|
|
There was a problem hiding this comment.
This seems like too much spacing. Since there's already a comment, should we keep the pattern of 1 blank line + the comment? At most 2 blank?
There was a problem hiding this comment.
Agree with 3 spaces, that was unintended, should have been 2.
To make the code more readable I split up different sections for instance variables though. Two spaces between sections and one space between subsections with a comment.
|
@appleguy Want to give that another look please. Thanks! |
- Delegate to layout spec engine if the node is a layout spec node but yoga engine was asked for calculate the layout - Change ASLayoutType to ASLayoutEngineType - Improve layout engine fall through code
maicki
force-pushed
the
MSLayoutTypesSeparation
branch
from
9afd7d1
to
d43abee
Compare
Generated by 🚫 Danger |
|
Post-review: LGTM and I'm excited to have a cleaner/better separation between the 2 layout engines. We'll test this internally soon. |
Currently most of the layout spec code is tightly coupled to
ASDisplayNodeand yoga related layout logic is guarded by multiple compiler flag.This PR tries to separate layout code logic a bit by moving it into different files and categories on
ASDisplayNode. This is a first small step to separate and should give us a good base for even separate them more.