"Bad template" when creating org-capture for table-line without '|'

"Bad template" when creating org-capture for table-line without '|'

[Rens Oliemans]

Hi,

I was trying to create an org-capture for a table-line type, and I noticed that
org-capture inserts the string "| Bad template |" when the template does not start with
'|'. This could make sense, since a line is (only) considered part of a table when it
starts with '|'.

However, I did not know that the _template_ also has to start with '|', and org-capture
still inserts the string "| Bad template |", which does not suggest in any way that the
lacking '|' would be the problem (after all, it is smart enough to prepend a '|').

Use this template to reproduce the behaviour, present on 9.7-pre
(release_9.6.23-1318-g990b89):

(setq org-capture-templates '(("t" "Test" table-line (file "test.org")
			       "%^t")))

I did not see this documented anywhere: this was quite confusing, and the only way I got a
hint at what I did wrong was to see the default template for table-line: "| %? |". It
seems that this behaviour is intended, looking at org-capture.el:1358 @ 990b89d3:

...
	 (pcase (org-trim (org-capture-get :template))
	   ((pred (string-match-p org-table-border-regexp))
	    "| %?Bad template |")
	   (text (concat text "\n"))))
...

however, is this also what's ideal? I would suggest one of the following alternatives:

- An error is signalled to the user with the root cause of the error: no '|' at start of
  template. In addition, the org-capture-templates variable documentation string and the
  manual would be updated. I am happy to create a patch for this.

- org-capture could pre- and suffix a '|' if the user did not supply it in their template.
  I am also happy to create a patch for this, however that would be my first so such a
  patch would perhaps have a few iterations ;)

What are your thoughts on this?

Best

Re: "Bad template" when creating org-capture for table-line without '|'

[Ihor Radchenko]

Rens Oliemans <hallo@rensoliemans.nl> writes:

> ...
> However, I did not know that the _template_ also has to start with '|', and org-capture
> still inserts the string "| Bad template |", which does not suggest in any way that the
> lacking '|' would be the problem (after all, it is smart enough to prepend a '|').
> ...
> however, is this also what's ideal? I would suggest one of the following alternatives:
>
> - An error is signalled to the user with the root cause of the error: no '|' at start of
>   template. In addition, the org-capture-templates variable documentation string and the
>   manual would be updated. I am happy to create a patch for this.
>
> - org-capture could pre- and suffix a '|' if the user did not supply it in their template.
>
> What are your thoughts on this?

Org mode is inconsistent here.

`org-capture-place-entry' uses `org-capture-verify-tree' and throws an
error when the template does not start from a headline or contains
headings with lower level after higher level.

Yet, `org-capture-place-item' prepends "-" when the template does not
start from a list item.

And, as you noticed, `org-capture-place-table-line' replaces the
template with "Bad template".

