[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 9000941c26cea3536cdcc0166a58e9597d9a4aa9..1c3601ec52369d51c202db544a559f39b2dd90bf 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
@@ -14,41 +14,100 @@ const NSInteger kMiddleButtonNumber = 2;
 
 }  // namespace
 
-@interface OmniboxPopupMatrix()
+@implementation OmniboxPopupTableController
+
+- (id)init {
+  if ((self = [super init])) {
+    array_ = [[NSMutableArray alloc] init];
+  }
+  return self;
+}
+
+- (NSInteger)numberOfRowsInTableView:(NSTableView*)tableView {
+  if (!array_)

[dschuyler, 2015/04/24 18:43:08]

Checking whether the member is initialized comes from advice in the Apple docs. 
They caution that numberOfRowsInTableView may be called at unexpected times and
that one should guard against this possibility in this call.

[groby-ooo-7-16, 2015/04/24 20:11:16]

I'm confused. If the member is not initialized, this becomes [nil count], which
automatically evaluates to 0. Do you have a link/quote?

[dschuyler, 2015/04/25 01:05:42]

Ah, so I was trying to do right by the 'Important' call out on this page.
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Applica...

Your comment explains that 'if' is not necessary - the code will still satisfy
the call out on that page.  Thanks!

Done.

+    return 0;
+  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];
+}
+
+- (NSCell*)tableView:(NSTableView*)tableView
+    dataCellForTableColumn:(NSTableColumn*)tableColumn
+                       row:(NSInteger)row {
+  // Returning the cell populates data in the table.
+  return [array_ objectAtIndex:row];
+}
+
+- (void)setCellCount:(NSInteger)count {
+  // Add more.
+  while (count >= (NSInteger)[array_ count]) {
+    [array_ addObject:[[OmniboxPopupCell alloc] init]];

[groby-ooo-7-16, 2015/04/24 20:11:16]

This leaks. (addObject retains, alloc retains)

[dschuyler, 2015/04/25 01:05:43]

Is autorelease the right thing to do here?

Done.

[groby-ooo-7-16, 2015/04/25 01:31:11]

Yes, it is :) 

Normal pattern for classes is to have a class method that returns an
autoreleased object.

[dschuyler, 2015/05/01 21:50:53]

Acknowledged.

+  }
+  // Remove excess.
+  while (count < (NSInteger)[array_ count]) {

[groby-ooo-7-16, 2015/04/24 20:11:16]

Why the cast?

Also, why not
if (count < [array_ count]) {
  [array_ removeObjectsInRange:NSMakeRange(count,[array_ count]-1)];

[dschuyler, 2015/04/25 01:05:43]

It's a compile error to compare signed and unsigned ints.  It was signed because
the api seemed to use signed values (even where I would lean toward unsigned
values).  I've changed the parameter to unsigned and removed the casts.
I'm too new to Cocoa to know about removeObjectsInRange and NSMakeRange.  And
because that throws an out of range exception :)  (the -1 should be -count).

Done.

[groby-ooo-7-16, 2015/04/25 01:31:11]

Excellent. Also, when you cast: Please always use static_cast<> and friends, not
c-style casts
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting)

[dschuyler, 2015/05/01 21:50:53]

Acknowledged.

+    [array_ removeLastObject];
+  }
+  DCHECK_EQ(count, (NSInteger)[array_ count]);
+  // Setup defaults.

[groby-ooo-7-16, 2015/04/24 20:11:16]

That comment is rather meaningless - what defaults?

[dschuyler, 2015/04/25 01:05:42]

Done.

+  if (count)
+    [[array_ objectAtIndex:0] setState:NSOnState];
+}
+
+- (NSArray*)getCellArray {
+  return array_;
+}
+
+- (CGFloat)tableView:(NSTableView*)tableView heightOfRow:(NSInteger)row {
+  return [[array_ objectAtIndex:row] cellSize].height;
+}
+
+@end
+
+@interface OmniboxPopupTableView ()
 - (void)resetTrackingArea;
 - (void)highlightRowAt:(NSInteger)rowIndex;
 - (void)highlightRowUnder:(NSEvent*)theEvent;
 - (BOOL)selectCellForEvent:(NSEvent*)theEvent;
 @end
 
-@implementation OmniboxPopupMatrix
+@implementation OmniboxPopupTableView
 
