GNU bug report logs - #74994
Improve Emacs iCalendar support

Previous Next

To reply to this bug, email your comments to 74994 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Improve Emacs iCalendar support
Date: Fri, 20 Dec 2024 14:07:22 +0100

Severity: wishlist

As discussed already a bit on emacs-devel, here:

https://lists.gnu.org/archive/html/emacs-devel/2024-10/msg00425.html

and in a write-up I posted here:

https://recursewithless.net/emacs/icalendar-parser-and-mode.org

I would like to see Emacs gain an updated, full-fledged implementation
of RFC5545, the current version of the iCalendar standard.

I have been working on this for a couple of months, and have some code
that's ready to be reviewed and discussed. I'm creating this bug to
track that discussion.

Message #8 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: 74994 <at> debbugs.gnu.org
Subject: [PATCH 1/2] New parser for iCalendar (RFC5545)
Date: Fri, 20 Dec 2024 20:47:48 +0100

[Message part 1 (text/plain, inline)]

Tags: patch

Here is a draft patch implementing a new parser for iCalendar data. This
code implements the grammar of RFC5545, functions to parse this grammar
to an abstract syntax tree, functions to validate syntax trees,
functions to print syntax trees, and a test suite for the parser and
printer functions containing all the examples from RFC5545.  The code is
organized as follows:

lisp/calendar/icalendar-ast.el: defines the abstract syntax tree,
  including the validation functions
lisp/calendar/icalendar-macs.el: defines the icalendar-define-param,
  icalendar-define-property, and icalendar-define-component macros
lisp/calendar/icalendar-parser.el: defines the parsing and printing
  functions, and all of the individual parameters, properties, and
  components defined in the RFC.
test/lisp/calendar/icalendar-parser-tests.el: the test suite.
  All the tests pass on my machine with Emacs 29.1 and with Emacs master.

Looking forward to your feedback! This is a (very?) large patch, so
please let me know if it would be better to submit it another way.

Thanks,
Richard

[0001-New-parser-for-RFC5545.patch (text/patch, attachment)]

Message #11 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: 74994 <at> debbugs.gnu.org
Subject: [PATCH 2/2] New major mode icalendar-mode
Date: Fri, 20 Dec 2024 20:53:49 +0100

[Message part 1 (text/plain, inline)]

Tags: patch

Here's a second patch which uses the parser provided by the last
patch to implement a new major mode, icalendar-mode, which for now just
provides syntax highlighting and some basic commands for folding and
unfolding lines.

Again, this is a draft; there's still plenty to be done. I'm looking
forward to your feedback.

Thanks,
Richard

[0002-New-major-mode-icalendar-mode.patch (text/patch, attachment)]

Message #14 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Jean Louis <bugs <at> gnu.support>
To: Richard Lawrence <rwl <at> recursewithless.net>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Mon, 23 Dec 2024 09:33:36 +0300

Is there no package that can accept structure and convert to
iCalendar?

-- 
Jean Louis

Message #17 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: Jean Louis <bugs <at> gnu.support>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Mon, 23 Dec 2024 09:43:32 +0100

Jean Louis <bugs <at> gnu.support> writes:

> Is there no package that can accept structure and convert to
> iCalendar?

Not that I was able to find after looking several times, and rather
extensively. I was surprised too.

Anyway, as I said in my initial post, I think that a more robust
iCalendar implementation belongs in Emacs core. There are already
*three* partial implementations in Emacs, but they are all incomplete
and support different applications (Gnus, Org, and diary). The reason
for this, I believe, is that the current icalendar.el is not well
documented and somewhat difficult to extend. The idea here is to provide
a library that all three of these applications, as well as third party
packages, can more easily use.

Best,
Richard

Message #20 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Jean Louis <bugs <at> gnu.support>
To: Richard Lawrence <rwl <at> recursewithless.net>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Mon, 23 Dec 2024 17:49:20 +0300

* Richard Lawrence <rwl <at> recursewithless.net> [2024-12-23 11:44]:
> Anyway, as I said in my initial post, I think that a more robust
> iCalendar implementation belongs in Emacs core. There are already
> *three* partial implementations in Emacs, but they are all incomplete
> and support different applications (Gnus, Org, and diary). The reason
> for this, I believe, is that the current icalendar.el is not well
> documented and somewhat difficult to extend. The idea here is to provide
> a library that all three of these applications, as well as third party
> packages, can more easily use.

I find iCalendar for compatibility with other software very useful.

I don't know how to apply your patch. Can you tell me step by step?

I am using development Emacs version, is patch for it?

-- 
Jean Louis

Message #23 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: Jean Louis <bugs <at> gnu.support>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Tue, 24 Dec 2024 09:08:18 +0100

Jean Louis <bugs <at> gnu.support> writes:

> I am using development Emacs version, is patch for it?

Yes, it was generated against Emacs master (commit 07cc8abca75 at the
time I generated the patches).

> I don't know how to apply your patch. Can you tell me step by step?

To be honest I'm new to the patch workflow myself but it should be:

  git apply path/to/the/patch

run from the repository root, on the master branch. If you want to try
out icalendar-mode, do this for both patches in succession (the second
patch, which provides icalendar-mode, requires the code introduced in
the first).

To run the test suite and verify that it works: make check

(Please let me know here if you see any errors in
test/lisp/calendar/icalendar-parser-tests.log)

