Re: Bug: [PATCH] Use crm for tag completion [9.4.6 (9.4.6-gab9f2a @ /home/ionasal/.emacs.d/elpa/org-9.4.6/)]

2021-07-08 Thread Nicolas Goaziou
Hello,

Allen Li  writes:

> * lisp/org-capture.el (org-capture-fill-template): Changed to use
> completing-read-multiple.
> * lisp/org.el (org-set-tags-command): Changed to use
> completing-read-multiple.
> (org-change-tag-in-region): Changed to use a simple completion table.
> * testing/lisp/test-org.el (test-org/set-tags-command): Fixed tests.
> * etc/ORG-NEWS (Tag completion now uses =completing-read-multiple=):
> Added news.

Applied. Thank you.

I just changed string-join into to good ole mapconcat as the code base
does not use subr-x.el

Regards,
-- 
Nicolas Goaziou



Bug: [PATCH] Use crm for tag completion [9.4.6 (9.4.6-gab9f2a @ /home/ionasal/.emacs.d/elpa/org-9.4.6/)]

2021-07-08 Thread Allen Li
Org mode's tag completion commands all use a custom completion function,
which makes it difficult for alternative completion functions to support
well.

Emacs already has a function for reading multiple things,
completing-read-multiple, which can be used for the tag completion use
case.

I have attached a patch for this change, which I have tested manually a
few times and also fixed the existing tests.  I have tested this with
vertico, which claims to strictly follow Emacs's completion API.

>From 933dc914694c14889af86c06ba0a8bbd88a316cf Mon Sep 17 00:00:00 2001
From: Allen Li 
Date: Thu, 8 Jul 2021 21:35:34 -0700
Subject: [PATCH] org: Use crm for completing tags

Change various places which use `completing-read' to read tags using a
custom completion function to instead use `completing-read-multiple'
with a completion table instead.

This makes tab completion play better with alternative completion
frameworks such as vertico, selectrum, etc.

`org-change-tag-in-region' only reads a single tag, so it is changed
to use a completion table with `completing-read'.  This also makes it
play better with alternative completion frameworks.

Note that there is still one use for `org-tags-completion-function',
which is for completing tag matches.  Completing tag matches is
different from completing lists of tags since the separators (+, -,
etc) have semantic meaning.  This commit does not address that use
case.

* lisp/org-capture.el (org-capture-fill-template): Changed to use
completing-read-multiple.
* lisp/org.el (org-set-tags-command): Changed to use
completing-read-multiple.
(org-change-tag-in-region): Changed to use a simple completion table.
* testing/lisp/test-org.el (test-org/set-tags-command): Fixed tests.
* etc/ORG-NEWS (Tag completion now uses =completing-read-multiple=):
Added news.
---
 etc/ORG-NEWS |  6 +
 lisp/org-capture.el  | 12 +-
 lisp/org.el  | 18 +--
 testing/lisp/test-org.el | 50 
 4 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 3f3971961..719ac3547 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -409,6 +409,12 @@ The function does not allow for a third optional parameter anymore.
 If a babel src block produces a raw LaTeX environment, it will now be
 recognised as a result, and so replaced when re-evaluated.
 
+*** Tag completion now uses =completing-read-multiple=
+
+Tag completion now uses =completing-read-multiple= with a simple
+completion table, which should allow better interoperability with
+custom completion functions.
+
 * Version 9.4
 ** Incompatible changes
 *** Possibly broken internal file links: please check and fix
diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 5ecec6309..c51744680 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -69,6 +69,7 @@
 (declare-function org-table-goto-line "org-table" (N))
 
 (defvar dired-buffers)
+(defvar crm-separator)
 (defvar org-end-time-was-given)
 (defvar org-keyword-properties)
 (defvar org-remember-default-headline)
@@ -1739,12 +1740,11 @@ The template may still contain \"%?\" for cursor positioning."
 			(org-add-colon-after-tag-completion t)
 			(ins (mapconcat
   #'identity
-  (org-split-string
-   (completing-read
-(if prompt (concat prompt ": ") "Tags: ")
-'org-tags-completion-function nil nil nil
-'org-tags-history)
-   "[^[:alnum:]_@#%]+")
+  (let ((crm-separator "[ \t]*:[ \t]*"))
+(completing-read-multiple
+ (if prompt (concat prompt ": ") "Tags: ")
+ org-last-tags-completion-table nil nil nil
+ 'org-tags-history))
   ":")))
 		   (when (org-string-nw-p ins)
 			 (unless (eq (char-before) ?:) (insert ":"))
diff --git a/lisp/org.el b/lisp/org.el
index 4fd8b6fa6..ed3ee3a1c 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -201,6 +201,8 @@ Stars are put in group 1 and the trimmed body in group 2.")
 ;; load languages based on value of `org-babel-load-languages'
 (defvar org-babel-load-languages)
 
+(defvar crm-separator)  ; dynamically scoped param
+
 ;;;###autoload
 (defun org-babel-do-load-languages (sym value)
   "Load the languages defined in `org-babel-load-languages'."
@@ -12054,12 +12056,14 @@ 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
-		 (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 "[ \t]*:[ \t]*"))
+		 (string-join (completing-read-multiple
+			   "Tags: "
+			   org-last-tags-completion-table
+			   nil nil (org-make-tag-