Org-mode mailing list
 help / color / mirror / Atom feed
* Bug: incorrect timestamps with :time-prompt and datetrees
@ 2020-12-24 10:42 Richard Lawrence
  2020-12-24 12:07 ` Richard Lawrence
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Lawrence @ 2020-12-24 10:42 UTC (permalink / raw)
  To: emacs-orgmode

Hi all,

I ran into a subtle bug yesterday. Basically, when using org-capture to
capture

  - an entry into a datetree,
  - on a date other than today (using :time-prompt in org-capture-templates)
  - with a capture template that inserts a timestamp (%T)

then I get incorrect results for either the timestamp or the location in
the datetree, depending on how I enter the date.

Here is a minimal working example:

#+begin_src emacs-lisp
  (setq org-capture-templates
      `(
        ("d" "Diary")
        ("da" "Appointment/Event" entry
         (file+olp+datetree "/tmp/diary.org")
         "**** %^{Description}\n     %T"
         :time-prompt t)))
#+end_src
Notice that the template contains %T, to expand to a timestamp with a
time, and also that the capture target is a datetree and :time-prompt is
true. My understanding is that this should both file the entry to the
entered date in the datetree, and fill the %T template with a timestamp
for the entered date (and time, if any).
  
Here are the results from running a few captures with this setup on
Dec 24 around 11AM. I tried various ways of scheduling an event on Dec 31
at 13:00; what I entered into the prompt of org-read-date is shown in
the headline:

#+begin_example

* 2020

** 2020-12 December

*** 2020-12-24 Thursday
**** Entered "31-12" 
     <2020-12-31 Thu 11:06>

This gets filed to the wrong day in the datetree, but the date in the
timestamp is correct. The time is also correct (it's the current
time, since no time was entered).

**** Entered "31-12 13:00" 
     <2020-12-31 Thu 13:00>

This gets filed to the wrong day in the datetree, but the date and time
in the timestamp are correct.
     
*** 2020-12-31 Thursday
**** Entered "12-31" 
     <2020-12-31 Thu 00:00>

