Fwd: Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]
Start of forwarded message From: Ihor Radchenko To: No Wayman Subject: Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)] Date: Sun, 20 Mar 2022 18:26:52 +0800 This patch is not listed in updates.orgmode.org Marking it (hopefully) Best, Ihor End of forwarded message
Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]
Bastien writes:
Hi,
No Wayman 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.
Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]
Hi, No Wayman writes: > The patch you are viewing is outdated. > This is the latest patch on offer: > > https://list.orgmode.org/87bl4rce4j@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/)]
Bastien writes: Hi, No Wayman 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@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/)]
Hi, No Wayman 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/)]
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?
>From 31fbfca4884083adacd95054155cda9ed0128fba Mon Sep 17 00:00:00 2001
From: Nicholas Vollmer
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 ( 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/)]
No Wayman 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/)]
Allen Li 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/)]
No Wayman writes: > Allen Li 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 ( 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
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 writes: No Wayman 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. >From d059c4369ac17fcba885adf87c95c6797894d230 Mon Sep 17 00:00:00 2001 From: Nicholas Vollmer 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 ( 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 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. >From 079f116f6bedcf2c2b1d08c18d71d8e432d94d38 Mon Sep 17 00:00:00 2001 From: Nicholas Vollmer 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 ( 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/)]
Timothy 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/)]
Allen Li 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. >From 34d292a60bc45923589646499b6e06c5cb3ca036 Mon Sep 17 00:00:00 2001 From: Nicholas Vollmer 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 ( 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
Re: [BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]
Timothy 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/)]
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/)]
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/)]
No Wayman 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 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/)]
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
[BUG] [BUG] inconsistent behavior when reading multiple tags [9.4.6 (9.4.6-g366444 @ /home/n/.emacs.d/straight/build/org/)]
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/)
>From 2ec28b530eb28cf788c38556ec3cb97f3bacd4af Mon Sep 17 00:00:00 2001
From: Nicholas Vollmer
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
