Ihor Radchenko <[email protected]> writes: > Morgan Smith <[email protected]> writes: > >>>> The specific reason I need to allow negative durations for this change >>>> is because I create negative duration in an intermediate step. >>> >>> Are you sure that we want to change Org syntax spec just to make the >>> implementation simpler? That's going to be a breaking change. >> >> Despite the time I've spent studying the parser, I honestly have no clue >> what the implications of this change would be. > > The reasoning is simple. > We have https://orgmode.org/worg/org-syntax.html > It affects programs beyond Elisp. > Any changes in the syntax (like allowing "-" in clock line duration), > will affect literally everything that needs to parse Org mode files. > Such changes should never be taken lightly, and we need to think about > implications beyond Emacs.
I did not think of this. This is a good point. Thanks for pointing that out! >> Optional argument N tells to change by that many units." >> - (let ((tschange (if (eq updown 'up) 'org-timestamp-up >> - 'org-timestamp-down)) >> - (timestamp? (org-at-timestamp-p 'lax)) >> - ts1 begts1 ts2 begts2 updatets1 tdiff) >> + ;; FIXME: this is the wrong way to deal with prefix arguments and is >> + ;; in conflict with the docstring >> + (setq n (prefix-numeric-value n)) > > Why do you need to add this at all? You could simply change the > interactive spec in the two callers of org-clock-timestamp-change. I threw the FIXME in there because I didn't want to investigate how prefix arguments or interactive statements work. I've now done minimal studying and have come to following conclusion: (prefix-numeric-value INTEGER) => INTEGER (prefix-numeric-value nil) => 1 Which means this actually isn't in conflict with the docstring and shouldn't cause any issues whatsoever. If we want to go through all the org functions like this to make sure the interactive specs are correct and we aren't calling `prefix-numeric-value' unnecessarily, then there are probably about 15 functions we need to change. However, I don't really see any particular issue so I'm just going to remove the FIXME entirely. Thanks for fixing the tests on emacs 28! I was wondering why my CI was broken and I was blaming my CI instead of the code. Not really sure how I managed to break it myself and not notice... Tests pass after each patch on emacs 30.2. Tests pass after final patch on emacs 28 and 29 and a recent emacs/master build. Tests pass after final patch with TZ set to UTC, Europe/Istanbul, and America/New_York.
>From 656be2556f76e5e7e811238861e0e65126aa9681 Mon Sep 17 00:00:00 2001 From: Morgan Smith <[email protected]> Date: Sun, 5 Apr 2026 18:23:40 -0400 Subject: [PATCH 1/2] org-element: Add match groups to `org-element-clock-line-re' * lisp/org-element.el (org-element-clock-line-re): Add 3 match groups and document them in the docstring. (org-element-clock-line-re-no-group): New regex that has the old value of 'org-element-clock-line-re'. (org-element--set-regexps, org-element--current-element): Use 'org-element-clock-line-re-no-group'. --- lisp/org-element.el | 56 +++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/lisp/org-element.el b/lisp/org-element.el index 313922ecc..fe78dfb2c 100644 --- a/lisp/org-element.el +++ b/lisp/org-element.el @@ -122,26 +122,38 @@ org-element-citation-prefix-re "Regexp matching a citation prefix. Style, if any, is located in match group 1.") -(defconst org-element-clock-line-re - (let ((duration ; "=> 212:12" - '(seq - (1+ (or ?\t ?\s)) "=>" (1+ (or ?\t ?\s)) - (1+ digit) ":" digit digit))) - (rx-to-string - `(seq - line-start (0+ (or ?\t ?\s)) - "CLOCK:" - (or - (seq - (1+ (or ?\t ?\s)) - (regexp ,org-ts-regexp-inactive) - (opt "--" - (regexp ,org-ts-regexp-inactive) - ,duration)) - ,duration) - (0+ (or ?\t ?\s)) - line-end))) - "Regexp matching a clock line.") +(rx-let + ((duration + (seq + (1+ (or ?\t ?\s)) "=>" (1+ (or ?\t ?\s)) + (1+ digit) ":" digit digit)) + (before (seq line-start (0+ (or ?\t ?\s)) "CLOCK:")) + (after (seq (0+ (or ?\t ?\s)) line-end))) + (defconst org-element-clock-line-re-no-group + (rx before + (or + (seq (1+ (or ?\t ?\s)) + (regexp org-ts-regexp-inactive) + (opt "--" + (regexp org-ts-regexp-inactive) + duration)) + duration) + after) + "Regexp matching a clock line.") + (defconst org-element-clock-line-re + (rx before + (or + (seq (1+ (or ?\t ?\s)) + (group-n 1 (regexp org-ts-regexp-inactive)) + (opt "--" + (group-n 2 (regexp org-ts-regexp-inactive)) + (group-n 3 duration))) + (group-n 3 duration)) + after) + "Regexp matching a clock line. +The first timestamp is in match group 1. +The second timestamp is in match group 2. +The duration is in match group 3.")) (defconst org-element-comment-string "COMMENT" "String marker for commented headlines.") @@ -244,7 +256,7 @@ org-element--set-regexps ;; LaTeX environments. "\\\\begin{\\([A-Za-z0-9*]+\\)}" "\\|" ;; Clock lines. - org-element-clock-line-re "\\|" + org-element-clock-line-re-no-group "\\|" ;; Lists. (let ((term (pcase org-plain-list-ordered-item-terminator (?\) ")") (?. "\\.") (_ "[.)]"))) @@ -4823,7 +4835,7 @@ org-element--current-element ;; a footnote definition: next item is always a paragraph. ((not (bolp)) (org-element-paragraph-parser limit (list (point)))) ;; Clock. - ((looking-at-p org-element-clock-line-re) (org-element-clock-parser limit)) + ((looking-at-p org-element-clock-line-re-no-group) (org-element-clock-parser limit)) ;; Inlinetask. (at-task? (org-element-inlinetask-parser limit raw-secondary-p)) ;; From there, elements can have affiliated keywords. base-commit: a8e1aefa94fc539e0b7019f3f3f84f1517e3cd18 -- 2.54.0
>From f5c44faa5c2a2dbd02e14c806784bca08df99ac5 Mon Sep 17 00:00:00 2001 From: Morgan Smith <[email protected]> Date: Sun, 29 Mar 2026 14:12:54 -0400 Subject: [PATCH 2/2] lisp/org-clock.el: Rewrite 'org-clock-timestamps-change' The previous implementation had a couple bugs. 1. It did strange things when the timestamps changed length mid-operation (like if the weekday name got added during the call to `org-timestamp-change'). 2. In certain instances it double incremented the first timestamp, leaving the second one unchanged. I assume this was also due to managing the point wrong. 3. When used on a single timestamp, it would find the next timestamp in the buffer and increment that one too. * lisp/org-clock.el (org-clock-timestamps-change): Rewrite. * testing/lisp/test-org-clock.el (test-org-clock/org-clock-timestamps-change): Use 'string-equal' instead of 'equal'. Add a test for a case that was previously failing. Allow testing up to day 28 in month. Test that point does not change. --- lisp/org-clock.el | 70 ++++++++++++---------------------- testing/lisp/test-org-clock.el | 46 ++++++++++++++-------- 2 files changed, 56 insertions(+), 60 deletions(-) diff --git a/lisp/org-clock.el b/lisp/org-clock.el index a1de045fe..f68f43e1f 100644 --- a/lisp/org-clock.el +++ b/lisp/org-clock.el @@ -61,6 +61,7 @@ org-element-use-cache (defvar org-frame-title-format-backup nil) (defvar org-state) (defvar org-link-bracket-re) +(defvar org-element-clock-line-re) (defgroup org-clock nil "Options concerning clocking working time in Org mode." @@ -1932,52 +1933,31 @@ org-clock-timestamps-change "Change CLOCK timestamps synchronously at cursor. UPDOWN tells whether to change `up' or `down'. Optional argument N tells to change by that many units." - (let ((tschange (if (eq updown 'up) 'org-timestamp-up - 'org-timestamp-down)) - (timestamp? (org-at-timestamp-p 'lax)) - ts1 begts1 ts2 begts2 updatets1 tdiff) + (setq n (prefix-numeric-value n)) + (let ((original-point (point)) + (tschange (if (eq updown 'up) n (- n))) + (timestamp? (org-at-timestamp-p 'lax))) (when (not (memq timestamp? '(nil bracket after))) - (save-excursion - (move-beginning-of-line 1) - (re-search-forward org-ts-regexp3 nil t) - (setq ts1 (match-string 0) begts1 (match-beginning 0)) - (when (re-search-forward org-ts-regexp3 nil t) - (setq ts2 (match-string 0) begts2 (match-beginning 0)))) - ;; Are we on the second timestamp? - (if (<= begts2 (point)) (setq updatets1 t)) - (if (not ts2) - ;; fall back on org-timestamp-up if there is only one - (funcall tschange n) - (funcall tschange n) - (let ((ts (if updatets1 ts2 ts1)) - (begts (if updatets1 begts1 begts2))) - (setq tdiff - (time-subtract - (org-time-string-to-time - (save-excursion - (goto-char (if updatets1 begts2 begts1)) - (looking-at org-ts-regexp3) - (match-string 0))) - (org-time-string-to-time ts))) - ;; `save-excursion' won't work because - ;; `org-timestamp-change' deletes and re-inserts the - ;; timestamp. - (let ((origin (point))) - (save-excursion - (goto-char begts) - (org-timestamp-change - (round (/ (float-time tdiff) - (pcase-exhaustive timestamp? - (`minute 60) - (`hour 3600) - (`day (* 24 3600)) - (`month (* 24 3600 31)) - (`year (* 24 3600 365.2))))) - timestamp? 'updown)) - ;; Move back to initial position, but never beyond updated - ;; clock. - (unless (< (point) origin) - (goto-char origin)))))))) + (forward-line 0) + (when (looking-at org-element-clock-line-re) + ;; Negative durations break the clock parser so we need to + ;; decide the increment order carefully. + (let* ((first-stop (or (and (> tschange 0) 2) 1)) + (second-stop (1+ (% first-stop 2)))) + (goto-char (match-beginning first-stop)) + (org-timestamp-change tschange timestamp? 'updown) + ;; Now that the first timestamp has been changed, the + ;; timestamp length and point might be different. So we have + ;; to run the search again from scratch. + (forward-line 0) + (looking-at org-element-clock-line-re) + (goto-char (match-beginning second-stop)) + (org-timestamp-change tschange timestamp? 'updown))) + ;; `save-excursion' or markers won't work because + ;; `org-timestamp-change' deletes and re-inserts the + ;; timestamp. Jump back to the numerical point which will be + ;; wrong if the timestamp has changed length + (goto-char original-point)))) ;;;###autoload (defun org-clock-cancel () diff --git a/testing/lisp/test-org-clock.el b/testing/lisp/test-org-clock.el index befd0e3e3..e336def5d 100644 --- a/testing/lisp/test-org-clock.el +++ b/testing/lisp/test-org-clock.el @@ -100,7 +100,7 @@ test-org-clock/org-clock-timestamps-change (let ((sun (org-test-get-day-name "Sun")) (mon (org-test-get-day-name "Mon"))) (should - (equal + (string-equal (format "CLOCK: [2023-02-19 %s 21:30]--[2023-02-19 %s 23:35] => 2:05" sun sun) (org-test-with-temp-text @@ -108,7 +108,7 @@ test-org-clock/org-clock-timestamps-change (org-clock-timestamps-change 'down 1) (buffer-string)))) (should - (equal + (string-equal (format "CLOCK: [2023-02-20 %s 00:00]--[2023-02-20 %s 00:40] => 0:40" mon mon) (org-test-with-temp-text @@ -116,12 +116,28 @@ test-org-clock/org-clock-timestamps-change (org-clock-timestamps-change 'up 1) (buffer-string)))) (should - (equal + (string-equal (format "CLOCK: [2023-02-20 %s 00:30]--[2023-02-20 %s 01:35] => 1:05" mon mon) (org-test-with-temp-text "CLOCK: [2023-02-19 Sun 2<point>3:30]--[2023-02-20 Mon 00:35] => 1:05" (org-clock-timestamps-change 'up 1) + (buffer-string)))) + (should + (string-equal + (format "CLOCK: [2026-03-29 %s 23:33]--[2026-03-30 %s 00:33] => 1:00" + sun mon) + (org-test-with-temp-text + "CLOCK: [2026-03-2<point>8 23:33]--[2026-03-29 00:33] => 1:00" + (org-clock-timestamps-change 'up 1) + (buffer-string)))) + ;; Don't touch things that aren't clocks + (should + (string-equal + "[2026-03-28 23:33]\n[2026-03-29 00:33]" + (org-test-with-temp-text + "[2026-03-2<point>8 23:33]\n[2026-03-29 00:33]" + (org-clock-timestamps-change 'up 1) (buffer-string))))) (let ((org-time-stamp-rounding-minutes '(1 1)) ;; No rounding! (now (decode-time)) test-text point) @@ -129,7 +145,7 @@ test-org-clock/org-clock-timestamps-change ;; 28th. This particular test is easier to write if the ;; days don't change when modifying the month. (setf (decoded-time-day now) - (min (decoded-time-day now) 27)) + (min (decoded-time-day now) 28)) (setq now (encode-time now)) ;; loop over regular timestamp formats and weekday-less timestamp ;; formats @@ -149,19 +165,19 @@ test-org-clock/org-clock-timestamps-change (while (not (eolp)) ;; change the timestamp unit at point one down, two up, ;; one down, which should give us the original timestamp - ;; again. However, point can move backward during that - ;; operation, so take care of that. *Not* using - ;; `save-excursion', which fails to restore point since - ;; the timestamp gets completely replaced. + ;; again. (setq point (point)) (org-clock-timestamps-down) - (let ((current-prefix-arg '(2))) - ;; We supply the prefix argument 2 to increment the - ;; minutes by 2. We supply the function argument 2 for - ;; everything else. Is this a bug? Probably. FIXME. - (org-clock-timestamps-up 2)) - (org-clock-timestamps-down) - (goto-char point) + (org-test-ignore-duplicate + (should (eq point (point))) + (let ((current-prefix-arg '(2))) + ;; We supply the prefix argument 2 to increment the + ;; minutes by 2. We supply the function argument 2 for + ;; everything else. Is this a bug? Probably. FIXME. + (org-clock-timestamps-up 2)) + (should (eq point (point))) + (org-clock-timestamps-down) + (should (eq point (point)))) (should (string= (buffer-substring (point-min) (point-max)) test-text)) -- 2.54.0
