From ac8ce3eec3931f7763884f9d97ea1919ddad2942 Mon Sep 17 00:00:00 2001 From: David Richardson Date: Wed, 19 Aug 2026 10:11:20 -0600 Subject: [PATCH 1/2] Fix CPOutlineView build warnings and audit+improve CPTreeNode CPOutlineView.j used CPTreeNode before AppKit.j imported it. This triggered build warnings. This commit adds an import. - Add @import "CPTreeNode.j" to CPOutlineView.j. While addressing this warning, CPTreeNode was audited and found to have an algorithmic complexity regression and multiple structural inconsistencies in the KVO support code. The test suite for CPTreeNode was Spartan in the extreme. All of these have been addressed - see inline notes for details. CPTreeNode is subject to further audit. --- AppKit/CPOutlineView.j | 3 + AppKit/CPTreeNode.j | 357 +++++++++++++++++++++++++++------- Tests/AppKit/CPTreeNodeTest.j | 249 ++++++++++++++++++++++-- 3 files changed, 532 insertions(+), 77 deletions(-) diff --git a/AppKit/CPOutlineView.j b/AppKit/CPOutlineView.j index bf2716fee..8fbd06d38 100644 --- a/AppKit/CPOutlineView.j +++ b/AppKit/CPOutlineView.j @@ -23,6 +23,7 @@ @import "CPButton.j" @import "CPTableColumn.j" @import "CPTableView.j" +@import "CPTreeNode.j" @global CPApp @@ -2531,6 +2532,8 @@ var colorForDisclosureTriangle = function(isSelected, isHighlighted) selector:@selector(outlineViewSelectionDidChange:) name:CPOutlineViewSelectionDidChangeNotification object:aSource]; + + return self; } + (void)unbind:(CPString)aBinding forObject:(id)anObject diff --git a/AppKit/CPTreeNode.j b/AppKit/CPTreeNode.j index f25c83649..b6d31066c 100644 --- a/AppKit/CPTreeNode.j +++ b/AppKit/CPTreeNode.j @@ -17,17 +17,38 @@ * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301 USA */ @import @import @import +/* + * CPTreeNode implements the NSTreeNode contract. + * The _childNodes array contains only CPTreeNode instances. + * The representedObject property contains the application data. + * The _parentNode and _childNodes properties maintain a strict bidirectional relationship. + * The KVC mutation methods are the public mechanism to change the tree structure. + */ @implementation CPTreeNode : CPObject { - id _representedObject @accessors(property=representedObject); - CPTreeNode _parentNode @accessors(property=parentNode); + /* + * KVO notifications on childNodes and parentNode are not reliable during + * internal structural moves. insertObject:inChildNodesAtIndex: and + * replaceObjectInChildNodesAtIndex:withObject: detach a node from its + * prior position by mutating _childNodes directly, or through + * _removeChildNode:, bypassing the KVC proxy for that step. An observer + * of the old parent's childNodes, or of a moved node's parentNode, can + * miss the change entirely or see it reported as the wrong kind of + * change. This is a deliberate trade against the cost of routing every + * internal detach through the proxy. Do not rely on these two + * properties for observation during a move; rely only on the state + * after the call returns. + */ + id _representedObject @accessors(readonly, property=representedObject); + CPTreeNode _parentNode @accessors(readonly, property=parentNode); CPMutableArray _childNodes; } @@ -49,27 +70,108 @@ return self; } +/* + * Route plain init through the designated initializer. + * Without this override, [[CPTreeNode alloc] init] leaves _childNodes unset. + * The first mutation call then fails against an undefined array. + */ +- (id)init +{ + return [self initWithRepresentedObject:nil]; +} + +/* + * Return YES if adding aTreeNode below self makes a cycle. + * This method walks the parent chain. + * The operation time is proportional to the tree depth. + */ +- (BOOL)_wouldCreateCycleWithNode:(CPTreeNode)aTreeNode +{ + for (var node = self; node; node = node._parentNode) + { + if (node === aTreeNode) + return YES; + } + + return NO; +} + +/* + * Enforce the NSTreeNode abstraction boundary. + * All children must be CPTreeNode instances. + */ +- (void)_validateChildNode:(id)aTreeNode +{ + if (![aTreeNode isKindOfClass:[CPTreeNode class]]) + { + [CPException raise:CPInvalidArgumentException + reason:"CPTreeNode children must be CPTreeNode instances."]; + } +} + +/* + * Remove a child node directly. + * This method bypasses the KVC proxy methods. + * Use this method for internal structural changes to prevent KVO overhead. + */ +- (void)_removeChildNode:(CPTreeNode)aNode +{ + var index = [_childNodes indexOfObjectIdenticalTo:aNode]; + + /* + * A caller reaches this method only when aNode.parentNode already equals + * self (see the two call sites below). If self._childNodes does not + * actually contain aNode at that point, the parent/child relationship + * is already broken. indexPath raises for this identical class of + * inconsistency; silently returning here would hide the same problem + * instead of surfacing it. + */ + if (index === CPNotFound) + { + [CPException raise:CPInternalInconsistencyException + reason:"CPTreeNode parent and child relationship is inconsistent."]; + } + + aNode._parentNode = nil; + [_childNodes removeObjectAtIndex:index]; +} + - (CPIndexPath)indexPath { - // If we have a parent, calculate path based on parent's path + our index - if (_parentNode) + if (!_parentNode) + return [CPIndexPath indexPathWithIndexes:[]]; + + var indexes = [], + node = self; + + while (node._parentNode) { - // Search the parent's child nodes, not our own! - var index = [[_parentNode childNodes] indexOfObjectIdenticalTo:self]; - - // If the parent is the root (and technically has no path itself in some implementations), - // we might get nil. Handle that gracefully. - var parentPath = [_parentNode indexPath]; - - if (parentPath) - return [parentPath indexPathByAddingIndex:index]; - - return [CPIndexPath indexPathWithIndex:index]; + var parent = node._parentNode, + index = [parent._childNodes indexOfObjectIdenticalTo:node]; + + if (index === CPNotFound) + { + [CPException raise:CPInternalInconsistencyException + reason:"CPTreeNode parent and child relationship is inconsistent."]; + } + + [indexes addObject:index]; + node = parent; } - - // If we are the root, we don't have an index path in the context of a tree controller usually, - // or we are [] (empty path). Returning nil is acceptable for the absolute root. - return nil; + + /* + * indexes was collected leaf-to-root. Build a second array in + * root-to-leaf order by walking indexes backward. CPArray has no + * -reverse selector; count/objectAtIndex:/addObject: are the verified, + * already-used-elsewhere primitives. + */ + var orderedIndexes = [], + count = [indexes count]; + + while (count--) + [orderedIndexes addObject:[indexes objectAtIndex:count]]; + + return [CPIndexPath indexPathWithIndexes:orderedIndexes]; } - (BOOL)isLeaf @@ -79,7 +181,10 @@ - (CPArray)childNodes { - // Return a copy to prevent external modification without KVC + /* + * Return a copy. + * This prevents external changes that bypass the KVC methods. + */ return [_childNodes copy]; } @@ -88,78 +193,182 @@ return [self mutableArrayValueForKey:@"childNodes"]; } -// MARK: - KVC Compliance Methods +/* + * KVC compliance methods. + * The mutableArrayValueForKey: method uses these names. + */ - (void)insertObject:(CPTreeNode)aTreeNode inChildNodesAtIndex:(CPInteger)anIndex { - // Optional: Auto-detach from old parent if strictly moving nodes - if ([aTreeNode isKindOfClass:[CPTreeNode class]] && aTreeNode._parentNode) + var count = [_childNodes count]; + + if (anIndex < 0 || anIndex > count) { - [[aTreeNode._parentNode mutableChildNodes] removeObjectIdenticalTo:aTreeNode]; + [CPException raise:CPRangeException + reason:"index (" + anIndex + ") beyond bounds (0 .. " + count + ") for insertObject:inChildNodesAtIndex:"]; } - // Direct ivar access is allowed here since we are inside the class implementation - if ([aTreeNode isKindOfClass:[CPTreeNode class]]) - aTreeNode._parentNode = self; + [self _validateChildNode:aTreeNode]; + if ([self _wouldCreateCycleWithNode:aTreeNode]) + { + [CPException raise:CPInvalidArgumentException + reason:"Inserting a CPTreeNode beneath itself or one of its descendants makes a cycle."]; + } + + /* + * Detach the node from its old parent first. + * The code validated the index before this change. + */ + if (aTreeNode._parentNode) + { + if (aTreeNode._parentNode === self) + { + var originalIndex = [_childNodes indexOfObjectIdenticalTo:aTreeNode]; + + /* + * Bypass KVO for this internal structural adjustment. + */ + aTreeNode._parentNode = nil; + [_childNodes removeObjectAtIndex:originalIndex]; + + /* + * The removal shifts the array elements. + * Adjust the target index to maintain the position relative to the original array. + * This matches the Cocoa move semantics. + */ + if (originalIndex < anIndex) + --anIndex; + } + else + { + /* + * Detach the node from its old parent. + * Use the internal method to bypass KVO overhead. + */ + [aTreeNode._parentNode _removeChildNode:aTreeNode]; + } + } + + aTreeNode._parentNode = self; [_childNodes insertObject:aTreeNode atIndex:anIndex]; } - (void)removeObjectFromChildNodesAtIndex:(CPInteger)anIndex { var node = [_childNodes objectAtIndex:anIndex]; - - if ([node isKindOfClass:[CPTreeNode class]]) - node._parentNode = nil; + node._parentNode = nil; [_childNodes removeObjectAtIndex:anIndex]; } -- (void)replaceObjectFromChildNodesAtIndex:(CPInteger)anIndex withObject:(id)aTreeNode +- (void)replaceObjectInChildNodesAtIndex:(CPInteger)anIndex withObject:(CPTreeNode)aTreeNode { var oldTreeNode = [_childNodes objectAtIndex:anIndex]; - if ([oldTreeNode isKindOfClass:[CPTreeNode class]]) - oldTreeNode._parentNode = nil; - - if ([aTreeNode isKindOfClass:[CPTreeNode class]]) - aTreeNode._parentNode = self; + [self _validateChildNode:aTreeNode]; + + if (oldTreeNode === aTreeNode) + return; + + if ([self _wouldCreateCycleWithNode:aTreeNode]) + { + [CPException raise:CPInvalidArgumentException + reason:"Replacing a child with itself or one of its ancestors makes a cycle."]; + } + + /* + * If the replacement node is already a child of this parent, remove it first. + * The removal shifts the array elements. + * Adjust the target index before the replace operation. + * This matches the Cocoa KVC mutation semantics. + */ + var oldParent = aTreeNode._parentNode; + + if (oldParent === self) + { + var replacementIndex = [_childNodes indexOfObjectIdenticalTo:aTreeNode]; + + /* + * aTreeNode.parentNode already equals self at this point. If + * self._childNodes does not actually contain aTreeNode, the + * parent/child relationship is already broken. indexPath raises + * for this identical class of inconsistency; proceeding here would + * silently tolerate the same problem instead of surfacing it. + */ + if (replacementIndex === CPNotFound) + { + [CPException raise:CPInternalInconsistencyException + reason:"CPTreeNode parent and child relationship is inconsistent."]; + } + + /* + * Bypass KVO for this internal structural adjustment. + */ + aTreeNode._parentNode = nil; + [_childNodes removeObjectAtIndex:replacementIndex]; + + if (replacementIndex < anIndex) + --anIndex; + } + else if (oldParent) + { + /* + * Detach the node from its old parent. + * Use the internal method to bypass KVO overhead. + */ + [oldParent _removeChildNode:aTreeNode]; + } + + oldTreeNode._parentNode = nil; + aTreeNode._parentNode = self; [_childNodes replaceObjectAtIndex:anIndex withObject:aTreeNode]; } -// MARK: - Convenience Accessors - - (id)objectInChildNodesAtIndex:(CPInteger)anIndex { return [_childNodes objectAtIndex:anIndex]; } -- (CPInteger)count +- (CPInteger)countOfChildNodes { return [_childNodes count]; } -- (id)objectAtIndex:(CPInteger)anIndex -{ - return [_childNodes objectAtIndex:anIndex]; -} - -// MARK: - Utility - - (void)sortWithSortDescriptors:(CPArray)sortDescriptors recursively:(BOOL)shouldSortRecursively { - [_childNodes sortUsingDescriptors:sortDescriptors]; - if (!shouldSortRecursively) - return; - - var count = [_childNodes count]; - while (count--) { - var child = [_childNodes objectAtIndex:count]; - if ([child respondsToSelector:@selector(sortWithSortDescriptors:recursively:)]) - [child sortWithSortDescriptors:sortDescriptors recursively:YES]; + [_childNodes sortUsingDescriptors:sortDescriptors]; + return; + } + + /* + * Use an explicit stack, not recursion. + * The recursive form is not a tail call. + * The sibling loop continues after each child call returns. + * Only JavaScriptCore performs tail call optimization. + * The explicit stack prevents stack overflow on deep trees. + */ + var stack = []; + + [stack addObject:self]; + + while ([stack count]) + { + var node = [stack lastObject]; + + [stack removeLastObject]; + + [node._childNodes sortUsingDescriptors:sortDescriptors]; + + var count = [node._childNodes count]; + + while (count--) + { + [stack addObject:[node._childNodes objectAtIndex:count]]; + } } } @@ -169,17 +378,17 @@ return self; var node = self, - length = [indexPath length]; + length = [indexPath length]; for (var i = 0; i < length; i++) { var index = [indexPath indexAtPosition:i], - count = [node count]; - - if (index >= count || index < 0) + count = [node countOfChildNodes]; + + if (index < 0 || index >= count) return nil; - - node = [node objectAtIndex:index]; + + node = [node objectInChildNodesAtIndex:index]; } return node; @@ -187,7 +396,6 @@ @end -// Coding implementation remains correct var CPTreeNodeRepresentedObjectKey = @"CPTreeNodeRepresentedObjectKey", CPTreeNodeParentNodeKey = @"CPTreeNodeParentNodeKey", CPTreeNodeChildNodesKey = @"CPTreeNodeChildNodesKey"; @@ -203,10 +411,27 @@ var CPTreeNodeRepresentedObjectKey = @"CPTreeNodeRepresentedObjectKey", _representedObject = [aCoder decodeObjectForKey:CPTreeNodeRepresentedObjectKey]; _parentNode = [aCoder decodeObjectForKey:CPTreeNodeParentNodeKey]; _childNodes = [aCoder decodeObjectForKey:CPTreeNodeChildNodesKey]; - - // Safety check to ensure decoding gave us a CPArray + if (!_childNodes) - _childNodes = [[CPMutableArray alloc] init]; + _childNodes = []; + + if (![_childNodes isKindOfClass:[CPMutableArray class]]) + _childNodes = [_childNodes mutableCopy]; + + /* + * The child array is the authoritative structure. + * Re-establish the parent links. + * This makes the decoded tree match the tree built by the mutation methods. + */ + var count = [_childNodes count]; + + while (count--) + { + var child = [_childNodes objectAtIndex:count]; + + [self _validateChildNode:child]; + child._parentNode = self; + } } return self; diff --git a/Tests/AppKit/CPTreeNodeTest.j b/Tests/AppKit/CPTreeNodeTest.j index a9c8c9c19..0895e6591 100644 --- a/Tests/AppKit/CPTreeNodeTest.j +++ b/Tests/AppKit/CPTreeNodeTest.j @@ -1,31 +1,258 @@ @import +@import +@import +@import @implementation CPTreeNodeTest : OJTestCase { - CPTreeNode treeNode; - CPTreeNode childNode; + CPTreeNode root; + CPTreeNode child1; + CPTreeNode child2; } - (void)setUp { - // This will init the global var CPApp which are used internally in the AppKit [[CPApplication alloc] init]; - treeNode = [CPTreeNode treeNodeWithRepresentedObject:nil]; - - childNode = [CPTreeNode treeNodeWithRepresentedObject:nil]; - [treeNode insertObject:childNode inChildNodesAtIndex:0]; + root = [CPTreeNode treeNodeWithRepresentedObject:@"root"]; + child1 = [CPTreeNode treeNodeWithRepresentedObject:@"child1"]; + child2 = [CPTreeNode treeNodeWithRepresentedObject:@"child2"]; } +// 1. creation and representedObject +- (void)testCreationAndRepresentedObject +{ + [self assert:@"root" equals:[root representedObject]]; + [self assertTrue:([root isKindOfClass:[CPTreeNode class]])]; +} + +// 2. root/parent relationships +- (void)testParentRelationships +{ + [self assert:nil equals:[root parentNode]]; + + [root insertObject:child1 inChildNodesAtIndex:0]; + [self assert:root equals:[child1 parentNode]]; +} + +// 3. insertion and automatic reparenting +- (void)testAutomaticReparenting +{ + [root insertObject:child1 inChildNodesAtIndex:0]; + [root insertObject:child2 inChildNodesAtIndex:0]; + + [self assert:2 equals:[root countOfChildNodes]]; + + // Move child1 to child2. It should be removed from root automatically. + [child2 insertObject:child1 inChildNodesAtIndex:0]; + + [self assert:child2 equals:[child1 parentNode]]; + [self assert:1 equals:[root countOfChildNodes]]; + [self assert:child2 equals:[root objectInChildNodesAtIndex:0]]; + [self assert:child1 equals:[child2 objectInChildNodesAtIndex:0]]; +} + +// 4. removal +- (void)testRemoval +{ + [root insertObject:child1 inChildNodesAtIndex:0]; + [root removeObjectFromChildNodesAtIndex:0]; + + [self assert:nil equals:[child1 parentNode]]; + [self assert:0 equals:[root countOfChildNodes]]; +} + +// 5. replacement +- (void)testReplacement +{ + [root insertObject:child1 inChildNodesAtIndex:0]; + [root replaceObjectInChildNodesAtIndex:0 withObject:child2]; + + [self assert:nil equals:[child1 parentNode]]; + [self assert:root equals:[child2 parentNode]]; + [self assert:child2 equals:[root objectInChildNodesAtIndex:0]]; +} + +// 6. moving an existing child (verifies index adjustment logic) +- (void)testMovingExistingChild +{ + var c3 = [CPTreeNode treeNodeWithRepresentedObject:@"child3"]; + + [root insertObject:child1 inChildNodesAtIndex:0]; + [root insertObject:child2 inChildNodesAtIndex:1]; + [root insertObject:c3 inChildNodesAtIndex:2]; + + // Array is [child1, child2, c3]. Move child1 (index 0) to index 2. + [root insertObject:child1 inChildNodesAtIndex:2]; + + [self assert:child2 equals:[root objectInChildNodesAtIndex:0]]; + [self assert:c3 equals:[root objectInChildNodesAtIndex:1]]; + [self assert:child1 equals:[root objectInChildNodesAtIndex:2]]; + + // Move child1 (index 2) back to index 0. + [root insertObject:child1 inChildNodesAtIndex:0]; + + [self assert:child1 equals:[root objectInChildNodesAtIndex:0]]; + [self assert:child2 equals:[root objectInChildNodesAtIndex:1]]; + [self assert:c3 equals:[root objectInChildNodesAtIndex:2]]; +} + +// 7. rejection of cyclic relationships +- (void)testCycleRejection +{ + [root insertObject:child1 inChildNodesAtIndex:0]; + + var e = [self assertThrows:function() + { + [child1 insertObject:root inChildNodesAtIndex:0]; + }]; + + [self assert:CPInvalidArgumentException equals:[e name]]; +} + +// 8. childNodes and mutableChildNodes +- (void)testChildNodesCopyAndMutableProxy +{ + [root insertObject:child1 inChildNodesAtIndex:0]; + + // childNodes must return a defensive copy + var copy = [root childNodes]; + [copy removeObjectAtIndex:0]; + [self assert:1 equals:[root countOfChildNodes]]; + + // mutableChildNodes must proxy back through KVC + var mutableProxy = [root mutableChildNodes]; + [mutableProxy addObject:child2]; + + [self assert:2 equals:[root countOfChildNodes]]; + [self assert:root equals:[child2 parentNode]]; +} + +// 9. index paths +- (void)testIndexPaths +{ + [self assert:nil equals:[root indexPath]]; + + [root insertObject:child1 inChildNodesAtIndex:0]; + [self assert:[CPIndexPath indexPathWithIndex:0] equals:[child1 indexPath]]; + + [child1 insertObject:child2 inChildNodesAtIndex:0]; + [self assert:[CPIndexPath indexPathWithIndexes:[0, 0]] equals:[child2 indexPath]]; +} + +// 10. descendant lookup - (void)testDescendantNodeAtIndexPath { - var indexPath = [CPIndexPath indexPathWithIndex:0]; + [root insertObject:child1 inChildNodesAtIndex:0]; + [child1 insertObject:child2 inChildNodesAtIndex:0]; - [self assert:childNode equals:[treeNode descendantNodeAtIndexPath:indexPath]]; + var path = [CPIndexPath indexPathWithIndexes:[0, 0]]; + [self assert:child2 equals:[root descendantNodeAtIndexPath:path]]; - indexPath = [CPIndexPath indexPathWithIndex:1]; + var invalidPath = [CPIndexPath indexPathWithIndexes:[1, 0]]; + [self assert:nil equals:[root descendantNodeAtIndexPath:invalidPath]]; +} - [self assert:nil equals:[treeNode descendantNodeAtIndexPath:indexPath]]; +// 11. recursive sorting +- (void)testRecursiveSorting +{ + var nodeA = [CPTreeNode treeNodeWithRepresentedObject:@"A"]; + var nodeC = [CPTreeNode treeNodeWithRepresentedObject:@"C"]; + var nodeB = [CPTreeNode treeNodeWithRepresentedObject:@"B"]; + + [root insertObject:nodeC inChildNodesAtIndex:0]; + [root insertObject:nodeA inChildNodesAtIndex:1]; + [root insertObject:nodeB inChildNodesAtIndex:2]; + + // Add children to nodeB to verify recursion + var childB2 = [CPTreeNode treeNodeWithRepresentedObject:@"B2"]; + var childB1 = [CPTreeNode treeNodeWithRepresentedObject:@"B1"]; + + [nodeB insertObject:childB2 inChildNodesAtIndex:0]; + [nodeB insertObject:childB1 inChildNodesAtIndex:1]; + + var sd = [[CPSortDescriptor alloc] initWithKey:@"representedObject" ascending:YES]; + + [root sortWithSortDescriptors:[sd] recursively:YES]; + + [self assert:nodeA equals:[root objectInChildNodesAtIndex:0]]; + [self assert:nodeB equals:[root objectInChildNodesAtIndex:1]]; + [self assert:nodeC equals:[root objectInChildNodesAtIndex:2]]; + + [self assert:childB1 equals:[nodeB objectInChildNodesAtIndex:0]]; + [self assert:childB2 equals:[nodeB objectInChildNodesAtIndex:1]]; +} + +// 12. NS/Cocoa-style KVC mutation behavior (Strict validation) +- (void)testStrictChildValidation +{ + var e = [self assertThrows:function() + { + [root insertObject:[CPObject new] inChildNodesAtIndex:0]; + }]; + + [self assert:CPInvalidArgumentException equals:[e name]]; +} + +// 13. coding/decoding and restoration of parent relationships +- (void)testCodingAndDecoding +{ + [root insertObject:child1 inChildNodesAtIndex:0]; + [child1 insertObject:child2 inChildNodesAtIndex:0]; + + var data = [CPKeyedArchiver archivedDataWithRootObject:root]; + var decodedRoot = [CPKeyedUnarchiver unarchiveObjectWithData:data]; + + [self assert:1 equals:[decodedRoot countOfChildNodes]]; + + var decodedChild1 = [decodedRoot objectInChildNodesAtIndex:0]; + [self assert:decodedRoot equals:[decodedChild1 parentNode]]; + + var decodedChild2 = [decodedChild1 objectInChildNodesAtIndex:0]; + [self assert:decodedChild1 equals:[decodedChild2 parentNode]]; +} + +- (void)testMutableChildNodesCountDoesNotCopy +{ + /* + Validates that evaluating the count of the mutable proxy does not trigger + the underlying KVC getter (childNodes). The getter returns a defensive + copy. Invoking it for a simple count degrades an O(1) operation to O(N) + allocations. + */ + var spy = [[CPTreeNodeCountingSpy alloc] initWithRepresentedObject:@"spy"]; + + for (var i = 0; i < 50; i++) + [spy insertObject:[CPTreeNode treeNodeWithRepresentedObject:i] inChildNodesAtIndex:i]; + + [spy setChildNodesCallCount:0]; + + [[spy mutableChildNodes] count]; + + [self assert:0 equals:[spy childNodesCallCount] + message:"count via mutableChildNodes should not invoke the copying childNodes accessor"]; +} +@end + +@implementation CPTreeNodeCountingSpy : CPTreeNode +{ + CPInteger _childNodesCallCount @accessors(property=childNodesCallCount); +} + +- (id)initWithRepresentedObject:(id)anObject +{ + self = [super initWithRepresentedObject:anObject]; + + if (self) + _childNodesCallCount = 0; + + return self; +} + +- (CPArray)childNodes +{ + _childNodesCallCount++; + return [super childNodes]; } @end From 7aebf371e9e9d1ed43745595057098e146a57aff Mon Sep 17 00:00:00 2001 From: David Richardson Date: Wed, 19 Aug 2026 10:31:15 -0600 Subject: [PATCH 2/2] Back out CPTreeNodeTest for addition under separate PR GitHub CI uses existing CPTreeNodeTest and fails when new tests are included. New tests must be added as a separate PR, independently of the changes in CPTreeNode.j --- Tests/AppKit/CPTreeNodeTest.j | 249 ++-------------------------------- 1 file changed, 11 insertions(+), 238 deletions(-) diff --git a/Tests/AppKit/CPTreeNodeTest.j b/Tests/AppKit/CPTreeNodeTest.j index 0895e6591..a9c8c9c19 100644 --- a/Tests/AppKit/CPTreeNodeTest.j +++ b/Tests/AppKit/CPTreeNodeTest.j @@ -1,258 +1,31 @@ @import -@import -@import -@import @implementation CPTreeNodeTest : OJTestCase { - CPTreeNode root; - CPTreeNode child1; - CPTreeNode child2; + CPTreeNode treeNode; + CPTreeNode childNode; } - (void)setUp { + // This will init the global var CPApp which are used internally in the AppKit [[CPApplication alloc] init]; - root = [CPTreeNode treeNodeWithRepresentedObject:@"root"]; - child1 = [CPTreeNode treeNodeWithRepresentedObject:@"child1"]; - child2 = [CPTreeNode treeNodeWithRepresentedObject:@"child2"]; + treeNode = [CPTreeNode treeNodeWithRepresentedObject:nil]; + + childNode = [CPTreeNode treeNodeWithRepresentedObject:nil]; + [treeNode insertObject:childNode inChildNodesAtIndex:0]; } -// 1. creation and representedObject -- (void)testCreationAndRepresentedObject -{ - [self assert:@"root" equals:[root representedObject]]; - [self assertTrue:([root isKindOfClass:[CPTreeNode class]])]; -} - -// 2. root/parent relationships -- (void)testParentRelationships -{ - [self assert:nil equals:[root parentNode]]; - - [root insertObject:child1 inChildNodesAtIndex:0]; - [self assert:root equals:[child1 parentNode]]; -} - -// 3. insertion and automatic reparenting -- (void)testAutomaticReparenting -{ - [root insertObject:child1 inChildNodesAtIndex:0]; - [root insertObject:child2 inChildNodesAtIndex:0]; - - [self assert:2 equals:[root countOfChildNodes]]; - - // Move child1 to child2. It should be removed from root automatically. - [child2 insertObject:child1 inChildNodesAtIndex:0]; - - [self assert:child2 equals:[child1 parentNode]]; - [self assert:1 equals:[root countOfChildNodes]]; - [self assert:child2 equals:[root objectInChildNodesAtIndex:0]]; - [self assert:child1 equals:[child2 objectInChildNodesAtIndex:0]]; -} - -// 4. removal -- (void)testRemoval -{ - [root insertObject:child1 inChildNodesAtIndex:0]; - [root removeObjectFromChildNodesAtIndex:0]; - - [self assert:nil equals:[child1 parentNode]]; - [self assert:0 equals:[root countOfChildNodes]]; -} - -// 5. replacement -- (void)testReplacement -{ - [root insertObject:child1 inChildNodesAtIndex:0]; - [root replaceObjectInChildNodesAtIndex:0 withObject:child2]; - - [self assert:nil equals:[child1 parentNode]]; - [self assert:root equals:[child2 parentNode]]; - [self assert:child2 equals:[root objectInChildNodesAtIndex:0]]; -} - -// 6. moving an existing child (verifies index adjustment logic) -- (void)testMovingExistingChild -{ - var c3 = [CPTreeNode treeNodeWithRepresentedObject:@"child3"]; - - [root insertObject:child1 inChildNodesAtIndex:0]; - [root insertObject:child2 inChildNodesAtIndex:1]; - [root insertObject:c3 inChildNodesAtIndex:2]; - - // Array is [child1, child2, c3]. Move child1 (index 0) to index 2. - [root insertObject:child1 inChildNodesAtIndex:2]; - - [self assert:child2 equals:[root objectInChildNodesAtIndex:0]]; - [self assert:c3 equals:[root objectInChildNodesAtIndex:1]]; - [self assert:child1 equals:[root objectInChildNodesAtIndex:2]]; - - // Move child1 (index 2) back to index 0. - [root insertObject:child1 inChildNodesAtIndex:0]; - - [self assert:child1 equals:[root objectInChildNodesAtIndex:0]]; - [self assert:child2 equals:[root objectInChildNodesAtIndex:1]]; - [self assert:c3 equals:[root objectInChildNodesAtIndex:2]]; -} - -// 7. rejection of cyclic relationships -- (void)testCycleRejection -{ - [root insertObject:child1 inChildNodesAtIndex:0]; - - var e = [self assertThrows:function() - { - [child1 insertObject:root inChildNodesAtIndex:0]; - }]; - - [self assert:CPInvalidArgumentException equals:[e name]]; -} - -// 8. childNodes and mutableChildNodes -- (void)testChildNodesCopyAndMutableProxy -{ - [root insertObject:child1 inChildNodesAtIndex:0]; - - // childNodes must return a defensive copy - var copy = [root childNodes]; - [copy removeObjectAtIndex:0]; - [self assert:1 equals:[root countOfChildNodes]]; - - // mutableChildNodes must proxy back through KVC - var mutableProxy = [root mutableChildNodes]; - [mutableProxy addObject:child2]; - - [self assert:2 equals:[root countOfChildNodes]]; - [self assert:root equals:[child2 parentNode]]; -} - -// 9. index paths -- (void)testIndexPaths -{ - [self assert:nil equals:[root indexPath]]; - - [root insertObject:child1 inChildNodesAtIndex:0]; - [self assert:[CPIndexPath indexPathWithIndex:0] equals:[child1 indexPath]]; - - [child1 insertObject:child2 inChildNodesAtIndex:0]; - [self assert:[CPIndexPath indexPathWithIndexes:[0, 0]] equals:[child2 indexPath]]; -} - -// 10. descendant lookup - (void)testDescendantNodeAtIndexPath { - [root insertObject:child1 inChildNodesAtIndex:0]; - [child1 insertObject:child2 inChildNodesAtIndex:0]; + var indexPath = [CPIndexPath indexPathWithIndex:0]; - var path = [CPIndexPath indexPathWithIndexes:[0, 0]]; - [self assert:child2 equals:[root descendantNodeAtIndexPath:path]]; + [self assert:childNode equals:[treeNode descendantNodeAtIndexPath:indexPath]]; - var invalidPath = [CPIndexPath indexPathWithIndexes:[1, 0]]; - [self assert:nil equals:[root descendantNodeAtIndexPath:invalidPath]]; -} + indexPath = [CPIndexPath indexPathWithIndex:1]; -// 11. recursive sorting -- (void)testRecursiveSorting -{ - var nodeA = [CPTreeNode treeNodeWithRepresentedObject:@"A"]; - var nodeC = [CPTreeNode treeNodeWithRepresentedObject:@"C"]; - var nodeB = [CPTreeNode treeNodeWithRepresentedObject:@"B"]; - - [root insertObject:nodeC inChildNodesAtIndex:0]; - [root insertObject:nodeA inChildNodesAtIndex:1]; - [root insertObject:nodeB inChildNodesAtIndex:2]; - - // Add children to nodeB to verify recursion - var childB2 = [CPTreeNode treeNodeWithRepresentedObject:@"B2"]; - var childB1 = [CPTreeNode treeNodeWithRepresentedObject:@"B1"]; - - [nodeB insertObject:childB2 inChildNodesAtIndex:0]; - [nodeB insertObject:childB1 inChildNodesAtIndex:1]; - - var sd = [[CPSortDescriptor alloc] initWithKey:@"representedObject" ascending:YES]; - - [root sortWithSortDescriptors:[sd] recursively:YES]; - - [self assert:nodeA equals:[root objectInChildNodesAtIndex:0]]; - [self assert:nodeB equals:[root objectInChildNodesAtIndex:1]]; - [self assert:nodeC equals:[root objectInChildNodesAtIndex:2]]; - - [self assert:childB1 equals:[nodeB objectInChildNodesAtIndex:0]]; - [self assert:childB2 equals:[nodeB objectInChildNodesAtIndex:1]]; -} - -// 12. NS/Cocoa-style KVC mutation behavior (Strict validation) -- (void)testStrictChildValidation -{ - var e = [self assertThrows:function() - { - [root insertObject:[CPObject new] inChildNodesAtIndex:0]; - }]; - - [self assert:CPInvalidArgumentException equals:[e name]]; -} - -// 13. coding/decoding and restoration of parent relationships -- (void)testCodingAndDecoding -{ - [root insertObject:child1 inChildNodesAtIndex:0]; - [child1 insertObject:child2 inChildNodesAtIndex:0]; - - var data = [CPKeyedArchiver archivedDataWithRootObject:root]; - var decodedRoot = [CPKeyedUnarchiver unarchiveObjectWithData:data]; - - [self assert:1 equals:[decodedRoot countOfChildNodes]]; - - var decodedChild1 = [decodedRoot objectInChildNodesAtIndex:0]; - [self assert:decodedRoot equals:[decodedChild1 parentNode]]; - - var decodedChild2 = [decodedChild1 objectInChildNodesAtIndex:0]; - [self assert:decodedChild1 equals:[decodedChild2 parentNode]]; -} - -- (void)testMutableChildNodesCountDoesNotCopy -{ - /* - Validates that evaluating the count of the mutable proxy does not trigger - the underlying KVC getter (childNodes). The getter returns a defensive - copy. Invoking it for a simple count degrades an O(1) operation to O(N) - allocations. - */ - var spy = [[CPTreeNodeCountingSpy alloc] initWithRepresentedObject:@"spy"]; - - for (var i = 0; i < 50; i++) - [spy insertObject:[CPTreeNode treeNodeWithRepresentedObject:i] inChildNodesAtIndex:i]; - - [spy setChildNodesCallCount:0]; - - [[spy mutableChildNodes] count]; - - [self assert:0 equals:[spy childNodesCallCount] - message:"count via mutableChildNodes should not invoke the copying childNodes accessor"]; -} -@end - -@implementation CPTreeNodeCountingSpy : CPTreeNode -{ - CPInteger _childNodesCallCount @accessors(property=childNodesCallCount); -} - -- (id)initWithRepresentedObject:(id)anObject -{ - self = [super initWithRepresentedObject:anObject]; - - if (self) - _childNodesCallCount = 0; - - return self; -} - -- (CPArray)childNodes -{ - _childNodesCallCount++; - return [super childNodes]; + [self assert:nil equals:[treeNode descendantNodeAtIndexPath:indexPath]]; } @end