Re: [PATCH] Use completing-read-multiple for org-set-tags-command
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.
Re: [PATCH] Use completing-read-multiple for org-set-tags-command
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.
Re: [PATCH] Use completing-read-multiple for org-set-tags-command
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.
Re: [PATCH] Use completing-read-multiple for org-set-tags-command
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!
Re: [PATCH] Use completing-read-multiple for org-set-tags-command
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.
Re: [PATCH] Use completing-read-multiple for 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.
Re: [PATCH] Use completing-read-multiple for org-set-tags-command
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))
Re: [PATCH] Use completing-read-multiple for org-set-tags-command
Find the updated patch attached. Clemens >From 1b50d7450fb23110603792e63c329d7db3115ae8 Mon Sep 17 00:00:00 2001 From: Clemens Radermacher 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 ( arg) "Set the tags for the current visible entry. @@ -11877,12 +11878,16 @@ (defun org-set-tags-command ( 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 ( args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda ( 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\nContents" - (cl-letf (((symbol-function 'completing-read) - (lambda ( args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda ( 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\nContents2" - (cl-letf (((symbol-function 'completing-read) - (lambda ( args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda ( 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 ( args) ": foo *:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda ( 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 ( args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda ( 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 ( args) ":foo:"))) + (cl-letf (((symbol-function 'completing-read-multiple) + (lambda ( 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 "* " - (cl-letf (((symbol-function 'completing-read) - (lambda
Re: [PATCH] Use completing-read-multiple for org-set-tags-command
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
Re: [PATCH] Use completing-read-multiple for org-set-tags-command
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
[PATCH] Use completing-read-multiple for org-set-tags-command
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 >From c8be9106110f266db774d73af4dcb6fbcef3bef8 Mon Sep 17 00:00:00 2001 From: Clemens Radermacher 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 ( 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