There aren't many user-facing functions yet, but if you want to test the
code with some iCalendar data you have in file, do M-x find-file
path/to/file; activate icalendar-mode if it doesn't activate
automatically; say "y" to unfold lines if asked (Note: there is
currently a bug where you may get asked multiple times; if you've
already got the data in an unfolded buffer and get asked again, you can
say "n"; I still need to look into this); and then, in the unfolded
buffer, try calling functions like this (e.g. with M-:): 

(icalendar-parse-component (point-max)), with point at the start of a
"BEGIN:..." line, e.g. at the start of BEGIN:VCALENDAR if you want to
parse a whole calendar.

(icalendar-parse-property (line-end-position)), with point at the start
of a property line 

If you get any parse errors, please let me know here!

Best,
Richard

Message #26 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: Ihor Radchenko <yantar92 <at> posteo.net>
Cc: 74994 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: Upstreaming org-element-ast (was: Improving Emacs' iCalendar
 support)
Date: Sun, 29 Dec 2024 21:19:15 +0100

Ihor Radchenko <yantar92 <at> posteo.net> writes:

> In bug#74994, Richard made a decision not to use org-element-ast and
> instead implement a custom parser generator.
>
> Richard, is there any specific reason why you had to make things from
> scratch? May org-element-ast be changed to fit your needs?
>
> If org-element-ast is not going to be useful outside Org mode, I see no
> good reason to invest time into upstreaming it, after all.

No, it was more that, as things stood, I wanted to wait and see what
would happen with upstreaming org-element-ast. My idea was to make it
easy to switch once that happened, but not to wait to make progress in
the meantime, that's all.

One thing about your question confuses me, namely: 

> ...instead implement a custom parser generator.

As I understand org-element-ast, it basically just defines the parse
tree representation and various accessors for working with it, not the
parser itself. Was your suggestion that I could also use the Org parser,
not just the parse tree representation? If so, then I misunderstood, and
presumably more code is involved than is found in org-element-ast.el,
right?

Best,
Richard

Message #29 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: Ihor Radchenko <yantar92 <at> posteo.net>
Cc: 74994 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: Upstreaming org-element-ast (was: Improving Emacs' iCalendar
 support)
Date: Sun, 29 Dec 2024 21:53:40 +0100

Richard Lawrence <rwl <at> recursewithless.net> writes:

> Ihor Radchenko <yantar92 <at> posteo.net> writes:
>
>> In bug#74994, Richard made a decision not to use org-element-ast and
>> instead implement a custom parser generator.
>>
>> Richard, is there any specific reason why you had to make things from
>> scratch? May org-element-ast be changed to fit your needs?
>>
>> If org-element-ast is not going to be useful outside Org mode, I see no
>> good reason to invest time into upstreaming it, after all.
>
> No, it was more that, as things stood, I wanted to wait and see what
> would happen with upstreaming org-element-ast. My idea was to make it
> easy to switch once that happened, but not to wait to make progress in
> the meantime, that's all.

Ah, looking over this again, there was one thing that I felt didn't fit
very well with org-element-ast: in iCalendar, "properties" can have both
a "value" and a list of "parameters". These are different types of
objects and it's most natural and useful not to lump them together, but
in the Org element AST they would probably both just be "contents" of a
property node (i.e., child nodes). Thus, in my draft icalendar-ast.el,
instead of using

(TYPE PROPERTIES CONTENTS)

as the basic representation for a node, I used

(TYPE META VALUE CHILDREN).

The org-element-ast representation is obviously capable of making the
distinction between values and parameters in some other way; but it's
pretty much the only example I can think of where I felt the
org-element-ast representation might not be the best for the needs of
iCalendar.

Best,
Richard

Message #32 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Richard Lawrence <rwl <at> recursewithless.net>
Cc: 74994 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: Upstreaming org-element-ast (was: Improving Emacs' iCalendar
 support)
Date: Mon, 30 Dec 2024 17:16:11 +0000

Richard Lawrence <rwl <at> recursewithless.net> writes:

>> Richard, is there any specific reason why you had to make things from
>> scratch? May org-element-ast be changed to fit your needs?
> ...
> No, it was more that, as things stood, I wanted to wait and see what
> would happen with upstreaming org-element-ast. My idea was to make it
> easy to switch once that happened, but not to wait to make progress in
> the meantime, that's all.

org-element-ast is already a part of Emacs. The process of upstreaming
in this particular case is simply a question of (1) making the library
more useful outside Org mode; (2) renaming it.

Renaming is trivial.
The main question is making things usable outside Org mode.
And that's where your work is the most valuable.
So, it was me who is waiting for your input before upstreaming :)

> One thing about your question confuses me, namely: 
>
>> ...instead implement a custom parser generator.
>
> As I understand org-element-ast, it basically just defines the parse
> tree representation and various accessors for working with it, not the
> parser itself. Was your suggestion that I could also use the Org parser,
> not just the parse tree representation? If so, then I misunderstood, and
> presumably more code is involved than is found in org-element-ast.el,
> right?

No, I did not mean that you should use org-element to generate parser.
I mostly referred to the way you implement the parser where part of the
parser configuration is stored in the AST. But I was reading your patch
very quickly and could have misunderstood something.

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Message #35 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Richard Lawrence <rwl <at> recursewithless.net>
Cc: 74994 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: Upstreaming org-element-ast (was: Improving Emacs' iCalendar
 support)
Date: Mon, 30 Dec 2024 17:18:01 +0000

