Created attachment 226837 [details] [review]
loginManager: Add support for suspend()

Logind provides a Suspend method, which we should use instead of
the UPower API when available. Expose this in loginManager, using
the UPower API for the ConsoleKit implementation.

Created attachment 226838 [details] [review]
powerMenu: Use LoginManager for suspend

Created attachment 226839 [details] [review]
userMenu: Use LoginManager for suspend

Created attachment 226860 [details] [review]
loginManager: Add support for suspend()

Fix a couple of issues; I didn't test extensively, but in a quick test suspend works for both systemd and the CK.

Review of attachment 226860 [details] [review]:

Looks good

Review of attachment 226839 [details] [review]:

++

Review of attachment 226838 [details] [review]:

++

As per bug 685562 comment 1, gnome-shell should be taking a delay inhibitor with logind and release it when it's finished locking the screen up.

Created attachment 227058 [details] [review]
loginManager: Add inhibitor API

If screen locking is enabled, the screen shield should drop down
on suspend. Currently this is achieved by either explicitly locking
the screen (when selecting "Suspend" from the user menu) or by
relying on g-s-d delaying the suspension enough time for the shield
to get into place (lid close, power button).
Systemd inhibitors offer a safer way to ensure that the screen is
locked before going to sleep, so add a small abstraction for them
in the loginManager - the ConsoleKit implementation will only
work when suspending explicitly from the shell, and thus does
not offer any improvement over the current situation.

Created attachment 227059 [details] [review]
screenShield: Use inhibitors to lock screen on suspend

If the screen lock is enabled, lock the screen before suspension.
When using systemd, this will cover both explicitly suspending from
the user menu and suspension initiated by g-s-d (lid close, power
button).

Created attachment 227060 [details] [review]
userMenu: Remove explicit screen lock on suspend

This is now handled by the screen shield itself.

Attachment 226838 [details] pushed as 9904434 - powerMenu: Use LoginManager for suspend
Attachment 226839 [details] pushed as fed007e - userMenu: Use LoginManager for suspend
Attachment 226860 [details] pushed as 1f183b8 - loginManager: Add support for suspend()

Review of attachment 227058 [details] [review]:

This looks very neat overall, I only have one doubt although it might stem from some misunderstanding on my part.

::: js/misc/loginManager.js
@@ +324,3 @@
+        if (index > -1)
+            this._inhibitors.splice(index, 1);
+        if (this._inhibitors.length == 0)

Don't you want to call suspend() here so that prepare-for-sleep gets emitted?

Review of attachment 227058 [details] [review]:

::: js/misc/loginManager.js
@@ +324,3 @@
+        if (index > -1)
+            this._inhibitors.splice(index, 1);
+        if (this._inhibitors.length == 0)

Actually, I don't think we want to do this here, do we?

let inhibitor = loginManager.inhibit();
inhibitor.release();
// machine suspends ?

The same goes for the systemd case I think, do we want to release the systemd inhibitor when calling code releases all inhibitors?

(In reply to comment #13)
> Review of attachment 227058 [details] [review]:
> 
> This looks very neat overall, I only have one doubt although it might stem from
> some misunderstanding on my part.
> 
> ::: js/misc/loginManager.js
> @@ +324,3 @@
> +        if (index > -1)
> +            this._inhibitors.splice(index, 1);
> +        if (this._inhibitors.length == 0)
> 
> Don't you want to call suspend() here so that prepare-for-sleep gets emitted?

No, we already did.

The good code path (systemd) works as follows:

 1. systemd emits PrepareForSleep on suspend
    (either us calling Suspend(), or lid close/power button/whatever)

 2. we emit prepare-for-sleep, to allow shell components to allow shell
    components to get stuff done before suspend by taking an inhibitor

 3. when the last inhibitor is released, we release the systemd inhibitor,
    so systemd goes ahead and suspends


Now the problem with the fallback path is that ConsoleKit doesn't have anything like PrepareForSleep/inhibit(). We still pretend to support the same API, but in reality we only do that for the only case where we know that we are about to suspend - namely, when loginManager.suspend() is called. So the above becomes:

 1.+2. in suspend(), instead of actually suspending we emit prepare-for-sleep,
    to allow shell components yadda yadda ...

 3. when the last inhibitor is released, we do the actual UPower call to suspend


So calling suspend() in 3. would create a loop by going straight back to 1.+2. :-)

On a side note, I've noticed in testing that prepare-for-sleep may end up being called repeatedly. Currently I'm addressing that by guarding against taking multiple inhibitors in the screenShield code (only in my local tree so far), but I'm considering doing something fancier by requiring components to pass an identifier to inhibit() and return an existing inhibitor if it exists.

Should we get this landed for 3.7.2 ?

Ping.

Can we get this landed ?

Created attachment 232836 [details] [review]
update for the last patch

Unfortunately, my testing seems to indicate that the dropping of the system inhibitor doesn't work. Not sure whats wrong there. But after I hit Fn-F4, I see the shield come down, and then I have to wait for logind's max. delay timeout before the suspend happens. running systemd-inhibit --list while waiting confirms that that gnome-shell inhibitor is still in place. Despite releaseSystemInhibitor being called.

Review of attachment 227058 [details] [review]:

::: js/misc/loginManager.js
@@ +186,3 @@
+        inhibitor.connect('released', Lang.bind(this, this._inhibitorReleased));
+        this._inhibitors.push(inhibitor);
+

No newline.

