[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"
+#include "base/mac/foundation_util.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([[NSArray alloc] init]);

[groby-ooo-7-16, 2015/05/20 01:02:30]

Why reset the array_ to an empty array?

[dschuyler, 2015/05/21 00:38:39]

Likely because I only just learned about nil as a valid message target.  :)

+    hovered_index_ = -1;
+  }
+  return self;
+}
+
+- (NSInteger)numberOfRowsInTableView:(NSTableView*)tableView {
+  return [array_ count];
+}
+
+- (id)tableView:(NSTableView*)tableView
+    objectValueForTableColumn:(NSTableColumn*)tableColumn
+                          row:(NSInteger)rowIndex {
+  return nil;

[groby-ooo-7-16, 2015/05/20 01:02:31]

This still makes me queasy. You are currently not implementing the
NSTableViewDataSource API as specified.

It's fine to skip setObjectValue for read-only tables, but this technically
_needs_ to be implemented.

Can you put a breakpoint/LOG in here and see if this is ever called?

[dschuyler, 2015/05/21 00:38:39]

It is being called.

Here's a link to a prior version of this function that returned a value and the
discussion about it:
https://codereview.chromium.org/1099403005/diff/200001/chrome/browser/ui/coco...

Should I change it back to returning a value?

[groby-ooo-7-16, 2015/05/21 03:01:48]

No, we should try to understand what's actually going on first :) (Seriously.
While Cocoa is mysterious, please make sure you understand why things are
there/need to be done. It'll make it much less fragile)

Implementing the data source is technically enough to display a cell-based
NSTableView, as per docs. (Everything on NSTableViewDelegate is optional
customization)

So, where is the value from the datasource communicated to the cell? The docs
are unfortunately not very clear on the subject, but the fact that this is
objectValueForTableColumn means likely that this is an objectValue for an
NSCell. 

But don't trust me, verify for yourself. print out callstacks from here and from
your cell's setObjectValue:

Better, turn on prints of all messages as sent around this, via
instrumentObjcMessageSends. (See this article on debugging message flow:
http://www.dribin.org/dave/blog/archives/2006/04/22/tracing_objc/ 
And read the technote on debugging, which will change your life:
https://developer.apple.com/library/mac/technotes/tn2124/_index.html)

You'll see that as the NSTableView displays individual cells, it will call this
function to get an objectValue, and then set it on the appropriate cell.

[Scott Hess - ex-Googler, 2015/05/21 04:46:35]

Something I have often done is created a distinct Xcode project and experimented
over there to figure out what's up in a non-Chrome environment.  The
Edit/Compile/Run loop is about infinity faster, so you can nail something down
in 15 minutes that might take a long grind in a Chrome context.

[dschuyler, 2015/05/26 18:40:19]

I'll look into this further.  I'm going to put an interim patch in without this.

[groby-ooo-7-16, 2015/05/26 21:23:22]

Waiting for the final patch before I do a full review, then. 

[dschuyler, 2015/05/28 20:34:22]

Done.

+}
+
+- (void)tableView:(NSTableView*)aTableView
+    setObjectValue:(id)anObject
+    forTableColumn:(NSTableColumn*)aTableColumn
+               row:(NSInteger)rowIndex {
+}
+
+- (void)tableView:(NSTableView*)tableView
+    willDisplayCell:(id)cell
+     forTableColumn:(NSTableColumn*)tableColumn
+                row:(NSInteger)rowIndex {
+  OmniboxPopupCell* popupCell =
+      base::mac::ObjCCastStrict<OmniboxPopupCell>(cell);
+  [popupCell setCellData:[array_ objectAtIndex:rowIndex]];
+  [popupCell
+      setState:([tableView selectedRow] == rowIndex) ? NSOnState : NSOffState];
+  [popupCell highlight:(hovered_index_ == rowIndex)

[groby-ooo-7-16, 2015/05/20 01:02:30]

hoveredIndex_ :)

Which brings up a question - can we have cells that are simultaneously
highlighted and hovered?

[dschuyler, 2015/05/21 00:38:39]

Highlighted and hovered are always in sync (or should be).  Selected and
highlights can be separate or the same.  If they are the same, then the Selected
value takes precedence in the GUI.

Done.

+             withFrame:[tableView bounds]
+                inView:tableView];
+}
+
+- (NSInteger)highlightedRow {
+  return hovered_index_;
+}
+
+- (void)highlightRowAt:(NSInteger)rowIndex withView:(NSView*)view {
+  hovered_index_ = rowIndex;
+}
+
+- (void)setDataArray:(NSArray*)array {
+  hovered_index_ = -1;
+  array_.reset([array retain]);
+}
+
+- (CGFloat)tableView:(NSTableView*)tableView heightOfRow:(NSInteger)row {
+  OmniboxPopupCellData* cellData = [array_ objectAtIndex:row];
+  return [cellData rowHeight];
+}
+
+@end
+
+@interface OmniboxPopupMatrix ()
 - (void)resetTrackingArea;
-- (void)highlightRowAt:(NSInteger)rowIndex;
 - (void)highlightRowUnder:(NSEvent*)theEvent;
 - (BOOL)selectCellForEvent:(NSEvent*)theEvent;
 @end
 
 @implementation OmniboxPopupMatrix
 
 - (id)initWithObserver:(OmniboxPopupMatrixObserver*)observer {
   if ((self = [super initWithFrame:NSZeroRect])) {
     observer_ = observer;
-    [self setCellClass:[OmniboxPopupCell class]];
+    controller_.reset([[OmniboxPopupTableController alloc] init]);
+
+    base::scoped_nsobject<NSTableColumn> column(
+        [[NSTableColumn alloc] initWithIdentifier:@"MainCell"]);
+    base::scoped_nsobject<OmniboxPopupCell> columnCell(
+        [[OmniboxPopupCell alloc] init]);
+    [column setDataCell:columnCell];
+    [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)setObserver:(OmniboxPopupMatrixObserver*)observer {
   observer_ = observer;
 }
 
-- (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(observer_);
     observer_->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(observer_);
     observer_->OnMatrixRowClicked(self, selectedRow);
   }
 }
 
+- (void)selectRowAt:(NSInteger)rowIndex {
+  NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:rowIndex];
+  [self selectRowIndexes:indexSet byExtendingSelection:NO];
+}
+
+- (NSInteger)highlightedRow {
+  return [controller_ highlightedRow];

[groby-ooo-7-16, 2015/05/20 01:02:31]

Why not [self selectedRow] ?

[dschuyler, 2015/05/21 00:38:39]

Highlighting is for mouse hovering.  Selection is a different thing.

+}
+
+- (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];
+  NSInteger oldRow = [controller_ highlightedRow];
+  NSInteger newRow = [self rowAtPoint:point];
+  if (oldRow != newRow) {
+    [controller_ highlightRowAt:newRow withView:self];
+    [self setNeedsDisplayInRect:[self rectOfRow:oldRow]];
+    [self setNeedsDisplayInRect:[self rectOfRow:newRow]];
   }
 }
 
-// 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(observer_);
     observer_->OnMatrixRowSelected(self, row);

[groby-ooo-7-16, 2015/05/20 01:02:31]

Do we not want to tell the observer when nothing is selected?

[dschuyler, 2015/05/21 00:38:39]

We keep the last selected item selected.  Telling it to deselect would be a
departure from the current GUI behavior.

     return YES;
   }
   return NO;
 }
 
 @end