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

Copy yogaChildren in accessor method. Avoid using accessor method internally #1283

Merged
merged 2 commits into from
Dec 20, 2018
+31 −10
Merged

Copy yogaChildren in accessor method. Avoid using accessor method internally #1283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b12b7a
Copy yogaChildren in accessor method. Avoid using accessor method int…
maicki Dec 13, 2018
732460e
Further improvements
maicki Dec 18, 2018
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
4 changes: 2 additions & 2 deletions Source/ASDisplayNode+Beta.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,7 @@ AS_EXTERN void ASDisplayNodePerformBlockOnEveryYogaChild(ASDisplayNode * _Nullab

@interface ASDisplayNode (Yoga)

// TODO: Make this and yogaCalculatedLayout atomic (lock).
@property (nullable, nonatomic) NSArray *yogaChildren;
@property (copy) NSArray *yogaChildren;

- (void)addYogaChild:(ASDisplayNode *)child;
- (void)removeYogaChild:(ASDisplayNode *)child;
Expand All @@ -198,6 +197,7 @@ AS_EXTERN void ASDisplayNodePerformBlockOnEveryYogaChild(ASDisplayNode * _Nullab
- (void)semanticContentAttributeDidChange:(UISemanticContentAttribute)attribute;

@property BOOL yogaLayoutInProgress;
// TODO: Make this atomic (lock).
@property (nullable, nonatomic) ASLayout *yogaCalculatedLayout;

// Will walk up the Yoga tree and returns the root node
Expand Down
33 changes: 26 additions & 7 deletions Source/ASDisplayNode+Yoga.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,32 +48,43 @@ - (void)setYogaChildren:(NSArray *)yogaChildren
for (ASDisplayNode *child in [_yogaChildren copy]) {
// 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];
[self _locked_removeYogaChild:child];
}
_yogaChildren = nil;
for (ASDisplayNode *child in yogaChildren) {
[self addYogaChild:child];
[self _locked_addYogaChild:child];
}
}

- (NSArray *)yogaChildren
{
return _yogaChildren;
ASLockScope(self.yogaRoot);
return [_yogaChildren copy] ?: @[];
Copy link
Member

Choose a reason for hiding this comment

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

We already lock on the setter, why not go ahead and lock here and declare the property atomic?

Copy link
Member

Choose a reason for hiding this comment

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

Because this diff was copied from an 11th-hour internal hotfix! But you're right, we should do it now. @maicki mind making the change and landing it with this?

Copy link
Member

Choose a reason for hiding this comment

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

I knew I'd been here before...

}

- (void)addYogaChild:(ASDisplayNode *)child
{
ASLockScope(self.yogaRoot);
[self _locked_addYogaChild:child];
}

- (void)_locked_addYogaChild:(ASDisplayNode *)child
{
[self insertYogaChild:child atIndex:_yogaChildren.count];
}

- (void)removeYogaChild:(ASDisplayNode *)child
{
ASLockScope(self.yogaRoot);
[self _locked_removeYogaChild:child];
}

- (void)_locked_removeYogaChild:(ASDisplayNode *)child
{
if (child == nil) {
return;
}

[_yogaChildren removeObjectIdenticalTo:child];

// YGNodeRef removal is done in setParent:
Expand All @@ -83,6 +94,11 @@ - (void)removeYogaChild:(ASDisplayNode *)child
- (void)insertYogaChild:(ASDisplayNode *)child atIndex:(NSUInteger)index
{
ASLockScope(self.yogaRoot);
[self _locked_insertYogaChild:child atIndex:index];
}

- (void)_locked_insertYogaChild:(ASDisplayNode *)child atIndex:(NSUInteger)index
{
if (child == nil) {
return;
}
Expand All @@ -91,14 +107,16 @@ - (void)insertYogaChild:(ASDisplayNode *)child atIndex:(NSUInteger)index
}

// Clean up state in case this child had another parent.
[self removeYogaChild:child];
[self _locked_removeYogaChild:child];

[_yogaChildren insertObject:child atIndex:index];

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

#pragma mark - Subclass Hooks

- (void)semanticContentAttributeDidChange:(UISemanticContentAttribute)attribute
{
UIUserInterfaceLayoutDirection layoutDirection =
Expand Down Expand Up @@ -168,8 +186,9 @@ - (void)setupYogaCalculatedLayout

YGNodeRef yogaNode = self.style.yogaNode;
uint32_t childCount = YGNodeGetChildCount(yogaNode);
ASDisplayNodeAssert(childCount == self.yogaChildren.count,
@"Yoga tree should always be in sync with .yogaNodes array! %@", self.yogaChildren);
ASDisplayNodeAssert(childCount == _yogaChildren.count,
@"Yoga tree should always be in sync with .yogaNodes array! %@",
_yogaChildren);

ASLayout *rawSublayouts[childCount];
int i = 0;
Expand Down
4 changes: 3 additions & 1 deletion Source/Layout/ASYogaUtilities.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ void ASDisplayNodePerformBlockOnEveryYogaChild(ASDisplayNode *node, void(^block)
return;
}
block(node);
for (ASDisplayNode *child in [node yogaChildren]) {
// We use the accessor here despite the copy, because the block may modify the yoga tree e.g.
// replacing a node.
for (ASDisplayNode *child in node.yogaChildren) {
ASDisplayNodePerformBlockOnEveryYogaChild(child, block);
}
}
Expand Down