From 249a63af95bb72c26a931fb991f2cdc97c3d4e8e Mon Sep 17 00:00:00 2001 From: Klaas Pieter Annema Date: Tue, 24 May 2011 12:28:25 +0200 Subject: [PATCH 1/6] don't use remove method for removeMany selector --- Foundation/CPArray+KVO.j | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Foundation/CPArray+KVO.j b/Foundation/CPArray+KVO.j index ad05ef8c6..d8e404f31 100644 --- a/Foundation/CPArray+KVO.j +++ b/Foundation/CPArray+KVO.j @@ -127,7 +127,7 @@ _removeManySEL = sel_getName(@"remove" + capitalizedKey + "AtIndexes:"); if ([_proxyObject respondsToSelector:_removeManySEL]) - _remove = [_proxyObject methodForSelector:_removeManySEL]; + _removeMany = [_proxyObject methodForSelector:_removeManySEL]; _replaceManySEL = sel_getName(@"replace" + capitalizedKey + "AtIndexes:with" + capitalizedKey + ":"); if ([_proxyObject respondsToSelector:_replaceManySEL]) From 815e64356d874a2f9499ad5f61aaae53b872c34c Mon Sep 17 00:00:00 2001 From: Klaas Pieter Annema Date: Tue, 24 May 2011 12:29:23 +0200 Subject: [PATCH 2/6] implement _minimumFrameSize in CPButton --- AppKit/CPButton.j | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/AppKit/CPButton.j b/AppKit/CPButton.j index 911a56d5b..095d3d0a3 100644 --- a/AppKit/CPButton.j +++ b/AppKit/CPButton.j @@ -560,14 +560,9 @@ CPButtonImageOffset = 3.0; return bounds; } -/*! - Adjust the size of the button to fit the title and surrounding button image. -*/ -- (void)sizeToFit +- (CGSize)_minimumFrameSize { - [self layoutSubviews]; - - var size, + var size = CGSizeMakeZero(), contentView = [self ephemeralSubviewNamed:@"content-view"]; if (contentView) @@ -591,9 +586,19 @@ CPButtonImageOffset = 3.0; if (maxSize.height >= 0.0) size.height = MIN(size.height, maxSize.height); - [self setFrameSize:size]; + return size; +} - if (contentView) +/*! + Adjust the size of the button to fit the title and surrounding button image. +*/ +- (void)sizeToFit +{ + [self layoutSubviews]; + + [self setFrameSize:[self _minimumFrameSize]]; + + if ([self ephemeralSubviewNamed:@"content-view"]) [self layoutSubviews]; } From e4950bd831116ac5c8582dff0a709460c7448a5e Mon Sep 17 00:00:00 2001 From: Alexander Ljungberg Date: Sat, 28 May 2011 01:26:28 -0400 Subject: [PATCH 3/6] Fixed: duplicate observer notification for array controller addObject: (on arrangedObjects). --- AppKit/CPArrayController.j | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/AppKit/CPArrayController.j b/AppKit/CPArrayController.j index d98f159d7..0d9fe2b49 100644 --- a/AppKit/CPArrayController.j +++ b/AppKit/CPArrayController.j @@ -70,7 +70,8 @@ + (CPSet)keyPathsForValuesAffectingArrangedObjects { - return [CPSet setWithObjects:"content", "filterPredicate", "sortDescriptors"]; + // Also depends on "filterPredicate" but we'll handle that manually. + return [CPSet setWithObjects:"content", "sortDescriptors"]; } + (CPSet)keyPathsForValuesAffectingSelection @@ -488,7 +489,14 @@ */ - (void)setFilterPredicate:(CPPredicate)value { + if (_filterPredicate === value) + return; + + // __setFilterPredicate will call _rearrangeObjects without + // sending notifications, so we must send them instead. + [self willChangeValueForKey:@"arrangedObjects"]; [self __setFilterPredicate:value]; + [self didChangeValueForKey:@"arrangedObjects"]; } /* @@ -501,8 +509,7 @@ return; _filterPredicate = value; - // Use the non-notification version since arrangedObjects already depends - // on filterPredicate. + // Use the non-notification version. [self _rearrangeObjects]; } @@ -745,6 +752,7 @@ else [self _rearrangeObjects]; + // This will also send notificaitons for arrangedObjects. [self didChangeValueForKey:@"content"]; if (_clearsFilterPredicateOnInsertion) [self didChangeValueForKey:@"filterPredicate"]; From a1a06c3f753b85cdf162ee51149c59488e64917f Mon Sep 17 00:00:00 2001 From: Alexander Ljungberg Date: Sat, 28 May 2011 01:47:25 -0400 Subject: [PATCH 4/6] Performance test array controller addObject: when clearing the filter predicate. --- Tests/AppKit/CPArrayControllerPerformance.j | 75 +++++++++++++++++---- 1 file changed, 61 insertions(+), 14 deletions(-) diff --git a/Tests/AppKit/CPArrayControllerPerformance.j b/Tests/AppKit/CPArrayControllerPerformance.j index 083f9b343..0e73ffffd 100644 --- a/Tests/AppKit/CPArrayControllerPerformance.j +++ b/Tests/AppKit/CPArrayControllerPerformance.j @@ -2,16 +2,17 @@ @import @import +var ELEMENTS = 200, + REPEATS = 25; + @implementation CPArrayControllerPerformance : OJTestCase -- (void)testRearrangeObjects +- (CPArrayController)setupWithElements:(int)aCount { - var ELEMENTS = 200, - REPEATS = 25, - ac = [CPArrayController new], + var ac = [CPArrayController new], array = []; - for (var i = 0; i < ELEMENTS; i++) + for (var i = 0; i < aCount; i++) { var s = [Sortable new]; [s setA:i]; @@ -19,15 +20,17 @@ array.push(s); } - var descriptors = [ - [CPSortDescriptor sortDescriptorWithKey:"a" ascending:NO], - ]; - [ac setContent:array]; - [ac setFilterPredicate:[CPPredicate predicateWithFormat:@"(b != %@)", 0]]; + return ac; +} + +- (void)testRearrangeObjects +{ + var ac = [self setupWithElements:ELEMENTS]; + // Filter alone var start = (new Date).getTime(); for (var i = 0; i < REPEATS; i++) @@ -41,7 +44,7 @@ for (var j = 0, count = [sorted count]; j < count; j++) { if (sorted[j].b == 0) - [self fail:"b == 0 should be filtered out (position: "+j+")"]; + [self fail:"b == 0 should be filtered out (position: " + j + ")"]; last = sorted[j]; } } @@ -49,7 +52,9 @@ CPLog.warn("testRearrangeObjects, filter: "+(end-start)+"ms"); - [ac setSortDescriptors:descriptors]; + [ac setSortDescriptors:[ + [CPSortDescriptor sortDescriptorWithKey:"a" ascending:NO], + ]]; // Filter and sort. start = (new Date).getTime(); @@ -64,9 +69,9 @@ for (var j = 0, count = [sorted count]; j < count; j++) { if (sorted[j].b == 0) - [self fail:"b == 0 should be filtered out (position: "+j+")"]; + [self fail:"b == 0 should be filtered out (position: " + j + ")"]; if (sorted[j].a >= last) - [self fail:"array values should be descending (position: "+j+")"]; + [self fail:"array values should be descending (position: " + j + ")"]; last = sorted[j]; } } @@ -75,6 +80,40 @@ CPLog.warn("testRearrangeObjects, filter and sort: "+(end-start)+"ms"); } +- (void)testAddObject_ +{ + var ac = [self setupWithElements:ELEMENTS], + predicate = [ac filterPredicate], + content = [[ac content] copy]; + + // Add object while clearing the predicate. + [ac setClearsFilterPredicateOnInsertion:YES]; + + [ac setSortDescriptors:[ + [CPSortDescriptor sortDescriptorWithKey:"a" ascending:NO], + ]]; + + var start = (new Date).getTime(); + for (var i = 0; i < REPEATS; i++) + { + [ac setFilterPredicate:predicate]; + [ac addObject:[Sortable sortableWithA:i B:i * 2]]; + + var sorted = [ac arrangedObjects], + last = ELEMENTS; + + // Verify that all is well. + for (var j = 0, count = [sorted count]; j < count; j++) + { + if (sorted[j].a >= last) + [self fail:"array values should be descending (position: " + j + ")"]; + last = sorted[j]; + } + } + var end = (new Date).getTime(); + CPLog.warn("testAddObject_, sorted, clear filter on insert: " + (end - start) + "ms"); +} + @end @implementation Sortable : CPObject @@ -83,4 +122,12 @@ int b @accessors; } ++ (id)sortableWithA:(int)anA B:(int)aB +{ + var r = [Sortable new]; + r.a = anA; + r.b = aB; + return r; +} + @end \ No newline at end of file From 2e5081b137dd575a9a6a80a7098b3fcba9a3e9d4 Mon Sep 17 00:00:00 2001 From: Alexander Ljungberg Date: Sat, 28 May 2011 02:02:13 -0400 Subject: [PATCH 5/6] Optimise array controller addObject: by eliminating entirely pointless rearrange when added objects did not pass the filter. Also avoided double rearrange if clearsFilterPredicateOnInsertion. CPArrayControllerPerformance.j execution time on "testAddObject_, sorted, filtered" down from ~420ms to ~60ms on my machine. --- AppKit/CPArrayController.j | 28 +++++++++++++++------ Tests/AppKit/CPArrayControllerPerformance.j | 26 ++++++++++++++++++- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/AppKit/CPArrayController.j b/AppKit/CPArrayController.j index 0d9fe2b49..42ad4acc3 100644 --- a/AppKit/CPArrayController.j +++ b/AppKit/CPArrayController.j @@ -719,8 +719,12 @@ if (![self canAdd]) return; - if (_clearsFilterPredicateOnInsertion) + var willClearPredicate = NO; + if (_clearsFilterPredicateOnInsertion && _filterPredicate) + { [self willChangeValueForKey:@"filterPredicate"]; + willClearPredicate = YES; + } [self willChangeValueForKey:@"content"]; @@ -735,11 +739,15 @@ [_contentObject addObject:object]; _disableSetContent = NO; - if (_clearsFilterPredicateOnInsertion) - [self __setFilterPredicate:nil]; - - if (_filterPredicate === nil || [_filterPredicate evaluateWithObject:object]) + if (willClearPredicate) { + // Full rearrange needed due to changed filter. + _filterPredicate = nil; + [self _rearrangeObjects]; + } + else if (_filterPredicate === nil || [_filterPredicate evaluateWithObject:object]) + { + // Insert directly into the array. var pos = [_arrangedObjects insertObject:object inArraySortedByDescriptors:_sortDescriptors]; // selectionIndexes change notification will be fired as a result of the @@ -749,12 +757,16 @@ else [_selectionIndexes shiftIndexesStartingAtIndex:pos by:1]; } - else - [self _rearrangeObjects]; + /* + else if (_filterPredicate !== nil) + ... + // Implies _filterPredicate && ![_filterPredicate evaluateWithObject:object], so the new object does + // not appear in arrangedObjects and we do not have to update at all. + */ // This will also send notificaitons for arrangedObjects. [self didChangeValueForKey:@"content"]; - if (_clearsFilterPredicateOnInsertion) + if (willClearPredicate) [self didChangeValueForKey:@"filterPredicate"]; } diff --git a/Tests/AppKit/CPArrayControllerPerformance.j b/Tests/AppKit/CPArrayControllerPerformance.j index 0e73ffffd..98137b1ee 100644 --- a/Tests/AppKit/CPArrayControllerPerformance.j +++ b/Tests/AppKit/CPArrayControllerPerformance.j @@ -94,7 +94,7 @@ var ELEMENTS = 200, ]]; var start = (new Date).getTime(); - for (var i = 0; i < REPEATS; i++) + for (var i = 0; i < REPEATS / 2; i++) { [ac setFilterPredicate:predicate]; [ac addObject:[Sortable sortableWithA:i B:i * 2]]; @@ -112,6 +112,30 @@ var ELEMENTS = 200, } var end = (new Date).getTime(); CPLog.warn("testAddObject_, sorted, clear filter on insert: " + (end - start) + "ms"); + + [ac setClearsFilterPredicateOnInsertion:NO]; + [ac setFilterPredicate:predicate]; + + var start = (new Date).getTime(); + for (var i = 0; i < REPEATS; i++) + { + [ac addObject:[Sortable sortableWithA:i B:i % 3]]; + + var sorted = [ac arrangedObjects], + last = ELEMENTS; + + // Verify that all is well. + for (var j = 0, count = [sorted count]; j < count; j++) + { + if (sorted[j].b == 0) + [self fail:"b == 0 should be filtered out (position: " + j + ")"]; + if (sorted[j].a >= last) + [self fail:"array values should be descending (position: " + j + ")"]; + last = sorted[j]; + } + } + var end = (new Date).getTime(); + CPLog.warn("testAddObject_, sorted, filtered: " + (end - start) + "ms"); } @end From cf04fcc9d050032c3350d9ba3a1a6d25b6a461a8 Mon Sep 17 00:00:00 2001 From: Alexander Ljungberg Date: Sat, 28 May 2011 02:06:27 -0400 Subject: [PATCH 6/6] Verify correctness of observation notifications on arrangedObjects when the addObjects: message is sent. --- Tests/AppKit/CPArrayControllerTest.j | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/Tests/AppKit/CPArrayControllerTest.j b/Tests/AppKit/CPArrayControllerTest.j index 5d5a85157..992852b9f 100644 --- a/Tests/AppKit/CPArrayControllerTest.j +++ b/Tests/AppKit/CPArrayControllerTest.j @@ -412,6 +412,32 @@ [self assert:newSelection equals:[arrayController selectionIndexes] message:@"selection was not set properly"]; } +- (void)testObservationDuringAddObject_ +{ + var arrayController = [self arrayController]; + + [arrayController addObserver:self forKeyPath:@"arrangedObjects" options:CPKeyValueObservingOptionOld | CPKeyValueObservingOptionNew context:nil]; + + // Add something to clear. + [arrayController setFilterPredicate:[CPPredicate predicateWithFormat:@"(name != %@)", "Francisco"]]; + observations = []; + var aPerson = [Employee employeeWithName:@"Alexander" department:[Department departmentWithName:@"Cosmic Path Finding"]]; + + [arrayController setClearsFilterPredicateOnInsertion:NO]; + [self assert:0 equals:[observations count] message:@"no observations before addObject test"]; + [arrayController addObject:aPerson]; + [self assert:1 equals:[observations count] message:@"exactly 1 notification for addObject (clearsFilterPredicate NO)"]; + + observations = []; + + // Even that this is on, adding an object should only result in one notification. + [arrayController setClearsFilterPredicateOnInsertion:YES]; + + [self assert:0 equals:[observations count] message:@"no observations before addObject test"]; + [arrayController addObject:aPerson]; + [self assert:1 equals:[observations count] message:@"exactly 1 notification for addObject (clearsFilterPredicate YES)"]; +} + - (void)testCompoundKeyPaths { var departmentNameField = [[CPTextField alloc] init];