Merge pull request #2853 from mrcarlberg/optimize_table_column_bindings_with_long_key_paths

Fixed: Optimization for bindings with longer key paths in table views.

AppKit/CPTableColumn.j

@@ -639,6 +639,53 @@ CPTableColumnUserResizingMask   = 1 << 1;
     }
 }
 
+
+/*!
+    @ignore
+
+    This method will return the object at a row in the first found CPArray in the key path
+    that is divided in a first and second part.
+    The first part is never a combined key path. The second part can be a combined key path.
+    If this optimization is not done we will create an array with the valueForKeyPath value on each row and then pick
+    the wanted value for the row and throw away all the other rows. It is much more effective to first
+    pick the row and then do the valueForKeyPath on the rest of the key path.
+    When the second part is depleated it will stop the search for a CPArray and return the
+    current object for the first part. The second part will then be nil.
+    The secondPartRef will always be updated with the rest of the key path that can be applied
+    to the returned object.
+    It will stop the search if the object is nil
+*/
+- (CPValueCoding)_firstObjectInArrayUsingKeyPathFirstPart:(CPString)firstPart secondPart:(CPStringRef)secondPartRef sourceObject:(CPValueCoding)source forRow:(unsigned)aRow
+{
+    var firstValue = [source valueForKeyPath:firstPart];
+
+    if (firstValue == nil)
+        return firstValue;
+
+    if ([firstValue isKindOfClass:CPArray])
+        return [firstValue objectAtIndex:aRow];
+
+    var secondPart = @deref(secondPartRef);
+
+    if (secondPart == nil)
+        return firstValue;
+
+    var dotIndex = secondPart.indexOf(".");
+
+    if (dotIndex === CPNotFound)
+    {
+        firstPart = secondPart;
+        @deref(secondPartRef) = nil;
+    }
+    else
+    {
+        firstPart = secondPart.substring(0, dotIndex);
+        @deref(secondPartRef) = secondPart.substring(dotIndex + 1);
+    }
+
+    return [self _firstObjectInArrayUsingKeyPathFirstPart:firstPart secondPart:secondPartRef sourceObject:firstValue forRow:aRow];
+}
+
 /*!
     @ignore
 */
@@ -655,7 +702,7 @@ CPTableColumnUserResizingMask   = 1 << 1;
             bindingInfo = binding._info,
             destination = [bindingInfo objectForKey:CPObservedObjectKey],
             keyPath = [bindingInfo objectForKey:CPObservedKeyPathKey],
-            dotIndex = keyPath.lastIndexOf("."),
+            dotIndex = keyPath.indexOf("."),
             value;
 
         if (dotIndex === CPNotFound)
@@ -670,17 +717,16 @@ CPTableColumnUserResizingMask   = 1 << 1;
 
                 The optimization is to get the array and access the value directly. This
                 turns the operation into a single access regardless of how long the model
-                array is.
+                array is or how long the key path is.
             */
 
             var firstPart = keyPath.substring(0, dotIndex),
-                secondPart = keyPath.substring(dotIndex + 1),
-                firstValue = [destination valueForKeyPath:firstPart];
+                secondPart = keyPath.substring(dotIndex + 1);
 
-            if ([firstValue isKindOfClass:CPArray])
-                value = [[firstValue objectAtIndex:aRow] valueForKeyPath:secondPart];
-            else
-                value = [[firstValue valueForKeyPath:secondPart] objectAtIndex:aRow];
+            value = [self _firstObjectInArrayUsingKeyPathFirstPart:firstPart secondPart:@ref(secondPart) sourceObject:destination forRow:aRow];
+
+            if (secondPart != nil)
+                value = [value valueForKeyPath:secondPart];
         }
 
         value = [binding transformValue:value withOptions:[bindingInfo objectForKey:CPOptionsKey]];

Tests/AppKit/CPTableViewTest.j

@@ -667,6 +667,69 @@
 
 }
 
