Created attachment 257424 [details] [review]
power: Use UPower directly instead of gnome-settings-daemon

UPower master exports a display device that can be used to
compute whether to show a status icon, and what we should show.

Review of attachment 257424 [details] [review]:

Needs rebasing after https://git.gnome.org/browse/gnome-shell/commit/?id=575b373cd5bc

Created attachment 257447 [details] [review]
power: Use UPower directly instead of gnome-settings-daemon

UPower master exports a display device that can be used to
compute whether to show a status icon, and what we should show.

Review of attachment 257447 [details] [review]:

::: js/ui/status/power.js
@@ -57,3 @@
     },
 
-    _statusForDevice: function(device) {

I really like the statusForDevice that simply returns our status string instead of a bunch of wacky flow control. Please keep it, even if you have to rename it _getStatus() or something.

@@ +59,3 @@
+    _syncStatusLabel: function() {
+        if (this._proxy.Type == UPower.DeviceKind.UPS)
+            this._item.label.text = _("UPS");

Are you sure the designers want to show UPS?

@@ +116,3 @@
     },
+
+    _sync: function() {

I'd prefer if this kept the post-rewrite form, with the _sync simply doing everything.

(In reply to comment #4)
> @@ +59,3 @@
> +    _syncStatusLabel: function() {
> +        if (this._proxy.Type == UPower.DeviceKind.UPS)
> +            this._item.label.text = _("UPS");
> 
> Are you sure the designers want to show UPS?

We need to show discharging UPSes, this is what this does.

Created attachment 257542 [details] [review]
power: Use UPower directly instead of gnome-settings-daemon

UPower master exports a display device that can be used to
compute whether to show a status icon, and what we should show.

Review of attachment 257542 [details] [review]:

::: js/ui/status/power.js
@@ +94,3 @@
 
+    _syncVisibility: function() {
+        let visible = this._proxy.IsPresent;

What does IsPresent mean if the primary device is e.g. a keyboard battery or something else like that?

What we want:

  * If there's no battery (or UPS) device:
    * Hide the menu item.
    * The indicator should be the power icon (system-shutdown-symbolic). It might make sense to add this as a separate indicator using _addIndicator.

  * If there's a battery (or UPS) device:
    * Show the menu item.
    * The indicator should be the battery icon.

@@ +115,3 @@
+     },
+
+    _syncLabels: function() {

Could you merge these three methods?

(In reply to comment #7)
> Review of attachment 257542 [details] [review]:
> 
> ::: js/ui/status/power.js
> @@ +94,3 @@
> 
> +    _syncVisibility: function() {
> +        let visible = this._proxy.IsPresent;
> 
> What does IsPresent mean if the primary device is e.g. a keyboard battery or
> something else like that?

It's never a keyboard battery. It's a composite device that aggregates all the batteries or UPSes on the system. On a system with neither, the properties are undefined, and IsPresent is false.

> What we want:
> 
>   * If there's no battery (or UPS) device:
>     * Hide the menu item.
>     * The indicator should be the power icon (system-shutdown-symbolic). It
> might make sense to add this as a separate indicator using _addIndicator.

I missed that last bit. I'll respin the code.

>   * If there's a battery (or UPS) device:
>     * Show the menu item.
>     * The indicator should be the battery icon.

That works as expected.

> @@ +115,3 @@
> +     },
> +
> +    _syncLabels: function() {
> 
> Could you merge these three methods?

Sure.

Created attachment 257550 [details] [review]
power: Use UPower directly instead of gnome-settings-daemon

UPower master exports a display device that can be used to
compute whether to show a status icon, and what we should show.

Review of attachment 257550 [details] [review]:

Looks good.

::: js/ui/status/power.js
@@ +120,2 @@
     },
+

Stray whitespace.

Pushed minus the stray linefeed.

Attachment 257550 [details] pushed as 0b8c0c2 - power: Use UPower directly instead of gnome-settings-daemon