Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add setNeedsLayout to yoga tree changes. #1361

Merged
merged 1 commit into from
Mar 5, 2019
Merged

Add setNeedsLayout to yoga tree changes. #1361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion Source/ASDisplayNode+Layout.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@
#import <AsyncDisplayKit/ASNodeController+Beta.h>
#import <AsyncDisplayKit/ASDisplayNode+Yoga.h>

@interface ASDisplayNode (ASLayoutElementStyleDelegate) <ASLayoutElementStyleDelegate>
@end

@implementation ASDisplayNode (ASLayoutElementStyleDelegate)

#pragma mark <ASLayoutElementStyleDelegate>

- (void)style:(ASLayoutElementStyle *)style propertyDidChange:(NSString *)propertyName {
[self setNeedsLayout];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some comments or context here? This is causing us infinite layout loops. Is it applicable to non-yoga?

Copy link
Member

@garrettmoon garrettmoon Mar 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually doesn't seem unreasonable, but it breaks ASTextNode which sets its style during layout 🙄 (calculateSizeThatFits:).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every time style:propertyDidChange: is called one of the layout properties changed. If some layout related properties (or some other related properties like e.g. attributed texton anASTextNode` are changing the node needs to be informed that a new layout pass should happen. The most obvious way is to do it in central place like here.

We should fix the ASTextNode issue though ... we should try getting setting any style properties out of size calculation. calculateSizeThatFits: should just use the current state and return the size based on it and don't adjust anything in the world around it as you could just call calculateSizeThatFits: without actually changing anything on the node therefore the next time it would draw with a different bounds as the constrainedSize passed in the ascender and descender could be wrong?

@rcancro Any idea how we could get adjusting the ascender / descender out of calculateSizeThatFits:. We should try to get the current renderer in setAttributedText: or if the bounds changes and update the ascender / descender with it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until a fix for ASTextNode is made available this should be reverted?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • -setAscender: only informs the delegate, if the style actually changed.
  • ASTextNode actually sets it twice, to different values, during layout so that's why we have an infinite loop.
  • Fixing the above would still mean double-layout.
  • I think it's correct to say that layout invalidations that happen DURING layout should always be ignored, if all operations on the tree used the same lock.
  • Since it doesn't, can we hack around this by using the ASLayoutElementContext thing? Does the presence of that signal that we are calculating a layout? @maicki
  • If we can't come up with anything before tomorrow morning, I'm OK with reverting this for now since ASTextNode is such a core type.

Copy link
Contributor

@maicki maicki Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I will get something going that does mean we don't have to revert it completely
  • ... and will follow with the ASLayoutElementContext question

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks y'all!

}

@end

#pragma mark - ASDisplayNode (ASLayoutElement)

@implementation ASDisplayNode (ASLayoutElement)
Expand All @@ -42,8 +55,9 @@ - (ASLayoutElementStyle *)style

- (ASLayoutElementStyle *)_locked_style
{
ASAssertLocked(__instanceLock__);
if (_style == nil) {
_style = [[ASLayoutElementStyle alloc] init];
_style = [[ASLayoutElementStyle alloc] initWithDelegate:self];
}
return _style;
}
Expand Down
2 changes: 2 additions & 0 deletions Source/ASDisplayNode+Yoga.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ - (void)_locked_removeYogaChild:(ASDisplayNode *)child

// YGNodeRef removal is done in setParent:
child.yogaParent = nil;
[self setNeedsLayout];
}

- (void)insertYogaChild:(ASDisplayNode *)child atIndex:(NSUInteger)index
Expand All @@ -115,6 +116,7 @@ - (void)_locked_insertYogaChild:(ASDisplayNode *)child atIndex:(NSUInteger)index

// YGNodeRef insertion is done in setParent:
child.yogaParent = self;
[self setNeedsLayout];
}

#pragma mark - Subclass Hooks
Expand Down
Loading