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