<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/109693/">https://git.reviewboard.kde.org/r/109693/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#e0e0e0" width="100%" cellpadding="12" style="border: 1px gray solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<h1 style="margin: 0; padding: 0; font-size: 10pt;">This change has been discarded.</h1>
</td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma and Solid.</div>
<div>By Oliver Henshaw.</div>
<p style="color: grey;"><i>Updated Sept. 2, 2015, 1:35 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=313123">313123</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-workspace
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Not enough</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>ksmserver/screenlocker/greeter/CMakeLists.txt <span style="color: grey">(11905152321377aa3135424fa2e09bb991878da5)</span></li>
<li>ksmserver/screenlocker/greeter/greeterapp.h <span style="color: grey">(f332bfc5c432b5b48b568c815904a12b4a74bd7a)</span></li>
<li>ksmserver/screenlocker/greeter/greeterapp.cpp <span style="color: grey">(b70c9d6c005aa66d2f85a42ef4f1dcb04ea44667)</span></li>
<li>ksmserver/screenlocker/greeter/main.cpp <span style="color: grey">(180d64d58b069e2d5b8362fb43c63ec2e834fadb)</span></li>
<li>ksmserver/screenlocker/interface.h <span style="color: grey">(018464cda6c1ea9511cd4553e615bee62a66949a)</span></li>
<li>ksmserver/screenlocker/interface.cpp <span style="color: grey">(32acad1b8afc8f15403d9e7b92dd49c51d355bdc)</span></li>
<li>ksmserver/screenlocker/ksldapp.h <span style="color: grey">(3047c00558d664ffac864582a4cb2c4f725a3cf4)</span></li>
<li>ksmserver/screenlocker/ksldapp.cpp <span style="color: grey">(7f2e6715f0c12baa1586e41fa8f767707c9f83c5)</span></li>
<li>plasma/screensaver/shell/CMakeLists.txt <span style="color: grey">(5c52da691acf24422efc420be9bdc71eb716b268)</span></li>
<li>plasma/screensaver/shell/main.cpp <span style="color: grey">(be151aa25dbff449d2e777ace9b43cac3de12f6d)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/109693/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>