GNOME Bugzilla – Bug 692527
Spruce up the apps-menu according to the latest design
Last modified: 2013-01-30 16:55:05 UTC
Mockup: http://rishi.fedorapeople.org/apps-menu.png The apps-menu extension was replaced with a version based on AxeMenu, which was iteratively toned down to match the above design.
Created attachment 234393 [details] [review] apps-menu: Replace it with a new version based on AxeMenu
Review of attachment 234393 [details] [review]: ::: extensions/apps-menu/extension.js @@ +66,3 @@ + _onDestroy : function() { + if (this._clickEventId) this.actor.disconnect(this._clickEventId); You don't need this (and later), destroy() chains up to dispose() and disconnects all signal handlers anyway. @@ +186,3 @@ + this.actor.connect('captured-event', Lang.bind(this, this._onCapturedEvent)); + + Main.overview.connect('showing', Lang.bind(this, function() { You need to disconnect this in disable(), so save the id. @@ +202,3 @@ + this.menu.connect('open-state-changed', Lang.bind(this, this._onOpenStateToggled)); + + Main.wm.addKeybinding('menu-toggle', Shouldn't we take over panel-main-menu (alt-f1) instead? @@ +213,3 @@ + // queue relayouts for us when the panel moves. Queue a relayout + // when that happens. + function() { Same here, you need to disconnect. @@ +326,3 @@ + this._activeContainer = null; + } + for (let i=0, l=children.length; i<l; i++) { this.parent(menu, open); @@ +369,3 @@ + _scrollToButton: function(button) { + var current_scroll_value = this.applicationsScrollBox.get_vscroll_bar().get_adjustment().get_value(); + var box_height = this.applicationsScrollBox.get_allocation_box().y2 - (Here and later) you should cache calls to get_allocation_box() for the same actor. Also, please use let, for consistency with the rest of the code. @@ +546,3 @@ + this.menu.actor.style = ('max-height: ' + + Math.round(monitor.height - Main.panel.actor.height-80) + + for (var i=0; i<apps.length; i++) { Isn't this already handled by PopupMenu?
Created attachment 234414 [details] [review] apps-menu: Keep hot corner working while the message tray is up This is a small fix I had pushed in the apps-menu branch, please squash with the main patch (unless the omission is a conscious change of course)
(In reply to comment #3) > Created an attachment (id=234414) [details] [review] > apps-menu: Keep hot corner working while the message tray is up > > This is a small fix I had pushed in the apps-menu branch, please squash with > the main patch (unless the omission is a conscious change of course) Thanks. Squashed.
Created attachment 234424 [details] [review] apps-menu: Replace it with a new version based on AxeMenu This includes some more cleanups of the original AxeMenu code: - I have replaced the custom buttons with PopupMenu.PopupBaseMenuItem - The HotCorner is disabled when the menu is open - Some style fixes
(In reply to comment #2) > Review of attachment 234393 [details] [review]: Thanks a lot for the review. That was fast. I was planning to cheat and slip in another pass before you got around to it. :-P > ::: extensions/apps-menu/extension.js > @@ +66,3 @@ > > + _onDestroy : function() { > + if (this._clickEventId) this.actor.disconnect(this._clickEventId); > > You don't need this (and later), destroy() chains up to dispose() and > disconnects all signal handlers anyway. Done. These custom button classes have been replaced with PopupMenu.PopupBaseMenuItems. > @@ +186,3 @@ > + this.actor.connect('captured-event', Lang.bind(this, > this._onCapturedEvent)); > + > + Main.overview.connect('showing', Lang.bind(this, function() { > > You need to disconnect this in disable(), so save the id. Done. > @@ +202,3 @@ > + this.menu.connect('open-state-changed', Lang.bind(this, > this._onOpenStateToggled)); > + > + Main.wm.addKeybinding('menu-toggle', > > Shouldn't we take over panel-main-menu (alt-f1) instead? I don't know. Should we? > @@ +213,3 @@ > + // queue relayouts for us when the panel moves. Queue a relayout > + // when that happens. > + function() { > > Same here, you need to disconnect. Done. > @@ +326,3 @@ > + this._activeContainer = null; > + } > + for (let i=0, l=children.length; i<l; i++) { > > this.parent(menu, open); Umm... I did not get this one. Which line are you referring to? > @@ +369,3 @@ > + _scrollToButton: function(button) { > + var current_scroll_value = > this.applicationsScrollBox.get_vscroll_bar().get_adjustment().get_value(); > + var box_height = this.applicationsScrollBox.get_allocation_box().y2 - > > (Here and later) you should cache calls to get_allocation_box() for the same > actor. > Also, please use let, for consistency with the rest of the code. Done. > @@ +546,3 @@ > + this.menu.actor.style = ('max-height: ' + > + Math.round(monitor.height - > Main.panel.actor.height-80) + > + for (var i=0; i<apps.length; i++) { > > Isn't this already handled by PopupMenu? Yes, you are right. Removed.
Review of attachment 234424 [details] [review]: More comments... ::: extensions/apps-menu/extension.js @@ +194,1 @@ + _capturedEventId = this.actor.connect('captured-event', Lang.bind(this, this._onCapturedEvent)); Not this id, the one from Main.overview.connect() @@ +208,2 @@ this._display(); + _installedChangedId = appSys.connect('installed-changed', Lang.bind(this, this.reDisplay)); Please use a deferred work, so that installing apps doesn't freeze the UI to rebuild the menu until actually needed. @@ +208,3 @@ this._display(); + _installedChangedId = appSys.connect('installed-changed', Lang.bind(this, this.reDisplay)); + this.menu.connect('open-state-changed', Lang.bind(this, this._onOpenStateToggled)); Override onOpenStateChanged from PanelMenu.Button instead. @@ +317,3 @@ + this._activeContainer = null; + } + This is the line I was referring to with "this.parent()". @@ +341,3 @@ + if (!entry.get_app_info().get_nodisplay()) { + let app = appSys.lookup_app_by_tree_entry(entry); + if (!this.applicationsByCategory[dir.get_menu_id()]) Here too, fetch the menu id into a variable, to reduce the marshalling overhead. ::: extensions/apps-menu/org.gnome.shell.extensions.apps-menu.gschema.xml.in @@ +3,3 @@ + <key name="menu-toggle" type="as"> + <default>["<Super>r"]</default> + <_summary>Toggle AxeMenu.</_summary> Application menu.
Created attachment 234746 [details] [review] apps-menu: Replace it with a new version based on AxeMenu - Addressed review comments - Some more cleanups and style fixes I have not been able to test the disable path after this round of changes because the tweak tool is misbehaving and refusing to start, and I could not quickly figure out how to directly use the org.gnome.Shell.Extensions API.
(In reply to comment #2) > Review of attachment 234393 [details] [review]: > @@ +202,3 @@ > + this.menu.connect('open-state-changed', Lang.bind(this, > this._onOpenStateToggled)); > + > + Main.wm.addKeybinding('menu-toggle', > > Shouldn't we take over panel-main-menu (alt-f1) instead? On second thoughts, I think you are right. I have changed it to take over panel-main-menu.
(In reply to comment #7) > Review of attachment 234424 [details] [review]: Thanks for the review. > ::: extensions/apps-menu/extension.js > @@ +194,1 @@ > + _capturedEventId = this.actor.connect('captured-event', > Lang.bind(this, this._onCapturedEvent)); > > Not this id, the one from Main.overview.connect() Ah, ok. Sorry about that. Changed. > @@ +208,2 @@ > this._display(); > + _installedChangedId = appSys.connect('installed-changed', > Lang.bind(this, this.reDisplay)); > > Please use a deferred work, so that installing apps doesn't freeze the UI to > rebuild the menu until actually needed. I changed it to only rebuild the menu if it is already open, and if its not, then it gets rebuilt the next time it is opened. > @@ +208,3 @@ > this._display(); > + _installedChangedId = appSys.connect('installed-changed', > Lang.bind(this, this.reDisplay)); > + this.menu.connect('open-state-changed', Lang.bind(this, > this._onOpenStateToggled)); > > Override onOpenStateChanged from PanelMenu.Button instead. Done. > @@ +317,3 @@ > + this._activeContainer = null; > + } > + > > This is the line I was referring to with "this.parent()". Yes, ofcourse. Done. > @@ +341,3 @@ > + if (!entry.get_app_info().get_nodisplay()) { > + let app = appSys.lookup_app_by_tree_entry(entry); > + if (!this.applicationsByCategory[dir.get_menu_id()]) > > Here too, fetch the menu id into a variable, to reduce the marshalling > overhead. Done. > ::: extensions/apps-menu/org.gnome.shell.extensions.apps-menu.gschema.xml.in > @@ +3,3 @@ > + <key name="menu-toggle" type="as"> > + <default>["<Super>r"]</default> > + <_summary>Toggle AxeMenu.</_summary> > > Application menu. Done.
(In reply to comment #8) > I have not been able to test the disable path after this round of changes > because the tweak tool is misbehaving and refusing to start, and I could not > quickly figure out how to directly use the org.gnome.Shell.Extensions API. gnome-shell-extension-tool -d my_uuid Or use: http://extensions.gnome.org/local/
Created attachment 234830 [details] [review] apps-menu: Replace it with a new version based on AxeMenu - Increased the height of the menu because too many categories had a scrollbar. Now on my system only Accessories and Internet have scrollbars. - Fixed some more style issues. - Replaced toggleMenu by overloading the toggle method on the PopupMenu.
Created attachment 234831 [details] [review] apps-menu: Replace it with a new version based on AxeMenu And get rid of all the remaining style classes. They are not adding much at the moment.
Review of attachment 234831 [details] [review]: Looks good to me!
Comment on attachment 234831 [details] [review] apps-menu: Replace it with a new version based on AxeMenu Thanks for the review. Pushed.