diff --git a/AppKit/CPArrayController.j b/AppKit/CPArrayController.j index b04f4e0f9..724a2edee 100644 --- a/AppKit/CPArrayController.j +++ b/AppKit/CPArrayController.j @@ -58,22 +58,30 @@ + (CPSet)keyPathsForValuesAffectingArrangedObjects { - return [CPSet setWithObjects:"content", "contentArray", "contentSet", "filterPredicate", "sortDescriptors"]; + return [CPSet setWithObjects:"content", "filterPredicate", "sortDescriptors"]; } + (CPSet)keyPathsForValuesAffectingSelection { - return [CPSet setWithObjects:"content", "contentArray", "contentSet", "selectionIndexes"]; + return [CPSet setWithObjects:"selectionIndexes"]; } + (CPSet)keyPathsForValuesAffectingSelectionIndex { - return [CPSet setWithObjects:"content", "contentArray", "contentSet", "selectionIndexes", "selection"]; + return [CPSet setWithObjects:"selectionIndexes"]; +} + ++ (CPSet)keyPathsForValuesAffectingSelectionIndexes +{ + // When the arranged objects change, selection preservation may cause the indexes + // to change. + return [CPSet setWithObjects:"arrangedObjects"]; } + (CPSet)keyPathsForValuesAffectingSelectedObjects { - return [CPSet setWithObjects:"content", "contentArray", "contentSet", "selectionIndexes", "selection"]; + // Don't need to depend on arrangedObjects here because selectionIndexes already does. + return [CPSet setWithObjects:"selectionIndexes"]; } + (CPSet)keyPathsForValuesAffectingCanRemove @@ -91,16 +99,6 @@ return [CPSet setWithObjects:"selectionIndexes"]; } -+ (BOOL)automaticallyNotifiesObserversForKey:(CPString)aKey -{ - if (![super automaticallyNotifiesObserversForKey:aKey]) - return NO; - if (aKey === @"selectionIndexes") - return NO; - - return YES; -} - - (id)init { self = [super init]; @@ -153,31 +151,50 @@ if(![value isKindOfClass:[CPArray class]]) value = [value]; - var oldSelection = nil, - oldSelectionIndexes = [self selectionIndexes]; + var oldSelectedObjects = nil, + oldSelectionIndexes = nil; if ([self preservesSelection]) - oldSelection = [self selectedObjects]; + oldSelectedObjects = [self selectedObjects]; + else + oldSelectionIndexes = [self selectionIndexes]; - // Avoid out of bounds selections. - _selectionIndexes = [CPIndexSet indexSet]; - //FIXME: copy? - [super setContent:value]; + /* + When the contents are changed, the selected indexes may no longer refer to the + same items. This would cause problems when setSelectedObjects is called below. + Any KVO observation would try to retrieve the 'before' value which could be + wrong or even throw an exception for no longer existing indexes. + + To avoid that, use the internal __setSelectedObjects which fires no notifications. + The selectionIndexes notifications will fire later since they depend on the + content key. This pattern is also applied for many other methods throughout this + class. + */ + + if (_clearsFilterPredicateOnInsertion) + [self willChangeValueForKey:@"filterPredicate"]; + + // Don't use [super setContent:] as that would fire the contentObject change. + // We need to be in control of when notifications fire. + _contentObject = value; if(_clearsFilterPredicateOnInsertion) - [self setFilterPredicate:nil]; - - [self rearrangeObjects]; - - if (oldSelection) - [self setSelectedObjects:oldSelection]; + [self __setFilterPredicate:nil]; // Causes a _rearrangeObjects. else - [self setSelectionIndexes:oldSelectionIndexes]; + [self _rearrangeObjects]; + + if ([self preservesSelection]) + [self __setSelectedObjects:oldSelectedObjects]; + else + [self __setSelectionIndexes:oldSelectionIndexes]; + + if (_clearsFilterPredicateOnInsertion) + [self didChangeValueForKey:@"filterPredicate"]; } - (void)_setContentArray:(id)anArray { - [self setContent:anArray]; + [self setContent:anArray]; } - (void)_setContentSet:(id)aSet @@ -216,30 +233,38 @@ - (void)rearrangeObjects { - // Rearranging reapplies the selection criteria and may cause objects to disappear, - // so take care of the selection. - // - // Sometimes rearrangeObjects is called by setContent which may cause two rounds of - // selection preservation. This is okay because setContent temporarily clears the - // selection and so this code below ends up preserving nothing in that case. - var oldSelection = nil, - oldSelectionIndexes = [[self selectionIndexes] copy]; - - if ([self preservesSelection]) - oldSelection = [self selectedObjects]; - - // Avoid out of bounds selections. - _selectionIndexes = [CPIndexSet indexSet]; - - [self _setArrangedObjects:[self arrangeObjects:[self contentArray]]]; - - if (oldSelection) - [self setSelectedObjects:oldSelection]; - else - [self setSelectionIndexes:oldSelectionIndexes]; + [self willChangeValueForKey:@"arrangedObjects"]; + [self _rearrangeObjects]; + [self didChangeValueForKey:@"arrangedObjects"]; } -- (void)_setArrangedObjects:(id)value +/* + Like rearrangeObjects but don't fire any change notifications. + @ignore +*/ +- (void)_rearrangeObjects +{ + /* + Rearranging reapplies the selection criteria and may cause objects to disappear, + so take care of the selection. + */ + var oldSelectedObjects = nil, + oldSelectionIndexes = nil; + + if ([self preservesSelection]) + oldSelectedObjects = [self selectedObjects]; + else + oldSelectionIndexes = [self selectionIndexes]; + + [self __setArrangedObjects:[self arrangeObjects:[self contentArray]]]; + + if ([self preservesSelection]) + [self __setSelectedObjects:oldSelectedObjects]; + else + [self __setSelectionIndexes:oldSelectionIndexes]; +} + +- (void)__setArrangedObjects:(id)value { if (_arrangedObjects === value) return; @@ -252,7 +277,6 @@ return _arrangedObjects; } - - (CPArray)sortDescriptors { return _sortDescriptors; @@ -264,7 +288,9 @@ return; _sortDescriptors = [value copy]; - [self rearrangeObjects]; + // Use the non-notification version since arrangedObjects already depends + // on sortDescriptors. + [self _rearrangeObjects]; } - (CPPredicate)filterPredicate @@ -273,12 +299,23 @@ } - (void)setFilterPredicate:(CPPredicate)value +{ + [self __setFilterPredicate:value]; +} + +/* + Like setFilterPredicate but don't fire any change notifications. + @ignore +*/ +- (void)__setFilterPredicate:(CPPredicate)value { if (_filterPredicate === value) return; _filterPredicate = value; - [self rearrangeObjects]; + // Use the non-notification version since arrangedObjects already depends + // on filterPredicate. + [self _rearrangeObjects]; } - (BOOL)alwaysUsesMultipleValuesMarker @@ -305,6 +342,18 @@ - (BOOL)setSelectionIndexes:(CPIndexSet)indexes { + [self __setSelectionIndexes:indexes]; +} + +/* + Like setSelectionIndexes but don't fire any change notifications. + @ignore +*/ +- (BOOL)__setSelectionIndexes:(CPIndexSet)indexes +{ + if (!indexes) + indexes = [CPIndexSet indexSet]; + if ([_selectionIndexes isEqualToIndexSet:indexes]) return NO; @@ -323,14 +372,8 @@ indexes = [CPIndexSet indexSetWithIndex:objectsCount-1]; } - [self willChangeValueForKey:@"selectionIndexes"]; - [self _selectionWillChange]; - _selectionIndexes = [indexes copy]; - [self _selectionDidChange]; - [self didChangeValueForKey:@"selectionIndexes"]; - // Push back the new selection to the model for selectionIndexes if we have one. // There won't be an infinite loop because of the equality check above. [[CPKeyValueBinding getBinding:@"selectionIndexes" forObject:self] reverseSetValueFor:@"selectionIndexes"]; @@ -346,6 +389,21 @@ } - (BOOL)setSelectedObjects:(CPArray)objects +{ + [self willChangeValueForKey:@"selectionIndexes"]; + [self _selectionWillChange]; + + [self __setSelectedObjects:objects]; + + [self didChangeValueForKey:@"selectionIndexes"]; + [self _selectionDidChange]; +} + +/* + Like setSelectedObjects but don't fire any change notifications. + @ignore +*/ +- (BOOL)__setSelectedObjects:(CPArray)objects { var set = [CPIndexSet indexSet], count = [objects count], @@ -359,7 +417,7 @@ [set addIndex:index]; } - [self setSelectionIndexes:set]; + [self __setSelectionIndexes:set]; return YES; } @@ -398,30 +456,32 @@ if (![self canAdd]) return; + if (_clearsFilterPredicateOnInsertion) + [self willChangeValueForKey:@"filterPredicate"]; + [self willChangeValueForKey:@"content"]; [_contentObject addObject:object]; - [self didChangeValueForKey:@"content"]; if (_clearsFilterPredicateOnInsertion) - [self setFilterPredicate:nil]; + [self __setFilterPredicate:nil]; if (_filterPredicate === nil || [_filterPredicate evaluateWithObject:object]) { var pos = [_arrangedObjects insertObject:object inArraySortedByDescriptors:_sortDescriptors]; + // selectionIndexes change notification will be fired as a result of the + // content change. Don't fire manually. if (_selectsInsertedObjects) - { - [self setSelectionIndex:pos]; - } + [self __setSelectionIndex:pos]; else - { - [self willChangeValueForKey:@"selectionIndexes"]; [_selectionIndexes shiftIndexesStartingAtIndex:pos by:1]; - [self didChangeValueForKey:@"selectionIndexes"]; - } } else - [self rearrangeObjects]; + [self _rearrangeObjects]; + + [self didChangeValueForKey:@"content"]; + if (_clearsFilterPredicateOnInsertion) + [self didChangeValueForKey:@"filterPredicate"]; } - (void)insertObject:(id)anObject atArrangedObjectIndex:(int)anIndex @@ -429,26 +489,30 @@ if (![self canAdd]) return; + if (_clearsFilterPredicateOnInsertion) + [self willChangeValueForKey:@"filterPredicate"]; + [self willChangeValueForKey:@"content"]; [_contentObject insertObject:anObject atIndex:anIndex]; - [self didChangeValueForKey:@"content"]; if (_clearsFilterPredicateOnInsertion) - [self setFilterPredicate:nil]; + [self __setFilterPredicate:nil]; [[self arrangedObjects] insertObject:anObject atIndex:anIndex]; + // selectionIndexes change notification will be fired as a result of the + // content change. Don't fire manually. if ([self selectsInsertedObjects]) - [self setSelectionIndex:anIndex]; + [self __setSelectionIndex:anIndex]; else - { - [self willChangeValueForKey:@"selectionIndexes"] [[self selectionIndexes] shiftIndexesStartingAtIndex:anIndex by:1]; - [self didChangeValueForKey:@"selectionIndexes"]; - } if ([self avoidsEmptySelection] && [[self selectionIndexes] count] <= 0 && [_contentObject count] > 0) - [self setSelectionIndexes:[CPIndexSet indexSetWithIndex:0]]; + [self __setSelectionIndexes:[CPIndexSet indexSetWithIndex:0]]; + + [self didChangeValueForKey:@"content"]; + if (_clearsFilterPredicateOnInsertion) + [self didChangeValueForKey:@"filterPredicate"]; } - (void)removeObject:(id)object @@ -458,18 +522,18 @@ [self willChangeValueForKey:@"content"]; [_contentObject removeObject:object]; - [self didChangeValueForKey:@"content"]; if (_filterPredicate === nil || [_filterPredicate evaluateWithObject:object]) { - [self willChangeValueForKey:@"selectionIndexes"]; + // selectionIndexes change notification will be fired as a result of the + // content change. Don't fire manually. var pos = [_arrangedObjects indexOfObject:object]; [_arrangedObjects removeObjectAtIndex:pos]; [_selectionIndexes shiftIndexesStartingAtIndex:pos by:-1]; - - [self didChangeValueForKey:@"selectionIndexes"]; } + + [self didChangeValueForKey:@"content"]; } -(void)add:(id)sender @@ -527,7 +591,6 @@ { [self willChangeValueForKey:@"content"]; [_contentObject removeObjectsInArray:objects]; - [self didChangeValueForKey:@"content"]; var arrangedObjects = [self arrangedObjects], position = [arrangedObjects indexOfObject:[objects objectAtIndex:0]]; @@ -550,9 +613,9 @@ selectionIndexes = [CPIndexSet indexSetWithIndex:objectsCount - 1]; } - [self willChangeValueForKey:@"selectionIndexes"]; _selectionIndexes = selectionIndexes; - [self didChangeValueForKey:@"selectionIndexes"]; + + [self didChangeValueForKey:@"content"]; } - (BOOL)canInsert diff --git a/Foundation/CPKeyValueObserving.j b/Foundation/CPKeyValueObserving.j index 6ea9c9a42..a08100481 100644 --- a/Foundation/CPKeyValueObserving.j +++ b/Foundation/CPKeyValueObserving.j @@ -27,7 +27,6 @@ @import "CPObject.j" @import "CPSet.j" - @implementation CPObject (KeyValueObserving) - (void)willChangeValueForKey:(CPString)aKey @@ -394,6 +393,7 @@ var kvoNewAndOld = CPKeyValueObservingOptionNew|CPKeyValueObservingOptionOld, - (void)_sendNotificationsForKey:(CPString)aKey changeOptions:(CPDictionary)changeOptions isBefore:(BOOL)isBefore { + // CPLog.warn("_sendNotificationsForKey: " + aKey + " ...isBefore: " + isBefore); var changes = _changesForKey[aKey]; if (isBefore) @@ -492,6 +492,9 @@ var kvoNewAndOld = CPKeyValueObservingOptionNew|CPKeyValueObservingOptionOld, { var keyPath = dependentKeyPaths[index]; + // CPLog.warn("firing dependepent key " + index + " for " + aKey + ": "+keyPath); + // objj_backtrace_print(CPLog.error); + [self _sendNotificationsForKey:keyPath changeOptions:isBefore ? [changeOptions copy] : _changesForKey[keyPath] isBefore:isBefore]; diff --git a/Tests/AppKit/CPArrayControllerTest.j b/Tests/AppKit/CPArrayControllerTest.j index d311ccd56..aa1b2676e 100644 --- a/Tests/AppKit/CPArrayControllerTest.j +++ b/Tests/AppKit/CPArrayControllerTest.j @@ -1,7 +1,9 @@ @implementation CPArrayControllerTest : OJTestCase { - CPArrayController _arrayController @accessors(property=arrayController); - CPArray _contentArray @accessors(property=contentArray); + CPArrayController _arrayController @accessors(property=arrayController); + CPArray _contentArray @accessors(property=contentArray); + + CPArray observations; } - (void)setUp @@ -19,7 +21,7 @@ - (void)testInitWithContent { [self assert:[self contentArray] equals:[[self arrayController] contentArray]]; - [self assert:[_CPObservableArray class] equals:[[[self arrayController] arrangedObjects] class]]; + [self assert:[_CPObservableArray class] equals:[[[self arrayController] arrangedObjects] class] message:"arranged objects should be observable"]; } - (void)testSetContent @@ -125,12 +127,126 @@ [self assert:[[self arrayController] contentArray] equals:[self contentArray]]; } +- (CPArray)setupObservationFixture +{ + // objj_msgSend_decorate(objj_backtrace_decorator); + + var ac = [self arrayController]; + [ac setSelectionIndex:0]; + [ac setPreservesSelection:YES]; + var newContent = [_contentArray copy]; + [newContent removeObjectAtIndex:0]; + + [ac addObserver:self forKeyPath:"content" options:CPKeyValueObservingOptionOld | CPKeyValueObservingOptionNew context:nil]; + [ac addObserver:self forKeyPath:"selectionIndexes" options:CPKeyValueObservingOptionOld | CPKeyValueObservingOptionNew context:nil]; + [ac addObserver:self forKeyPath:"selectedObjects" options:CPKeyValueObservingOptionOld | CPKeyValueObservingOptionNew context:nil]; + [ac addObserver:self forKeyPath:"arrangedObjects" options:CPKeyValueObservingOptionOld | CPKeyValueObservingOptionNew context:nil]; + + observations = []; + + return newContent; +} + +- (void)testObservationDuringSetContent +{ + /* + There are two areas where CPArrayController can get into trouble with observation. + + The more serious one is that the before values when observing selectedObjects + need to be correct and not affected by the new setContent: values. + + Second, the array controller should not send out more than one of each notification + since repeated notifications for the same change can be a huge performance drain + from such a central piece of code. + */ + + var ac = [self arrayController], + newContent = [self setupObservationFixture]; + + [ac setContent:newContent]; + + [observations sortUsingFunction:function(a, b) { return [a.keyPath compare:b.keyPath] } context:nil]; + [self assert:4 equals:[observations count] message:"exactly 4 change notifications should be sent for new content"]; + + // for (var i=0; i", [self name], [self age]]; } @end \ No newline at end of file