[Yoga] Refine the handling of measurement functions when Yoga is used. #408
Conversation
Source/ASDisplayNode+Yoga.mm
Outdated
| @@ -36,11 +36,15 @@ @interface ASDisplayNode (YogaInternal) | |||
| - (ASSizeRange)_locked_constrainedSizeForLayoutPass; | |||
| @end | |||
|
|
|||
| @interface ASLayout (YogaInternal) | |||
| @property (nonatomic, getter=isFlattened) BOOL flattened; | |||
There was a problem hiding this comment.
@nguyenhuy could you explain why the diff to this file in this other PR is correct?
https://github.com/TextureGroup/Texture/pull/395/files#diff-599cfec3d0b870d11b2965c36e2b6a72
I haven't been able to study the change in detail, but I wanted to be certain that flattening will still be performed correctly for Yoga nodes with this removed.
| @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.
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.
| 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.
Thankfully I had fixed this a while ago and it was introduced only shortly before that, but just now getting around to upstreaming this.
This ensures that measure funcs are not set for container / empty spacer nodes, because Yoga has more internal capabilities than layoutThatFits: knows about. Avoid need for the main layout pass to directly call setup for measure funcs, by making it an implicit part of .yogaLayoutInProgress =. Tear down the measure func after the layout pass to avoid retain cycle.
appleguy
force-pushed
the
YogaMeasureFuncRefinement
branch
from
5b5d3c5
to
e0c1b19
Compare
Generated by 🚫 Danger |
|
@nguyenhuy could you explain briefly why the Flattened management is no longer needed? E.g. was removed in: https://github.com/TextureGroup/Texture/pull/395/files#diff-599cfec3d0b870d11b2965c36e2b6a72 I haven't been able to study the change in detail, but I wanted to be certain that flattening will still be performed correctly for Yoga nodes with this removed. |
| @@ -143,7 +143,7 @@ - (NSString *)asciiArtName | |||
| { | |||
| NSString *string = NSStringFromClass([self class]); | |||
| if (_debugName) { | |||
| string = [string stringByAppendingString:[NSString stringWithFormat:@"\"%@\"", _debugName]]; | |||
| string = [string stringByAppendingString:[NSString stringWithFormat:@"\"%@\"",_debugName]]; | |||
There was a problem hiding this comment.
lol strange change. This will cause a spurious merge conflict with #399 (which improves the perf) so better to revert it.
|
I removed the space because it actually was added accidentally in a recent
diff, and I noticed it when pulling in from upstream. Is it harder for you
to fix the conflict than me to revert? I can revert if you want to remove
it yourself :)
…On Sun, Jul 2, 2017 at 12:17 PM Adlai Holler ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Source/ASDisplayNode+Layout.mm
<#408 (comment)>:
> @@ -143,7 +143,7 @@ - (NSString *)asciiArtName
{
NSString *string = NSStringFromClass([self class]);
if (_debugName) {
- string = [string stringByAppendingString:[NSString stringWithFormat:@"\"%@\"", _debugName]];
+ string = [string stringByAppendingString:[NSString stringWithFormat:@"\"%@\"",_debugName]];
lol strange change. This will cause a spurious merge conflict with #399
<#399> (which improves the
perf) so better to revert it.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#408 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAigA71JJRvJgXMuyrJ027T-WxGu8G35ks5sJ-yugaJpZM4OLgRw>
.
|
|
Eh don't worry about it let's fry other fish. Besides we'll all be dead and gone before the tracing diff gets reviewed #salty |
|
@Adlai-Holler :-D it turns out that I am running into a really shitty issue with layout right now (where image loading indeed causes massive layout churn by ascending to the root, which then also triggers main thread relayout when the collection requests a cell's size -- basically, horribly defeating the point of everything). I logged on to go check out the tracing diff to see if I can utilize it. Sorry it's taken a few days, I still have a lot of other reviews waiting on me both internal and external! |
|
Lol no worries, just whining. Hope the tracing diff helps! |
TextureGroup#408) This ensures that measure funcs are not set for container / empty spacer nodes, because Yoga has more internal capabilities than layoutThatFits: knows about. Avoid need for the main layout pass to directly call setup for measure funcs, by making it an implicit part of .yogaLayoutInProgress =. Tear down the measure func after the layout pass to avoid retain cycle.
This ensures that measure funcs are not set for container / empty spacer
nodes, because Yoga has more internal capabilities than layoutThatFits:
knows about.
Avoid need for the main layout pass to directly call setup for measure
funcs, by making it an implicit part of .yogaLayoutInProgress =.
Tear down the measure func after the layout pass to avoid retain cycle.