New implementation of change notifications from CPArrayController.

It is crucial that we send notification only before and after complete changes. In the middle notifications might cause observers to see or react to inconsistent data (e.g. selection indexes pointing to rows no longer present). Added some unit tests - more might be needed in the future. Fixed: before and after values when observing array controller key paths during content changes or rearranges were wrong. Fixed: the array controller sent out multiple redundant change notifications.

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