Created attachment 230651 [details]
power high res mockups for comparision

"Battery" and "Charging - fully charged" looks like they use the same font size. The second is smaller in the mockup.

The mockup have a clearly defined line between the toolbar and the individual panels control area.

If this panel gets changed, all of them needs to:
Current: http://i.imgur.com/XDRxM.jpg
Proposed: http://i.imgur.com/iARyX.jpg

This also makes a clearer distinction between what areas are draggable and who are not.

thanks for the detailed review

I've pushed fixes for almost all of these points.

Known leftover todos at this point:

- implement screen power saving
- hide screen controls when not useful
- Figure out what to do about secondary batteries

To see what I mean with the last one, grep for 'fake' in cc-power-panel.c, and change the #if 0 to #if 1. You'll get some fake devices then, including a fully charged secondary battery.

One more known leftover: remove the padding outside the scrollbar

Created attachment 230751 [details]
modified screenshot

Thanks for the updates, Matthias. This is looking very nice indeed. A couple of minor issues with the latest version (051cb44e0b7):

 * The window needs to be taller.
 * The padding above the battery section needs to be the same as the distance between the other sections below.
 * It would look better if there was more internal horizontal padding in each list row (and it would be good if this was consistent on both the right and left hand sides). 20px looked good in my experiments.
 * It would help if the section headings were indented by 6px.

See the modified screenshot for how these modifications look.

