Hello Ihor! To fight my perfectionism I tried to finish this one quick and dirty. To make everything perfect in this section of our codebase would take forever (I tried and gave up).
Ihor Radchenko <[email protected]> writes: > Morgan Smith <[email protected]> writes: > >> + (setq n (prefix-numeric-value n)) > > I think that is a wrong place to do it. > According to the docstring "Optional argument N tells to change by that > many units.", so it should already be a number. > We need to fix the callers instead. I'll just put a FIXME there instead of fixing it. I didn't cause this issue, the code already does that (just not as explicitly). > >> + (let ((original-point (point)) > > Better use marker or something, I think. > `org-timestamp-change' can change nearby clocks, leading to point being > completely off. org-timestamp-change deletes and does a fresh insert of the timestamp which causes markers and save-excursion to be completely off. If you want this to be nicer then you'll have to make `org-timestamp-change' play nicer. This solution might not be ideal but it yields the correct result most of the time. > >> + (move-beginning-of-line 1) > > If we are rewriting this anyway, better use (forward-line 0). That will > ignore fields and invisible text. done. > >> + (looking-at regex) > > So, what will happen if we are not on a clock entry, and there are three > or more timestamps on the current line? > > [2026-04-04 Sat] [2026-04-04 Sat] [2026-04<point>-04 Sat] > > And what if we are at diary sexp timestamp? > >> + (goto-char (1+ (match-beginning 1))) >> + (org-timestamp-change tschange timestamp? 'updown) >> + (when (looking-at regex) > > This will search for the next timestamp following the changed timestamp. > What if that's not a clock? > > [2026<point>-04-04 Sat] [2026-04-04 Sat] Ok I hear you. I need to use a more specific regex. Unfortunately, we don't have the regex I want so I have to make it myself. See patch 1 and 2 which are new and introduce it. I have made the executive decision to allow negative durations in clock lines. Personally I accidentally make negative durations all the time. I'll edit a timestamp and hit "C-c C-c" and see a negative duration. This lets me know I messed something up. The specific reason I need to allow negative durations for this change is because I create negative duration in an intermediate step. If I want to increment the following by one hour CLOCK: [2026-06-07 Sun 10:00]--[2026-06-07 Sun 10:30] => 0:30 I'll have an intermediate step that looks like this CLOCK: [2026-06-07 Sun 11:00]--[2026-06-07 Sun 10:30] => -0:30 If I incremented the second timestamp first I could avoid negative durations but I'd have to invert that logic when I decrement timestamps. So just allowing negative durations seems easier to me.
>From 6b8ebd8a0d1f31fcec7a564ee66fa58ba90da331 Mon Sep 17 00:00:00 2001 From: Morgan Smith <[email protected]> Date: Sun, 5 Apr 2026 18:23:40 -0400 Subject: [PATCH 1/3] 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: a6854849052129c25622f30cde038a660f4f29d0 -- 2.54.0
>From 8e703671bfb8a8605b356e2423fa85f23186ebea Mon Sep 17 00:00:00 2001 From: Morgan Smith <[email protected]> Date: Sun, 7 Jun 2026 20:20:19 -0400 Subject: [PATCH 2/3] org-element: Allow clock durations to be negative While a clock entry with a negative duration probably doesn't have much utility, they are extremely easy to make accidentally. If we stop parsing them just because someone incremented a timestamp one too many times, that wouldn't be very nice. * lisp/org-element.el (org-element-clock-line-re) (org-element-clock-line-re-no-group): Allow clock durations to be prefixed with "-". --- lisp/org-element.el | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lisp/org-element.el b/lisp/org-element.el index fe78dfb2c..bffd25b72 100644 --- a/lisp/org-element.el +++ b/lisp/org-element.el @@ -126,7 +126,7 @@ org-element-citation-prefix-re ((duration (seq (1+ (or ?\t ?\s)) "=>" (1+ (or ?\t ?\s)) - (1+ digit) ":" digit digit)) + (opt "-") (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 -- 2.54.0
>From e896013b1237a18afa042c8b2823f477737653a2 Mon Sep 17 00:00:00 2001 From: Morgan Smith <[email protected]> Date: Sun, 29 Mar 2026 14:12:54 -0400 Subject: [PATCH 3/3] 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 | 73 +++++++++++++--------------------- testing/lisp/test-org-clock.el | 46 ++++++++++++++------- 2 files changed, 59 insertions(+), 60 deletions(-) diff --git a/lisp/org-clock.el b/lisp/org-clock.el index a1de045fe..bd1b2278e 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,34 @@ 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) + ;; FIXME: this is the wrong way to deal with prefix arguments and is + ;; in conflict with the docstring + (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-let* ((timestamp-match-group + (and (looking-at org-element-clock-line-re) + (or (and (org-point-in-group original-point 1) 1) + (and (org-point-in-group original-point 2) 2))))) + (goto-char (match-beginning timestamp-match-group)) + (org-timestamp-change tschange timestamp? 'updown) + ;; If we just changed the first one, now change the second + (when (eq timestamp-match-group 1) + ;; 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 2)) + (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
