Created attachment 315119 [details] [review]
power: Add battery percentage label

FIXME: where should the GSetting live?
FIXME: couldn't figure out how to center it vertically

WIP: http://i.imgur.com/WMnYIlf.png

Allan's notes: smaller text, less padding between the icon and its label.

Review of attachment 315119 [details] [review]:

::: js/ui/status/power.js
@@ +36,2 @@
         this._indicator = this._addIndicator();
+        this._percentageLabel = new St.Label({ text: '100%' });

new St.Label({ y_expand: true,
               y_align: Clutter.ActorAlign.CENTER });

Where would I put the GSettings? In gsettings-desktop-schemas (even if unused by mutter?) or in gnome-shell?

I don't have a strong preference - unless the option would be exposed in Settings, I think gnome-shell is fine, otherwise maybe stick it in org.gnome.desktop.interface?

Created attachment 315413 [details] [review]
power: Add battery percentage label

FIXME: remove some padding

Smaller text, centered

(In reply to Bastien Nocera from comment #7)
> Created attachment 315413 [details] [review] [review]
> power: Add battery percentage label
> 
> FIXME: remove some padding

Really not sure how to fix that in a way that would be acceptable for gnome-shell, so I'll just leave it there. The code is functional.

Review of attachment 315413 [details] [review]:

::: js/ui/status/power.js
@@ +43,2 @@
         this._indicator = this._addIndicator();
+        //this._indicator.indicators.add_actor

Stray comment

@@ +99,3 @@
             this._item.actor.show();
+            if (this._desktopSettings.get_boolean(SHOW_BATTERY_PERCENTAGE))
+              this._percentageLabel.show();

Or:
  this._percentageLabel.visible = this._desktopSettings.get_boolean(SHOW_BATTERY_PERCENTAGE);

(this is just a side comment, no strong preference on my part)

@@ +115,3 @@
         this._item.icon.icon_name = icon;
 
+        // The label

???

(In reply to Florian Müllner from comment #9)
<snip>
> +        // The label
> 
> ???

Read the code in context? :)

Created attachment 315734 [details] [review]
power: Add battery percentage label

An oft requested feature, available in 4 separate extensions to
gnome-shell, and in most mobile OSes.

Review of attachment 315734 [details] [review]:

OK

It's missing some padding fixes (lower spacing between icon and label, but not between label and drop-down arrow).

> > +        // The label
> > 
> > ???
> 
> Read the code in context? :)

Well, there's a variable called "label" which is used to set a label text - adding a "The label" comment doesn't really provide any additional information ;-)
(I understand that the only reason the comment is there is consistency with the "status label" comment, which isn't the most useful comment either ...)

(In reply to Florian Müllner from comment #14)
> > > +        // The label
> > > 
> > > ???
> > 
> > Read the code in context? :)
> 
> Well, there's a variable called "label" which is used to set a label text -
> adding a "The label" comment doesn't really provide any additional
> information ;-)
> (I understand that the only reason the comment is there is consistency with
> the "status label" comment, which isn't the most useful comment either ...)

That's the only reason, I'm fine removing it. But I find it weird when separate sections are added and only some have headings/comments. In any case, whatever is comfortable for you.

(In reply to Bastien Nocera from comment #13)
> It's missing some padding fixes (lower spacing between icon and label, but
> not between label and drop-down arrow).

Probably the easiest way:

 - add something like "this.indicators.add_style_class('power-status');" in init
 - use .power-status.status-indicators-box in the CSS to override the spacing


(In reply to Bastien Nocera from comment #15)
> That's the only reason, I'm fine removing it. But I find it weird when
> separate sections are added and only some have headings/comments.

Yeah, I think "icon label" hits the spot between consistency with other sections and non-informativeness.

(In reply to Florian Müllner from comment #16)
> (In reply to Bastien Nocera from comment #13)
> > It's missing some padding fixes (lower spacing between icon and label, but
> > not between label and drop-down arrow).
> 
> Probably the easiest way:
> 
>  - add something like "this.indicators.add_style_class('power-status');" in
> init
>  - use .power-status.status-indicators-box in the CSS to override the spacing

Couldn't figure out how to regenerate (or understand) the SASS code that I'm supposed to edit.

> (In reply to Bastien Nocera from comment #15)
> > That's the only reason, I'm fine removing it. But I find it weird when
> > separate sections are added and only some have headings/comments.
> 
> Yeah, I think "icon label" hits the spot between consistency with other
> sections and non-informativeness.

Changed locally.

Created attachment 315777 [details] [review]
Tweak power indicator spacing

(In reply to Bastien Nocera from comment #17)
> Couldn't figure out how to regenerate (or understand) the SASS code that I'm
> supposed to edit.

Yeah, it's way too complicated. I'm attaching a sass patch, if it looks good to you I'll push it and do a gnome-shell patch you can squash.


> > Yeah, I think "icon label" hits the spot between consistency with other
> > sections and non-informativeness.
> 
> Changed locally.

Huh? I meant to say that the comment in the current patch versions looks good to me.

Created attachment 315818 [details] [review]
power: Add battery percentage label

An oft requested feature, available in 4 separate extensions to
gnome-shell, and in most mobile OSes.

(In reply to Florian Müllner from comment #18)
> Created attachment 315777 [details] [review] [review]
> Tweak power indicator spacing
> 
> (In reply to Bastien Nocera from comment #17)
> > Couldn't figure out how to regenerate (or understand) the SASS code that I'm
> > supposed to edit.
> 
> Yeah, it's way too complicated. I'm attaching a sass patch, if it looks good
> to you I'll push it and do a gnome-shell patch you can squash.

Allan said "good enough". I figured out the packages I needed to regenerate the gnome-shell CSS files.

> > > Yeah, I think "icon label" hits the spot between consistency with other
> > > sections and non-informativeness.
> > 
> > Changed locally.
> 
> Huh? I meant to say that the comment in the current patch versions looks
> good to me.

Well, that would be why it was already fixed locally ;)

I've merged the gsettings-desktop-schemas patch. I'll let you commit this last patch.

Attachment 315818 [details] pushed as 31201d9 - power: Add battery percentage label