Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VTimeZone.FromSystemTimeZone method does not create DAYLIGHT or STANDARD collections #58

Closed
mcshaz opened this issue Jul 21, 2016 · 33 comments

Comments

@mcshaz
Copy link
Contributor

mcshaz commented Jul 21, 2016

The spec mandates that timezone contain at least 1 of these elements. Will post some tests which demonstrate.

@rianjs
Copy link
Collaborator

rianjs commented Jul 21, 2016

Btw, my thinking on timezones diverges from the spec... We can serialize and deserialize time zone collections so as not to lose it, but those collections of saving/standard transitions will be ignored in favor of using NodaTime for time zone calculations. There's a unit test somewhere that contains a big collection of standard/saving transitions, and it's deserialized properly, so maybe the reserialization is broken? Just a thought.

This an area of the spec that doesn't make a lot of sense in the modern world. It probably made more sense in the 90s. Manifests of when a time change occurred are... a terrible idea.

@mcshaz
Copy link
Contributor Author

mcshaz commented Jul 21, 2016

Absolutely agree re deserialising being crazy to rely on the provided manifest. For the serialising however, I believe we should try and meet the spec because the client application reading the ics string may be crazy enough to rely on the manifests! The library also claims to meet the spec on the readme, and so a developer using the library probably could reasonably expect we have attempted to comply. You have already sped this code up considerably, and so adding the manifest should be trivial in terms of serialization time.

I will put the appropriate tests up, get the output validating (by meeting the vtimezone specification) – which will then result in most tests passing, and then maybe quickly add another method FromNodaTimeZone, so that NodaTime is available (preferred) end to end.

@rianjs
Copy link
Collaborator

rianjs commented Jul 21, 2016

and then maybe quickly add another method FromNodaTimeZone, so that NodaTime is available (preferred) end to end.

I don't want to expose the implementation details of the library in this fashion. Usage of NodaTime should be invisible at the API boundary level. I'm OK with noting in the docs that the library relies on NodaTime, but I want its usage to be invisible. In fact, stuff like GetOccurrences is already using NodaTime, as is the conversion to/from UTC and local time. As an interesting note, the implementation of this method is responsible for bringing unit test runtimes from about 7 seconds down to less than 3:

https://github.com/rianjs/ical.net/blob/d61c1da19395fa7420f62c2dfe7d81ba0035c192/ical.NET/Utility/DateUtil.cs#L98

I had no idea the improvement was so significant until I changed its internals while working on the .NET Core port.

@mcshaz
Copy link
Contributor Author

mcshaz commented Jul 21, 2016

I don't want to expose the implementation details of the library in this fashion.

No worries/wilco.

@rianjs
Copy link
Collaborator

rianjs commented Jul 22, 2016

I've looked at this and thought about it some more. Right now we're capturing the time zone id, which allows us to do time zone calculations on things like recurrences and whatnot, which is the most important thing, IMO. I've read the VTIMEZONE spec page, and... I see what it says, and I can't help but disagree with it, because it just doesn't make any sense in the modern world...

Interoperability between two calendaring and scheduling applications, especially for recurring events, to-dos or journal entries, is dependent on the ability to capture and convey date and time information in an unambiguous format. The specification of current time zone information is integral to this behavior.

I'm with the spec...

If present, the "VTIMEZONE" calendar component defines the set of Standard Time and Daylight Saving Time observances (or rules) for a particular time zone for a given interval of time.

...and they've lost me. Clients should NOT be able to specify anything other than an industry-standard time zone. (BCL, serialization, or IANA.) It turns out that date and time programming is hard, and most implementations do stupid things out of ignorance of how dates, times, and calendars work. If I were on the RFC board, I would strongly advocate for the removal of the VTIMEZONE ruleset entirely.

As the spec is written, it allows a client to specify their own time zone behavior. As in... conjure up a time zone of their own invention, with its own rule set. That's crazy!

