Handle incidences in different time zones in MemoryCalendar.
ClosedPublic

Authored by dcaliste on Oct 30 2019, 9:25 AM.

Details

Summary

a calendar and an incidence may be given in different
time zones. This may confuse the date cache in MemoryCalendar.
Properly move to the calendar time zone when setting incidences
in the date cache.

Test Plan

I've added a method in testmemorycalendar to demonstrate
the issue, using a UTC calendar and adding an event from a time
zone where the date as already changed. I've added in this test
various possibility to track addition, modification and deletion
of such events.

Diff Detail

Branch
tz
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18343
Build 18361: arc lint + arc unit
dcaliste requested review of this revision.Oct 30 2019, 9:25 AM
dcaliste created this revision.

Would you please review this patch for time zone proper handling in MemoryCalendar ? The block deletion in rawEventsForDate() is in my opinion justified by the fact that when looking in the cache, we're looking for events that are neither multidays nor recurrent. So testing the end date for such one day only events has no point. What do you think ?

dcaliste added a reviewer: vkrause.
dcaliste added a subscriber: KDE PIM.
dvratil accepted this revision.Nov 6 2019, 10:38 AM

Looks good to me. Thanks!

This revision is now accepted and ready to land.Nov 6 2019, 10:38 AM

Great, thanks for the review, I'll push it later today or tomorrow.

dcaliste closed this revision.Nov 7 2019, 3:12 PM

This seems to have side-effects on kitinerary's unit tests: https://build.kde.org/job/Applications/job/kitinerary/job/kf5-qt5%20SUSEQt5.12/152/testReport/projectroot/autotests/calendarhandlertest/ - the problem seems to be that this is setting an invalid timezone on MemoryCalendar. Is that API mis-use anyway, or should we harden MemoryCalendar against this?

@vkrause Oups, that's bad. I'm giving a look. Sorry for the introduced issue but thanks for reporting :/

Indeed, such snippet is segfaulting on the second toTimeZone() call:

QDateTime dt(QDate(2019, 11, 18), QTime(10, 0));
qDebug() << dt.toTimeZone(QTimeZone::systemTimeZone());
qDebug() << dt.toTimeZone(QTimeZone());

This is happening on the test machine, because the systemTimeZone() is not defined and returning an invalid time zone, I guess. So the calendar in the test (created with such a call) does not have a valid timezone defined.

Just like that, I would say it's a Qt bug, toTimeZone() should not segfault on an invalid time zone argument. Besides, for KCalendarCore, I'm wondering what it means to have a calendar without timezone defined. Does it make sense ? There is several possibilities to solve the issue:

  • patch calendar constructor to fallback to UTC if the provided timezone on construction is invalid.
  • allow calendars to have invalid timezone and investigate everywhere if it's not creating issues. But the date accessor in memory calendar does not make sense without time zone defined for instance.

It would prefer to go for the first solution, but I may be wrong, what do you think ?

Indeed, such snippet is segfaulting on the second toTimeZone() call:

QDateTime dt(QDate(2019, 11, 18), QTime(10, 0));
qDebug() << dt.toTimeZone(QTimeZone::systemTimeZone());
qDebug() << dt.toTimeZone(QTimeZone());

This is happening on the test machine, because the systemTimeZone() is not defined and returning an invalid time zone, I guess. So the calendar in the test (created with such a call) does not have a valid timezone defined.

The crashing code was using QTimeZone(), I added systemTimeZone() there to work around this issue. But yes, this confirms the problem I think.

Just like that, I would say it's a Qt bug, toTimeZone() should not segfault on an invalid time zone argument. Besides, for KCalendarCore, I'm wondering what it means to have a calendar without timezone defined. Does it make sense ? There is several possibilities to solve the issue:

  • patch calendar constructor to fallback to UTC if the provided timezone on construction is invalid.
  • allow calendars to have invalid timezone and investigate everywhere if it's not creating issues. But the date accessor in memory calendar does not make sense without time zone defined for instance.

    It would prefer to go for the first solution, but I may be wrong, what do you think ?

I agree. I don't see what the semantics of an invalid timezone would be here, so just protecting against accidentally passing one in should be good enough.

I added systemTimeZone() there to work around this issue.

It seems that the test machine is returning an invalid time zone when calling systemTimeZone(). At least, that's my guess.

I'm working on a patch to Calendar, to guard against invalid time zone.

@vkrause sorry, I misunderstand by looking at the current code of kitinerary… Before it was with a QTimeZone(), but systemTimeZone() is actually working. Patch is coming to ensure that systemTimeZone is used when creating a MemoryCalendar from an invalid time zone argument.

@vkrause see https://phabricator.kde.org/D25367 for a patch ensuring a fallback to the system time zone if invalid.