GNOME Bugzilla – Bug 595154
gnome-session does not lock the screen on suspend or hibernate anymore
Last modified: 2009-09-14 15:37:38 UTC
Created attachment 143144 [details] [review] patch that fixes the issue When I converted the code from using gnome-power-manager to DeviceKit-power, I forgot that gnome-power-manager used to manually lock gnome-screensaver on suspend and hibernate, if the use had the lock option set in gnome-screensaver. I've attached a patch to fix this. Please review! Thanks.
So I don't think gnome-session should do that. Shouldn't DK-power send some signal that gnome-screensaver would listen to?
The DK-p stuff is async, and the method will return as soon as the suspend is started. It would be very racey to do this "bottom-up" the stack, as we need to ensure the screensaver is locked (as in actually showing on the screen) before we start the suspend to ensure we don't actually lock on /resume/. This is why it's run as sync, not async in my patch. Doing this "bottom-up" would require us to have the session register with DK-p, and then for a little sync dance to happen every time we suspend. Whilist you could argue that's architectually more valid (we've been there, it wasn't pretty) it's not the sort of thing we can discuss a day before hard code freeze :-) Richard.
(In reply to comment #2) > Doing this "bottom-up" would require us to have the session register with DK-p, > and then for a little sync dance to happen every time we suspend. Whilist you > could argue that's architectually more valid (we've been there, it wasn't > pretty) it's not the sort of thing we can discuss a day before hard code freeze > :-) Oh, I'm not talking about fixing this correctly right now -- timing is indeed bad. But in the long term, I think this should be kept out of gnome-session. I mean, this could even be done in the DK-power library -- this would make more sense than hard-coding this in gnome-session.
Comment on attachment 143144 [details] [review] patch that fixes the issue >+static gboolean >+sleep_lock_is_enabled (GsmManager *manager) >+{ >+ GError *error; >+ gboolean enable_lock; >+ >+ error = NULL; >+ enable_lock = gconf_client_get_bool (manager->priv->gconf_client, >+ KEY_SLEEP_LOCK, >+ &error); >+ >+ if (error) { >+ g_warning ("Error retrieving configuration key '%s': %s", >+ KEY_SLEEP_LOCK, >+ error->message); >+ g_error_free (error); >+ >+ /* If we fail to query gconf key, disable auto save */ >+ enable_lock = FALSE; >+ } It should default to TRUE, IMHO. (And what's this comment about auto save? ;-)) >+ return enable_lock; >+} >+ > static void > manager_attempt_hibernate (GsmManager *manager) > { >@@ -936,9 +963,17 @@ manager_attempt_hibernate (GsmManager *manager) > NULL); > > if (can_hibernate) { >+ ret = sleep_lock_is_enabled (manager); >+ if (ret) { >+ error = NULL; >+ ret = g_spawn_command_line_sync ("gnome-screensaver-command --lock", NULL, NULL, NULL, &error); >+ if (!ret) { >+ g_warning ("Couldn't lock screen: %s", error->message); >+ g_error_free (error); >+ } >+ } Please create a function for this code so that we don't have it twice.
Created attachment 143152 [details] [review] new patch New patch, thanks for the speedy review.
Comment on attachment 143152 [details] [review] new patch >+/* FIXME: after the string freeze, move this key to gnome-session */ Oh, didn't see this at first? Why? I would remove that :-) Then I have only minor coding style nitpicks (see below), please fix them and commit, thanks! >+ error = NULL; >+ enable_lock = gconf_client_get_bool (manager->priv->gconf_client, >+ KEY_SLEEP_LOCK, >+ &error); I'd put the last two lines on one line. >+ if (error) { >+ g_warning ("Error retrieving configuration key '%s': %s", >+ KEY_SLEEP_LOCK, >+ error->message); Same here. >+static void >+manager_perhaps_lock (GsmManager *manager) >+{ >+ GError *error; >+ gboolean ret; >+ >+ /* only lock if gnome-screensaver is set to lock */ >+ ret = sleep_lock_is_enabled (manager); >+ if (ret) { if (!sleep_lock_is_enabled (manager)) { return; } (I prefer to avoid indentations)
Changed and committed. Thanks.