my own kdatetime patches: toString with InvalidOffset; operator==

[David Faure]

On Thursday 25 October 2012 22:47:00 Jon Severinsson wrote:
> torsdagen den 25 oktober 2012 19:25:29 skrev du:
> > On Thursday 25 October 2012 16:30:20 Jon Severinsson wrote:
> > > I triggered this in Qt5 too, and I figured out the underlying reason for
> > > it to  happen. If, when KSystemTimeZonesPrivate::setLocalZone() tries to
> > > figure out what time zone to use for KSystemTimeZones::local(), the
> > > kconfig "ktimezonedrc"/ "TimeZones"/"LocalZone" time zone does not exist
> > > in the kconfig  "ktimezonedrc"/"TimeZones"/"ZoneinfoDir" directory, it
> > > will create a "valid" KTzTimeZone with garbage data.
> > > 
> > > This happens because KDateTimeTest::cleanupTestCase() successfully
> > > removes the  temporary zoneinfo dir it creates, but used the wrong path
> > > when trying to remove the ktimezonedrc kconfig file.
> > > 
> > > Patches for both KDateTimeTest::cleanupTestCase() and
> > > KSystemTimeZonesPrivate::setLocalZone() will be uploaded to review board
> > > shortly.
> > 
> > Well done, I was really wondering what triggered the issue. This is why I
> > didn't commit my fixes yet -- I wanted to write a unittest for them, but
> > didn't know what to write in the unittest (all my attempts failed).
> > 
> > Would you be interested in adding a corresponding unittest to
> > reproduceably
> > trigger the above issue, and get into an "invalid timezone in a valid
> > KDateTime"?
> 
> What is it you want me to test?
> With my patches there is no way for KSystemTimeZones::local() to create an
> valid but garbage time zone, and I will add test to verify that, but that
> don't involve KDateTime.

OK. Sounds good.

> Getting a valid KDateTime with a "valid but garbage" timezone spec is
> currently as simple as this:
> KTzfileTimeZoneSource source("/invalid/tzinfo/dir");
> KDateTime
> date(QDate(2000,1,1),QTime(0,0,0),KTzfileTimeZone(&source,"Europe/London"))
> ; But what to do whith that?

Call toString and check what you get ;)
Well -- that's the unittest for my patch then! Thanks for the input, this 
allowed me to finish it. Attached.

> I could fix KDateTime::Spec::setType(KTimeZone) (and thus the implicit
> constructor) to detect "valid but garbage" as an KDateTime::Invalid time
> zone spec and verify that the KDateTime is considered invalid. 

Ah, that's another possibility, yes. That would make my patch useless then, 
but that's fine :-)

I'm no KDateTime expert, I'll let you choose the solution that makes sense to 
you.

> > I also wonder if isValid() shouldn't return false, instead, in this
> > particular case (invalid timezone offset). That would make more sense I
> > suppose, and then my patch to toString() is unnecessary since the method
> > starts with "return empty if the datetime is invalid" (which doesn't make
> > debugging any easier, though...)
> 
> This would change the KTimeZone::isValid() from a simple QString::isEmpty()
> call into a quite complex operation, possible including reading a file from
> disk and parsing it's content, I don't think that is advisable. Not to
> mention the infinite loop it would create :(
> 
> ( This because KTimeZone uses lazy initialization, and don't call
> KTimeZoneSource::parse() until the first the call to KTimeZone::data(true),
> which is currently used by all getters, but not by isValid(). And as
> KTimeZone::data(bool) starts of by calling "if (!isValid()) return 0;",
> using any getters in isValid() would be a bad idea... )

OK... forget that then, your earlier solution seems more practical.

Actually by isValid I was referring to KDateTime::isValid, not to 
KTimeZone::isValid. So this was a misunderstanding, and we actually agree :)

Thanks!

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE, in particular KDE Frameworks 5
-------------- next part --------------
A non-text attachment was scrubbed...
Name: invalidtz_fix.diff
Type: text/x-patch
Size: 1665 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20121025/72e39b6b/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20121025/72e39b6b/attachment-0001.sig>