PoC: Allow any TimeZone implementation

[WhyNotHugo]

Make the TimeZone parametrised in as many places as possible. This allows using any TimeZone implementations for most of the logic. Most notably, this allows using chrono::Local.

Using chrono::Local makes it much easier to construct TimeZone instances from parsed VTIMEZONE components and to interoperate with other crates.

Some notes:

• There's a few new .clone() calls. This is because chrono_tz::Tz implements Copy, but other TimeZone implementation do not. In reality, these values were already being copied before anyway, but this was done implicitly.
• For error messages, to_string() cannot be used, since TimeZone does not implement Display. The implementations in the chrono package are the most obvious offenders here. Debug is now used instead for error messages.
• The validate_until validation is no longer called. It assumes that all RRules use the rrule::Tz TimeZone implementation. I don't see an obvious way to fix it, and this is still a pending item.

This is a proof of concept (which I wrote on a long train trip). Tests pass, but this probably warrants some extra discussion.

Fixes: #24

[fmeringdal]

Hey, thanks for the contribution!

I am a bit hesitant of introducing a generic type for RRuleSet for a couple of reasons:

• It forces DTSTART, UNTIL, RDATE and EXDATE values to all use the same timezone. The RFC actually says that UNTIL has to use UTC when DTSTART is not using local timezone. Adding more generic types could help, but it makes for a very bad API.
• I personally think adding a generic argument to the RRuleSet type makes the API a bit more awkward. The generic type propagates upwards to the parent struct unless the user know they will use a specific timezone everywhere. I imagine that just want to get date-times back with a timezone defined in the TZID parameter for DTSTART and that can't be defined at compile time.
• The RFC also says that the resulting occurrences from the iteration should be in the same timezone as what is used in the start date.
• The current rrule::Tz supports almost all relevant timezone formats and can easily be extended with more formats if needed. It is also interoperable with the timezones in both chrono and chrono-tz.

Overall it feels that it restricts a bit more than it enables. I am interested to hear more about your current interoperability issues with the current design as that might help me understand you use-case better. How about adding a all_with_timezone<Tz: TimeZone>(tz: Tz) -> DateTime<Tz> method?

[WhyNotHugo]

I'll be out this week so can't really reply in full detail (also replying from phone, sorry about any mistakes). As I mentioned on another issue: the biggest problem with the current API is that it assumes that TZID matches a timezone name form chrono_tz. The spec is very clear in that TZID isn't a reference to a global database. The grand majority of the samples I have (my personal and work calendars for over a decade) also don't fit this assumption. So while the idea of the current implement sounds nice, it doesn't follow the spec, nor work for real life examples. Only under 10% of the entries in my calendars seem to fit this assumption. Using chrono_tz simply isn't an approach that works here -- timezones need to be parsed from VTIMZONE components. It will work for entries created by this library, but that's not very interoperable. And parsing timezones exactly what I'm trying to do: I'm parsing VTIMEZONES and writing a VTimeZone type that basically implements TimeZone and can be used for things like RRULES. You are however, also right in that this approach is also wrong due to forcing DTSTART and UNTIL to use the same timezone. I'm not sure that two generic params would be a bad idea; as this patch shows, most cases can be inferred and don't need to be specified explicitly. I'm not sure what you mean by saying some timezones can't be specified at compile time. chrono_tz is unusual in that each TimeZone is defined at compile time, but this is not a requirement; and impl of TimeZone can read a text file at runtime and behave according to that input. That's literally what I'm doing, that text file being an icalendar component. The current rrule:Tz just supports some statically defined timezones. But not timezones defined at runtime or read from files. Most important, it CANNOT support timezones read from I calendar components as defined by the spec; it can only a behaviour that doesn't match the spec.

…

On Fri, 9 Sep 2022, at 21:34, Fredrik Meringdal wrote: Hey, thanks for the contribution! I am a bit hesitant of introducing a generic type for `RRuleSet` for a couple of reasons: * It forces `DTSTART`, `UNTIL`, `RDATE` and `EXDATE` values to all use the same timezone. The RFC actually says that `UNTIL` has to use `UTC` when `DTSTART` is not using local timezone. Adding more generic types could help, but it makes for a very bad API. * I personally think adding a generic argument to the `RRuleSet` type makes the API a bit more awkward. The generic type propagates upwards to the parent struct unless the user know they will use a specific timezone everywhere. I imagine that just want to get date-times back with a timezone defined in the `TZID` parameter for `DTSTART` and that can't be defined at compile time. * The RFC also says that the resulting occurrences from the iteration should be in the same timezone as what is used in the start date. * The current `rrule::Tz` supports almost all timezone formats that are being used and can easily be extended with more formats if needed. Overall it feels that it restricts a bit more than it enables. I am interested to hear more about your current interoperability issues with the current design as that might help me understand you use-case better. How about adding a `all_with_timezone<Tz: TimeZone>(tz: Tz) -> DateTime<Tz>` method? — Reply to this email directly, view it on GitHub <#85 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFSNO5MWOBPFO4O4FIDHZTV5OGNZANCNFSM6AAAAAAQIBYKBI>. You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Hugo Osvaldo Barrera

[WhyNotHugo]

