Read floating date time as LocalTime.
Needs ReviewPublic

Authored by dcaliste on Nov 13 2019, 3:12 PM.

Details

Reviewers
vkrause
pvuorela
dvratil
Group Reviewers
KDE PIM
Summary

Date time like DTSTART:20191113T120000 are considered like floating
date time in RFC5545. This commit modifies the icalformat_p readICalDateTime
functions not to use the system time zone in that case, but the
LocalTime spec. Otherwise, there is no way to distinguish incoming
floating time from time given in the system time zone. Then when
exporting to iCal format, the floating format is not conserved.

Also properly export date time in UTC using the Z character and not TZID=UTC
as mentioned in RFC5545 section 3.3.5:
"The "TZID" property parameter MUST NOT be applied to DATE-TIME

properties whose time values are specified in UTC."
Test Plan

A test is added in testicalformat.cpp to verify that floating date time,
date, UTC date time and date time with a time zone are properly read
and exported.

Diff Detail

Repository
R172 KCalendar Core
Branch
rfc5545
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18756
Build 18774: arc lint + arc unit
dcaliste created this revision.Nov 13 2019, 3:12 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptNov 13 2019, 3:12 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
dcaliste requested review of this revision.Nov 13 2019, 3:12 PM

I think that tests are still passing properly, but this change may have unexpected consequences. I would like to tqke the opportunity of this pqtch to discuss if it's a valid approach for floating date time input. Thank you.

dcaliste added inline comments.Nov 13 2019, 5:03 PM
src/icalformat_p.cpp
2372

This is to avoid to create a DTSTART;TZID=UTC:20191113T12000000 entry. Which is understandable by the parser and by other software I guess, but is not conforming to RFC5545 (https://tools.ietf.org/html/rfc5545#section-3.3.5).

2501

Here, I needed to verify tzid because QTimeZone("") is returning the system time zone on my Debian and not an invalid time zone as the documentation is suggesting.

I don't know if it's a Qt bug or not. Reading the time zone related code is quite convoluted with a lot of ifdefs...

src/icaltimezones.cpp
141

The only call to this routine is from the icalformat_p.cpp in readICalDateTime if I grepped well. Thus changing this return for an imvalid QTimeZone should have no other impact than the one desired in this patch.

dcaliste updated this revision to Diff 69688.Nov 13 2019, 5:05 PM

Correct wrong indentation.

I can't argue with the code or with your logic.

I think we should commit to master. are there any specific bugs you are trying to fix with this?

@winterz thanks for looking at this. I'm sorry, I forgot to talk about the context. Indeed, I've no bug related to this. This is coming for the work I'm doing to port from the ancient Meego-era KCalCore code in SailfishOS to the latest upstream. One of the work in the transition in SailfishOS is to replace KDateTime by QDateTime. Doing this, I notice that the ClockTime spec of KDateTime does not exist anymore. This spec is currently used to store floating time events. So I wonder, how are they doing upstream ? And the best way to look at this, in my opinion, was to look at the reading of iCal floating time in icalformat_p.cpp. That's how I notice that floating times were converted into current system time zone, which is fine at the moment of reading the iCal data, but leave no way to store the event as a floating date (so viewing this event after a time zone change like travelling, will actually display the event at the wrong time) and worst, sending back this event to a server in a sync process will loose the floating date information and will attach this event to a specific time zone. Thus the patch.

"I think we should commit to master." it's a framework module now. We can't just push and wait and see :)

@mlaurent I agree. That is the point of my opening sentence "take the opportunity of this patch to discuss the consequences here". I'm wondering how codes using KCalendarCore are currently handling floating time events incoming from iCal data during a sync operation for instance. Looking at the code, I guess that they are ignoring the floating property and display the event in the current time zone. What happens when they are travelling to another time zone ? What happen when they upsync again this event to a remote server ? Didn't any users already notice that floating time events are not kept floating after up sync ? Am I completely wrong and I'm missing something here that make possible to handle floating time events with the current code ?

It's not in a hurry to merge this patch since SailfishOS is not going to transition soon. But I would be glad if some users (I mean code users) of KCalendarCore can test this patch and discuss on the consequences on their side and what do they think of the handling of floating time events.

@winterz thanks for the tests, it's greatly appreciated. I'll give a look to the bugs you mentioned to see if there is some report already about floating time issues with timezone.

Besides, the patch here, is just enabling the possibility to distinguish from iCal data floating times from converted ones to system time zone. Maybe some further handling will be required in other parts to actually fix possible issues in KOrganizer. At least in SailfishOS, I know we have some bugs that requires modifications higher in the stack. I'll see if corrections should be proposed in KCalendarCore.