Ihor Radchenko <yanta...@posteo.net> writes:
> Morgan Smith <morgan.j.sm...@outlook.com> writes:
>
>> I apologize for the delay in re-submitting this. I have created a
>> number of tests to test the current implementation. Then I have
>> reimplemented `org-clock-sum' using the org-element api (ensuring all
>> tests still pass). Then I added a new feature that will issue a warning
>> if the timestamp is not fully specified.
>>
>> This brings a big performance improvement.
>
> Thanks!
> I have a few comments.
>
>> From d3d0174bb3bc9be108d50cca0e738db4d9f2598b Mon Sep 17 00:00:00 2001
>> From: Morgan Smith <morgan.j.sm...@outlook.com>
>> Date: Sat, 9 Aug 2025 14:09:20 -0400
>> Subject: [PATCH 4/6] Testing: test for malformed clock lines
>
> This test is failing unless patch #5 is applied. Something is off.
>
Somehow I got the impression that the existing error handling wasn't
doing anything about underspecified timestamps. Not sure how I got that
impression because that test failing is proof that it is working.
I fixed everything up and merged my 5th and 6th patch together. The
error handling also doesn't require condition-case anymore which is
nice.
>> + (when (and org-clock-report-include-clocking-task
>> + (eq (org-clocking-buffer)
>> (current-buffer))
>> + (eq (marker-position org-clock-hd-marker)
>> + (org-element-begin
>> headline-or-inlinetask))
>> + (or (not tstart)
>> + (>= (float-time org-clock-start-time)
>> tstart))
>> + (or (not tend)
>> + (<= (float-time org-clock-start-time)
>> tend)))
>
> This is not how it worked before. Previously, the clocked task was only
> included when tstart and tend are both non-nil. Could you please explain
> this change?
>
I've got a bad habit of wanting to change everything. I've restored the
original conditionals.
This was actually why one of the inlinetask tests failed on the old
org-clock-sum implementation. Now that test passes :).
>> + ;; TODO: can inlinetasks contain inlinetasks?
>
> They cannot.
Thanks!
>From 3a008af13c25f3b87cc4cd7c440255c925a5c5b1 Mon Sep 17 00:00:00 2001
From: Morgan Smith <morgan.j.sm...@outlook.com>
Date: Wed, 17 Apr 2024 17:51:35 -0400
Subject: [PATCH 1/5] Testing: Test clock times without timestamps
* testing/lisp/test-org-clock.el (test-org-clock/clocktable/insert):
Add a clock time that does not include timestamps.
---
testing/lisp/test-org-clock.el | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/testing/lisp/test-org-clock.el b/testing/lisp/test-org-clock.el
index 8a196ee96..62e4d7507 100644
--- a/testing/lisp/test-org-clock.el
+++ b/testing/lisp/test-org-clock.el
@@ -345,13 +345,12 @@ test-org-clock/clocktable/insert
(equal
"| Headline | Time |
|--------------+--------|
-| *Total time* | *1:00* |
+| *Total time* | *2:00* |
|--------------+--------|
-| H1 | 1:00 |"
+| H1 | 2:00 |"
(org-test-with-temp-text "* H1\n<point>"
- (insert (org-test-clock-create-clock ". 1:00" ". 2:00"))
-
- (goto-line 2)
+ (insert (org-test-clock-create-clock ". 1:00" ". 2:00")
+ "CLOCK: => 1:00\n")
(require 'org-clock)
(org-dynamic-block-insert-dblock "clocktable")
--
2.50.1
>From 41f228760a71900125496a85f9fd87d5155b2b0f Mon Sep 17 00:00:00 2001
From: Morgan Smith <morgan.j.sm...@outlook.com>
Date: Wed, 8 May 2024 10:36:07 -0400
Subject: [PATCH 2/5] Testing: New test test-org-clock/clocktable/open-clock
* testing/lisp/test-org-clock.el
(test-org-clock/clocktable/open-clock): New test.
---
testing/lisp/test-org-clock.el | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/testing/lisp/test-org-clock.el b/testing/lisp/test-org-clock.el
index 62e4d7507..1ae91ebba 100644
--- a/testing/lisp/test-org-clock.el
+++ b/testing/lisp/test-org-clock.el
@@ -362,6 +362,39 @@ test-org-clock/clocktable/insert
(point) (progn (search-forward "#+END:") (line-end-position 0))))
(delete-region (point) (search-forward "#+END:\n")))))))
+(ert-deftest test-org-clock/clocktable/open-clock ()
+ "Test open clocks.
+Open clocks should be ignored unless it is clocked in and
+`org-clock-report-include-clocking-task' is t."
+ (let ((time-reported "| Headline | Time |
+|--------------+--------|
+| *Total time* | *1:00* |
+|--------------+--------|
+| H1 | 1:00 |")
+ (time-not-reported "| Headline | Time |
+|--------------+--------|
+| *Total time* | *0:00* |"))
+ (dolist (org-clock-report-include-clocking-task '(nil t))
+ (dolist (actually-clock-in '(nil t))
+ ;; Without leading characters then `org-clock-hd-marker' doesn't
+ ;; get updated when clocktable is inserted and test fails.
+ (org-test-with-temp-text "\n*<point> H1\n"
+ (should
+ (equal
+ (if (and org-clock-report-include-clocking-task
+ actually-clock-in)
+ time-reported
+ time-not-reported)
+ (progn
+ (if actually-clock-in
+ (org-clock-in nil (- (float-time) (* 60 60)))
+ (goto-char (point-max))
+ (insert (org-test-clock-create-clock "-1h")))
+ ;; Unless tstart and tend are fully specified it doesn't work
+ (test-org-clock-clocktable-contents ":tstart \"<-2d>\" :tend \"<tomorrow>\""))))
+ (when actually-clock-in
+ (org-clock-cancel)))))))
+
(ert-deftest test-org-clock/clocktable/ranges ()
"Test ranges in Clock table."
;; Relative time: Previous two days.
--
2.50.1
>From c30bc66b5ba46ed17605d7b5274a86f638eeb9e2 Mon Sep 17 00:00:00 2001
From: Morgan Smith <morgan.j.sm...@outlook.com>
Date: Sat, 9 Aug 2025 14:04:07 -0400
Subject: [PATCH 3/5] Testing: Add tests for clocktables from inline tasks
* testing/lisp/test-org-clock.el
(test-org-clock/clocktable/inlinetask/insert)
(test-org-clock/clocktable/inlinetask/open-clock): New tests
---
testing/lisp/test-org-clock.el | 73 ++++++++++++++++++++++++++++++++++
1 file changed, 73 insertions(+)
diff --git a/testing/lisp/test-org-clock.el b/testing/lisp/test-org-clock.el
index 1ae91ebba..f57c1e9d3 100644
--- a/testing/lisp/test-org-clock.el
+++ b/testing/lisp/test-org-clock.el
@@ -1497,5 +1497,78 @@ test-org-clock/special-range
cases))))
(should-not failed)))
+;;; Inline tasks clocktable
+
+(require 'org-inlinetask)
+
+(ert-deftest test-org-clock/clocktable/inlinetask/insert ()
+ "Test insert clocktable on an inline task."
+ :expected-result :failed
+ ;; Instead of the expected result we get this monstrosity:
+ ;; | *Total time* | *2:00* |
+ ;; |--------------+--------|
+ ;; | H1 | 2:00 |
+ ;; | I | 2:00 |
+ (should
+ (equal
+ "| Headline | Time |
+|--------------+--------|
+| *Total time* | *2:00* |
+|--------------+--------|
+| H1 | 2:00 |"
+ (let ((org-inlinetask-min-level 5)
+ (org-adapt-indentation t))
+ (org-test-with-temp-text "* H1
+***** I
+<point>
+***** END
+foo"
+ (insert (org-test-clock-create-clock ". 1:00" ". 2:00")
+ "CLOCK: => 1:00\n")
+ (require 'org-clock)
+ (goto-char (point-min))
+ (org-dynamic-block-insert-dblock "clocktable")
+ (unwind-protect
+ (save-excursion
+ (when (search-forward "#+CAPTION:") (forward-line))
+ (buffer-substring-no-properties
+ (point) (progn (search-forward "#+END:") (line-end-position 0))))
+ (delete-region (point) (search-forward "#+END:\n"))))))))
+
+(ert-deftest test-org-clock/clocktable/inlinetask/open-clock ()
+ "Test open clocks on an inline task.
+Open clocks should be ignored unless it is clocked in and
+`org-clock-report-include-clocking-task' is t."
+ (let ((time-reported "| Headline | Time |
+|--------------+--------|
+| *Total time* | *1:00* |
+|--------------+--------|
+| H1 | 1:00 |")
+ (time-not-reported "| Headline | Time |
+|--------------+--------|
+| *Total time* | *0:00* |"))
+ (dolist (org-clock-report-include-clocking-task '(nil t))
+ (dolist (actually-clock-in '(nil t))
+ (org-test-with-temp-text
+ "* H1
+***** I
+<point>
+***** END
+foo"
+ (should
+ (equal
+ (if (and org-clock-report-include-clocking-task
+ actually-clock-in)
+ time-reported
+ time-not-reported)
+ (progn
+ (if actually-clock-in
+ (org-clock-in nil (- (float-time) (* 60 60)))
+ (insert (org-test-clock-create-clock "-1h")))
+ ;; Unless tstart and tend are fully specified it doesn't work
+ (test-org-clock-clocktable-contents ":tstart \"<-2d>\" :tend \"<tomorrow>\""))))
+ (when actually-clock-in
+ (org-clock-cancel)))))))
+
(provide 'test-org-clock)
;;; test-org-clock.el end here
--
2.50.1
>From 2f2ac6ae9188c63a40d6d761fcd79856f7b8af74 Mon Sep 17 00:00:00 2001
From: Morgan Smith <morgan.j.sm...@outlook.com>
Date: Sat, 9 Aug 2025 14:09:20 -0400
Subject: [PATCH 4/5] Testing: test for malformed clock lines
* testing/lisp/test-org-clock.el
(test-org-clock/clocktable/malformed-clock-lines): New test
---
testing/lisp/test-org-clock.el | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/testing/lisp/test-org-clock.el b/testing/lisp/test-org-clock.el
index f57c1e9d3..5f9d2e679 100644
--- a/testing/lisp/test-org-clock.el
+++ b/testing/lisp/test-org-clock.el
@@ -1316,6 +1316,30 @@ test-org-clock/clocktable/hidefiles
(test-org-clock-clocktable-contents
(format ":hidefiles t :scope (lambda () (list %S))" the-file))))))))
+(ert-deftest test-org-clock/clocktable/malformed-clock-lines ()
+ "Test clocktable with malformed clock lines."
+ (let (org-warning)
+ (cl-letf* (((symbol-function #'org-display-warning)
+ (lambda (message) (setq org-warning message))))
+ (should
+ (equal
+ "| Headline | Time |
+|--------------+--------|
+| *Total time* | *0:00* |"
+ (org-test-with-temp-text "* H1\n<point>"
+ (insert
+ "CLOCK: [2012-01-01 sun. 00rr:04]--[2012-01-01 sun. 00:05] => 0:01")
+ (require 'org-clock)
+ (org-dynamic-block-insert-dblock "clocktable")
+ (goto-char (point-min))
+ (unwind-protect
+ (save-excursion
+ (when (search-forward "#+CAPTION:") (forward-line))
+ (buffer-substring-no-properties
+ (point) (progn (search-forward "#+END:") (line-end-position 0))))
+ (delete-region (point) (search-forward "#+END:\n"))))))
+ (should (string-prefix-p "org-clock-sum: Ignoring invalid" org-warning)))))
+
;;; Mode line
(ert-deftest test-org-clock/mode-line ()
--
2.50.1
>From 877deddd95bd9857ad4c07e19f8c7f2ecba64006 Mon Sep 17 00:00:00 2001
From: Morgan Smith <morgan.j.sm...@outlook.com>
Date: Thu, 11 Apr 2024 12:23:21 -0400
Subject: [PATCH 5/5] lisp/org-clock.el (org-clock-sum): Rewrite using element
api
* lisp/org-clock.el (org-clock-sum): Rewrite using element api.
(org--clock-ranges): New function.
* testing/lisp/test-org-clock.el
(test-org-clock/clocktable/inlinetask/insert): Enable test.
---
lisp/org-clock.el | 219 ++++++++++++++++++---------------
testing/lisp/test-org-clock.el | 6 -
2 files changed, 118 insertions(+), 107 deletions(-)
diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index 8b1752384..f855cc595 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -33,15 +33,13 @@
(require 'cl-lib)
(require 'org)
+(require 'org-element)
(declare-function calendar-iso-to-absolute "cal-iso" (date))
(declare-function notifications-notify "notifications" (&rest params))
(declare-function org-element-property "org-element-ast" (property node))
-(declare-function org-element-contents-end "org-element" (node))
-(declare-function org-element-end "org-element" (node))
(declare-function org-element-type "org-element-ast" (node &optional anonymous))
(declare-function org-element-type-p "org-element-ast" (node types))
-(defvar org-element-use-cache)
(declare-function org-inlinetask-at-task-p "org-inlinetask" ())
(declare-function org-inlinetask-goto-beginning "org-inlinetask" ())
(declare-function org-inlinetask-goto-end "org-inlinetask" ())
@@ -2069,105 +2067,67 @@ org-clock-sum
HEADLINE-FILTER is a zero-arg function that, if specified, is called for
each headline in the time range with point at the headline. Headlines for
which HEADLINE-FILTER returns nil are excluded from the clock summation.
-PROPNAME lets you set a custom text property instead of :org-clock-minutes."
+PROPNAME lets you set a custom text property instead of :org-clock-minutes.
+
+Clocking entries that are open (as in don't have an end time) that are
+not the current clocking entry will be ignored."
(with-silent-modifications
- (let* ((re (concat "^\\(\\*+\\)[ \t]\\|^[ \t]*"
- org-clock-string
- "[ \t]*\\(?:\\(\\[.*?\\]\\)-+\\(\\[.*?\\]\\)\\|=>[ \t]+\\([0-9]+\\):\\([0-9]+\\)\\)"))
- (lmax 30)
- (ltimes (make-vector lmax 0))
- (level 0)
- (tstart (cond ((stringp tstart) (org-time-string-to-seconds tstart))
- ((consp tstart) (float-time tstart))
- (t tstart)))
- (tend (cond ((stringp tend) (org-time-string-to-seconds tend))
- ((consp tend) (float-time tend))
- (t tend)))
- (t1 0)
- time)
- (remove-text-properties (point-min) (point-max)
- `(,(or propname :org-clock-minutes) t
- :org-clock-force-headline-inclusion t))
- (save-excursion
- (goto-char (point-max))
- (while (re-search-backward re nil t)
- (let* ((element (save-match-data (org-element-at-point)))
- (element-type (org-element-type element)))
- (cond
- ((and (eq element-type 'clock) (match-end 2))
- ;; Two time stamps.
- (condition-case nil
- (let* ((timestamp (org-element-property :value element))
- (ts (float-time
- (org-encode-time
- (list 0
- (org-element-property :minute-start timestamp)
- (org-element-property :hour-start timestamp)
- (org-element-property :day-start timestamp)
- (org-element-property :month-start timestamp)
- (org-element-property :year-start timestamp)
- nil -1 nil))))
- (te (float-time
- (org-encode-time
- (list 0
- (org-element-property :minute-end timestamp)
- (org-element-property :hour-end timestamp)
- (org-element-property :day-end timestamp)
- (org-element-property :month-end timestamp)
- (org-element-property :year-end timestamp)
- nil -1 nil))))
- (dt (- (if tend (min te tend) te)
- (if tstart (max ts tstart) ts))))
- (when (> dt 0) (cl-incf t1 (floor dt 60))))
- (error
- (org-display-warning (format "org-clock-sum: Ignoring invalid %s" (org-current-line-string))))))
- ((match-end 4)
- ;; A naked time.
- (setq t1 (+ t1 (string-to-number (match-string 5))
- (* 60 (string-to-number (match-string 4))))))
- ((memq element-type '(headline inlinetask)) ;A headline
- ;; Add the currently clocking item time to the total.
- (when (and org-clock-report-include-clocking-task
- (eq (org-clocking-buffer) (current-buffer))
- (eq (marker-position org-clock-hd-marker) (point))
- tstart
- tend
- (>= (float-time org-clock-start-time) tstart)
- (<= (float-time org-clock-start-time) tend))
- (let ((time (floor (org-time-convert-to-integer
- (time-since org-clock-start-time))
- 60)))
- (setq t1 (+ t1 time))))
- (let* ((headline-forced
- (get-text-property (point)
- :org-clock-force-headline-inclusion))
- (headline-included
- (or (null headline-filter)
- (save-excursion
- (save-match-data (funcall headline-filter))))))
- (setq level (- (match-end 1) (match-beginning 1)))
- (when (>= level lmax)
- (setq ltimes (vconcat ltimes (make-vector lmax 0)) lmax (* 2 lmax)))
- (when (or (> t1 0) (> (aref ltimes level) 0))
- (when (or headline-included headline-forced)
- (if headline-included
- (cl-loop for l from 0 to level do
- (aset ltimes l (+ (aref ltimes l) t1))))
- (setq time (aref ltimes level))
- (goto-char (match-beginning 0))
- (put-text-property (point) (line-end-position)
- (or propname :org-clock-minutes) time)
- (when headline-filter
- (save-excursion
- (save-match-data
- (while (org-up-heading-safe)
- (put-text-property
- (point) (line-end-position)
- :org-clock-force-headline-inclusion t))))))
- (setq t1 0)
- (cl-loop for l from level to (1- lmax) do
- (aset ltimes l 0))))))))
- (setq org-clock-file-total-minutes (aref ltimes 0))))))
+ (let ((tstart (cond ((stringp tstart) (org-time-string-to-seconds tstart))
+ ((consp tstart) (float-time tstart))
+ (t tstart)))
+ (tend (cond ((stringp tend) (org-time-string-to-seconds tend))
+ ((consp tend) (float-time tend))
+ (t tend)))
+ (propname (or propname :org-clock-minutes))
+ (t1 0)
+ (total 0)
+ time)
+ (remove-text-properties (point-min) (point-max) `(,propname t))
+ (org-element-cache-map
+ (lambda (headline-or-inlinetask)
+ (when (or (null headline-filter)
+ (save-excursion
+ (funcall headline-filter)))
+ (mapc
+ (lambda (range)
+ (setq time
+ (pcase range
+ (`(,_ . open)
+ (when (and org-clock-report-include-clocking-task
+ (eq (org-clocking-buffer) (current-buffer))
+ (eq (marker-position org-clock-hd-marker)
+ (org-element-begin headline-or-inlinetask))
+ tstart
+ tend
+ (>= (float-time org-clock-start-time) tstart)
+ (<= (float-time org-clock-start-time) tend))
+ (floor (org-time-convert-to-integer
+ (time-since org-clock-start-time))
+ 60)))
+ ((pred floatp) range)
+ (`(,time1 . ,time2)
+ (let* ((ts (float-time time1))
+ (te (float-time time2))
+ (dt (- (if tend (min te tend) te)
+ (if tstart (max ts tstart) ts))))
+ (floor dt 60)))))
+ (when (and time (> time 0)) (cl-incf t1 time)))
+ (org--clock-ranges headline-or-inlinetask))
+ (when (> t1 0)
+ (setq total (+ total t1))
+ (org-element-lineage-map headline-or-inlinetask
+ (lambda (parent)
+ (put-text-property
+ (org-element-begin parent) (1- (org-element-contents-begin parent))
+ propname
+ (+ t1 (or (get-text-property
+ (org-element-begin parent)
+ propname)
+ 0))))
+ '(headline) t))
+ (setq t1 0)))
+ :narrow t)
+ (setq org-clock-file-total-minutes total))))
(defun org-clock-sum-current-item (&optional tstart)
"Return time, clocked on current item in total."
@@ -2182,6 +2142,63 @@ org-clock-sum-current-item
(org-clock-sum tstart)
org-clock-file-total-minutes)))
+(defun org--clock-ranges (headline)
+ "Return a list of clock ranges of HEADLINE.
+Does not recurse into subheadings.
+Ranges are in one of these formats:
+ (cons time . time)
+ (cons time . \\='open) The clock does not have an end time
+ float The number of minutes as a float"
+ (unless (org-element-type-p headline '(headline inlinetask))
+ (error "Argument must be a headline"))
+ (and
+ (org-element-contents-begin headline) ;; empty headline
+ (or
+ (org-element-cache-get-key headline :clock-ranges)
+ (let ((clock-ranges
+ (org-element-cache-map
+ (lambda (elem)
+ (when (org-element-type-p elem 'clock)
+ (if-let* ((timestamp (org-element-property :value elem)))
+ (progn
+ (if
+ (and
+ (org-element-property :minute-start timestamp)
+ (org-element-property :hour-start timestamp)
+ (org-element-property :day-start timestamp)
+ (org-element-property :month-start timestamp)
+ (org-element-property :year-start timestamp)
+ ;; In org-element, when the end doesn't exist, it is set to the start.
+ ;; This means we can't check that the end is fully specified.
+ ;; (org-element-property :minute-end timestamp)
+ ;; (org-element-property :hour-end timestamp)
+ ;; (org-element-property :day-end timestamp)
+ ;; (org-element-property :month-end timestamp)
+ ;; (org-element-property :year-end timestamp)
+ )
+ (cons (org-timestamp-to-time timestamp)
+ (if (eq 'running (org-element-property :status elem))
+ 'open
+ (org-timestamp-to-time timestamp t)))
+ (org-display-warning
+ (format "org-clock-sum: Ignoring invalid timestamp: %s"
+ (org-element-property :raw-value timestamp)))))
+ (when (org-element-property :duration elem)
+ (org-duration-to-minutes (org-element-property :duration elem))))))
+ ;; XXX: using these arguments would be more intuitive
+ ;; but don't seem to work due to bugs in
+ ;; `org-element-cache-map'
+ ;; :restrict-elements '(clock)
+ ;; :after-element headline
+ :granularity 'element
+ :next-re org-element-clock-line-re
+ :from-pos (org-element-contents-begin headline)
+ :to-pos (save-excursion
+ (goto-char (org-element-begin headline))
+ (org-entry-end-position)))))
+ (org-element-cache-store-key headline :clock-ranges clock-ranges)
+ clock-ranges))))
+
;;;###autoload
(defun org-clock-display (&optional arg)
"Show subtree times in the entire buffer.
diff --git a/testing/lisp/test-org-clock.el b/testing/lisp/test-org-clock.el
index 5f9d2e679..9f22721f4 100644
--- a/testing/lisp/test-org-clock.el
+++ b/testing/lisp/test-org-clock.el
@@ -1527,12 +1527,6 @@ test-org-clock/special-range
(ert-deftest test-org-clock/clocktable/inlinetask/insert ()
"Test insert clocktable on an inline task."
- :expected-result :failed
- ;; Instead of the expected result we get this monstrosity:
- ;; | *Total time* | *2:00* |
- ;; |--------------+--------|
- ;; | H1 | 2:00 |
- ;; | I | 2:00 |
(should
(equal
"| Headline | Time |
--
2.50.1
