[PATCH] implementation of a timezone backend for Windows

[Till Adam]

On Saturday 14 February 2009 20:18:37 David Jarvie wrote:
> On Thursday 12 February 2009 23:14:38 Patrick Spendrin wrote:
> > Till Adam schrieb:
> > > Folks,
> > >
> > > as part of our work on Kontact on Windows in the enterprise4 branch
> > > we've implemented a KTimeZone backend for Windows. You can find the
> > > patch at:
>
> I wondered when somebody might tackle this. I hope you had fun ;)

Marc did all the research, I just integrated what he found into your 
framework. Nice job on that, btw.

> > > http://kdab.net/~till/e4-patches/timezone-support-windows.diff
> > >
> > > It's working ok, in e4, so we'd like to commit it to trunk, since it's
> > > the only unmerged piece we have still. Didn't want to merge it before
> > > 4.2 as we were not fully done with it yet. Patch applies fine to trunk.
> > > David (Jarvie), please review, when you have time. Windows folk, maybe
> > > you can also give it a read, in case we're doing something stupid,
> > > Windows wise. Meanwhile I'll see about the unit tests on Windows etc.,
> > > not sure they all pass yet.
>
> I can only comment on how the code fits in with the existing code, since
> I'm not a Windows coder.

Thanks for taking the time to do that.

> The first question is, does Windows have alternative methods of accessing
> time zone data? Unix allows time zone data to be accessed either by the
> libc library or by accessing the tzfile data. The former provides more
> limited data (and is used by KSystemTimeZone), while the latter provides
> all the system knows about time zones at the expense of extra overhead (and
> is used by KTzfileTimeZone). Your implementation would be suitable if
> Windows provides only one data access method, or if it provides more than
> one method but you're only using the 'easy' access method just now.

Hm, it's the best and most complete we've found.

> On the assumption that they provide the fullest information available on
> Windows, I wonder whether the KSystemTimeZoneWindows classes could perhaps
> be made publicly accessible in the same manner as the Unix-specific
> KTzfileTimeZone classes. Just an idea.

Hm, would that add anything?

> I do wonder whether implementation code in ktimezone_windows.h shouldn't be
> separated out into a .cpp file. It isn't particularly easy to read with the
> declarations and implementation mixed together. Either that, or use
> 'inline' declarations to allow all the implementation code to be moved to
> the end of the file.

Ok, I'll clean that up.

> The Windows code uses assert() calls to check passed parameters whereas the
> existing time zone code uses tests and returns a default value if the tests
> fail. Shouldn't they use a common approach?

Matter of coding style, but yes. I'll change the code accordingly.

> I notice you use qDebug() and qWarning() instead of the KDE equivalents. Is
> there a particular reason for this? Aside from the more obvious effects,
> this prevents debug output using debug area 161 which is used by the other
> time zone classes.

The meat of the functionality was implemented as a standalone Qt program, 
before being integrated into kdelibs. Again, something for me to clean up, 
will do that.

> kdebase/runtime/ktimezoned/ktimezoned.cpp needs to have Windows code added
> in the appropriate places as well. Or, if ktimezoned is by any chance
> unnecessary on Windows, it should be disabled on that system.

I actually have a patch for it, just forgot to attach it. It's:

http://websvn.kde.org/branches/kdepim/enterprise4/kdebase-4.1-
branch/runtime/ktimezoned/ktimezoned.cpp?r1=849447&r2=920715

> > Looks ok so far.
> > maybe the test application could be removed from the comments and
> > converted into a unittest instead.
>
> I'd also move the test code rather than leaving it in comments (without
> explanation - seeing a commented out KTimeZone class declaration makes one
> wonder, what on earth is going on here? until one sees the commented out
> main() code later on). Or if really necessary, use conditional compilation
> rather than commenting it out.

Yes, this is another oversight. I'll try to get whatever isn't covered by the 
unit tests already into there and otherwise remove this code. It's another 
left over from the genesis of this code as a standalone app.

Thanks,

Till


-- 
Till Adam
KDAB - platform independent software services
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20090214/5bfd9c3e/attachment.htm>