bug#34315: [PATCH] icalendar.el: DURATION fix + more robust timezone handling

[Lars Ingebrigtsen]

Thomas,
the patch looks good so far. Could you please provide some testcases so
that I can add some unit tests?

This was over a year ago.

As far as I understand it, the patch itself is good and fixes a real
problem in icalendar date parsing? But then the thread turned towards
questions about daylight savings time handling in Windows, and the patch
itself wasn't applied?

[Ulf Jasper]

It seems that I lost track of this issue. Sorry.

Post by Lars Ingebrigtsen
As far as I understand it, the patch itself is good and fixes a real
problem in icalendar date parsing?

IIRC the patch looked good insofar as it appeared to address only the
observed problem and to be properly documented and formatted although I
was not able to reproduce the problem.

Post by Lars Ingebrigtsen
But then the thread turned towards questions about daylight savings
time handling in Windows, and the patch itself wasn't applied?

Right, the patch was not applied.

Eli pointed out that the root cause lies in the dst handling in
ms-windows (and not in newsticker). That makes the patch more of a
work-around than a fix.

I admit that I am not very much inclined to check and apply the patch as
I do not have a ms-windows box to run tests on.

Eli, what do you think?

[Eli Zaretskii]

Date: Mon, 10 Aug 2020 18:08:34 +0200
IIRC the patch looked good insofar as it appeared to address only the
observed problem and to be properly documented and formatted although I
was not able to reproduce the problem.

Post by Lars Ingebrigtsen
But then the thread turned towards questions about daylight savings
time handling in Windows, and the patch itself wasn't applied?

Right, the patch was not applied.
Eli pointed out that the root cause lies in the dst handling in
ms-windows (and not in newsticker). That makes the patch more of a
work-around than a fix.
I admit that I am not very much inclined to check and apply the patch as
I do not have a ms-windows box to run tests on.
Eli, what do you think?

My comments were not about the patch (which I admit I don't really
understand, as I don't use icalendar), but about the attempts to see
what happens with time under different TZ settings.

As for the patch, I've looked at it again now, and I don't think it's
specific to MS-Windows, is it? If so, and if you think it's good,
feel free to install.

Thanks.

[Thomas Plass]

Eli Zaretskii wrote at 19:45 on August 10, 2020:
:
: As for the patch, I've looked at it again now, and I don't think it's
: specific to MS-Windows, is it?

Exactly, the patch has nothing to do with MS-Windows' DST handling
(though incidentally, it was developed while dealing with that issue).

It fixes general issues with icalendar.el's handling of timezones as
commonly used in ical data and the standard tzurl.org repo. It is
also completely backwards-compatible as far as I can make out.

Here's the README for the testcases I provided to Ulf along with the patch:

This ZIP contains a patch for icalendar.el 0.19 and a set of iCalendar test files.
VTIMEZONE sections contained therein were retrieved from http://tzurl.org.

- Europe_Berlin-20181103T201500_no_in-calendar_VTIMEZONE.ics
no VTIMEZONE, so no TZID references for DTSTART/DTEND, contains
non-standard property X-WR-TIMEZONE which is invisible to icalendar.el

- Europe_Berlin-20181103T201500_in-calendar_VTIMEZONE_tzurl_standard.ics
standard VTIMEZONE ("Outlook-style")

- Europe_Berlin-20181103T201500_in-calendar_VTIMEZONE_tzurl_historical.ics
comprehensive VTIMEZONE including historical records

- America_Creston-20181103T121500_in-calendar_VTIMEZONE_tzurl_standard.ics
standard ("Outlook-style") VTIMEZONE

- America_Creston-20181103T121500_in-calendar_VTIMEZONE_tzurl_historical_RDATE.ics
comprehensive VTIMEZONE including historical records which use RDATE, not RRULE

[Lars Ingebrigtsen]

Post by Thomas Plass
It fixes general issues with icalendar.el's handling of timezones as
commonly used in ical data and the standard tzurl.org repo. It is
also completely backwards-compatible as far as I can make out.

Can you respin the patch and add the test cases to
test/lisp/calendar/icalendar-tests.el? The icalendar test files should
go to a new test/data/icalendar directory, I guess.

[Ulf Jasper]

Post by Lars Ingebrigtsen

Post by Thomas Plass
It fixes general issues with icalendar.el's handling of timezones as
commonly used in ical data and the standard tzurl.org repo. It is
also completely backwards-compatible as far as I can make out.

Can you respin the patch and add the test cases to
test/lisp/calendar/icalendar-tests.el? The icalendar test files should
go to a new test/data/icalendar directory, I guess.

IIRC icalendar-tests.el does not make use of the testdata directory.

Thomas, could you please provide the expected results for the test files,
one for each ics file? Thanks!

[Lars Ingebrigtsen]

Post by Ulf Jasper
IIRC icalendar-tests.el does not make use of the testdata directory.

No, but I think it should. :-) At least when adding new tests -- it's
easier to be sure that we're really testing the right thing when we've
not stringified the icalendar data into a Lisp string...

