Mac: give visual feedback for bookmark button drags.

Mac: give visual feedback for bookmark button drags.

Draw a "ugly black bar" (where "black" is actually a theme colour) to indicate
where a bookmark button that's being dragged will end up if dropped. This is a
stopgap measure for the beta. Later, we will make the buttons move around
instead (even if implementable in the time remaining, such animations would be a
crash risk).

BUG=17608
TEST=Drag bookmark bar buttons around. Make sure that the bar indicating where a button will be dropped is visible enough and accurate. Repeat with various themes.

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

[viettrungluu, 2009-11-25 02:11:32]

As the description says, this is a stopgap measure for the beta.

[mrossetti, 2009-11-25 04:07:31]

http://codereview.chromium.org/437051/diff/3001/4004
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/437051/diff/3001/4004#newcode133
chrome/browser/cocoa/bookmark_bar_view.mm:133: if (dropIndicatorShown_) {
I think you want to use draggingEnded:.  draggingExited: is called whenever the
drag leaves the view (which we'll certainly use when doing the proper animation)
but not if the drag concludes within the view.  draggingEnded: is always called
once the drag concludes.

http://codereview.chromium.org/437051/diff/3001/4004#newcode181
chrome/browser/cocoa/bookmark_bar_view.mm:181: dropIndicatorShown_ = NO;
You probably won't need this if you use draggingEnded: as noted above.

[viettrungluu, 2009-11-25 04:24:23]

Thanks, Mike, for taking a look.

http://codereview.chromium.org/437051/diff/3001/4004
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/437051/diff/3001/4004#newcode133
chrome/browser/cocoa/bookmark_bar_view.mm:133: if (dropIndicatorShown_) {
On 2009/11/25 04:07:31, mrossetti wrote:
> I think you want to use draggingEnded:.  draggingExited: is called whenever
the
> drag leaves the view (which we'll certainly use when doing the proper
animation)
> but not if the drag concludes within the view.  draggingEnded: is always
called
> once the drag concludes.

I don't think so. The problem with using -draggingEnded: is that then the
indicator bar remains displayed even when dropping wouldn't move the button to
that point. Probably for safety I should also hook -draggingEnded: instead of
putting the code in -performDragOperation:.

[mrossetti_google.com, 2009-11-25 04:40:43]

If you're sure, then LGTM.  But in my tests, draggingEnded: was always
called when a drag concluded regardless of where the mouse was at that
point, and draggingExited: was called only whan the drag left the view's
area (and as many times as the same drag left the view's area).

Mike

On Tue, Nov 24, 2009 at 9:24 PM, <viettrungluu@chromium.org> wrote:

> Thanks, Mike, for taking a look.
>
>
>
> http://codereview.chromium.org/437051/diff/3001/4004
> File chrome/browser/cocoa/bookmark_bar_view.mm (right):
>
> http://codereview.chromium.org/437051/diff/3001/4004#newcode133
> chrome/browser/cocoa/bookmark_bar_view.mm:133: if (dropIndicatorShown_)
> {
> On 2009/11/25 04:07:31, mrossetti wrote:
>
>> I think you want to use draggingEnded:.  draggingExited: is called
>>
> whenever the
>
>> drag leaves the view (which we'll certainly use when doing the proper
>>
> animation)
>
>> but not if the drag concludes within the view.  draggingEnded: is
>>
> always called
>
>> once the drag concludes.
>>
>
> I don't think so. The problem with using -draggingEnded: is that then
> the indicator bar remains displayed even when dropping wouldn't move the
> button to that point. Probably for safety I should also hook
> -draggingEnded: instead of putting the code in -performDragOperation:.
>
>
> http://codereview.chromium.org/437051
>

[viettrungluu, 2009-11-25 04:45:35]

I guess the point is that we want to hide the indicator bar whenever the 
drag leaves the view's area (since it then no longer indicates the drop 
destination), so we need to hook -draggingExited:. -draggingEnded: is 
only called when the drag actually ends (basically when the mouse button 
is released).

Thanks again,

- Trung

Mike Rossetti wrote:
> If you're sure, then LGTM.  But in my tests, draggingEnded: was always 
> called when a drag concluded regardless of where the mouse was at that 
> point, and draggingExited: was called only whan the drag left the view's 
> area (and as many times as the same drag left the view's area).
> 
> Mike
> 
> On Tue, Nov 24, 2009 at 9:24 PM, <viettrungluu@chromium.org 
> <mailto:viettrungluu@chromium.org>> wrote:
> 
>     Thanks, Mike, for taking a look.
> 
> 
> 
>     http://codereview.chromium.org/437051/diff/3001/4004
>     File chrome/browser/cocoa/bookmark_bar_view.mm
>     <http://bookmark_bar_view.mm> (right):
> 
>     http://codereview.chromium.org/437051/diff/3001/4004#newcode133
>     chrome/browser/cocoa/bookmark_bar_view.mm:133
>     <http://bookmark_bar_view.mm:133>: if (dropIndicatorShown_)
>     {
>     On 2009/11/25 04:07:31, mrossetti wrote:
> 
>         I think you want to use draggingEnded:.  draggingExited: is called
> 
>     whenever the
> 
>         drag leaves the view (which we'll certainly use when doing the
>         proper
> 
>     animation)
> 
>         but not if the drag concludes within the view.  draggingEnded: is
> 
>     always called
> 
>         once the drag concludes.
> 
> 
>     I don't think so. The problem with using -draggingEnded: is that then
>     the indicator bar remains displayed even when dropping wouldn't move the
>     button to that point. Probably for safety I should also hook
>     -draggingEnded: instead of putting the code in -performDragOperation:.
> 
> 
>     http://codereview.chromium.org/437051
> 
> 

[pink (ping after 24hrs), 2009-11-25 16:26:44]

LGTM with some comments.

http://codereview.chromium.org/437051/diff/5001/6001
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/437051/diff/5001/6001#newcode179
chrome/browser/cocoa/bookmark_bar_controller.h:179: toPoint:(NSPoint)point;
in the comment, specify the coordinate system for |point| (i believe it's the
window's coordinate system, given how you're passing in from the drag info).

http://codereview.chromium.org/437051/diff/5001/6002
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/437051/diff/5001/6002#newcode544
chrome/browser/cocoa/bookmark_bar_controller.mm:544: if (destIndex >= 0) {
check sourceNode here as well (dchecks do nothing in release).

http://codereview.chromium.org/437051/diff/5001/6004
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/437051/diff/5001/6004#newcode85
chrome/browser/cocoa/bookmark_bar_view.mm:85: // Don't let the indicator be
half-off the view.
i don't understand what this means.

http://codereview.chromium.org/437051/diff/5001/6004#newcode88
chrome/browser/cocoa/bookmark_bar_view.mm:88: NSRect uglyBlackBar =
tell me how you really feel :-)

[viettrungluu, 2009-11-25 16:52:19]

Thanks, Pink.

http://codereview.chromium.org/437051/diff/5001/6001
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/437051/diff/5001/6001#newcode179
chrome/browser/cocoa/bookmark_bar_controller.h:179: toPoint:(NSPoint)point;
On 2009/11/25 16:26:44, pink wrote:
> in the comment, specify the coordinate system for |point| (i believe it's the
> window's coordinate system, given how you're passing in from the drag info).

Done.

http://codereview.chromium.org/437051/diff/5001/6002
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/437051/diff/5001/6002#newcode544
chrome/browser/cocoa/bookmark_bar_controller.mm:544: if (destIndex >= 0) {
On 2009/11/25 16:26:44, pink wrote:
> check sourceNode here as well (dchecks do nothing in release).

Done. Also added the corresponding paranoia check to -indexForDragOfButton:....

http://codereview.chromium.org/437051/diff/5001/6004
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/437051/diff/5001/6004#newcode85
chrome/browser/cocoa/bookmark_bar_view.mm:85: // Don't let the indicator be
half-off the view.
On 2009/11/25 16:26:44, pink wrote:
> i don't understand what this means.

Well, it is a bit cryptic. It just ensures that the indicator won't be clipped
(due to being partly out of the visible area). Comment changed.

http://codereview.chromium.org/437051/diff/5001/6004#newcode88
chrome/browser/cocoa/bookmark_bar_view.mm:88: NSRect uglyBlackBar =
On 2009/11/25 16:26:44, pink wrote:
> tell me how you really feel :-)

Actually, I think those were the words Ben used to describe this stopgap. But
for added value, I put in "uglyBlackBarColor". :-)