GNOME Bugzilla – Bug 704368
Initial work of the aggregate system menus
Last modified: 2013-07-26 08:43:11 UTC
This implements new designs for the current volume, power, bluetooth and system (previously user) menus. This does not squash them into one menu yet; it simply repurposes the existing menus. This does not implement the new designs for the network menu -- there are significant changes to that, and it will take a bug of its own to get through. Putting everything in one giant aggregate menu will happen at the *end* of development, after which we'll introduce the two new status components. See https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Created attachment 249345 [details] [review] userWidget: Rename the default style class of the user widget As the status chooser is going away, it doesn't make sense to keep this around.
Created attachment 249346 [details] [review] status: Remove settings actions These won't be used in the new combined panel menu
Created attachment 249347 [details] [review] userMenu: Implement new user menu design For now, turn it into another system status button with a simple icon. We'll revamp this when we revamp how panel menus work in general.
Created attachment 249348 [details] [review] popupMenu: Remove combo boxes and child menus They're no longer used with the removal of the avatar widget.
Created attachment 249349 [details] [review] popupMenu: Add a status label and icon to submenu menu items This will allow us to implement the new submenu designs in the aggregate menu.
Created attachment 249350 [details] [review] power: Restyle the power menu for the new design Remove the icon next to devices, and make the percentage have the "status" color.
Created attachment 249351 [details] [review] power: Remove other devices Simply have one section
Created attachment 249352 [details] [review] power: Use UPowerGlib for constants
Created attachment 249353 [details] [review] power: Implement new power menu design
Created attachment 249354 [details] [review] status: Rebrand the user menu as the system menu As the user menu no longer really shows anything about the user, and just power menu and settings, it's not really deserving of the "user menu" name, so rename it to the ever-blandly-named "system menu".
Created attachment 249355 [details] [review] system: Remove suspend from the features of the system menu This will go in the end session dialog instead.
Created attachment 249356 [details] [review] system: Remove Install Updates & Restart This will go in the end session dialog.
Created attachment 249357 [details] [review] system: Move the Switch User and Log Out items to a new submenu
Created attachment 249358 [details] [review] system: Use icons for actions at the bottom of the system menu
Created attachment 249359 [details] [review] volume: Implement new volume menu design
Created attachment 249360 [details] [review] slider: Make clicking anywhere on the slider menu item pass to the slider
Created attachment 249361 [details] [review] slider: Explicitly grab the device that was clicked It seems the Clutter bug mentioned has been fixed.
Created attachment 249362 [details] [review] bluetooth: Adapt to new designs for the bluetooth menu
Review of attachment 249345 [details] [review]: Ok
Review of attachment 249346 [details] [review]: ::: js/ui/status/bluetooth.js @@ -257,3 @@ - default: - break; - } Is it possible we end up with an empty device menu after this?
Review of attachment 249347 [details] [review]: Ok
Review of attachment 249348 [details] [review]: Lovely!
Review of attachment 249349 [details] [review]: ::: js/ui/popupMenu.js @@ +1292,3 @@ Extends: PopupBaseMenuItem, + _init: function(text, wantIcon) { Heh... I've been bitten by boolean parameters in the past. How about params?
Review of attachment 249350 [details] [review]: ::: js/ui/status/power.js @@ +181,2 @@ let percentLabel = new St.Label({ text: C_("percent of battery remaining", "%d%%").format(Math.round(percentage)), + style_class: 'popup-status-menu-item popup-battery-percentage' }); Maybe popup-battery-percentage can have the color in the theme, rather than adding two classes?
Review of attachment 249351 [details] [review]: ::: js/ui/status/power.js @@ +126,3 @@ _devicesChanged: function() { this._syncIcon(); this._readPrimaryDevice(); I think you can merge these in one function now.
Review of attachment 249352 [details] [review]: Ok
Review of attachment 249353 [details] [review]: Ok
Review of attachment 249354 [details] [review]: No. All kinds of extensions are hooking into this, changing the ID is harmful. Change just the icon and a11y name.
Review of attachment 249355 [details] [review]: Ok
Review of attachment 249356 [details] [review]: Ok (but how are users notified that updates are ready to install?)
Review of attachment 249357 [details] [review]: ::: js/ui/status/system.js @@ +56,3 @@ this._loginManager = LoginManager.getLoginManager(); this._userManager = AccountsService.UserManager.get_default(); this._user = this._userManager.get_user(GLib.get_user_name()); You moved these in an earlier patch...
Review of attachment 249358 [details] [review]: Yep
(In reply to comment #20) > Is it possible we end up with an empty device menu after this? Does it matter? We're removing the device menu. (In reply to comment #23) > Heh... I've been bitten by boolean parameters in the past. > How about params? This is really only meant to be temporary until we can use icons everywhere we use submenus. The only case that isn't true right now is remote menu (for which we do have icons) and keyboard status (where we also have icons) (In reply to comment #24) > Maybe popup-battery-percentage can have the color in the theme, rather than > adding two classes? I don't see what's wrong with two classes: it's a status item, and it's also a percentage. (In reply to comment #25) > I think you can merge these in one function now. I can, but I like that we have one for syncing the primary device and one for the icon. I renamed each function so now we have _sync, _syncIcon, and _syncStatusLabel, which matches the naming scheme used elsewhere. (In reply to comment #28) > No. > > All kinds of extensions are hooking into this, changing the ID is harmful. > Change just the icon and a11y name. I do not care about extensions compatibility. The change with the new menu design is just too big. The aggregate menu will stop it from being in the statusArea entirely, as well: See: https://git.gnome.org/browse/gnome-shell/commit/?h=wip/aggregate-menu&id=71afb331cd2ed85b55afb243b24af64c2d2f859d (In reply to comment #30) > Ok (but how are users notified that updates are ready to install?) The thought is that the restart button will glow yellow or green or something. (In reply to comment #31) > You moved these in an earlier patch... No I didn't. It's like that in master. I need createSubMenu after we get the user manager since we depend on it in the submenu.
Review of attachment 249359 [details] [review]: ::: js/ui/popupMenu.js @@ -500,3 @@ - } -}); - Why this removal (not mentioned in the commit message)?
Review of attachment 249360 [details] [review]: Yes, this was a regression from earlier popup menu refactoring, but we should keep it in mind.
Review of attachment 249361 [details] [review]: Or maybe we stopped sending core events mixed with XI2 slave events...
Review of attachment 249362 [details] [review]: ::: js/ui/status/bluetooth.js @@ +49,3 @@ + _sync: function() { + let nDevices = this._applet.get_devices().length; Unless you patch libgnome-bluetooht-applet, get_devices() gives you all known devices (including, say, your phone you associated once), not all connected devices. Also, you don't seem to account the killswitch state here, so why are you listening to it?
(In reply to comment #33) > (In reply to comment #20) > > Is it possible we end up with an empty device menu after this? > > Does it matter? We're removing the device menu. Ok > (In reply to comment #23) > > Heh... I've been bitten by boolean parameters in the past. > > How about params? > > This is really only meant to be temporary until we can use icons everywhere we > use submenus. The only case that isn't true right now is remote menu (for which > we do have icons) and keyboard status (where we also have icons) Make sure it goes away soon then. > (In reply to comment #24) > > Maybe popup-battery-percentage can have the color in the theme, rather than > > adding two classes? > > I don't see what's wrong with two classes: it's a status item, and it's also a > percentage. Well, we tend to avoid it everywhere else, but ok. > (In reply to comment #25) > > I think you can merge these in one function now. > > I can, but I like that we have one for syncing the primary device and one for > the icon. I renamed each function so now we have _sync, _syncIcon, and > _syncStatusLabel, which matches the naming scheme used elsewhere. That's fine. > (In reply to comment #28) > > No. > > > > All kinds of extensions are hooking into this, changing the ID is harmful. > > Change just the icon and a11y name. > > I do not care about extensions compatibility. The change with the new menu > design is just too big. The aggregate menu will stop it from being in the > statusArea entirely, as well: > > See: > https://git.gnome.org/browse/gnome-shell/commit/?h=wip/aggregate-menu&id=71afb331cd2ed85b55afb243b24af64c2d2f859d Eh... I really don't understand why you can't keep SystemStatusButton as a deprecated class and add SystemIndicator along side that, but that's complementary. > (In reply to comment #30) > > Ok (but how are users notified that updates are ready to install?) > > The thought is that the restart button will glow yellow or green or something. Nice. > (In reply to comment #31) > > You moved these in an earlier patch... > > No I didn't. It's like that in master. I need createSubMenu after we get the > user manager since we depend on it in the submenu. Ok, I was probably misleaded by how splinter showed the diff.
Review of attachment 249354 [details] [review]: This is sad, but let's not block it on extensions...
(In reply to comment #34) > Why this removal (not mentioned in the commit message)? Because it's unused. I should point that out in the commit message, then. (In reply to comment #37) > Unless you patch libgnome-bluetooht-applet, get_devices() gives you all known > devices (including, say, your phone you associated once), not all connected > devices. Aha, thanks. > Also, you don't seem to account the killswitch state here, so why are you > listening to it? Because an earlier verison of the patch listened to it.
Created attachment 249590 [details] [review] volume: Implement new volume menu design Since the designs require that we have a custom layout for the slider item, this means that the PopupSliderMenuItem is unused, so remove it as well.
Created attachment 249591 [details] [review] bluetooth: Adapt to new designs for the bluetooth menu
Review of attachment 249590 [details] [review]: Looks good, except that the commit message might explain more of the new designs.
Review of attachment 249591 [details] [review]: ::: js/ui/status/bluetooth.js @@ +21,3 @@ this.parent('bluetooth-disabled-symbolic', _("Bluetooth")); + // The Bluetooth menu only appears when Bluetooth is turned on. Still this doesn't convince me. Having bluetooth on does not necessarily mean using bluetooth headphones (which is basically what connected devices are).
Attachment 249345 [details] pushed as 8ce5b08 - userWidget: Rename the default style class of the user widget Attachment 249346 [details] pushed as 2cd4177 - status: Remove settings actions Attachment 249347 [details] pushed as d802416 - userMenu: Implement new user menu design Attachment 249348 [details] pushed as 3816db0 - popupMenu: Remove combo boxes and child menus Attachment 249349 [details] pushed as fb0f9cd - popupMenu: Add a status label and icon to submenu menu items Attachment 249350 [details] pushed as 040bb93 - power: Restyle the power menu for the new design Attachment 249351 [details] pushed as 8cce1b4 - power: Remove other devices Attachment 249352 [details] pushed as 22c8be6 - power: Use UPowerGlib for constants Attachment 249353 [details] pushed as 37a316e - power: Implement new power menu design Attachment 249354 [details] pushed as ca861d8 - status: Rebrand the user menu as the system menu Attachment 249355 [details] pushed as 14372ba - system: Remove suspend from the features of the system menu Attachment 249356 [details] pushed as de8ca5c - system: Remove Install Updates & Restart Attachment 249357 [details] pushed as d1a763b - system: Move the Switch User and Log Out items to a new submenu Attachment 249358 [details] pushed as c50b23e - system: Use icons for actions at the bottom of the system menu Attachment 249360 [details] pushed as 0cee0e0 - slider: Make clicking anywhere on the slider menu item pass to the slider Attachment 249361 [details] pushed as 44f8fec - slider: Explicitly grab the device that was clicked Attachment 249590 [details] pushed as 73cd595 - volume: Implement new volume menu design Pushed most of the commits; on IRC we agreed that we need more refined designs for the bluetooth menu, so we're waiting on the designers before pushing that one.
Review of attachment 249591 [details] [review]: Almost. ::: js/ui/status/bluetooth.js @@ +42,3 @@ + _sync: function() { + let connectedDevices = this._applet.get_devices().map(function(device) { + return device.connected; .filter(), not .map()
Attachment 249591 [details] pushed as 7dc9a9b - bluetooth: Adapt to new designs for the bluetooth menu