> 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.
> 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/ - Martin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113260/#review41816 ----------------------------------------------------------- On Oct. 15, 2013, 8:09 p.m., Martin Klapetek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/113260/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2013, 8:09 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/CMakeLists.txt bafc85e > ktimezoned/ktimezoned.h ff21807 > ktimezoned/ktimezoned.cpp f380c09 > 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 > >
