<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/5320/">http://svn.reviewboard.kde.org/r/5320/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 13th, 2010, 4:44 p.m., <b>Dario Freddi</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;">First of all, thanks for looking into this and most of all for submitting a patch.
Unfortunately, just like Sebastian said, I'm afraid I'm not willing to let this patch in. However, in the upcoming solid sprint I plan to address this. Luckily, UPower has a signal which does exactly what you tried to achieve here. I will get in touch with you as soon as I will implement the needed part in PowerDevil/Solid so you can code another patch against the new method. How does this sound to you?</pre>
</blockquote>
<p>On September 13th, 2010, 4:52 p.m., <b>Björn Ruberg</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;">That's sounds good. But this patch does not mean that the problem can never be fixed correctly.
Maybe this can be seen as a fix for the 4.5 branch while you add the correct functionality for 4.6?</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;">Mail from Sebastian Kügler:
> I'd rather not have it in high-level Plasma components since it's
> essentially working around a limitation in HAL. There are a couple of
> problems with putting it in powerdevil as is:
>
> - the DBus method you expose is only valid for the HAL backend, if someone
> uses the (future) upower backend, you'd still use this workaround. Also,
> app developers might start to rely on this behaviour.
> - DBus interfaces are to be regarded as public API, therefore we cannot
> just change them
> - the workaround is in the wrong layer of the stack, the app (timeengine in
> this case) shouldn't work around limitations which we need to fix below in
> the stack (i.e. in Solid)
>
> Maybe we can put this workaround into the hal backend itself, and have it
> emit resumed() when it detects this clock skew. The upower backend would
> then emit the same signal, but triggered in the correct way. The
> applications get the hotness in a transparant manner and can adapt (i.e.
> our timeengine fires updates).
>
> This doesn't solve the problem that we're adding public API (the DBus
> method in powerdevil), but doing it in this future-proof (yey, right!) way
> would make it more acceptable IMO. You'll need to get it past Kevin,
> though. >:)
I would be fine reworking the patch in that manner IF it will be accepted then. May be I get some stubs provided so that I know where I have to go into powerdevil/solid.
But concerning your dbus-concerns: I'm inventing a signal standbyRequested(). I can rename it awaitingStandby() or whatever. Important thing is, I don't see a reason why such a signal should not be in the public API. I can imagine different usages for it, i.e. applications gracefully closing network connections before standby.
The whole polling stuff is there to work around the missing "resumed" signal. Surely that can be moved in the lower layer. The main benefit of this that there will actually be an "resumed()"-signal that other applications can catch. But as there will come a correct signal for this with the 4.6 release (I hope so :)), the patch might even stay as it is for the 4.5 line.</pre>
<br />
<p>- Björn</p>
<br />
<p>On September 13th, 2010, 11:20 a.m., Björn Ruberg wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma and Solid.</div>
<div>By Björn Ruberg.</div>
<p style="color: grey;"><i>Updated 2010-09-13 11:20:38</i></p>
<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;">NOTE: This a little bit ugly as I have to workaround the missing "going to suspend"-signal in the linux userland
This patch adds a signal to powerdevil that is emitted when a suspend/hibernate/standby is requested there. The signal is caught by the time engine what starts to look for an unexpected change in system time for two minutes by polling. If a clock skrew is detected, all sources are updated immediatly.
This fixes the bug that the plasma clocks are showing old time after suspending your machine up until 59 seconds (whenever the normal update is scheduled). This is not exactly a patch for bug 181380. But the clock skrew detection code can be used quite similar for detecting normal time changes. I'll look into that as soon this is accepted.
Please tell me whether I can commit this to 4.5 trunk as it is a bugfix for a nasty glitch.</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;">Suspended machine, waited three minutes, woke up again - and noticed that all clocks on screen showed the correct time almost immediatly :)</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=181380">181380</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/KDE/kdebase/workspace/plasma/generic/dataengines/time/timeengine.h <span style="color: grey">(1166313)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/dataengines/time/timeengine.cpp <span style="color: grey">(1166313)</span></li>
<li>/trunk/KDE/kdebase/workspace/powerdevil/daemon/PowerDevilDaemon.h <span style="color: grey">(1166313)</span></li>
<li>/trunk/KDE/kdebase/workspace/powerdevil/daemon/PowerDevilDaemon.cpp <span style="color: grey">(1166313)</span></li>
<li>/trunk/KDE/kdebase/workspace/powerdevil/daemon/org.kde.PowerDevil.xml <span style="color: grey">(1166313)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5320/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>