|
|
Created:
10 years, 4 months ago by Robert Sesek Modified:
9 years, 7 months ago CC:
chromium-reviews, John Grabowski, pam+watch_chromium.org, ben+cc_chromium.org Base URL:
http://src.chromium.org/git/chromium.git Visibility:
Public. |
Description[Mac] Implement highlight-on-hover for the Wrench menu buttons.
R=pinkerton, shess, mark
BUG=51643
TEST=Open Wrench menu in sticky and non-sticky mode. Hover over buttons and they highlight.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56430
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix conversion method #
Total comments: 2
Patch Set 3 : Disable the timer if on 10.5 #
Total comments: 3
Patch Set 4 : Addressed all comments #
Messages
Total messages: 15 (0 generated)
http://codereview.chromium.org/3137013/diff/1/3 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/1/3#newcode56 chrome/browser/cocoa/menu_tracked_button.mm:56: afterDelay:0.05 // This is the smallest delay that works. do we really need to be checking 20 times a second to see if the mouse is still in the button? That seems really wasteful. Can't we do 0.5s ? If not, then your comment is backwards, no? It's the "largest" delay that works, because nothing larger will work. http://codereview.chromium.org/3137013/diff/1/3#newcode76 chrome/browser/cocoa/menu_tracked_button.mm:76: // she is merely hovering here. If it is not, then disable the highlight. If s/she/they ? we are usually gender-neutral in comments. http://codereview.chromium.org/3137013/diff/1/3#newcode81 chrome/browser/cocoa/menu_tracked_button.mm:81: point = [[self window] convertScreenToBase:point]; I don't think you want convertScreenToBase. Check with Avi. I think you want to [convertPoint:point fromView:nil]
http://codereview.chromium.org/3137013/diff/1/3 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/1/3#newcode56 chrome/browser/cocoa/menu_tracked_button.mm:56: afterDelay:0.05 // This is the smallest delay that works. On 2010/08/17 17:41:01, pink wrote: > do we really need to be checking 20 times a second to see if the mouse is still > in the button? That seems really wasteful. Can't we do 0.5s ? If not, then your > comment is backwards, no? It's the "largest" delay that works, because nothing > larger will work. It's absolutely necessary to check this frequently. The delay is how long the button could be stuck in hover state if the user mouses out. 1/20th of a second is the mean time for a user to visually perceive change according to Wikipedia. The delay is noticeable, but barely. If it was changed to 1/2 a second, the delay would be jarringly noticeable. I'll fix the comment. http://codereview.chromium.org/3137013/diff/1/3#newcode76 chrome/browser/cocoa/menu_tracked_button.mm:76: // she is merely hovering here. If it is not, then disable the highlight. If On 2010/08/17 17:41:01, pink wrote: > s/she/they ? we are usually gender-neutral in comments. 'They' is plural and would break subject-verb agreement. 'It' implies non-human. Using she is grammatically and politically acceptable. I could also reword the comment to repeat the dereferenced subject. http://codereview.chromium.org/3137013/diff/1/3#newcode81 chrome/browser/cocoa/menu_tracked_button.mm:81: point = [[self window] convertScreenToBase:point]; On 2010/08/17 17:41:01, pink wrote: > I don't think you want convertScreenToBase. Check with Avi. I think you want to > [convertPoint:point fromView:nil] I was under the impression that the two were equivalent when |fromView| is nil.
http://codereview.chromium.org/3137013/diff/1/3 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/1/3#newcode81 chrome/browser/cocoa/menu_tracked_button.mm:81: point = [[self window] convertScreenToBase:point]; we've had a lot of bugs crop up from this, i think avi figured out the difference at one point, he should be able to explain it.
drive-by http://codereview.chromium.org/3137013/diff/1/3 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/1/3#newcode82 chrome/browser/cocoa/menu_tracked_button.mm:82: point = [self convertPointFromBase:point]; I find pink's comment mis-leading. I do think you need -[NSWindow convertScreenToBase:] to get the point from screen coordinates to the window's coordinates, but then you want -[NSView convertPoint:fromView:nil] to get it to your target view's coordinates. I think pink was refering to this thread: https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...
No, the event's mouse location is already in the window's coordinate system, hence the need for only one transform. You do not, and should not, be using the base coordinate transforms here. On Tue, Aug 17, 2010 at 3:55 PM, <shess@chromium.org> wrote: > drive-by > > > http://codereview.chromium.org/3137013/diff/1/3 > File chrome/browser/cocoa/menu_tracked_button.mm (right): > > http://codereview.chromium.org/3137013/diff/1/3#newcode82 > chrome/browser/cocoa/menu_tracked_button.mm:82: point = [self > convertPointFromBase:point]; > I find pink's comment mis-leading. I do think you need -[NSWindow > convertScreenToBase:] to get the point from screen coordinates to the > window's coordinates, but then you want -[NSView > convertPoint:fromView:nil] to get it to your target view's coordinates. > > I think pink was refering to this thread: > https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr... > > http://codereview.chromium.org/3137013/show > -- Mike Pinkerton Mac Weenie pinkerton@google.com
On Tue, Aug 17, 2010 at 3:56 PM, Mike Pinkerton <pinkerton@chromium.org>wrote: > No, the event's mouse location is already in the window's coordinate > system, hence the need for only one transform. You do not, and should > not, be using the base coordinate transforms here. +[NSEvent mouseLocation] returns in screen's coordinates. But the -[NSEvent locationInWindow] is in window's base. http://codereview.chromium.org/3137013/show
per irc, we were all correct in one way or another :-) On Tue, Aug 17, 2010 at 3:59 PM, Robert Sesek <rsesek@chromium.org> wrote: > > On Tue, Aug 17, 2010 at 3:56 PM, Mike Pinkerton <pinkerton@chromium.org> > wrote: >> >> No, the event's mouse location is already in the window's coordinate >> system, hence the need for only one transform. You do not, and should >> not, be using the base coordinate transforms here. > > +[NSEvent mouseLocation] returns in screen's coordinates. But the -[NSEvent > locationInWindow] is in window's base. > http://codereview.chromium.org/3137013/show -- Mike Pinkerton Mac Weenie pinkerton@google.com
Updated to keep the conversion from screen to base, but use convertPoint:fromView: to get to the view's coordinate system.
http://codereview.chromium.org/3137013/diff/11001/5003 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/11001/5003#newcode79 chrome/browser/cocoa/menu_tracked_button.mm:79: if (floor(NSAppKitVersionNumber) <= NSAppKitVersionNumber10_5) { Is this the right place to bottleneck? That means that we run the timer on 10.5 even though we don't use it at all.
http://codereview.chromium.org/3137013/diff/11001/5003 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/11001/5003#newcode79 chrome/browser/cocoa/menu_tracked_button.mm:79: if (floor(NSAppKitVersionNumber) <= NSAppKitVersionNumber10_5) { On 2010/08/17 20:41:04, pink wrote: > Is this the right place to bottleneck? That means that we run the timer on 10.5 > even though we don't use it at all. Good call. Fixed.
lgtm http://codereview.chromium.org/3137013/diff/15001/16002 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/15001/16002#newcode14 chrome/browser/cocoa/menu_tracked_button.mm:14: // Apple does not define NSAppKitVersionNumber10_5 when using the 10.5 SDK. The Worth burying this in shouldHighlightOnHover since nothing else needs it?
http://codereview.chromium.org/3137013/diff/15001/16002 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/15001/16002#newcode63 chrome/browser/cocoa/menu_tracked_button.mm:63: afterDelay:0.05 // This is the largest delay that works. Isn’t this the shortest delay that works? You should say what stops working when the delay is shorter. http://codereview.chromium.org/3137013/diff/15001/16002#newcode107 chrome/browser/cocoa/menu_tracked_button.mm:107: return floor(NSAppKitVersionNumber) > NSAppKitVersionNumber10_5; #include <math.h> to use floor. Better yet, #include <cmath> and use std::floor. std::floor is overloaded based on argument type and always does the right thing.
Addressed all comments.
LG allays. |