GNU bug report logs - #45780
28.0.50; [PATCH] Face used for affixation function annotations

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 45780 in the body.
You can then email your comments to 45780 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: Clemens <clemera <at> posteo.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.0.50; [PATCH] Face used for affixation function annotations
Date: Mon, 11 Jan 2021 13:38:13 +0100

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

As per the comment above the affected code, the client can specify the 
face when prefix and suffix are provided. The prefix is already checked 
earlier and what remained was checking the suffix not the prefix.

There is another thing I would like to bring up in this context: When 
the annotations returned by annotation/affixation functions already 
specify a face I think it would be nicer if the completion-annotations 
face wouldn't be applied generally. In Selectrum we use something like:

(if (text-property-not-all 0 (length str) 'face nil str)
    str
  (propertize str 'face 'completions-annotations))


This gives the client full control over the visual appearance if that is 
preferred. Maybe this approach could also make sense to be included in 
Emacs? Currently for the annotation function the completions-annotations 
face is always applied and for the affixation function it also still 
gets applied when the affixation function returns a two candidate list 
(like read-extended-command--affixation on current master). The case of 
also allowing a two candidate list to be returned by affixation 
functions is also currently undocumented.


    Clemens

[affix.patch (text/x-patch, attachment)]

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

From: Juri Linkov <juri <at> linkov.net>
To: Clemens <clemera <at> posteo.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Mon, 11 Jan 2021 20:38:55 +0200

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

Hi Clemens,

> As per the comment above the affected code, the client can specify the face
> when prefix and suffix are provided. The prefix is already checked earlier
> and what remained was checking the suffix not the prefix.

Shouldn't then this code with font-lock-prepend-text-property
be removed completely?  Since both prefix and suffix are non-nil,
this makes code no-op.

> There is another thing I would like to bring up in this context: When the
> annotations returned by annotation/affixation functions already specify
> a face I think it would be nicer if the completion-annotations face
> wouldn't be applied generally. In Selectrum we use something like:
>
> (if (text-property-not-all 0 (length str) 'face nil str)
>     str
>   (propertize str 'face 'completions-annotations))

So you propose to search for the face text-property in the provided string
to decide whether to add the default face in completion--insert-strings?

> This gives the client full control over the visual appearance if that is
> preferred. Maybe this approach could also make sense to be included in
> Emacs?

Do you see any possible backward-compatibility issues with changing this in
Emacs?  For example, when a package like Selectrum puts another face
on the completion string, then it will be displayed instead of the default
completion-annotations face.

> Currently for the annotation function the completions-annotations
> face is always applied and for the affixation function it also still gets
> applied when the affixation function returns a two candidate list (like
> read-extended-command--affixation on current master).  The case of also
> allowing a two candidate list to be returned by affixation functions is
> also currently undocumented.

Thanks for noticing the documentation problem.  Do you think
this fix is sufficient:

[affix-doc.patch (text/x-diff, inline)]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 3eca9d066f..227966020c 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -122,7 +122,8 @@ completion-metadata
    returns a string to append to STRING.
 - `affixation-function': function to prepend/append a prefix/suffix to
    entries.  Takes one argument (COMPLETIONS) and should return a list
-   of completions with a list of three elements: completion, its prefix
+   of completions with a list of either two elements: completion
+   and suffix, or three elements: completion, its prefix
    and suffix.  This function takes priority over `annotation-function'
    when both are provided, so only this function is used.
 - `display-sort-function': function to sort entries in *Completions*.
@@ -1941,6 +1942,7 @@ completion-extra-properties
 `:affixation-function': Function to prepend/append a prefix/suffix to
    completions.  The function must accept one argument, a list of
    completions, and return a list where each element is a list of
+   either two elements: a completion, and a suffix, or
    three elements: a completion, a prefix and a suffix.
    This function takes priority over `:annotation-function'
    when both are provided, so only this function is used.

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

From: Clemens <clemera <at> posteo.net>
To: Juri Linkov <juri <at> linkov.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Mon, 11 Jan 2021 21:07:57 +0100

>> As per the comment above the affected code, the client can specify the face
>> when prefix and suffix are provided. The prefix is already checked earlier
>> and what remained was checking the suffix not the prefix.
> 
> Shouldn't then this code with font-lock-prepend-text-property
> be removed completely?  Since both prefix and suffix are non-nil,
> this makes code no-op.

You are right I assumed the suffix can also be nil but looking at the 
let binding earlier in the code this can't be the case when there is a 
prefix which is derived from the fact that there is a suffix annotation 
in the first place :)

>> There is another thing I would like to bring up in this context: When the
>> annotations returned by annotation/affixation functions already specify
>> a face I think it would be nicer if the completion-annotations face
>> wouldn't be applied generally. In Selectrum we use something like:
>>
>> (if (text-property-not-all 0 (length str) 'face nil str)
>>      str
>>    (propertize str 'face 'completions-annotations))
> 
> So you propose to search for the face text-property in the provided string
> to decide whether to add the default face in completion--insert-strings?

Yes, the strings of the prefix/suffix.

>> This gives the client full control over the visual appearance if that is
>> preferred. Maybe this approach could also make sense to be included in
>> Emacs?
> 
> Do you see any possible backward-compatibility issues with changing this in
> Emacs?  For example, when a package like Selectrum puts another face
> on the completion string, then it will be displayed instead of the default
> completion-annotations face.

We already do this for annotations/affixations in Selectrum but only 
based on the face of the annotation/affixation itself, the completion 
string doesn't affect this. I hope this wouldn't have any visual 
downsides for old code which assumes the faces get merged but I haven't 
encountered any cases where code tried to apply custom faces to 
annotations besides the marginalia package. Letting the client control 
it makes it easier to configure the display as it's hard to predict what 
will come out of face merging with the face the user has configured as 
`completion-annotations` face. This new behaviour could also only be 
applied for affixation functions to avoid any possibly bad effects of 
existing code.

> Thanks for noticing the documentation problem.  Do you think
> this fix is sufficient:

Looks good to me, too. Thank you!

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

From: Juri Linkov <juri <at> linkov.net>
To: Clemens <clemera <at> posteo.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Tue, 12 Jan 2021 20:30:54 +0200

>>> This gives the client full control over the visual appearance if that is
>>> preferred. Maybe this approach could also make sense to be included in
>>> Emacs?
>> Do you see any possible backward-compatibility issues with changing this
>> in
>> Emacs?  For example, when a package like Selectrum puts another face
>> on the completion string, then it will be displayed instead of the default
>> completion-annotations face.
>
> We already do this for annotations/affixations in Selectrum but only based
> on the face of the annotation/affixation itself, the completion string
> doesn't affect this. I hope this wouldn't have any visual downsides for old
> code which assumes the faces get merged but I haven't encountered any cases
> where code tried to apply custom faces to annotations besides the
> marginalia package. Letting the client control it makes it easier to
> configure the display as it's hard to predict what will come out of face
> merging with the face the user has configured as `completion-annotations`
> face. This new behaviour could also only be applied for affixation
> functions to avoid any possibly bad effects of existing code.

It seems the current logic already supports overriding faces for
completion strings:

1. when only annotation suffix string is provided, then the face
   completion-annotations is added;

2. when both prefix and suffix are provided, then the client decides
   what face to add.  Also it's possible to provide an empty prefix
   string to be able to specify a custom face for the suffix string.

So when the client wants to override the default annotation face,
this is already easy to do using something like (this is not a patch
to commit, but just demonstration of current abilities):

diff --git a/lisp/simple.el b/lisp/simple.el
index 4896a282ec..ca308d0bb6 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1969,7 +1969,7 @@ read-extended-command--affixation
                              (format " (%s)" (car obsolete)))
                             ((and binding (not (stringp binding)))
                              (format " (%s)" (key-description binding))))))
-         (if suffix (list command-name suffix) command-name)))
+         (if suffix (list command-name "" (propertize suffix 'face 'shadow)) command-name)))
      command-names)))

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

From: Clemens <clemera <at> posteo.net>
To: Juri Linkov <juri <at> linkov.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Wed, 13 Jan 2021 19:06:38 +0100

> 1. when only annotation suffix string is provided, then the face
>     completion-annotations is added;
> 
> 2. when both prefix and suffix are provided, then the client decides
>     what face to add.  Also it's possible to provide an empty prefix
>     string to be able to specify a custom face for the suffix string.
> 
> So when the client wants to override the default annotation face,
> this is already easy to do using something like (this is not a patch
> to commit, but just demonstration of current abilities):

I would prefer the more automatic behaviour I proposed as having 
completion-annotations face is nice when the client has not thought 
about it but when the client has provided a string with a face it is 
likely the client wants exactly that face and not a combination with 
completion-annotations face. Basing the decision on a provided prefix 
seems a bit arbitrary and one would need to figure this out by looking 
at the code.

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

From: Juri Linkov <juri <at> linkov.net>
To: Clemens <clemera <at> posteo.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Thu, 14 Jan 2021 11:00:16 +0200

>> 1. when only annotation suffix string is provided, then the face
>>     completion-annotations is added;
>> 2. when both prefix and suffix are provided, then the client decides
>>     what face to add.  Also it's possible to provide an empty prefix
>>     string to be able to specify a custom face for the suffix string.
>> So when the client wants to override the default annotation face,
>> this is already easy to do using something like (this is not a patch
>> to commit, but just demonstration of current abilities):
>
> I would prefer the more automatic behaviour I proposed as having
> completion-annotations face is nice when the client has not thought about
> it but when the client has provided a string with a face it is likely the
> client wants exactly that face and not a combination with
> completion-annotations face. Basing the decision on a provided prefix seems
> a bit arbitrary and one would need to figure this out by looking at
> the code.

Do you want to use the completion-annotations face conditionally only
for annotations, i.e. when only the suffix is provided by the client?
Because when a prefix is provided as well, then it's not an annotation
anymore, so the completion-annotations face is not applicable to prefixes.

Doing this is not something new, we already have the same logic
in minibuffer-message:

    (unless (or (null minibuffer-message-properties)
                ;; Don't overwrite the face properties the caller has set
                (text-properties-at 0 message))
      (setq message (apply #'propertize message minibuffer-message-properties)))

Is this logic suitable for completion-annotations?

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

From: Clemens <clemera <at> posteo.net>
To: Juri Linkov <juri <at> linkov.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Thu, 14 Jan 2021 18:21:00 +0100

> Do you want to use the completion-annotations face conditionally only
> for annotations, i.e. when only the suffix is provided by the client?
> Because when a prefix is provided as well, then it's not an annotation
> anymore, so the completion-annotations face is not applicable to prefixes.


I see, personally I think of all strings besides the completions 
themselves as annotations ;) Makes sense to do it only for the suffix then.

> Doing this is not something new, we already have the same logic
> in minibuffer-message:
> 
>      (unless (or (null minibuffer-message-properties)
>                  ;; Don't overwrite the face properties the caller has set
>                  (text-properties-at 0 message))
>        (setq message (apply #'propertize message minibuffer-message-properties)))
> 
> Is this logic suitable for completion-annotations?

I guess this could also be used, the version I posted earlier only 
checks for the face property and then also check the whole string:

(if (text-property-not-all 0 (length str) 'face nil str)
        str
      (propertize str 'face face))


When only the face matters my proposed version might be better?

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

From: Juri Linkov <juri <at> linkov.net>
To: Clemens <clemera <at> posteo.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Thu, 14 Jan 2021 20:59:29 +0200

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

>> Do you want to use the completion-annotations face conditionally only
>> for annotations, i.e. when only the suffix is provided by the client?
>> Because when a prefix is provided as well, then it's not an annotation
>> anymore, so the completion-annotations face is not applicable to prefixes.
>
> I see, personally I think of all strings besides the completions themselves
> as annotations ;) Makes sense to do it only for the suffix then.
>
>> Doing this is not something new, we already have the same logic
>> in minibuffer-message:
>>      (unless (or (null minibuffer-message-properties)
>>                  ;; Don't overwrite the face properties the caller has set
>>                  (text-properties-at 0 message))
>>        (setq message (apply #'propertize message minibuffer-message-properties)))
>> Is this logic suitable for completion-annotations?
>
> I guess this could also be used, the version I posted earlier only checks
> for the face property and then also check the whole string:
>
> (if (text-property-not-all 0 (length str) 'face nil str)
>         str
>       (propertize str 'face face))
>
> When only the face matters my proposed version might be better?

I agree its purpose is quite different from the example above.

Then maybe something like this should do what you want:

[completion--insert-strings-text-property-not-all.patch (text/x-diff, inline)]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 315f2d369a..31d7be3441 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1785,14 +1800,7 @@ completion--insert-strings
                 (when prefix
                   (let ((beg (point))
                         (end (progn (insert prefix) (point))))
-                    (put-text-property beg end 'mouse-face nil)
-                    ;; When both prefix and suffix are added
-                    ;; by the caller via affixation-function,
-                    ;; then allow the caller to decide
-                    ;; what faces to put on prefix and suffix.
-                    (unless prefix
-                      (font-lock-prepend-text-property
-                       beg end 'face 'completions-annotations))))
+                    (put-text-property beg end 'mouse-face nil)))
                 (put-text-property (point) (progn (insert (car str)) (point))
                                    'mouse-face 'highlight)
                 (let ((beg (point))
@@ -1800,7 +1808,12 @@ completion--insert-strings
                   (put-text-property beg end 'mouse-face nil)
                   ;; Put the predefined face only when suffix
                   ;; is added via annotation-function.
-                  (unless prefix
+                  ;; Otherwise, when only suffix is added
+                  ;; by the caller via annotation-function,
+                  ;; then allow the caller to decide
+                  ;; what faces to put on suffix.
+                  (unless (or prefix (text-property-not-all
+                                      0 (length suffix) 'face nil suffix))
                     (font-lock-prepend-text-property
                      beg end 'face 'completions-annotations)))))
 	    (cond

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

From: Clemens <clemera <at> posteo.net>
To: Juri Linkov <juri <at> linkov.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Thu, 14 Jan 2021 20:43:19 +0100

> I agree its purpose is quite different from the example above.
> 
> Then maybe something like this should do what you want:

Yes, that would be nice if you also think it would be okay to change it 
this way, thank you!

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

From: Juri Linkov <juri <at> linkov.net>
To: Clemens <clemera <at> posteo.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Mon, 25 Jan 2021 20:02:51 +0200

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

>> I agree its purpose is quite different from the example above.
>> Then maybe something like this should do what you want:
>
> Yes, that would be nice if you also think it would be okay to change it
> this way, thank you!

To make sure that everything is right, here is a brief table
for coming changes, where overriden-face is a face specified
by the client:

#+begin_quote
| prefix | suffix | overriden-face | result face             |
|--------+--------+----------------+-------------------------+
|    N   |    Y   |       N        | completions-annotations on suffix
|    Y   |    N   |       N        | no face
|    Y   |    Y   |       N        | no face
|    N   |    Y   |       Y        | overriden-face on suffix
|    Y   |    N   |       Y        | overriden-face on prefix
|    Y   |    Y   |       Y        | overriden-face on prefix and suffix
#+end_quote

Or maybe better to represent this as a test:

[completion--insert-strings-faces.patch (text/x-diff, inline)]

diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 3ebca14a28..7349b191ca 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -1,4 +1,4 @@
-;;; completion-tests.el --- Tests for completion functions  -*- lexical-binding: t; -*-
+;;; minibuffer-tests.el --- Tests for completion functions  -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 2013-2021 Free Software Foundation, Inc.
 
@@ -107,5 +107,23 @@ completion-table-test-quoting
                                                 nil (length input))
                      (cons output (length output)))))))
 
-(provide 'completion-tests)
-;;; completion-tests.el ends here
+(ert-deftest completion--insert-strings-faces ()
+  (with-temp-buffer
+    (completion--insert-strings
+     '(("completion1" "suffix1")))
+    (should (equal (get-text-property 12 'face) '(completions-annotations))))
+  (with-temp-buffer
+    (completion--insert-strings
+     '(("completion1" #("suffix1" 0 7 (face shadow)))))
+    (should (equal (get-text-property 12 'face) 'shadow)))
+  (with-temp-buffer
+    (completion--insert-strings
+     '(("completion1" "prefix1" "suffix1")))
+    (should (equal (get-text-property 19 'face) nil)))
+  (with-temp-buffer
+    (completion--insert-strings
+     '(("completion1" "prefix1" #("suffix1" 0 7 (face shadow)))))
+    (should (equal (get-text-property 19 'face) 'shadow))))
+
+(provide 'minibuffer-tests)
+;;; minibuffer-tests.el ends here

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

From: Juri Linkov <juri <at> linkov.net>
To: Clemens <clemera <at> posteo.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Sat, 30 Jan 2021 21:13:32 +0200

tags 45780 fixed
close 45780 28.0.50
thanks

>>> I agree its purpose is quite different from the example above.
>>> Then maybe something like this should do what you want:
>>
>> Yes, that would be nice if you also think it would be okay to change it
>> this way, thank you!

Now the change is pushed to master in commit b32d4bf682.

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

From: Clemens <clemera <at> posteo.net>
To: Juri Linkov <juri <at> linkov.net>
Cc: 45780 <at> debbugs.gnu.org
Subject: Re: bug#45780: 28.0.50; [PATCH] Face used for affixation function
 annotations
Date: Sun, 31 Jan 2021 10:36:17 +0100

> Now the change is pushed to master in commit b32d4bf682.


Nice! Thanks, for discussing and implementing it!

Previous Next

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