Mac: Enable OK Button when editing a bookmark text to be blank...

Enable the OK button when editing a bookmark even when there is no text, and keep alignment correct in both cases. Also strip out newlines from bookmark titles.

[Patch by feldstein.]

BUG=26353
TEST=Add and remove titles from bookmarks in the bookmark bar and verify that they look correct, and icons are centered when there is no text.

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

[viettrungluu, 2009-11-03 20:07:36]

I'll wait for you to sync and merge, but here are a few quick style comments:

http://codereview.chromium.org/353024/diff/1/2
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/353024/diff/1/2#newcode526
Line 526: BookmarkButtonCell *cell = [[[BookmarkButtonCell alloc]
initTextCell:nil]
Things which aren't your fault (since they were in the original):
- the style in this file (and in most of our files) is "BookmarkButtonCell*
cell",
- probably this would be more readable if everything after '=' were put on the
next line (and on a single line with "autorelease];"), indented two tabs (per
style guide).

http://codereview.chromium.org/353024/diff/1/2#newcode719
Line 719: NSString* title = [cell title];
This (and the following) has an extra level of indentation.

http://codereview.chromium.org/353024/diff/1/4
File chrome/browser/cocoa/bookmark_button_cell.h (right):

http://codereview.chromium.org/353024/diff/1/4#newcode16
Line 16: - (void) setBookmarkCellText:(NSString*)title andImage:(NSImage*)image;
Style: "(void)setBookmarkCellText..." (no space between ')' and method name).
Also, it might be more readable if you put "andImage:..." on the next line,
aligned along ':'.

http://codereview.chromium.org/353024/diff/1/3
File chrome/browser/cocoa/bookmark_button_cell.mm (right):

http://codereview.chromium.org/353024/diff/1/3#newcode48
Line 48: - (void) setBookmarkCellText:(NSString*)title andImage:(NSImage*)image
{
Same comments I had for the prototype.

[mrossetti, 2009-11-03 20:08:04]

Your copy of bookmark_editor_controller is seriously out of date.  Please
resolve and resubmit.

http://codereview.chromium.org/353024/diff/1/5
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/353024/diff/1/5#newcode145
Line 145: if (node_ && (node_->is_folder() || newURL.is_valid())) {
Your copy of bookmark_editor_controller is seriously out of date.  Please
resolve and resubmit.

[John Grabowski, 2009-11-03 21:35:29]

http://codereview.chromium.org/353024/diff/1/4
File chrome/browser/cocoa/bookmark_button_cell.h (right):

http://codereview.chromium.org/353024/diff/1/4#newcode16
Line 16: - (void) setBookmarkCellText:(NSString*)title andImage:(NSImage*)image;
Also doc above the method.  Example: mention the image is NSImageLeft'ed

http://codereview.chromium.org/353024/diff/1/3
File chrome/browser/cocoa/bookmark_button_cell.mm (right):

http://codereview.chromium.org/353024/diff/1/3#newcode49
Line 49: title = [title stringByTrimmingCharactersInSet:
Windows does not do this; it lets me name a bookmark "             ...."
Or even "                       "
Why do we need to enforce it here?

http://codereview.chromium.org/353024/diff/1/3#newcode51
Line 51: title = [title stringByReplacingOccurrencesOfString:@"\n" withString:@"
"];
How is it possible to end up with a newline?  All the edit dialogs have RET
mapped to OK.

http://codereview.chromium.org/353024/diff/1/3#newcode55
Line 55: [self setImagePosition:NSImageOnly];
 If no title, exactly what is the visual difference between NSImageLeft and
NSImageOnly?

[feldstein, 2009-11-04 02:44:48]

Replies to a couple of comments, new version is coming in a few minutes.

http://codereview.chromium.org/353024/diff/1/3
File chrome/browser/cocoa/bookmark_button_cell.mm (right):

http://codereview.chromium.org/353024/diff/1/3#newcode49
Line 49: title = [title stringByTrimmingCharactersInSet:
On 2009/11/03 21:35:30, John Grabowski wrote:
> Windows does not do this; it lets me name a bookmark "             ...."
> Or even "                       "
> Why do we need to enforce it here?

I guess we don't, I just removed it in the next patch version I am about to
submit.

http://codereview.chromium.org/353024/diff/1/3#newcode51
Line 51: title = [title stringByReplacingOccurrencesOfString:@"\n" withString:@"
"];
On 2009/11/03 21:35:30, John Grabowski wrote:
> How is it possible to end up with a newline?  All the edit dialogs have RET
> mapped to OK.

You can copy paste it in there.  An edge case, but it does look janky. Issue
19724 is the bug for it.

http://codereview.chromium.org/353024/diff/1/3#newcode55
Line 55: [self setImagePosition:NSImageOnly];
On 2009/11/03 21:35:30, John Grabowski wrote:
>  If no title, exactly what is the visual difference between NSImageLeft and
> NSImageOnly?

If you have NSImageLeft it leaves some padding to the right of the favicon. 
NSImageOnly gets rid of the placeholder for the text and centers it within the
hover bevel.

[pink (ping after 24hrs), 2009-11-04 14:07:51]

drive-by comments.

http://codereview.chromium.org/353024/diff/6001/12
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/353024/diff/6001/12#newcode602
Line 602: [[[BookmarkButtonCell alloc] initTextCell:nil] autorelease];
continuation lines are indented an additional 4 spaces.

http://codereview.chromium.org/353024/diff/6001/14
File chrome/browser/cocoa/bookmark_button_cell.h (right):

http://codereview.chromium.org/353024/diff/6001/14#newcode19
Line 19: // if there is text in the title, and centered (NSImageCenter) if ther
is
s/ther/there

http://codereview.chromium.org/353024/diff/6001/14#newcode22
Line 22: andImage:(NSImage*)image;
you generally don't need to include conjunctions in your method names.
Prepositions are accepted, but the conjunction is implied (and obvious). Just
call it -setBookmarkCellText:image:

http://codereview.chromium.org/353024/diff/6001/13
File chrome/browser/cocoa/bookmark_button_cell.mm (right):

http://codereview.chromium.org/353024/diff/6001/13#newcode41
Line 41: if(image){
space after |if| before paren.

[feldstein, 2009-11-04 23:24:58]

Added a new patch for review to incorporate your comments and fix a unit test.
-Mike

[John Grabowski, 2009-11-04 23:44:57]

LGTM

[viettrungluu, 2009-11-05 00:02:39]

Mostly nits, one substantial comment (at the bottom).

http://codereview.chromium.org/353024/diff/9001/9002
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/353024/diff/9001/9002#newcode597
Line 597: BookmarkButtonCell *cell =
<whine> |BookmarkButtonCell* cell| in this file.

http://codereview.chromium.org/353024/diff/9001/9005
File chrome/browser/cocoa/bookmark_button_cell.h (right):

http://codereview.chromium.org/353024/diff/9001/9005#newcode16
Line 16: // setBookmarkCellText:andImage: is used to set the text and image
Nit #1: Usually "code" should be put between |'s in comments (though it's up to
you to figure out what constitutes "code").

Nit #2: Strictly speaking, the '-' is part of the method's signature.

Also, you've changed the name of this method. So that should be (in my opinion):
|-setBookmarkCellText:image:| in this comment.

http://codereview.chromium.org/353024/diff/9001/9005#newcode18
Line 18: // if there is text in the title, and centered (NSImageCenter) if there
is
... if there is...?

http://codereview.chromium.org/353024/diff/9001/9003
File chrome/browser/cocoa/bookmark_button_cell.mm (right):

http://codereview.chromium.org/353024/diff/9001/9003#newcode38
Line 38: image:(NSImage *)image {
(NSString*) and (NSImage*) (no space)

http://codereview.chromium.org/353024/diff/9001/9003#newcode40
Line 40: withString:@" "];
There are other types of white space which should probably be eliminated. You
should consider using the functions in base/string_util.h (TrimWhitespace() and
friends, or CollapseWhitespace() and friends).

[feldstein, 2009-11-05 00:34:07]

Fixed the nitpicks, should be good to go now

http://codereview.chromium.org/353024/diff/9001/9003
File chrome/browser/cocoa/bookmark_button_cell.mm (right):

http://codereview.chromium.org/353024/diff/9001/9003#newcode40
Line 40: withString:@" "];
On 2009/11/05 00:02:39, viettrungluu wrote:
> There are other types of white space which should probably be eliminated. You
> should consider using the functions in base/string_util.h (TrimWhitespace()
and
> friends, or CollapseWhitespace() and friends).

We don't really want to trim/collapse whitespace, just line breaks.

[viettrungluu, 2009-11-05 00:36:24]

OK, lgtm.

On 2009/11/05 00:34:07, feldstein wrote:
> Fixed the nitpicks, should be good to go now
> 
> http://codereview.chromium.org/353024/diff/9001/9003
> File chrome/browser/cocoa/bookmark_button_cell.mm (right):
> 
> http://codereview.chromium.org/353024/diff/9001/9003#newcode40
> Line 40: withString:@" "];
> On 2009/11/05 00:02:39, viettrungluu wrote:
> > There are other types of white space which should probably be eliminated.
You
> > should consider using the functions in base/string_util.h (TrimWhitespace()
> and
> > friends, or CollapseWhitespace() and friends).
> 
> We don't really want to trim/collapse whitespace, just line breaks.

[mrossetti, 2009-11-05 03:26:13]

One small nit remains.

LGTM

http://codereview.chromium.org/353024/diff/10009/10013
File chrome/browser/cocoa/bookmark_button_cell.h (right):

http://codereview.chromium.org/353024/diff/10009/10013#newcode18
Line 18: // if there is text in the title, and centered (NSImageCenter) if there
is
What does that last part of the comment, "...if there is..." mean?  Terminate
comments with period.