[-- Attachment #1: Type: text/plain, Size: 349 bytes --] Hello, Usage of org-set-tags-command can be improved by using completing-read-multiple so you continue to get completion after the first tag. This is my first contribution to org I followed https://orgmode.org/worg/org-contribute.html and hope I got everything right. I have signed the FSF documents (I have packages on ELPA). Clemens [-- Attachment #2: 0001-org.el-Use-completing-read-multiple-for-org-set-tags.patch --] [-- Type: text/x-patch, Size: 1403 bytes --] From c8be9106110f266db774d73af4dcb6fbcef3bef8 Mon Sep 17 00:00:00 2001 From: Clemens Radermacher <clemera@posteo.net> Date: Sun, 19 Jul 2020 14:30:37 +0200 Subject: [PATCH] org.el: Use `completing-read-multiple' for `org-set-tags-command' * lisp/org.el (org-set-tags-command): Use `completing-read-multiple' when prompting for tags. --- lisp/org.el | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lisp/org.el b/lisp/org.el index 12a853bd6..e804ec7dd 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -11877,12 +11877,16 @@ (defun org-set-tags-command (&optional arg) inherited-tags table (and org-fast-tag-selection-include-todo org-todo-key-alist)) - (let ((org-add-colon-after-tag-completion (< 1 (length table)))) - (org-trim (completing-read - "Tags: " - #'org-tags-completion-function - nil nil (org-make-tag-string current-tags) - 'org-tags-history))))))) + (let ((org-add-colon-after-tag-completion (< 1 (length table))) + (crm-separator"[ ]*:[ ]*")) + (org-trim + (mapconcat #'identity + (completing-read-multiple + "Tags: " + #'org-tags-completion-function + nil nil (org-make-tag-string current-tags) + 'org-tags-history) + ":"))))))) (org-set-tags tags))))) ;; `save-excursion' may not replace the point at the right ;; position. -- 2.17.1
I just noticed org has a test suit *surprise*. With my patch it fails, sorry for that I will look into it and update. Clemens
> I just noticed org has a test suit *surprise*. With my patch it fails,
> sorry for that I will look into it and update.
>
> Clemens
I updated the tests and it's working now. Let me know if anything else
is missing.
Clemens
[-- Attachment #1: Type: text/plain, Size: 50 bytes --] Find the updated patch attached. Clemens [-- Attachment #2: 0001-org.el-Use-completing-read-multiple-for-org-set-tags.patch --] [-- Type: text/x-patch, Size: 6198 bytes --] From 1b50d7450fb23110603792e63c329d7db3115ae8 Mon Sep 17 00:00:00 2001 From: Clemens Radermacher <clemera@posteo.net> Date: Sun, 19 Jul 2020 14:30:37 +0200 Subject: [PATCH] org.el: Use `completing-read-multiple' for `org-set-tags-command' * lisp/org.el (org-set-tags-command): Use `completing-read-multiple' when prompting for tags. * testing/lisp/test-org.el: Update tests to use `completing-read-multiple' too. --- lisp/org.el | 17 +++++++++++------ testing/lisp/test-org.el | 40 ++++++++++++++++++++-------------------- 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/lisp/org.el b/lisp/org.el index 12a853bd6..968883401 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -11819,6 +11819,7 @@ (defun org--align-tags-here (to-col) ;; it now points to BLANK-START. Use COLUMN instead. (if in-blank? (org-move-to-column column) (goto-char origin)))))) +(defvar crm-separator) (defun org-set-tags-command (&optional arg) "Set the tags for the current visible entry. @@ -11877,12 +11878,16 @@ (defun org-set-tags-command (&optional arg) inherited-tags table (and org-fast-tag-selection-include-todo org-todo-key-alist)) - (let ((org-add-colon-after-tag-completion (< 1 (length table)))) - (org-trim (completing-read - "Tags: " - #'org-tags-completion-function - nil nil (org-make-tag-string current-tags) - 'org-tags-history))))))) + (let ((org-add-colon-after-tag-completion (< 1 (length table))) + (crm-separator"[ ]*:[ ]*")) + (org-trim + (mapconcat #'identity + (completing-read-multiple + "Tags: " + #'org-tags-completion-function + nil nil (org-make-tag-string current-tags) + 'org-tags-history) + ":"))))))) (org-set-tags tags))))) ;; `save-excursion' may not replace the point at the right ;; position. diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el index 4f8c74539..32be5ad4a 100644 --- a/testing/lisp/test-org.el +++ b/testing/lisp/test-org.el @@ -6844,8 +6844,8 @@ (should (equal "* H1 :foo:" (org-test-with-temp-text "* H1" - (cl-letf (((symbol-function 'completing-read) - (lambda (&rest args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda (&rest args) '(":foo:")))) (let ((org-use-fast-tag-selection nil) (org-tags-column 1)) (org-set-tags-command))) @@ -6854,8 +6854,8 @@ (should (equal "* H1 :foo:\nContents" (org-test-with-temp-text "* H1\n<point>Contents" - (cl-letf (((symbol-function 'completing-read) - (lambda (&rest args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda (&rest args) '(":foo:")))) (let ((org-use-fast-tag-selection nil) (org-tags-column 1)) (org-set-tags-command))) @@ -6863,8 +6863,8 @@ (should-not (equal "* H1 :foo:\nContents2" (org-test-with-temp-text "* H1\n<point>Contents2" - (cl-letf (((symbol-function 'completing-read) - (lambda (&rest args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda (&rest args) '(":foo:")))) (let ((org-use-fast-tag-selection nil) (org-tags-column 1)) (org-set-tags-command))) @@ -6873,8 +6873,8 @@ (should (equal "* H1 :foo:" (org-test-with-temp-text "* H1" - (cl-letf (((symbol-function 'completing-read) - (lambda (&rest args) ": foo *:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda (&rest args) '(": foo *:")))) (let ((org-use-fast-tag-selection nil) (org-tags-column 1)) (org-set-tags-command))) @@ -6885,8 +6885,8 @@ (should (equal "* H1 :foo:\nContents\n* H2 :foo:" (org-test-with-temp-text "* H1\nContents\n* H2" - (cl-letf (((symbol-function 'completing-read) - (lambda (&rest args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda (&rest args) '(":foo:")))) (let ((org-use-fast-tag-selection nil) (org-loop-over-headlines-in-active-region t) (org-tags-column 1)) @@ -6898,8 +6898,8 @@ (should (equal "* H1\nContents\n* H2 :foo:" (org-test-with-temp-text "* H1\nContents\n* H2" - (cl-letf (((symbol-function 'completing-read) - (lambda (&rest args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda (&rest args) '(":foo:")))) (let ((org-use-fast-tag-selection nil) (org-loop-over-headlines-in-active-region nil) (org-tags-column 1)) @@ -6918,8 +6918,8 @@ (should (equal ":foo:" (org-test-with-temp-text "* <point>" - (cl-letf (((symbol-function 'completing-read) - (lambda (&rest args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda (&rest args) '(":foo:")))) (let ((org-use-fast-tag-selection nil) (org-tags-column 1)) (org-set-tags-command))) @@ -6928,8 +6928,8 @@ (should (equal "* H1 :foo:" (org-test-with-temp-text "* H1" - (cl-letf (((symbol-function 'completing-read) - (lambda (&rest args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda (&rest args) '(":foo:")))) (let ((org-use-fast-tag-selection nil) (org-tags-column 1)) (org-set-tags-command))) @@ -6938,8 +6938,8 @@ (should (equal "* H1 :foo:" (org-test-with-temp-text "*<point>* H1" - (cl-letf (((symbol-function 'completing-read) - (lambda (&rest args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda (&rest args) '(":foo:")))) (let ((org-use-fast-tag-selection nil) (org-tags-column 1)) (org-set-tags-command))) @@ -6948,8 +6948,8 @@ (should (equal " b :foo:" (org-test-with-temp-text "* a<point> b" - (cl-letf (((symbol-function 'completing-read) - (lambda (&rest args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda (&rest args) '(":foo:")))) (let ((org-use-fast-tag-selection nil) (org-tags-column 1)) (org-set-tags-command))) -- 2.17.1
Clemens writes:
> Usage of org-set-tags-command can be improved by using
> completing-read-multiple so you continue to get completion after the
> first tag.
>
> This is my first contribution to org I followed
> https://orgmode.org/worg/org-contribute.html and hope I got everything
> right.
Thanks! You got the process right.
Note, though, that org-set-tags-command already supports completing
multiple tags through org-tags-completion-function, which it passes as
the COLLECTION argument to completing-read. In order to see that in
action, you may need to tell the completion library you use to fall back
to the built-in completing-read. As an example, I use Helm and have
this in my configuration:
(add-to-list 'helm-completing-read-handlers-alist
'(org-set-tags-command))
> Note, though, that org-set-tags-command already supports completing > multiple tags through org-tags-completion-function, which it passes as > the COLLECTION argument to completing-read. In order to see that in > action, you may need to tell the completion library you use to fall back > to the built-in completing-read. As an example, I use Helm and have > this in my configuration: > > (add-to-list 'helm-completing-read-handlers-alist > '(org-set-tags-command)) > I looked and helm-org does include a similar adjustment to make it work correctly: https://github.com/emacs-helm/helm-org/blob/b7a18dfc17e8b933956d61d68c435eee03a96c24/helm-org.el#L490-L523 My patch aims to get you completion with the default completion and also for any framework that complies to it out of the box. Without my patch (and without helm-org) you don't get completion after the first tag I think.
Clemens writes:
> My patch aims to get you completion with the default completion and also
> for any framework that complies to it out of the box. Without my patch
> (and without helm-org) you don't get completion after the first tag I think.
With the built-in completion, org-set-tags-command already supports
completing multiple tags. So that aim reduces to using
completing-read-multiple because other completion libraries are more
likely to play nicely with crm.
At the moment that's not mentioned in the commit message (and, less
importantly, it wasn't mentioned in the message introducing the patch),
but in my view that aim should be the emphasis of the commit message.
It'd be good to note the popular completion libraries that don't work
out of the box with the current approach, and whether they do after this
patch.
It's also probably worth mentioning why org-tags-completion-function is
still passed as the completion function to completing-read-multiple, as
the completion function's main purpose of completing multiple tags could
now be fulfilled by completing-read-multiple alone. And what about the
other spots that use org-tags-completion-function?
Thanks.
> With the built-in completion, org-set-tags-command already supports > completing multiple tags. Interesting, I couldn't figure out how. I tried with emacs -Q (v 26.3) and I get only completion for the first tag. If you try to get completion after entering the first you get an [no match] message. Maybe I'm doing something wrong? How do you get completion after the first tag with default completion? > So that aim reduces to using > completing-read-multiple because other completion libraries are more > likely to play nicely with crm. > > At the moment that's not mentioned in the commit message (and, less > importantly, it wasn't mentioned in the message introducing the patch), > but in my view that aim should be the emphasis of the commit message. > It'd be good to note the popular completion libraries that don't work > out of the box with the current approach, and whether they do after this > patch. As described above my initial aim was really to get completion after entering the first tag (that is mentioned in my message introducing the patch but it is a good idea to mention it also in the commit). Completion frameworks automatically benefit from this if they support `completing-read-multiple`. > It's also probably worth mentioning why org-tags-completion-function is > still passed as the completion function to completing-read-multiple, as > the completion function's main purpose of completing multiple tags could > now be fulfilled by completing-read-multiple alone. And what about the > other spots that use org-tags-completion-function? I haven't looked at `org-tags-completion-function` in detail will have to check if something could be adjusted there, too. I thought usages in other spots wouldn't be affected and should automatically work with the new enhanced behavior. I will look into that, had you something specific in mind? Thanks for your review and help!
Sorry, I just figured out you get indeed completion after the first tag my sandbox wasn't clean (usually that shouldn't affect the completion behavior but somehow it did, I have to check why). In this case your are also right that `org-tags-completion-function` need to be adjusted. My patch still is useful to make frameworks which support `completing-read-multiple` automatically work though while the current approach is tightly integrated with the default completion. I will do some more work and report back.
Now that I know that completion works for multiple tags with default completion I'm unsure if it is worth to proceed with this. I wondered how this would go unnoticed for such a long time in org and now I know that the failure was on my part. On the other hand switching to `completing-read-multiple` would improve the situation for completion frameworks which handle it correctly and would also be more idiomatic for this use case.
Clemens writes:
> Now that I know that completion works for multiple tags with default
> completion I'm unsure if it is worth to proceed with this. I wondered
> how this would go unnoticed for such a long time in org and now I know
> that the failure was on my part. On the other hand switching to
> `completing-read-multiple` would improve the situation for completion
> frameworks which handle it correctly and would also be more idiomatic
> for this use case.
Yeah, I'm also undecided. I'm open to the idea of moving from
org-tags-completion-function to crm for the reason you give, but, with
any proposal for that, I think it'd be important to see an inspection of
the current call sites (only a handful) and an analysis of whether we
can retain the current behavior.
Then again, this isn't the only place that passes a function for
completing-read's COLLECTION argument: there's also
org-agenda-filter-completion-function. At a glance I think it would be
harder to replace with crm.
But perhaps we could at least avoid some confusion by explicitly
documenting spots where completion frameworks may interact with the
intended completion functionality.