<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/121915/">https://git.reviewboard.kde.org/r/121915/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On gener 9th, 2015, 11:25 a.m. UTC, <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Where do you plan to use this? We are not maintaining compatibility (so far) in our dbus apis, so why add this at all?</p></pre>
</blockquote>
<p>On gener 9th, 2015, 12:04 p.m. UTC, <b>Daniel Vrátil</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KScreen. For now we are listening to UPower, which IMO sucks and we should use PowerDevil instead (as it provides abstraction for alternative backends). This makes it just easier to monitor changes.</p></pre>
</blockquote>
<p>On gener 9th, 2015, 4:05 p.m. UTC, <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This has a few problems:
1-It couples KScreen more to PLasma by adding a runtime dependency (which is ok if you decide to do so)
2-It might create a deadlock since kscreen is a kded module asking to another module (powerdevil) for things.
3-We will depend on Powerdevil if we use kscreen in other places (sddm desperatly needs something like kscreen, or kscreen itself)
4-Adds a dependency to an api that is not stable (so far it has not been), we have changed it many times and I am sure we will change it again</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">My recommendation for this will be to merge the new power management api in Solid and make kscreen depend on it. Solid already offers backends (so we don't have to implement this in kscreen) and we don't have to expose powerdevil internals.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So this is a -1 from my side (if the only reason to add this is KSCreen).</p></pre>
</blockquote>
<p>On gener 9th, 2015, 4:11 p.m. UTC, <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">2-It might create a deadlock since kscreen is a kded module asking to another module (powerdevil) for things.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As dfaure explained in https://git.reviewboard.kde.org/r/121885/ QtDbus is really smart and in-process DBus calls are made into direct methods and should never deadlock. Which is cool :)</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Oh wow! One less problem to worry about then.
We still need to be careful not to deadlock by doing sync calls between the same kded modules (happened already).</p></pre>
<br />
<p>- Àlex</p>
<br />
<p>On gener 8th, 2015, 12:04 p.m. UTC, Daniel Vrátil wrote:</p>
<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 Daniel Vrátil.</div>
<p style="color: grey;"><i>Updated gen. 8, 2015, 12:04 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
powerdevil
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There's no way to detect when lid has been closed other than listening to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">changed</code> signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened.</p></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>daemon/org.kde.Solid.PowerManagement.xml <span style="color: grey">(53f77e5)</span></li>
<li>daemon/powerdevilbackendinterface.h <span style="color: grey">(702b66b)</span></li>
<li>daemon/powerdevilbackendinterface.cpp <span style="color: grey">(6dd8c71)</span></li>
<li>daemon/powerdevilcore.h <span style="color: grey">(e255703)</span></li>
<li>daemon/powerdevilcore.cpp <span style="color: grey">(d51ab19)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121915/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>