This gets filed to the correct day in the datetree and the date in the
timestamp is correct. The time is 00:00 because no time was entered.
(Why isn't the time 11:06, though, like in the first example?)

**** Entered "12-31 13:00" 
     <2021-01-12 Tue 13:00>

This is the most problematic case: it gets filed to the correct day in
the datetree, but the date in the timestamp is incorrect!

**** Entered "2020-12-31 13:00" 
     <2020-12-31 Thu 13:00>

If I enter the date in full ISO format, the location in the datetree and
the timestamp are both correct.

#+end_example

Possibly relevant here is the value of calendar-date-style, which is
'american for me. I tested briefly with 'european but it did not make a
difference for the "31-12" cases.

This is org 9.4 running from maint (commit ab00524fc). I spent a while
stepping through org-capture and org-read-date but haven't found the
problem yet. I suspect this snippet from a cond form in the middle of
org-capture-set-target-location:

#+begin_src
    ((string-match "\\([^ ]+\\)-[^ ]+[ ]+\\(.*\\)"
                    org-read-date-final-answer)
        ;; Replace any time range by its start.
        (apply #'encode-time
            (org-read-date-analyze
                ;; it looks to me like this is maybe sending the wrong value
                ;; into org-read-date-analyze: 
                (replace-match "\\1 \\2" nil nil  
                               org-read-date-final-answer)

#+end_src

Will report here if I find out more exactly.
Happy holidays, everyone!

-- 
Best,
Richard


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Bug: incorrect timestamps with :time-prompt and datetrees
  2020-12-24 10:42 Bug: incorrect timestamps with :time-prompt and datetrees Richard Lawrence
@ 2020-12-24 12:07 ` Richard Lawrence
  2021-01-06 12:16   ` Richard Lawrence
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Lawrence @ 2020-12-24 12:07 UTC (permalink / raw)
  To: emacs-orgmode

Richard Lawrence <richard.lawrence@uni-tuebingen.de> writes:

> This is org 9.4 running from maint (commit ab00524fc). I spent a while
> stepping through org-capture and org-read-date but haven't found the
> problem yet. I suspect this snippet from a cond form in the middle of
> org-capture-set-target-location:
>
> #+begin_src
>     ((string-match "\\([^ ]+\\)-[^ ]+[ ]+\\(.*\\)"
>                     org-read-date-final-answer)
>         ;; Replace any time range by its start.
>         (apply #'encode-time
>             (org-read-date-analyze
>                 ;; it looks to me like this is maybe sending the wrong value
>                 ;; into org-read-date-analyze: 
>                 (replace-match "\\1 \\2" nil nil  
>                                org-read-date-final-answer)
>
> #+end_src

I can now confirm that this is the problem. It looks like what is happening here
is that the regex is meant to match a time range, but ends up matching
the date: thus a string like "12-31 13:00" gets mangled to "12 13:00"
and sent into org-read-date-analyze.  If I comment out this branch of
the cond form, and enter "12-31 13:00" at the org-read-date-prompt, then
both the timestamp and the location in the datetree are correct.

So I guess the solution is...a better regex is needed to cause this
branch to fire only in the intended case, namely, a time range. Should
it be checking for "am"/"pm" or ":" or similar? Otherwise it seems
impossible to distinguish a date specification like "11-12" from a time
range. 

-- 
Best,
Richard


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Bug: incorrect timestamps with :time-prompt and datetrees
  2020-12-24 12:07 ` Richard Lawrence
@ 2021-01-06 12:16   ` Richard Lawrence
  2021-01-12  8:41     ` [PATCH] " Richard Lawrence
  2021-01-18  6:35     ` Bug: " Kyle Meyer
  0 siblings, 2 replies; 6+ messages in thread
From: Richard Lawrence @ 2021-01-06 12:16 UTC (permalink / raw)
  To: emacs-orgmode

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

Hi everyone,

Richard Lawrence <richard.lawrence@uni-tuebingen.de> writes:

> I can now confirm that this is the problem. It looks like what is happening here
> is that the regex is meant to match a time range, but ends up matching
> the date: thus a string like "12-31 13:00" gets mangled to "12 13:00"
> and sent into org-read-date-analyze.

> So I guess the solution is...a better regex is needed to cause this
> branch to fire only in the intended case, namely, a time range. 

Here is a patch for this issue. It uses a narrower regex to match a time
range. This regex requires time ranges to have ":MM" or an AM/PM
specification in the end time, to prevent mangling strings that are
interpreted as dates, like "11-12".

This patch is a minimal change that gets the code working in the way
that seems to have been intended, so it seems worth applying to maint.

However, the way the code is intended to work doesn't seem right to me,
because it simply throws away time range information at the time prompt.
If you enter a time range like "13:00-14:00" at the time prompt, you
will get a timestamp with "13:00" for the time when the %T template is
expanded. (This is because org-capture-set-target-location uses the
beginning of the entered time range to set :default-time, which must be
an encoded time value, and there is no obvious way to set a time range.)
This is a surprising contrast with the behavior of %^T, which preserves
the time range information in the timestamp entered. But fixing this
will be a larger change and possibly requires some discussion.

-- 
Best,
Richard


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-capture-fix-expansion-of-T-when-capturing-to-a-d.patch --]
[-- Type: text/x-diff, Size: 1326 bytes --]

From a41c751f15488a8b0d48d6b1b1744dbbc003f9f0 Mon Sep 17 00:00:00 2001
From: Richard Lawrence <richard.lawrence@berkeley.edu>
Date: Wed, 6 Jan 2021 11:53:42 +0100
Subject: [PATCH] org-capture: fix expansion of %T when capturing to a datetree

* org-capture.el (org-capture-set-target-location): Use a narrower
regular expression to replace a time range by its start time when
setting :default-time, so that dates do not get mangled.

TINYCHANGE
---
 lisp/org-capture.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index f40f2b335..df0eccdbb 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1038,12 +1038,12 @@ Store them in the capture property list."
 			 (apply #'encode-time 0 0
 				org-extend-today-until
 				(cl-cdddr (decode-time prompt-time))))
-			((string-match "\\([^ ]+\\)-[^ ]+[ ]+\\(.*\\)"
+			((string-match "\\(--?\\([012]?[0-9]\\)\\(\\(:[0-5][0-9]\\)\\|\\(am\\|AM\\|pm\\|PM\\)\\>\\)\\)\\(.*\\)"
 				       org-read-date-final-answer)
 			 ;; Replace any time range by its start.
 			 (apply #'encode-time
 				(org-read-date-analyze
-				 (replace-match "\\1 \\2" nil nil
+				 (replace-match "\\6" nil nil
 						org-read-date-final-answer)
 				 prompt-time (decode-time prompt-time))))
 			(t prompt-time)))
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] incorrect timestamps with :time-prompt and datetrees
  2021-01-06 12:16   ` Richard Lawrence
@ 2021-01-12  8:41     ` Richard Lawrence
  2021-01-18  6:35     ` Bug: " Kyle Meyer
  1 sibling, 0 replies; 6+ messages in thread
From: Richard Lawrence @ 2021-01-12  8:41 UTC (permalink / raw)
  To: emacs-orgmode

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

Hi everyone,

Bumping this, since I forgot to put "PATCH" in the subject line before.

Richard Lawrence <richard.lawrence@uni-tuebingen.de> writes:

> Here is a patch for this issue. It uses a narrower regex to match a time
> range. This regex requires time ranges to have ":MM" or an AM/PM
> specification in the end time, to prevent mangling strings that are
> interpreted as dates, like "11-12".
>
> This patch is a minimal change that gets the code working in the way
> that seems to have been intended, so it seems worth applying to maint.
>
> However, the way the code is intended to work doesn't seem right to me,
> because it simply throws away time range information at the time prompt.
> If you enter a time range like "13:00-14:00" at the time prompt, you
> will get a timestamp with "13:00" for the time when the %T template is
> expanded. (This is because org-capture-set-target-location uses the
> beginning of the entered time range to set :default-time, which must be
> an encoded time value, and there is no obvious way to set a time range.)
> This is a surprising contrast with the behavior of %^T, which preserves
> the time range information in the timestamp entered. But fixing this
> will be a larger change and possibly requires some discussion.

-- 
Best,
Richard


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-capture-fix-expansion-of-T-when-capturing-to-a-d.patch --]
[-- Type: text/x-diff, Size: 1326 bytes --]

From a6c223664aad6096943e236c9a51c30246e57669 Mon Sep 17 00:00:00 2001
From: Richard Lawrence <richard.lawrence@berkeley.edu>
Date: Wed, 6 Jan 2021 11:53:42 +0100
Subject: [PATCH] org-capture: fix expansion of %T when capturing to a datetree

* org-capture.el (org-capture-set-target-location): Use a narrower
regular expression to replace a time range by its start time when
setting :default-time, so that dates do not get mangled.

TINYCHANGE
---
 lisp/org-capture.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index f40f2b335..df0eccdbb 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1038,12 +1038,12 @@ Store them in the capture property list."
 			 (apply #'encode-time 0 0
 				org-extend-today-until
 				(cl-cdddr (decode-time prompt-time))))
-			((string-match "\\([^ ]+\\)-[^ ]+[ ]+\\(.*\\)"
+			((string-match "\\(--?\\([012]?[0-9]\\)\\(\\(:[0-5][0-9]\\)\\|\\(am\\|AM\\|pm\\|PM\\)\\>\\)\\)\\(.*\\)"
 				       org-read-date-final-answer)
 			 ;; Replace any time range by its start.
 			 (apply #'encode-time
 				(org-read-date-analyze
-				 (replace-match "\\1 \\2" nil nil
+				 (replace-match "\\6" nil nil
 						org-read-date-final-answer)
 				 prompt-time (decode-time prompt-time))))
 			(t prompt-time)))
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Bug: incorrect timestamps with :time-prompt and datetrees
  2021-01-06 12:16   ` Richard Lawrence
  2021-01-12  8:41     ` [PATCH] " Richard Lawrence
@ 2021-01-18  6:35     ` Kyle Meyer
  2021-01-19  2:25       ` [PATCH] capture: Fix handling of time range for :time-prompt Kyle Meyer
  1 sibling, 1 reply; 6+ messages in thread
From: Kyle Meyer @ 2021-01-18  6:35 UTC (permalink / raw)
  To: Richard Lawrence; +Cc: emacs-orgmode

Richard Lawrence writes:

> Hi everyone,
>
> Richard Lawrence <richard.lawrence@uni-tuebingen.de> writes:
>
>> I can now confirm that this is the problem. It looks like what is happening here
>> is that the regex is meant to match a time range, but ends up matching
>> the date: thus a string like "12-31 13:00" gets mangled to "12 13:00"
>> and sent into org-read-date-analyze.
>
>> So I guess the solution is...a better regex is needed to cause this
>> branch to fire only in the intended case, namely, a time range. 
>
> Here is a patch for this issue. It uses a narrower regex to match a time
> range. This regex requires time ranges to have ":MM" or an AM/PM
> specification in the end time, to prevent mangling strings that are
> interpreted as dates, like "11-12".

Thanks for the detailed analysis and the patch.

> This patch is a minimal change that gets the code working in the way
> that seems to have been intended, so it seems worth applying to maint.

Yes, seems so.  The original change came in b61ff117b (org-capture.el:
Set a correct time value with file+datetree+prompt, 2012-09-24), and I
believe the related thread is
<https://orgmode.org/list/20120923194954.GE25237@boo.workgroup/T/#u>.

I'm okay with the minimal fix to the regexp, though I wonder if we can
avoid the follow-up call to org-read-date-analyze entirely.  I'm running
out of time to test thoroughly tonight, but perhaps something like this
(ignoring the potential cond->if cleanup):

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 038b24312..04e56d655 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1026,27 +1026,19 @@ (defun org-capture-set-target-location (&optional target)
 	      ((or (org-capture-get :time-prompt)
 		   (equal current-prefix-arg 1))
 	       ;; Prompt for date.
-	       (let* ((org-end-time-was-given nil)
+               (let* ((org-time-was-given nil)
+                      (org-end-time-was-given nil)
                       (prompt-time (org-read-date
 				    nil t nil "Date for tree entry:")))
 		 (org-capture-put
 		  :default-time
-		  (cond ((and (or (not (boundp 'org-time-was-given))
-				  (not org-time-was-given))
+                  (cond ((and (or (not org-time-was-given))
 			      (not (= (time-to-days prompt-time) (org-today))))
 			 ;; Use 00:00 when no time is given for another
 			 ;; date than today?
 			 (apply #'encode-time 0 0
 				org-extend-today-until
 				(cl-cdddr (decode-time prompt-time))))
-			((string-match "\\([^ ]+\\)-[^ ]+[ ]+\\(.*\\)"
-				       org-read-date-final-answer)
-			 ;; Replace any time range by its start.
-			 (apply #'encode-time
-				(org-read-date-analyze
-				 (replace-match "\\1 \\2" nil nil
-						org-read-date-final-answer)
-				 prompt-time (decode-time prompt-time))))
 			(t prompt-time)))
 		 (time-to-days prompt-time)))
 	      (t
--8<---------------cut here---------------end--------------->8---

> However, the way the code is intended to work doesn't seem right to me,
> because it simply throws away time range information at the time prompt.
> If you enter a time range like "13:00-14:00" at the time prompt, you
> will get a timestamp with "13:00" for the time when the %T template is
> expanded. (This is because org-capture-set-target-location uses the
> beginning of the entered time range to set :default-time, which must be
> an encoded time value, and there is no obvious way to set a time range.)
> This is a surprising contrast with the behavior of %^T, which preserves
> the time range information in the timestamp entered. But fixing this
> will be a larger change and possibly requires some discussion.

Hmm, I haven't wrapped my head around that enough to know what I think,
but perhaps the thread I pointed to above is relevant, at least for
historical context.

> Subject: [PATCH] org-capture: fix expansion of %T when capturing to a datetree
>
> * org-capture.el (org-capture-set-target-location): Use a narrower
> regular expression to replace a time range by its start time when
> setting :default-time, so that dates do not get mangled.
>
> TINYCHANGE
> ---
>  lisp/org-capture.el | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lisp/org-capture.el b/lisp/org-capture.el
> index f40f2b335..df0eccdbb 100644
> --- a/lisp/org-capture.el
> +++ b/lisp/org-capture.el
> @@ -1038,12 +1038,12 @@ Store them in the capture property list."
>  			 (apply #'encode-time 0 0
>  				org-extend-today-until
>  				(cl-cdddr (decode-time prompt-time))))
> -			((string-match "\\([^ ]+\\)-[^ ]+[ ]+\\(.*\\)"
> +			((string-match "\\(--?\\([012]?[0-9]\\)\\(\\(:[0-5][0-9]\\)\\|\\(am\\|AM\\|pm\\|PM\\)\\>\\)\\)\\(.*\\)"
>  				       org-read-date-final-answer)

Judging by org-plain-time-of-day-regexp, mixed case (e.g. "Pm") is
allowed too.

Also, you could make it so that the reader doesn't need to count regexp
groups by dropping the trailing group and then just using an empty
string with replace-match.  Non-capturing groups could also be used,
though that'd make an already long regexp longer.

>  			 ;; Replace any time range by its start.
>  			 (apply #'encode-time
>  				(org-read-date-analyze
> -				 (replace-match "\\1 \\2" nil nil
> +				 (replace-match "\\6" nil nil
>  						org-read-date-final-answer)
>  				 prompt-time (decode-time prompt-time))))
>  			(t prompt-time)))
> -- 
> 2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] capture: Fix handling of time range for :time-prompt
  2021-01-18  6:35     ` Bug: " Kyle Meyer
@ 2021-01-19  2:25       ` Kyle Meyer
  0 siblings, 0 replies; 6+ messages in thread
From: Kyle Meyer @ 2021-01-19  2:25 UTC (permalink / raw)
  To: Richard Lawrence; +Cc: emacs-orgmode

Kyle Meyer writes:

> I'm okay with the minimal fix to the regexp, though I wonder if we can
> avoid the follow-up call to org-read-date-analyze entirely.  I'm running
> out of time to test thoroughly tonight, but perhaps something like this
> (ignoring the potential cond->if cleanup):

Testing that against the cases in your initial report, I believe it
leads to the same results as your patch, so here's a cleaned-up version.

-- >8 --
Subject: [PATCH] capture: Fix handling of time range for :time-prompt

* lisp/org-capture.el (org-capture-set-target-location): Bind
org-end-time-was-given around the org-read-date call to get a return
value that uses the start time rather than doing custom adjustment of
the return value.

If org-capture-set-target-location detects that the answer to
org-read-date has a time range, it strips the end time from the answer
and calls org-read-date-analyze again.  (org-read-date already calls
it underneath.)  The regexp it uses, however, can easily match a date,
leading to a bogus result.

org-read-date-analyze is already capable of processing the time range
in a way that matches org-capture-set-target-location's intent: when
org-end-time-was-given is bound, org-read-date-analyze splits off the
end value of the range and stores it in org-end-time-was-given.  Drop
the custom logic and let org-read-date-analyze handle the range.

Reported-by: Richard Lawrence <richard.lawrence@berkeley.edu>
Ref: https://orgmode.org/list/87h7obh4ct.fsf@aquinas
---
 lisp/org-capture.el | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index f40f2b335..d7b69f228 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1025,28 +1025,23 @@ (defun org-capture-set-target-location (&optional target)
 	       (time-to-days org-overriding-default-time))
 	      ((or (org-capture-get :time-prompt)
 		   (equal current-prefix-arg 1))
-	       ;; Prompt for date.
-	       (let ((prompt-time (org-read-date
-				   nil t nil "Date for tree entry:")))
+               ;; Prompt for date.  Bind `org-end-time-was-given' so
+               ;; that `org-read-date-analyze' handles the time range
+               ;; case and returns `prompt-time' with the start value.
+               (let* ((org-time-was-given nil)
+                      (org-end-time-was-given nil)
+                      (prompt-time (org-read-date
+				    nil t nil "Date for tree entry:")))
 		 (org-capture-put
 		  :default-time
-		  (cond ((and (or (not (boundp 'org-time-was-given))
-				  (not org-time-was-given))
-			      (not (= (time-to-days prompt-time) (org-today))))
-			 ;; Use 00:00 when no time is given for another
-			 ;; date than today?
-			 (apply #'encode-time 0 0
-				org-extend-today-until
-				(cl-cdddr (decode-time prompt-time))))
-			((string-match "\\([^ ]+\\)-[^ ]+[ ]+\\(.*\\)"
-				       org-read-date-final-answer)
-			 ;; Replace any time range by its start.
-			 (apply #'encode-time
-				(org-read-date-analyze
-				 (replace-match "\\1 \\2" nil nil
-						org-read-date-final-answer)
-				 prompt-time (decode-time prompt-time))))
-			(t prompt-time)))
+                  (if (and (or (not org-time-was-given))
+                           (not (= (time-to-days prompt-time) (org-today))))
+                      ;; Use 00:00 when no time is given for another
+                      ;; date than today?
+                      (apply #'encode-time 0 0
+                             org-extend-today-until
+                             (cl-cdddr (decode-time prompt-time)))
+                    prompt-time))
 		 (time-to-days prompt-time)))
 	      (t
 	       ;; Current date, possibly corrected for late night

base-commit: 9e8215f4a5df7d03ac787da78d28f69a4c18e7d3
-- 
2.29.2



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-01-19  2:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-24 10:42 Bug: incorrect timestamps with :time-prompt and datetrees Richard Lawrence
2020-12-24 12:07 ` Richard Lawrence
2021-01-06 12:16   ` Richard Lawrence
2021-01-12  8:41     ` [PATCH] " Richard Lawrence
2021-01-18  6:35     ` Bug: " Kyle Meyer
2021-01-19  2:25       ` [PATCH] capture: Fix handling of time range for :time-prompt Kyle Meyer

Org-mode mailing list

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://orgmode.org/list/0 list/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 list list/ https://orgmode.org/list \
		emacs-orgmode@gnu.org
	public-inbox-index list

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.orgmode
	nntp://news.gmane.io/gmane.emacs.orgmode


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git