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 720198 - Fix battery key action
Fix battery key action
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: media-keys
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
Depends on:
Blocks:
 
 
Reported: 2013-12-10 15:18 UTC by Carlos Garnacho
Modified: 2013-12-16 13:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
power: Add DBus method to show the power level OSD (4.29 KB, patch)
2013-12-10 15:20 UTC, Carlos Garnacho
rejected Details | Review
media-keys: Fix battery key handling (3.92 KB, patch)
2013-12-10 15:21 UTC, Carlos Garnacho
needs-work Details | Review
media-keys: Fix battery key handling (4.42 KB, patch)
2013-12-12 15:28 UTC, Carlos Garnacho
needs-work Details | Review
media-keys: Fix battery key handling (4.60 KB, patch)
2013-12-13 11:14 UTC, Carlos Garnacho
committed Details | Review

Description Carlos Garnacho 2013-12-10 15:18:53 UTC
Since the cleanup performed in bug #709736, the battery key action is broken, since it relied on the power plugin dbus interface that was removed too. In order to avoid adding back that much code, I suggest instead having an OSD interface with a Show method on the power plugin, so the media-keys plugin just triggers that. I'm attaching a couple of patches that do that
Comment 1 Carlos Garnacho 2013-12-10 15:20:33 UTC
Created attachment 263920 [details] [review]
power: Add DBus method to show the power level OSD

This DBus call will indirectly request gnome-shell to show an OSD
containing information about battery status.
Comment 2 Carlos Garnacho 2013-12-10 15:21:09 UTC
Created attachment 263921 [details] [review]
media-keys: Fix battery key handling

Since commit 5bbe63ff22, the power plugin DBus interface doesn't have the
properties that media-keys plugin expects here. So use the new Show() call
on the .Power.OSD interface to resuscitate this functionality.
Comment 3 Bastien Nocera 2013-12-10 15:29:15 UTC
Review of attachment 263921 [details] [review]:

I'd rather you used the "display device" in UPower 0.99.x instead. It shouldn't add much more code, and removes the need for power plugin changes.
Comment 4 Bastien Nocera 2013-12-10 15:29:58 UTC
Review of attachment 263920 [details] [review]:

As per review for the media-keys patch, marking as rejected.
Comment 5 Carlos Garnacho 2013-12-12 15:28:55 UTC
Created attachment 264086 [details] [review]
media-keys: Fix battery key handling

Since commit 5bbe63ff22, the power plugin DBus interface doesn't have the
properties that media-keys plugin expects here. So keep an UPower display
device to fetch icon/percentage for the OSD.
Comment 6 Bastien Nocera 2013-12-12 16:24:01 UTC
Review of attachment 264086 [details] [review]:

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +1929,1 @@
+        if (manager->priv->composite_device == NULL)

There should always be a composite device.

g_return_if_fail (); would be appropriate here.

@@ +1930,3 @@
                 return;
 
+        g_debug ("showing battery level OSD");

You'll need to get the type as well. If it's not UPS or battery, you shouldn't show anything because there's no battery to show state for.
Comment 7 Carlos Garnacho 2013-12-13 11:14:54 UTC
Created attachment 264134 [details] [review]
media-keys: Fix battery key handling

Since commit 5bbe63ff22, the power plugin DBus interface doesn't have the
properties that media-keys plugin expects here. So keep an UPower display
device to fetch icon/percentage for the OSD.
Comment 8 Bastien Nocera 2013-12-13 12:08:35 UTC
Review of attachment 264134 [details] [review]:

Looks good!
Comment 9 Carlos Garnacho 2013-12-16 13:22:55 UTC
Attachment 264134 [details] pushed as 79ce853 - media-keys: Fix battery key handling