-- (id)initWithDelegate:(OmniboxPopupMatrixDelegate*)delegate {
+- (id)initWithDelegate:(OmniboxPopupTableDelegate*)delegate {
   if ((self = [super initWithFrame:NSZeroRect])) {
     delegate_ = delegate;
-    [self setCellClass:[OmniboxPopupCell class]];
+    controller_ = [[OmniboxPopupTableController alloc] init];
+
+    NSTableColumn* column =
+        [[NSTableColumn alloc] initWithIdentifier:@"MainCell"];
+    [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 setBackgroundColor:[NSColor controlBackgroundColor]];
-    [self renewRows:0 columns:1];

[dschuyler, 2015/04/24 18:43:08]

The initial column is now added in a different function.  So we still add the
column, but for a TableView it needs to be done a different way.

     [self setAllowsEmptySelection:YES];
-    [self setMode:NSRadioModeMatrix];

[dschuyler, 2015/04/24 18:43:08]

There doesn't seem to be an equivalent for this concept in TableView, so just
removing it seems the right thing to do.

-    [self deselectAllCells];
+    [self deselectAll:self];
+    [self setSelectionHighlightStyle:NSTableViewSelectionHighlightStyleNone];
 
     [self resetTrackingArea];
   }
   return self;
 }
 
-- (void)setDelegate:(OmniboxPopupMatrixDelegate*)delegate {
+- (void)setCppDelegate:(OmniboxPopupTableDelegate*)delegate {
   delegate_ = delegate;
 }
 
 - (NSInteger)highlightedRow {
-  NSArray* cells = [self cells];
+  NSArray* cells = [controller_ getCellArray];
   const NSUInteger count = [cells count];
   for(NSUInteger i = 0; i < count; ++i) {
     if ([[cells objectAtIndex:i] isHighlighted]) {
@@ -106,7 +165,7 @@ const NSInteger kMiddleButtonNumber = 2;
   const NSInteger highlightedRow = [self highlightedRow];
   if (highlightedRow != -1) {
     DCHECK(delegate_);
-    delegate_->OnMatrixRowMiddleClicked(self, highlightedRow);
+    delegate_->OnTableRowMiddleClicked(self, highlightedRow);
   }
 }
 
@@ -141,10 +200,21 @@ const NSInteger kMiddleButtonNumber = 2;
     }
 
     DCHECK(delegate_);
-    delegate_->OnMatrixRowClicked(self, selectedRow);
+    delegate_->OnTableRowClicked(self, selectedRow);
   }
 }
 
+- (void)setCellCount:(NSInteger)count {
+  return [controller_ setCellCount:count];

[dschuyler, 2015/04/24 18:43:08]

I'm torn on whether a wrapper call like this is good.  What are your thoughts?

[groby-ooo-7-16, 2015/04/24 20:11:17]

It is :) (Long-form argument at http://en.wikipedia.org/wiki/Law_of_Demeter &
https://pragprog.com/articles/tell-dont-ask)

[dschuyler, 2015/04/25 01:05:43]

Acknowledged.

+}
+
+- (OmniboxPopupCell*)cellAtRow:(NSInteger)row {
+  NSArray* array = [controller_ getCellArray];
+  if (row < 0 || row >= (NSInteger)[array count])

[dschuyler, 2015/04/24 18:43:08]

row == -1 does happen normally.  The high guard is just for defensive
programming, I haven't seen it happen.

[groby-ooo-7-16, 2015/04/24 20:11:17]

That kind of screams for an NSUInteger

[dschuyler, 2015/04/25 01:05:43]

I heard that screaming as well, though I thought I was doing the right thing by
doing what I saw other code doing (signed as an array index).

Done.

[groby-ooo-7-16, 2015/04/25 01:31:11]

Sigh. It's my old habits. We actually *shouldn't* use unsigned. Styleguide
explicitly says no. (Use a DCHECK to assert sane ranges, if you must)

+    return nil;
+  return [array objectAtIndex:row];
+}
+
 - (void)resetTrackingArea {
   if (trackingArea_.get())
     [self removeTrackingArea:trackingArea_.get()];
@@ -163,32 +233,30 @@ const NSInteger kMiddleButtonNumber = 2;
 - (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];
+  OmniboxPopupCell* highlightCell = [self cellAtRow:rowIndex];

[groby-ooo-7-16, 2015/04/24 20:11:16]

Why check that here? Why not just tell the controller to highlight a cell?

Also, why not just remember which cell the highlightcell is, as opposed to
scanning everything all the time?

[dschuyler, 2015/04/25 01:05:43]

It comes from a presumption that the prior Googler that setup the code that way
had valid reasons.  The same thoughts occurred to me, but I wanted to get this
working prior to diverging too far.

I added the 'if' partly because of the thought: why are we looping over all the
cells every time this is called - well, I can at least early out if it's not
changing.

Would this be something I should revisit after this CL?

[groby-ooo-7-16, 2015/04/25 01:31:11]

Yeah, probably. I'd ping the Googler in Question (GIQ? ;) why this is.

+  if ([highlightCell isHighlighted])
+    return;
 
-  for (NSCell* cell in [self cells]) {
-    [cell setHighlighted:(cell == highlightCell)];
+  for (OmniboxPopupCell* cell in controller_.getCellArray) {
+    if ([cell isHighlighted] || cell == highlightCell) {
+      [cell highlight:(cell == highlightCell) withFrame:_bounds inView:self];
+    }
   }
 }
 
 - (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];
-  }
+  [self highlightRowAt:[self rowAtPoint:point]];
 }
 
 // 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];
+  if (row != -1) {
     DCHECK(delegate_);
-    delegate_->OnMatrixRowSelected(self, row);
+    delegate_->OnTableRowSelected(self, row);
     return YES;
   }
   return NO;