Objective-C Readability CL for bauerb.

chrome/browser/ui/cocoa/table_model_array_controller.mm

 // Copyright (c) 2010 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/table_model_array_controller.h"
 
 #include "app/table_model.h"
 #include "base/logging.h"
 #include "base/sys_string_conversions.h"
 #include "chrome/browser/remove_rows_table_model.h"
 
 @interface TableModelArrayController (PrivateMethods)

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

no need anymore to have these as (PrivateMethods). Just () will suffice.

[Bernhard Bauer, 2011/01/08 15:41:02]

Ah! Done.

 
 - (NSUInteger)offsetForGroupID:(int)groupID;
 - (NSUInteger)offsetForGroupID:(int)groupID startingOffset:(NSUInteger)offset;
 - (NSIndexSet*)controllerRowsForModelRowsInRange:(NSRange)range;
 - (void)setModelRows:(RemoveRowsTableModel::Rows*)modelRows
     fromControllerRows:(NSIndexSet*)rows;
 - (void)modelDidChange;
 - (void)modelDidAddItemsInRange:(NSRange)range;
 - (void)modelDidRemoveItemsInRange:(NSRange)range;
 - (NSDictionary*)columnValuesForRow:(NSInteger)row;
@@ skipping 62 matching lines @@
       NSDictionary* group = [NSDictionary dictionaryWithObjectsAndKeys:
           base::SysWideToNSString(it->title), groupTitle_.get(),
           [NSNumber numberWithBool:YES], kIsGroupRow,
           nil];
       [self addObject:group];
     }
   }
   [self modelDidAddItemsInRange:NSMakeRange(0, model_->RowCount())];
 }
 
 - (NSUInteger)offsetForGroupID:(int)groupID startingOffset:(NSUInteger)offset {

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

any good reason to make this 1 based as opposed to 0 based for offset?

[Bernhard Bauer, 2011/01/08 15:41:02]

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

   const TableModel::Groups& groups = model_->GetGroups();
   DCHECK_GT(offset, 0u);
   for (NSUInteger i = offset - 1; i < groups.size(); ++i) {
     if (groups[i].id == groupID)
       return i + 1;
   }
   NOTREACHED();
   return NSNotFound;
 }
 
@@ skipping 37 matching lines @@
   NSUInteger offset = 1;
   for (NSUInteger i = range.location; i < NSMaxRange(range); ++i) {
     int group = model_->GetGroupID(i);
     offset = [self offsetForGroupID:group startingOffset:offset];
     [indexes addIndex:i + offset];
   }
   return indexes;
 }
 
 - (void)modelDidAddItemsInRange:(NSRange)range {
   NSMutableArray* rows = [NSMutableArray arrayWithCapacity:range.length];

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

check to see if range.length is 0, and just return?

[Bernhard Bauer, 2011/01/08 15:41:02]

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.

   for (NSUInteger i=range.location; i<NSMaxRange(range); ++i)

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

spaces around < and =

[Bernhard Bauer, 2011/01/08 15:41:02]

Done.

     [rows addObject:[self columnValuesForRow:i]];
   [self insertObjects:rows
       atArrangedObjectIndexes:[self controllerRowsForModelRowsInRange:range]];

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

you could probably break this line into two lines and get easier debugging and
better readability.

[Bernhard Bauer, 2011/01/08 15:41:02]

Done.

 }
 
 - (void)modelDidRemoveItemsInRange:(NSRange)range {
   NSMutableIndexSet* indexes =
       [NSMutableIndexSet indexSetWithIndexesInRange:range];
   if (model_->HasGroups()) {
     // When this method is called, the model has already removed items, so
     // accessing items in the model from |range.location| on may not be possible
     // anymore. Therefore we use the item right before that, if it exists.
     NSUInteger offset = 0;
     if (range.location > 0) {

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

Should you check range for a length of 0? in the case of a 0 length, location
may be undefined (or worse is NSNotFound).

[Bernhard Bauer, 2011/01/08 15:41:02]

Done.

       int last_group = model_->GetGroupID(range.location - 1);
       offset = [self offsetForGroupID:last_group];
     }
     [indexes shiftIndexesStartingAtIndex:0 by:offset];
     for (NSUInteger row = range.location + offset;
          row < NSMaxRange(range) + offset;
          ++row) {
       if ([self tableView:nil isGroupRow:row]) {
         // Skip over group rows.
         [indexes shiftIndexesStartingAtIndex:row by:1];
@@ skipping 27 matching lines @@
   [self setModelRows:&rows fromControllerRows:[self selectionIndexes]];
   return model_->CanRemoveRows(rows);
 }
 
 - (IBAction)remove:(id)sender {
   RemoveRowsTableModel::Rows rows;
   [self setModelRows:&rows fromControllerRows:[self selectionIndexes]];
   model_->RemoveRows(rows);
 }
 
 // Table View Delegate --------------------------------------------------------

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

instead of these style breaks, perhaps "#pragma mark"s?

[Bernhard Bauer, 2011/01/08 15:41:02]

Done.

 
 - (BOOL)tableView:(NSTableView*)tv isGroupRow:(NSInteger)row {

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

usually not keen on abbreviations like "tv"

[Bernhard Bauer, 2011/01/08 15:41:02]

Done.

   NSDictionary* values = [[self arrangedObjects] objectAtIndex:row];
   return [[values objectForKey:kIsGroupRow] boolValue];
 }
 
 - (NSIndexSet*)tableView:(NSTableView*)tableView
     selectionIndexesForProposedSelection:(NSIndexSet*)proposedIndexes {
   NSMutableIndexSet* indexes = [proposedIndexes mutableCopy];
   for (NSUInteger i = [proposedIndexes firstIndex];
        i != NSNotFound;
        i = [proposedIndexes indexGreaterThanIndex:i]) {
     if ([self tableView:tableView isGroupRow:i]) {
       [indexes removeIndex:i];
       NSUInteger row = i + 1;
       while (row < [[self arrangedObjects] count] &&
              ![self tableView:tableView isGroupRow:row])
         [indexes addIndex:row++];
     }
   }
   return indexes;
 }
 
 // Actions --------------------------------------------------------------------
 
 - (IBAction)removeAll:(id)sender {
   model_->RemoveAll();
 }
 
 @end
+