I think that the best course of action would be automatically fixing the
templates, as you propose in option (2), changing
`org-capture-place-entry' to automatically prepend "* " if necessary
(still leaving the subtree check though).

>   I am also happy to create a patch for this, however that would be my first so such a
>   patch would perhaps have a few iterations ;)

That would be welcome. We can take as many iterations as necessary.
You may refer to
https://orgmode.org/worg/org-contribute.html#first-patch for instructions.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

[PATCH 1/2] org-capture: Allow entry template to start without heading

[Rens Oliemans]

* lisp/org-capture.el (org-capture-place-entry): Prepend heading to
template if the template does not yet start with a heading.

* testing/lisp/test-org-capture.el (test-org-capture/entry): Add two
tests: no error is raised when org-capture is called with a template
that does not start with a heading; and org-capture should error with
a template with a lower heading after a higher heading.

Link: https://list.orgmode.org/877chnc0lr.fsf@localhost/
---
First iteration of these patches, please let me know if anything can be improved, either
about the code or patches themselves (I am not used to sending patches via email).

 lisp/org-capture.el              |  1 +
 testing/lisp/test-org-capture.el | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index da14f45c0..750778f8b 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1198,6 +1198,7 @@ may have been stored before."
 	(exact-position (org-capture-get :exact-position))
 	(insert-here? (org-capture-get :insert-here))
 	(level 1))
+    (unless (string-match "^*" template) (setq template (concat "* " template)))
     (org-capture-verify-tree template)
     (when exact-position (goto-char exact-position))
     (cond
diff --git a/testing/lisp/test-org-capture.el b/testing/lisp/test-org-capture.el
index 0ed44c6af..9ab078193 100644
--- a/testing/lisp/test-org-capture.el
+++ b/testing/lisp/test-org-capture.el
@@ -244,6 +244,23 @@
 		:immediate-finish t))))
        (org-capture nil "t")
        (buffer-string))))
+  ;; Do not raise an error on templates that do not start with a heading.
+  (should
+   (org-test-with-temp-text-in-file ""
+     (let* ((file (buffer-file-name))
+            (org-capture-templates
+             `(("t" "Test" entry (file ,file) "Foo"
+                :immediate-finish t))))
+       (org-capture nil "t"))))
+  ;; Raise an error on templates with a lower level heading after a
+  ;; higher level one.
+  (should-error
+   (org-test-with-temp-text-in-file ""
+     (let* ((file (buffer-file-name))
+            (org-capture-templates
+             `(("t" "Test" entry (file ,file) "** X\n* Y"
+	        :immediate-finish t))))
+       (org-capture nil "t"))))
   ;; With a 0 prefix argument, ignore surrounding lists.
   (should
    (equal "Foo\n* X\nBar\n"
-- 
2.44.0

Re: [PATCH 1/2] org-capture: Allow entry template to start without heading

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

Rens Oliemans <hallo@rensoliemans.nl> writes:

> * lisp/org-capture.el (org-capture-place-entry): Prepend heading to
> template if the template does not yet start with a heading.
>
> * testing/lisp/test-org-capture.el (test-org-capture/entry): Add two
> tests: no error is raised when org-capture is called with a template
> that does not start with a heading; and org-capture should error with
> a template with a lower heading after a higher heading.
>
> Link: https://list.orgmode.org/877chnc0lr.fsf@localhost/
> ---
> First iteration of these patches, please let me know if anything can be improved, either
> about the code or patches themselves (I am not used to sending patches via email).

Thanks!
I have improved your patches a little, fixing the regular expression
used to match headings ("^*" is not accurate, you need
org-outline-regexp-bol), and adding another test case.
See the attached.

Before I install the patches, may I know if you have FSF copyright
assignment? See https://orgmode.org/worg/org-contribute.html#copyright


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2-0001-org-capture-Allow-entry-template-to-start-without.patch --]
[-- Type: text/x-patch, Size: 2830 bytes --]

From 36639ac711f099b49900d886ad28d29abc1b29ed Mon Sep 17 00:00:00 2001
Message-ID: <36639ac711f099b49900d886ad28d29abc1b29ed.1713102029.git.yantar92@posteo.net>
From: Rens Oliemans <hallo@rensoliemans.nl>
Date: Sun, 14 Apr 2024 13:24:49 +0200
Subject: [PATCH v2 1/2] org-capture: Allow entry template to start without
 heading

* lisp/org-capture.el (org-capture-place-entry): Prepend heading to
template if the template does not yet start with a heading.

* testing/lisp/test-org-capture.el (test-org-capture/entry): Add two
tests: no error is raised when org-capture is called with a template
that does not start with a heading; and org-capture should error with
a template with a lower heading after a higher heading.

Link: https://list.orgmode.org/877chnc0lr.fsf@localhost/
---
 lisp/org-capture.el              |  2 ++
 testing/lisp/test-org-capture.el | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index da14f45c0..a95a38162 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1198,6 +1198,8 @@ (defun org-capture-place-entry ()
 	(exact-position (org-capture-get :exact-position))
 	(insert-here? (org-capture-get :insert-here))
 	(level 1))
+    (unless (string-match org-outline-regexp-bol template)
+      (setq template (concat "* " template)))
     (org-capture-verify-tree template)
     (when exact-position (goto-char exact-position))
     (cond
diff --git a/testing/lisp/test-org-capture.el b/testing/lisp/test-org-capture.el
index 0ed44c6af..4e9139c40 100644
--- a/testing/lisp/test-org-capture.el
+++ b/testing/lisp/test-org-capture.el
@@ -244,6 +244,30 @@ (ert-deftest test-org-capture/entry ()
 		:immediate-finish t))))
        (org-capture nil "t")
        (buffer-string))))
+  ;; Do not raise an error on templates that do not start with a heading.
+  (should
+   (org-test-with-temp-text-in-file ""
+     (let* ((file (buffer-file-name))
+            (org-capture-templates
+             `(("t" "Test" entry (file ,file) "Foo"
+                :immediate-finish t))))
+       (org-capture nil "t"))))
+  (should
+   (org-test-with-temp-text-in-file ""
+     (let* ((file (buffer-file-name))
+            (org-capture-templates
+             `(("t" "Test" entry (file ,file) "*bold*"
+                :immediate-finish t))))
+       (org-capture nil "t"))))
+  ;; Raise an error on templates with a lower level heading after a
+  ;; higher level one.
+  (should-error
+   (org-test-with-temp-text-in-file ""
+     (let* ((file (buffer-file-name))
+            (org-capture-templates
+             `(("t" "Test" entry (file ,file) "** X\n* Y"
+	        :immediate-finish t))))
+       (org-capture nil "t"))))
   ;; With a 0 prefix argument, ignore surrounding lists.
   (should
    (equal "Foo\n* X\nBar\n"
-- 
2.44.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: v2-0002-org-capture-Allow-table-line-entry-to-start-witho.patch --]
[-- Type: text/x-patch, Size: 2775 bytes --]

From af0b56f3338b8700bd6096e4963c95436b1a14b0 Mon Sep 17 00:00:00 2001
Message-ID: <af0b56f3338b8700bd6096e4963c95436b1a14b0.1713102029.git.yantar92@posteo.net>
In-Reply-To: <36639ac711f099b49900d886ad28d29abc1b29ed.1713102029.git.yantar92@posteo.net>
References: <36639ac711f099b49900d886ad28d29abc1b29ed.1713102029.git.yantar92@posteo.net>
From: Rens Oliemans <hallo@rensoliemans.nl>
Date: Sun, 14 Apr 2024 13:24:51 +0200
Subject: [PATCH v2 2/2] org-capture: Allow table-line entry to start without |

* lisp/org-capture.el (org-capture-place-table-line): Prepend
table-line begin ('|') if the template does not start with it.

* testing/lisp/test-org-capture.el (test-org-capture/table-line):
Verify that a template gets prepended with a '|' if it does not start
with it.
---
 lisp/org-capture.el              | 15 ++++++++-------
 testing/lisp/test-org-capture.el | 10 ++++++++++
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index a95a38162..205d09da8 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1356,13 +1356,14 @@ (defun org-capture-place-item ()
 (defun org-capture-place-table-line ()
   "Place the template as a table line."
   (require 'org-table)
-  (let ((text
-	 (pcase (org-trim (org-capture-get :template))
-	   ((pred (string-match-p org-table-border-regexp))
-	    "| %?Bad template |")
-	   (text (concat text "\n"))))
-	(table-line-pos (org-capture-get :table-line-pos))
-	beg end)
+  (let* ((template (org-trim (org-capture-get :template)))
+         (text
+	  (pcase template
+	    ((pred (string-match-p org-table-border-regexp))
+	     (concat "| " template))
+	    (text (concat text "\n"))))
+	 (table-line-pos (org-capture-get :table-line-pos))
+	 beg end)
     (cond
      ((org-capture-get :exact-position)
       (org-with-point-at (org-capture-get :exact-position)
diff --git a/testing/lisp/test-org-capture.el b/testing/lisp/test-org-capture.el
index 4e9139c40..f97d08bce 100644
--- a/testing/lisp/test-org-capture.el
+++ b/testing/lisp/test-org-capture.el
@@ -613,6 +613,16 @@ (ert-deftest test-org-capture/table-line ()
 		       "| 2 |" :immediate-finish t))))
 	      (org-capture nil "t"))
 	    (buffer-string))))
+  ;; Prepend | when the template does not start with it
+  (should
+   (equal "| 1 |\n| 2 |\n"
+          (org-test-with-temp-text-in-file "| 1 |\n"
+            (let* ((file (buffer-file-name))
+                   (org-capture-templates
+                    `(("t" "Table" table-line (file ,file)
+                       "2 |" :immediate-finish t))))
+              (org-capture nil "t")
+              (buffer-string)))))
   ;; When `:prepend' is nil, add the row at the end of the table.
   (should
    (equal "| a |\n| x |\n"
-- 
2.44.0


[-- Attachment #4: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH 1/2] org-capture: Allow entry template to start without heading

[Rens Oliemans]

Ihor Radchenko <yantar92@posteo.net> writes:

> I have improved your patches a little, fixing the regular expression
> used to match headings ("^*" is not accurate, you need
> org-outline-regexp-bol), and adding another test case.
> See the attached.

Ah I see the difference, good catch. Thank you for the improvements!

> Before I install the patches, may I know if you have FSF copyright
> assignment? See https://orgmode.org/worg/org-contribute.html#copyright

I had not done that yet. I have just sent the form to the FSF. I will let
you know when the process is complete.

Best,
Rens

Re: [PATCH 1/2] org-capture: Allow entry template to start without heading

[Ihor Radchenko]

Rens Oliemans <hallo@rensoliemans.nl> writes:

>> Before I install the patches, may I know if you have FSF copyright
>> assignment? See https://orgmode.org/worg/org-contribute.html#copyright
>
> I had not done that yet. I have just sent the form to the FSF. I will let
> you know when the process is complete.

It has been one month since the last activity in this thread.
May I know if you got any reply from FSF?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH 1/2] org-capture: Allow entry template to start without heading

[Rens Oliemans]

Ihor Radchenko <yantar92@posteo.net> writes:

> It has been one month since the last activity in this thread.
> May I know if you got any reply from FSF?

I did, I got and signed the assignment, on Friday the 10th of May. I will get a reply of
the FSF once they have signed it, and will post here when that happens.

Best,
Rens

Re: [PATCH 1/2] org-capture: Allow entry template to start without heading

[Rens Oliemans]

Ihor Radchenko <yantar92@posteo.net> writes:

> It has been one month since the last activity in this thread.
> May I know if you got any reply from FSF?

My copyright assignment with the FSF is now complete!

Re: [PATCH 1/2] org-capture: Allow entry template to start without heading

[Ihor Radchenko]

Rens Oliemans <hallo@rensoliemans.nl> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> It has been one month since the last activity in this thread.
>> May I know if you got any reply from FSF?
>
> My copyright assignment with the FSF is now complete!

Bastien, may you please check FSF records?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH 1/2] org-capture: Allow entry template to start without heading

[Bastien Guerry]

Ihor Radchenko <yantar92@posteo.net> writes:

> Bastien, may you please check FSF records?

The record for Rens is valid.

Thanks for your contributions!

-- 
 Bastien Guerry

Re: [PATCH 1/2] org-capture: Allow entry template to start without heading

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> I have improved your patches a little, fixing the regular expression
> used to match headings ("^*" is not accurate, you need
> org-outline-regexp-bol), and adding another test case.
> See the attached.
>
> Before I install the patches, may I know if you have FSF copyright
> assignment? See https://orgmode.org/worg/org-contribute.html#copyright

Applied, onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=0c8fba93f
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a62f75c84

I added you to the contributor list:
https://git.sr.ht/~bzg/worg/commit/05c4acae

Thanks for your contribution!

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH 1/2] org-capture: Allow entry template to start without heading

[Rens Oliemans]

Ihor Radchenko <yantar92@posteo.net> writes:

> Applied, onto main.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=0c8fba93f
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a62f75c84
>
> I added you to the contributor list:
> https://git.sr.ht/~bzg/worg/commit/05c4acae

Great, thanks!

[PATCH 2/2] org-capture: Allow table-line entry to start without |

[Rens Oliemans]

* lisp/org-capture.el (org-capture-place-table-line): Prepend
table-line begin ('|') if the template does not start with it.

* testing/lisp/test-org-capture.el (test-org-capture/table-line):
Verify that a template gets prepended with a '|' if it does not start
with it.
---
 lisp/org-capture.el              | 15 ++++++++-------
 testing/lisp/test-org-capture.el | 10 ++++++++++
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 750778f8b..a56e75fda 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1355,13 +1355,14 @@ may have been stored before."
 (defun org-capture-place-table-line ()
   "Place the template as a table line."
   (require 'org-table)
-  (let ((text
-	 (pcase (org-trim (org-capture-get :template))
-	   ((pred (string-match-p org-table-border-regexp))
-	    "| %?Bad template |")
-	   (text (concat text "\n"))))
-	(table-line-pos (org-capture-get :table-line-pos))
-	beg end)
+  (let* ((template (org-trim (org-capture-get :template)))
+         (text
+	  (pcase template
+	    ((pred (string-match-p org-table-border-regexp))
+	     (concat "| " template))
+	    (text (concat text "\n"))))
+	 (table-line-pos (org-capture-get :table-line-pos))
+	 beg end)
     (cond
      ((org-capture-get :exact-position)
       (org-with-point-at (org-capture-get :exact-position)
diff --git a/testing/lisp/test-org-capture.el b/testing/lisp/test-org-capture.el
index 9ab078193..6c13e8283 100644
--- a/testing/lisp/test-org-capture.el
+++ b/testing/lisp/test-org-capture.el
@@ -606,6 +606,16 @@
 		       "| 2 |" :immediate-finish t))))
 	      (org-capture nil "t"))
 	    (buffer-string))))
+  ;; Prepend | when the template does not start with it
+  (should
+   (equal "| 1 |\n| 2 |\n"
+          (org-test-with-temp-text-in-file "| 1 |\n"
+            (let* ((file (buffer-file-name))
+                   (org-capture-templates
+                    `(("t" "Table" table-line (file ,file)
+                       "2 |" :immediate-finish t))))
+              (org-capture nil "t")
+              (buffer-string)))))
   ;; When `:prepend' is nil, add the row at the end of the table.
   (should
    (equal "| a |\n| x |\n"
-- 
2.44.0