BTW, for larger context, I'm writing a small tool that parses icalendar entires and shows a notification at times specified by alarms in them. https://git.sr.ht/~whynothugo/valarmd I've been working with icalendar stuff for years (eg: todoman and various contributions here and there). My usually approach is to follow the spec, but also test with my personal calendar (which has accumulated all sorts of events during many years, from all sorts of sources; appointments, meetings, different apps I've used, etc). This ensures that not only strictly standard things work, but also things that diverge slightly (which are too common). The idea that TZID matches a name in the TZ database is common, but regrettably not correct. In particular, when looking at entries in my calendar, already the second entry is an exception: it has TZID:GMT-0300, but actually defines the timezone for Argentina with its DST transitions. Anything short of parsing the the VTIMEZONE will yield bogus results. I do WISH that TZID matched a simple DB, but it's just not the case, and for compatibility's sake, we can't assume that'll ever be the case either.

[fmeringdal]

Hey again, I didn't forget about this. Quite busy times these days, but I will try to get back to you within the next week :)

[WhyNotHugo]

I kinda figured out that std::any could work for the validate_until impementation, but how now hit a bigger roadblock with RDATE.

Checking the spec, I don't see any restrictions on the timezone's for RDATE. Apparently, an RDATE for an RRULE can use any timezone, so the same recurrence rule might include recurrence dates with UTC, floating times, and a VTIMEZONE. I would really really like to be wrong here, but I checked the spec twice and don't see any restrictions on this.

With the approach in this PR, I don't think it's possible to represent this, since we'd need to keep a Vec<DateTime<_>>, where each DateTime instance can have a different generic type. Using rrule::Tz works around this, although it would need to be extended with a new enum variant like Tz::VTimeZone in order to actually use the timezones defined in the icalendar object.

I checked my personal calendar and only found RDATE used inside VTIMEZONE definitions. The offset for these DATE-TIMEs is defined via the TZOFFSETFROM property, and this currently cannot be represented via rrule::Tz either. A new variant (e.g.: Tz::FixedOffset) can cover this too.

In order to extend rrule::Tz with a new VTimeZone variant, its associated type (e.g.: struct VTimeZone) would need to be a bulid dependency of this crate. However, parsing and instantiating these VTimeZone instances requires an rrule implementation. Due to this circular dependency, I don't see how the VTimeZone type and the RRule type can be implemented in different crates. For RRULES components inside VEVENTS and VTODOS (e.g.: RRULEs NOT in a VTIMEZONE component), there's a strong dependency on a VTIMEZONE implementation anyway in order to calculate recurrence.

[fmeringdal]

I see, thanks for describing you use-case so well. I am by no means any timezone expert and was under the impression that using timezones found in DB was "good enough", but apparently it is not sufficient as you have pointed out.

My previous point about the generic types on RRule making for an awkward API is also not quite true as the types could default to rrule::Tz and become invisible for users who don't care about them. I assume there is still a bit of exploration that needs to done in this PR to find a good ergonomic API while at the same time being mostly compliant with the RFC (we will probably never be fully compliant nor do I aim for that).

How is the current implementation in this PR working for your personal calendar? Is it able to process most of your entries or is there still a bunch of entries that does not work?

[WhyNotHugo]

This kinda works for my calendar (though I have several bits of the VTimeZone impl still incomplete). This PR enables using that custom VTimeZone impl, but has another issue:

In particular, RDates can have different timezones, so we'd need to keep a Vec<DateTime> with mixed timezones -- and this can only be done using an enum TimeZone like the one being used right now (discussion on chrono on this topic).

As such, I don't think it's a good idea to merge this (hence marking as draft). I think moving VTimeZone into this repo is the only suitable way to address the whole issue, because RRuleSet depends on VTimeZone, but VTimeZone depends on RRuleSet.

Essentially, all enum variants would be:

pub enum Tz {
    /// Floating (e.g.: no timezone).
    Local(Local),
    /// Timezone represented by `chrono_tz::Tz` (mostly for _creating_ instances and backwards compat).
    Tz(chrono_tz::Tz),
    // A timezone read from an icalendar object's VTIMEZONE component.
    VTimeZone(VTimeZone),
}

[reedts]

Any developments/news on this? Would be super cool if there was a way to make this crate work with other, generic TimeZone implementations!

[WhyNotHugo]

@reedts: This particular details is a blocker for this to move forward:

In particular, RDates can have different timezones, so we'd need to keep a Vec with mixed timezones -- and this can only be done using an enum TimeZone like the one being used right now (chronotope/chrono#822).

There's some exploratory work for that in chronotope/chrono#830, but that's not a minor undertaking TBH, and I don't fully recall where discussions left off.

[reedts]

@WhyNotHugo thank you for the headsup.

I myself see no way around implementing VTimeZone in this crate either, for the reasons you already mentioned. Seems not that easy :/...

[WhyNotHugo]

Indeed. Although only the VTimeZone type itself needs to be in this crate; logic to parse that from a raw icalendar component can be external.

I'm currently focused on some caldav-related work, but I want to jump onto this in future. I have in mind writing a SAX-like parser fro icalendar, so after I have that I might start work on parsing VTIMEZONE and eventually get to this.

[WhyNotHugo]

I'll go ahead and close this. This approach might not be feasible at all, and #24 exists to continue discussion anyway.