<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://git.reviewboard.kde.org/r/113260/">http://git.reviewboard.kde.org/r/113260/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 16th, 2013, 12:15 p.m. UTC, <b>John Layt</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;">Wow, there sure is a lot of code in there catering for all the possible corner cases :-) QTimeZone has a lot less places it checks, so I'll need to do an in-depth comparison, but given Qt5 will only support modern distros I think most of it is redundant.
One thought is if we still want to have a signal that the system database has been updated? While Qt doesn't cache the time zone data, the apps probably will cache the QTimeZone instances themselves and may need to be told that the database has changed so they can take action. Then again, it's not something that will happen very often, and what will the apps do in response? If they refresh their cache the shared copies in QDateTime won't update. I guess QTimeZone will need a "reloadData()" method added instead.
I've looked at the Windows code and it mostly looks OK, all it does is monitor the registry for changes. What you do need to change is the DBus signal from "configChanged" to your new "timeZoneChanged" signal.
With regards the signal name change, does it need to go in the porting guide or somewhere so people know it has changed?
In Qt 5.3 I will try adding a QEvent for TimeZoneChanged and TimeZoneDatabaseChanged, but I can't guarantee it will get in so we still need the daemon for now.</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;">> Wow, there sure is a lot of code in there catering for all the possible corner cases :-) QTimeZone has a lot less places it checks, so I'll need to do an in-depth comparison, but given Qt5 will only support modern distros I think most of it is redundant.
Part of that is support for Solaris, but I see Solaris is not supported by Qt anymore [1][2], so I just removed it altogether.
> One thought is if we still want to have a signal that the system database has been updated? While Qt doesn't cache the time zone data, the apps probably will cache the QTimeZone instances themselves and may need to be told that the database has changed so they can take action.
I think it might be worth having it...I'll add it back, can't hurt :)
> I've looked at the Windows code and it mostly looks OK, all it does is monitor the registry for changes. What you do need to change is the DBus signal from "configChanged" to your new "timeZoneChanged" signal.
Ah yes, I forgot to add that to the patch, sorry.
> With regards the signal name change, does it need to go in the porting guide or somewhere so people know it has changed?
I wanted to add it, but that's in kdelibs. Question is, should there be a guide for runtime/workspace too?
> In Qt 5.3 I will try adding a QEvent for TimeZoneChanged and TimeZoneDatabaseChanged, but I can't guarantee it will get in so we still need the daemon for now.
Ok, keep us posted. (It would be cool to have cross-desktop solution for this too...or system-wide spec at least, so we could use non-qt/-kde apps still supporting timezone changes, but oh well).
[1] - http://qt-project.org/doc/qt-5.1/qtdoc/supported-platforms.html
[2] - http://qt.digia.com/Product/Supported-Platforms/</pre>
<br />
<p>- Martin</p>
<br />
<p>On October 15th, 2013, 8:09 p.m. UTC, Martin Klapetek wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Runtime, KDE Frameworks, Plasma, and John Layt.</div>
<div>By Martin Klapetek.</div>
<p style="color: grey;"><i>Updated Oct. 15, 2013, 8:09 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-runtime
</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;">Originally I wanted to port KTimeZoned 1:1 to Qt5/KF5, but then I found out that all the stuff KTZD was doing was added in QTimeZone, that includes reading correct system files/env variables to obtain the timezone and returning the proper system zone (KTZD did all this by itself). It also doesn't need to parse the timezone files itself or watch for their changes as QTimeZone objects are not stored.
So now it's just a thin module watching /etc/timezone (used by Debian-based distros) and /etc/localtime (used by eg. Fedora or Suse, but also by Debian in conjunction with /etc/timezone) for changes and if it detects a change, it checks if the new timezone is really different and if it is, it sends out a DBus signal "timeZoneChange". I changed it from "configChanged" as I think "timeZoneChanged" makes way more sense.
I didn't touch the Windows part as I have no way to test, would be nice if someone could help with that.
EDIT: I removed the other two DBus signals which were not used and I'm unsure KTZD is the correct place for that now anyway. The only usage in KSystemTimeZone can be replaced by own KDirWatch instance.</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;">Tested by changing the timezone in different ways, change was detected and signalled out.</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>ktimezoned/CMakeLists.txt <span style="color: grey">(bafc85e)</span></li>
<li>ktimezoned/ktimezoned.h <span style="color: grey">(ff21807)</span></li>
<li>ktimezoned/ktimezoned.cpp <span style="color: grey">(f380c09)</span></li>
<li>ktimezoned/ktimezoned_win.h <span style="color: grey">(26e21cc)</span></li>
<li>ktimezoned/ktimezoned_win.cpp <span style="color: grey">(cadfe3a)</span></li>
<li>ktimezoned/ktimezonedbase.h <span style="color: grey">(ca00aca)</span></li>
<li>ktimezoned/org.kde.KTimeZoned.xml <span style="color: grey">(daaa0b7)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/113260/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>