bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations

Juri Linkov-2
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:


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.
Reply | Threaded
Open this post in threaded view
|

bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations

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.

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!



Reply | Threaded
Open this post in threaded view
|

bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations

Juri Linkov-2
>>> 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)))



Reply | Threaded
Open this post in threaded view
|

bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations

Clemens

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



Reply | Threaded
Open this post in threaded view
|

bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations

Juri Linkov-2
>> 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?



Reply | Threaded
Open this post in threaded view
|

bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations

Clemens

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



Reply | Threaded
Open this post in threaded view
|

bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations

Juri Linkov-2
>> 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:


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
Reply | Threaded
Open this post in threaded view
|

bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations

Clemens

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