Review Request 109693: Try to complete locking screen before suspend

[Oliver Henshaw]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/109693/
-----------------------------------------------------------

(Updated Sept. 2, 2015, 1:35 p.m.)


Status
------

This change has been discarded.


Review request for Plasma and Solid.


Bugs: 313123
    http://bugs.kde.org/show_bug.cgi?id=313123


Repository: kde-workspace


Description
-------

Here's an attempt to get the screen lock to fully activate before resume starts.


This is a problem on systems that use systemd/logind to suspend as it suspends immediately, so that there's unlikely to be time for the lock process to hide the screen before the system suspends. There shouldn't be a problem on systems that use upower, as it waits a second before suspending - there's no guarantee that the screen locks in time, but it should in practice. But there are reports of problems with upower (on ubuntu, at least) - this needs further investigation to see if there's something else going on.

It's also not clear why the lock process waits a further ten or so seconds after resume before hiding the screen.


* Delay emitting org.freedesktop.ScreenSaver.ActiveChanged and replying to org.freedesktop.ScreenSaver.Lock until the lock process is ready. We can't have the ksmserver blocking in the .Lock call as this blocks the lock process (as QApplication constructs a QSessionManager which attempts to communicate with ksmserver over XSMP) so delay replying and complete the transaction when the lock process signal its readiness.

- The qml lockscreen still blocks on powerdevil as it queries supported suspend methods using the synchronous Solid::PowerManagement API. So disable this for now. This is harmless for the plasma desktop but does affect the plasma active qml locker.

- The plasma overlay locker seems to be broken. I think this is an unrelated breakage on my test system but can't be sure. It should be fine in theory, but might have a problem if there are any power-management plasmoids on it.


Other approaches to breaking the powerdevil <-> lock process deadlock are:

* Have SuspendSession::lockScreenAndWait() call face.call(QDBus::BlockWithGui, "Lock") to return to the event loop. 
- But does this mean that the entire call-chain to this point would need to be re-entrant with respect to the event loop?
* Make powerdevil Actions into transactions that are queued and signal their completion (or failure)
- Tricky and fairly wide-ranging. And probably not suitable for 4.10.
* Have the kscreenlocker_greet lock process use asynchronous dbus calls to query powerdevil rather than the synchronous API. I'm not sure if there's already some prototype asynchronous API (for libsolid2) to cannibalise for this.
* Have ksmserver or the lock process take a delay lock -  http://www.freedesktop.org/wiki/Software/systemd/inhibit - and return from org.freedesktop.ScreenSaver.Lock as soon as the lock process is running, as we do now.
- This would solve the problem on systems that use logind to suspend, but do nothing on systems that suspend with upower. As mentioned above, it's unclear what the cause of the problem is on upower systems.


The patch series is in my clone at kde:clones/kde-workspace/oliverhenshaw/kde-workspace in the branch review/screenlocker-waitforlock and comprises:


[1/5] Represent lock state with an enum


[2/5] Emit activeChanged() only when lock process ready

* synchronously call desktopResized in the screen locker app
  (this causes the lock process windows to be created)
* signal that the lock process is ready by calling
  org.kde.screensaver.saverLockReady()
* introduce a Waiting LockState
* finish locking in lockProcessReady() when receive notification over
  dbus - start locked timer and emit locked() here rather than earlier

This is based on 62c2c398611b2e6ef6e1e38ed79bc9540fc02ec9 and the old
screensaver implementation (from 4.9 and earlier). But delaying the
lock() qt signal, and thus delaying the activeChanged() dbus signal,
until the lock prcess is ready is new behaviour.


[3/5] Delay reply from dbus calls until screen is locked

This prevents e.g. suspending before the screen is properly locked.

Based on the old screensaver implementation (from 4.9 and earlier).


[4/5] Print when lock process starts and when ready

Useful for timing slow lock process creation


[5/5] Skip blocking Solid::PowerManagement call

Solid::PowerManagement API calls powerdevil synchronously, causing
deadlock until dbus timeout.


Diffs
-----

  ksmserver/screenlocker/greeter/CMakeLists.txt 11905152321377aa3135424fa2e09bb991878da5 
  ksmserver/screenlocker/greeter/greeterapp.h f332bfc5c432b5b48b568c815904a12b4a74bd7a 
  ksmserver/screenlocker/greeter/greeterapp.cpp b70c9d6c005aa66d2f85a42ef4f1dcb04ea44667 
  ksmserver/screenlocker/greeter/main.cpp 180d64d58b069e2d5b8362fb43c63ec2e834fadb 
  ksmserver/screenlocker/interface.h 018464cda6c1ea9511cd4553e615bee62a66949a 
  ksmserver/screenlocker/interface.cpp 32acad1b8afc8f15403d9e7b92dd49c51d355bdc 
  ksmserver/screenlocker/ksldapp.h 3047c00558d664ffac864582a4cb2c4f725a3cf4 
  ksmserver/screenlocker/ksldapp.cpp 7f2e6715f0c12baa1586e41fa8f767707c9f83c5 
  plasma/screensaver/shell/CMakeLists.txt 5c52da691acf24422efc420be9bdc71eb716b268 
  plasma/screensaver/shell/main.cpp be151aa25dbff449d2e777ace9b43cac3de12f6d 

Diff: https://git.reviewboard.kde.org/r/109693/diff/


Testing
-------

Not enough


Thanks,

Oliver Henshaw

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20150902/6291326d/attachment-0001.html>