[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.h"
 
 #include "base/logging.h"
 #import "chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h"
 
 namespace {
 
 // NSEvent -buttonNumber for middle mouse button.
 const NSInteger kMiddleButtonNumber = 2;
 
 }  // namespace
 
-@interface OmniboxPopupMatrix()
+@implementation OmniboxPopupTableController
+
+- (id)init {
+  if ((self = [super init])) {
+    array_.reset([[NSMutableArray alloc] init]);
+    hovered_cell_ = -1;
+  }
+  return self;
+}
+
+- (NSInteger)numberOfRowsInTableView:(NSTableView*)tableView {
+  return [array_ count];
+}
+
+- (id)tableView:(NSTableView*)aTableView
+    objectValueForTableColumn:(NSTableColumn*)aTableColumn
+                          row:(NSInteger)rowIndex {
+  // Returning the cell allows selection highlighting to work properly.
+  return [array_ objectAtIndex:rowIndex];

[Scott Hess - ex-Googler, 2015/05/04 19:40:20]

This isn't a cell, though, it's a dictionary.

[dschuyler, 2015/05/04 23:51:40]

Whoops, old comment.  Removed now.

Done.

+}
+
+- (void)tableView:(NSTableView*)aTableView
+    willDisplayCell:(id)aCell

[Scott Hess - ex-Googler, 2015/05/04 19:40:20]

You should use ObjCCast<> or ObjCCastStrict<> to make this something more
specific than id before otherwise messaging it.  One is "Give me one of these or
nil", the other is "Give me one of these or die", probably the latter is in
order here.

[dschuyler, 2015/05/04 23:51:40]

Done.

+     forTableColumn:(NSTableColumn*)aTableColumn
+                row:(NSInteger)rowIndex {
+  NSDictionary* dictionary = [array_ objectAtIndex:rowIndex];
+
+  NSImage* image = [dictionary objectForKey:@"image"];
+  [aCell setImage:image];
+
+  AutocompleteMatchWrapper* acm = [dictionary objectForKey:@"match"];
+  [aCell setMatch:acm->match_];

[Scott Hess - ex-Googler, 2015/05/04 19:40:20]

Since you manufacture the various internal info in -setMatch:, I don't think
setting all that information on every display is necessarily great.  I'd
definitely drop a LOG(ERROR) somewhere and track how often more-expensive things
are being called when you do things like hover around.

You might also want to find someone else to rope in who is more current on
NSTableView.  It's possible my memories of which cases want to cache a bunch of
cells versus which cases want to just re-build the cell on the fly are wrong.

[dschuyler, 2015/05/04 23:51:41]

It does get called during hover/highlight changes.

+
+  [aCell
+      setState:([aTableView selectedRow] == rowIndex) ? NSOnState : NSOffState];
+  [aCell highlight:(hovered_cell_ == rowIndex)
+         withFrame:[aTableView bounds]
+            inView:aTableView];
+}
+
+- (NSInteger)highlightedRow {
+  return hovered_cell_;
+}
+
+- (void)highlightRowAt:(NSInteger)rowIndex withView:(NSView*)view {
+  hovered_cell_ = rowIndex;
+}
+
+- (void)setDataArray:(NSArray*)array {
+  hovered_cell_ = -1;
+  array_.reset([array retain]);
+}
+
+- (CGFloat)tableView:(NSTableView*)tableView heightOfRow:(NSInteger)row {
+  return [[[array_ objectAtIndex:row] objectForKey:@"height"] floatValue];
+}
+
+@end
+
+@interface OmniboxPopupMatrix ()
 - (void)resetTrackingArea;
-- (void)highlightRowAt:(NSInteger)rowIndex;
 - (void)highlightRowUnder:(NSEvent*)theEvent;
 - (BOOL)selectCellForEvent:(NSEvent*)theEvent;
 @end
 
 @implementation OmniboxPopupMatrix
 
 - (id)initWithDelegate:(OmniboxPopupMatrixDelegate*)delegate {
   if ((self = [super initWithFrame:NSZeroRect])) {
     delegate_ = delegate;
-    [self setCellClass:[OmniboxPopupCell class]];
+    controller_.reset([[OmniboxPopupTableController alloc] init]);
+
+    base::scoped_nsobject<NSTableColumn> column(
+        [[NSTableColumn alloc] initWithIdentifier:@"MainCell"]);
+    [column setDataCell:[[[OmniboxPopupCell alloc] init] autorelease]];
+    [self addTableColumn:column];
+
+    [self setDelegate:controller_];
+    [self setDataSource:controller_];
 
     // Cells pack with no spacing.
     [self setIntercellSpacing:NSMakeSize(0.0, 0.0)];
 
-    [self setDrawsBackground:YES];
+    [self setSelectionHighlightStyle:NSTableViewSelectionHighlightStyleNone];
     [self setBackgroundColor:[NSColor controlBackgroundColor]];
-    [self renewRows:0 columns:1];
     [self setAllowsEmptySelection:YES];
-    [self setMode:NSRadioModeMatrix];
-    [self deselectAllCells];
+    [self deselectAll:self];
 
     [self resetTrackingArea];
   }
   return self;
 }
 
-- (void)setDelegate:(OmniboxPopupMatrixDelegate*)delegate {
+- (void)setTheDelegate:(OmniboxPopupMatrixDelegate*)delegate {

[Scott Hess - ex-Googler, 2015/05/04 19:40:20]

WRT our discussing in the CL, I worry that -setTheDelegate: is even worse than
-setCppDelegate:.

[dschuyler, 2015/05/04 23:51:40]

Let's do the name change in another CL.

   delegate_ = delegate;
 }
 
-- (NSInteger)highlightedRow {
-  NSArray* cells = [self cells];
-  const NSUInteger count = [cells count];
-  for(NSUInteger i = 0; i < count; ++i) {
-    if ([[cells objectAtIndex:i] isHighlighted]) {
-      return i;
-    }
-  }
-  return -1;
-}
-
 - (void)updateTrackingAreas {
   [self resetTrackingArea];
   [super updateTrackingAreas];
 }
 
 // Callbacks from tracking area.
 - (void)mouseEntered:(NSEvent*)theEvent {
   [self highlightRowUnder:theEvent];
 }
 
 - (void)mouseMoved:(NSEvent*)theEvent {
   [self highlightRowUnder:theEvent];
 }
 
 - (void)mouseExited:(NSEvent*)theEvent {
-  [self highlightRowAt:-1];
+  [controller_ highlightRowAt:-1 withView:self];
 }
 
 // The tracking area events aren't forwarded during a drag, so handle
 // highlighting manually for middle-click and middle-drag.
 - (void)otherMouseDown:(NSEvent*)theEvent {
   if ([theEvent buttonNumber] == kMiddleButtonNumber) {
     [self highlightRowUnder:theEvent];
   }
   [super otherMouseDown:theEvent];
 }
 
 - (void)otherMouseDragged:(NSEvent*)theEvent {
   if ([theEvent buttonNumber] == kMiddleButtonNumber) {
     [self highlightRowUnder:theEvent];
   }
   [super otherMouseDragged:theEvent];
 }
 
 - (void)otherMouseUp:(NSEvent*)theEvent {
   // Only intercept middle button.
   if ([theEvent buttonNumber] != kMiddleButtonNumber) {
     [super otherMouseUp:theEvent];
     return;
   }
 
   // -otherMouseDragged: should always have been called at this location, but
   // make sure the user is getting the right feedback.
   [self highlightRowUnder:theEvent];
 
-  const NSInteger highlightedRow = [self highlightedRow];
+  const NSInteger highlightedRow = [controller_ highlightedRow];
   if (highlightedRow != -1) {
     DCHECK(delegate_);
     delegate_->OnMatrixRowMiddleClicked(self, highlightedRow);
   }
 }
 
 // Track the mouse until released, keeping the cell under the mouse selected.
 // If the mouse wanders off-view, revert to the originally-selected cell. If
 // the mouse is released over a cell, call the delegate to open the row's URL.
 - (void)mouseDown:(NSEvent*)theEvent {
   NSCell* selectedCell = [self selectedCell];
 
   // Clear any existing highlight.
-  [self highlightRowAt:-1];
+  [controller_ highlightRowAt:-1 withView:self];
 
   do {
     if (![self selectCellForEvent:theEvent]) {
       [self selectCell:selectedCell];
     }
 
     const NSUInteger mask = NSLeftMouseUpMask | NSLeftMouseDraggedMask;
     theEvent = [[self window] nextEventMatchingMask:mask];
   } while ([theEvent type] == NSLeftMouseDragged);
 
   // Do not message the delegate if released outside view.
   if (![self selectCellForEvent:theEvent]) {
     [self selectCell:selectedCell];
   } else {
     const NSInteger selectedRow = [self selectedRow];
 
     // No row could be selected if the model failed to update.
     if (selectedRow == -1) {
       NOTREACHED();
       return;
     }
 
     DCHECK(delegate_);
     delegate_->OnMatrixRowClicked(self, selectedRow);
   }
 }
 
+- (void)selectRowAt:(NSInteger)rowIndex {
+  NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:rowIndex];
+  [self selectRowIndexes:indexSet byExtendingSelection:NO];
+}
+
+- (NSInteger)highlightedRow {
+  return [controller_ highlightedRow];
+}
+
+- (void)setDataArray:(NSArray*)array {
+  [controller_ setDataArray:array];
+}
+
 - (void)resetTrackingArea {
   if (trackingArea_.get())
     [self removeTrackingArea:trackingArea_.get()];
 
   trackingArea_.reset([[CrTrackingArea alloc]
       initWithRect:[self frame]
            options:NSTrackingMouseEnteredAndExited |
                    NSTrackingMouseMoved |
                    NSTrackingActiveInActiveApp |
                    NSTrackingInVisibleRect
              owner:self
           userInfo:nil]);
   [self addTrackingArea:trackingArea_.get()];
 }
 
-- (void)highlightRowAt:(NSInteger)rowIndex {
-  // highlightCell will be nil if rowIndex is out of range, so no cell will be
-  // highlighted.
-  NSCell* highlightCell = [self cellAtRow:rowIndex column:0];
-
-  for (NSCell* cell in [self cells]) {
-    [cell setHighlighted:(cell == highlightCell)];
-  }
-}
-
 - (void)highlightRowUnder:(NSEvent*)theEvent {
   NSPoint point = [self convertPoint:[theEvent locationInWindow] fromView:nil];
-  NSInteger row, column;
-  if ([self getRow:&row column:&column forPoint:point]) {
-    [self highlightRowAt:row];
-  } else {
-    [self highlightRowAt:-1];
-  }
+  [controller_ highlightRowAt:[self rowAtPoint:point] withView:self];
+  [self setNeedsDisplay:YES];
 }
 
 // Select cell under |theEvent|, returning YES if a selection is made.
 - (BOOL)selectCellForEvent:(NSEvent*)theEvent {
   NSPoint point = [self convertPoint:[theEvent locationInWindow] fromView:nil];
 
-  NSInteger row, column;
-  if ([self getRow:&row column:&column forPoint:point]) {
-    DCHECK_EQ(column, 0);
+  NSInteger row = [self rowAtPoint:point];
+  [self selectRowAt:row];
+  if (row != -1) {
     DCHECK(delegate_);
     delegate_->OnMatrixRowSelected(self, row);
     return YES;
   }
   return NO;
 }
 
 @end