GNU bug report logs - #74865
[PATCH] Use `completion-table-with-metadata'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 74865 in the body.
You can then email your comments to 74865 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: bug-gnu-emacs <at> gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, Juri Linkov <juri <at> linkov.net>
Subject: [PATCH] Use `completion-table-with-metadata'
Date: Sat, 14 Dec 2024 13:57:11 +0100

[Message part 1 (text/plain, inline)]

Tags: patch

This is a small follow-up of bug#74616.  There I have introduced the
`completion-table-with-metadata' helper function, which we can now take
advantage of at more places.  By using `completion-table-with-metadata'
we avoid problems with completion sessions in recursive minibuffers.

Daniel

[0001-Use-completion-table-with-metadata.patch (text/patch, attachment)]

Message #8 received at 74865 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Daniel Mendler <mail <at> daniel-mendler.de>
Cc: 74865 <at> debbugs.gnu.org
Subject: bug#74865: [PATCH] Use `completion-table-with-metadata'
Date: Mon, 16 Dec 2024 20:20:10 +0200

> This is a small follow-up of bug#74616.  There I have introduced the
> `completion-table-with-metadata' helper function, which we can now take
> advantage of at more places.  By using `completion-table-with-metadata'
> we avoid problems with completion sessions in recursive minibuffers.

Thanks for the patch, now pushed.

`completion-table-with-metadata' is a very useful function.
I already used it for the new command 'comint-complete-input-ring'
in bug#74694.

Also it could be used in `read-char-by-name' as well.

Maybe `completion-table-with-metadata' should be announced
in the "Lisp Changes" section of etc/NEWS?

Message #11 received at 74865 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: 74865 <at> debbugs.gnu.org
Subject: Re: bug#74865: [PATCH] Use `completion-table-with-metadata'
Date: Mon, 16 Dec 2024 19:29:04 +0100

Juri Linkov <juri <at> linkov.net> writes:

>> This is a small follow-up of bug#74616.  There I have introduced the
>> `completion-table-with-metadata' helper function, which we can now take
>> advantage of at more places.  By using `completion-table-with-metadata'
>> we avoid problems with completion sessions in recursive minibuffers.
>
> Thanks for the patch, now pushed.

Thanks!

> `completion-table-with-metadata' is a very useful function.
> I already used it for the new command 'comint-complete-input-ring'
> in bug#74694.
>
> Also it could be used in `read-char-by-name' as well.

Yes, there are certainly more use cases. I didn't use it for
`read-char-by-name' since this would change the caching behavior
slightly. The character names would be loaded a little bit earlier,
which would slow down the initialization of `read-char-by-name'.

My patch only changed the call sites which are exactly equivalent with
and without the patch.

> Maybe `completion-table-with-metadata' should be announced
> in the "Lisp Changes" section of etc/NEWS?

Yes, please go ahead adding it to the NEWS. I will backport the function
also via Compat.

Daniel

Message #16 received at 74865-done <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: 74865-done <at> debbugs.gnu.org
Subject: Re: bug#74865: [PATCH] Use `completion-table-with-metadata'
Date: Mon, 16 Dec 2024 20:04:09 +0100

Closing.

Message #19 received at 74865 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Daniel Mendler <mail <at> daniel-mendler.de>
Cc: 74865 <at> debbugs.gnu.org
Subject: Re: bug#74865: [PATCH] Use `completion-table-with-metadata'
Date: Mon, 16 Dec 2024 21:13:25 +0200

>> Also it could be used in `read-char-by-name' as well.
>
> Yes, there are certainly more use cases. I didn't use it for
> `read-char-by-name' since this would change the caching behavior
> slightly. The character names would be loaded a little bit earlier,
> which would slow down the initialization of `read-char-by-name'.

Maybe the caching behavior could be preserved by turning
`completion-table-with-metadata' into a macro?

>> Maybe `completion-table-with-metadata' should be announced
>> in the "Lisp Changes" section of etc/NEWS?
>
> Yes, please go ahead adding it to the NEWS. I will backport the function
> also via Compat.

Ok, now added to NEWS.

Message #22 received at 74865 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: 74865 <at> debbugs.gnu.org
Subject: Re: bug#74865: [PATCH] Use `completion-table-with-metadata'
Date: Mon, 16 Dec 2024 20:26:45 +0100

Juri Linkov <juri <at> linkov.net> writes:

>>> Also it could be used in `read-char-by-name' as well.
>>
>> Yes, there are certainly more use cases. I didn't use it for
>> `read-char-by-name' since this would change the caching behavior
>> slightly. The character names would be loaded a little bit earlier,
>> which would slow down the initialization of `read-char-by-name'.
>
> Maybe the caching behavior could be preserved by turning
> `completion-table-with-metadata' into a macro?

Let us please keep `completion-table-with-metadata' a function for
composeability and ease of reasoning. `completion-table-with-metadata'
follows the many already existing `completion-table-with-*' functions in
minibuffer.el.

In order to delay initialization of the completion candidates we can use
the following code:

(completion-table-with-metadata
 (lambda (string pred action)
   (complete-with-action action (ucs-names) string pred))
 metadata)

(completion-table-with-metadata
 (completion-table-dynamic (lambda (_) (ucs-names)))
 metadata)

>>> Maybe `completion-table-with-metadata' should be announced
>>> in the "Lisp Changes" section of etc/NEWS?
>>
>> Yes, please go ahead adding it to the NEWS. I will backport the function
>> also via Compat.
>
> Ok, now added to NEWS.