Richard Lawrence <rwl <at> recursewithless.net> writes:

> Ah, looking over this again, there was one thing that I felt didn't fit
> very well with org-element-ast: in iCalendar, "properties" can have both
> a "value" and a list of "parameters". These are different types of
> objects and it's most natural and useful not to lump them together, but
> in the Org element AST they would probably both just be "contents" of a
> property node (i.e., child nodes). Thus, in my draft icalendar-ast.el,
> instead of using
>
> (TYPE PROPERTIES CONTENTS)
>
> as the basic representation for a node, I used
>
> (TYPE META VALUE CHILDREN).
>
> The org-element-ast representation is obviously capable of making the
> distinction between values and parameters in some other way; but it's
> pretty much the only example I can think of where I felt the
> org-element-ast representation might not be the best for the needs of
> iCalendar.

Have you looked into secondary nodes?

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Message #38 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Acknowledgement (Improve Emacs iCalendar support)
Date: Sun, 19 Jan 2025 08:59:30 +0100

Dear Emacs maintainers,

I know this bug was opened right before the holidays, and it's a big
patch, so I understand if no one's had time to look at it yet. Before I
put any more work into this, though, it would useful if someone could
just give this a quick look and tell me if it seems like I'm headed in
the right direction: a "yes, this looks reasonable, let us know when
you're finished" or a "there's no way this will get merged, please start
over with a new approach" would be helpful feedback to have at this
point. (Of course, more detailed feedback is welcome too, if someone has
time.)

Thank you!

Best,
Richard

Message #41 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Lawrence <rwl <at> recursewithless.net>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Acknowledgement (Improve Emacs iCalendar support)
Date: Sun, 19 Jan 2025 11:21:53 +0200

> From: Richard Lawrence <rwl <at> recursewithless.net>
> Date: Sun, 19 Jan 2025 08:59:30 +0100
> 
> I know this bug was opened right before the holidays, and it's a big
> patch, so I understand if no one's had time to look at it yet. Before I
> put any more work into this, though, it would useful if someone could
> just give this a quick look and tell me if it seems like I'm headed in
> the right direction: a "yes, this looks reasonable, let us know when
> you're finished" or a "there's no way this will get merged, please start
> over with a new approach" would be helpful feedback to have at this
> point. (Of course, more detailed feedback is welcome too, if someone has
> time.)

You posted 2 patches, both of them quite large.  We don't have
icalendar experts on board whom we could ask to review the patches.
Are there any specific aspects of the patches you'd like us to look
into, or some specific design issues you'd like to discuss?  That'd
make the review more focused.

I can see already that your doc strings are not always according to
our conventions (the first line must be a single complete sentence),
and also the commit log messages need to be more detailed.  But these
are aspects best deferred until you have a version close to the final
one, now is too early for that.

One other comment is to be sure the many cl-isms you use don't get in
the way of debugging the code, since Edebug is known to provide weaker
support for some cl-lib construct and cl-macs macros.  So if you find
some macros or constructs that would make debugging harder, and can
replace them with simpler code without sacrificing too much, that
would be appreciated.

Other than that, please help us provide more focused review by
pointing out the issues and aspects for which you'd like our feedback.

Thanks.

Message #44 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Acknowledgement (Improve Emacs iCalendar support)
Date: Sun, 19 Jan 2025 21:24:04 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Richard Lawrence <rwl <at> recursewithless.net>
>> Date: Sun, 19 Jan 2025 08:59:30 +0100
>> 
>> I know this bug was opened right before the holidays, and it's a big
>> patch, so I understand if no one's had time to look at it yet. Before I
>> put any more work into this, though, it would useful if someone could
>> just give this a quick look and tell me if it seems like I'm headed in
>> the right direction: a "yes, this looks reasonable, let us know when
>> you're finished" or a "there's no way this will get merged, please start
>> over with a new approach" would be helpful feedback to have at this
>> point. (Of course, more detailed feedback is welcome too, if someone has
>> time.)
>
> You posted 2 patches, both of them quite large.  We don't have
> icalendar experts on board whom we could ask to review the patches.

Right, I guess at this point that "expert" may be me. :)

I wasn't looking for any very detailed feedback, especially about
anything iCalendar-specific, just a quick check-in to make sure I
haven't already made some decision that would prevent the code from
being merged, out of my own inexperience.

> Are there any specific aspects of the patches you'd like us to look
> into, or some specific design issues you'd like to discuss?  That'd
> make the review more focused.

Here are a few things I've been wondering about, for anyone who has time
to take a look:

1) Is it alright that this is a ground-up rewrite, rather than an
evolution of icalendar.el? And how should I be thinking about the future
of icalendar.el?

Having written this code, I can now understand and appreciate some of
the code in icalendar.el better, but am still of the opinion that it is
too closely tied to diary import/export to be as useful to other parts
of Emacs (especially Org and Gnus) as it could be.

I've been thinking it would make sense eventually to:
  - rename icalendar.el to something like diary-icalendar.el
  - rewrite the code there to use the new parser in icalendar-parser.el
  - rename the user-facing functions
    (icalendar-{import/export}-file and their variants)
    to make clear that they are diary-specific
  - delete(?)/deprecate the old parsing code

But let me know if that is not in the cards.

