Re: [gpm] gnome-power-manager and DPMS

[William Jon McCann <mccann jhu edu>]

Hey,

Not sure this kind of workaround is good in the long run but anyway.  A 
few problems with this patch...


> Index: src/gpm-manager.c
> ===================================================================
> RCS file: /cvs/gnome/gnome-power-manager/src/gpm-manager.c,v
> retrieving revision 1.161
> diff -u -u -r1.161 gpm-manager.c
> --- src/gpm-manager.c	20 Jul 2006 21:23:58 -0000	1.161
> +++ src/gpm-manager.c	30 Jul 2006 12:48:28 -0000
> @@ -528,25 +528,33 @@
>  static void
>  sync_dpms_policy (GpmManager *manager)
>  {
> -	GError  *error;
> -	gboolean res;
> -	gboolean on_ac;
> -	guint    standby;
> -	guint    suspend;
> -	guint    off;
> +	GError     *error;
> +	gboolean    res;
> +	gboolean    on_ac;
> +	guint       timeout;
> +	guint       standby;
> +	guint       suspend;
> +	guint       off;
> +	const char *dpms_method;

gconf_client_get_string doesn't return a const char *.


>  	error = NULL;
>  
>  	gpm_power_get_on_ac (manager->priv->power, &on_ac, NULL);
>  
>  	if (on_ac) {
> -		standby = gconf_client_get_int (manager->priv->gconf_client,
> +		timeout = gconf_client_get_int (manager->priv->gconf_client,
>  						GPM_PREF_AC_SLEEP_DISPLAY,
>  						&error);
> +		dpms_method = gconf_client_get_string (manager->priv->gconf_client,
> +						       GPM_PREF_AC_DPMS_METHOD,
> +						       &error);


You are using the error variable twice without checking it, freeing it 
and reinitializing it.

>  	} else {
> -		standby = gconf_client_get_int (manager->priv->gconf_client,
> +		timeout = gconf_client_get_int (manager->priv->gconf_client,
>  						GPM_PREF_BATTERY_SLEEP_DISPLAY,
>  						&error);
> +		dpms_method = gconf_client_get_string (manager->priv->gconf_client,
> +						       GPM_PREF_BATTERY_DPMS_METHOD,
> +						       &error);
>  	}

Same here.


>  
>  	if (error) {
> @@ -556,14 +564,38 @@
>  	}
>  
>  	/* old policy was in seconds, warn the user if too small */
> -	if (standby < 60) {
> +	if (timeout < 60) {
>  		gpm_warning ("standby timeout is invalid, please re-configure");
>  		return;
>  	}
>  
> -	/* try to make up some reasonable numbers */
> -	suspend = standby;
> -	off     = standby * 2;
> +	/* If we have no dpms_method, possible due to a schema problem */
> +	if (dpms_method == NULL) {
> +		dpms_method = "default";
> +	}

You'll have to do a g_strdup() here if you want to do it this way.


> +
> +	/* Some monitors do not support certain suspend states, so we have to
> +	 * provide a way to only use the one that works. */
> +	if (strcmp (dpms_method, "standby") == 0) {
> +		standby = timeout;
> +		suspend = 0;
> +		off     = 0;
> +	} else if (strcmp (dpms_method, "suspend") == 0) {
> +		standby = 0;
> +		suspend = timeout;
> +		off     = 0;
> +	} else if (strcmp (dpms_method, "off") == 0) {
> +		standby = 0;
> +		suspend = 0;
> +		off     = timeout;
> +	} else {
> +		/* suspend after one timeout, turn off after another */
> +		standby = timeout;
> +		suspend = timeout;
> +		off     = timeout * 2;
> +	}


You should handle the dpms_method == "default" case explicitly and use 
the else case to give a warning or error.  Also you might want to handle 
default first if it is the most commonly occurring case.

You also need to free dpms_method.

Jon