Review Request 113260: Port KTimeZoned to Qt5/KF5

Mon Oct 21 11:20:19 UTC 2013

> On Oct. 16, 2013, 12:15 p.m., John Layt wrote:
> > 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.
> 
> Martin Klapetek wrote:
>     > 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/
> 
> David Faure wrote:
>     There are still people around with a Solaris system. Not supported doesn't mean we should actively remove existing support, IMHO.
>     
>     About the porting guide: the distinction between kdelibs and kde-runtime will disappear, given the framework-ification. So treat it as a porting guide "from kde 4 to kde-frameworks", i.e. it's fine to include "runtime" stuff in there.
>     
>     About a solution in Qt --- that means each and every Qt app will have to monitor the same timezone file(s), isn't that a bit too expensive? (not sure, just wondering)

> There are still people around with a Solaris system. Not supported doesn't mean we should actively remove existing support, IMHO.

Is there any attempt to make sure KF5/PW2 is working with Solaris? Because if the underlying components won't work on Solaris, there's no point in keeping Solaris code in one tiny module sitting on top of the bigger blocks.

> it's fine to include "runtime" stuff in there.

Cool, will add it then :)

- Martin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113260/#review41816
-----------------------------------------------------------

On Oct. 18, 2013, 1 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113260/
> -----------------------------------------------------------
> 
> (Updated Oct. 18, 2013, 1 p.m.)
> 
> 
> Review request for KDE Runtime, KDE Frameworks, Plasma, and John Layt.
> 
> 
> Repository: kde-runtime
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   ktimezoned/ktimezoned.cpp f380c09 
>   ktimezoned/ktimezoned.h ff21807 
>   ktimezoned/CMakeLists.txt bafc85e 
>   ktimezoned/ktimezoned_win.h 26e21cc 
>   ktimezoned/ktimezoned_win.cpp cadfe3a 
>   ktimezoned/ktimezonedbase.h ca00aca 
>   ktimezoned/org.kde.KTimeZoned.xml daaa0b7 
> 
> Diff: http://git.reviewboard.kde.org/r/113260/diff/
> 
> 
> Testing
> -------
> 
> Tested by changing the timezone in different ways, change was detected and signalled out.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

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