Review Request 113260: Port KTimeZoned to Qt5/KF5

John Layt jlayt at kde.org
Mon Oct 21 13:15:58 BST 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)
> 
> Martin Klapetek wrote:
>     > 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 :)

I'm not sure Qt5 even compiles on Solaris, and it might be hard to convince Thiago to accept code for it, but I can ask what the status there is.  The alternative is to have timeZone() and listTimeZones() calls in ktimezoned that wrap the Qt or Solaris calls.

As for Qt monitoring the time zones: yes that would probably be the end result which is not ideal, but AFAIK we can't use a daemon inside Qt, or call dbus inside QtCore which rules out using ktimezoned if it's running, or systemd if it ever adds this feature.  I have to post to the development list the design proposal for it and see if it gets shot down or not.


- John


-----------------------------------------------------------
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/kde-core-devel/attachments/20131021/9056b935/attachment.htm>

More information about the kde-core-devel mailing list