After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 682540 - Lock & login screens - combine battery, network and volume menus
Lock & login screens - combine battery, network and volume menus
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: lock-screen
3.5.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-08-23 13:24 UTC by Allan Day
Modified: 2012-11-28 07:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
StIcon: deprecate StIcon:icon-name and StIcon:icon-type (10.53 KB, patch)
2012-08-26 14:07 UTC, Giovanni Campagna
committed Details | Review
PanelMenu: Allow multiple icons in one SystemStatusButton (4.79 KB, patch)
2012-08-26 14:07 UTC, Giovanni Campagna
committed Details | Review
VolumeMenu: show icons from network and power when locked (8.92 KB, patch)
2012-08-26 14:08 UTC, Giovanni Campagna
none Details | Review
st: Remove StIconType (48.56 KB, patch)
2012-08-26 15:08 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review
VolumeMenu: show icons from network and power when locked (9.24 KB, patch)
2012-08-26 15:13 UTC, Giovanni Campagna
reviewed Details | Review
Add a new lock screen menu to combine volume network and power (19.13 KB, patch)
2012-08-26 16:15 UTC, Giovanni Campagna
needs-work Details | Review
Add a new lock screen menu to combine volume network and power (18.74 KB, patch)
2012-08-26 16:52 UTC, Giovanni Campagna
committed Details | Review
st: Remove StIconType (47.68 KB, patch)
2012-08-29 09:49 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
st: Remove StIconType (47.67 KB, patch)
2012-08-29 19:07 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Allan Day 2012-08-23 13:24:41 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.
Comment 1 Giovanni Campagna 2012-08-23 13:28:01 UTC
Argh, this has regressed since I landed the screen shield - originally there would be no network or battery menu there.
Comment 2 Giovanni Campagna 2012-08-26 14:07:50 UTC
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.
Comment 3 Giovanni Campagna 2012-08-26 14:07:59 UTC
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.
Comment 4 Giovanni Campagna 2012-08-26 14:08:08 UTC
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.
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-08-26 14:35:13 UTC
Review of attachment 222463 [details] [review]:

Do you want my patch for this, or are you just going to leave all existing icons deprecated?
Comment 6 Giovanni Campagna 2012-08-26 14:49:07 UTC
(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.
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-08-26 15:08:47 UTC
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.
Comment 8 Giovanni Campagna 2012-08-26 15:13:15 UTC
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.
Comment 9 Jasper St. Pierre (not reading bugmail) 2012-08-26 15:16:31 UTC
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.
Comment 10 Giovanni Campagna 2012-08-26 16:15:53 UTC
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.
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-08-26 16:25:22 UTC
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.
Comment 12 Giovanni Campagna 2012-08-26 16:52:23 UTC
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.
Comment 13 Jasper St. Pierre (not reading bugmail) 2012-08-28 22:55:38 UTC
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?
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-08-28 22:56:07 UTC
Review of attachment 222464 [details] [review]:

Yes.
Comment 15 Jasper St. Pierre (not reading bugmail) 2012-08-28 22:57:02 UTC
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)
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-08-28 22:57:25 UTC
Review of attachment 222472 [details] [review]:

I'll try to update this patch to something that works later tonight.
Comment 17 Giovanni Campagna 2012-08-28 23:12:40 UTC
(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.
Comment 18 Giovanni Campagna 2012-08-28 23:16:13 UTC
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
Comment 19 Jasper St. Pierre (not reading bugmail) 2012-08-29 09:49:11 UTC
Created attachment 222728 [details] [review]
st: Remove StIconType

GTK+ works by explicitly specifying a -symbolic suffix for all icons.
Do the same here.
Comment 20 Giovanni Campagna 2012-08-29 11:32:05 UTC
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?
Comment 21 Giovanni Campagna 2012-08-29 11:32:21 UTC
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?
Comment 22 Jasper St. Pierre (not reading bugmail) 2012-08-29 17:05:44 UTC
(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.
Comment 23 Jasper St. Pierre (not reading bugmail) 2012-08-29 19:07:18 UTC
Created attachment 222827 [details] [review]
st: Remove StIconType

GTK+ works by explicitly specifying a -symbolic suffix for all icons.
Do the same here.
Comment 24 Giovanni Campagna 2012-08-29 19:38:24 UTC
Review of attachment 222827 [details] [review]:

Let's do this.
Comment 25 Jasper St. Pierre (not reading bugmail) 2012-08-29 19:41:47 UTC
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.
Comment 26 Dag Odenhall 2012-11-28 07:59:35 UTC
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.