GNOME Bugzilla – Bug 653950
Add the capability to get and set the DPMS mode of the screen
Last modified: 2011-07-13 10:40:39 UTC
Created attachment 191232 [details] [review] patch for review This is presently per-screen, but future versions of xrandr will allow us to set the DPMS mode of each output. This functionality is required by the power plugin in gnome-settings-daemon.
Review of attachment 191232 [details] [review]: ::: libgnome-desktop/gnome-rr.c @@ +1176,3 @@ + **/ +gboolean +gnome_rr_screen_get_dpms_mode (GnomeRRScreen *screen, Since the "mode" argument *is* the only thing you get out of this function (aside from errors), we should only allow mode!=NULL --- it's not one of those "get this value but not this other one" functions. Can you please g_return_val_if_fail (mode != NULL)? @@ +1210,3 @@ + GNOME_RR_ERROR_UNKNOWN, + "DPMS is not enabled"); + goto out; I don't know the full semantics of DPMSInfo(), but it would seem that enabled == FALSE is not an error, just a question of how the display is configured. I'd prefer to have a GNOME_RR_DPMS_DISABLED value and make this not an error: with the way you have it right now, there's no way to distinguish "couldn't get DPMS info" from "DPMS is disabled" from the caller. @@ +1252,3 @@ + + /* set, if the new mode is different */ + ret = gnome_rr_screen_get_dpms_mode (screen, ¤t_mode, error); This seems harmless, but is it superfluous? Won't the X server simply ignore the request if the value is the same? @@ +1271,3 @@ + state = DPMSModeOff; + break; + default: ... and if you do add a GNOME_RR_DPMS_DISABLED as suggested above, please make this set() function not accept that value.
Created attachment 191239 [details] [review] patch for review (In reply to comment #1) > Can you please g_return_val_if_fail (mode != NULL)? Done. > I'd prefer to have a GNOME_RR_DPMS_DISABLED value and make this not an error Done. > @@ +1252,3 @@ > + > + /* set, if the new mode is different */ > + ret = gnome_rr_screen_get_dpms_mode (screen, ¤t_mode, error); > > This seems harmless, but is it superfluous? Won't the X server simply ignore > the request if the value is the same? New xservers yes, old xservers no. Also, but getting the value before setting we get all the enabled etc checks for free. > @@ +1271,3 @@ > + state = DPMSModeOff; > + break; > + default: > > ... and if you do add a GNOME_RR_DPMS_DISABLED as suggested above, please make > this set() function not accept that value. It already hits a assert in that case. Thanks for the super-speedy review. New patch attached.
Ping? This bug is blocking about 6 patches from gnome-settings-daemon. Thanks.
Review of attachment 191239 [details] [review]: Small thing that we might want to fix. Else, I guess if your patch fixes all issues Federico had, you can likely go ahead. ::: libgnome-desktop/gnome-rr.c @@ +1191,3 @@ + GNOME_RR_ERROR, + GNOME_RR_ERROR_NO_DPMS_EXTENSION, + "Display is not DPMS capable"); I think we'd want to translate those error strings, wouldn't we?
(In reply to comment #4) > I think we'd want to translate those error strings, wouldn't we? http://mail.gnome.org/archives/desktop-devel-list/2011-March/msg00069.html Federico, it's your call. At the moment libgnome-desktop is a bit of mixture of both. Anyway, patch committed, thanks Vincent.