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 595154 - gnome-session does not lock the screen on suspend or hibernate anymore
gnome-session does not lock the screen on suspend or hibernate anymore
Status: RESOLVED FIXED
Product: gnome-session
Classification: Core
Component: gnome-session
git master
Other Linux
: Normal major
: ---
Assigned To: Richard Hughes
Session Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-09-14 11:21 UTC by Richard Hughes
Modified: 2009-09-14 15:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch that fixes the issue (3.57 KB, patch)
2009-09-14 11:21 UTC, Richard Hughes
needs-work Details | Review
new patch (3.43 KB, patch)
2009-09-14 13:12 UTC, Richard Hughes
accepted-commit_now Details | Review

Description Richard Hughes 2009-09-14 11:21:57 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.
Comment 1 Vincent Untz 2009-09-14 11:31:16 UTC
So I don't think gnome-session should do that. Shouldn't DK-power send some signal that gnome-screensaver would listen to?
Comment 2 Richard Hughes 2009-09-14 11:47:37 UTC
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.
Comment 3 Vincent Untz 2009-09-14 12:11:22 UTC
(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 4 Vincent Untz 2009-09-14 12:13:11 UTC
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.
Comment 5 Richard Hughes 2009-09-14 13:12:49 UTC
Created attachment 143152 [details] [review]
new patch

New patch, thanks for the speedy review.
Comment 6 Vincent Untz 2009-09-14 14:35:09 UTC
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)
Comment 7 Richard Hughes 2009-09-14 15:37:38 UTC
Changed and committed. Thanks.