GNOME Bugzilla – Bug 689109
popupMenu: Port to GrabHelper
Last modified: 2012-12-08 13:46:12 UTC
This was a patch that was meant to be written but was never completed because of time pressure for the 3.6 message tray. I did some light testing, so please try and break this as much as possible. Play around with the message tray too, since I changed some grab helper code.
Created attachment 229935 [details] [review] popupMenu: Port to GrabHelper
Review of attachment 229935 [details] [review]: Nope, breaks keynav.
Created attachment 230696 [details] [review] grabHelper: Use captured-event for escape ungrabs I have no idea why we used 'event' rather than 'captured-event' before. 'event' has some really strange quirks that came up when porting PopupMenu to the GrabHelper
Created attachment 230697 [details] [review] grabHelper: Treat the current grabbed actor as a grabbed actor This should be obvious, but I guess it wasn't necessary for the message tray case.
Created attachment 230698 [details] [review] grabHelper: Drop to the actor clicked on This is necessary for child popups in menus, e.g. while in a combo box, clicking outside of the user menu should drop the entire menu, but clicking on the user menu itself should only drop the combo box.
Created attachment 230699 [details] [review] grabHelper: Fix up event handling We need to return 'true' to signify that we handled the event.
Created attachment 230700 [details] [review] popupMenu: Port to GrabHelper
Created attachment 230701 [details] [review] popupMenu: Introduce PopupDummyMenu This is designed for things like the activities button that sort of need a menu to make navigation work, but not really have it do anything.
Review of attachment 230696 [details] [review]: There is a comment explaining why event was used, but ok.
Review of attachment 230697 [details] [review]: Yes
Review of attachment 230698 [details] [review]: ::: js/ui/grabHelper.js @@ +79,3 @@ + _actorInGrabStack: function(actor) { + while (actor) { + for (let i = 0; i < this._grabStack.length; i++) { You can use this._findStackIndex() instead of hand coding this.
Review of attachment 230699 [details] [review]: ::: js/ui/grabHelper.js @@ +330,2 @@ if (Main.keyboard.shouldTakeEvent(event)) + return true; No, shouldTakeEvent() means that the keyboard wants to get the event through the normal clutter propagation, so you must return false here.
Review of attachment 230700 [details] [review]: LGTM
Review of attachment 230701 [details] [review]: I don't like it much, but ok ::: js/ui/panel.js @@ +600,3 @@ menu = new PopupMenu.RemoteMenu(this.actor, this._targetApp.menu, this._targetApp.action_group); } else { + if (this.menu && this.menu.isDummyQuitMenu) If you change dontCreateMenu semantics to always create a menu, checking this.menu becomes redundant.
(In reply to comment #14) > Review of attachment 230701 [details] [review]: > > I don't like it much, but ok Feel free to suggest or write a better alternative. I think it's cleaner.
(In reply to comment #9) > Review of attachment 230696 [details] [review]: > > There is a comment explaining why event was used, but ok. It's about running before 'key-press-event', which makes no explanation as to why we can't use 'captured-event'.
Attachment 230696 [details] pushed as 41db363 - grabHelper: Use captured-event for escape ungrabs Attachment 230697 [details] pushed as 27ffad2 - grabHelper: Treat the current grabbed actor as a grabbed actor Attachment 230698 [details] pushed as 066e5cd - grabHelper: Drop to the actor clicked on Attachment 230700 [details] pushed as 9dfc3af - popupMenu: Port to GrabHelper Attachment 230701 [details] pushed as fc9a96a - popupMenu: Introduce PopupDummyMenu Pushed with suggested changes.
Not really sure what caused the problem in the first place, but even after these changes the 'mash the meta key' thing still gets us to a state where it's impossible to direct input events to a window....