@@ +199,3 @@
+
+    _prepareForSleep: function(proxy, sender, [active]) {
+        if (active) {

What does "active" determine? Whether our inhibitor is active?

So, when we take the "else" path, ensureInhibitor is called, which sets up an inhibitor, and PrepareForSleep is re-emitted with "active" being true? Did I get that right?

@@ +220,3 @@
+                    this._systemdInhibitor = new Gio.UnixInputStream({ fd: fd });
+                } catch(e) {
+                    log(e);

logError, otherwise we'll fail with a TypeError.

@@ +316,3 @@
+        inhibitor.connect('released', Lang.bind(this, this._inhibitorReleased));
+        this._inhibitors.push(inhibitor);
+

No newline.

Review of attachment 227059 [details] [review]:

::: js/ui/screenShield.js
@@ +423,3 @@
+        this._loginManager.connect('prepare-for-sleep', Lang.bind(this,
+            function() {
+                if (!this._settings.get_boolean(LOCK_ENABLED_KEY))

Not a giant fan of the inline function, would prefer if we had a this._prepareForSleep.

Review of attachment 227060 [details] [review]:

OK. Feel free to update whatever patch you have to. This review goes to both of them.

(In reply to comment #20)
> Review of attachment 227058 [details] [review]:
> @@ +199,3 @@
> +
> +    _prepareForSleep: function(proxy, sender, [active]) {
> +        if (active) {
> 
> What does "active" determine? Whether our inhibitor is active?
> 
> So, when we take the "else" path, ensureInhibitor is called, which sets up an
> inhibitor, and PrepareForSleep is re-emitted with "active" being true? Did I
> get that right?

The gsd power plugin code will shed some light:
        g_variant_get (parameters, "(b)", &is_about_to_suspend);
        if (is_about_to_suspend) {
                handle_suspend_actions (manager);
        } else {
                handle_resume_actions (manager);
        }

is_about_to_suspend is a clearer name.

Created attachment 235254 [details] [review]
loginManager: Add inhibitor API

- Update for review comments
 - Fix  problem with releasing inhibitor

Created attachment 235255 [details] [review]
screenShield: Use inhibitors to lock screen on suspend

Don't use an anonymous function for the 'prepare-for-sleep' handler.

Review of attachment 235254 [details] [review]:

::: js/misc/loginManager.js
@@ +192,3 @@
+            if (this._inhibitors.length == 0)
+                this._releaseSystemdInhibitor();
+        } else /* coming back from suspend */ {

The spec only says that preparation time is over. It says nothing whether it is emitted before or after the actual suspend, only that it "might be emitted after".

Can I some clarification on the state machine, and an update to the docs if it's a guarantee?

@@ +199,3 @@
+    _ensureInhibitor: function() {
+        let inVariant = GLib.Variant.new('(ssss)',
+                                         ['sleep', 'gnome-shell',

who should be "GNOME Shell"

@@ +200,3 @@
+        let inVariant = GLib.Variant.new('(ssss)',
+                                         ['sleep', 'gnome-shell',
+                                         'screen shield', 'delay']);

why should be "Locking the screen", except that's not the right place for this sort of stuff. I think we should take multiple systemd inhibitors so we can change the "why" strings up. I assume the end session dialog will show those instead soon.

Also, I think it should be translated, too.

Review of attachment 235255 [details] [review]:

OK.

(In reply to comment #26)
> The spec only says that preparation time is over. It says nothing whether it is
> emitted before or after the actual suspend, only that it "might be emitted
> after".
> 
> Can I some clarification on the state machine, and an update to the docs if
> it's a guarantee?

It doesn't really matter I think, what matters is this:
"It is usually the signal on which applications request a new delay lock in order to be synchronously notified about the next suspend/shutdown."

So what we do (and other users like g-s-d):

 - take an inhibitor on initialization (to indicate that we want to delay
   suspend)
 - do what we need to do when receiving PrepareForSleep(true) and release the
   inhibitor when done (the PrepareForSleep signal is guaranteed to be emitted,
   as there's at least one inhibitor - ours)
 - take a new inhibitor when receiving PrepareForSleep(false) to be notified
   about the next suspend

Created attachment 235257 [details] [review]
loginManager: Add inhibitor API

Update patch to make the abstraction as thin as possible - LoginManager.inhibit() will now (asynchronously) return an InputStream that consumers are supposed to close to release the inhibitor; the 'prepare-for-sleep' signal has been changed to match the logind one as well.

Created attachment 235258 [details] [review]
screenShield: Use inhibitors to lock screen on suspend

Update to the new inhibitor API

Review of attachment 235258 [details] [review]:

OK.

::: js/ui/screenShield.js
@@ +548,3 @@
         this._becameActiveId = 0;
         this._lockTimeoutId = 0;
+        this._suspendInhibitor = null;

Seems to be unused? Did you have a rename in the works?

Review of attachment 235257 [details] [review]:

OK.

(In reply to comment #31)
>          this._lockTimeoutId = 0;
> +        this._suspendInhibitor = null;
> 
> Seems to be unused? Did you have a rename in the works?

No, it's a left-over from the previous patch version. Removed.


Attachment 235257 [details] pushed as 8747c0c - loginManager: Add inhibitor API
Attachment 235258 [details] pushed as 6bcad45 - screenShield: Use inhibitors to lock screen on suspend

Attachment 227060 [details] pushed as 62ce907 - userMenu: Remove explicit screen lock on suspend

*** Bug 695899 has been marked as a duplicate of this bug. ***