Thanks.

Daniel

Message #25 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Daniel Mendler <mail <at> daniel-mendler.de>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: bug-gnu-emacs <at> gnu.org, juri <at> linkov.net
Subject: Re: [PATCH] Use `completion-table-with-metadata'
Date: Sat, 28 Dec 2024 13:27:15 +0200

> From: Daniel Mendler <mail <at> daniel-mendler.de>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, Juri Linkov <juri <at> linkov.net>
> Date: Sat, 14 Dec 2024 13:57:11 +0100
> 
> This is a small follow-up of bug#74616.  There I have introduced the
> `completion-table-with-metadata' helper function, which we can now take
> advantage of at more places.  By using `completion-table-with-metadata'
> we avoid problems with completion sessions in recursive minibuffers.

I'd ask Stefan (CC'ed) to review this, since I don't feel I know
enough about this to have a useful opinion.

Message #28 received at 74865 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Daniel Mendler <mail <at> daniel-mendler.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Juri Linkov <juri <at> linkov.net>,
 74865 <at> debbugs.gnu.org
Subject: Re: bug#74865: [PATCH] Use `completion-table-with-metadata'
Date: Thu, 02 Jan 2025 10:46:26 -0500

I like this patch, thank you: `completion-extra-properties' should
ideally never be let-bound but should be set buffer-locally instead
(e.g. via `minibuffer-with-setup-hook`).

> +(defun proced--read-signal (count)
> +  "Read a SIGNAL via `completing-read' for COUNT processes."
> +  (completing-read
> +   (format-prompt "Send signal [%s]"
> +                  "TERM"
> +                  (if (= 1 count)
> +                      "1 process"
> +                    (format "%d processes" count)))
> +   (completion-table-with-metadata
> +    (completion-table-case-fold proced-signal-list)
> +    `((annotation-function
> +       . ,(lambda (s) (cdr (assoc s proced-signal-list))))))
> +   nil nil nil nil "TERM"))

[ FWIW, I'd pass `prompt` rather than `count` as argument.  ]


        Stefan

Message #37 received at 74865 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Daniel Mendler <mail <at> daniel-mendler.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74865 <at> debbugs.gnu.org
Subject: Re: bug#74865: [PATCH] Use `completion-table-with-metadata'
Date: Mon, 03 Feb 2025 09:50:46 +0200

This change removed affixation-function from another if-branch in 'read-face-name'
for the case when 'multiple' is t that uses completing-read-multiple:

> diff --git a/lisp/faces.el b/lisp/faces.el
> index f8ec0f1a187..05df685c679 100644
> --- a/lisp/faces.el
> +++ b/lisp/faces.el
> @@ -1147,17 +1147,6 @@ read-face-name
>      (let ((prompt (if default
>                        (format-prompt prompt default)
>                      (format "%s: " prompt)))
> -          (completion-extra-properties
> -           `(:affixation-function
> -             ,(lambda (faces)
> -                (mapcar
> -                 (lambda (face)
> -                   (list face
> -                         (concat (propertize read-face-name-sample-text
> -                                             'face face)
> -                                 "\t")
> -                         ""))
> -                 faces))))
>            aliasfaces nonaliasfaces faces)
>        ;; Build up the completion tables.
>        (mapatoms (lambda (s)
> @@ -1180,7 +1169,18 @@ read-face-name
>              (nreverse faces))
>          (let ((face (completing-read
>                       prompt
> -                     (completion-table-in-turn nonaliasfaces aliasfaces)
> +                     (completion-table-with-metadata
> +                      (completion-table-in-turn nonaliasfaces aliasfaces)
> +                      `((affixation-function
> +                        . ,(lambda (faces)
> +                             (mapcar
> +                              (lambda (face)
> +                                (list face
> +                                      (concat (propertize read-face-name-sample-text
> +                                                          'face face)
> +                                              "\t")
> +                                      ""))
> +                              faces)))))
>                       nil t nil 'face-name-history defaults)))
>            (when (facep face) (if (stringp face)
>                                   (intern face)

Message #40 received at 74865 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74865 <at> debbugs.gnu.org
Subject: Re: bug#74865: [PATCH] Use `completion-table-with-metadata'
Date: Mon, 03 Feb 2025 09:59:28 +0100

[Message part 1 (text/plain, inline)]

Juri Linkov <juri <at> linkov.net> writes:

> This change removed affixation-function from another if-branch in 'read-face-name'
> for the case when 'multiple' is t that uses completing-read-multiple:

Hello Juri,

I have attached a patch to this mail which corrects this. Please give it
a test. Thank you!

Daniel

[0001-read-face-name-Build-common-completion-table-for-CR-.patch (text/x-diff, attachment)]

Message #43 received at 74865 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Daniel Mendler <mail <at> daniel-mendler.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74865 <at> debbugs.gnu.org
Subject: Re: bug#74865: [PATCH] Use `completion-table-with-metadata'
Date: Mon, 03 Feb 2025 20:33:28 +0200

close 74865 31.0.50
thanks

>> This change removed affixation-function from another if-branch in 'read-face-name'
>> for the case when 'multiple' is t that uses completing-read-multiple:
>
> I have attached a patch to this mail which corrects this. Please give it
> a test. Thank you!

Thanks, I confirm everything works correctly, so now pushed.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.