The "VTIMEZONE" calendar component MUST be present if the iCalendar object contains an RRULE that generates dates on both sides of a time zone shift (e.g. both in Standard Time and Daylight Saving Time) unless the iCalendar object intends to convey a floating time (See the section "4.1.10.11 Time" for proper interpretation of floating time). It can be present if the iCalendar object does not contain such a RRULE. In addition, if a RRULE is present, there MUST be valid time zone information for all recurrence instances.

Ugh, just... no.

The fact that the examples have a paragraph of expository text conveys demonstrates how poor the choice was. It's the RFC equivalent of a code smell:

This is an example showing time zone information for the Eastern United States using "RDATE" property. Note that this is only suitable for a recurring event that starts on or later than April 6, 1997 at 03:00:00 EDT (i.e., the earliest effective transition date and time) and ends no later than April 7, 1998 02:00:00 EST (i.e., latest valid date and time for EST in this scenario). For example, this can be used for a recurring event that occurs every Friday, 8am-9:00 AM, starting June 1, 1997, ending December 31, 1997.

Terrible.

So what does that mean for ical.net? I think that means I'm prioritizing this pretty low on the list of possible things to work on. I can't help but think the spec will change, and VTIMEZONE will become much simpler. (Akin to "Specify your IANA or serialized time zone, and that's it. Recurrences will be computed based on the time zone information for the id you provided.")

