[PATCH] Use completing-read-multiple for org-set-tags-command

[PATCH] Use completing-read-multiple for org-set-tags-command

[Clemens]

[-- 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

Re: [PATCH] Use completing-read-multiple for org-set-tags-command

[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

Re: [PATCH] Use completing-read-multiple for org-set-tags-command

[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

Re: [PATCH] Use completing-read-multiple for org-set-tags-command

[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

Re: [PATCH] Use completing-read-multiple for org-set-tags-command

[Kyle Meyer]

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

[Clemens]

 > 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

[Kyle Meyer]

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

[Clemens]

> 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]

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

[Clemens]

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

[Kyle Meyer]

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.