2) It would be nice if someone who has more experience writing parsers
could give me some feedback about the overall design of the parser. In
particular:

  - I've used macros (see icalendar-macs.el) to create a small "DSL" for
    defining iCalendar types. These macros store parsing-related information for
    each type as properties of the symbols which name them. There's a lot of
    dynamic dispatch in the parser based on these type symbols' properties.
    This adds some complexity but (I hope) makes the parser more "atomic"/
    extensible. Does this seem like a reasonable approach in general?
  - I ran into one issue that feels like a design flaw: the parser separates
    "reading" (converting a string to an Elisp value) into a function
    distinct from the parsing function which matches that string (see e.g.
    ical:parse-property-value in icalendar-parser.el, which calls
    ical:read-property-value). In simple cases this nicely factors out a pure
    function from one which depends on a lot of global buffer state;
    but in more complicated cases the "pure" reader function depends on
    the match data and so isn't pure at all (see e.g. ical:read-dur-value).
    Is there a better way to do this? (Not make the distinction? Pass
    the match data explicitly? ...?)
  - I haven't really thought about performance at all. My parser is
    definitely slower/more memory intensive than icalendar.el's at the moment
    (though hopefully this is mostly because it has more to offer). Are there
    any things I should already be thinking about to make it more efficient?

3) Likewise, part of my patch was icalendar-mode.el, and it would be
useful to have some feedback about the major mode:

  - whether an icalendar-mode is even a useful thing to have, and what
    could make it more useful
  - whether the approach to syntax highlighting is reasonable (the
    icalendar-define-* macros define matching functions for each
    individual icalendar type, which are eventually piled
    into a long list of matchers in the mode's value for font-lock-keywords)
  - whether there's a better solution to the problem of needing to unfold
    lines *before* a buffer containing iCalendar data is decoded
    (is there anything like a hook that runs before decoding?)
  - what still needs to be done to make the code in icalendar-mode.el
    compliant with Emacs' major mode conventions (see the long list of
    TODOs in the mode definition)

> I can see already that your doc strings are not always according to
> our conventions (the first line must be a single complete sentence),
> and also the commit log messages need to be more detailed.  But these
> are aspects best deferred until you have a version close to the final
> one, now is too early for that.

OK, but that's good to keep in mind, thanks.

(Re: commit messages: should I be making commits in Emacs' ChangeLog
format as I go along, and post a whole new patch series here after
revisions? Or does it make more sense to squash everything at the end
into one or a few commits, and only format the log messages at that
point?)

> One other comment is to be sure the many cl-isms you use don't get in
> the way of debugging the code, since Edebug is known to provide weaker
> support for some cl-lib construct and cl-macs macros.  So if you find
> some macros or constructs that would make debugging harder, and can
> replace them with simpler code without sacrificing too much, that
> would be appreciated.

Yes, this is another thing I was wondering about, so thanks for bringing
it up. I have actually tried to keep cl-isms to a minimum: I am only
using cl-defmacro (for keyword arguments) and the cl type system (mostly
cl-deftype and cl-typep, since the parser needs some way of representing
the types defined by the iCalendar standard, and checking that values
conform to those types). So far this hasn't gotten in the way of
debugging for me, though I am not a heavy Edebug user. (The posted patch
also uses one cl-defstruct but my recent exchange with Ihor Radchenko on
emacs-devel has convinced me that I can move to using
org-element-ast.el, which will eliminate that.)

I am using read-symbol-shorthands to turn the CL-style "ical:" prefix
into "icalendar-", which to me is more readable, but that's easy to
change/eliminate if others don't like it.

Thanks!
Richard

Message #47 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Lawrence <rwl <at> recursewithless.net>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Mon, 20 Jan 2025 14:14:56 +0200

> From: Richard Lawrence <rwl <at> recursewithless.net>
> Cc: 74994 <at> debbugs.gnu.org
> Date: Sun, 19 Jan 2025 21:24:04 +0100
> 
> 1) Is it alright that this is a ground-up rewrite, rather than an
> evolution of icalendar.el? And how should I be thinking about the future
> of icalendar.el?
> 
> Having written this code, I can now understand and appreciate some of
> the code in icalendar.el better, but am still of the opinion that it is
> too closely tied to diary import/export to be as useful to other parts
> of Emacs (especially Org and Gnus) as it could be.

If you still think it's better to have a complete implementation, I
think you've answered your question, and I don't see anyone around who
knows this better and could challenge your opinions.

> I've been thinking it would make sense eventually to:
>   - rename icalendar.el to something like diary-icalendar.el
>   - rewrite the code there to use the new parser in icalendar-parser.el
>   - rename the user-facing functions
>     (icalendar-{import/export}-file and their variants)
>     to make clear that they are diary-specific
>   - delete(?)/deprecate the old parsing code

I'd prefer to do this without renaming the old file.  That is, rename
your new implementation to, say, icalendar2.el.  The rest of your plan
sounds good to me.

