Keep apart VEVENT.DTEND and .DURATION

[minichma]

Fixes #574
Fixes #629

According to RFC 5545 VEVENT's properties DTEND and DURATION cannot be calculated from each other, because the evaluation rules are slightly different. The previous implementation of CalendarEvent extrapolated both values based on each other, and as a consequence remove the information, which of both was originally specified.

With this PR the extrapolation only happens on the fly when needed but only the originally specified property is stored. The additional information is not used for any calculations so far, but can in the future.

DISCUSS: This is a breaking change, as uses might expect DtEnd to be calculated from Duration and vice versa. An alternative would be to keep the behavior of the properties as is but calculate fallback on the fly when needed rather than upfront. That way the CalendarEvent.DtEnd and .Duration properties would behave as before but the serialized versions might still be slightly different. However, the behavior would be less transparent and potentially confuse new users. TBD

[axunonb]

Yes, I saw these calculations when reviewing the code for the v4.3 release. Following RFC 5545 is clearly the preferred way to go. We can expect more breaking changes, so we could also consider a temporary flag like v4-compatibility-enabled. This could be introduced at any time, and all new code could have RFC compliance in focus. v5 could then switch compatibility == false by default. What do you think?

There will be a new major version anyway, as soon as the project is migrated to NRT enabled. I can't foresee yet, how much time this will require and how much risks this will bring because of low code coverage in some areas.

[minichma]

Yes, a temporary flag would certainly be an option. Where would we put it? But on the other hand, this would introduce quite some additional complexity to the code and require a considerable amount of additional testing. As you intend to introduce a v5 soon anyways, it might also be a good idea to start collecting those breaking changes from now on. Similar code is contained in Period.cs, which seems to be related to #564 and should be improved as well. Keeping everything backwards-compatible might not be worth the effort.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[axunonb]

To setup a project and a new v5 branch looks like the way to go, I agree.

[minichma]

@axunonb While rebasing this PR I stumbled across following change, because the affected test fails now

https://github.com/ical-org/ical.net/pull/620/files#r1807942277

The problem is that cloning of the calendar events normalizes the ORGANIZER uri. As a consequence, in the affected test, if the event is cloned, it will be serialized to lower-case mailto:.... However, if it is not cloned, it will NOT be converted to lower-case. The reason for that behavior is, that in

https://github.com/axunonb/ical.net/blob/469c767a00d6e0d1c88bda8dd647ad8487f8f050/Ical.Net/DataTypes/Attendee.cs#L251

we create a copy of the Uri by applying a pattern of the form newUri = new Uri(oldUri.ToString()). The copy will not have the same OriginalString as the old uri.

Previously the calendar has always been copied during serialization due to the fact, that DTEND and DURATION have always both been present and due to this:

https://github.com/axunonb/ical.net/blob/fedbd4f10de072b69662fec40b14ec05681a9a61/Ical.Net/Serialization/EventSerializer.cs#L26

The question is now, what the expected behavior would be. From my perspective copying should not have any side effects, so we should just copy the reference to the URI rather than cloning the URI itself. Copying the reference is safe, as Uri is immutable. The question is, whether an implicit conversion from lower to upper-case should happen or not. In this case I'd say its the responsibility of the caller to provide clean input. I'd generally recommend keeping the original input rather than applying sanitization.

[axunonb]

copying should not have any side effects, so we should just copy the reference to the URI rather than cloning the URI itself

agree

I'd generally recommend keeping the original input rather than applying sanitization

agree

I remember this failing test, and obviously I was not diving deep enough. Thanks!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[axunonb]

Love it, no more ExtrapolateTimes, thanks!
There is a new branch version/v5 where this PR should get merged.