Merge pull request #3280 from enquora/CPOutlineView-build-warnings-cleanup

Fix CPOutlineView build warnings and audit+improve CPTreeNode

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

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 <Foundation/CPObject.j>
 @import <Foundation/CPIndexPath.j>
 @import <Foundation/CPArray.j>
 
+/*
+ * 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;