GNOME Bugzilla – Bug 702539
PopupMenu changes and fixes for the aggregate menu
Last modified: 2013-07-15 16:38:10 UTC
Chapter two of my swamp draining, yak shaving adventure. Some of these are simply basic cleanups, other ones are predecessors for more important PopupMenu features that are really tied more to the aggregate menu work and are hard to split out. Hopefully the rationale is well-explained in each commit.
Created attachment 247105 [details] [review] popupMenu: Introduce a way of closing toplevels from sections As the aggregate menu will be built out of sections from each of the menus, we need to ensure that activating an item in one of these sections can close the main menu, even when it is not a menu item. The new API also needs to be flexible enough to ensure that animations can be controlled, like the buttons that lock the screen or launch a new session. Port the user menu to use this new API as well.
Created attachment 247106 [details] [review] popupMenu: Don't close submenus when activating items It looks ugly to have the menu shrinking and having its insides flowing around while the menu is fading out of existence...
Created attachment 247107 [details] [review] popupMenu: Simplify allocation code Use ClutterActor.allocate_align_fill() so we don't have to do this math ourselves. At the same time, clean up the RTL handling so that it's easier to follow.
Created attachment 247108 [details] [review] popupMenu: Ignore submenus when getting the column widths The new designs don't want these to be aligned the same way.
Created attachment 247109 [details] [review] popupMenu: Don't animate submenus closing when closing the menu It looks distracting.
Created attachment 247110 [details] [review] popupMenu: Remove connectSubMenuSignals The code here is a bit messy, as the signal disconnection is handled in two different places. Share code in a better, different way.
Created attachment 247111 [details] [review] popupMenu: Emit open-state-changed at the start of animating a submenu Doing it at the end has confusing semantics, especially as there is this point where isOpen is true, but the corresponding open-state-changed has not been emitted.
Created attachment 247112 [details] [review] popupMenu: Only allow one submenu to be open at a time When the user opens another submenu, close the first one.
Created attachment 247113 [details] [review] popupMenu: Remove 'sensitive' input param It's hard to implement properly, was broken, and unused. If somebody really wants it, they can call setSensitive after constructing the item.
Created attachment 247114 [details] [review] popupMenu: Propagate sensitivity to menu children This way, if a parent is insensitive, all children will be, too.
Created attachment 247115 [details] [review] popupMenu: Ensure that submenus are properly hidden when insensitive We don't actually propagate sensitivity information to submenus; we simply make sure that they can never be open when the parent is insensitive.
Review of attachment 247105 [details] [review]: ::: js/ui/popupMenu.js @@ +1029,3 @@ + + this.connect('activate', Lang.bind(this, function(self, animate) { + this.close(animate); You should not connect to your own signals, override the function instead. ::: js/ui/userMenu.js @@ +805,2 @@ _onLockScreenActivate: function() { + this.menu.itemActivated(BoxPointer.PopupAnimation.NONE); Can we keep close as the public API, and leave itemActivated for internal use?
Review of attachment 247106 [details] [review]: ::: js/ui/popupMenu.js @@ -761,3 @@ object._subMenuActivateId = menu.connect('activate', Lang.bind(this, function(animate) { this.emit('activate', animate); - this.close(animate); Uhm, in this context, 'this' refers to the parent menu, not the submenu. (Also, the function declaration seems to be wrong, the emitter argument is missing)
Review of attachment 247107 [details] [review]: Looks right.
Review of attachment 247108 [details] [review]: Ok
Review of attachment 247109 [details] [review]: Isn't this covered by the previous patch? Please squash.
Review of attachment 247110 [details] [review]: Ok
Review of attachment 247111 [details] [review]: Ok
Review of attachment 247112 [details] [review]: Ok
Review of attachment 247113 [details] [review]: I don't understand this. What is the problem?
Review of attachment 247114 [details] [review]: If the parent is insensitive, isn't the menu supposed to be closed, making the sensitivity irrelevant?
Review of attachment 247115 [details] [review]: Which is kind of what I was saying in my previous review. Either this or the previous patch are redundant.
In any case, the whole stack gives me the following traces: (gnome-shell-real:26790): Gjs-WARNING **: JS ERROR: Exception in callback for signal: destroy: Error: No signal connection 7 found _disconnect@/opt/gnome/share/gjs-1.0/signals.js:74 PopupMenuBase<.addMenuItem/<@/opt/gnome/share/gnome-shell/js/ui/popupMenu.js:886 _emit@/opt/gnome/share/gjs-1.0/signals.js:124 PopupBaseMenuItem<.destroy@/opt/gnome/share/gnome-shell/js/ui/popupMenu.js:160 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 _parent@/opt/gnome/share/gjs-1.0/lang.js:175 PopupSubMenuMenuItem<.destroy@/opt/gnome/share/gnome-shell/js/ui/popupMenu.js:1378 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 PopupMenuBase<.removeAll@/opt/gnome/share/gnome-shell/js/ui/popupMenu.js:977 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 PopupMenuBase<.destroy@/opt/gnome/share/gnome-shell/js/ui/popupMenu.js:990 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 _parent@/opt/gnome/share/gjs-1.0/lang.js:175 RemoteMenu<.destroy@/opt/gnome/share/gnome-shell/js/ui/remoteMenu.js:197 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 Button<.setMenu@/opt/gnome/share/gnome-shell/js/ui/panelMenu.js:146 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 AppMenuButton<._maybeSetMenu@/opt/gnome/share/gnome-shell/js/ui/panel.js:647 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 AppMenuButton<._sync@/opt/gnome/share/gnome-shell/js/ui/panel.js:616 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 AppMenuButton<._focusAppChanged@/opt/gnome/share/gnome-shell/js/ui/panel.js:530 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 activateWindow@/opt/gnome/share/gnome-shell/js/ui/main.js:496 WindowSwitcherPopup<._finish@/opt/gnome/share/gnome-shell/js/ui/altTab.js:400 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 SwitcherPopup<._keyReleaseEvent@/opt/gnome/share/gnome-shell/js/ui/switcherPopup.js:203 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 and _disconnect@/opt/gnome/share/gjs-1.0/signals.js:74 PopupMenuBase<.addMenuItem/<@/opt/gnome/share/gnome-shell/js/ui/popupMenu.js:886 _emit@/opt/gnome/share/gjs-1.0/signals.js:124 PopupBaseMenuItem<.destroy@/opt/gnome/share/gnome-shell/js/ui/popupMenu.js:160 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 _parent@/opt/gnome/share/gjs-1.0/lang.js:175 PopupSubMenuMenuItem<.destroy@/opt/gnome/share/gnome-shell/js/ui/popupMenu.js:1378 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 PopupMenuBase<.removeAll@/opt/gnome/share/gnome-shell/js/ui/popupMenu.js:977 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 NMDevice<._clearSection@/opt/gnome/share/gnome-shell/js/ui/status/network.js:446 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 _parent@/opt/gnome/share/gjs-1.0/lang.js:175 NMDeviceWireless<._clearSection@/opt/gnome/share/gnome-shell/js/ui/status/network.js:1060 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 NMDevice<._queueCreateSection@/opt/gnome/share/gnome-shell/js/ui/status/network.js:439 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 NMDeviceWireless<._onApStrengthChanged@/opt/gnome/share/gnome-shell/js/ui/status/network.js:938 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213
Review of attachment 247113 [details] [review]: setSensitive does: if (this.sensitive == sensitive) return; Given that we set this.sensitive right above we call setSensitive in the constructor, it will always take the shortcut out. Calling setSensitive from the constructor is also annoying for subclasses that override syncSensitive / setSensitive, as they might want to manipulate actors that haven't been constructed yet.
(In reply to comment #21) > If the parent is insensitive, isn't the menu supposed to be closed, making the > sensitivity irrelevant? PopupMenuSections also have parents.
(In reply to comment #12) > Review of attachment 247105 [details] [review]: > > ::: js/ui/popupMenu.js > @@ +1029,3 @@ > + > + this.connect('activate', Lang.bind(this, function(self, animate) { > + this.close(animate); > > You should not connect to your own signals, override the function instead. What function? Instead of saying this.emit('animate'), do you want me to modify all places where we do that to call this._activate(animate); which is overridden in PopupMenu to close()? > ::: js/ui/userMenu.js > @@ +805,2 @@ > _onLockScreenActivate: function() { > + this.menu.itemActivated(BoxPointer.PopupAnimation.NONE); > > Can we keep close as the public API, and leave itemActivated for internal use? If we call close() on a toplevel, it will propagate down to all children. This would mean that calling close() as a public API to close a toplevel from a child will both propagate up and back down the tree? Do we want to change those semantics so that close() only propagates up?
Created attachment 248884 [details] [review] popupMenu: Add a simple way to get the toplevel for a submenu / section This will be used to avoid some nasty signal propagation when wanting to rework how sections / submenus work.
Created attachment 248885 [details] [review] popupMenu: Introduce a way of closing toplevels from sections As the aggregate menu will be built out of sections from each of the menus, we need to ensure that activating an item in one of these sections can close the main menu, even when it is not a menu item. The new API also needs to be flexible enough to ensure that animations can be controlled, like the buttons that lock the screen or launch a new session. Port the user menu to use this new API as well.
Created attachment 248886 [details] [review] popupMenu: Don't close submenus when activating items It looks ugly to have the menu shrinking and having its insides flowing around while the menu is fading out of existence...
Created attachment 248887 [details] [review] popupMenu: Simplify allocation code Use ClutterActor.allocate_align_fill() so we don't have to do this math ourselves. At the same time, clean up the RTL handling so that it's easier to follow.
Created attachment 248888 [details] [review] popupMenu: Properly implement all-spanning actors All-spanning actors do not have columns, so don't stuff their width in the column. Instead, propagate it manually through the API, making sure that the menu is never shorter than the greatest spanning actor.
Created attachment 248889 [details] [review] popupMenu: Ignore submenus when getting the column widths The new designs don't want these to be aligned the same way.
Created attachment 248890 [details] [review] popupMenu: Don't animate submenus closing when closing the menu It looks distracting.
Created attachment 248891 [details] [review] popupMenu: Remove connectSubMenuSignals The code here is a bit messy, as the signal disconnection is handled in two different places. Share code in a better, different way.
Created attachment 248892 [details] [review] popupMenu: Emit open-state-changed at the start of animating a submenu Doing it at the end has confusing semantics, especially as there is this point where isOpen is true, but the corresponding open-state-changed has not been emitted.
Created attachment 248893 [details] [review] popupMenu: Only allow one submenu to be open at a time When the user opens another submenu, close the first one.
Created attachment 248894 [details] [review] popupMenu: Remove 'sensitive' input param It's hard to implement properly, was broken, and unused. If somebody really wants it, they can call setSensitive after constructing the item.
Created attachment 248895 [details] [review] popupMenu: Propagate sensitivity to menu children This way, if a parent is insensitive, all children will be, too.
Created attachment 248896 [details] [review] popupMenu: Ensure that submenus are properly hidden when insensitive We don't actually propagate sensitivity information to submenus; we simply make sure that they can never be open when the parent is insensitive.
Review of attachment 248884 [details] [review]: Yes.
Review of attachment 248885 [details] [review]: I remember the discussion on IRC, this is ok
Review of attachment 248886 [details] [review]: Yep
Review of attachment 248887 [details] [review]: This was already acn, right?
Review of attachment 248888 [details] [review]: Is this tested with classic mode? At some point places-menu had problems with stuff not spanning properly ::: js/ui/popupMenu.js @@ +210,3 @@ let child = this._children[i]; let [min, natural] = child.actor.get_preferred_width(-1); + if (child.span == -1) { Can you have more than one children with span == -1? (Does it even make sense?) And how about the alignment of them? Are they all in the first column?
Review of attachment 248889 [details] [review]: Ok, but... does this mean that potentially like the dot gets a different size (because for instance in one case it's a check mark and in the other it's a dot), misaligning the label? I don't think we want that.
Review of attachment 248890 [details] [review]: Don't we stop closing submenus during the closing animation in a previous patch? (Indeed, my previous review was "squash")
Review of attachment 248891 [details] [review]: Ok
Review of attachment 248892 [details] [review]: Open-state-change is also used for syncing the separators (as well as other stuff), so at least in the closing case we probably don't want this. Or maybe we want a new "animation-finished" signal for that?
Review of attachment 248893 [details] [review]: OK
Review of attachment 248894 [details] [review]: Ok
Review of attachment 248895 [details] [review]: Just a style nit. ::: js/ui/popupMenu.js @@ +1217,3 @@ + return false; + + return this.sourceActor._delegate.getSensitive(); The && you use above looks nicer.
Review of attachment 248896 [details] [review]: ::: js/ui/popupMenu.js @@ +1369,3 @@ + syncSensitive: function() { + let sensitive = this.parent(); + this._triangle.visible = sensitive; Do we want to hide the triangle as well? Is it in the designs? To me, it doesn't look right: the item is still a submenu item, even if disabled.
Review of attachment 248889 [details] [review]: The design is manually allocated to just be inside the left padding of the actor. It doesn't have a "column", per se.
Review of attachment 248892 [details] [review]: After IRC discussion, this is not significantly more broken than now, so ok.
Review of attachment 248889 [details] [review]: Ok then.
(In reply to comment #52) > Do we want to hide the triangle as well? > Is it in the designs? > > To me, it doesn't look right: the item is still a submenu item, even if > disabled. Sort of. See the design here. https://raw.github.com/gnome-design-team/gnome-mockups/master/shell/system-menu/combined-system-status-menu-v3-examples.png If we don't have an active user session (locked, greeter) we hide the submenu and triangle. I'm fine with a "setShowTriangle" or something if you think that sensitivity shouldn't be tied to visibility, though I think that the triangle implies it can be clicked, and I'd hate to see people hover over the triangle, expecting a traditional menu to pop to the side after a timeout.
(In reply to comment #44) > Review of attachment 248888 [details] [review]: > Can you have more than one children with span == -1? (Does it even make sense?) > And how about the alignment of them? Are they all in the first column? No, you can't. Well, you can, but what happens is that it spills out of the allocation, you see some ugly overlapping stuff, and Clutter starts spewing invalid allocation warnings. I should have probably written the commit message better, but the patch was motivated by the menu being too wide, because spanning actors (volume sliders) were counted as in the first column, but allocated across the entire space. Now that we have a fixed width design for the aggregate menu, I'm just going to drop this altogether.
(In reply to comment #46) > Don't we stop closing submenus during the closing animation in a previous > patch? There's two patches: 1) don't close submenus when activating an item within the submenu, 2) don't close all submenus when closing the toplevel. If you want me to squash them together, I can, but I don't know what the commit message would be.
(In reply to comment #58) > (In reply to comment #46) > > Don't we stop closing submenus during the closing animation in a previous > > patch? > > There's two patches: 1) don't close submenus when activating an item within the > submenu, 2) don't close all submenus when closing the toplevel. > > If you want me to squash them together, I can, but I don't know what the commit > message would be. How about "don't close the submenus when their container is closing"? After call, the problem with activating the item inside the submenu is that it closes the entire menu. (In reply to comment #56) > (In reply to comment #52) > > Do we want to hide the triangle as well? > > Is it in the designs? > > > > To me, it doesn't look right: the item is still a submenu item, even if > > disabled. > > Sort of. See the design here. > > https://raw.github.com/gnome-design-team/gnome-mockups/master/shell/system-menu/combined-system-status-menu-v3-examples.png > > If we don't have an active user session (locked, greeter) we hide the submenu > and triangle. I'm fine with a "setShowTriangle" or something if you think that > sensitivity shouldn't be tied to visibility, though I think that the triangle > implies it can be clicked, and I'd hate to see people hover over the triangle, > expecting a traditional menu to pop to the side after a timeout. Is that the only use case we have for submenus? In that case, I guess setSensitivity is fine.
(In reply to comment #59) > Is that the only use case we have for submenus? > In that case, I guess setSensitivity is fine. In core, yes. There's also RemoteMenu, but technically we also need to support nested submenus (and we don't). There's the extension usecases and classic mode, but I don't ever know if we use sensitivity with those.
Attachment 248884 [details] pushed as c1e2d66 - popupMenu: Add a simple way to get the toplevel for a submenu / section Attachment 248885 [details] pushed as 7db0900 - popupMenu: Introduce a way of closing toplevels from sections Attachment 248887 [details] pushed as 2fa4055 - popupMenu: Simplify allocation code Attachment 248889 [details] pushed as ef1eabf - popupMenu: Ignore submenus when getting the column widths Attachment 248891 [details] pushed as 1e781ec - popupMenu: Remove connectSubMenuSignals Attachment 248892 [details] pushed as 2634747 - popupMenu: Emit open-state-changed at the start of animating a submenu Attachment 248893 [details] pushed as 5c036ea - popupMenu: Only allow one submenu to be open at a time Attachment 248894 [details] pushed as bc317bf - popupMenu: Remove 'sensitive' input param Attachment 248895 [details] pushed as 86835db - popupMenu: Propagate sensitivity to menu children Attachment 248896 [details] pushed as 4b889ea - popupMenu: Ensure that submenus are properly hidden when insensitive Pushed with fixes and squashing.