[BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

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


Remember to cover the basics, that is, what you expected to happen 
and
what in fact did happen.  You don't know how to make a good 
report?  See

     https://orgmode.org/manual/Feedback.html#Feedback

Your bug report will be posted to the Org mailing list.
------------------------------------------------------------------------

622f9fa76c8ee0766b15945c013b0950d703b955 ("org: Use crm for 
completing tags")
lexically binds crm-separator to ":" while prompting for multiple 
tags in
org-set-tags-command and org-capture-fill-template.

org-set-tags-command correctly parses the tags when using a comma 
(the default
crm-separator) to separate them, but completion is broken unless 
":" is used as
the separator.
org-capture-fill-template incorrectly parses this.

Reproducible from emacs -Q:

org-set-tags-command:
1. Insert a new Org entry in the a buffer 
2. M-x org-set-tags-command 
3. At the prompt enter "one,two,three"
4. Entry's tags are correctly set to :one:two:three: 

org-capture-fill-template:
1. evaluate (org-capture-fill-template "%^g")
2. At the prompt enter "one,two,three"
3. String returned is incorrect: ":one,two,three:"


I can think of a couple ways we could improve this and make the 
behavior consistent.
We could introduce a defcustom which allows the user to choose 
whether or not
Org will rebind crm-separator during commands/functions which 
utilize
completing-read-multiple.
e.g.

#+begin_src emacs-lisp :lexical t
(defcustom org-contextual-crm-separator t
  "Use contextual binding of crm-separator during Org commands.
For example, when setting tags, \":\" is used as the separator.
If nil, `crm-separator' is used.")
#+end_src

Alternatively, we could change the lexical binding we're using 
during these commands/functions.
Instead of binding the separator explicitly to "[ \t]*:[ \t]*", we 
could instead use a regexp
which will match any character not described by org-tag-re. The 
benefit of
this is that completing-read-multiple completion will work for any 
separator
that doesn't match org-tag-re transparently and the results should 
be
consistent between org-set-tags-command and 
org-capture-fill-template.

The attached patch implements that change, which seems to do the 
right thing
from my testing. I can polish it, but I wanted to get opinions on 
the solution first.

Note this does not change the case of org-todo-list, which binds
crm-separator to "|". I chose not to touch that because I'm not 
fully aware 
of what the restrictions on characters for todo-keywords are, if 
any.
org-todo-list also does the right thing by mentioning the 
separator in the prompt.

We could also combine the approaches:

- Introduce a defcustom that determines if we're going to 
  contextually rebind crm-separator
- Use a broader regexp when contextually binding crm-separator
- Mention the separator in the prompt when rebound

Thoughts?


Emacs  : GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ 
Version 3.24.30, cairo version 1.17.4)
 of 2021-08-02
Package: Org mode version 9.4.6 (9.4.6-g366444 @ 
/home/n/.emacs.d/straight/build/org/)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: rfc-broader-regexp-for-lexically-bound-crm-separator --]
[-- Type: text/x-patch, Size: 2064 bytes --]

From 2ec28b530eb28cf788c38556ec3cb97f3bacd4af Mon Sep 17 00:00:00 2001
From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
Date: Thu, 26 Aug 2021 12:51:27 -0400
Subject: [PATCH] RFC: broader regexp for lexically bound crm-separator

For testing purposes. Ammended commit message pending.
---
 lisp/org-capture.el | 2 +-
 lisp/org.el         | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index c51744680..47b47c3ca 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1740,7 +1740,7 @@ The template may still contain \"%?\" for cursor positioning."
 			    (org-add-colon-after-tag-completion t)
 			    (ins (mapconcat
 				  #'identity
-				  (let ((crm-separator "[ \t]*:[ \t]*"))
+				  (let ((crm-separator org-tag-crm-separator))
                                     (completing-read-multiple
 				     (if prompt (concat prompt ": ") "Tags: ")
 				     org-last-tags-completion-table nil nil nil
diff --git a/lisp/org.el b/lisp/org.el
index ce68f4692..f5e396cba 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -602,6 +602,9 @@ not contribute to the agenda listings.")
 (defconst org-tag-re "[[:alnum:]_@#%]+"
   "Regexp matching a single tag.")
 
+(defconst org-tag-crm-separator "[ \t]*[^[:alnum:]_@#%][ \t]*"
+  "Regexp used to determine `crm-separator' when reading multiple tags.")
+
 (defconst org-tag-group-re "[ \t]+\\(:\\([[:alnum:]_@#%:]+\\):\\)[ \t]*$"
   "Regexp matching the tag group at the end of a line, with leading spaces.
 Tags are stored in match group 1.  Match group 2 stores the tags
@@ -12066,7 +12069,7 @@ in Lisp code use `org-set-tags' instead."
 		      table
 		      (and org-fast-tag-selection-include-todo org-todo-key-alist))
 		   (let ((org-add-colon-after-tag-completion (< 1 (length table)))
-                         (crm-separator "[ \t]*:[ \t]*"))
+                         (crm-separator org-tag-crm-separator))
 		     (mapconcat #'identity
                                 (completing-read-multiple
 			         "Tags: "
-- 
2.33.0

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[Timothy]

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

Hi NoWayman,

Thanks for your suggestion. At a glance it looks reasonable to me, but would you
be able to explain the default value you’ve set? It isn’t obvious to me how you
arrived at `"[ \t]*[^[:alnum:]_@#%][ \t]*"'. Also, do you think a
one-size-fits-all solution is a good fit here? I don’t do much with tags
personally, so I’m probably not the best person to judge.

> (defconst org-tag-crm-separator “[ ]*[^[:alnum:]_@#%][ ]*”
>   “Regexp used to determine `crm-separator’ when reading multiple tags.”)

All the best,
Timothy

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Timothy <tecosaur@gmail.com> writes:

> Hi NoWayman,
>
> Thanks for your suggestion. At a glance it looks reasonable to 
> me, but would you
> be able to explain the default value you’ve set? It isn’t 
> obvious to me how you
> arrived at `"[ \t]*[^[:alnum:]_@#%][ \t]*"'. Also, do you think 
> a
> one-size-fits-all solution is a good fit here? I don’t do much 
> with tags
> personally, so I’m probably not the best person to judge.
>
>> (defconst org-tag-crm-separator “[ ]*[^[:alnum:]_@#%][ ]*”
>>   “Regexp used to determine `crm-separator’ when reading 
>>   multiple tags.”)
>
> All the best,
> Timothy

I was aiming for the inverse of `org-tag-re' which is:

"[[:alnum:]_@#%]+"

The idea being we treat any character which is not a valid tag 
character as the crm separator.

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[Allen Li]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> org-set-tags-command correctly parses the tags when using a comma
> (the default
> crm-separator) to separate them, but completion is broken unless
> ":" is used as
> the separator.
> org-capture-fill-template incorrectly parses this.
>
> Reproducible from emacs -Q:
>
> org-set-tags-command:
> 1. Insert a new Org entry in the a buffer
> 2. M-x org-set-tags-command
> 3. At the prompt enter "one,two,three"
> 4. Entry's tags are correctly set to :one:two:three:
>
> org-capture-fill-template:
> 1. evaluate (org-capture-fill-template "%^g")
> 2. At the prompt enter "one,two,three"
> 3. String returned is incorrect: ":one,two,three:"

Sorry about that (I wrote the crm patch).  I did not consider people
deliberately using invalid tag characters to separate tags.  It's an
(un?)happy coincidence that org-set-tags-commands retains this behavior,
because the fast tags selection logic gets in the way.

The inconsistency in behavior can be easily fixed by deleting the code
in org-set-tags-commands that replaces invalid tag characters with ":".

The question here is, which behavior do we want?  My philosphy is that
programs shouldn't try to silently re-interpret the user's intentions.
For example, if I accidentally mistyped the tag "green_blue" as
"green-blue", I don't want Org to "helpfully" split one tag into two
tags "green:blue".  I may not realize the data corruption until too
late.

If we want the other behavior (invalid tag characters can separate
tags), then it's also a simple matter of changing crm-separator wherever
only tags are completed to recognize all invalid tag characters.

There's also the option to only allow ":" and "," as separators.

> Note this does not change the case of org-todo-list, which binds
> crm-separator to "|". I chose not to touch that because I'm not
> fully aware
> of what the restrictions on characters for todo-keywords are, if
> any.
> org-todo-list also does the right thing by mentioning the
> separator in the prompt.

Agreed, org-todo-list is out of scope.

>
> We could also combine the approaches:
>
> - Introduce a defcustom that determines if we're going to
>   contextually rebind crm-separator
> - Use a broader regexp when contextually binding crm-separator
> - Mention the separator in the prompt when rebound

Whichever behavior we choose, I don't think it's worth making it customizable.

>
> Thoughts?
>
>
> Emacs  : GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+
> Version 3.24.30, cairo version 1.17.4)
>  of 2021-08-02
> Package: Org mode version 9.4.6 (9.4.6-g366444 @
> /home/n/.emacs.d/straight/build/org/)

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[Timothy]

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

Hi NoWayman,

>> would you be able to explain the default value you’ve set? It isn’t obvious to me how you
> I was aiming for the inverse of `org-tag-re’ which is:
>
> “[[:alnum:]_@#%]+”
>
> The idea being we treat any character which is not a valid tag character as the
> crm separator.

Ah, right — that looks sensible. I think we should probably note that in a
comment somewhere, or perhaps construct the regexp with `rx' so it’s actually
using `org-tag-re'?

All the best,
Timothy

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[Timothy]

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

Hi Allen,

> The question here is, which behavior do we want?  My philosphy is that
> programs shouldn’t try to silently re-interpret the user’s intentions.
> For example, if I accidentally mistyped the tag “green_blue” as
> “green-blue”, I don’t want Org to “helpfully” split one tag into two
> tags “green:blue”.  I may not realize the data corruption until too
> late.
>
> If we want the other behavior (invalid tag characters can separate
> tags), then it’s also a simple matter of changing crm-separator wherever
> only tags are completed to recognize all invalid tag characters.
>
> There’s also the option to only allow “:” and “,” as separators.
>
> Whichever behavior we choose, I don’t think it’s worth making it customizable.

Reading your thoughts I’m inclined to agree, perhaps it’s best if we settle on a
set of “sensible” separation characters, document them, and leave it at that.

NoWayman, what do you think of that?

All the best,
Timothy

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Timothy <tecosaur@gmail.com> writes:

> Ah, right — that looks sensible. I think we should probably note 
> that in a
> comment somewhere,

Yes, a comment is warranted.
I'll address minor issues like this once a solution is agreed 
upon.

> or perhaps construct the regexp with `rx' so it’s actually using 
> `org-tag-re'?

Unfortunately I don't think it's that simple.
They are very similar regexps, but I don't think we can just plug 
`org-tag-re'
into an rx form and have it work. A direct comparison:


 org-tag-re =>            "[[:alnum:]_@#%]+"
 org-tag-crm-separator => "[^[:alnum:]_@#%]"

This is brittle, but we could define `org-tag-crm-separator' in 
terms of `org-tag-re' like so:

(defvar org-tag-crm-separator (concat "[^" (substring org-tag-re 1 
-1))))

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

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


Allen Li <darkfeline@felesatra.moe> writes:

> Sorry about that (I wrote the crm patch).  I did not consider 
> people
> deliberately using invalid tag characters to separate tags. 
> It's an
> (un?)happy coincidence that org-set-tags-commands retains this 
> behavior,
> because the fast tags selection logic gets in the way.
>
> The inconsistency in behavior can be easily fixed by deleting 
> the code
> in org-set-tags-commands that replaces invalid tag characters 
> with ":".
>
> The question here is, which behavior do we want?  My philosphy 
> is that
> programs shouldn't try to silently re-interpret the user's 
> intentions.
> For example, if I accidentally mistyped the tag "green_blue" as
> "green-blue", I don't want Org to "helpfully" split one tag into 
> two
> tags "green:blue".  I may not realize the data corruption until 
> too
> late.

Consider the current behavior of `org-capture-fill-template':

If you were to mistype your tag as "green-blue" it would be 
captured as part of the headline string instead of a set of tags.
Future tag completions would not include any reference to it, and 
so you likely wouldn't notice until long after the fact 
(especially in the case of a template with a non-nil 
:immediate-finish).
So the risk of data corruption exists now by allowing the function 
to return an invalid tag string.
 
> There's also the option to only allow ":" and "," as separators.
> Whichever behavior we choose, I don't think it's worth making it 
> customizable.

I would rather not have to type ":" to separate Org tags when CRM 
allows me to use ","
(or another character of my choice) everywhere else. This violates 
the principle of least surprise.

An even simpler solution than my original patch is to have a 
defcustom which allows the user to customize
the CRM separator character(s) Org uses to read tags.

See the attached patch for a first draft implementation.

It defaults to what you've proposed (allowing ":" and "," to 
delimit tags).
It avoids introducing a new regexp variable which needs to be 
maintained in lockstep with `org-tag-re'.
It's customizable.
It informs the user about the tag delimiting characters in the 
prompt.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: org-tags-crm-separators-defcustom --]
[-- Type: text/x-patch, Size: 3339 bytes --]

From 34d292a60bc45923589646499b6e06c5cb3ca036 Mon Sep 17 00:00:00 2001
From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
Date: Fri, 3 Sep 2021 14:50:48 -0400
Subject: [PATCH] Add org-tags-crm-separators

commit message/formatting pending discussion of solution.
---
 lisp/org-capture.el |  7 +++++--
 lisp/org.el         | 17 ++++++++++++++---
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index c51744680..e9cd8f490 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1740,9 +1740,12 @@ The template may still contain \"%?\" for cursor positioning."
 			    (org-add-colon-after-tag-completion t)
 			    (ins (mapconcat
 				  #'identity
-				  (let ((crm-separator "[ \t]*:[ \t]*"))
+				  (let* ((separators (or org-tags-crm-separators '(?: ?,)))
+                                         (crm-separator (org-tags-crm-regexp separators)))
                                     (completing-read-multiple
-				     (if prompt (concat prompt ": ") "Tags: ")
+				     (if prompt (concat prompt ": ")
+                                       (format "Tags (%s to delimit): "
+                                               (string-join (mapcar #'char-to-string separators) " ")))
 				     org-last-tags-completion-table nil nil nil
 				     'org-tags-history))
 				  ":")))
diff --git a/lisp/org.el b/lisp/org.el
index ce68f4692..d6739f7ed 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -2980,6 +2980,11 @@ is better to limit inheritance to certain tags using the variables
 	  (const :tag "Reverse alphabetical" org-string-collate-greaterp)
 	  (function :tag "Custom function" nil)))
 
+(defcustom org-tags-crm-separators '(?: ?,)
+  "List of characters used to separate tags during Org commands which read mutiple tags."
+  :type 'list
+  :group 'org-tags)
+
 (defvar org-tags-history nil
   "History of minibuffer reads for tags.")
 (defvar org-last-tags-completion-table nil
@@ -12007,6 +12012,10 @@ tags."
 	;; it now points to BLANK-START.  Use COLUMN instead.
 	(if in-blank? (org-move-to-column column) (goto-char origin))))))
 
+(defun org-tags-crm-regexp (chars)
+  "Return `crm-separator' regexp using CHARS as separators."
+  (format "[ \t]*%s[ \t]*" (regexp-opt (mapcar #'char-to-string chars))))
+
 (defun org-set-tags-command (&optional arg)
   "Set the tags for the current visible entry.
 
@@ -12065,11 +12074,13 @@ in Lisp code use `org-set-tags' instead."
 		      inherited-tags
 		      table
 		      (and org-fast-tag-selection-include-todo org-todo-key-alist))
-		   (let ((org-add-colon-after-tag-completion (< 1 (length table)))
-                         (crm-separator "[ \t]*:[ \t]*"))
+		   (let* ((org-add-colon-after-tag-completion (< 1 (length table)))
+                          (separators (or org-tags-crm-separators '(?: ?,)))
+                          (crm-separator (org-tags-crm-regexp separators)))
 		     (mapconcat #'identity
                                 (completing-read-multiple
-			         "Tags: "
+                                 (format "Tags (%s to delimit): "
+                                         (string-join (mapcar #'char-to-string separators) " "))
 			         org-last-tags-completion-table
 			         nil nil (org-make-tag-string current-tags)
 			         'org-tags-history)
-- 
2.33.0

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Timothy <tecosaur@gmail.com> writes:

> Reading your thoughts I’m inclined to agree, perhaps it’s best 
> if we settle on a
> set of “sensible” separation characters, document them, and 
> leave it at that.
>
> NoWayman, what do you think of that?


I disagree with the idea that it shouldn't be customizable.
See the second patch I've offered.
I feel it addresses the issue without preventing customization.

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

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


No Wayman <iarchivedmywholelife@gmail.com> writes:

> See the attached patch for a first draft implementation.

Attached is a second draft which compiles cleanly and makes use of 
mapconcat where appropriate.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: org-tags-crm-separators-2 --]
[-- Type: text/x-patch, Size: 3298 bytes --]

From 079f116f6bedcf2c2b1d08c18d71d8e432d94d38 Mon Sep 17 00:00:00 2001
From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
Date: Fri, 3 Sep 2021 14:50:48 -0400
Subject: [PATCH] Add org-tags-crm-separators

commit message/formatting pending discussion of solution.
---
 lisp/org-capture.el |  7 +++++--
 lisp/org.el         | 17 ++++++++++++++---
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index c51744680..e51d039d5 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1740,9 +1740,12 @@ The template may still contain \"%?\" for cursor positioning."
 			    (org-add-colon-after-tag-completion t)
 			    (ins (mapconcat
 				  #'identity
-				  (let ((crm-separator "[ \t]*:[ \t]*"))
+				  (let* ((separators (or org-tags-crm-separators '(?: ?,)))
+                                         (crm-separator (org-tags-crm-regexp separators)))
                                     (completing-read-multiple
-				     (if prompt (concat prompt ": ") "Tags: ")
+				     (if prompt (concat prompt ": ")
+                                       (format "Tags (%s to delimit): "
+                                               (mapconcat #'char-to-string separators " ")))
 				     org-last-tags-completion-table nil nil nil
 				     'org-tags-history))
 				  ":")))
diff --git a/lisp/org.el b/lisp/org.el
index ce68f4692..4cd173c99 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -2980,6 +2980,11 @@ is better to limit inheritance to certain tags using the variables
 	  (const :tag "Reverse alphabetical" org-string-collate-greaterp)
 	  (function :tag "Custom function" nil)))
 
+(defcustom org-tags-crm-separators '(?: ?,)
+  "List of tag delimiting characters used when reading multiple tags."
+  :type 'list
+  :group 'org-tags)
+
 (defvar org-tags-history nil
   "History of minibuffer reads for tags.")
 (defvar org-last-tags-completion-table nil
@@ -12007,6 +12012,10 @@ tags."
 	;; it now points to BLANK-START.  Use COLUMN instead.
 	(if in-blank? (org-move-to-column column) (goto-char origin))))))
 
+(defun org-tags-crm-regexp (chars)
+  "Return `crm-separator' regexp using CHARS as separators."
+  (format "[ \t]*%s[ \t]*" (regexp-opt (mapcar #'char-to-string chars))))
+
 (defun org-set-tags-command (&optional arg)
   "Set the tags for the current visible entry.
 
@@ -12065,11 +12074,13 @@ in Lisp code use `org-set-tags' instead."
 		      inherited-tags
 		      table
 		      (and org-fast-tag-selection-include-todo org-todo-key-alist))
-		   (let ((org-add-colon-after-tag-completion (< 1 (length table)))
-                         (crm-separator "[ \t]*:[ \t]*"))
+		   (let* ((org-add-colon-after-tag-completion (< 1 (length table)))
+                          (separators (or org-tags-crm-separators '(?: ?,)))
+                          (crm-separator (org-tags-crm-regexp separators)))
 		     (mapconcat #'identity
                                 (completing-read-multiple
-			         "Tags: "
+                                 (format "Tags (%s to delimit): "
+                                         (mapconcat #'char-to-string separators " "))
 			         org-last-tags-completion-table
 			         nil nil (org-make-tag-string current-tags)
 			         'org-tags-history)
-- 
2.33.0

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

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


No Wayman <iarchivedmywholelife@gmail.com> writes:

> No Wayman <iarchivedmywholelife@gmail.com> writes:
>
>> See the attached patch for a first draft implementation.
>
> Attached is a second draft which compiles cleanly and makes use 
> of mapconcat
> where appropriate.
>
> [2. org-tags-crm-separators-2 --- text/x-patch; 
> 0001-Add-org-tags-crm-separators.patch]...

See attached with amended commit message.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: org-tags-crm-separators-3 --]
[-- Type: text/x-patch, Size: 3513 bytes --]

From d059c4369ac17fcba885adf87c95c6797894d230 Mon Sep 17 00:00:00 2001
From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
Date: Fri, 3 Sep 2021 14:50:48 -0400
Subject: [PATCH] Add org-tags-crm-separators

* org.el (org-tags-crm-separators): Defcustom controlling which
characters are used to delimit tags in commands which utilize
`completing-read-multiple'.
(org-set-tags-command, org-capture-fill-template): Make use of org-tags-crm-separators; Show
delimiters in tag prompt.
---
 lisp/org-capture.el |  7 +++++--
 lisp/org.el         | 17 ++++++++++++++---
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index c51744680..e51d039d5 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1740,9 +1740,12 @@ The template may still contain \"%?\" for cursor positioning."
 			    (org-add-colon-after-tag-completion t)
 			    (ins (mapconcat
 				  #'identity
-				  (let ((crm-separator "[ \t]*:[ \t]*"))
+				  (let* ((separators (or org-tags-crm-separators '(?: ?,)))
+                                         (crm-separator (org-tags-crm-regexp separators)))
                                     (completing-read-multiple
-				     (if prompt (concat prompt ": ") "Tags: ")
+				     (if prompt (concat prompt ": ")
+                                       (format "Tags (%s to delimit): "
+                                               (mapconcat #'char-to-string separators " ")))
 				     org-last-tags-completion-table nil nil nil
 				     'org-tags-history))
 				  ":")))
diff --git a/lisp/org.el b/lisp/org.el
index ce68f4692..4cd173c99 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -2980,6 +2980,11 @@ is better to limit inheritance to certain tags using the variables
 	  (const :tag "Reverse alphabetical" org-string-collate-greaterp)
 	  (function :tag "Custom function" nil)))
 
+(defcustom org-tags-crm-separators '(?: ?,)
+  "List of tag delimiting characters used when reading multiple tags."
+  :type 'list
+  :group 'org-tags)
+
 (defvar org-tags-history nil
   "History of minibuffer reads for tags.")
 (defvar org-last-tags-completion-table nil
@@ -12007,6 +12012,10 @@ tags."
 	;; it now points to BLANK-START.  Use COLUMN instead.
 	(if in-blank? (org-move-to-column column) (goto-char origin))))))
 
+(defun org-tags-crm-regexp (chars)
+  "Return `crm-separator' regexp using CHARS as separators."
+  (format "[ \t]*%s[ \t]*" (regexp-opt (mapcar #'char-to-string chars))))
+
 (defun org-set-tags-command (&optional arg)
   "Set the tags for the current visible entry.
 
@@ -12065,11 +12074,13 @@ in Lisp code use `org-set-tags' instead."
 		      inherited-tags
 		      table
 		      (and org-fast-tag-selection-include-todo org-todo-key-alist))
-		   (let ((org-add-colon-after-tag-completion (< 1 (length table)))
-                         (crm-separator "[ \t]*:[ \t]*"))
+		   (let* ((org-add-colon-after-tag-completion (< 1 (length table)))
+                          (separators (or org-tags-crm-separators '(?: ?,)))
+                          (crm-separator (org-tags-crm-regexp separators)))
 		     (mapconcat #'identity
                                 (completing-read-multiple
-			         "Tags: "
+                                 (format "Tags (%s to delimit): "
+                                         (mapconcat #'char-to-string separators " "))
 			         org-last-tags-completion-table
 			         nil nil (org-make-tag-string current-tags)
 			         'org-tags-history)
-- 
2.33.0

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[Allen Li]

No Wayman <iarchivedmywholelife@gmail.com> writes:
> Allen Li <darkfeline@felesatra.moe> writes:
>> The question here is, which behavior do we want?  My philosphy 
>> is that
>> programs shouldn't try to silently re-interpret the user's 
>> intentions.
>> For example, if I accidentally mistyped the tag "green_blue" as
>> "green-blue", I don't want Org to "helpfully" split one tag into 
>> two
>> tags "green:blue".  I may not realize the data corruption until 
>> too
>> late.
>
> Consider the current behavior of `org-capture-fill-template':
>
> If you were to mistype your tag as "green-blue" it would be 
> captured as part of the headline string instead of a set of tags.
> Future tag completions would not include any reference to it, and 
> so you likely wouldn't notice until long after the fact 
> (especially in the case of a template with a non-nil 
> :immediate-finish).
> So the risk of data corruption exists now by allowing the function 
> to return an invalid tag string.

green-blue is recoverable, and green:blue is not.  Consider a file where
some headings are tagged :green:blue: and some are tagged :green_blue:.
If green-blue gets changed into :green:blue:, then it is no longer
possible to tell which :green:blue: headings are supposed to be
:green_blue:.  If they were left as green-blue, it is trivial to fix
them with a search-replace.  It is also easy to notice that they were
typed incorrectly because the tags would be highlighted differently (as
they are invalid).

> It defaults to what you've proposed (allowing ":" and "," to 
> delimit tags).
> It avoids introducing a new regexp variable which needs to be 
> maintained in lockstep with `org-tag-re'.
> It's customizable.
> It informs the user about the tag delimiting characters in the 
> prompt.

Yes, I think using only ":" and "," is the best default option.  I still
don't think there is a need to make it customizable (I doubt anyone is
typing tags separated with ! or @ or #), but I suppose
there's minimal harm from doing so.

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index c51744680..e51d039d5 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1740,9 +1740,12 @@ The template may still contain \"%?\" for cursor positioning."
 			    (org-add-colon-after-tag-completion t)
 			    (ins (mapconcat
 				  #'identity
-				  (let ((crm-separator "[ \t]*:[ \t]*"))
+				  (let* ((separators (or org-tags-crm-separators '(?: ?,)))
+                                         (crm-separator (org-tags-crm-regexp separators)))
                                     (completing-read-multiple
-				     (if prompt (concat prompt ": ") "Tags: ")
+				     (if prompt (concat prompt ": ")
+                                       (format "Tags (%s to delimit): "
+                                               (mapconcat #'char-to-string separators " ")))
 				     org-last-tags-completion-table nil nil nil
 				     'org-tags-history))
 				  ":")))

I am -0.5 on showing the delimiters since this is not conventional for
completing-read-multiple, especially after we add support for "," like
most other uses of completing-read-multiple.  It needlessly inflates the
length of the prompt.

I suggest adding a helper function for the:

  (separators (or org-tags-crm-separators '(?: ?,)))
   (crm-separator (org-tags-crm-regexp separators))

so you can call it like:

  (crm-separator (org-tags--crm-separator-regexp))

since you repeat this verbatim below.

diff --git a/lisp/org.el b/lisp/org.el
index ce68f4692..4cd173c99 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -2980,6 +2980,11 @@ is better to limit inheritance to certain tags using the variables
 	  (const :tag "Reverse alphabetical" org-string-collate-greaterp)
 	  (function :tag "Custom function" nil)))
 
+(defcustom org-tags-crm-separators '(?: ?,)
+  "List of tag delimiting characters used when reading multiple tags."
+  :type 'list
+  :group 'org-tags)
+
 (defvar org-tags-history nil
   "History of minibuffer reads for tags.")
 (defvar org-last-tags-completion-table nil

You should make the :type a list of characters so the widget is more
user friendly.

@@ -12007,6 +12012,10 @@ tags."
 	;; it now points to BLANK-START.  Use COLUMN instead.
 	(if in-blank? (org-move-to-column column) (goto-char origin))))))
 
+(defun org-tags-crm-regexp (chars)
+  "Return `crm-separator' regexp using CHARS as separators."
+  (format "[ \t]*%s[ \t]*" (regexp-opt (mapcar #'char-to-string chars))))
+
 (defun org-set-tags-command (&optional arg)
   "Set the tags for the current visible entry.
 
@@ -12065,11 +12074,13 @@ in Lisp code use `org-set-tags' instead."
 		      inherited-tags
 		      table
 		      (and org-fast-tag-selection-include-todo org-todo-key-alist))
-		   (let ((org-add-colon-after-tag-completion (< 1 (length table)))
-                         (crm-separator "[ \t]*:[ \t]*"))
+		   (let* ((org-add-colon-after-tag-completion (< 1 (length table)))
+                          (separators (or org-tags-crm-separators '(?: ?,)))
+                          (crm-separator (org-tags-crm-regexp separators)))
 		     (mapconcat #'identity
                                 (completing-read-multiple
-			         "Tags: "
+                                 (format "Tags (%s to delimit): "
+                                         (mapconcat #'char-to-string separators " "))
 			         org-last-tags-completion-table
 			         nil nil (org-make-tag-string current-tags)
 			         'org-tags-history)
-- 
2.33.0

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Allen Li <darkfeline@felesatra.moe> writes:

> green-blue is recoverable, and green:blue is not.  Consider a 
> file where
> some headings are tagged :green:blue: and some are tagged 
> :green_blue:.
> If green-blue gets changed into :green:blue:, then it is no 
> longer
> possible to tell which :green:blue: headings are supposed to be
> :green_blue:.  If they were left as green-blue, it is trivial to 
> fix
> them with a search-replace.  It is also easy to notice that they 
> were
> typed incorrectly because the tags would be highlighted 
> differently (as
> they are invalid).

It's not so easy to notice such corruption in the case of captures 
which are filed away and then searched for (by tag) at a later 
time.
But that's neither here nor there. We both agree the behavior 
should be more consistent, and the patch I've proposed takes care 
of that.

> Yes, I think using only ":" and "," is the best default option. 
> I still
> don't think there is a need to make it customizable (I doubt 
> anyone is
> typing tags separated with ! or @ or #), but I suppose
> there's minimal harm from doing so.

I don't see the need to prevent customization here, either.
There may be use cases we don't anticiapte and it adds very little 
in the way of maitenance.
Consider if the author of crm.el decided to hardcode the 
separator.
Your original patch would not have been so trivial.

> I am -0.5 on showing the delimiters since this is not 
> conventional for
> completing-read-multiple, especially after we add support for 
> "," like
> most other uses of completing-read-multiple.  It needlessly 
> inflates the
> length of the prompt.

I don't know what you mean by -0.5, but I wouldn't say it's 
needless.
`org-todo-list' adds the following to the prompt:

> "Keyword (or KWD1|KWD2|...): "

We're talking a handful of characters at most. e.g.

> "Tags (: , to delimit): "

Actually shorter than what `org-todo-list' does now.
I'm open to suggestions on improving that prompt format as well.

> I suggest adding a helper function for the:
>
>   (separators (or org-tags-crm-separators '(?: ?,)))
>    (crm-separator (org-tags-crm-regexp separators))
>
> so you can call it like:
>
>   (crm-separator (org-tags--crm-separator-regexp))
>
> since you repeat this verbatim below.

The separators are stored in a separate variable for use in the 
prompt.
I'll have to take another look to see if we can eliminate the 
duplicate code.
I do agree the function can be "private" and named more 
appropriately.
I'll change that in the next revision.

> You should make the :type a list of characters so the widget is 
> more
> user friendly
 
Agreed. I'll tidy that up as well once a solution is agreed upon.
(IME Org/Emacs patches require more discussion up front, so I'd 
rather deal with that first
instead of putting too much time into a trivial patch up front)

So it looks like the remaining issue is whether or not it's worth 
displaying the tag delimiters in the prompt. I'll think on it some 
more and give it some time to see if anyone else has any arguments 
in favor or against the idea. If I don't see anything by the 
weekend, I'll amend the patch with the changes suggested above.

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[Allen Li]

No Wayman <iarchivedmywholelife@gmail.com> writes:
>> Yes, I think using only ":" and "," is the best default option. I
>> still
>> don't think there is a need to make it customizable (I doubt anyone
>> is
>> typing tags separated with ! or @ or #), but I suppose
>> there's minimal harm from doing so.
>
> I don't see the need to prevent customization here, either.
> There may be use cases we don't anticiapte and it adds very little in
> the way of maitenance.
> Consider if the author of crm.el decided to hardcode the separator.
> Your original patch would not have been so trivial.

crm.el is a library whose purpose is to provide a customizable
completion API.  That's not comparable to Org mode's tag setting commands.

>
>> I am -0.5 on showing the delimiters since this is not conventional
>> for
>> completing-read-multiple, especially after we add support for ","
>> like
>> most other uses of completing-read-multiple.  It needlessly inflates
>> the
>> length of the prompt.
>
> I don't know what you mean by -0.5, but I wouldn't say it's needless.
> `org-todo-list' adds the following to the prompt:
>
>> "Keyword (or KWD1|KWD2|...): "
>
> We're talking a handful of characters at most. e.g.
>
>> "Tags (: , to delimit): "
>
> Actually shorter than what `org-todo-list' does now.
> I'm open to suggestions on improving that prompt format as well.

-0.5 means slightly against (+1 means agree and -1 means disagree).

> So it looks like the remaining issue is whether or not it's worth
> displaying the tag delimiters in the prompt. I'll think on it some 
> more and give it some time to see if anyone else has any arguments in
> favor or against the idea. If I don't see anything by the weekend,
> I'll amend the patch with the changes suggested above.

I don't want to bikeshed it further since it is not that important and
not worth either of our time.  I have already stated my reasons that I
would personally not add these things, but it does not bother me enough,
nor is it significant enough to spend more time on.

Whether to make the delimiters customizable: No, because I don't think
there's a need.  Maybe one Org mode user will need it and they can
define their own command.

Whether to display separators in prompt: No, because both , and : are
intuitive, and also no if delimiters are customizable since the user
knows what the delimiters are (since they set them).

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

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


Tim, Allen:

The attached compromise implements the bare minimum.
Tags can be separated with "," or ":" in the previously mentioned 
cases.
Scrapped the defcustom and showing delimiters in the prompt.
Any objections?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: allow-comma-to-separate-tags --]
[-- Type: text/x-patch, Size: 2026 bytes --]

From 31fbfca4884083adacd95054155cda9ed0128fba Mon Sep 17 00:00:00 2001
From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
Date: Fri, 3 Sep 2021 14:50:48 -0400
Subject: [PATCH] Allow "," to delimit tags

* org.el: Add `org-tags-crm-separtor' regexp
(org-set-tags-command, org-capture-fill-template): Use `org-tags-crm-separators'.
---
 lisp/org-capture.el | 2 +-
 lisp/org.el         | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index cbdb30c03..cd65f8e6d 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1739,7 +1739,7 @@ The template may still contain \"%?\" for cursor positioning."
 			    (org-add-colon-after-tag-completion t)
 			    (ins (mapconcat
 				  #'identity
-				  (let ((crm-separator "[ \t]*:[ \t]*"))
+				  (let ((crm-separator org-tags-crm-separator))
                                     (completing-read-multiple
 				     (if prompt (concat prompt ": ") "Tags: ")
 				     org-last-tags-completion-table nil nil nil
diff --git a/lisp/org.el b/lisp/org.el
index 8b9d57157..572e291c7 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -12007,6 +12007,9 @@ tags."
 	;; it now points to BLANK-START.  Use COLUMN instead.
 	(if in-blank? (org-move-to-column column) (goto-char origin))))))
 
+(defvar org-tags-crm-separator (format "[ \t]*%s[ \t]*" (regexp-opt '("," ":")))
+  "Regexp to separate tags in commands which use `completing-read-multiple'.")
+
 (defun org-set-tags-command (&optional arg)
   "Set the tags for the current visible entry.
 
@@ -12066,7 +12069,7 @@ in Lisp code use `org-set-tags' instead."
 		      table
 		      (and org-fast-tag-selection-include-todo org-todo-key-alist))
 		   (let ((org-add-colon-after-tag-completion (< 1 (length table)))
-                         (crm-separator "[ \t]*:[ \t]*"))
+                         (crm-separator org-tags-crm-separator))
 		     (mapconcat #'identity
                                 (completing-read-multiple
 			         "Tags: "
-- 
2.33.0

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[Bastien]

Hi,

No Wayman <iarchivedmywholelife@gmail.com> writes:

> * org.el (org-tags-crm-separators): Defcustom controlling which
> characters are used to delimit tags in commands which utilize
> `completing-read-multiple'.

Why should we allow anything else than ":" for separating tags in
commands which utilize completing-read-multiple?

I surely miss something obvious.

If we relax a constraint, I'd rather have this hardcoded and well
documented than adding a new defvar or defcustom.

-- 
 Bastien

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Bastien <bzg@gnu.org> writes:

> Hi,
>
> No Wayman <iarchivedmywholelife@gmail.com> writes:
>
>> * org.el (org-tags-crm-separators): Defcustom controlling which
>> characters are used to delimit tags in commands which utilize
>> `completing-read-multiple'.
>
> Why should we allow anything else than ":" for separating tags 
> in
> commands which utilize completing-read-multiple?
>
> I surely miss something obvious.

The patch you are viewing is outdated.
This is the latest patch on offer:

https://list.orgmode.org/87bl4rce4j.fsf@gmail.com/2-0001-Allow-to-delimit-tags.patch

It allows "," (the default for completing-read-multiple) and ":" 
to delimit tags when completing-read-multiple is used.
The reason for allowing "," is that it's easier to type than ":". 
I make liberal use of tags and IMO typing a "Shift+;" between each 
tag is annoying and slow.
The comma is also used as the default separator when 
completing-read-multiple is used.

> If we relax a constraint, I'd rather have this hardcoded and 
> well
> documented than adding a new defvar or defcustom.

The latest patch removed the defcustom and replaced it with a 
defvar for the crm-separator regexp.
If it would ease your mind I'd be happy to convert it to a 
defconst.
It's also worth noting that the constraint was only recently 
introduced.
"," worked fine to delimit tags in `org-set-tags-command' prior to 
the switch to completing-read-multiple.

Regarding documentation, let me know where you'd prefer it 
documented. 

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[Bastien]

Hi,

No Wayman <iarchivedmywholelife@gmail.com> writes:

> The patch you are viewing is outdated.
> This is the latest patch on offer:
>
> https://list.orgmode.org/87bl4rce4j.fsf@gmail.com/2-0001-Allow-to-delimit-tags.patch

Thanks.

> It allows "," (the default for completing-read-multiple) and ":" to
> delimit tags when completing-read-multiple is used.

What does not work if we don't allow ","?

> The reason for allowing "," is that it's easier to type than ":". I
> make liberal use of tags and IMO typing a "Shift+;" between each tag
> is annoying and slow.

Okay (but note that we don't all use the same keyboard...)

> The comma is also used as the default separator when
> completing-read-multiple is used.
>
>> If we relax a constraint, I'd rather have this hardcoded and well
>> documented than adding a new defvar or defcustom.
>
> The latest patch removed the defcustom and replaced it with a defvar
> for the crm-separator regexp.
> If it would ease your mind I'd be happy to convert it to a defconst.

Instead of a defvar that we don't want the user to modify, why not
hardcoding the addition of the coma?  I'd prefer this.

> It's also worth noting that the constraint was only recently
> introduced.
> "," worked fine to delimit tags in `org-set-tags-command' prior to the
> switch to completing-read-multiple.

Okay, this buys it - if I understand what really does not work when
allowing ":".

> Regarding documentation, let me know where you'd prefer it
> documented.

Any place in the manual that refers to tag separators, explicitely or
implicitely, I've not checked if there are some.  Also in the code
itself, as a comment, to explain why both "," and ":" should be
allowed (avoid the keyboard-based argument, which is too subjective.)

Thanks!

-- 
 Bastien

Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Bastien <bzg@gnu.org> writes:

> Hi,
>
> No Wayman <iarchivedmywholelife@gmail.com> writes:
>
> Instead of a defvar that we don't want the user to modify, why 
> not
> hardcoding the addition of the coma?  I'd prefer this.

`completing-read-multiple' is currently used in two spots to read 
tags:
`org-set-tags-command' and `org-capture-fill-template'.
To be clear, you would rather I hard code the same regexp and 
comment in those two locations rather than abstracting it into a 
defconst?

> Any place in the manual that refers to tag separators, 
> explicitly or
> implicitly, I've not checked if there are some.  Also in the 
> code
> itself, as a comment, to explain why both "," and ":" should be
> allowed

I don't see why we need to add anything to the manual.
We're not changing the syntax of tags.
We're utilizing the normal behavior of completing-read-multiple.
The manual states for `org-set-tags-command':

"By default Org mode uses the standard minibuffer completion 
facilities for entering tags."

The comma is standard when reading multiple items.
That seems to cover it to me.

Another solution I proposed was indicating the delimiters in the 
minibuffer prompt similar to what is currently done in 
`org-todo-list'.
e.g.

org-todo-list prompt:        "Keyword (or KWD1|KWD2|...): "
org-set-tags-command prompt: "Tags ("," or ":" to delimit): "

The only argument I heard against that was from one person who 
thought it "wasted space".
I don't consider that a strong argument considering the length of 
the prompt for `org-todo-list'.

>(avoid the keyboard-based argument, which is too subjective.)

If one can customize their keyboard layout, I don't see why we 
should be so rigid about a delimiter in a prompt.
It's a moot point, though, I've abandoned that proposal because it 
was apparently too controversial.