479881 - Double-clicking on location/search field splitter should collapse/uncollapse the search field

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Ilya Sherman]

When the search field is hidden, double clicking on the splitter should restore it to its original size.  When the search field is visible, double clicking the splitter should collapse it.

Comment 1

[Chris Lawson (gone)]

(In reply to comment #0)
> When the search field is hidden, double clicking on the splitter should restore
> it to its original size.  When the search field is visible, double clicking the
> splitter should collapse it.

To elaborate a bit, I don't think double-clicking to collapse is a behaviour that always makes intuitive sense on *SplitViews, but I think double-clicking the splitter to collapse the search field makes sense because the search field is almost always secondary to the location field in a Web browser. I'd be OK with ignoring that half of this bug, though (and I was the one who suggested that part on IRC), if other people think it's not logical.

Comment 2

[Ilya Sherman]

Attachment: Patch v1.0

Hey, look, Apple wrote this for us -- how nice :)

The reference prototype for the method [1] has |NSInteger| where I have |int|.  I'm not sure where NSInteger is defined -- it seems to be only OS X 10.5+, but is a typedef to int or long, depending on the environment [2].  Is just using int fine for us; or is there something better we can do?
[1] http://developer.apple.com/mac/library/documentation/cocoa/Reference/NSSplitViewDelegate_Protocol/Reference/Reference.html#//apple_ref/occ/intfm/NSSplitViewDelegate/splitView:shouldCollapseSubview:forDoubleClickOnDividerAtIndex:
[2] http://developer.apple.com/mac/library/documentation/cocoa/Reference/Foundation/Miscellaneous/Foundation_DataTypes/Reference/reference.html#//apple_ref/doc/c_ref/NSInteger

Also, I'm like 99% sure that this has no effect on Tiger, but it would be nice to check.

Comment 3

[Chris Lawson (gone)]

Comment on attachment 419144 [details] [diff] [review]
Patch v1.0

I'm not really qualified to answer the int/NSInteger question, but I'll offer this:

http://cocoadev.com/index.pl?NSUInteger

r=me on the code and functionality, with the caveat that when I tested this using Troubleshoot Camino, I had to manually expand the search field first (I normally keep it collapsed) before double-clicking would work. Once I did that, every subsequent double-click event worked as expected. This may not have been an issue if I had tested on a different machine, or if I had tested by quitting my "main" Camino and doing it that way.

Comment 4

[Stuart Morgan]

Comment on attachment 419144 [details] [diff] [review]
Patch v1.0

>+// TODO int should be NSInteger

Remove this; it's true of basically every Apple API we use through the whole codebase, so there's no reason to call this case out specifically. Once Camino is 10.5+ (or once Camino starts using GTM, so we can snag the handy header of conditional definitions), the whole codebase will need to be scrubbed for this kind of thing to make it 64-bit clean.

sr=smorgan otherwise (although I'm a bit surprised this works, since we override a bunch of splitview stuff).

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Eiichi, can you give this patch a quick test on 10.4 to make sure nothing bad happens?  The expected outcome on 10.4 is that the splitview behaves exactly as it did before.

(From update of attachment 419144 [details] [diff] [review])
>+// NB This only works for OS X at least 10.5, but is harmless on 10.4.

Nit: since this is Mac-only code, I don't think we need "OS X" here, either; something like "This works on 10.5 and up but is harmless on 10.4." (or "10.5+"; I forget what our style is) seems more succinct.

Comment 6

[Eiichi]

(In reply to comment #5)
> Eiichi, can you give this patch a quick test on 10.4 to make sure nothing bad
> happens?  The expected outcome on 10.4 is that the splitview behaves exactly as
> it did before.

I tested the patch on 10.4, and it seemed that nothing bad happened and Camino behaved as before.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #6)
> I tested the patch on 10.4, and it seemed that nothing bad happened and Camino
> behaved as before.

Thank you!

Landed on cvs trunk with the comment fixes.