-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[Yoga] Refine the handling of measurement functions when Yoga is used. #408
Changes from all commits
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 |
---|---|---|
|
@@ -40,7 +40,7 @@ @implementation ASDisplayNode (Yoga) | |
|
||
- (void)setYogaChildren:(NSArray *)yogaChildren | ||
{ | ||
for (ASDisplayNode *child in _yogaChildren) { | ||
for (ASDisplayNode *child in [_yogaChildren copy]) { | ||
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. This is important - if this method was called previously with a non-nil _yogaChildren (not too common), then it would always crash due to mutate-while-iterating. |
||
// Make sure to un-associate the YGNodeRef tree before replacing _yogaChildren | ||
// If this becomes a performance bottleneck, it can be optimized by not doing the NSArray removals here. | ||
[self removeYogaChild:child]; | ||
|
@@ -68,14 +68,8 @@ - (void)addYogaChild:(ASDisplayNode *)child | |
// Clean up state in case this child had another parent. | ||
[self removeYogaChild:child]; | ||
|
||
BOOL hadZeroChildren = (_yogaChildren.count == 0); | ||
|
||
[_yogaChildren addObject:child]; | ||
|
||
// Ensure any measure function is removed before inserting the YGNodeRef child. | ||
if (hadZeroChildren) { | ||
[self updateYogaMeasureFuncIfNeeded]; | ||
} | ||
// YGNodeRef insertion is done in setParent: | ||
child.yogaParent = self; | ||
} | ||
|
@@ -86,15 +80,10 @@ - (void)removeYogaChild:(ASDisplayNode *)child | |
return; | ||
} | ||
|
||
BOOL hadChildren = (_yogaChildren.count > 0); | ||
[_yogaChildren removeObjectIdenticalTo:child]; | ||
|
||
// YGNodeRef removal is done in setParent: | ||
child.yogaParent = nil; | ||
// Ensure any measure function is re-added after removing the YGNodeRef child. | ||
if (hadChildren && _yogaChildren.count == 0) { | ||
[self updateYogaMeasureFuncIfNeeded]; | ||
} | ||
} | ||
|
||
- (void)semanticContentAttributeDidChange:(UISemanticContentAttribute)attribute | ||
|
@@ -144,6 +133,7 @@ - (ASLayout *)yogaCalculatedLayout | |
- (void)setYogaLayoutInProgress:(BOOL)yogaLayoutInProgress | ||
{ | ||
setFlag(YogaLayoutInProgress, yogaLayoutInProgress); | ||
[self updateYogaMeasureFuncIfNeeded]; | ||
} | ||
|
||
- (BOOL)yogaLayoutInProgress | ||
|
@@ -184,8 +174,14 @@ - (void)updateYogaMeasureFuncIfNeeded | |
{ | ||
// Size calculation via calculateSizeThatFits: or layoutSpecThatFits: | ||
// This will be used for ASTextNode, as well as any other node that has no Yoga children | ||
id <ASLayoutElement> layoutElementToMeasure = (self.yogaChildren.count == 0 ? self : nil); | ||
ASLayoutElementYogaUpdateMeasureFunc(self.style.yogaNode, layoutElementToMeasure); | ||
BOOL isLeafNode = (self.yogaChildren.count == 0); | ||
BOOL definesCustomLayout = [self implementsLayoutMethod]; | ||
|
||
// We set the measure func only during layout. Otherwise, a cycle is created: | ||
// The YGNodeRef Context will retain the ASDisplayNode, which retains the style, which owns the YGNodeRef. | ||
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. Thankfully I had fixed this a while ago and it was introduced only shortly before that, but just now getting around to upstreaming this. |
||
BOOL shouldHaveMeasureFunc = (isLeafNode && definesCustomLayout && checkFlag(YogaLayoutInProgress)); | ||
|
||
ASLayoutElementYogaUpdateMeasureFunc(self.style.yogaNode, shouldHaveMeasureFunc ? self : nil); | ||
} | ||
|
||
- (void)invalidateCalculatedYogaLayout | ||
|
@@ -214,7 +210,6 @@ - (void)calculateLayoutFromYogaRoot:(ASSizeRange)rootConstrainedSize | |
// Prepare all children for the layout pass with the current Yoga tree configuration. | ||
ASDisplayNodePerformBlockOnEveryYogaChild(self, ^(ASDisplayNode * _Nonnull node) { | ||
node.yogaLayoutInProgress = YES; | ||
[node updateYogaMeasureFuncIfNeeded]; | ||
}); | ||
|
||
if (ASSizeRangeEqualToSizeRange(rootConstrainedSize, ASSizeRangeUnconstrained)) { | ||
|
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.
lol strange change. This will cause a spurious merge conflict with #399 (which improves the perf) so better to revert it.