GNOME Bugzilla – Bug 649470
[uber-patch] port gnome-power-manager to gnome-settings-daemon
Last modified: 2011-08-22 11:51:20 UTC
Created attachment 187289 [details] [review] patch for initial comments In a post-3.0 world, it makes no sense to have gnome-power-manager be a separate process to g-s-d. The plan for some time has been to merge gnome-power-manager into gnome-settings-daemon and merge the other optional tools into gnome-utils. This patch is the result of two days work. I've spent the best part of yesterday shaking a yak and getting rid of dubious features, random settings keys and *pre-HAL* legacy code. That said, there is probably quite a lot of randomness in the code (and it's a pretty massive patch), so it's likely I'll have to shave some more. Please comment on general parts of the patch rather than do a line by line review, as this I'm sure will be the first iteration of many. I've tested this patch with todays jhbuild and seems to work correctly. When it's properly reviewed, we have to remember to change a couple of dbus interface names, one in gnome-shell (power applet) and one in gnome-control-center (brightness slider). Thanks. So, if I've not said it already, go easy on me :-) Thanks. Richard.
Review of attachment 187289 [details] [review]: A lot of the schema entries in org.gnome.settings-daemon.plugins.power.gschema.xml.in.in should be namespaced (backlight, ups, etc.). All the code to handle keyboard keys should be moved to the media-keys plugin, and use its own D-Bus interface internally. This should shave quite a bit of code already. Finally, a lot of the filenames, and suffixes have vague names (it's not possible to tell what they do unless we know the code, or read that code. ::: data/org.gnome.settings-daemon.plugins.power.gschema.xml.in.in @@ +1,2 @@ <schemalist> + <enum id="org.gnome.settings-daemon.plugins.power.IconPolicy"> That's not how we add enumerations. See gsd-enums.h. @@ +127,3 @@ + <default>50</default> + <_summary>LCD dimming amount when on battery</_summary> + <default>false</default> Amount in what? @@ +132,3 @@ + <default>1.0</default> + <_summary>LCD brightness when on AC</_summary> + <_description>If the screen should be reduced in brightness when the computer is on battery power.</_description> Remove the "possible values are..." and add ranges instead. @@ +142,3 @@ + <default>100</default> + <_summary>Keyboard backlight brightness when on AC power.</_summary> + <_description>The amount to dim the brightness of the display when on battery power. Possible values are between 0 and 100.</_description> Why do we use 0 -> 100 instead of 0.0 -> 1.0 as for the display backlight. @@ +156,3 @@ + <key name="use-time-for-policy" type="b"> + <default>true</default> + <key name="kbd-backlight-battery-reduce" type="b"> Notifications, or for the policy? Or for policy notifications, or for notifications policy? @@ +162,3 @@ + <default>true</default> + <_summary>Lock GNOME keyring on sleep</_summary> + <_description>If the keyboard backlight brightness should be reduced when the computer is on battery power</_description> Couldn't this be done through a separate plugin? Is this useful when we already have a screenlock? Can't the gnome-keyring daemon do that itself? @@ +260,3 @@ + <default>'present'</default> + <_summary>When to show the notification icon</_summary> + <_summary>If we should show the recalled battery warning for a broken battery</_summary> Isn't this redundant given gnome-shell (and the fallback session)'s designs? ::: plugins/power/.gitignore @@ +1,2 @@ +gpm-marshal.c +gpm-marshal.h This is automated (and if it isn't, it should be). Totem has code to automatically add generated files to .gitignore. See git.mk. ::: plugins/power/egg-console-kit.c @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- Can the egg-console-kit stuff be made available to the automount plugin as well? @@ +3,3 @@ + * Copyright (C) 2006-2010 Richard Hughes <richard@hughsie.com> + * + * Licensed under the GNU General Public License Version 2 Version 2+, or just remove those lines (it's explained underneath). ::: plugins/power/gpm-backlight.c @@ +51,3 @@ +#include "egg-console-kit.h" + +#define GPM_DBUS_INTERFACE_BACKLIGHT "org.gnome.SettingsDaemon.PowerManager.Backlight" Manager is a bit redundant here. ::: plugins/power/gpm-brightness.c @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- Need to rename either this file, or gpm-backlight. What they do really isn't clear (call this one gsd-xrandr-backlight, or something?) ::: plugins/power/gpm-button.c @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- Given the code in media-keys already does all this. Remove this completely, add hard-coded handlers in media-keys, and call out to the power plugin through D-Bus. Note that this is how the XRandR keys are handled now. ::: plugins/power/gpm-common.c @@ +74,3 @@ + * @levels: The number of discrete levels + * + * We have to be carefull when converting from %->discrete as precision is very careful, not carefull. @@ +97,3 @@ + * @levels: The number of discrete levels + * + * We have to be carefull when converting from discrete->%. Careful. ::: plugins/power/gpm-engine.c @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- Need to find a better name for this file. ::: plugins/power/gpm-idle.c @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- Need to make what this does clearer as well. ::: plugins/power/gpm-idletime.c @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- idle vs. idletime. What does what? ::: plugins/power/gpm-osd-dialog.c @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- This is a copy paste, and probably outdated, version of what we have in the media-keys plugin. Remove it. ::: plugins/power/gsd-media-keys-window.c @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- Again, duplicate, remove.
Created attachment 192749 [details] [review] Add the idle actions, ported from g-p-m 2nd to last patch before the migration is complete! :-) Please review, thanks.
Created attachment 193147 [details] [review] Show a status icon if running in fallback mode
Both look fine to commit.
Created attachment 193154 [details] [review] Add the backlight helper This is just the commit for the backlight helper, the connecting code that calls this will follow (likely tomorrow, other stuff to finish today). Please review, thanks.
Review of attachment 193154 [details] [review]: Might be worth mentioned that it's Linux only in the commit message. ::: configure.ac @@ +78,3 @@ ) +PKG_CHECK_MODULES(POLKIT_HELPER, namespace that, we have other helpers (and it's not helping polkit either). So BACKLIGHT_HELPER instead? And it also depends on Linux. Check for udev being present? ::: plugins/power/Makefile.am @@ +54,3 @@ + org.gnome.settings-daemon.plugins.power.policy + +# You will need a recent intltool or the patch from this bug Remove that comment, and up the intltool dependency in configure.ac @@ +58,3 @@ +@INTLTOOL_POLICY_RULE@ + + We use .in.in usually. @@ +59,3 @@ + +.in2.in: +# You will need a recent intltool or the patch from this bug Missing $(AM_V_GEN) @@ +87,3 @@ $(plugin_DATA) DISTCLEANFILES = \ Probably want those in CLEANFILES instead. ::: plugins/power/gsd-backlight-helper.c @@ +38,3 @@ + +/** + * gsd_backlight_helper_get_best_backlight: If you're not actually going to document it, remove it. @@ +139,3 @@ + * main: + **/ +gint int @@ +140,3 @@ + **/ +gint +main (gint argc, gchar *argv[]) int, char @@ +169,3 @@ + }; + + /* setup translations */ Overall, I cannot see any particular reason to translate any of the error messages, seeing as they would only be presented in debug logs, not in the UI. @@ +243,3 @@ + if (uid != 0 || euid != 0) { + /* TRANSLATORS: only able to install profiles as root */ + g_print ("%s\n", _("This program can only be used by the root user")); Again, why translate that? @@ +251,3 @@ + pkexec_uid_str = g_getenv ("PKEXEC_UID"); + if (pkexec_uid_str == NULL) { + /* TRANSLATORS: the program must never be directly run */ Why translate that?
Created attachment 193158 [details] [review] Add the backlight helper (In reply to comment #6) > Review of attachment 193154 [details] [review]: > > Might be worth mentioned that it's Linux only in the commit message. Fixed. > +PKG_CHECK_MODULES(POLKIT_HELPER, > > namespace that, we have other helpers (and it's not helping polkit either). So > BACKLIGHT_HELPER instead? Fixed. > And it also depends on Linux. Check for udev being present? I wondered that, but the new executable doesn't actually depend on udev itself. I'd be happy to wrap the BACKLIGHT_HELPER in a --enable-linux-type-thing if you want, but even on non-linux systems it'll compile fine, it just won't do anything. > ::: plugins/power/Makefile.am > @@ +54,3 @@ > + org.gnome.settings-daemon.plugins.power.policy > + > +# You will need a recent intltool or the patch from this bug > > Remove that comment, and up the intltool dependency in configure.ac Fixed, we already dep on an intltool newer than from 2007 [0.37.1]. > @@ +58,3 @@ > +@INTLTOOL_POLICY_RULE@ > + > + > > We use .in.in usually. I tried this, but the autoconf gods didn't like it. The person that contributed the .in2 file said it was the only way to get the 2nd substitution. I can dig the bug up if you like. > @@ +59,3 @@ > + > +.in2.in: > +# You will need a recent intltool or the patch from this bug > > Missing $(AM_V_GEN) Fixed. > @@ +87,3 @@ > $(plugin_DATA) > > DISTCLEANFILES = \ > > Probably want those in CLEANFILES instead. Fixed. > ::: plugins/power/gsd-backlight-helper.c > @@ +38,3 @@ > + > +/** > + * gsd_backlight_helper_get_best_backlight: > > If you're not actually going to document it, remove it. Fixed. > @@ +139,3 @@ > + * main: > + **/ > +gint > > int Fixed. > @@ +140,3 @@ > + **/ > +gint > +main (gint argc, gchar *argv[]) > > int, char Fixed. > @@ +169,3 @@ > + }; > + > + /* setup translations */ > > Overall, I cannot see any particular reason to translate any of the error > messages, seeing as they would only be presented in debug logs, not in the UI. Agreed, removed. > @@ +243,3 @@ > + if (uid != 0 || euid != 0) { > + /* TRANSLATORS: only able to install profiles as root */ > + g_print ("%s\n", _("This program can only be used by the root user")); > > Again, why translate that? Fixed. > @@ +251,3 @@ > + pkexec_uid_str = g_getenv ("PKEXEC_UID"); > + if (pkexec_uid_str == NULL) { > + /* TRANSLATORS: the program must never be directly run */ > > Why translate that? Fixed. New patch attached. Thanks for the speedy review.
(In reply to comment #7) > > And it also depends on Linux. Check for udev being present? > > I wondered that, but the new executable doesn't actually depend on udev itself. > I'd be happy to wrap the BACKLIGHT_HELPER in a --enable-linux-type-thing if you > want, but even on non-linux systems it'll compile fine, it just won't do > anything. In that case, make sure never to call the helper on non-Linux platforms (in your upcoming patches), and add a g_assert() in the helper code for non-Linux platforms. > > @@ +58,3 @@ > > +@INTLTOOL_POLICY_RULE@ > > + > > + > > > > We use .in.in usually. > > I tried this, but the autoconf gods didn't like it. The person that contributed > the .in2 file said it was the only way to get the 2nd substitution. I can dig > the bug up if you like. It will work fine if you don't use the ".in2.in:" rule, and use explicit names instead. That's how it's done for desktop files for example.
Created attachment 193163 [details] [review] Add the backlight helper (In reply to comment #8) > In that case, make sure never to call the helper on non-Linux platforms (in > your upcoming patches), and add a g_assert() in the helper code for non-Linux > platforms. Will do. I think a g_critical might be more polite than asserting the g-s-d process tho. > It will work fine if you don't use the ".in2.in:" rule, and use explicit names > instead. That's how it's done for desktop files for example. Ahh yes, of course. The expansion was catching me out. I've fixed this in the attached patch to be .in.in like the others. Thanks.
Created attachment 193191 [details] [review] Fall back to the backlight helper if xbacklight is not availble The patch is untested, but should work. I'd appreciate a quick review before testing and polishing to make sure this is what you had in mind. Thanks!
(In reply to comment #9) > Created an attachment (id=193163) [details] [review] > Add the backlight helper > > (In reply to comment #8) > > In that case, make sure never to call the helper on non-Linux platforms (in > > your upcoming patches), and add a g_assert() in the helper code for non-Linux > > platforms. > > Will do. I think a g_critical might be more polite than asserting the g-s-d > process tho. Adding a g_assert() in the helper, not to g-s-d. In gnome-settings-daemon, just make sure the helper isn't called.
Review of attachment 193163 [details] [review]: ::: plugins/power/Makefile.am @@ +81,3 @@ + $(plugin_DATA) \ + org.gnome.settings-daemon.plugins.power.policy \ + org.gnome.settings-daemon.plugins.power.policy.in You're missing a POTFILES.skip for org.gnome.settings-daemon.plugins.power.policy.in, otherwise make check will fail. ::: plugins/power/gsd-backlight-helper.c @@ +161,3 @@ + + /* setup type system */ + g_type_init (); add a g_assert_not_reached() here for non-Linux platforms. ::: plugins/power/org.gnome.settings-daemon.plugins.power.policy.in.in @@ +20,3 @@ + --> + <_description>Modify the laptop brightness</_description> + <_message>Authentication is required to modify the laptop brightness</_message> Given that there's no interactive version of this, why do we want to translate this?
Review of attachment 193191 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +2195,3 @@ +/** + * backlight_helper_get_value: Incomplete documentation. @@ +2205,3 @@ + guint64 value = -1; + gchar *command = NULL; +{ There should be code here to not do anything on non-Linux platforms.
Created attachment 193264 [details] [review] Add the backlight helper (In reply to comment #12) > You're missing a POTFILES.skip for > org.gnome.settings-daemon.plugins.power.policy.in, otherwise make check will > fail. Fixed. > ::: plugins/power/gsd-backlight-helper.c > @@ +161,3 @@ > + > + /* setup type system */ > + g_type_init (); > > add a g_assert_not_reached() here for non-Linux platforms. Fixed. > ::: plugins/power/org.gnome.settings-daemon.plugins.power.policy.in.in > @@ +20,3 @@ > + --> > + <_description>Modify the laptop brightness</_description> > + <_message>Authentication is required to modify the laptop > brightness</_message> > > Given that there's no interactive version of this, why do we want to translate > this? Because the dialog is going to be shown if the administrator changes the PolicyKit authentication required, e.g. from "yes" to "auth" -- although I don't see this as common I think it's good form to translate the authentication strings. That said, if you don't like it I can easily rip it out.
Created attachment 193265 [details] [review] Fall back to the backlight helper if xbacklight is not availble (In reply to comment #13) > Review of attachment 193191 [details] [review]: > > ::: plugins/power/gsd-power-manager.c > @@ +2195,3 @@ > > +/** > + * backlight_helper_get_value: > > Incomplete documentation. Fixed. > @@ +2205,3 @@ > + guint64 value = -1; > + gchar *command = NULL; > +{ > > There should be code here to not do anything on non-Linux platforms. Fixed.
Both patches are fine by me. Now you just need to look into those crashers ;)
(In reply to comment #16) > Both patches are fine by me. Now you just need to look into those crashers ;) Okay, committed both (with a tiny fix in the first patch) and they both seem to work after casual testing. I'll run clang on all the new code, and see what pops up.
Is that fixed now? Can we get onto the power button behaviour now?