[Ulf Jasper]

Post by Lars Ingebrigtsen

Post by Ulf Jasper
IIRC icalendar-tests.el does not make use of the testdata directory.

No, but I think it should. :-) At least when adding new tests -- it's
easier to be sure that we're really testing the right thing when we've
not stringified the icalendar data into a Lisp string...

Agree. icalendar-tests are hard to read. sigh.

[Lars Ingebrigtsen]

Attached is an updated ZIP containing the respun patch and the
unmodified samples. The patch is against
https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/calendar/icalendar.el
blob: d76c11050312b4a04ac1cbda436b3c08fc6f2cc5
Hopefully, this is OK as it is.

It would be better to add the test cases in the test/data directory and
add the tests as code to icalendar-tests.el...

: Thomas, could you please provide the expected results for the test files,
: one for each ics file? Thanks!
- The local timezone of the person running the code. Where I'm
sitting, it is November 3, 2018 20:15h for all samples.

(etc)

This is why the test cases should bind the time zone to whatever it is
it's testing -- that way we can easily ensure that the code continues to
work.

[Thomas Plass]

Lars Ingebrigtsen wrote at 15:12 on August 12, 2020:
:
: It would be better to add the test cases in the test/data directory and
: add the tests as code to icalendar-tests.el...

Certainly. But apparently I'm too stupid to find that file and the
test/data dir. Can someone send it to me? As I have no commit
privileges and also am unable to build Emacs, I haven't checked out
the git. So if the maintainer(s) could assist or extend the tests,
I'd be grateful.

[Ulf Jasper]

Post by Thomas Plass
: It would be better to add the test cases in the test/data directory and
: add the tests as code to icalendar-tests.el...
Certainly. But apparently I'm too stupid to find that file and the
test/data dir. Can someone send it to me? As I have no commit
privileges and also am unable to build Emacs, I haven't checked out
the git. So if the maintainer(s) could assist or extend the tests,
I'd be grateful.

Don't worry. I shall write or enhance the tests for the
icalendar-functions that you changed. I shall also write tests that
check import of the ics files that you supplied.

[Ulf Jasper]

Just pushed two commits containing 1) unit tests and 2) your patch and
more tests.

I had to make a small change in the patched version of
`icalendar--decode-isodatetime' regarding input data in utc zone.
Please have a look.

[Thomas Plass]

Thanks, approved. Defaulting to local time when decoding seems
reasonable.

Ulf Jasper wrote at 20:05 on September 2, 2020:
: I had to make a small change in the patched version of
: `icalendar--decode-isodatetime' regarding input data in utc zone.

[Ulf Jasper]

Thomas,

after committing your changes I learned that your contribution counts as
a non-trivial change that is sufficiently large to make a copyright
assignment necessary.

So, would you please assign the copyright for your contribution(s) to
the FSF?

In order to do this, please ask on emacs-***@gnu.org, and we will send
you the necessary form together with the instructions to fill and email
it, in order to start this legal paperwork.

I should have asked you to assign copyright before I incorporated your
changes. My mistake. Sorry!

All the best,
Ulf