emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Kyle Meyer <kyle@kyleam.com>
To: Richard Lawrence <richard.lawrence@uni-tuebingen.de>
Cc: emacs-orgmode@gnu.org
Subject: Re: Bug: incorrect timestamps with :time-prompt and datetrees
Date: Mon, 18 Jan 2021 01:35:52 -0500	[thread overview]
Message-ID: <87o8hm3g6v.fsf@kyleam.com> (raw)
In-Reply-To: <87ble2uuoa.fsf@aquinas>

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


  parent reply	other threads:[~2021-01-18  6:37 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-24 10:42 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     ` Kyle Meyer [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87o8hm3g6v.fsf@kyleam.com \
    --to=kyle@kyleam.com \
    --cc=emacs-orgmode@gnu.org \
    --cc=richard.lawrence@uni-tuebingen.de \
    --subject='Re: Bug: incorrect timestamps with :time-prompt and datetrees' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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