Add support for timezone and all day events #5
Conversation
There was a problem hiding this comment.
One point to note, funnily enough $event->dtstart_array[2] contains timezone information while $event->dtstart doesn't. Bug? As long as timezone information is present in DTSTART and DTEND, master should work fine anyway.
Secondly, if there is no timezone information in the date OR the event, we should NOT show the user the timezone. Currently, it looks like
All day event (no tz)
2020-12-01 (UI-TZ)
but we don't really know if the event is UI-TZ, which is very misleading.
Overall (also refer to my other comments for context), I think we need to change the flow here to check if the individual dates have TZ information, if not, set it to TZ from the event. If the event doesn't have TZ either, then assume it's the one set in UI, but don't display it.
| } catch (Exception $e) { | ||
| // Use UI timezone if the calendar have no timezone specified | ||
| $ical_tz = $ui_tz; | ||
| } |
There was a problem hiding this comment.
These try-catch blocks are ugly. Can't we use null coalescing instead?
| $dtstart = new DateTime($event->dtstart, $ui_tz); | ||
| $dtend = new DateTime($event->dtend, $ui_tz); | ||
| // All day events ends at next day midnight, we should fix the day | ||
| $dtend->modify('-1 day'); |
There was a problem hiding this comment.
I don't think this is right. From RFC 5545 (https://tools.ietf.org/html/rfc5545#page-54)
The "VEVENT" is also the calendar component used to specify an
anniversary or daily reminder within a calendar. These events
have a DATE value type for the "DTSTART" property instead of the
default value type of DATE-TIME. If such a "VEVENT" has a "DTEND"
property, it MUST be specified as a DATE value also. The
anniversary type of "VEVENT" can span more than one date (i.e.,
"DTEND" property value is set to a calendar date after the
"DTSTART" property value). If such a "VEVENT" has a "DURATION"
property, it MUST be specified as a "dur-day" or "dur-week" value.
From what I understand, DTEND would be absent in a single day event, so we need to check for that instead. Also see the examples on Pg. 55
| // Events with proper time should use calendar timezone. | ||
| // Note that if DTSTART/DTEND ends with "Z", UTC is used instead of the given timezone automatically | ||
| $dtstart = new DateTime($event->dtstart, $ical_tz); | ||
| $dtend = new DateTime($event->dtend, $ical_tz); |
There was a problem hiding this comment.
We should really be checking if the dates have timezone information instead ... correct me if I'm wrong, but I think it is possible that the event date and the DTSTART/DTEND may be in different timezones. In that sense, we should fall back to the event timezone only if the the date timezone is missing.
There was a problem hiding this comment.
What you can do probably is check if dtstart_array[0] and dtend_array[0] respectively have the key TZID. If yes, directly use dtstart_array[2] in format_date, otherwise create a new DateTime object using the timezone from calendarTimeZone if any.
| if ($is_all_day) { | ||
| // All day events shouldn't display time | ||
| $dtstr = $rcmail->format_date($dtstart, $date_format) | ||
| . (!$is_oneday ? ' – ' . $rcmail->format_date($dtend, $date_format) : ''); |
| $dtstr = $rcmail->format_date($dtstart, $date_format) | ||
| . (!$is_oneday ? ' – ' . $rcmail->format_date($dtend, $date_format) : ''); | ||
| } else { | ||
| $dtstr = $rcmail->format_date($dtstart, $combined_format) . ' – ' |
The previous implementation had use only the date-time part of dtstart_array, ignoring timezone information. This pull request provides a hopefully complete timezone implementation.
I also added support for all-day events. When displaying an all-day event the time part is uninteresting (00:00), plus the end date can be ambiguous to read (next day midnight). Timezone is an other twits in this story, because converting the timezone to local time often shifts the date. All of this issues are handled correctly in my patch.
I'm attaching some test .ics file I worked with.
test-events.zip