[PATCH] ox-icalendar: fix handling of timestamps

[PATCH] ox-icalendar: fix handling of timestamps

[Viktor Rosenfeld]

  * ox-icalendar.el (org-icalendar-entry): Honor setting of
  `org-icalendar-with-timestamps' for timestamps on headlines
  and checkboxes.

  The setting `org-icalendar-with-timestamps' was only applied
  to timestamps which do not appear on a heading or on a
  checkbox. E.g., with `org-icalendar-with-timestamps' set to
  'active, an heading containing an inactive timestamp on would
  be exported. This patch fixes this.

TINYCHANGE
---
 lisp/ox-icalendar.el | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/lisp/ox-icalendar.el b/lisp/ox-icalendar.el
index c6ab295..8f15124 100644
--- a/lisp/ox-icalendar.el
+++ b/lisp/ox-icalendar.el
@@ -580,15 +580,24 @@ inlinetask within the section."
      ;; When collecting plain timestamps from a headline and
      ;; its title, skip inlinetasks since collection will
      ;; happen once ENTRY is one of them.
-     (let ((counter 0))
+     (let ((counter 0)
+     (with-timestamps (plist-get info :with-timestamps)))
        (mapconcat
         'identity
         (org-element-map (cons (org-element-property :title entry)
              (org-element-contents inside))
       'timestamp
     (lambda (ts)
-      (let ((uid (format "TS%d-%s" (incf counter) uid)))
-        (org-icalendar--vevent entry ts uid summary loc desc cat)))
+      (let ((type (org-element-property :type ts))
+      (uid (format "TS%d-%s" (incf counter) uid)))
+        (when (or (eq with-timestamps 'all)
+            (and (eq with-timestamps 'active)
+           (or (eq type 'active)
+         (eq type 'active-range)))
+            (and (eq with-timestamps 'inactive)
+           (or (eq type 'inactive)
+         (eq type 'inactive-range))))
+          (org-icalendar--vevent entry ts uid summary loc desc cat))))
     info nil (and (eq type 'headline) 'inlinetask))
         ""))
      ;; Task: First check if it is appropriate to export it.
-- 
1.8.3.4

Re: [PATCH] ox-icalendar: fix handling of timestamps

[Aaron Ecay]

Hi Viktor,

Thanks for this patch; I had also noticed this problem.

2013ko abuztuak 10an, Viktor Rosenfeld-ek idatzi zuen:
> 
>   * ox-icalendar.el (org-icalendar-entry): Honor setting of
>   `org-icalendar-with-timestamps' for timestamps on headlines
>   and checkboxes.
> 
>   The setting `org-icalendar-with-timestamps' was only applied
>   to timestamps which do not appear on a heading or on a
>   checkbox. E.g., with `org-icalendar-with-timestamps' set to
>   'active, an heading containing an inactive timestamp on would
>   be exported. This patch fixes this.
> 
> TINYCHANGE
> ---
>  lisp/ox-icalendar.el | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/lisp/ox-icalendar.el b/lisp/ox-icalendar.el
> index c6ab295..8f15124 100644
> --- a/lisp/ox-icalendar.el
> +++ b/lisp/ox-icalendar.el
> @@ -580,15 +580,24 @@ inlinetask within the section."
>       ;; When collecting plain timestamps from a headline and
>       ;; its title, skip inlinetasks since collection will
>       ;; happen once ENTRY is one of them.
> -     (let ((counter 0))
> +     (let ((counter 0)
> +     (with-timestamps (plist-get info :with-timestamps)))
>         (mapconcat
>          'identity
>          (org-element-map (cons (org-element-property :title entry)
>               (org-element-contents inside))
>        'timestamp
>      (lambda (ts)
> -      (let ((uid (format "TS%d-%s" (incf counter) uid)))
> -        (org-icalendar--vevent entry ts uid summary loc desc cat)))
> +      (let ((type (org-element-property :type ts))
> +      (uid (format "TS%d-%s" (incf counter) uid)))
> +        (when (or (eq with-timestamps 'all)

Here, I think you want to compare with t, not 'all (check the defcustom
for ‘org-icalendar-with-timestamps’).

> +            (and (eq with-timestamps 'active)
> +           (or (eq type 'active)
> +         (eq type 'active-range)))

This is only a cosmetic comment, so feel free to disregard it, but:
might the ‘(or ...)’ be cleaner as ‘(memq type '(active active-range))’?

-- 
Aaron Ecay

Re: [PATCH] ox-icalendar: fix handling of timestamps

[Nicolas Goaziou]

Hello,

Viktor Rosenfeld <listuser36@gmail.com> writes:

>   * ox-icalendar.el (org-icalendar-entry): Honor setting of
>   `org-icalendar-with-timestamps' for timestamps on headlines
>   and checkboxes.
>
>   The setting `org-icalendar-with-timestamps' was only applied
>   to timestamps which do not appear on a heading or on a
>   checkbox. E.g., with `org-icalendar-with-timestamps' set to
>   'active, an heading containing an inactive timestamp on would
>   be exported. This patch fixes this.

This would make icalendar back-end inconsistent with other back-ends,
see `org-export-with-timestamps' docstring.

If inconsistency is desirable in this case, `org-icalendar-with-timestamps'
docstring should clearly state it.


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] ox-icalendar: fix handling of timestamps

[Viktor Rosenfeld]

Hi Nicolas,

Nicolas Goaziou wrote:

> Hello,
> 
> Viktor Rosenfeld <listuser36@gmail.com> writes:
> 
> >   * ox-icalendar.el (org-icalendar-entry): Honor setting of
> >   `org-icalendar-with-timestamps' for timestamps on headlines
> >   and checkboxes.
> >
> >   The setting `org-icalendar-with-timestamps' was only applied
> >   to timestamps which do not appear on a heading or on a
> >   checkbox. E.g., with `org-icalendar-with-timestamps' set to
> >   'active, an heading containing an inactive timestamp on would
> >   be exported. This patch fixes this.
> 
> This would make icalendar back-end inconsistent with other back-ends,
> see `org-export-with-timestamps' docstring.
> 
> If inconsistency is desirable in this case, `org-icalendar-with-timestamps'
> docstring should clearly state it.

The docstring of `org-icalendar-with-timestamps' already states:

  This variable has precedence over `org-export-with-timestamps'.
  It can also be set with the #+OPTIONS line, e.g. "<:t".

I believe that inconsistency is desirable here. Consider the following
use case with three headlines:

* TODO An appointment in the future
<2013-08-12 So 09:00>
* DONE A note about an appointment in the past
[2013-08-10 Fr 09:00]
* WAIT A reminder how long I've been waiting for something [2013-08-10 Fr]

The previous behavior, with `org-icalendar-with-timestamps' set to
'active, was that the first and the last headlines were picked up (even
though the timestamp in the last headline is inactive). This was
unexpected because the two inactive timestamps are handled differently.
My expectation was that only the first headline should have been
exported. This is what my patch achieves.

(Putting timestamps on the heading is useful to me because then I can
see them in the agenda. Also, I often put timestamps on lines with
checkboxes. However, I do not want to see any of those in my calendar
when I export only active timestamps.)

Cheers,
Viktor

> 
> 
> Regards,
> 
> -- 
> Nicolas Goaziou
> 

Re: [PATCH] ox-icalendar: fix handling of timestamps

[Viktor Rosenfeld]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

Hi Aaron,

Aaron Ecay wrote:

> >      (lambda (ts)
> > -      (let ((uid (format "TS%d-%s" (incf counter) uid)))
> > -        (org-icalendar--vevent entry ts uid summary loc desc cat)))
> > +      (let ((type (org-element-property :type ts))
> > +      (uid (format "TS%d-%s" (incf counter) uid)))
> > +        (when (or (eq with-timestamps 'all)
> 
> Here, I think you want to compare with t, not 'all (check the defcustom
> for ‘org-icalendar-with-timestamps’).

Thanks, fixed!

> 
> > +            (and (eq with-timestamps 'active)
> > +           (or (eq type 'active)
> > +         (eq type 'active-range)))
> 
> This is only a cosmetic comment, so feel free to disregard it, but:
> might the ‘(or ...)’ be cleaner as ‘(memq type '(active active-range))’?

Thanks, fixed! I did not like the construction with `or' but I didn't
know about `memq'. Learning Elisp as I go...

New patch is attached.

Cheers,
Viktor
> 
> -- 
> Aaron Ecay
> 

[-- Attachment #2: 0001-ox-icalendar-fix-handling-of-timestamps.patch --]
[-- Type: text/plain, Size: 2054 bytes --]

From 52511b5e2a538d3bb0375c2e32caef0a27e1998e Mon Sep 17 00:00:00 2001
From: Viktor Rosenfeld <listuser36@gmail.com>
Date: Sun, 11 Aug 2013 03:59:29 +0200
Subject: [PATCH] ox-icalendar: fix handling of timestamps

	* ox-icalendar.el (org-icalendar-entry): Honor setting of
	`org-icalendar-with-timestamps' for timestamps on headlines
	and checkboxes.

	The setting `org-icalendar-with-timestamps' was only applied
	to timestamps which do not appear on a heading or on a
	checkbox. E.g., with `org-icalendar-with-timestamps' set to
	'active, an heading containing an inactive timestamp on would
	be exported. This patch fixes this.

TINYCHANGE
---
 lisp/ox-icalendar.el | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lisp/ox-icalendar.el b/lisp/ox-icalendar.el
index c6ab295..ab83a48 100644
--- a/lisp/ox-icalendar.el
+++ b/lisp/ox-icalendar.el
@@ -580,15 +580,22 @@ inlinetask within the section."
 	    ;; When collecting plain timestamps from a headline and
 	    ;; its title, skip inlinetasks since collection will
 	    ;; happen once ENTRY is one of them.
-	    (let ((counter 0))
+	    (let ((counter 0)
+		  (with-timestamps (plist-get info :with-timestamps)))
 	      (mapconcat
 	       'identity
 	       (org-element-map (cons (org-element-property :title entry)
 				      (org-element-contents inside))
 		   'timestamp
 		 (lambda (ts)
-		   (let ((uid (format "TS%d-%s" (incf counter) uid)))
-		     (org-icalendar--vevent entry ts uid summary loc desc cat)))
+		   (let ((type (org-element-property :type ts))
+			 (uid (format "TS%d-%s" (incf counter) uid)))
+		     (when (or (eq with-timestamps t)
+			       (and (eq with-timestamps 'active)
+				    (memq type '(active active-range)))
+			       (and (eq with-timestamps 'inactive)
+				    (memq type '(inactive 'inactive-range))))
+		       (org-icalendar--vevent entry ts uid summary loc desc cat))))
 		 info nil (and (eq type 'headline) 'inlinetask))
 	       ""))
 	    ;; Task: First check if it is appropriate to export it.
-- 
1.8.3.4

Re: [PATCH] ox-icalendar: fix handling of timestamps

[Nicolas Goaziou]

Viktor Rosenfeld <listuser36@gmail.com> writes:

> The docstring of `org-icalendar-with-timestamps' already states:
>
>   This variable has precedence over `org-export-with-timestamps'.
>   It can also be set with the #+OPTIONS line, e.g. "<:t".

This wouldn't be sufficient: "has precedence over" isn't a synonym for
"change the meaning of".

> I believe that inconsistency is desirable here. Consider the following
> use case with three headlines:
>
> * TODO An appointment in the future
> <2013-08-12 So 09:00>
> * DONE A note about an appointment in the past
> [2013-08-10 Fr 09:00]
> * WAIT A reminder how long I've been waiting for something [2013-08-10 Fr]
>
> The previous behavior, with `org-icalendar-with-timestamps' set to
> 'active, was that the first and the last headlines were picked up (even
> though the timestamp in the last headline is inactive). This was
> unexpected because the two inactive timestamps are handled
> differently.

This is to be expected according to `org-export-with-timestamps'.

> My expectation was that only the first headline should have been
> exported. This is what my patch achieves.

The meaning of `org-export-with-timestamps' is the result of a discussion in
this ML. Please read the whole thread starting at:

  http://permalink.gmane.org/gmane.emacs.orgmode/69971


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] ox-icalendar: fix handling of timestamps

[Viktor Rosenfeld]

Hi Nicolas,

Nicolas Goaziou wrote:

> Viktor Rosenfeld <listuser36@gmail.com> writes:
> 
> > The docstring of `org-icalendar-with-timestamps' already states:
> >
> >   This variable has precedence over `org-export-with-timestamps'.
> >   It can also be set with the #+OPTIONS line, e.g. "<:t".
> 
> This wouldn't be sufficient: "has precedence over" isn't a synonym for
> "change the meaning of".

Okay, I see the change in meaning now.
 
> > I believe that inconsistency is desirable here. Consider the following
> > use case with three headlines:
> >
> > * TODO An appointment in the future
> > <2013-08-12 So 09:00>
> > * DONE A note about an appointment in the past
> > [2013-08-10 Fr 09:00]
> > * WAIT A reminder how long I've been waiting for something [2013-08-10 Fr]
> >
> > The previous behavior, with `org-icalendar-with-timestamps' set to
> > 'active, was that the first and the last headlines were picked up (even
> > though the timestamp in the last headline is inactive). This was
> > unexpected because the two inactive timestamps are handled
> > differently.
> 
> This is to be expected according to `org-export-with-timestamps'.
> 
> > My expectation was that only the first headline should have been
> > exported. This is what my patch achieves.
> 
> The meaning of `org-export-with-timestamps' is the result of a discussion in
> this ML. Please read the whole thread starting at:
> 
>   http://permalink.gmane.org/gmane.emacs.orgmode/69971

Thanks for the link. (It made me realize that something was wrong in my
setup. For some reason, I was picking up `org-export-with-timestamps'
from `org-exp.el', i.e., pre-8.0 code.)

In any case, the docstring of `org-export-with-timestamps' states:

  This only applies to timestamps isolated in a paragraph containing
  only timestamps.  Other timestamps are always exported.

This explains the observerd behavior. But I don't think it's appropriate
for the export to iCal. Quoting Carsten from
http://thread.gmane.org/gmane.emacs.orgmode/69971/focus=70068:

  Some people throw in time stamps often while they work, just
  as a little label, indicating that they were working on this
  at a specific date, or that the entry was created on a specific
  date.  Many people I know have a hook that throws in such a
  time stamp in each new entry created.  This creates a lot of
  clutter when you print it, which is why you can turn off
  export of timestamps.

  That option was not meant for a contextual line like your
  first example.  If you use the time stamps in this way, you
  probably will not turn off timestamp export at all, you
  will just leave it on.  If you mix both ways of using
  time stamps - well, too bad.

So the timestamp in the following example is clutter and can be turned
off (i.e., it will not be exported):

  * Meet X
    <2013-08-11 So>

But the following will always be exported, even though the date is just
o note and will not not cause the task to appear in the agenda:

  * Do stuff
    - Started on [2013-08-11 So]

I want to make the iCal export mirror the agenda.

I think the underlying problem is that there is no way in Org to
annotate a timestamp as a fixed appointment. There's SCHEDULED and
DEADLINE, but there's no APPT. The consensus is to use a standalone
active timestamp for fixed appointments, which is easy enough. But then
I would expect only those to appear in an iCal export.

So I propose to append the docstring of
`org-icalendar-export-timestamps':

  This variable has precedence over and overrides the behavior of
  `org-export-with-timestamps'. The setting is applied to every
  timestamp below a headline and not only to those which are isolated in
  a paragraph containing only timestamps.

  It can also be set with the #+OPTIONS line, e.g. "<:t". 

Cheers,
Viktor

(PS: Sorry for the long post.)
> 
> 
> Regards,
> 
> -- 
> Nicolas Goaziou
> 

Re: [PATCH] ox-icalendar: fix handling of timestamps

[Viktor Rosenfeld]

Hi Nicolas,

Viktor Rosenfeld wrote:

> So I propose to append the docstring of
> `org-icalendar-export-timestamps':
> 
>   This variable has precedence over and overrides the behavior of
>   `org-export-with-timestamps'. The setting is applied to every
>   timestamp below a headline and not only to those which are isolated in
>   a paragraph containing only timestamps.
> 
>   It can also be set with the #+OPTIONS line, e.g. "<:t". 

I just realized that this is not enough. The option
`org-icalendar-with-timestamps' already changes the meaning of
`org-export-with-timestamps'. The latter only removes timestamps from
the export whereas during the iCalendar export the presence of a
timestamp determines whether the heading is exported at all. So it's
more akin to the behavior of :export: and :noexport: tags.

I would change the docstring of `org-icalendar-export-timestamps' as
such:

  Non-nil means export headlines with timestamps.
 
  It can be set to any of the following values:
    t           export headlines containing any kind of timestamp
    `active'    export headlines containing active timestamps
    `inactive'  export headlines containing inactive timestamps
    nil         do not any headlines

  Note: This variable overrides the behavior of
  `org-export-with-timestamps'.  `org-export-with-timestamps' controls
  whether a timestamp is exported or removed from the export.
  `org-icalendar-export-timestamps' controls whether a headline is
  exported or removed from the exported. In addition,
  `org-icalendar-export-timestamps' applies to every timestamp below a
  headline and not only to those which are isolated in a paragraph
  containing only timestamps.

  This variable can also be set with the #+OPTIONS line, e.g. "<:t". 

Note that setting the variable in the OPTIONS line is useful to remove a
file from the iCalendar export even though it is displayed in the
agenda. I actually have this usecase (although right now I exclude by
tag).

Cheers,
Viktor

Re: [PATCH] ox-icalendar: fix handling of timestamps

[Nicolas Goaziou]

Hello,

Viktor Rosenfeld <listuser36@gmail.com> writes:

>> So I propose to append the docstring of
>> `org-icalendar-export-timestamps':
>> 
>>   This variable has precedence over and overrides the behavior of
>>   `org-export-with-timestamps'. The setting is applied to every
>>   timestamp below a headline

What about those _on_ the headline?

> I just realized that this is not enough. The option
> `org-icalendar-with-timestamps' already changes the meaning of
> `org-export-with-timestamps'. The latter only removes timestamps from
> the export whereas during the iCalendar export the presence of a
> timestamp determines whether the heading is exported at all. So it's
> more akin to the behavior of :export: and :noexport: tags.

Not at all. There are other ways to trigger a headline export (a TODO
tag, another timestamp in the section, SCHEDULED information...).
`org-icalendar-exclude-tags' is much stronger, since it will ignore
headline unconditionally.

> I would change the docstring of `org-icalendar-export-timestamps' as
> such:
>
>   Non-nil means export headlines with timestamps.
>  
>   It can be set to any of the following values:
>     t           export headlines containing any kind of timestamp
>     `active'    export headlines containing active timestamps
>     `inactive'  export headlines containing inactive timestamps
>     nil         do not any headlines

I'd rather use something like:

  t   generate an event for every timestamp type in the headline

and so forth.


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] ox-icalendar: fix handling of timestamps

[Nicolas Goaziou]

Viktor Rosenfeld <listuser36@gmail.com> writes:

Thanks for the patch. A small comment follows:

> +		   (let ((type (org-element-property :type ts))
> +			 (uid (format "TS%d-%s" (incf counter) uid)))
> +		     (when (or (eq with-timestamps t)
> +			       (and (eq with-timestamps 'active)
> +				    (memq type '(active active-range)))
> +			       (and (eq with-timestamps 'inactive)
> +				    (memq type '(inactive 'inactive-range))))
> +		       (org-icalendar--vevent entry ts uid summary loc desc cat))))

This is not correct since COUNTER is increased before we know if the
VEVENT will be created. IOW, (let ((uid ...))) should be moved after the
`when'.


Regards,

-- 
Nicolas Goaziou