Objective-C Readability CL for bauerb.

Objective-C Readability CL for bauerb.

TableModelArrayController is a subclass of NSArrayController that adapts a TableModel (including groups) for use with Cocoa bindings.

Original CL: http://codereview.chromium.org/3327016

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71056

[dmac, 2011-01-06 06:04:05]

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
File chrome/browser/ui/cocoa/table_model_array_controller.h (right):

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.h:21: // NSArrayController,
to be used with bindings.
grammar nit: this first sentence needs some work

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
File chrome/browser/ui/cocoa/table_model_array_controller.mm (right):

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:12: @interface
TableModelArrayController (PrivateMethods)
no need anymore to have these as (PrivateMethods). Just () will suffice.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:95: -
(NSUInteger)offsetForGroupID:(int)groupID startingOffset:(NSUInteger)offset {
any good reason to make this 1 based as opposed to 0 based for offset?

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:153: NSMutableArray*
rows = [NSMutableArray arrayWithCapacity:range.length];
check to see if range.length is 0, and just return?

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:154: for (NSUInteger
i=range.location; i<NSMaxRange(range); ++i)
spaces around < and =

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:157:
atArrangedObjectIndexes:[self controllerRowsForModelRowsInRange:range]];
you could probably break this line into two lines and get easier debugging and
better readability.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:168: if (range.location
> 0) {
Should you check range for a length of 0? in the case of a 0 length, location
may be undefined (or worse is NSNotFound).

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:216: // Table View
Delegate --------------------------------------------------------
instead of these style breaks, perhaps "#pragma mark"s?

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:218: -
(BOOL)tableView:(NSTableView*)tv isGroupRow:(NSInteger)row {
usually not keen on abbreviations like "tv"

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
File chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm (right):

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm:1: // Copyright
(c) 2010 The Chromium Authors. All rights reserved.
any idea what amount of coverage you get with this?

[Bernhard Bauer, 2011-01-08 15:41:01]

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
File chrome/browser/ui/cocoa/table_model_array_controller.h (right):

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.h:21: // NSArrayController,
to be used with bindings.
On 2011/01/06 06:04:05, dmac wrote:
> grammar nit: this first sentence needs some work

Ok, I hope it sounds a bit less awkward now.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
File chrome/browser/ui/cocoa/table_model_array_controller.mm (right):

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:12: @interface
TableModelArrayController (PrivateMethods)
On 2011/01/06 06:04:05, dmac wrote:
> no need anymore to have these as (PrivateMethods). Just () will suffice.

Ah! Done.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:95: -
(NSUInteger)offsetForGroupID:(int)groupID startingOffset:(NSUInteger)offset {
On 2011/01/06 06:04:05, dmac wrote:
> any good reason to make this 1 based as opposed to 0 based for offset?

If the TableModel has groups, we add one additional row for each group as a
header, so the first group has offset one.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:153: NSMutableArray*
rows = [NSMutableArray arrayWithCapacity:range.length];
On 2011/01/06 06:04:05, dmac wrote:
> check to see if range.length is 0, and just return?

OK, done. I don't think anyone actually calls this method with a range of
{NSNotfound, 0}, but the contract doesn't really say it's not allowed.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:154: for (NSUInteger
i=range.location; i<NSMaxRange(range); ++i)
On 2011/01/06 06:04:05, dmac wrote:
> spaces around < and =

Done.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:157:
atArrangedObjectIndexes:[self controllerRowsForModelRowsInRange:range]];
On 2011/01/06 06:04:05, dmac wrote:
> you could probably break this line into two lines and get easier debugging and
> better readability.

Done.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:168: if (range.location
> 0) {
On 2011/01/06 06:04:05, dmac wrote:
> Should you check range for a length of 0? in the case of a 0 length, location
> may be undefined (or worse is NSNotFound).

Done.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:216: // Table View
Delegate --------------------------------------------------------
On 2011/01/06 06:04:05, dmac wrote:
> instead of these style breaks, perhaps "#pragma mark"s?

Done.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller.mm:218: -
(BOOL)tableView:(NSTableView*)tv isGroupRow:(NSInteger)row {
On 2011/01/06 06:04:05, dmac wrote:
> usually not keen on abbreviations like "tv"

Done.

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
File chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm (right):

http://codereview.chromium.org/5831001/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm:1: // Copyright
(c) 2010 The Chromium Authors. All rights reserved.
On 2011/01/06 06:04:05, dmac wrote:
> any idea what amount of coverage you get with this?

The complicated parts are translating the row indices from Cocoa to TableModel
and back, and incrementally updating the internal array of arranged objects, and
these are definitely getting exercised. I did actually find a bunch of bugs with
these tests.

[dmac, 2011-01-11 18:18:20]

LGTM, sorry about the delay getting back to you. Please follow through with the
rest of the instructions about getting readability and it should be granted
shortly. Congrats.