+/*!
+    Test that we don't build up an array with all the value for each key path and then pick the row and
+    throws away all the other values. This is performance critical as this is done for every column on every row.
+
+    This is tested by setting up a key path to the name property that is combined. We count the number of times each
+    part in the key path is accessed with valueForKey.
+*/
+- (void)testLazyPrepareDataView
+{
+    //objj_msgSend_decorate(objj_backtrace_decorator);
+
+    var frameRect = CGRectMake(0, 0, 1024, 64);
+
+    [theWindow setFrame:frameRect];
+
+    var table = [[CPTableView alloc] initWithFrame:CGRectMake(0, 0, 100, 100)],
+        container = [TestDataSource new];
+
+    var tc1 = [[CPTableColumn alloc] initWithIdentifier:@"A"];
+    [table addTableColumn:tc1];
+
+    var tc2 = [[CPTableColumn alloc] initWithIdentifier:@"B"];
+    [table addTableColumn:tc2];
+
+    var scrollView = [[CPScrollView alloc] initWithFrame:frameRect];
+
+    [[theWindow contentView] addSubview:scrollView];
+    [scrollView setDocumentView:table];
+    [theWindow makeFirstResponder:table];
+
+    var i = 1;
+    var dataArray = @[
+        [ValueForKeyCountingDictionary dictionaryWithObject:[ValueForKeyCountingDictionary dictionaryWithObject:@{@"name":@"B"+i, @"longname": @"BB"+i++} forKey:@"b"] forKey:@"a"],
+        [ValueForKeyCountingDictionary dictionaryWithObject:[ValueForKeyCountingDictionary dictionaryWithObject:@{@"name":@"B"+i, @"longname": @"BB"+i++} forKey:@"b"] forKey:@"a"],
+        [ValueForKeyCountingDictionary dictionaryWithObject:[ValueForKeyCountingDictionary dictionaryWithObject:@{@"name":@"B"+i, @"longname": @"BB"+i++} forKey:@"b"] forKey:@"a"],
+    ];
+
+    var ac = [[CPArrayController alloc] init];
+    [ac bind:@"contentArray" toObject:container withKeyPath:@"tableEntries" options:nil];
+    [tc1 bind:@"value" toObject:ac withKeyPath:@"arrangedObjects.a.b.name" options:nil];
+    [tc2 bind:@"value" toObject:ac withKeyPath:@"arrangedObjects.a.b.longname" options:nil];
+
+    [container setTableEntries:dataArray];
+
+    // The first two are visible so they are accessed both when setting up the observers and displaying the cell in the table view
+    [self assert:[[dataArray[0] countingDictionary] objectForKey:@"a"] equals:5];
+    [self assert:[[dataArray[1] countingDictionary] objectForKey:@"a"] equals:5];
+     // This is not visible so it is only accessed when setting up the observers
+    [self assert:[[dataArray[2] countingDictionary] objectForKey:@"a"] equals:2];
+
+    var enumerateViewsInRowsCall = 0;
+
+    // Checks that the displayed data matches the model data.
+    [table enumerateAvailableViewsUsingBlock:function(dataView, aRow, aColumn, stop)
+    {
+        enumerateViewsInRowsCall++;
+        var data_value = [[[container tableEntries] objectAtIndex:aRow] valueForKeyPath:aColumn === 0 ? @"a.b.name" : @"a.b.longname"];
+        [self assert:[dataView objectValue] equals:data_value];
+    }];
+
+    [self assert:enumerateViewsInRowsCall equals:4]; // Only 4 views visible
+}
+
 @end
 
 @implementation FirstResponderConfigurableTableView : CPTableView
@@ -752,6 +815,35 @@
 @implementation CustomTextView1 : CPTextField
 @end
 
+@implementation ValueForKeyCountingDictionary : CPMutableDictionary
+{
+    CPMutableDictionary countingDictionary @accessors;
+}
+
+- (id)initWithObjects:(CPArray)objects forKeys:(CPArray)keyArray
+{
+    self = [super initWithObjects:objects forKeys:keyArray];
+    if (self) {
+        countingDictionary = [CPDictionary new];
+    }
+
+    return self;
+}
+
+- (id)valueForKey:(CPString)aKey
+{
+    var countsForKey = [countingDictionary objectForKey:aKey];
+
+    countsForKey = (countsForKey == nil ? 1 : countsForKey + 1);
+    //print([self UID] + @": valueForKey:" + aKey + ", countForKey: " + countsForKey);
+    //objj_backtrace_print(CPLog.error);
+    [countingDictionary setObject:countsForKey forKey:aKey];
+
+    return [super valueForKey:aKey];
+}
+
+@end
+
 var getAllViews = function(aView)
 {
     var views = [aView],