GNOME Bugzilla – Bug 687425
'Enter' should activate legacy status icons
Last modified: 2013-01-18 19:54:03 UTC
- Hit Ctrl+M to open the message tray - Select an IM chat and hit 'Enter' - The notification is activated and you can chat, all good - Start Tomboy - Hit Ctrl+M to open the message tray - Select the tomboy status icon and hit 'Enter' - Nothing happens We should stay coherent and activate the status icon like if I'd have clicked on it.
Created attachment 227903 [details] [review] MessageTray: pass keyboard events to tray icons Synthetize XKeyEvents for clicks emulated by StButton using Return or Space.
Review of attachment 227903 [details] [review]: ::: js/ui/keyboard.js @@ +587,3 @@ + handleSummaryClick: function(button) { + if (button != St.ButtonMask.ONE) This seems unrelated. Why do we care about the keyboard status icon here? ::: js/ui/notificationDaemon.js @@ +576,3 @@ // clicks are always forwarded, as the right click menu is not useful for // tray icons + if (button == St.Button.ONE && St.Button.ONE doesn't exist. ::: src/shell-tray-icon.c @@ -189,3 @@ Window xwindow, xrootwindow; - g_return_if_fail (clutter_event_type (event) == CLUTTER_BUTTON_RELEASE); I'd prefer an || here that describes the events allowed. @@ +242,3 @@ + xkevent.time = xcevent.time; + xkevent.x = xcevent.x; + xkevent.y = xcevent.y; A lot of these properties are the same. Split them out into a shared section, please.
(In reply to comment #2) > Review of attachment 227903 [details] [review]: > > ::: js/ui/keyboard.js > @@ +587,3 @@ > > + handleSummaryClick: function(button) { > + if (button != St.ButtonMask.ONE) > > This seems unrelated. Why do we care about the keyboard status icon here? Because I changed the API of handleSummaryClick (and because keyboard navigation makes sense there too) > @@ +242,3 @@ > + xkevent.time = xcevent.time; > + xkevent.x = xcevent.x; > + xkevent.y = xcevent.y; > > A lot of these properties are the same. Split them out into a shared section, > please. They're not the same, unless you consider reasonable to assume that XKeyEvent and XButtonEvent share more than the common XEvent structure (which is true in practice)
*** Bug 687077 has been marked as a duplicate of this bug. ***
Created attachment 233021 [details] [review] MessageTray: pass keyboard events to tray icons Synthetize XKeyEvents for clicks emulated by StButton using Return or Space.
Review of attachment 233021 [details] [review]: ::: js/ui/keyboard.js @@ +639,3 @@ + handleSummaryClick: function(button) { + if (button != St.ButtonMask.ONE) Make this into a separate commit -- this is changing behaviour unrelated to this bug. @@ -640,3 @@ - handleSummaryClick: function() { - let event = Clutter.get_current_event(); - if (event.type() != Clutter.EventType.BUTTON_RELEASE) Are we intentionally dropping checking for release here? ::: js/ui/notificationDaemon.js @@ +576,3 @@ // clicks are always forwarded, as the right click menu is not useful for // tray icons + if (button == St.ButtonMask.ONE && St.ButtonMask is wrong. button == 1 is correct. ::: src/shell-tray-icon.c @@ +236,3 @@ + else + { + XKeyEvent *xkevent = (XKeyEvent*) &xbevent; Yeah, I missed that these were different before, so even though the code is long-winded, I like it more.
(In reply to comment #6) > Review of attachment 233021 [details] [review]: > > ::: js/ui/keyboard.js > @@ +639,3 @@ > > + handleSummaryClick: function(button) { > + if (button != St.ButtonMask.ONE) > > Make this into a separate commit -- this is changing behaviour unrelated to > this bug. Indeed it is changing behavior. I should not do this. > @@ -640,3 @@ > - handleSummaryClick: function() { > - let event = Clutter.get_current_event(); > - if (event.type() != Clutter.EventType.BUTTON_RELEASE) > > Are we intentionally dropping checking for release here? Yes, because sometimes the current event will be a key release. It is fine, however, because StButton only emits clicked on release.
Created attachment 233100 [details] [review] MessageTray: pass keyboard events to tray icons Synthetize XKeyEvents for clicks emulated by StButton using Return or Space.
Review of attachment 233100 [details] [review]: ACN after one fix, otherwise looks good. ::: src/shell-tray-icon.c @@ -189,3 @@ Window xwindow, xrootwindow; - g_return_if_fail (clutter_event_type (event) == CLUTTER_BUTTON_RELEASE); I'd prefer to modify this so it's: ClutterEventType event_type = clutter_event_type (event); g_return_if_fail (event_type == CLUTTER_BUTTON_RELEASE || event_type == CLUTTER_KEY_RELEASE);
Attachment 233100 [details] pushed as 59ecd61 - MessageTray: pass keyboard events to tray icons