GNOME Bugzilla – Bug 699272
Move the message tray menu to the message tray button
Last modified: 2013-08-19 13:35:37 UTC
See patches. This implements the design seen in: https://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/#Wireframes This is a preliminary patch stack, of sorts. It does not contain support for the Notifications or Chat switches yet, as it's unknown what they will do. Review and feedback appreciated.
Created attachment 242859 [details] [review] popupMenu: Allow PopupMenuManager to take an existing grab helper We want to use PopupMenuManager for the message tray menu, but in order to prevent interference with having two GrabHelpers manage the tray, simply modify PopupMenuManager to allow the existing GrabHelper used in the message tray.
Created attachment 242860 [details] [review] popupMenu: Don't use "grabbed" to determine whether to change menus If we have an existing non-menu grab on the grab helper, we shouldn't open a menu when hovering. Make sure that the topmost grab is indeed a menu.
Created attachment 242861 [details] [review] messageTray: Move adding the menu actor to MessageTrayContextMenu This matches BackgroundMenu and EntryMenu.
Created attachment 242862 [details] [review] messageTray: Use a PopupMenuManager for the message tray context menu
Created attachment 242863 [details] [review] messageTray: Rename the context menu to the tray menu
Created attachment 242864 [details] [review] messageTray: Move the tray menu to a button
Review of attachment 242859 [details] [review]: You should at least explain the changes related to .actor and .actor._delegate, or put a comment there.
Review of attachment 242860 [details] [review]: Ok
Review of attachment 242861 [details] [review]: Yes
Review of attachment 242862 [details] [review]: Ok
Review of attachment 242862 [details] [review]: Or maybe not. I think we need a nested grab, to block clicks on sources when the menu is up.
Review of attachment 242863 [details] [review]: Cosmetic and not necessary, please avoid it.
Review of attachment 242864 [details] [review]: ::: js/ui/messageTray.js @@ +1581,2 @@ this.actor.connect('button-release-event', Lang.bind(this, function(actor, event) { + this._trayMenu.close(BoxPointer.PopupAnimation.FULL); This needs to be handled by GrabHelper. @@ +1719,3 @@ + track_hover: true, + can_focus: true, + accessible_name: _("Tray Menu"), Tray Menu is a bit technical, Chat Menu maybe?
Created attachment 251372 [details] [review] messageTray: Move adding the menu actor to MessageTrayContextMenu This matches BackgroundMenu and EntryMenu.
Created attachment 251373 [details] [review] messageTray: Use a PopupMenuManager for the message tray context menu
Created attachment 251374 [details] [review] messageTray: Rename the context menu to the tray menu
Created attachment 251375 [details] [review] messageTray: Move the tray menu to a button
Review of attachment 251372 [details] [review]: sure
Review of attachment 251373 [details] [review]: looks like a reasonable consolidation of code. Looking I can't spot any immediate functional differences.
Review of attachment 251374 [details] [review]: calling it trayMenu is a little redundant since it's in the messageTray class. I'd probably just call it menu, but whatever!
(In reply to comment #23) > calling it trayMenu is a little redundant since it's in the messageTray class. > I'd probably just call it menu, but whatever! Well, this and this is sort of weird quirk of the MessageTray code, we have the "notification widget" (which corresponds to banners), and the "tray actor" (which corresponds to the system texture tray). We use this._trayState and this._traySummoned, etc. to talk about the "tray actor" itself. Due to historical reasons, we have "this.actor" which corresponds to the gray tray (it used to be a container for both), but I'm going to change that in a future cleanup.
Attachment 251372 [details] pushed as e6ef115 - messageTray: Move adding the menu actor to MessageTrayContextMenu Attachment 251373 [details] pushed as f5c0706 - messageTray: Use a PopupMenuManager for the message tray context menu Attachment 251374 [details] pushed as 548111e - messageTray: Rename the context menu to the tray menu
Created attachment 251526 [details] [review] messageTray: Move the tray menu to a button Implements latest designs.
Review of attachment 251526 [details] [review]: Didn't test because does not apply cleanly and I am to lazy to rebase right now. But code looks good to me modulo magic number. ::: js/ui/messageTray.js @@ +1822,3 @@ + this._messageTrayMenuButton = new MessageTrayMenuButton(this); + this._messageTrayMenuButton.actor.margin_left = 16; Magic number use a constant.
Created attachment 251959 [details] [review] grabHelper: Introduce a stack of grab helpers GrabHelpers use a 'captured-event' to steal events and emulate modality or grab-like semantics. There can be issues when you try to use multiple GrabHelpers stacked on each other. As Clutter follows the DOM-like semantics of "first come, first serve", when a second GrabHelper connects to 'captured-event', its callback will only be processed *after* the first GrabHelper's callback is called. This breaks the expectation of narrowing modality where new modals take priority over the old ones. Solving this globally in a cleaner manner would require a rewrite of pushModal/GrabHelper. As a stopgap fix for now, use one shared 'captured-event' handler between all GrabHelper instances, and delegate to the individual GrabHelpers.
Created attachment 251960 [details] [review] messageTray: Move the tray menu to a button
we should probably get this landed for the ui freeze
I can confirm that the GrabHelper patch fixes the hover issues with the tray menu. Also, the available menu items work, which is always good. But, wasn't the design supposed to include a Chat switch?
Review of attachment 251959 [details] [review]: ::: js/ui/grabHelper.js @@ +15,3 @@ +function _onCapturedEvent(actor, event) { + let grabHelper = _grabHelperStack[_grabHelperStack.length - 1]; + return grabHelper._onCapturedEvent(event); You're calling it from outside the class, make it public? Maybe handleCapturedEvent()? @@ +22,3 @@ + + if (_capturedEventId == 0) + _capturedEventId = global.stage.connect('captured-event', _onCapturedEvent); captured-event is also used for wayland event handling (including updating the pointer position on screen). Are there some ordering guarantees in GObject, or nothing at all? Because otherwise we need to move away from captured-event entirely
Review of attachment 251960 [details] [review]: Looks good
(In reply to comment #32) > Are there some ordering guarantees in GObject, or nothing at all? > Because otherwise we need to move away from captured-event entirely The guarantee is first-come first-serve. If A connects before B, then A gets the signal emission before B. connect_after and vfunc emission make things more complicated, but if captured-event is always the first signal connected, then it should be guaranteed to be called first.
(In reply to comment #31) > Also, the available menu items work, which is always good. But, wasn't the > design supposed to include a Chat switch? Since we don't have a Chat app yet, I'm not sure what it would do. You could turn it on, but would turning it off mean `pkill empathy`? Setting telepathy status to offline?
(In reply to comment #35) > (In reply to comment #31) > > Also, the available menu items work, which is always good. But, wasn't the > > design supposed to include a Chat switch? > > Since we don't have a Chat app yet, I'm not sure what it would do. You could > turn it on, but would turning it off mean `pkill empathy`? Setting telepathy > status to offline? Set telepathy to off, and then if empathy doesn't die after the usual 60 seconds timeout it's an empathy bug. Basically, what the old selector did.
Attachment 251959 [details] pushed as 8d9aa63 - grabHelper: Introduce a stack of grab helpers Attachment 251960 [details] pushed as d1a8177 - messageTray: Move the tray menu to a button Pushed with suggestion changes. Other switches will come soon.