Urgh, why are we re-writing stuff that works? If gnome-shell meant re-writing everything in javascript just to integrate fully I'm sure I would have vetoed the original proposal. What is it in gnome-power-manager that doesn't work well with the shell?

The idea is to have "indicators" fully integrated with the Shell, i.e. written in the same toolkit and with the same appearance. GTK+ looks weird when coming out of the top panel.

I don't think that amounts to that much code. g-p-m is still used for configuration (and of course all the management), only the notification icon and the menu have to be moved to the Shell. Is that what you understood at first?

(In reply to comment #2)
> The idea is to have "indicators" fully integrated with the Shell, i.e. written
> in the same toolkit and with the same appearance. GTK+ looks weird when coming
> out of the top panel.

That's ridiculous. If "GTK+ looks weird" then the shell needs to provide the hooks for native applications to use, not for the the shell to just start re-writing subsets of applications. What's next? A rewrite of nm-applet?

> I don't think that amounts to that much code. g-p-m is still used for
> configuration (and of course all the management), only the notification icon
> and the menu have to be moved to the Shell. Is that what you understood at
> first?

That's the thing, it's not. You're showing the battery data directly. That fails hard for CSR based devices, and multiple batteries on laptops. If you just wanted to put the UI in the shell and use g-p-m as a backend, then you would need to make the GPM DBus interface much larger, and use that. Just processing the data from upower is not enough to show a sane UI.

(In reply to comment #3)
> (In reply to comment #2)
> > The idea is to have "indicators" fully integrated with the Shell, i.e. written
> > in the same toolkit and with the same appearance. GTK+ looks weird when coming
> > out of the top panel.
> 
> That's ridiculous. If "GTK+ looks weird" then the shell needs to provide the
> hooks for native applications to use, not for the the shell to just start
> re-writing subsets of applications. What's next? A rewrite of nm-applet?

Except that it is not as easy as it sounds, and still you would get usability issues. For example, Javascript based indicators are fully keyboard navigable (including moving from power to user status and back to network and activities), while GtkStatusIcon are not.
Also, you would still need to develop two UIs, one Gtk-based for GNOME Panel and other DEs, the other using ShellToolkit and Clutter. If this happens in Javascript or in C doesn't make much difference (except that Javascript is by far easier to write and maintain).
Similarly it is secondary if it happens in the gnome-shell repository or inside gnome-power-manager. I posted the patch here following advice from bug 621017, but it could be easily rewritten using GNOME Shell extension support.

> > I don't think that amounts to that much code. g-p-m is still used for
> > configuration (and of course all the management), only the notification icon
> > and the menu have to be moved to the Shell. Is that what you understood at
> > first?
> 
> That's the thing, it's not. You're showing the battery data directly. That
> fails hard for CSR based devices, and multiple batteries on laptops. If you
> just wanted to put the UI in the shell and use g-p-m as a backend, then you
> would need to make the GPM DBus interface much larger, and use that. Just
> processing the data from upower is not enough to show a sane UI.

I admit I did not read full g-p-m source code, so I don't know what changes would be needed, but I think that as a pre-alpha it could work.
For what concerns multiple batteries, currently only the last one (in DBus order) is recognized as the main laptop battery for the purposes of tooltip and icon (and similarly it assumes that you have only one AC connector), but this can be easily changed to show an average percentage, while still showing them individually in the popup menu, as it is now.

(In reply to comment #1)
> If gnome-shell meant re-writing everything in javascript just to integrate
> fully I'm sure I would have vetoed the original proposal.

I'm pretty sure this is why you were CC'ed ;)


(In reply to comment #3)
> > I don't think that amounts to that much code. g-p-m is still used for
> > configuration (and of course all the management), only the notification icon
> > and the menu have to be moved to the Shell. Is that what you understood at
> > first?
> 
> That's the thing, it's not. You're showing the battery data directly.

Yes, and I'm confident that the patch will be rejected for that reason. The idea is to move the code for status icons of system components into the shell[0] - not the components itself. If we cannot interface with g-p-m to get the required information/notifications, then we should work together on that interface instead of re-inventing the wheel poorly.


[0] http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus

(In reply to comment #4)
> I posted the patch here following advice from bug 621017,
> but it could be easily rewritten using GNOME Shell extension support.

No, extensions are strictly optional, not meant to implement basic functionality (this is one of the big flaws of gnome-panel). If the system has a battery, then we must show an indicator for it.

> (In reply to comment #3)
> > > I don't think that amounts to that much code. g-p-m is still used for
> > > configuration (and of course all the management), only the notification icon
> > > and the menu have to be moved to the Shell. Is that what you understood at
> > > first?
> > 
> > That's the thing, it's not. You're showing the battery data directly.
> 
> Yes, and I'm confident that the patch will be rejected for that reason. The
> idea is to move the code for status icons of system components into the
> shell[0] - not the components itself. If we cannot interface with g-p-m to get
> the required information/notifications, then we should work together on that
> interface instead of re-inventing the wheel poorly.

Uhm? Are you proposing that g-p-m exposes a DBus interface with battery info? To me that looks like re-inventing the UPower (DBus for power part of udev) wheel, even though it may be more UI oriented.
(btw, in the patch notifications are generated by g-p-m through libnotify, only the icon is managed by JS)

(In reply to comment #4)
> For example, Javascript based indicators are fully keyboard navigable
> (including moving from power to user status and back to network and
> activities), while GtkStatusIcon are not.

Then that's ridiculous. Somebody needs to inform the release team if this missing feature is going to mean re-writing all of the system tray using stuff. What about Bluetooth? Power? Network?

I really think that putting the status area code in the shell and fixing the usability bugs in one place is a better solution than just rewriting all the session inside gnome-shell.

(In reply to comment #7)
> Uhm? Are you proposing that g-p-m exposes a DBus interface with battery info?

That's one way of doing it, yes. GetDevicesForDisplay() returning an array of (icon,text,percentage) and a property called default-icon would be 20 seconds worth of thought. You don't have to duplicate the whole UPower tree, just the UI components.

Of course, it's more integration work than just slapping a UI on upower. I still think fixing the bugs in the status area would be a better use of developer time than re-inventing the wheel.

Richard.

Ohh, if you're re-writing *all* of the session that uses the status area into javascript, please add gnome-packagekit to your list. One less thing for me to maintain.

(In reply to comment #8)
> (In reply to comment #4)
> > For example, Javascript based indicators are fully keyboard navigable
> > (including moving from power to user status and back to network and
> > activities), while GtkStatusIcon are not.
> 
> Then that's ridiculous. Somebody needs to inform the release team if this
> missing feature is going to mean re-writing all of the system tray using stuff.
> What about Bluetooth? Power? Network?
> 
> I really think that putting the status area code in the shell and fixing the
> usability bugs in one place is a better solution than just rewriting all the
> session inside gnome-shell.

How do you propose that, then?

(In reply to comment #9)
> (In reply to comment #7)
> > Uhm? Are you proposing that g-p-m exposes a DBus interface with battery info?
> 
> That's one way of doing it, yes. GetDevicesForDisplay() returning an array of
> (icon,text,percentage) and a property called default-icon would be 20 seconds
> worth of thought. You don't have to duplicate the whole UPower tree, just the
> UI components.
> 
> Of course, it's more integration work than just slapping a UI on upower. I
> still think fixing the bugs in the status area would be a better use of
> developer time than re-inventing the wheel.

Still you get g-p-m specific code inside the shell. That or UPower to me does not make much difference (expecially given that code based on UPower exists, while a DBus interface for g-p-m does not).

(In reply to comment #10)
> Ohh, if you're re-writing *all* of the session that uses the status area into
> javascript, please add gnome-packagekit to your list. One less thing for me to
> maintain.

I'm not rewriting myself, I am showing how it is possible to use Shell APIs and the ShellToolkit to write indicators that are better integrated into the shell.
I was not planning to write the Battery and Power indicator, just it looked so easy once accustomed to the DBus API.

The first important thing to realize is that in the GNOME Shell design, the system status area is a privileged place.

 * The icons there largely correspond to your systems hardware - battery, network, sound volume, etc.

 * The icons there are static - since they largely correspond to hardware, they may be somewhat different on different systems, but if an icon is there, it will typically always be there. It's not a place for notifications. Notifications live in the message tray; and work through the existing notification protocol.

 * The icons there should have a highly integrated behavior with the shell - a consistent look to their "menus", integrated keyboard navigation, stylized contents with the ability to have custom UI elements in the drop-down, etc.

It will be possible to add status icons as part of extensions (since extensions are just more JS), and we will likely want to to eventually allow "system extensions" that are invisible to the user. We can't predict every piece of hardware that someone will have on a system shipping with GNOME, so an integrator might need a way to add an extra icon. But, for now, given the choices of:

 - Poorly integrated GTK+ based status icons running out of process.

 - Coming up with complicated new protocols for external icons, based on predicted future needs and then supporting them stably.

 - Trying to stabilize the internal Javascript interfaces so that g-p-m could drop a Javascript file.

Simply shipping the icons as part of the shell makes a lot of sense. The hardware components we want to integrate with - power management, network, bluetooth, volume, have corresponding GNOME components. In some cases, interacting with the system level components makes sense (for volume, we want to drive PulseAudio directly), in other cases, we may need to extend the GNOME pieces to export additional D-Bus interfaces. I certainly don't want it ever to be the case that we have thousands of lines of Javascript encoding all sorts of hadware details living in the gnome-shell module.

For package installation / upgrades, we'd certainly see the interaction is through the message tray, rather than through the status area. The interaction likely needs a lot of tuning.

(Note that as a short term thing, what we are going to do is to exile status icons for non-hardware components to the message tray. This causes problems for getting the interactions in the message tray exactly right, for keynav, etc, just as it does in the system status area. And it's not clear to me that PackageKit actually needs persistent icons other than the icons that we already show reminding the user of past notifications. But if the status icon is removed we need some way to "remind" the user if they log out or reboot after seeing a PackageKit message. We'd certainly like to discuss the PackageKit interaction more, but not in the context of this bug.)

(In reply to comment #9)
> Of course, it's more integration work than just slapping a UI on upower. I
> still think fixing the bugs in the status area would be a better use of
> developer time than re-inventing the wheel.

The fundamental problem with the status area is that the behavior of items is defined entirely by the embedded process, which makes it impossible for the embedding process to provide a consistent behavior. It is not a mere question of "fixing bugs", e.g. KDE created a complete replacement[0] for Plasma because they considered the existing xembed solution unfixable.
Unfortunately the KDE spec is pretty much tailored for plasma, which is why it was decided to not follow Ubuntu in adapting it for the shell.


(In reply to comment #10)
> Ohh, if you're re-writing *all* of the session that uses the status area into
> javascript, please add gnome-packagekit to your list. One less thing for me to
> maintain.

No, the plan is to only allow a very limited set of "system status" items in the panel - in particular notification icons as the status icon of gnome-packagekit will be moved elsewhere (namely the message tray).



[0] http://www.notmart.org/misc/statusnotifieritem/index.html

The work should definitely be based on top of gnome-power-manager, either loading in the interesting parts as a introspected library, or as a D-Bus daemon (gpm could hide its status icon when it has the gnome-shell as a client).

The new UI presented by the shell should definitely not be a new client to upower, throwing away all the UI tweaks that have gone into gnome-power-manager.

(In reply to comment #10)
> Ohh, if you're re-writing *all* of the session that uses the status area into
> javascript, please add gnome-packagekit to your list. One less thing for me to
> maintain.

Hi Richard!  No, we want to share as much code as possible with GNOME 2.  However we have a new design for various components, and that does entail writing new code.

I think the approach of adding a DBus interface to gnome-power-manager makes sense.

Comment on attachment 164359 [details] [review]
Patch for experimental UPIndicator

as discussed in bug 623656, we're going to do this by getting the information from gnome-power-manager itself, not from upower. parts of the patch may still be useful.

Per bug 624361, I started implementing this using the DBus API described in http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/BatteryPower and found out some problem with this approach, so I am waiting some input from g-p-m developers before proceeding.

Created attachment 173354 [details] [review]
Status area: introduce battery & power indicator

Add an indicator for battery charge and power indication in the
system status area, using the newly exported DBus API from
gnome-power-manager.

(In reply to comment #18)
> Add an indicator for battery charge and power indication in the
> system status area, using the newly exported DBus API from
> gnome-power-manager.

Note: I wanted to thank Giovanni for his help in the GPM DBus interface, and writing this g-p-m interface code. It's appreciated.

One thing to note:

If you want to know what's using power, then you probably want to call "gnome-power-statistics --device wakeups" as this will focus on the wakeups section when it's started.

If you want to make the battery drop down clickable (for more info about the specific battery, like we have with gnome2), and highlight the correct device, then call "gnome-power-statistics --device /org/freedesktop/UPower/devices/battery_BAT0" -- the ID is the first string in the tuple returned by g-p-m.

The only other thing is I've noticed you've defined UPDeviceType manually. UPower is now introspectable (in git master there are loads of fixups, so it might actually work) so it might make sense to use that. It's up to you really.

Richard.

Ohh, and is there a mechanism yet for g-p-m to know to not show the status are icon? Is it going to be hidden by the shell (blacklisted) or does g-p-m has to pay attention to some environment variable? Or maybe just check if the notification daemon supports persistence or not

(In reply to comment #20)
> Ohh, and is there a mechanism yet for g-p-m to know to not show the status are
> icon? Is it going to be hidden by the shell (blacklisted) or does g-p-m has to
> pay attention to some environment variable? Or maybe just check if the
> notification daemon supports persistence or not

Notification persistence cannot be used as it is supported by notification-daemon (so g-p-m would not be showing its icon in gnome-panel).
If you want to have different behaviour for the shell, the best way is to watch org.gnome.Shell, else every tray icon with WM_CLASS 'gnome-power-manager' is automatically filtered by the shell, so no issue.

Giovanni, if you make the changes for gnome-power-statistics, I'll reassign this bug to DanW and get something upstream. Thanks.

Created attachment 173431 [details] [review]
Status area: introduce battery & power indicator

Add an indicator for battery charge and power indication in the
system status area, using the newly exported DBus API from
gnome-power-manager.

Still not using UPower as I don't want to spend time loading typelibs
and resolving types just for two enums.

(In reply to comment #23)
> Created an attachment (id=173431) [details] [review]
> Status area: introduce battery & power indicator
> 
> Add an indicator for battery charge and power indication in the
> system status area, using the newly exported DBus API from
> gnome-power-manager.

Looks good to me, thanks!

Comment on attachment 173431 [details] [review]
Status area: introduce battery & power indicator

We need to do something to ensure that the new g-p-m is running don't we? Like, if org.gnome.PowerManager exists but doesn't support the right interfaces, we need to kill it and start the version in our $prefix?

>+const N_ = Gettext.ngettext;

unused, so it can just be removed, but if ngettext is needed in the future, "N_" is not the right abbreviation. In C, N_() is used as a no-op macro to mark strings in array/struct initializers, etc, where the string needs to be extracted for translation, but you can't actually call gettext() on it at that point because a function call would be a syntax error.

there doesn't seem to be any standard abbreviation for ngettext()...

>+// copied from misc/telepathy.js
>+function makeProxyClass(iface) {

hm... i need to push on bug 610859 again

>+        this.menu.addAction(_("What is using power..."),function() {

the mockup has "What's" rather than "What is", which I think is better, because "What is" sounds more like a question, and then it would need a "?" at the end

>+        this.menu.addAction(_("Power settings"),function() {

capital "S" for consistency with other status menus

>+    _primaryDeviceChanged: function() {

seems more like it should be "_checkIfPrimaryDeviceChanged" (and likewise the other function)

>+                this._batteryItem.actor.hide();

you need to hide _deviceSep here too... _otherDevicesChanged might finish before _primaryDeviceChanged does.

>+            let device_id, device_type, summary, percentage, state, time;
>+            [device_id, device_type, summary, percentage, state, time] = device;

you can combine that into one line: "let [device_id, etc] = device;"

>+            if (device_type == UPDeviceType.BATTERY) {
>+                let text;
>+                let minutes = Math.floor(time / 60);

unused variables? Leftover from an earlier version but "summary" already has the right text now?

Hm... it looks like "summary" is generated from information which isn't otherwise directly exposed via the D-Bus API. This means if the designers decide we need to show more/less information, that will require changes to gnome-power-manager, which is what we were trying to avoid.

>+                let device_id, device_type, summary, percentage, state, time;
>+                [device_id, device_type, summary, percentage, state, time] = devices[i];

(again, can combine)

>+                if (device_type == UPDeviceType.AC_POWER || device_id == this._primaryDeviceId)
>+                    continue;

hm... so GetDevices returns the primary device, but doesn't tell us that it's the primary, so we have to call GetPrimaryDevice as well? That seems like it could be made simpler.

>+                this.menu.addMenuItem(item, 2 + position);

I'd like it if we could avoid the "2" there. set a "firstOtherDeviceIndex" property from _init or something.

>+                // work around gjs not supporting interfaces, in
>+                // particular Gio.Icon
>+                // (and will fail if the Gio.Icon is not a Gio.ThemedIcon)
>+                let theme_icon = /GThemedIcon ([a-z\-]+)/.exec(icon);

i'm not sure what this all means...

>diff --git a/tools/build/gnome-shell.modules b/tools/build/gnome-shell.modules

I also needed to install upower-devel from packages, and I needed to add libnotify to gnome-shell.modules, because git gnome-power-manager requires git libnotify. And then it requires git control-center as well and I gave up trying to actually build it at that point...

Created attachment 173511 [details] [review]
more build stuff for power icon

ok, here's the rest of what I needed to get this running

Please skip the tooltip for now, because the StWidget tooltip code is awful. If we decide we want a tooltip later we're going to have to fix StWidget first anyway.

The text provided by the GetDevices "status" field doesn't match the mockups, which want a time remaining (for the primary device) or a simple name (for other devices) and a percentage

(In reply to comment #25)
> >+// copied from misc/telepathy.js
> >+function makeProxyClass(iface) {
> 
> hm... i need to push on bug 610859 again

done. you can use DBus.makeProxyClass now

Created attachment 173636 [details] [review]
Status area: introduce battery & power indicator

Add an indicator for battery charge and power indication in the
system status area, using the newly exported DBus API from
gnome-power-manager.

Still has problems on the gnome-power-manager part (a battery that is
not charging is not considered "primary", summary does not match mockups),
will file a bug on that.

A primary battery is often a virtual device, e.g. two laptop batteries (or two UPS devices) squashed into one virtual battery to give some system state. If the device is not virtual, you probably want to skip the device from the GetDevices method (you can just compare the id's).

The fact that there is no primary device if there is nothing downloading is intentional. The primary device is designed to show overall system state and should not be shown if nothing is happening. This is just like g-p-m does now, in git master, with the GtkStatusIcon.

Richard.

Comment on attachment 173636 [details] [review]
Status area: introduce battery & power indicator

> Still has problems on the gnome-power-manager part (a battery that is
> not charging is not considered "primary", summary does not match mockups),
> will file a bug on that.

Well, for the summary not matching mockups, I think the answer is to not use the "summary" field. The mockups just want name, icon, and percentage.

(Oh, there's no icon for non-primary batteries though, so I guess that does require more API.)

>+  <autotools id="libcanberra" autogen-sh="configure">
>+    <branch repo="0pointer.de" module="libcanberra/libcanberra-0.25.tar.gz" version="0.25"

we've since added 0.26 (with no patch needed) to the build config, so that (and the patch, and the .gitignore modification) can go away

>+                // icon is a stringified GIcon (Gio.Icon), should be
>+                // loaded with Gio.Icon.new_for_string
>+                // work around gjs not supporting static methods
>+                // (will fail if the GIcon is not a GThemedIcon)

This is still unclear although I've figured it out now; gjs doesn't support static methods on interface types (filed now as bug 633998; it does support them on object types), so we can't call Gio.Icon.new_for_string(), and "will fail" refers to the *workaround*, not gjs.

Anyway, we should not be making assumptions about how g_icon_to_string/new_from_string work. If we can't call g_icon_new_for_string() directly, then add a wrapper in libgnome-shell.

>+        if (0 /* FIXME: bug 622450 */) {

oh, forgot we had a partial patch. let me look at that again

(In reply to comment #30)
> The fact that there is no primary device if there is nothing downloading is
> intentional. The primary device is designed to show overall system state and
> should not be shown if nothing is happening. This is just like g-p-m does now,
> in git master, with the GtkStatusIcon.

One thing is not having a battery at all, in which case it is correct to hide all UI, another is to have a charging battery. In that case I would like to know how long it will take to full charge.

Also, in g-p-m 2.30 there was a GConf key called icon-policy, in case you wanted to see the icon whatever the system state. The same key has no effect in GSettings and g-p-m 2.91 though.

(In reply to comment #31)
> (From update of attachment 173636 [details] [review])
> > Still has problems on the gnome-power-manager part (a battery that is
> > not charging is not considered "primary", summary does not match mockups),
> > will file a bug on that.
> 
> Well, for the summary not matching mockups, I think the answer is to not use
> the "summary" field. The mockups just want name, icon, and percentage.

I will change g-p-m to only export name as "summary" (we haven't got the necessary info to compute it ourselves). Changed the patch to display percentage separately and aligned.

> (Oh, there's no icon for non-primary batteries though, so I guess that does
> require more API.)

No problem, we can compute the icon using percentage and state.

Created attachment 174009 [details] [review]
Status area: introduce battery & power indicator

Add an indicator for battery charge and power indication in the
system status area, using the newly exported DBus API from
gnome-power-manager.

Comment on attachment 174009 [details] [review]
Status area: introduce battery & power indicator

>diff --git a/.gitignore b/.gitignore
>-*.patch

this is still correct, but is no longer relevant to this patch; you can just commit it separately

>+    _readPrimaryDevice: function() {
>+        this._proxy.GetPrimaryDeviceRemote(Lang.bind(this, function(device, error) {
>+            if (error) {

you need to hide deviceSep.actor in this case

>+                item._power_device_id = device_id;

does not appear to be used any more

>+    _devicesChanged: function() {
>+        this._proxy.GetRemote('Icon', Lang.bind(this, function(icon, error) {
>+            if (icon) {
>+                this._iconName = null;
>+                if (this._iconActor)
>+                    this._iconActor.destroy();
>+                this._iconActor = St.TextureCache.get_default().load_gicon_from_string (icon);
>+                this.actor.set_child(this._iconActor);
>+                this.actor.show();

No, those are SystemStatusButton's private variables. You don't get to modify them. (And they've been changed to use St.Icon since you wrote this patch anyway.)

I think what we want is a shell method to wrap g_icon_new_for_string() (Owen points out that workarounds for gjs bugs belong in Shell, not St) which you can call from here. Then a SystemStatusButton.setGIcon() method that takes a GIcon, and a "gicon" property on StIcon that causes it to use st_texture_cache_load_gicon().

>+        if (0 /* FIXME: bug 622450 */) {

"false", not "0"

>+        if (!this._restarted && /org\.freedesktop\.DBus\.Error\.(UnknownMethod|InvalidArgs)/.test(error.message)) {

that feels backwards. can you say

    error.message.match(/org\.freedesktop\.DBus\.Error\.(UnknownMethod|InvalidArgs)/)

instead?

>+            GLib.spawn_command_line_sync('killall gnome-power-manager');

We've been using "pkill" rather than "killall" because it's more portable. You want:

    GLib.spawn_command_line_sync('pkill -f "^gnome-power-manager$"')

(I think. I didn't test it. The whole killing-the-old-one-and-starting-the-new-one bit isn't working for me anyway.)

>+        this._size = 16; // keep in sync with PopupImageMenuItem

needs to be updated to use St.Icon instead of using St.TextureCache directly.

>+        this._label = new St.Label({ text: summary });
>+        this.addActor(this.label);
>+        this.addActor(new St.Label({ text: '%d%%'.format(percentage) }));
...
>+        this.addActor(this._icon);

If you haven't discussed it with the designers, you should just stick to the layout in the mockups:

    ICON Name     %age

to get the icon and name to stick together correctly even if we add switches or submenus to other items later, you'll want to put them in an St.BoxLayout together and then addActor the boxlayout, and then separately addActor the icon.

Created attachment 174546 [details] [review]
StIcon: add support for GIcon

Add a "gicon" property so that a GIcon can be used instead of an
icon name, while still getting icon recoloring from the theme.
Also include a compatibility wrapper in libshell until GJS has
support for interface static methods.

Created attachment 174547 [details] [review]
Status area: introduce battery & power indicator

Add an indicator for battery charge and power indication in the
system status area, using the newly exported DBus API from
gnome-power-manager.

Created attachment 174602 [details] [review]
Status area: introduce battery & power indicator

Add an indicator for battery charge and power indication in the
system status area, using the newly exported DBus API from
gnome-power-manager.

Comment on attachment 174546 [details] [review]
StIcon: add support for GIcon

>+        this._iconActor = new St.Icon({ icon_name: iconName, icon_type: St.IconType.SYMBOLIC, style_class: 'system-status-icon'});

put the properties on separate lines (like how it was in setIcon before)

>     setIcon: function(iconName) {
>         this._iconName = iconName;

this._iconName is unused and can just be removed (both here and in setGIcon). (It should have been removed with the StIcon-ification, but was missed.)

>diff --git a/js/ui/popupMenu.js b/js/ui/popupMenu.js

doesn't belong in this patch...

>+ * shell_util_icon_from_string:
>+ * @string: a stringified #GIcon
>+ *
>+ * Returns: (transfer full): the icon which is represented by @string

explain that it's just a wrapper, and why. Also, you should probably include the GError arg and pass that on. (When we switch to being able to use g_icon_new_for_string() directly, the caller is going to have to deal with the possibility of an exception, so they might as well have to deal with it now as well.)

>+GIcon*

space between type name and "*"

>+                               G_PARAM_STATIC_STRINGS | G_PARAM_READWRITE);

for consistency, use ST_PARAM_READWRITE (which is defined as the same thing as what you wrote)

>+GIcon*

space before "*"

>+st_icon_set_gicon (StIcon *icon, GIcon *gicon)
>+{
>+  g_return_if_fail (ST_IS_ICON (icon));
>+  g_return_if_fail (G_IS_ICON (icon));

should be (gicon) not (icon) in second line

>+  if (icon->priv->gicon) {
>+    g_object_unref (icon->priv->gicon);

indentation style. should be

    if (icon->priv->gicon)
      {
        g_object_unref (icon->priv->gicon);

>+    icon->priv->gicon = (GIcon*) g_object_ref (icon);

g_object_ref returns a gpointer explicitly so that you don't have to bother casting it


Also, setting a non-NULL gicon should clear icon_name, and setting a non-NULL icon_name should clear gicon.

Comment on attachment 174546 [details] [review]
StIcon: add support for GIcon

>+    icon->priv->gicon = (GIcon*) g_object_ref (icon);

s/icon/gicon/ !

Comment on attachment 174602 [details] [review]
Status area: introduce battery & power indicator

mostly ok, I'm going to attach a patch that you should squash to this one.

(I'm assuming the bugs in the DeviceItem code are because you weren't able to test it because you don't have any additional battery-powered devices.)

Created attachment 174622 [details] [review]
fixes

Created attachment 174628 [details] [review]
StIcon: add support for GIcon

Add a "gicon" property so that a GIcon can be used instead of an
icon name, while still getting icon recoloring from the theme.
Also include a compatibility wrapper in libshell until GJS has
support for interface static methods.

Review of attachment 174622 [details] [review]:

::: js/ui/status/power.js
@@ +102,3 @@
                 this._batteryItem.label.text = summary;
                 this._batteryItem.actor.show();
+                this._deviceSep.actor.show();

You also need to check that you have additional devices, or you end up with two consecutive separators.

Created attachment 174629 [details] [review]
Status area: introduce battery & power indicator

Add an indicator for battery charge and power indication in the
system status area, using the newly exported DBus API from
gnome-power-manager.

Comment on attachment 174628 [details] [review]
StIcon: add support for GIcon

>+  prev_icon_name = icon->priv->icon_name;
>+  if (prev_icon_name)
>+    {
>+      g_free (icon->priv->icon_name);
>+      icon->priv->icon_name = NULL;
>+      g_object_notify (G_OBJECT (icon), "icon-name");
>+    }
>+
>+  if (gicon)
>+    icon->priv->gicon = g_object_ref (gicon);
>+
>+  if (gicon != icon->priv->gicon)
>+    g_object_notify (G_OBJECT (icon), "gicon");

Two problems here: first, the check at the end is a no-op, since you just set icon->priv->gicon. You probably want to just check that at the beginning, and return without doing anything if they're equal.

Second, you're notifying on "icon-name" while the object is in an inconsistent state (neither icon-name nor gicon is set). You should set priv->gicon before clearing priv->icon_name. (Or alternatively, use g_object_freeze_notify/g_object_thaw_notify, but there's no good reason to do it that way here.)

Comment on attachment 174629 [details] [review]
Status area: introduce battery & power indicator

ok except:

>+        if (false /* FIXME: bug 622450 */) {

don't need that any more! :)

Created attachment 174634 [details] [review]
StIcon: add support for GIcon

Add a "gicon" property so that a GIcon can be used instead of an
icon name, while still getting icon recoloring from the theme.
Also include a compatibility wrapper in libshell until GJS has
support for interface static methods.

Review of attachment 174629 [details] [review]:

::: tools/build/gnome-shell.modules
@@ +233,3 @@
+  <autotools id="gnome-power-manager">
+    <branch repo="git.gnome.org" module="gnome-power-manager" />
+    <dependencies>

Missing <dep package="gnome-control-center"/>

Comment on attachment 174629 [details] [review]
Status area: introduce battery & power indicator

Committed with the suggested fix.