diff --git a/tests/YGPersistenceTest.cpp b/tests/YGPersistenceTest.cpp index c9d94e89..69bd59b1 100644 --- a/tests/YGPersistenceTest.cpp +++ b/tests/YGPersistenceTest.cpp @@ -6,6 +6,7 @@ */ #include #include +#include TEST(YogaTest, cloning_shared_root) { const YGConfigRef config = YGConfigNew(); @@ -104,7 +105,7 @@ TEST(YogaTest, cloning_shared_root) { YGConfigFree(config); } -TEST(YogaTest, mutating_children_of_a_clone_clones) { +TEST(YogaTest, mutating_children_of_a_clone_clones_only_after_layout) { const YGConfigRef config = YGConfigNew(); const YGNodeRef root = YGNodeNewWithConfig(config); @@ -129,7 +130,7 @@ TEST(YogaTest, mutating_children_of_a_clone_clones) { ASSERT_EQ(1, YGNodeGetChildCount(root2)); ASSERT_EQ(2, YGNodeGetChildCount(root3)); ASSERT_EQ(root3_child1, YGNodeGetChild(root3, 1)); - ASSERT_NE(YGNodeGetChild(root2, 0), YGNodeGetChild(root3, 0)); + ASSERT_EQ(YGNodeGetChild(root2, 0), YGNodeGetChild(root3, 0)); const YGNodeRef root4 = YGNodeClone(root3); ASSERT_EQ(root3_child1, YGNodeGetChild(root4, 1)); @@ -137,7 +138,12 @@ TEST(YogaTest, mutating_children_of_a_clone_clones) { YGNodeRemoveChild(root4, root3_child1); ASSERT_EQ(2, YGNodeGetChildCount(root3)); ASSERT_EQ(1, YGNodeGetChildCount(root4)); + ASSERT_EQ(YGNodeGetChild(root3, 0), YGNodeGetChild(root4, 0)); + + YGNodeCalculateLayout(root4, YGUndefined, YGUndefined, YGDirectionLTR); ASSERT_NE(YGNodeGetChild(root3, 0), YGNodeGetChild(root4, 0)); + YGNodeCalculateLayout(root3, YGUndefined, YGUndefined, YGDirectionLTR); + ASSERT_NE(YGNodeGetChild(root2, 0), YGNodeGetChild(root3, 0)); YGNodeFreeRecursive(root4); YGNodeFreeRecursive(root3); @@ -245,3 +251,34 @@ TEST(YogaTest, cloning_and_freeing) { ASSERT_EQ(initialInstanceCount, YGNodeGetInstanceCount()); } + +TEST(YogaTest, mixed_shared_and_owned_children) { + // Don't try this at home! + + YGNodeRef root0 = YGNodeNew(); + YGNodeRef root1 = YGNodeNew(); + + YGNodeRef root0_child0 = YGNodeNew(); + YGNodeRef root0_child0_0 = YGNodeNew(); + YGNodeInsertChild(root0, root0_child0, 0); + YGNodeInsertChild(root0_child0, root0_child0_0, 0); + + YGNodeRef root1_child0 = YGNodeNew(); + YGNodeRef root1_child2 = YGNodeNew(); + YGNodeInsertChild(root1, root1_child0, 0); + YGNodeInsertChild(root1, root1_child2, 1); + + auto children = root1->getChildren(); + children.insert(children.begin() + 1, root0_child0); + root1->setChildren(children); + auto secondChild = YGNodeGetChild(root1, 1); + ASSERT_EQ(secondChild, YGNodeGetChild(root0, 0)); + ASSERT_EQ(YGNodeGetChild(secondChild, 0), YGNodeGetChild(root0_child0, 0)); + + YGNodeCalculateLayout(root1, YGUndefined, YGUndefined, YGDirectionLTR); + secondChild = YGNodeGetChild(root1, 1); + ASSERT_NE(secondChild, YGNodeGetChild(root0, 0)); + ASSERT_EQ(YGNodeGetOwner(secondChild), root1); + ASSERT_NE(YGNodeGetChild(secondChild, 0), YGNodeGetChild(root0_child0, 0)); + ASSERT_EQ(YGNodeGetOwner(YGNodeGetChild(secondChild, 0)), secondChild); +} diff --git a/yoga/YGNode.cpp b/yoga/YGNode.cpp index 4fcef90e..fedb3b9e 100644 --- a/yoga/YGNode.cpp +++ b/yoga/YGNode.cpp @@ -386,30 +386,7 @@ void YGNode::clearChildren() { // Other Methods void YGNode::cloneChildrenIfNeeded() { - // YGNodeRemoveChild in yoga.cpp has a forked variant of this algorithm - // optimized for deletions. - - const uint32_t childCount = static_cast(children_.size()); - if (childCount == 0) { - // This is an empty set. Nothing to clone. - return; - } - - const YGNodeRef firstChild = children_.front(); - if (firstChild->getOwner() == this) { - // If the first child has this node as its owner, we assume that it is - // already unique. We can do this because if we have it has a child, that - // means that its owner was at some point cloned which made that subtree - // immutable. We also assume that all its sibling are cloned as well. - return; - } - - for (uint32_t i = 0; i < childCount; ++i) { - const YGNodeRef oldChild = children_[i]; - YGNodeRef newChild = config_->cloneNode(oldChild, this, i); - replaceChild(newChild, i); - newChild->setOwner(this); - } + iterChildrenAfterCloningIfNeeded([](YGNodeRef) {}); } void YGNode::markDirtyAndPropogate() { diff --git a/yoga/YGNode.h b/yoga/YGNode.h index 63087fe4..3c2a9903 100644 --- a/yoga/YGNode.h +++ b/yoga/YGNode.h @@ -143,6 +143,22 @@ public: return children_; } + // Applies a callback to all children, after cloning them if they are not + // owned. + template + void iterChildrenAfterCloningIfNeeded(T callback) { + int i = 0; + for (YGNodeRef& child : children_) { + if (child->getOwner() != this) { + child = config_->cloneNode(child, this, i); + child->setOwner(this); + } + i += 1; + + callback(child); + } + } + YGNodeRef getChild(uint32_t index) const { return children_.at(index); } diff --git a/yoga/Yoga.cpp b/yoga/Yoga.cpp index 290e50a8..22c2a038 100644 --- a/yoga/Yoga.cpp +++ b/yoga/Yoga.cpp @@ -302,14 +302,16 @@ static void YGConfigFreeRecursive(const YGNodeRef root) { void YGNodeFreeRecursiveWithCleanupFunc( const YGNodeRef root, YGNodeCleanupFunc cleanup) { - while (YGNodeGetChildCount(root) > 0) { - const YGNodeRef child = YGNodeGetChild(root, 0); + uint32_t skipped = 0; + while (YGNodeGetChildCount(root) > skipped) { + const YGNodeRef child = YGNodeGetChild(root, skipped); if (child->getOwner() != root) { // Don't free shared nodes that we don't own. - break; + skipped += 1; + } else { + YGNodeRemoveChild(root, child); + YGNodeFreeRecursive(child); } - YGNodeRemoveChild(root, child); - YGNodeFreeRecursive(child); } if (cleanup != nullptr) { cleanup(root); @@ -381,70 +383,40 @@ bool YGNodeIsReferenceBaseline(YGNodeRef node) { } void YGNodeInsertChild( - const YGNodeRef node, + const YGNodeRef owner, const YGNodeRef child, const uint32_t index) { YGAssertWithNode( - node, + owner, child->getOwner() == nullptr, "Child already has a owner, it must be removed first."); YGAssertWithNode( - node, - !node->hasMeasureFunc(), + owner, + !owner->hasMeasureFunc(), "Cannot add child: Nodes with measure functions cannot have children."); - node->cloneChildrenIfNeeded(); - node->insertChild(child, index); - YGNodeRef owner = child->getOwner() ? nullptr : node; + owner->insertChild(child, index); child->setOwner(owner); - node->markDirtyAndPropogate(); + owner->markDirtyAndPropogate(); } void YGNodeRemoveChild(const YGNodeRef owner, const YGNodeRef excludedChild) { - // This algorithm is a forked variant from cloneChildrenIfNeeded in YGNode - // that excludes a child. - const uint32_t childCount = YGNodeGetChildCount(owner); - - if (childCount == 0) { + if (YGNodeGetChildCount(owner) == 0) { // This is an empty set. Nothing to remove. return; } - const YGNodeRef firstChild = YGNodeGetChild(owner, 0); - if (firstChild->getOwner() == owner) { - // If the first child has this node as its owner, we assume that it is - // already unique. We can now try to delete a child in this list. - if (owner->removeChild(excludedChild)) { - excludedChild->setLayout( - YGNode().getLayout()); // layout is no longer valid - excludedChild->setOwner(nullptr); - owner->markDirtyAndPropogate(); - } - return; - } - // Otherwise we have to clone the node list except for the child we're trying - // to delete. We don't want to simply clone all children, because then the - // host will need to free the clone of the child that was just deleted. - uint32_t nextInsertIndex = 0; - for (uint32_t i = 0; i < childCount; i++) { - const YGNodeRef oldChild = owner->getChild(i); - if (excludedChild == oldChild) { - // Ignore the deleted child. Don't reset its layout or owner since it is - // still valid in the other owner. However, since this owner has now - // changed, we need to mark it as dirty. - owner->markDirtyAndPropogate(); - continue; - } - YGNodeRef newChild = - owner->getConfig()->cloneNode(oldChild, owner, nextInsertIndex); - owner->replaceChild(newChild, nextInsertIndex); - newChild->setOwner(owner); - nextInsertIndex++; - } - while (nextInsertIndex < childCount) { - owner->removeChild(nextInsertIndex); - nextInsertIndex++; + // Children may be shared between parents, which is indicated by not having an + // owner. We only want to reset the child completely if it is owned + // exclusively by one node. + auto childOwner = excludedChild->getOwner(); + if (owner->removeChild(excludedChild)) { + if (owner == childOwner) { + excludedChild->setLayout({}); // layout is no longer valid + excludedChild->setOwner(nullptr); + } + owner->markDirtyAndPropogate(); } } @@ -1822,12 +1794,8 @@ static void YGZeroOutLayoutRecursivly(const YGNodeRef node) { node->setLayoutDimension(0, 0); node->setLayoutDimension(0, 1); node->setHasNewLayout(true); - node->cloneChildrenIfNeeded(); - const uint32_t childCount = YGNodeGetChildCount(node); - for (uint32_t i = 0; i < childCount; i++) { - const YGNodeRef child = node->getChild(i); - YGZeroOutLayoutRecursivly(child); - } + + node->iterChildrenAfterCloningIfNeeded(YGZeroOutLayoutRecursivly); } static float YGNodeCalculateAvailableInnerDim(