(In reply to comment #0)
<snip>
>  * Some of the status strings obviously need changing. "Charging - fully
> charged" can simply be "Charging"

It should be "charged".

>  * s/Suspend on Battery Power/When on Battery Power
> 
>  * s/Suspend when Plugged In/When Plugged In

You can't use those sentence constructs, they don't translate (imagine what would happen if one language put the combo box in the middle of that statement).

Created attachment 230889 [details]
screenshot 1

Created attachment 230893 [details]
screenshot 2

I've pushed fixes for the review in comment 7

(In reply to comment #11)
> I've pushed fixes for the review in comment 7

Thanks Mattias. They look excellent.

One piece of feedback that we have got a few times is that people are uncertain what "Screen Power Saving" means. If anyone can think of a better label for this, let me know.

It could also be a good idea to add an explanatory subtext under "Screen Power Saving" (in the same style as the one below "Battery"). Something like "Automatically dims and blanks the screen", perhaps...

Created attachment 231115 [details]
screenshot

- White space under the "Power saving" section when screen brightness can't be changed
- "When battery power is critical" section is clipped
- Loads of white space at the bottom

I also get 2 warnings about unref'ing some objects on exit.

(In reply to comment #13)
> Created an attachment (id=231115) [details]
> screenshot
> 
> - White space under the "Power saving" section when screen brightness can't be
> changed
> - "When battery power is critical" section is clipped
> - Loads of white space at the bottom

These are size allocation bugs between gtk and egglistbox - haven't tracked those down.

(In reply to comment #14)
> I also get 2 warnings about unref'ing some objects on exit.

I believe these come out of the egglistbox code, will have a look

(In reply to comment #14)
> I also get 2 warnings about unref'ing some objects on exit.

These are fixed now.

The sizing issues are also fixed now

Bastien, I this branch is ready for another review and/or merge. All the issues pointed out previously have been addressed.

Created attachment 231769 [details] [review]
squashed patch

Review of attachment 231769 [details] [review]:

The killswitch code is a bit confused. You handle killswitches on the Bluetooth side, but not on the NM side.

I would do away with using separate code for both of those, and use the CcRfkillGlib code in the network panel instead.
Or possibly use cut'n'paste the related code from the bluetooth panel instead...

A couple of other comments:
- is there supposed to be so much white space around the panel (compared to the search panel for example)?
- automatic suspend has empty delays by default on my system (nothing matching the default values maybe?)
- there seems to be a bug in the focus drawing, the top item in a frame gets a dotted line on all for borders, tabbing to a lower one shows a dotted line on only 3 sides (no line at the top)
- On my desktop system "When battery power is critical" only offers "Power off" as upower says my machine cannot hibernate. There shouldn't be a drop-down, but a simple label instead.
- The "Screen Power Saving" string needs explaining
- The "Power Saving" frame probably needs some explanation, otherwise it looks plain bizarre. "You can turn off the devices you do not use to save energy".

::: panels/power/cc-power-panel.c
@@ +73,3 @@
+
+#ifdef HAVE_BLUETOOTH
+  BluetoothKillswitch *bt_killswitch;

Use the "default-adapter-powered" property of the BluetoothClient object instead.
We never actually set the killswitch to off in the Bluetooth panel, we only use it to know when the device is hard-locked.

Looking and modifying the powered property should be enough.

@@ +694,3 @@
     }
 
+#if 0

Remove those, or add a commented-out #define at the top of the file.

@@ +771,3 @@
+  priv->setting_brightness = TRUE;
+
+  /* push this to g-p-m */

g-p-m?

@@ +1075,3 @@
   g_ptr_array_unref (devices);
+
+#if 0

Ditto.

@@ +1192,3 @@
+  enabled = gtk_switch_get_active (sw);
+  g_debug ("Setting wifi %s", enabled ? "enabled" : "disabled");
+  nm_client_wireless_set_enabled (panel->priv->nm_client, enabled);

That doesn't disable WWAN hardware such as 3G dongles or WiMax (which nm_client_wwan_set_enabled() and nm_client_wimax_set_enabled() handle respectively).

@@ +1401,3 @@
+  const gchar *s;
+
+  ac_action = g_settings_get_enum (priv->gsd_settings, "sleep-inactive-ac-type");

Use the proper enums for this.

@@ +1406,3 @@
+  battery_timeout = g_settings_get_int (priv->gsd_settings, "sleep-inactive-battery-timeout");
+
+  if (ac_action == 5)

This would save us from using magic numbers.

@@ +1738,3 @@
+                            "org.gnome.SettingsDaemon",
+                            "/org/gnome/SettingsDaemon/Power",
+                            "org.gnome.SettingsDaemon.Power.Screen",

Should the keyboard backlight be handled as well?

@@ +1773,3 @@
+  g_object_unref (widget);
+
+  g_idle_add (grr, self);

Urgh.

(In reply to comment #21)
> Review of attachment 231769 [details] [review]:
> 
> The killswitch code is a bit confused. You handle killswitches on the Bluetooth
> side, but not on the NM side.
> 
> I would do away with using separate code for both of those, and use the
> CcRfkillGlib code in the network panel instead.
> Or possibly use cut'n'paste the related code from the bluetooth panel
> instead...

I'll have to look at that...


> A couple of other comments:
> - is there supposed to be so much white space around the panel (compared to the
> search panel for example)?

Compare to the mockups in https://live.gnome.org/Design/SystemSettings/Power ? Or maybe you are referring to the desktop case where we show less UI ? The patch as attached was forcing a fixed content height regardless of that; I've now changed it so that we only force the content height and show the scrollbar when there's enough UI to require scrolling.


> - automatic suspend has empty delays by default on my system (nothing matching
> the default values maybe?)

Yeah, I was not handling the default values right, indeed. Fixed in the next patch


> - there seems to be a bug in the focus drawing, the top item in a frame gets a
> dotted line on all for borders, tabbing to a lower one shows a dotted line on
> only 3 sides (no line at the top)

I'm seeing that too. Something between adwaita and egglistbox.


> - On my desktop system "When battery power is critical" only offers "Power off"
> as upower says my machine cannot hibernate. There shouldn't be a drop-down, but
> a simple label instead.


On a desktop system without battery, the entire row is supposed to be hidden. I'll attach a screenshot of what it looks like for me when I force 'no batteries'. However, the design was explicit about keeping the combobox if hibernate is not available:

"The hibernate option should be insensitive if the hardware capability is not present. That combobox should still be displayed, however."

Created attachment 231860 [details]
no batteries == no critical battery row

(In reply to comment #21)

> +
> +#ifdef HAVE_BLUETOOTH
> +  BluetoothKillswitch *bt_killswitch;
> 
> Use the "default-adapter-powered" property of the BluetoothClient object
> instead.
> We never actually set the killswitch to off in the Bluetooth panel, we only use
> it to know when the device is hard-locked.
> 
> Looking and modifying the powered property should be enough.
> 

Unfortunately, the property is readonly.

Created attachment 231861 [details] [review]
updated patch

Sorry, wrongly committed...

The focus rendering issue has been fixed in egg-list-box now.

I've tried fixing up some of the issues, but there's just too many things that I'm not happy with:
- Split the Wi-Fi toggle that does everything into 2 (Wi-Fi and Mobile Broadband, the latter for WiMax and cellular)
- I don't like the descriptions in the third person instead of actions. We're describing a switch, I'd prefer we used the imperative like everywhere else.
- "Screen Power Saving" isn't a good label, even with a description. We should turn on the screensaver/dpms a finite amount of time after the screen is locked, which would mean we can label it as "Dim screen when not in use" which is far clearer.
- Bluetooth switch should be disabled for now, and I'll handle making the bluetooth one shareable)
- I didn't have an explanation for the necessity of having a drop-down menu when there's a single possible value (and the others are disabled). Eg. my machine shows:
"When Battery Power is Critical" "Hibernate"
with "Hibernate" being dimmed because it's not possible on this machine. We should just show the only possible value (Power off) as a label.
- The Label under Battery shows "Using battery power". The battery isn't using battery power... It's discharging, or make it clearer that this is the power status for the computer

(In reply to comment #28)
> I've tried fixing up some of the issues, but there's just too many things that
> I'm not happy with:
> - Split the Wi-Fi toggle that does everything into 2 (Wi-Fi and Mobile
> Broadband, the latter for WiMax and cellular)

You want to split it, or you don't think it should be split? The idea for this section is to help someone control their power consumption: if these facilities can be toggled in such a way as to (a) enable/disable specific features and (b) individually affect power consumption, then it would be good to have switches for them.

> - I don't like the descriptions in the third person instead of actions. We're
> describing a switch, I'd prefer we used the imperative like everywhere else.

I'm not sure I fully understand this. Can you give an example of a label that you don't like and what you think it should be changed to?

> - "Screen Power Saving" isn't a good label, even with a description. We should
> turn on the screensaver/dpms a finite amount of time after the screen is
> locked, which would mean we can label it as "Dim screen when not in use" which
> is far clearer.

The idea behind this feature was to combine dimming the screen with the timeout for blanking the screen. "Screen Power Saving" is meant to reflect that. If adjusting the screen blank time out doesn't make sense, or if someone can suggest a better label, then let's change it.

> - Bluetooth switch should be disabled for now, and I'll handle making the
> bluetooth one shareable)
> - I didn't have an explanation for the necessity of having a drop-down menu
> when there's a single possible value (and the others are disabled). Eg. my
> machine shows:
> "When Battery Power is Critical" "Hibernate"
> with "Hibernate" being dimmed because it's not possible on this machine. We
> should just show the only possible value (Power off) as a label.

Makes sense.

> - The Label under Battery shows "Using battery power". The battery isn't using
> battery power... It's discharging, or make it clearer that this is the power
> status for the computer

When charging, the string should read "Foo hours bar minutes until fully charged". When discharging, it should read "Foo hours bar minutes remaining".

Created attachment 232013 [details] [review]
updated patch

Created attachment 232014 [details]
screenshot - new battery section

Created attachment 232015 [details]
screenshot - devices section, and secondary battery

Created attachment 232016 [details]
screenshot - separate mobile and wifi switches

The latest patch addresses most of the issues Bastien has raised:

- splits the wifi switch into wifi and mobile

- redone bluetooth using ::default-adapter-powered instead of killswitch

- make the hibernate combo a simple label if only one option

I've also implemented the latest battery section design: https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/power/power-hi-res.png
To do so, I had to port the code to get battery information from up-client instead of calling into gsd's power plugin.

*** Bug 689063 has been marked as a duplicate of this bug. ***

(In reply to comment #29)
> (In reply to comment #28)
> > I've tried fixing up some of the issues, but there's just too many things that
> > I'm not happy with:
> > - Split the Wi-Fi toggle that does everything into 2 (Wi-Fi and Mobile
> > Broadband, the latter for WiMax and cellular)
> 
> You want to split it, or you don't think it should be split? The idea for this
> section is to help someone control their power consumption: if these facilities
> can be toggled in such a way as to (a) enable/disable specific features and (b)
> individually affect power consumption, then it would be good to have switches
> for them.

I want it split, and that's fixed in Matthias' patch.

> > - I don't like the descriptions in the third person instead of actions. We're
> > describing a switch, I'd prefer we used the imperative like everywhere else.
> 
> I'm not sure I fully understand this. Can you give an example of a label that
> you don't like and what you think it should be changed to?

"Automatically dims and blanks the screen" isn't an action, it's what happens when idle. But the other switches are instant.

Similarly, "Turns off wireless devices". We usually use the imperative "Turn off wireless devices" rather than a descriptive third-person (Turns off wireless devices).

> > - "Screen Power Saving" isn't a good label, even with a description. We should
> > turn on the screensaver/dpms a finite amount of time after the screen is
> > locked, which would mean we can label it as "Dim screen when not in use" which
> > is far clearer.
> 
> The idea behind this feature was to combine dimming the screen with the timeout
> for blanking the screen.

I don't understand what it does... Means it just blanks the screen when it used to dim it? It reduces the amount of time before dimming? Or before blanking?

> "Screen Power Saving" is meant to reflect that. If
> adjusting the screen blank time out doesn't make sense, or if someone can
> suggest a better label, then let's change it.

I think the feature is too hard to explain. This needs to be simplified (both the functionality and the label).

Review of attachment 232013 [details] [review]:

::: panels/power/cc-power-panel.c
@@ +253,3 @@
           case UP_DEVICE_STATE_PENDING_DISCHARGE:
             /* TRANSLATORS: primary battery */
+            details = g_strdup (_("Discharging"));

Allan said:
> When discharging, it should read "Foo hours bar minutes remaining".

@@ +1015,2 @@
   devices = up_client_get_devices (self->priv->up_client);
+g_print ("got %d devices from upower\n", devices->len);

Remove the debug please (or use g_debug).

@@ +1333,3 @@
+  gtk_box_pack_start (GTK_BOX (box), box2, TRUE, TRUE, 0);
+
+  label = gtk_label_new (_("Screen _Power Saving"));

This is still a horrible label.

@@ +1399,3 @@
+  gtk_box_pack_start (GTK_BOX (box2), label, TRUE, TRUE, 0);
+
+  label = gtk_label_new ("Turns off WWAN and WiMax devices");

I don't think that WWAN should appear in user-visible strings. Mobile broadband is better even if the wording makes us think that WiMax both is and isn't mobile broadband (because of the header). Better than using WWAN anyway.

Looking at my phone, it has 3 screen-related settings (the second is

Brightness                                                  [ ] 
Adjust brightness of screen

Brightness                                                  [10 %]
10%

Screen timeout                                              [15 seconds]
Adjust the delay before the screen automatically turns off

(In reply to comment #36)
> > > - Split the Wi-Fi toggle that does everything into 2 (Wi-Fi and Mobile
> > > Broadband, the latter for WiMax and cellular)
> > 
> > You want to split it, or you don't think it should be split? The idea for this
> > section is to help someone control their power consumption: if these facilities
> > can be toggled in such a way as to (a) enable/disable specific features and (b)
> > individually affect power consumption, then it would be good to have switches
> > for them.
> 
> I want it split, and that's fixed in Matthias' patch.

Cool.

> > > - I don't like the descriptions in the third person instead of actions. We're
> > > describing a switch, I'd prefer we used the imperative like everywhere else.
> > 
> > I'm not sure I fully understand this. Can you give an example of a label that
> > you don't like and what you think it should be changed to?
> 
> "Automatically dims and blanks the screen" isn't an action, it's what happens
> when idle. But the other switches are instant.
> 
> Similarly, "Turns off wireless devices". We usually use the imperative "Turn
> off wireless devices" rather than a descriptive third-person (Turns off
> wireless devices).

The phrasing in the mockups makes sense to me as a description of the label above. It's a subtext (a label for a label) rather than a label for the control.

> > > - "Screen Power Saving" isn't a good label, even with a description. We should
> > > turn on the screensaver/dpms a finite amount of time after the screen is
> > > locked, which would mean we can label it as "Dim screen when not in use" which
> > > is far clearer.
> > 
> > The idea behind this feature was to combine dimming the screen with the timeout
> > for blanking the screen.
> 
> I don't understand what it does... Means it just blanks the screen when it used
> to dim it? It reduces the amount of time before dimming? Or before blanking?

If the feature is on, the screen is dimmed when the user is idle (the same as the old "dim screen when inactive" or whatever it was). It will also reduce the timeout for blanking the screen.

> > "Screen Power Saving" is meant to reflect that. If
> > adjusting the screen blank time out doesn't make sense, or if someone can
> > suggest a better label, then let's change it.
> 
> I think the feature is too hard to explain. This needs to be simplified (both
> the functionality and the label).

That's a fair point. The concept for this feature originated from the need to deal with the existing options in the screen panel ("turn screen off when inactive for:" and "dim screen when inactive"); the solution was to combine them.

However, perhaps it would be better to keep a control for the "dim screen when idle" functionality and make "turn screen off when inactive" purely automatic or into a switch (rather than a combobox with different timeouts).

(In reply to comment #38)
> Looking at my phone, it has 3 screen-related settings (the second is

Mine has brightness too, along with a single "time-out" feature to select when the screen gets locked (and the screen turned off).

I think we could expose the lock timeout, dimming and turning off the screen would be respectively fractions and multipliers of that:
- dim after 1/2 the lock timeout or 1 minute, whichever is lower
- turn off the screen after 2x the lock timeout on desktops, instantly on laptops (or 2x on AC, instantly on battery).

This would mean that the power panel has something similar to the privacy panel.

Also, this would mean having the ability to pop down the shell's shield without having to enter a password. Is that possible yet?

(In reply to comment #37)
> Review of attachment 232013 [details] [review]:
> 
> ::: panels/power/cc-power-panel.c
> @@ +253,3 @@
>            case UP_DEVICE_STATE_PENDING_DISCHARGE:
>              /* TRANSLATORS: primary battery */
> +            details = g_strdup (_("Discharging"));

That's still to do. The "Screen Power Saving" section discussion goes to bug 691002.

(In reply to comment #41)
> (In reply to comment #37)
> > Review of attachment 232013 [details] [review] [details]:
> > 
> > ::: panels/power/cc-power-panel.c
> > @@ +253,3 @@
> >            case UP_DEVICE_STATE_PENDING_DISCHARGE:
> >              /* TRANSLATORS: primary battery */
> > +            details = g_strdup (_("Discharging"));
> 
> That's still to do. The "Screen Power Saving" section discussion goes to bug
> 691002.

If you look at the entire get_details_string() function, you will notice that this string is only chosen if we don't have information about the remaining time. If we do have a time, we use this code:

                /* TRANSLATORS: %1 is a time string, e.g. "1 hour 5 minutes" */
                details = g_strdup_printf (_("%s remaining"), time_string);

So I think this is probably as good as we can do.

OK, let's consider this done for now then.