GNOME Bugzilla – Bug 682540
Lock & login screens - combine battery, network and volume menus
Last modified: 2012-11-28 07:59:35 UTC
We need to allow users to see if they have a low battery or if they have lost their network connection from the lock screen. We also need to allow them to change the volume. However, it doesn't make sense to show extra information about the battery, or to provide links to the relevant system settings panels. Therefore, combine the battery, network and volume system status icons into a single menu which contains a volume slider.
Argh, this has regressed since I landed the screen shield - originally there would be no network or battery menu there.
Created attachment 222463 [details] [review] StIcon: deprecate StIcon:icon-name and StIcon:icon-type Reroute setting those properties to a GIcon. API users are expected to create GIcon directly now. The advantage is that from a StIcon you can now create a similar one by accessing :gicon.
Created attachment 222464 [details] [review] PanelMenu: Allow multiple icons in one SystemStatusButton This generalizes the code previously used by NetworkMenu, so that all SystemStatusButton now can add multiple icons.
Created attachment 222465 [details] [review] VolumeMenu: show icons from network and power when locked The design has a combined volume-network-power indicator in the lock screen. Implement it by creating additional icons in the volume menu, that are bound to the real ones.
Review of attachment 222463 [details] [review]: Do you want my patch for this, or are you just going to leave all existing icons deprecated?
(In reply to comment #5) > Review of attachment 222463 [details] [review]: > > Do you want my patch for this, or are you just going to leave all existing > icons deprecated? I was thinking of the latter actually. Deprecated doesn't mean removed, not now at least. Of course, if you still have the patch, why not.
Created attachment 222472 [details] [review] st: Remove StIconType GTK+ works by explicitly specifying a -symbolic suffix for all icons. Do the same here. Here's my *very* large patch, as-is. Will require modification to work with your stuff, unless you want to rebase your patch on top of mine. This is after a very quick rebase with no testing, so it will probably require some modification. Merge conflict central, here.
Created attachment 222474 [details] [review] VolumeMenu: show icons from network and power when locked The design has a combined volume-network-power indicator in the lock screen. Implement it by creating additional icons in the volume menu, that are bound to the real ones. Fixed a problem in GDM mode.
Review of attachment 222474 [details] [review]: Not a fan of this style approach. I'd prefer that it would be some other actor, not the volume menu, even though the dropdown only has the volume menu in it.
Created attachment 222478 [details] [review] Add a new lock screen menu to combine volume network and power The design has a combined volume-network-power indicator in the lock screen, which when opened shows a volume slider. Implement it by abstracting the volume menu into a PopupMenuSection, and by creating three StIcons bound to the real ones.
Review of attachment 222478 [details] [review]: ::: js/ui/sessionMode.js @@ +46,3 @@ order: [ 'a11y', 'display', 'keyboard', + 'volume', 'battery', 'lockScreen', 'powerMenu' This is the silly part. Fine for now, but we need to get a better system in the future. @@ +52,3 @@ 'volume': imports.ui.status.volume.Indicator, 'battery': imports.ui.status.power.Indicator, + 'lockScreen': imports.ui.status.lockScreenMenu.Indicator, Can the screen be locked in GDM? I didn't think it could. ::: js/ui/status/lockScreenMenu.js @@ +25,3 @@ + GObject.BindingFlags.SYNC_CREATE); + this._volume.mainIcon.bind_property('visible', this._volumeIcon, 'visible', + GObject.BindingFlags.SYNC_CREATE); I was thinking it would be better if you just added the actor directly, but then I thought that this would be a whole other can of worms. ::: js/ui/status/volume.js @@ +3,2 @@ const Clutter = imports.gi.Clutter; +const GObject = imports.gi.GObject; Leftover imports. @@ +7,3 @@ const St = imports.gi.St; +const Main = imports.ui.main; Leftover imports. @@ +250,3 @@ + _onScrollEvent: function(actor, event) { + let direction = event.get_scroll_direction(); + this._volumeMenu.scroll(event); You need to pass direction here.
Created attachment 222483 [details] [review] Add a new lock screen menu to combine volume network and power The design has a combined volume-network-power indicator in the lock screen, which when opened shows a volume slider. Implement it by abstracting the volume menu into a PopupMenuSection, and by creating three StIcons bound to the real ones.
Review of attachment 222483 [details] [review]: I'm a fan of this. ::: js/ui/sessionMode.js @@ +74,2 @@ ], implementation: { Is there any reason this can't just be STANDARD_TRAY_AREA_IMPLEMENTATION?
Review of attachment 222464 [details] [review]: Yes.
Review of attachment 222463 [details] [review]: Sure. In the future I'd like to see if we could deprecate load_icon_name in favor of load_gicon (_with_colors, but renamed)
Review of attachment 222472 [details] [review]: I'll try to update this patch to something that works later tonight.
(In reply to comment #13) > Review of attachment 222483 [details] [review]: > > I'm a fan of this. > > ::: js/ui/sessionMode.js > @@ +74,2 @@ > ], > implementation: { > > Is there any reason this can't just be STANDARD_TRAY_AREA_IMPLEMENTATION? Bug 682546 for that.
Attachment 222463 [details] pushed as d3b0d23 - StIcon: deprecate StIcon:icon-name and StIcon:icon-type Attachment 222464 [details] pushed as 41dc9e0 - PanelMenu: Allow multiple icons in one SystemStatusButton Attachment 222483 [details] pushed as c1de278 - Add a new lock screen menu to combine volume network and power Leaving open for axing StIconType
Created attachment 222728 [details] [review] st: Remove StIconType GTK+ works by explicitly specifying a -symbolic suffix for all icons. Do the same here.
Review of attachment 222728 [details] [review]: Mostly ok. Two observations: - This commit removes the symbolic icon name generation. This is not a problem for the gnome icon theme, since all the icons we use are present, but in other themes you now risk a non symbolic icon in place of a more generic symbolic - This commit removes the icon_type property, and this technically an API break, in API freeze. I don't think you need R-T approval, but a heads up to gnome-shell-list would be nice. ::: js/ui/endSessionDialog.js @@ -328,3 @@ let icon = textureCache.load_icon_name(this._iconBin.get_theme_node(), iconName, - St.IconType.SYMBOLIC, Need to change 'avatar-default' to 'avatar-default-symbolic' for this. ::: js/ui/messageTray.js @@ -2463,3 @@ _init: function() { - this.parent(_("System Information"), 'dialog-information', St.IconType.SYMBOLIC); - this.setTransient(true); Unrelated change? ::: js/ui/status/network.js @@ +1690,3 @@ if (!this._source) { this._source = new MessageTray.Source(_("Network Manager"), + 'network-transmit-symbolic'); Why not network-transmit-receive-symbolic? ::: src/st/st-icon.c @@ +54,2 @@ GIcon *gicon; + gchar *icon_name; icon_name? What for?
(In reply to comment #21) > Review of attachment 222728 [details] [review]: > > Mostly ok. Two observations: > - This commit removes the symbolic icon name generation. This is not a problem > for the gnome icon theme, since all the icons we use are present, but in other > themes you now risk a non symbolic icon in place of a more generic symbolic GTK+ doesn't do any mangling. I'm basically trying to match GTK+, here.
Created attachment 222827 [details] [review] st: Remove StIconType GTK+ works by explicitly specifying a -symbolic suffix for all icons. Do the same here.
Review of attachment 222827 [details] [review]: Let's do this.
Attachment 222827 [details] pushed as c21b1e5 - st: Remove StIconType Let the merge conflicts roll in. If need be, we can back this out before release.
I'll go ahead and say that this is a really confusing UI, to the point that it prompted me to report it as bug 689200. I understand the rationale for it, but maybe it would be better to have the menu attach only to the volume icon, and have the other icons not have menus at all, i.e. not be clickable. That might also be confusing to some, but less so than the current UI I'd expect. Or perhaps move them to the left, like the user name which isn't clickable either.