> 2) It would be nice if someone who has more experience writing parsers
> could give me some feedback about the overall design of the parser. In
> particular:
> 
>   - I've used macros (see icalendar-macs.el) to create a small "DSL" for
>     defining iCalendar types. These macros store parsing-related information for
>     each type as properties of the symbols which name them. There's a lot of
>     dynamic dispatch in the parser based on these type symbols' properties.
>     This adds some complexity but (I hope) makes the parser more "atomic"/
>     extensible. Does this seem like a reasonable approach in general?
>   - I ran into one issue that feels like a design flaw: the parser separates
>     "reading" (converting a string to an Elisp value) into a function
>     distinct from the parsing function which matches that string (see e.g.
>     ical:parse-property-value in icalendar-parser.el, which calls
>     ical:read-property-value). In simple cases this nicely factors out a pure
>     function from one which depends on a lot of global buffer state;
>     but in more complicated cases the "pure" reader function depends on
>     the match data and so isn't pure at all (see e.g. ical:read-dur-value).
>     Is there a better way to do this? (Not make the distinction? Pass
>     the match data explicitly? ...?)
>   - I haven't really thought about performance at all. My parser is
>     definitely slower/more memory intensive than icalendar.el's at the moment
>     (though hopefully this is mostly because it has more to offer). Are there
>     any things I should already be thinking about to make it more efficient?
> 
> 3) Likewise, part of my patch was icalendar-mode.el, and it would be
> useful to have some feedback about the major mode:
> 
>   - whether an icalendar-mode is even a useful thing to have, and what
>     could make it more useful
>   - whether the approach to syntax highlighting is reasonable (the
>     icalendar-define-* macros define matching functions for each
>     individual icalendar type, which are eventually piled
>     into a long list of matchers in the mode's value for font-lock-keywords)
>   - whether there's a better solution to the problem of needing to unfold
>     lines *before* a buffer containing iCalendar data is decoded
>     (is there anything like a hook that runs before decoding?)
>   - what still needs to be done to make the code in icalendar-mode.el
>     compliant with Emacs' major mode conventions (see the long list of
>     TODOs in the mode definition)

I added Stefan to the discussion, in the hope that he could give you
some useful advice about (some of) these aspects.

> > I can see already that your doc strings are not always according to
> > our conventions (the first line must be a single complete sentence),
> > and also the commit log messages need to be more detailed.  But these
> > are aspects best deferred until you have a version close to the final
> > one, now is too early for that.
> 
> OK, but that's good to keep in mind, thanks.
> 
> (Re: commit messages: should I be making commits in Emacs' ChangeLog
> format as I go along, and post a whole new patch series here after
> revisions? Or does it make more sense to squash everything at the end
> into one or a few commits, and only format the log messages at that
> point?)

We usually do the latter.

Message #50 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 74994 <at> debbugs.gnu.org, Richard Lawrence <rwl <at> recursewithless.net>
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Tue, 21 Jan 2025 18:33:02 -0500

>> I've been thinking it would make sense eventually to:
>>   - rename icalendar.el to something like diary-icalendar.el
>>   - rewrite the code there to use the new parser in icalendar-parser.el

I don't have an opinion on the file naming part, but I suggest you scrap
the "eventually" above, otherwise there's a strong risk it'll never
happen (either for lack of interest or because the API that doesn't
quite fit).

>> 2) It would be nice if someone who has more experience writing parsers
>> could give me some feedback about the overall design of the parser. In
>> particular:

[ Note: I have not yet had time to look at the code.  ]

>>   - I've used macros (see icalendar-macs.el) to create a small "DSL" for
>>     defining iCalendar types. These macros store parsing-related information for
>>     each type as properties of the symbols which name them. There's a lot of
>>     dynamic dispatch in the parser based on these type symbols' properties.
>>     This adds some complexity but (I hope) makes the parser more "atomic"/
>>     extensible. Does this seem like a reasonable approach in general?

It sounds like a reasonable design, yes.

In `bindat.el` I used a similar approach except that each construct (I
guess in your case, that means each "type") is stored as a method (in
a generic function) instead of a property of a symbol.  I'm not sure
it's the perfect solution, but it's nice that `C-h o` on the generic
function can then provide a documentation of each of the constructs.

Other options we use elsewhere is to use function names constructed from
a constant prefix plus the name of the construct, so instead of

    (funcall (get 'foo 'bar) ...)