Perhaps there's a way to emit a collection of start and end transitions for a given time period using NodaTime and/or TimeZoneInfo. There probably is. But I'll be honest: I just don't care, because clients shouldn't be trying to provide it, and they shouldn't be trying to honor it when someone else provides it... because honoring it would be insane. In many ways, I feel as though ical.net's lack of support for this is a surprisingly sane choice. (And dday didn't support it historically either, so...)

That said, if you want to work on it, I won't stop you. But lets keep the functional outcome to "ical.net doesn't lose information the spec allows for that the client provides" and that's about it.

@mcshaz
Copy link
Contributor Author

mcshaz commented Jul 24, 2016

agree with all of the above (although historically - dday did at least serialise a valid timezone component with daylight/standard components etc). I think I have a lightweight and less confusing way to comply with spec (as compared to all the vtimezone sub component classes pruned a couple of months ago) in the experimental branch (in fact I think given other problems with UTC times, lowercase enum etc, the experimental branch is the first time I have seen a serialised dday.ical object fully validate against both online validators).

The only thing which really matters is whether the vast majority of calendar applications can reliably parse the information. In truth, I have no idea of this, and I suspect/hope it doesn't make any difference.

@rianjs
Copy link
Collaborator

rianjs commented Jul 24, 2016

I think I have a lightweight and less confusing way to comply with spec (as compared to all the vtimezone sub component classes pruned a couple of months ago) in the experimental branch (in fact I think given other problems with UTC times, lowercase enum etc, the experimental branch is the first time I have seen a serialised dday.ical object fully validate against both online validators).

Oh, excellent!

The only thing which really matters is whether the vast majority of calendar applications can reliably parse the information. In truth, I have no idea of this, and I suspect/hope it doesn't make any difference.

What applications have you tried? Is there anything I can help test with? Offhand, I have...

  • Outlook 2013
  • Outlook 365 web application
  • Google Calendar
  • Yahoo calendar
  • Apple mail
  • ...and I can probably find whatever the Mozilla mail client is, and try that

@aluxnimm
Copy link

aluxnimm commented Aug 9, 2016

First of all, thanks for bringing DDay.iCal back to life. We use that library heavily in outlookcaldavsynchronizer and had many issues, especially with VTIMEZONE definitions. So we would prefer to switch to ical.net. But since CalDAV servers depend on those daylight/standard components I hope you will add that to serialization also. We use NodaTime internally as well and would need a correct VTIMEZONE with DAYLIGHT/STANDARD components generated from the conversion from a Windows/Outlook timezone to a corresponding IANA timezone.

br,
Alex

@rianjs
Copy link
Collaborator

rianjs commented Aug 12, 2016

@aluxnimm, can you elaborate on this a bit?

We use NodaTime internally as well and would need a correct VTIMEZONE with DAYLIGHT/STANDARD components generated from the conversion from a Windows/Outlook timezone to a corresponding IANA timezone.

I'm not sure I understand.

@aluxnimm
Copy link

sure. right now we use dday.ical to generate icalendar data with vtimezones from windows timezones serialized. what we would prefer is a correct mapping from a windows timezone to an iana one e.g. from W. Europe Standard Time to Europe/Berlin and a VTIMEZONE generated with correct standard and daylight components for that iana zone.

@aluxnimm
Copy link

@mcshaz just checked your experimental branch code for VTIMEZONE serialization.

For my timezone with earlierst date to support 1970-1-1 it generates for example

BEGIN:VTIMEZONE
TZID:W. Europe Standard Time
BEGIN:STANDARD
DTSTART:00011002T030000
RRULE:FREQ=YEARLY;BYMONTH=10;BYDAY=-1SU;BYHOUR=3;BYMINUTE=0
TZNAME:Mitteleuropäische Sommerzeit
TZOFFSETFROM:+0200
TZOFFSETTO:+0100
END:STANDARD
BEGIN:DAYLIGHT
DTSTART:00010301T020000
RRULE:FREQ=YEARLY;BYMONTH=3;BYDAY=-1SU;BYHOUR=2;BYMINUTE=0
TZOFFSETFROM:+0100
TZOFFSETTO:+0200
END:DAYLIGHT
END:VTIMEZONE

which is not really correct because of DTSTART in year 0001 and will cause problems with various caldav servers.

A more complex timezone like Moscow with daylight saving time the whole year is also wrongly generated like this

BEGIN:VTIMEZONE
TZID:Russian Standard Time
BEGIN:STANDARD
DTSTART:00011002T030000
RRULE:FREQ=YEARLY;BYMONTH=10;BYDAY=-1SU;BYHOUR=3;BYMINUTE=0
TZNAME:RTZ 2 Sommerzeit
TZOFFSETFROM:+0400
TZOFFSETTO:+0300
END:STANDARD
BEGIN:STANDARD
DTSTART;VALUE=DATE:20110102
RRULE:FREQ=YEARLY;BYMONTH=1;BYDAY=1SA;BYHOUR=0;BYMINUTE=0
TZNAME:RTZ 2 Sommerzeit
TZOFFSETFROM:+0400
TZOFFSETTO:+0300
END:STANDARD
BEGIN:STANDARD
DTSTART;VALUE=DATE:20120102
RRULE:FREQ=YEARLY;BYMONTH=1;BYMONTHDAY=1;BYHOUR=0;BYMINUTE=0
TZNAME:RTZ 2 Sommerzeit
TZOFFSETFROM:+0300
TZOFFSETTO:+0300
END:STANDARD
BEGIN:STANDARD
DTSTART;VALUE=DATE:20130102
RRULE:FREQ=YEARLY;BYMONTH=1;BYMONTHDAY=1;BYHOUR=0;BYMINUTE=0
TZNAME:RTZ 2 Sommerzeit
TZOFFSETFROM:+0300
TZOFFSETTO:+0300
END:STANDARD
BEGIN:STANDARD
DTSTART:20141002T020000
RRULE:FREQ=YEARLY;BYMONTH=10;BYDAY=-1SU;BYHOUR=2;BYMINUTE=0
TZNAME:RTZ 2 Sommerzeit
TZOFFSETFROM:+0400
TZOFFSETTO:+0300
END:STANDARD
BEGIN:DAYLIGHT
DTSTART:00010301T020000
RRULE:FREQ=YEARLY;BYMONTH=3;BYDAY=-1SU;BYHOUR=2;BYMINUTE=0
TZOFFSETFROM:+0300
TZOFFSETTO:+0400
END:DAYLIGHT
BEGIN:DAYLIGHT
DTSTART:20110301T020000
RRULE:FREQ=YEARLY;BYMONTH=3;BYDAY=-1SU;BYHOUR=2;BYMINUTE=0
TZOFFSETFROM:+0300
TZOFFSETTO:+0400
END:DAYLIGHT
BEGIN:DAYLIGHT
DTSTART;VALUE=DATE:20120101
RRULE:FREQ=YEARLY;BYMONTH=1;BYMONTHDAY=1;BYHOUR=0;BYMINUTE=0
TZOFFSETFROM:+0300
TZOFFSETTO:+0300
END:DAYLIGHT
BEGIN:DAYLIGHT
DTSTART;VALUE=DATE:20130101
RRULE:FREQ=YEARLY;BYMONTH=1;BYMONTHDAY=1;BYHOUR=0;BYMINUTE=0
TZOFFSETFROM:+0300
TZOFFSETTO:+0300
END:DAYLIGHT
BEGIN:DAYLIGHT
DTSTART;VALUE=DATE:20140101
RRULE:FREQ=YEARLY;BYMONTH=1;BYDAY=1WE;BYHOUR=0;BYMINUTE=0
TZOFFSETFROM:+0300
TZOFFSETTO:+0400
END:DAYLIGHT
END:VTIMEZONE

The correct IANA ones can be found in http://tzurl.org/zoneinfo/.

@rianjs
Copy link
Collaborator

rianjs commented Aug 28, 2016

sure. right now we use dday.ical to generate icalendar data with vtimezones from windows timezones serialized. what we would prefer is a correct mapping from a windows timezone to an iana one e.g. from W. Europe Standard Time to Europe/Berlin and a VTIMEZONE generated with correct standard and daylight components for that iana zone.

So you want to:

  1. Generate a mapping from BCL to IANA time zone (I think there's a source of truth somewhere in NodaTime, if memory serves)
  2. Produce a manifest of time zone change information for that IANA time zone (which should be possible in versions of NodaTime that aren't the PCL)
  3. Emit this manifest in standard VTIMEZONE format in the VCALENDAR declaration/header

Am I understanding you correctly?

If so, I'm OK with that, except for "a correct mapping from a windows timezone to an iana one e.g. from W. Europe Standard Time to Europe/Berlin". This mapping falls outside the purview of what ical.net is meant to be, and is more in line with NodaTime's goals.

How about this instead?

Produce a manifest of time zone change information ("adjustment rules" in Noda Time parlance) for the time zone that the user specifies, serialized in VTIMEZONE format.

The net effect of this would be:

  • Western European Summer Time would use NodaTime to look up the adjustment rules for the WEST time zone, and provide them in standard VTIMEZONE format
  • Europe/Berlin would use NodaTime to look up the adjustment rules, and provide them in standard VTIMEZONE format
  • If NodaTime happens to do the WEST -> Europe/Berlin translation behind the scenes, so be it, but ical.net will not enforce the mapping.

I trust the maintainers of Noda Time to do a better job than I could do.

What do you think?

@aluxnimm
Copy link

I agree and we can do the mapping ourselves no problem.

Your proposal would be awesome for us. But I am not quite sure if and how NodaTime exposes all necessary data via its Api for generating the VTIMEZONE since it keeps the tz data in some internal format.

Meanwhile I download the tzurl.org ics files as workaround and cache them locally.

@rianjs
Copy link
Collaborator

rianjs commented Aug 28, 2016

I wrote a small proof of concept for historical data before I replied. ;)

var eastern = DateTimeZoneProviders.Bcl["Eastern Standard Time"];
var ny = DateTimeZoneProviders.Tzdb["America/New_York"];

var start = Instant.FromDateTimeOffset(new DateTimeOffset(1970, 1, 1, 0, 0, 0, TimeSpan.Zero));
var end = Instant.FromDateTimeOffset(DateTimeOffset.UtcNow);

var nyRules = ny.GetZoneIntervals(start, end);
var easternRules = eastern.GetZoneIntervals(start, end);

These rules are different until ~1987, when they suddenly converge for this time zone. (I'm sure each zone is different.) So it's possible to get a manifest of adjustment rules.

Obviously this won't work right now, as this feature doesn't exist, but I think I think your application code should look fairly simple like this:

var calendar = new Calendar();
calendar.IncludeTimeZoneManifest = true;    //Default = false, as VTIMEZONE is optional
calendar.Add(eventA);
calendar.Add(eventB);

During serialization, ical.net would look for the adjustment rule that starts just before the earliest event's starting date-time, and serializes all the ZoneInfo objects until the last recurrence ends, or until we have no more adjustment rules to serialize.

@aluxnimm
Copy link

How can you generate proper RRULEs from that transition data when you don't know the underlying rule like last sunday in march?

@aluxnimm
Copy link

Similar issue how to generate VTIMEZONE from transition data is discussed in
https://github.com/fruux/sabre-vobject/issues/44

@rianjs
Copy link
Collaborator

rianjs commented Oct 3, 2016

So I don't view this as low priority anymore. It seems reasonable to be able to emit VTIMEZONE information upon serialization. That said:

  • It's not low priority, but the level of effort required is non-trivial. So I don't know when it will get done. I would be very happy if someone else was motivated to take this on. ical.net is a side project -- I don't get paid to work on it, except where its deficiencies overlap with making progress on paid work. So if one of you can make a credible argument for working on this as part of your day job, go for it. :)
  • ical.net will continue to ignore VTIMEZONE information when doing conversions. Microsoft's own information is low-resolution, and only goes back to 2010. Noda Time provides a better (and faster) way to these computations, so ical.net will continue to delegate to that library.

@codepwner
Copy link

This issue is affecting me as well. I was using DDay for an organization largely Outlook based. When I converted to ICal.Net, suddenly all my REQUEST events became "not supported calendar" and when I inspected the actual file, it seems the only difference is the BEGIN:STANDARD ... END:STANDARD block.

Is there any workaround for this at the moment?

image

@codepwner
Copy link

I have found a workaround which actually makes me happier than the existing DDay.ICal version...

Existing Solution:
var eventStub = new Ical.Net.Event
{
Start = new CalDateTime(StartSiteLocal),
End = new CalDateTime(EndSiteLocal)
}
var iCal = new Calendar();
iCal.Events.Add(eventStub);
iCal.AddTimeZone(CrossSite.Site[Site].NodaTimeZone);

This would not work with Outlook because the BEGIN:STANDARD / DAYLIGHT section was eliminated.

I attempted to use UTC as the timezone without success. I tried no timezone and Outlook somewhere assumed not UTC nor my local timezone, but PST.

Then I found out the start / end times can be set to UniversalTime themselves which adds a Z to the datetime property in the ICAL.

Working Code:
var eventStub = new Ical.Net.Event
{
Start = new CalDateTime(StartSite),
End = new CalDateTime(EndSite)
}
eventStub.Start.IsUniversalTime = true;
eventStub.End.IsUniversalTime = true;

@aluxnimm
Copy link

This can be a workaround for some cases but not for recurring events which should start for example every week Monday at 10am and span over DST changes. Such information can't be represented when converting all times to UTC.

@ScarlettCode
Copy link

Hi,

Let me know if I should open a new issue for this..

Nuget : Ical.Net 2.3.0

When using the basic example in the wiki but leaving out the recurrence rule so just having a single event with a start and end time Outlook 2016 throws an error when syncing saying that the VEVENT references an undeclared time zone with TZID of UTC and its found an approximate replacement.

It still adds the event to the calendar but the error means I can't use it in production. If I export the calendar I've just added the ics file Outlook gives me (after its modification) has a Z after the time instead of putting TZID=UTC in front of the time. #263 is a breaking change for Outlook 2016

Using @codepwner work around no longer seems to work as that also puts the TZID=UTC in front of the time.

I've rolled back to 2.2.19 and that allows me to use the .IsUniversalTime = true work around. Are there any current solutions to get things working with Outlook 2016 and 2.3.0?

@rianjs I'm going to pull down the source and start getting myself familiar with it and then I'd be interested in helping solve this issue. I know there are many different thoughts on what's the best way forward so maybe you could outline what you want to happen and I can start working on getting it done?

Thanks

@rianjs
Copy link
Collaborator

rianjs commented May 4, 2017

@ScarlettCode - I think your comment belongs in #263, can you repost it there instead?

@beriniwlew
Copy link
Collaborator

I'm going to start working on this.

@beriniwlew
Copy link
Collaborator

beriniwlew commented Jun 29, 2017

Here's what I've got so far:
https://github.com/beriniwlew/ical.net

Still very alpha-ish, so I wouldn't use this in production.

@programcsharp
Copy link

I tried to switch over from a (very very old) version of DDay.iCal today and ran smack into this issue.

A lot of apps and all of the validators look for the timezone. I agree that its better to interpret directly on parse, but on render it needs to have that output to make everything happy.

Great work on this so far, @beriniwlew! I think this is the last thing needed to be able to upgrade. I fixed a couple of long standing casing issues in another area, so hopefully with the timezone it'll be fully compliant.

I did attempt your branch in production, despite all warnings to the contrary 😬 , but hit a lookup issue:

image

@beriniwlew
Copy link
Collaborator

beriniwlew commented Jul 13, 2017

@programcsharp Honestly, I'm thinking about redoing what I did.

I don't think it should be up to us to try to generate the STANDARD/DAYLIGHT values from NodaTime's info. I've found that Apple's ics repository contains far more tz information than it already.

It seems that it already in the proper format, too. Perhaps we could get the right VTIMEZONE info by deserializing from these existing ics files.

What do you think?

@programcsharp
Copy link

programcsharp commented Jul 13, 2017

TBH, I'm not as worried about the exactness of the data as I am with generating RFC valid iCal files. I'm only generating events in the future, and mostly in major time zones that don't have arcane rules. And as @rianjs said, anybody doing this right should probably use their own database for comparisons anyway.

My main goal is to be able to render an iCal flagged with a timezone and not have it kicked back by validators or apps/sites that are sticklers for the offset being rendered if timezone is specified.

If I was designing it, I'd recommend to make it pluggable and provide a default implementation using TimeZoneInfo.GetAdjustmentRules(). That would get to compliant first off and be simple.

Take your TimeZoneInfo component, copy in the latest DDay.iCal FromSystemTimeZone implementation, and that's a v1. Maybe provide an empty implementation as well, in case anybody's relying on the current behavior.

Then as a v2, you could do either a NodaTime provider or one based on the Apple db. Or even both.

Thoughts?

@beriniwlew
Copy link
Collaborator

What TZID did you have an issue with, by the way?

@programcsharp
Copy link

programcsharp commented Jul 13, 2017

MST (Phoenix/Arizona) -- we don't have daylight savings, so dunno if that's what affected it.

EDIT: Or maybe it's because there are no adjustments in the db? DDay.iCal has a check for that where it adds in a dummy record:

            // If no time zone information was recorded, at least
            // add a STANDARD time zone element to indicate the
            // base time zone information.
            if (dday_tz.TimeZoneInfos.Count == 0)
            {
                var dday_tzinfo_standard = new DDay.iCal.iCalTimeZoneInfo();
                dday_tzinfo_standard.Name = "STANDARD";
                dday_tzinfo_standard.TimeZoneName = tzinfo.StandardName;
                dday_tzinfo_standard.Start = earliest;                
                dday_tzinfo_standard.OffsetFrom = new UTCOffset(utcOffset);
                dday_tzinfo_standard.OffsetTo = new UTCOffset(utcOffset);

                // Add the "standard" time rule to the time zone
                dday_tz.AddChild(dday_tzinfo_standard);
            }

Here's what it looked like from DDay.iCal before I ported to ical.net:

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//ddaysoftware.com//NONSGML DDay.iCal 1.0//EN
BEGIN:VTIMEZONE
TZID:US Mountain Standard Time
BEGIN:STANDARD
DTSTART;VALUE=DATE:20160101
TZNAME:US Mountain Standard Time
TZOFFSETFROM:-0700
TZOFFSETTO:-0700
END:STANDARD
END:VTIMEZONE

@beriniwlew
Copy link
Collaborator

@programcsharp Check my fork now

@programcsharp
Copy link

programcsharp commented Jul 14, 2017 via email

@beriniwlew
Copy link
Collaborator

I think I've gone about as far as I can without a review.

I've made a pull request (#304), so let me know what to fix over there.

@rianjs
Copy link
Collaborator

rianjs commented Nov 13, 2017

Fixed in nuget version 4.0.0: https://www.nuget.org/packages/Ical.Net/4.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants