[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

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

Index: chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
index 3548b9f9881a2f951b9912894c79b92dfc3d4e94..80f28aec3342a4a1841075870b299c082e59f29b 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
@@ -5,6 +5,7 @@
 #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 {
@@ -14,9 +15,68 @@ 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
@@ -26,17 +86,25 @@ const NSInteger kMiddleButtonNumber = 2;
 - (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];
   }
@@ -47,17 +115,6 @@ const NSInteger kMiddleButtonNumber = 2;
   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];
@@ -73,7 +130,7 @@ const NSInteger kMiddleButtonNumber = 2;
 }
 
 - (void)mouseExited:(NSEvent*)theEvent {
-  [self highlightRowAt:-1];
+  [controller_ highlightRowAt:-1 withView:self];
 }
 
 // The tracking area events aren't forwarded during a drag, so handle
@@ -103,7 +160,7 @@ const NSInteger kMiddleButtonNumber = 2;
   // 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);
@@ -117,7 +174,7 @@ const NSInteger kMiddleButtonNumber = 2;
   NSCell* selectedCell = [self selectedCell];
 
   // Clear any existing highlight.
-  [self highlightRowAt:-1];
+  [controller_ highlightRowAt:-1 withView:self];
 
   do {
     if (![self selectCellForEvent:theEvent]) {
@@ -145,6 +202,19 @@ const NSInteger kMiddleButtonNumber = 2;
   }
 }
 
+- (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()];
@@ -160,33 +230,23 @@ const NSInteger kMiddleButtonNumber = 2;
   [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;