you might be able to macroexpand to something like

    (,(intern (format "bar %s" 'foo)) ...)

so you get (for free) compile-time warnings when using a construct that
doesn't exist, and you avoid a `get` at runtime (IIRC, we use that
approach in `peg.el`).

>>   - I ran into one issue that feels like a design flaw: the parser separates
>>     "reading" (converting a string to an Elisp value) into a function
>>     distinct from the parsing function which matches that string (see e.g.
>>     ical:parse-property-value in icalendar-parser.el, which calls
>>     ical:read-property-value). In simple cases this nicely factors out a pure
>>     function from one which depends on a lot of global buffer state;
>>     but in more complicated cases the "pure" reader function depends on
>>     the match data and so isn't pure at all (see e.g. ical:read-dur-value).
>>     Is there a better way to do this? (Not make the distinction? Pass
>>     the match data explicitly? ...?)

Is the separation useful to users (including internal users) of the
parser?  This kind of problem doesn't directly ring a bell, so I don't
have a good suggestion to make.  Maybe when I get time to read the
code...  In the mean time, I can just mention that it reminds of the PEG
parsers generated by `peg.el` which use a limited form of backtracking:
they have a pure(ish) part that "explores" and accumulates "actions to
perform if we find a match" and only once we found a global parse do we
actually run the (side-effecting) actions which might build a parse tree.

>>   - whether an icalendar-mode is even a useful thing to have, and what
>>     could make it more useful

I don't think we need to be shy about major modes.  If you don't use it,
it doesn't cost much (because it's not loaded), so count me as "+1".

>>   - whether the approach to syntax highlighting is reasonable (the
>>     icalendar-define-* macros define matching functions for each
>>     individual icalendar type, which are eventually piled
>>     into a long list of matchers in the mode's value for font-lock-keywords)
>>   - what still needs to be done to make the code in icalendar-mode.el
>>     compliant with Emacs' major mode conventions (see the long list of
>>     TODOs in the mode definition)

[ Will see when I finally read the code.  ]

>>   - whether there's a better solution to the problem of needing to unfold
>>     lines *before* a buffer containing iCalendar data is decoded
>>     (is there anything like a hook that runs before decoding?)

[ Sorry, I don't understand this question.  ]


        Stefan

Message #53 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Wed, 22 Jan 2025 08:03:59 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Richard Lawrence <rwl <at> recursewithless.net>

>> I've been thinking it would make sense eventually to:
>>   - rename icalendar.el to something like diary-icalendar.el
>>   - rewrite the code there to use the new parser in icalendar-parser.el
>>   - rename the user-facing functions
>>     (icalendar-{import/export}-file and their variants)
>>     to make clear that they are diary-specific
>>   - delete(?)/deprecate the old parsing code
>
> I'd prefer to do this without renaming the old file.  That is, rename
> your new implementation to, say, icalendar2.el.  The rest of your plan
> sounds good to me.

Is this more about maintaining the source history, or about not wanting
to break the setups of people who have (require 'icalendar) or similar
in their configuration?

So far, all my new code lives in files with names like
icalendar-parser.el and there is no contention for the name
icalendar.el.

My thought was that icalendar.el could/should primarily serve as a
top-level module for the library, require-ing all the new modules
itself, so that users of the library can just (require 'icalendar) and
not have to worry about how the code is organized.

My reason for "renaming" was simply to move the existing diary-specific
parts of icalendar.el (which is most of the file) to diary-icalendar.el,
to be consistent with gnus-icalendar.el in Gnus, ox-icalendar.el in Org,
and the organization of the other new files.

If backward compatibility for users is the concern, icalendar.el could
itself (require 'diary-icalendar) and then there's no problem.

If instead it's about keeping the old code in the same file for the sake
of history, I understand; I don't have to move it, just thought it would
be cleaner if I did.

Message #56 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Wed, 22 Jan 2025 08:43:38 +0100

Hi Stefan,

thanks for your feedback!

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>>>   - I've used macros (see icalendar-macs.el) to create a small "DSL" for
>>>     defining iCalendar types. These macros store parsing-related information for
>>>     each type as properties of the symbols which name them. There's a lot of
>>>     dynamic dispatch in the parser based on these type symbols' properties.
>>>     This adds some complexity but (I hope) makes the parser more "atomic"/
>>>     extensible. Does this seem like a reasonable approach in general?
>
> It sounds like a reasonable design, yes.
>
> In `bindat.el` I used a similar approach except that each construct (I
> guess in your case, that means each "type") is stored as a method (in
> a generic function) instead of a property of a symbol.  I'm not sure
> it's the perfect solution, but it's nice that `C-h o` on the generic
> function can then provide a documentation of each of the constructs.

This would mean relying more heavily on cl-lib, correct? Generic
functions and methods are part of cl-lib's CLOS implementation?

C-h o already works with my code (see the describe-symbol backend at the
end of icalendar-parser.el), but maybe the generic functions approach is
cleaner. I'll think about it.

> Other options we use elsewhere is to use function names constructed from
> a constant prefix plus the name of the construct, so instead of
>
>     (funcall (get 'foo 'bar) ...)
>
> you might be able to macroexpand to something like
>
>     (,(intern (format "bar %s" 'foo)) ...)
>
> so you get (for free) compile-time warnings when using a construct that
> doesn't exist, and you avoid a `get` at runtime (IIRC, we use that
> approach in `peg.el`).

I hadn't thought of that. Would this prevent users of the library from
defining new types after the library is compiled, though? The iCalendar
standard allows extensions in "X-" properties and components; I don't
want to do anything that would make it difficult e.g. for Org to use
these to encode its own data structures.

>>>   - I ran into one issue that feels like a design flaw: the parser separates
>>>     "reading" (converting a string to an Elisp value) into a function
>>>     distinct from the parsing function which matches that string (see e.g.
>>>     ical:parse-property-value in icalendar-parser.el, which calls
>>>     ical:read-property-value). In simple cases this nicely factors out a pure
>>>     function from one which depends on a lot of global buffer state;
>>>     but in more complicated cases the "pure" reader function depends on
>>>     the match data and so isn't pure at all (see e.g. ical:read-dur-value).
>>>     Is there a better way to do this? (Not make the distinction? Pass
>>>     the match data explicitly? ...?)
>
> Is the separation useful to users (including internal users) of the
> parser? This kind of problem doesn't directly ring a bell, so I don't
> have a good suggestion to make.

It's certainly useful when debugging. Calling a pure function with M-:
or e in the debugger to make sure it's doing what I expect is generally
a lot easier than getting a whole buffer into the right parsing state.
If I can declare them pure, it might also have some performance
benefits.

>>>   - whether there's a better solution to the problem of needing to unfold
>>>     lines *before* a buffer containing iCalendar data is decoded
>>>     (is there anything like a hook that runs before decoding?)
>
> [ Sorry, I don't understand this question.  ]

The standard says that long lines need to be "folded" (wrapped) by
inserting a CR-LF-space sequence. It defines long lines as those longer
than 75 *bytes*, and explicitly says that implementations need to handle
the case where the line-wrapping sequence occurs in the middle of a
multi-byte character. So the only safe way to unwrap lines is before a
buffer gets decoded.

So far the best user interface I could come up with was to check for
long lines when icalendar-mode starts and ask the user whether they want
to unwrap them. If they do, it re-loads the raw data into a new buffer,
unwraps the lines, decodes the buffer, and then re-starts icalendar-mode
in the new buffer. But I find this pretty awkward in practice, because
you end up with two buffers containing the same data (modulo whitespace)
and visiting the same file, and I'm not sure how to improve this.

Thanks again for your thoughts!

Best,
Richard

Message #59 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Richard Lawrence <rwl <at> recursewithless.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Wed, 22 Jan 2025 04:11:59 -0500

>> In `bindat.el` I used a similar approach except that each construct (I
>> guess in your case, that means each "type") is stored as a method (in
>> a generic function) instead of a property of a symbol.  I'm not sure
>> it's the perfect solution, but it's nice that `C-h o` on the generic
>> function can then provide a documentation of each of the constructs.
>
> This would mean relying more heavily on cl-lib, correct? Generic
> functions and methods are part of cl-lib's CLOS implementation?

No.  They (ab)use the "cl-" prefix for historical reasons (EIEIO had
already used the non-prefixed `defgeneric/defmethod` names), but
`cl-generic.el` is not part of cl-lib (and it is preloaded into Emacs).

> C-h o already works with my code (see the describe-symbol backend at the
> end of icalendar-parser.el), but maybe the generic functions approach is
> cleaner. I'll think about it.
>
>> Other options we use elsewhere is to use function names constructed from
>> a constant prefix plus the name of the construct, so instead of
>>
>>     (funcall (get 'foo 'bar) ...)
>>
>> you might be able to macroexpand to something like
>>
>>     (,(intern (format "bar %s" 'foo)) ...)
>>
>> so you get (for free) compile-time warnings when using a construct that
>> doesn't exist, and you avoid a `get` at runtime (IIRC, we use that
>> approach in `peg.el`).
> I hadn't thought of that. Would this prevent users of the library from
> defining new types after the library is compiled, though?

No, tho when the above `'foo` part is not a constant but is computed
dynamically, the code is less efficient than a funcall+get.

> It's certainly useful when debugging. Calling a pure function with M-:
> or e in the debugger to make sure it's doing what I expect is generally
> a lot easier than getting a whole buffer into the right parsing state.

🙂

> If I can declare them pure, it might also have some performance
> benefits.

I'd be surprised if it makes a measurable difference, tho.
The debugging argument is much more compelling.

> The standard says that long lines need to be "folded" (wrapped) by
> inserting a CR-LF-space sequence. It defines long lines as those longer
> than 75 *bytes*, and explicitly says that implementations need to handle
> the case where the line-wrapping sequence occurs in the middle of a
> multi-byte character. So the only safe way to unwrap lines is before a
> buffer gets decoded.

Eww!

> So far the best user interface I could come up with was to check for
> long lines when icalendar-mode starts and ask the user whether they want
> to unwrap them. If they do, it re-loads the raw data into a new buffer,
> unwraps the lines, decodes the buffer, and then re-starts icalendar-mode
> in the new buffer. But I find this pretty awkward in practice, because
> you end up with two buffers containing the same data (modulo whitespace)
> and visiting the same file, and I'm not sure how to improve this.

Maybe strongly encourage the user to save the result back into the
original file?

How common is it for multibyte sequences to split in this way?

Is it always UTF8?  If it's always UTF8, then multibyte sequences split
in two *will* result in "eight-bit" byte chars, so you should be able to
recognize them reliably even in the already-decoded buffer with a regexp
along the lines of "[\200-\377]+\n [\200-\377]+" and you should then be
able handle them "directly/locally" without reloading the undecoded file.
Something like:

    (while (re-search-forward "[\200-\377]+\n [\200-\377]+" nil t)
      (delete-region (1- (line-beginning-position))
                     (1+ (line-beginning-position)))
      (decode-coding-region (match-beg 0) (- (match-end 0) 2) 'utf-8))


- Stefan

Message #62 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Lawrence <rwl <at> recursewithless.net>
Cc: monnier <at> iro.umontreal.ca, 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Wed, 22 Jan 2025 16:25:25 +0200

> From: Richard Lawrence <rwl <at> recursewithless.net>
> Cc: 74994 <at> debbugs.gnu.org
> Date: Wed, 22 Jan 2025 08:03:59 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Richard Lawrence <rwl <at> recursewithless.net>
> 
> >> I've been thinking it would make sense eventually to:
> >>   - rename icalendar.el to something like diary-icalendar.el
> >>   - rewrite the code there to use the new parser in icalendar-parser.el
> >>   - rename the user-facing functions
> >>     (icalendar-{import/export}-file and their variants)
> >>     to make clear that they are diary-specific
> >>   - delete(?)/deprecate the old parsing code
> >
> > I'd prefer to do this without renaming the old file.  That is, rename
> > your new implementation to, say, icalendar2.el.  The rest of your plan
> > sounds good to me.
> 
> Is this more about maintaining the source history, or about not wanting
> to break the setups of people who have (require 'icalendar) or similar
> in their configuration?

The former.  As good as current VCS forensic tools are, some of the
relevant VCS commands still don't work across renames.  So it is
preferable to rename only when necessary.

> So far, all my new code lives in files with names like
> icalendar-parser.el and there is no contention for the name
> icalendar.el.
> 
> My thought was that icalendar.el could/should primarily serve as a
> top-level module for the library, require-ing all the new modules
> itself, so that users of the library can just (require 'icalendar) and
> not have to worry about how the code is organized.
> 
> My reason for "renaming" was simply to move the existing diary-specific
> parts of icalendar.el (which is most of the file) to diary-icalendar.el,
> to be consistent with gnus-icalendar.el in Gnus, ox-icalendar.el in Org,
> and the organization of the other new files.

If there's a lot of stuff to be moved out of the file, I think it's
better to leave the file alone, and instead start new files (as you
have evidently done).

Message #65 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Thu, 23 Jan 2025 19:12:20 +0100

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> The standard says that long lines need to be "folded" (wrapped) by
>> inserting a CR-LF-space sequence. It defines long lines as those longer
>> than 75 *bytes*, and explicitly says that implementations need to handle
>> the case where the line-wrapping sequence occurs in the middle of a
>> multi-byte character. So the only safe way to unwrap lines is before a
>> buffer gets decoded.
>
> Eww!

Yes indeed. 

>> So far the best user interface I could come up with was to check for
>> long lines when icalendar-mode starts and ask the user whether they want
>> to unwrap them. If they do, it re-loads the raw data into a new buffer,
>> unwraps the lines, decodes the buffer, and then re-starts icalendar-mode
>> in the new buffer. But I find this pretty awkward in practice, because
>> you end up with two buffers containing the same data (modulo whitespace)
>> and visiting the same file, and I'm not sure how to improve this.
>
> Maybe strongly encourage the user to save the result back into the
> original file?

Yes, that's already what I do, setting buffer-file-name to point to the
original file in the new buffer as well; and there's a prompt to re-wrap
lines on save. I suppose what I could do is unconditionally kill the old
buffer and then steal its name for the new one (or just erase it and
reload the data into the same buffer), so that from the user's
perspective, it's "the same" buffer. Does that seem better?

> How common is it for multibyte sequences to split in this way?

No idea. Probably not common. 

> Is it always UTF8?

Alas, no. According to the standard, UTF-8 is the "default" encoding,
and implementations must support it, but as far as I can tell, the
standard allows using another encoding via the MIME charset parameter
(I infer this from section 8.1, which mentions the possibility).  

> If it's always UTF8, then multibyte sequences split
> in two *will* result in "eight-bit" byte chars, so you should be able to
> recognize them reliably even in the already-decoded buffer with a regexp
> along the lines of "[\200-\377]+\n [\200-\377]+" and you should then be
> able handle them "directly/locally" without reloading the undecoded file.
>
> Something like:
>
>     (while (re-search-forward "[\200-\377]+\n [\200-\377]+" nil t)
>       (delete-region (1- (line-beginning-position))
>                      (1+ (line-beginning-position)))
>       (decode-coding-region (match-beg 0) (- (match-end 0) 2) 'utf-8))

Hmm, that's an interesting idea, thanks. I will look into plugging this
into the unwrapping code, at least when the coding system is known to be
UTF-8.

Message #68 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Improve Emacs iCalendar support
Date: Thu, 23 Jan 2025 19:48:33 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:
 
>> Is this more about maintaining the source history, or about not wanting
>> to break the setups of people who have (require 'icalendar) or similar
>> in their configuration?
>
> The former.  As good as current VCS forensic tools are, some of the
> relevant VCS commands still don't work across renames.  So it is
> preferable to rename only when necessary.
> ...
> If there's a lot of stuff to be moved out of the file, I think it's
> better to leave the file alone, and instead start new files (as you
> have evidently done).

OK, got it. I'll see what I can do to clean up and generalize
icalendar.el without moving the existing code.

To be honest, though, I've found this rough going over the last few days
(in agreement with my previous attempts). The code in icalendar.el is
not easy for me to understand, even now; much of it appears to have been
written by someone who was more used to writing in a language like C,
and also had apprehensions about long variable names, the overhead of
accessor functions which aren't "car" or "cdr", the use of other data
structures when strings will do, etc. I'm finding it hard to integrate
the new parser with the existing code without just wholesale replacing
entire function bodies, and I find myself constantly wondering if it's
worth the effort. I may end up just leaving icalendar.el untouched and
writing an alternative diary-icalendar feature from scratch; certainly
that seems easier to me right now. If I decide that's the best route,
would anything stand in the way of that new file becoming the
default way of doing things, recommended in the manual, etc.?

Message #71 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Updated patch for Bug#74994: add support for recurrence rules
Date: Wed, 12 Mar 2025 11:02:28 +0100

[Message part 1 (text/plain, inline)]

Tags: patch

I've been steadily working on this and I've reached another milestone
this week: I now have a working implementation of recurrence rules,
including time zone support. This is the big, complicated part of
iCalendar semantics that none of the existing code completely supports;
but it's essential, because most date-times in iCalendar are defined as
local times with a reference to a time zone, and calculating an actual
UTC offset requires applying the recurrence rules for that time zone.
Updated patch attached; the main addition here is icalendar-recur.el,
plus supporting functions in icalendar-macs.el and icalendar-utils.el.

It was a *lot* more work to get this working than I expected (mostly
because time zones are complicated...who knew? ;), but I can at least
say now that all the examples in RFC5545 work; see the tests in
icalendar-recur-tests.el.

[0001-Updated-patch-for-Bug-74994-add-support-for-recurren.patch (text/patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.