emacs-orgmode@gnu.org archives
 help / color / mirror / code / 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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
  2021-01-31 11:15         ` Richard Lawrence
  0 siblings, 1 reply; 9+ 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] 9+ messages in thread

* Re: [PATCH] capture: Fix handling of time range for :time-prompt
  2021-01-19  2:25       ` [PATCH] capture: Fix handling of time range for :time-prompt Kyle Meyer
@ 2021-01-31 11:15         ` Richard Lawrence
  2021-02-02  4:42           ` Kyle Meyer
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Lawrence @ 2021-01-31 11:15 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: emacs-orgmode

Dear Kyle and all,

Thanks for following up! Sorry it's taken me so long to reply.

Kyle Meyer <kyle@kyleam.com> writes:

> 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.

I confirm that this cleaned up version works for me and gets the same
results for my test cases. I think it should be applied (modulo one
nitpick below). Are you willing to apply it, Kyle? I don't have commit
rights myself.

> -- >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))

Nitpick: (or (not org-time-was-given)) is equivalent to (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

As for this:

> 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>.

Thanks for the reference to this thread. I would like to be able to do
exactly what Gregor mentioned there:

- be prompted when capturing for the date and time / time range of the
  event/appointment.
- record the event/appointment in a date tree under the date entered at
  the prompt
- have a timestamp with the full time information entered at the prompt
  appear in the resulting heading

In short: enter the full date and time information *once*, and use this
both to place the entry in the datetree and to give it a timestamp.

As far as I can tell, that is not fully possible today, even with this
patch. The reason is that time *range* information entered at the prompt
generated by :time-prompt gets thrown away. The reason for *that* is
that org-read-date is called with t as its to-time argument (the second
argument), so that the date is returned in Emacs' internal time
representation, which doesn't represent ranges.

Bastien's recommended solution back then was to use %^t and %^T in the
capture template instead of %t and %T. The problem with that is that it
requires entering the date twice: once at the prompt generated by
:time-prompt, and once at the prompt to replace the %^T in the template.

Could we instead save, say, :start-time and :end-time values in
org-capture-plist after the :time-prompt, and use them to populate %T,
%U, etc. in the template? This seems like the right solution to me, but
it might require modifying org-read-date, which is a hairy piece of
code. What do others think about this? Should I come up with a patch for
this, or is there a different solution that the community and
maintainers would prefer?

-- 
Best,
Richard


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

* Re: [PATCH] capture: Fix handling of time range for :time-prompt
  2021-01-31 11:15         ` Richard Lawrence
@ 2021-02-02  4:42           ` Kyle Meyer
  2021-02-02 16:59             ` Richard Lawrence
  0 siblings, 1 reply; 9+ messages in thread
From: Kyle Meyer @ 2021-02-02  4:42 UTC (permalink / raw)
  To: Richard Lawrence; +Cc: emacs-orgmode

Richard Lawrence writes:

> Kyle Meyer <kyle@kyleam.com> writes:
>
>> 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.
>
> I confirm that this cleaned up version works for me and gets the same
> results for my test cases. I think it should be applied (modulo one
> nitpick below). Are you willing to apply it, Kyle? I don't have commit
> rights myself.

Sure.  Thank you for testing it.

>> -		  (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
[...]
>> +                  (if (and (or (not org-time-was-given))
>
> Nitpick: (or (not org-time-was-given)) is equivalent to (not org-time-was-given)

Eh, thanks for spotting my sloppy conversion.  Fixed, though I ended up
rearranging things a bit more to avoid unnecessary `not's that were in
the original code.

Pushed (3e64d3475).

> As for this:
>
>> 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>.
>
> Thanks for the reference to this thread. I would like to be able to do
> exactly what Gregor mentioned there:
>
> - be prompted when capturing for the date and time / time range of the
>   event/appointment.
> - record the event/appointment in a date tree under the date entered at
>   the prompt
> - have a timestamp with the full time information entered at the prompt
>   appear in the resulting heading
>
> In short: enter the full date and time information *once*, and use this
> both to place the entry in the datetree and to give it a timestamp.

This isn't functionality I use myself, so I'm not sure I have things
straight in my head, but, yeah, sounds reasonable.
>
> As far as I can tell, that is not fully possible today, even with this
> patch. The reason is that time *range* information entered at the prompt
> generated by :time-prompt gets thrown away. The reason for *that* is
> that org-read-date is called with t as its to-time argument (the second
> argument), so that the date is returned in Emacs' internal time
> representation, which doesn't represent ranges.

Hmm.  Is it really about the TO-TIME argument?  If org-read-date is
called with TO-TIME as nil, doesn't it still throw away the end of the
range?

  (let ((org-time-was-given nil)
        (org-end-time-was-given nil))
    (org-read-date nil nil nil "Date for tree entry:"))
  ;; enter "3pm-4pm" => "2021-02-01 15:00"

Or, actually, it stores the end in org-end-time-was-given, but it does
that regardless of the TO-TIME argument.

  ;; TO-TIME nil
  (let ((org-time-was-given nil)
        (org-end-time-was-given nil))
    (org-read-date nil nil nil "Date for tree entry:")
    org-end-time-was-given)
  ;; enter "3pm-4pm" => "16:00"

  ;; TO-TIME t
  (let ((org-time-was-given nil)
        (org-end-time-was-given nil))
    (org-read-date nil t nil "Date for tree entry:")
    org-end-time-was-given)
  ;; enter "3pm-4pm" => "16:00"

And the org-time-stamp command uses a non-nil TO-TIME, formatting the
time stamp with something like this:

  ;; org-time-stamp
  (let* ((org-time-was-given nil)
         (org-end-time-was-given nil))
    (org-insert-time-stamp
     (org-read-date nil t nil "Date for tree entry:")
     org-time-was-given
     nil nil nil
     (list org-end-time-was-given)))
  ;; enter "3pm-4pm" => "16:00" => <2021-02-01 Mon 15:00-16:00>

> Bastien's recommended solution back then was to use %^t and %^T in the
> capture template instead of %t and %T. The problem with that is that it
> requires entering the date twice: once at the prompt generated by
> :time-prompt, and once at the prompt to replace the %^T in the template.
>
> Could we instead save, say, :start-time and :end-time values in
> org-capture-plist after the :time-prompt, and use them to populate %T,
> %U, etc. in the template? This seems like the right solution to me, but
> it might require modifying org-read-date, which is a hairy piece of
> code. What do others think about this? Should I come up with a patch for
> this, or is there a different solution that the community and
> maintainers would prefer?

I don't have a good grasp of all the details here yet, but something in
that direction sounds worth considering to me.  And perhaps by carrying
along org-end-time-was-given, you won't need to modify org-read-date.

Thanks.


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

* Re: [PATCH] capture: Fix handling of time range for :time-prompt
  2021-02-02  4:42           ` Kyle Meyer
@ 2021-02-02 16:59             ` Richard Lawrence
  0 siblings, 0 replies; 9+ messages in thread
From: Richard Lawrence @ 2021-02-02 16:59 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: emacs-orgmode

Kyle Meyer <kyle@kyleam.com> writes:

> Pushed (3e64d3475).

Thank you!

>> As far as I can tell, that is not fully possible today, even with this
>> patch. The reason is that time *range* information entered at the prompt
>> generated by :time-prompt gets thrown away. The reason for *that* is
>> that org-read-date is called with t as its to-time argument (the second
>> argument), so that the date is returned in Emacs' internal time
>> representation, which doesn't represent ranges.
>
> Hmm.  Is it really about the TO-TIME argument?  If org-read-date is
> called with TO-TIME as nil, doesn't it still throw away the end of the
> range?
>
>   (let ((org-time-was-given nil)
>         (org-end-time-was-given nil))
>     (org-read-date nil nil nil "Date for tree entry:"))
>   ;; enter "3pm-4pm" => "2021-02-01 15:00"
>
> Or, actually, it stores the end in org-end-time-was-given, but it does
> that regardless of the TO-TIME argument.

Ah, that's interesting, thanks for pointing it out. I thought I had
checked to see if org-end-time-was-given had a value after org-read-date
gets called for a capture :time-prompt, but now I see that of course
that wouldn't work without the surrounding let binding...   

OK, so with your patch, the way is clear to stick this value in
org-capture-plist and make use of it when filling %T and friends in
templates. I'll see if I can come up with a patch to do that, and
report back.

-- 
Best,
Richard


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

end of thread, other threads:[~2021-02-02 17:04 UTC | newest]

Thread overview: 9+ 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
2021-01-31 11:15         ` Richard Lawrence
2021-02-02  4:42           ` Kyle Meyer
2021-02-02 16:59             ` Richard Lawrence

Code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).