[Mac] Strip newlines from paste.

[Mac] Strip newlines from paste.

http://crbug.com/11817
TEST=Copy an URL somewhere and break it apart with newlines at random places.  Copy/paste it into omnibox and the newlines should be gone.  Try this one:

http://dev.chro
mium.org/develo
pers/how-tos/ge
t-the-code

[John Grabowski, 2009-07-17 19:36:05]

http://codereview.chromium.org/159018/diff/1/3
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/159018/diff/1/3#newcode118
Line 118: // TODO(shess): Copied from autocomplete_edit_view_win.cc.  Could this
Probably not simply, since autocomplete_edit_view.h has no non-pure-virtual
methods.  But C&P sucks (for code, not to use this code) so please try and move
to a common location.

http://codereview.chromium.org/159018/diff/1/3#newcode120
Line 120: std::wstring GetClipboardText() {
Unit test!

http://codereview.chromium.org/159018/diff/1/3#newcode127
Line 127: clipboard->ReadText(&text);
Because a wstring is a string16 on Windows, but on the Mac a WCHAR_T is 4 bytes
wide.

http://codereview.chromium.org/159018/diff/1/3#newcode602
Line 602: const std::wstring text(GetClipboardText());
For Mac apps in general, a paste of a "nothing" string will do nothing.
However, for this code, you'll be clearing the selection and pasting "nothing".
Is that what you want?

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

http://codereview.chromium.org/159018/diff/1/5#newcode20
Line 20: - (BOOL)textShouldPaste:(NSText*)fieldEditor {
Unit test to confirm this returns YES always.

[Scott Hess - ex-Googler, 2009-07-17 22:02:03]

http://codereview.chromium.org/159018/diff/1/3
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/159018/diff/1/3#newcode118
Line 118: // TODO(shess): Copied from autocomplete_edit_view_win.cc.  Could this
On 2009/07/17 19:36:05, John Grabowski wrote:
> Probably not simply, since autocomplete_edit_view.h has no non-pure-virtual
> methods.  But C&P sucks (for code, not to use this code) so please try and
move
> to a common location.

?  Was thinking AutocompleteEditModel, perhaps as a static function.

Just to step back for a minute.  I had initially been inclined to handle this
stuff in AutocompleteTextFieldEditor directly, since it's a native clipboard and
a native field.  When I went to look at how other platforms handled it, I saw
that Windows had a function which was darn near portable, so it seemed
reasonable to assume it's doing the right thing.  AFAICT, GTK doesn't handle
this at all (though it's possible their control just doesn't allow newlines in).

This may start falling apart if we need to support other clipboards.  I don't
think our Clipboard API will let me sensibly handle drag-and-drop using my
current code.  That might argue for managing this w/in the native control.

There, that took like two minutes to write up, should have did that before
writing unit tests for the code as it's currently laid out :-).

http://codereview.chromium.org/159018/diff/1/3#newcode120
Line 120: std::wstring GetClipboardText() {
On 2009/07/17 19:36:05, John Grabowski wrote:
> Unit test!

Done.

http://codereview.chromium.org/159018/diff/1/3#newcode127
Line 127: clipboard->ReadText(&text);
On 2009/07/17 19:36:05, John Grabowski wrote:
> Because a wstring is a string16 on Windows, but on the Mac a WCHAR_T is 4
bytes
> wide.

OK, then I guess I can drop the TODO!.

http://codereview.chromium.org/159018/diff/1/3#newcode602
Line 602: const std::wstring text(GetClipboardText());
On 2009/07/17 19:36:05, John Grabowski wrote:
> For Mac apps in general, a paste of a "nothing" string will do nothing.
> However, for this code, you'll be clearing the selection and pasting
"nothing".
> Is that what you want?

I've modified the code to distinguish the nothing-available case from the
what-is-available-is-nothing case.  Presumably if you really did manage to copy
@"" to the clipboard, that's what you want pasted.

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

http://codereview.chromium.org/159018/diff/1/4#newcode15
Line 15: @interface NSObject (AutocompleteTextFieldDelegateMethods)
Q: Should these guys be @protocol rather than a category?

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

http://codereview.chromium.org/159018/diff/1/5#newcode20
Line 20: - (BOOL)textShouldPaste:(NSText*)fieldEditor {
On 2009/07/17 19:36:05, John Grabowski wrote:
> Unit test to confirm this returns YES always.

I was just thinking that I forgot to unit test these paths.  Sorry about that.

[John Grabowski, 2009-07-18 01:25:38]

LGTM once category becomes a protocol.
Great work here Scott.

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

http://codereview.chromium.org/159018/diff/1/4#newcode15
Line 15: @interface NSObject (AutocompleteTextFieldDelegateMethods)
On 2009/07/17 22:02:04, shess wrote:
> Q: Should these guys be @protocol rather than a category?

Absolutely should be a protocol.  

Categories are for adding features to existing classes.  For example, AppKit
adds drawBlah methods to an NSString in a category.  All strings will then know
how to draw themselves.  Your intent here is not to add functionality to all
NSObjects.

Protocols are for defining an interface for a class to implement, somewhat like
a pure virtual base class (remember no MI in objc).  That looks like your intent
here.

http://codereview.chromium.org/159018/diff/10/12
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/159018/diff/10/12#newcode594
Line 594: bool AutocompleteEditViewMac::GetClipboardText(Clipboard* clipboard,
As discussed verbally, sorry to mislead.  Turns out copying "" to the clipboard
empties the clipboard, so there is no need to differentiate between "" and
<nothing>.  A bool seems logically clearer but I wouldn't want to
over-complicate things.

http://codereview.chromium.org/159018/diff/10/13
File chrome/browser/autocomplete/autocomplete_edit_view_mac_unittest.mm (right):

http://codereview.chromium.org/159018/diff/10/13#newcode60
Line 60: EXPECT_FALSE(AutocompleteEditViewMac::GetClipboardText(&clipboard,
&text));
1 more test for empty clipboard?

http://codereview.chromium.org/159018/diff/10/16
File chrome/browser/cocoa/autocomplete_text_field_editor.h (right):

http://codereview.chromium.org/159018/diff/10/16#newcode18
Line 18: @interface AutocompleteTextFieldEditor : NSTextView {
Nit: naming this BlahBlahTextView would be more clear what kind of class this
is.

http://codereview.chromium.org/159018/diff/10/19
File chrome/browser/cocoa/autocomplete_text_field_unittest.mm (right):

http://codereview.chromium.org/159018/diff/10/19#newcode55
Line 55: TEST_F(AutocompleteTextFieldTest, TextShouldPaste) {
excellent

[TVL, 2009-07-19 05:01:27]

On Fri, Jul 17, 2009 at 2:39 PM, <shess@chromium.org> wrote:

>
> Reviewers: John Grabowski,
>
> Description:
> [Mac] Strip newlines from paste.
>
> http://crbug.com/11817
> TEST=Copy an URL somewhere and break it apart with newlines at random
> places.  Copy/paste it into omnibox and the newlines should be gone.
> Try this one:
>
> http://dev.chro
> mium.org/develo
> pers/how-tos/ge
> t-the-code
>

Drive by comment:  While eating the spaces in an url in nice, if I'm trying
to do a search, mapping the newlines to spaces would be better.  Very often
I end up using a address and pasting it in for pulling up a map/directions.
 Safari and Camino actually get this right because paste in url bar eats the
returns and paste in search bar turns them into spaces.  But since we have
one bar, I'm not sure how we get this "right".  :(

TVL



>
> Please review this at http://codereview.chromium.org/159018
>
> Affected files:
>  M chrome/browser/autocomplete/autocomplete_edit_view_mac.h
>  M chrome/browser/autocomplete/autocomplete_edit_view_mac.mm
>  M chrome/browser/cocoa/autocomplete_text_field.h
>  M chrome/browser/cocoa/autocomplete_text_field.mm
>  M chrome/browser/cocoa/autocomplete_text_field_editor.h
>  M chrome/browser/cocoa/autocomplete_text_field_editor.mm
>
>
>

[Scott Hess - ex-Googler, 2009-07-20 21:27:03]

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

http://codereview.chromium.org/159018/diff/1/4#newcode15
Line 15: @interface NSObject (AutocompleteTextFieldDelegateMethods)
On 2009/07/18 01:25:38, John Grabowski wrote:
> On 2009/07/17 22:02:04, shess wrote:
> > Q: Should these guys be @protocol rather than a category?
> 
> Absolutely should be a protocol.  

Done.

> Categories are for adding features to existing classes.  For example, AppKit
> adds drawBlah methods to an NSString in a category.  All strings will then
know
> how to draw themselves.  Your intent here is not to add functionality to all
> NSObjects.
> 
> Protocols are for defining an interface for a class to implement, somewhat
like
> a pure virtual base class (remember no MI in objc).  That looks like your
intent
> here.

Kinda.  Except that I'll be checking conformance using -respondsToSelector:
rather than -conformsToProtocol:.  This document seems to suggest categories for
informal protocols:

http://www.devworld.apple.com/documentation/Cocoa/Conceptual/ObjectiveC/Artic...

I think either way accomplishes the goal of getting the selector into the
Objective-C runtime.  Doing it as a protocol may be higher memory overhead (I
don't think that @interface-only categories have any meaning to the ObjC
runtime).

http://codereview.chromium.org/159018/diff/10/12
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/159018/diff/10/12#newcode594
Line 594: bool AutocompleteEditViewMac::GetClipboardText(Clipboard* clipboard,
On 2009/07/18 01:25:38, John Grabowski wrote:
> As discussed verbally, sorry to mislead.  Turns out copying "" to the
clipboard
> empties the clipboard, so there is no need to differentiate between "" and
> <nothing>.  A bool seems logically clearer but I wouldn't want to
> over-complicate things.

OK, I went back to a signature which returns std::wstring.

http://codereview.chromium.org/159018/diff/10/13
File chrome/browser/autocomplete/autocomplete_edit_view_mac_unittest.mm (right):

http://codereview.chromium.org/159018/diff/10/13#newcode60
Line 60: EXPECT_FALSE(AutocompleteEditViewMac::GetClipboardText(&clipboard,
&text));
On 2009/07/18 01:25:38, John Grabowski wrote:
> 1 more test for empty clipboard?

AFAICT, there is no way to test that.  If I don't write something to the
Clipboard, I'll see whatever was on the generalPasteboard when I started.

UPDATE: I found a way.  Done.

http://codereview.chromium.org/159018/diff/10/16
File chrome/browser/cocoa/autocomplete_text_field_editor.h (right):

http://codereview.chromium.org/159018/diff/10/16#newcode18
Line 18: @interface AutocompleteTextFieldEditor : NSTextView {
On 2009/07/18 01:25:38, John Grabowski wrote:
> Nit: naming this BlahBlahTextView would be more clear what kind of class this
> is.

You sure?  It's intended to only ever be used as the AutocompleteTextField's
field editor.  It should never be used as a distinct text view.

[Nico, 2009-07-21 17:17:31]

> Drive by comment:  While eating the spaces in an url in nice, if I'm trying
> to do a search, mapping the newlines to spaces would be better.  Very often
> I end up using a address and pasting it in for pulling up a map/directions.
>  Safari and Camino actually get this right because paste in url bar eats the
> returns and paste in search bar turns them into spaces.  But since we have
> one bar, I'm not sure how we get this "right".  :(

Remove newlines if the thing that's being pasted looks like an URL, replace with
spaces else?