Fix: Calculation for occurrences include DST transitions correctly #673
Conversation
Updated `CalendarEvent` and `EventEvaluator` to enhance event handling, particularly around time zones and daylight saving time transitions. Key changes include: - Enabled nullable reference types in `CalendarEvent` and `EventEvaluator.` - Updated `CalendarEvent` to throw `InvalidOperationException` when trying to set both `DtEnd` and `Duration`. - Event end time and duration calculations in `EventEvaluator` reflect any DST transitions. - Added new test class `RecurrenceTests_From_Issues` to validate changes with various edge cases and scenarios. Resolves ical-org#660 Resolves ical-org#623 Resolves ical-org#671 Resolves ical-org#567 Resolves ical-org#342 Resolves ical-org#298
axunonb
force-pushed
the
pr/first-event-duration
branch
from
552bdde to
3da388c
Compare
| // and use the timezone from the event's definition. | ||
| endTime = new CalDateTime( | ||
| DateOnly.FromDateTime(endDt), | ||
| period.StartTime.HasTime ? TimeOnly.FromDateTime(endDt) : null, |
There was a problem hiding this comment.
Here we risk confusing the tzid. Simply use var endDt = period.StartTime.Add(nominalDuration)? We must make sure that Add() and GetFirst..() both operate on the same kind of duration, nominal vs exact.
There was a problem hiding this comment.
Oh, I guess I missed your point. Of course, you apply the same 'nominal' duration (not sure whether this is the right term if the tz is different) and preserve the tz. So in the end the exact duration is close to the original one (except for the DST changes). Yeah, I guess this is a reasonable approach, but I would still discourage implementing it like that. The RFC requires applying the nominal duration only if specified via the DURATION property, in which case there is only a single tz. I guess this is due to the ambiguity that comes from comparing times in different zones. Anyhow, to my understanding there's no hint in the RFC of how to apply 'nominal' durations in case of different tz.
I therefore keep my suggestion to calculate in exact durations for now until we precisely implement keeping both apart (in which case we also don't have to deal with this ambiguity). This way we are not fully precise if a duration is specified via the DURATION property, but at lease if specified via DTEND. In the way it's implemented in the PR we could be off in both cases.
|
Thanks for the review and valuable comments. Timezone-aware occurrencesI agree that the solution for determining the end time of the period is unwieldy or unintuitive. DurationWith regard to duration, we could create a new issue #675 now, which would then be processed in a separate PR. Refactored
|
`CalendarEvent` - Renamed `CalcFirstNominalDuration` to `GetTimeSpanToAddToPeriodStartTime`. The logic remains unchanged. `Period` - Removed private `ExtrapolateTimes()` method - Enabled nullable reference types - Refactored constructors and properties - Added `GetOriginalValues` method - Enhanced documentation and exception handling - Fixed bug in `CollidesWith(Period)` - Added PeriodTests.cs to cover unit tests for `Period` class. - Shortcoming: `Period.Duration` may still implicitly return a nominal or an exact duration.
axunonb
force-pushed
the
pr/first-event-duration
branch
from
8e1b611 to
494be88
Compare
|
Updated
CalendarEventandEventEvaluatorto enhance event handling, particularly around time zones and daylight saving time transitions.Key changes include:
CalendarEventandEventEvaluator.CalendarEventto throwInvalidOperationExceptionwhen trying to set bothDtEndandDuration.EventEvaluatorreflect any DST transitions.RecurrenceTests_From_Issuesto validate changes with various edge cases and scenarios.Resolves #660
Resolves #623
Resolves #671
Resolves #567
Resolves #342
Resolves #298