Hello, Attached are patches that incorporate your feedback and are rebased on master. I also think I fixed a few bugs along the way.
I manually tested file+headline, file+olp, and file+olp+datetree, with only <file-spec> and no headline/olp as well as with both <file-spec> with headline/olp. I also took care to test each with :prepend nil and :prepend t, as well as file+olp+datetree without an existing datetree and with at least one existing datetree. Additionally, I updated the :type of 'org-capture-templates' accordingly. I've never dealt with a :type as complex as the one for 'org-capture-templates', so I'm not sure if my solution is the best one. But it seems not to throw any errors or warnings, and the widgets in the Customize menu seem right. On Sun, May 18 2025, Ihor Radchenko wrote: >> - For non-unique headings, the full outline path is safer >> + For non-unique headings, the full outline path is >> + safer. If no headings are given, or if >> + function-returning-list-of-strings or >> + symbol-containing-list-of-strings return nil, the entry >> + will be inserted at the end of <file-spec> on top >> + level. > > You missed optional :prepend property in capture templates. > See `org-capture-place-entry'. The location will depend on :prepend > being t/nil unless you explicitly set :exact-position when processing > the capture template. I see. So you're saying that the current implementation is fine but the description of resulting behavior is inaccurate, namely, that the entry can be inserted at the beginning of the target location if :prepend is non-nil? In the attached patches, I've updated the docstrings and comments to reflect this understanding. Let me know if I misunderstood the change(s) you wanted from me. >> - (m (org-find-olp (cons expanded-file-path >> - (apply #'org-capture-expand-olp >> expanded-file-path outline-path))))) >> + (expanded-olp (apply #'org-capture-expand-olp >> expanded-file-path outline-path)) >> + ;; If expanded-olp is nil, then create the datetree at >> + ;; the end of the file specified by expanded-file-path >> + (m (if expanded-olp >> + (org-find-olp (cons expanded-file-path expanded-olp)) >> + (set-buffer (org-capture-target-buffer >> expanded-file-path)) >> + (save-restriction (widen) (point-max-marker))))) > > I am not sure if I understand the purpose of the last line here. I've added a comment to explain the reasoning. Does the comment suffice in its explanation? Or have I misunderstood something. >> + (let* ((expanded-file-path (org-capture-expand-file path)) >> + (expanded-olp (apply #'org-capture-expand-olp >> expanded-file-path outline-path)) >> + ;; If expanded-olp is nil, then file-level-datetree-p >> + ;; is non-nil and the datetree should be created at >> + ;; the end of the file specified by expanded-file-path > > There is no guarantee that it will be created at the end. Ditto my response to your feedback mentioning :prepend. >> + (pcase headline >> + ((pred not) (goto-char (point-max))) > > Nitpick: `null' is a bit more comprehensive as predicate name. Done. -- Kind regards, Kristoffer
From c4a0fe797ca01030a607e6d2bd7397596a1b3d3e Mon Sep 17 00:00:00 2001 From: Kristoffer Balintona <[email protected]> Date: Fri, 9 May 2025 11:40:13 -0500 Subject: [PATCH 1/2] lisp/org-capture.el: Accept optional and nil olp for file+olp+datetree and file+olp * lisp/org-capture.el (org-capture-expand-olp): When the supplied olp is nil, org-capture-expand-olp now returns nil. (org-capture-set-target-location): The file+olp+datetree and file+olp target specifications now accept a nil or omitted olp. Update the docstring to reflect the new behaviors. * testing/lisp/test-org-capture.el (org-capture-expand-olp): Add test case for org-capture-expand-olp now accepting a nil olp. --- lisp/org-capture.el | 137 +++++++++++++++++++++---------- testing/lisp/test-org-capture.el | 7 ++ 2 files changed, 99 insertions(+), 45 deletions(-) diff --git a/lisp/org-capture.el b/lisp/org-capture.el index 3c8040854..24c2d51f0 100644 --- a/lisp/org-capture.el +++ b/lisp/org-capture.el @@ -207,21 +207,33 @@ target Specification of where the captured item should be placed. (file+headline <file-spec> symbol-containing-string) Fast configuration if the target heading is unique in the file + (file+olp <file-spec>) (file+olp <file-spec> \"Level 1 heading\" \"Level 2\" ...) (file+olp <file-spec> function-returning-list-of-strings) (file+olp <file-spec> symbol-containing-list-of-strings) - For non-unique headings, the full outline path is safer + For non-unique headings, the full outline path is + safer. The entry is created at the outline path (a + list of strings denoting headlines). If no outline + path is specified or if the outline path specification + is nil, the entry will be inserted at the top level of + <file-spec>. (file+regexp <file-spec> \"regexp to find location\") File to the entry containing matching regexp + (file+olp+datetree <file-spec>) (file+olp+datetree <file-spec> \"Level 1 heading\" ...) (file+olp+datetree <file-spec> function-returning-list-of-strings) (file+olp+datetree <file-spec> symbol-containing-list-of-strings) - Will create a heading in a date tree for today's date. - If no heading is given, the tree will be on top level. - To prompt for date instead of using TODAY, use the - :time-prompt property. To create a week-tree, use the + Will create an entry in a datetree under the specified + outline path. for today\\='s date. If no outline path + is given or if the outline path specification is nil, + the entry will be inserted into the first already + existing top level datetree in <file-spec> or, if no + top level datetree exists, a newly created datetree at + the end of <file-spec>. To prompt for a date instead + of using today\\='s date, use the + :time-prompt property. To create a week-tree,use the :tree-type property. (file+function <file-spec> function-finding-location) @@ -451,11 +463,10 @@ you can escape ambiguous cases with a backward slash, e.g., \\%i." (file :tag "Literal") (function :tag "Function") (variable :tag "Variable"))) - (olp-variants '(choice :tag "Outline path" - (repeat :tag "Outline path" :inline t - (string :tag "Headline")) - (function :tag "Function") - (variable :tag "Variable")))) + (olp-variants-choices '((repeat :tag "Outline path" :inline t + (string :tag "Headline")) + (function :tag "Function") + (variable :tag "Variable")))) `(repeat (choice :value ("" "" entry (file "~/org/notes.org") "") (list :tag "Multikey description" @@ -484,20 +495,22 @@ you can escape ambiguous cases with a backward slash, e.g., \\%i." (string :tag "Headline") (function :tag "Function") (variable :tag "Variable"))) - (list :tag "File & Outline path" - (const :format "" file+olp) - ,file-variants - ,olp-variants) + (list :tag "File [ & Outline path ]" + (const :format "" file+olp) + ,file-variants + (choice :tag "Outline path" + (const :tag "Top level" nil) + ,@olp-variants-choices)) (list :tag "File & Regexp" (const :format "" file+regexp) ,file-variants (regexp :tag " Regexp")) - (list :tag "File [ & Outline path ] & Date tree" - (const :format "" file+olp+datetree) - ,file-variants - ,(append - olp-variants - '((const :tag "Date tree at top level" nil)))) + (list :tag "File [ & Outline path ] & Date tree" + (const :format "" file+olp+datetree) + ,file-variants + (choice :tag "Outline path" + (const :tag "Date tree at top level" nil) + ,@olp-variants-choices)) (list :tag "File & function" (const :format "" file+function) ,file-variants @@ -1094,10 +1107,29 @@ Store them in the capture property list." (unless (bolp) (insert "\n")) (insert "* " headline "\n") (forward-line -1))) - (`(file+olp ,path . ,(and outline-path (guard outline-path))) + ((or `(file+olp ,path) + `(file+olp ,path . ,outline-path)) (let* ((expanded-file-path (org-capture-expand-file path)) - (m (org-find-olp (cons expanded-file-path - (apply #'org-capture-expand-olp expanded-file-path outline-path))))) + (expanded-olp (apply #'org-capture-expand-olp expanded-file-path outline-path)) + ;; Vary behavior depending on whether expanded-olp is + ;; nil or non-nil. If expanded-olp is non-nil, then + ;; create the entry at that outline path. If + ;; expanded-olp is nil (no olp is provided), then + ;; create the entry in the expanded-file-path file + (m (if expanded-olp + (org-find-olp (cons expanded-file-path expanded-olp)) + (set-buffer (org-capture-target-buffer expanded-file-path)) + ;; If expanded-olp is nil or omitted, then the + ;; user is essentially using the form (file + ;; <file-spec>), so behave as such by setting + ;; `target-entry-p' to nil + (setq target-entry-p nil) + ;; Return a marker pointing to the end of the + ;; file to cause the new entry to be created + ;; there. We widen first to consider the + ;; possibility of the buffer already being open + ;; and narrowed by the user + (save-restriction (widen) (point-max-marker))))) (set-buffer (marker-buffer m)) (org-capture-put-target-region-and-position) (widen) @@ -1116,13 +1148,24 @@ Store them in the capture property list." (org-capture-put :exact-position (point)) (setq target-entry-p (and (derived-mode-p 'org-mode) (org-at-heading-p))))) - (`(file+olp+datetree ,path . ,outline-path) - (let ((m (if outline-path - (let ((expanded-file-path (org-capture-expand-file path))) - (org-find-olp (cons expanded-file-path - (apply #'org-capture-expand-olp expanded-file-path outline-path)))) - (set-buffer (org-capture-target-buffer path)) - (point-marker)))) + ((or `(file+olp+datetree ,path) + `(file+olp+datetree ,path . ,outline-path)) + (let* ((expanded-file-path (org-capture-expand-file path)) + (expanded-olp (apply #'org-capture-expand-olp expanded-file-path outline-path)) + ;; Vary behavior depending on whether expanded-olp is + ;; nil or non-nil. If expanded-olp is non-nil, then + ;; create the datetree at that outline path. If + ;; expanded-olp is nil (no olp is provided), then + ;; create the datetree in the expanded-file-path file + (m (if expanded-olp + (org-find-olp (cons expanded-file-path expanded-olp)) + (set-buffer (org-capture-target-buffer expanded-file-path)) + ;; Return a marker pointing to the end of the + ;; buffer to cause the new entry to be created + ;; there. We widen first to consider the + ;; possibility of the buffer already being open + ;; and narrowed by the user + (save-restriction (widen) (point-max-marker))))) (set-buffer (marker-buffer m)) (org-capture-put-target-region-and-position) (widen) @@ -1183,7 +1226,7 @@ Store them in the capture property list." (org-today)))) ;; the following is the keep-restriction argument for ;; org-datetree-find-date-create - (when outline-path 'subtree-at-point)))) + (when expanded-olp 'subtree-at-point)))) (`(file+function ,path ,(and function (pred functionp))) (set-buffer (org-capture-target-buffer path)) (org-capture-put-target-region-and-position) @@ -1232,20 +1275,24 @@ an error." (defun org-capture-expand-olp (file &rest olp) "Expand functions, symbols and outline paths in FILE for OLP. -When OLP is a function, call it with no arguments while the current -buffer is the FILE-visiting buffer. When it is a variable, return its -value. When it is a list of string, return it. In any other case, -signal an error." - (let* ((first (car olp)) - (final-olp (cond ((not (memq nil (mapcar #'stringp olp))) olp) - ((and (not (cdr olp)) (functionp first)) - (with-current-buffer (find-file-noselect file) - (funcall first))) - ((and (not (cdr olp)) (symbolp first) (boundp first)) - (symbol-value first)) - (t nil)))) - (or final-olp - (error "org-capture: Invalid outline path target: %S" olp)))) +Return a list of strings representing an outline path (OLP) in FILE. + +The behavior of this function is as follows: +- When OLP is a function, call it with no arguments while the current + buffer is the FILE-visiting buffer. +- When it is a variable, return its value. +- When it is a list of strings, return that list. +- When OLP is nil, return nil. +In any other case, signal an error." + (let* ((first (car olp))) + (cond ((and (= 1 (length olp)) (null first)) nil) + ((not (memq nil (mapcar #'stringp olp))) olp) + ((and (not (cdr olp)) (functionp first)) + (with-current-buffer (find-file-noselect file) + (funcall first))) + ((and (not (cdr olp)) (symbolp first) (boundp first)) + (symbol-value first)) + (t (error "org-capture: Invalid outline path target: %S" olp))))) (defun org-capture-expand-file (file) "Expand functions, symbols and file names for FILE. diff --git a/testing/lisp/test-org-capture.el b/testing/lisp/test-org-capture.el index 6b49a2df7..cc1c07480 100644 --- a/testing/lisp/test-org-capture.el +++ b/testing/lisp/test-org-capture.el @@ -1091,6 +1091,13 @@ before\nglobal-before\nafter\nglobal-after" '("A" "B" "C") (org-test-with-temp-text-in-file "" (org-capture-expand-olp buffer-file-name "A" "B" "C")))) + ;; `org-capture-expand-olp' should return nil if the outline path is + ;; nil + (should + (equal + nil + (org-test-with-temp-text-in-file "" + (org-capture-expand-olp buffer-file-name nil)))) ;; The current buffer during the funcall of the lambda is the temporary ;; test file. (should -- 2.52.0
From 2f7268f0795c5b437ffc4f6651c54a480ce09b59 Mon Sep 17 00:00:00 2001 From: Kristoffer Balintona <[email protected]> Date: Fri, 9 May 2025 11:40:13 -0500 Subject: [PATCH 2/2] lisp/org-capture.el: Accept optional or nil headline for the file+headline * lisp/org-capture.el (org-capture-expand-headline): When the supplied headline is nil, org-capture-expand-olp now returns nil. (org-capture-set-target-location): The file+headline target specification now accepts a nil or omitted headline. In either case, insert the capture entry at the top level of the file. Update docstring to reflect the new behaviors. * testing/lisp/test-org-capture.el (test-org-capture/org-capture-expand-headline): Add new test for org-capture-expand-headline that tests these new behaviors as well as several previously existing ones. --- lisp/org-capture.el | 58 ++++++++++++++++++++------------ testing/lisp/test-org-capture.el | 42 +++++++++++++++++++++++ 2 files changed, 79 insertions(+), 21 deletions(-) diff --git a/lisp/org-capture.el b/lisp/org-capture.el index 24c2d51f0..2a027c175 100644 --- a/lisp/org-capture.el +++ b/lisp/org-capture.el @@ -202,10 +202,16 @@ target Specification of where the captured item should be placed. (id \"id of existing Org entry\") File as child of this entry, or in the body of the entry + (file+headline <file-spec>) (file+headline <file-spec> \"node headline\") (file+headline <file-spec> function-returning-string) (file+headline <file-spec> symbol-containing-string) - Fast configuration if the target heading is unique in the file + Fast configuration if the target heading is unique in + the file. The entry is created at the headline + specified by a string, symbol, or function. If no + headline is provided or if the headline specification + is nil, the entry will be inserted at the top level of + <file-spec>. (file+olp <file-spec>) (file+olp <file-spec> \"Level 1 heading\" \"Level 2\" ...) @@ -492,6 +498,7 @@ you can escape ambiguous cases with a backward slash, e.g., \\%i." (const :format "" file+headline) ,file-variants (choice :tag "Headline" + (const :tag "Top level" nil) (string :tag "Headline") (function :tag "Function") (variable :tag "Variable"))) @@ -1084,7 +1091,8 @@ Store them in the capture property list." (org-capture-put-target-region-and-position) (goto-char position)) (_ (error "Cannot find target ID \"%s\"" id)))) - (`(file+headline ,path ,headline) + ((or `(file+headline ,path) + `(file+headline ,path ,headline)) (set-buffer (org-capture-target-buffer path)) ;; Org expects the target file to be in Org mode, otherwise ;; it throws an error. However, the default notes files @@ -1099,15 +1107,19 @@ Store them in the capture property list." (widen) (goto-char (point-min)) (setq headline (org-capture-expand-headline headline)) - (if (re-search-forward (format org-complex-heading-regexp-format - (regexp-quote headline)) - nil t) - (forward-line 0) - (goto-char (point-max)) - (unless (bolp) (insert "\n")) - (insert "* " headline "\n") - (forward-line -1))) - ((or `(file+olp ,path) + (pcase headline + ((pred null) (goto-char (point-max))) + ((pred stringp) + (re-search-forward (format org-complex-heading-regexp-format + (regexp-quote headline)) + nil t) + (forward-line 0)) + (_ + (goto-char (point-max)) + (unless (bolp) (insert "\n")) + (insert "* " headline "\n") + (forward-line -1)))) + ((or `(file+olp ,path) `(file+olp ,path . ,outline-path)) (let* ((expanded-file-path (org-capture-expand-file path)) (expanded-olp (apply #'org-capture-expand-olp expanded-file-path outline-path)) @@ -1262,16 +1274,20 @@ Store them in the capture property list." (defun org-capture-expand-headline (headline) "Expand functions, symbols and headline names for HEADLINE. -When HEADLINE is a function, call it. When it is a variable, return -its value. When it is a string, return it. In any other case, signal -an error." - (let* ((final-headline (cond ((stringp headline) headline) - ((functionp headline) (funcall headline)) - ((and (symbolp headline) (boundp headline)) - (symbol-value headline)) - (t nil)))) - (or final-headline - (error "org-capture: Invalid headline target: %S" headline)))) +Return a string representing a headline. + +The behavior of this function is as follows: +- When HEADLINE is a function, call it. +- When it is a variable, return its value. +- When it is a string, return that string. +- When headline is nil, return nil. +In any other case, signal an error." + (cond ((null headline) nil) + ((stringp headline) headline) + ((functionp headline) (funcall headline)) + ((and (symbolp headline) (boundp headline)) + (symbol-value headline)) + (t (error "org-capture: Invalid headline target: %S" headline)))) (defun org-capture-expand-olp (file &rest olp) "Expand functions, symbols and outline paths in FILE for OLP. diff --git a/testing/lisp/test-org-capture.el b/testing/lisp/test-org-capture.el index cc1c07480..05421c4ae 100644 --- a/testing/lisp/test-org-capture.el +++ b/testing/lisp/test-org-capture.el @@ -1083,6 +1083,48 @@ before\nglobal-before\nafter\nglobal-after" (org-capture nil "t") (buffer-string)))))) +(ert-deftest test-org-capture/org-capture-expand-headline () + "Test `org-capture-expand-headline'." + ;; `org-capture-expand-headline' should return nil when headline is + ;; nil + (should + (equal + nil + (let ((file (make-temp-file "org-test"))) + (unwind-protect + (org-capture-expand-headline nil) + (delete-file file))))) + ;; `org-capture-expand-headline' should return headline if it is a + ;; string + (should + (equal + "A" + (let ((file (make-temp-file "org-test"))) + (unwind-protect + (org-capture-expand-headline "A") + (delete-file file))))) + ;; `org-capture-expand-headline' should evaluate headline if it is a + ;; function and return its value + (should + (equal + "A" + (let ((file (make-temp-file "org-test"))) + (unwind-protect + (org-capture-expand-headline (lambda () "A")) + (delete-file file))))) + ;; `org-capture-expand-headline' should return the value of headline + ;; if it is a symbol + (should + (equal + "A" + (let ((file (make-temp-file "org-test"))) + (unwind-protect + (progn + (setq temp "A") + (org-capture-expand-headline 'temp)) + (makunbound 'temp) + (delete-file file)))))) + (ert-deftest test-org-capture/org-capture-expand-olp () "Test org-capture-expand-olp." ;; `org-capture-expand-olp' accepts inlined outline path. -- 2.52.0
