unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
@ 2021-01-11 12:38 Clemens
  2021-01-11 18:38 ` Juri Linkov
  0 siblings, 1 reply; 12+ messages in thread
From: Clemens @ 2021-01-11 12:38 UTC (permalink / raw)
  To: 45780

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

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

[-- Attachment #2: affix.patch --]
[-- Type: text/x-patch, Size: 645 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 556f5d3a56..a6afb04efb 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1790,7 +1790,7 @@ completion--insert-strings
                     ;; by the caller via affixation-function,
                     ;; then allow the caller to decide
                     ;; what faces to put on prefix and suffix.
-                    (unless prefix
+                    (unless suffix
                       (font-lock-prepend-text-property
                        beg end 'face 'completions-annotations))))
                 (put-text-property (point) (progn (insert (car str)) (point))

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-11 12:38 bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations Clemens
@ 2021-01-11 18:38 ` Juri Linkov
  2021-01-11 20:07   ` Clemens
  0 siblings, 1 reply; 12+ messages in thread
From: Juri Linkov @ 2021-01-11 18:38 UTC (permalink / raw)
  To: Clemens; +Cc: 45780

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: affix-doc.patch --]
[-- Type: text/x-diff, Size: 1253 bytes --]

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.

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-11 18:38 ` Juri Linkov
@ 2021-01-11 20:07   ` Clemens
  2021-01-12 18:30     ` Juri Linkov
  0 siblings, 1 reply; 12+ messages in thread
From: Clemens @ 2021-01-11 20:07 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 45780

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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-11 20:07   ` Clemens
@ 2021-01-12 18:30     ` Juri Linkov
  2021-01-13 18:06       ` Clemens
  0 siblings, 1 reply; 12+ messages in thread
From: Juri Linkov @ 2021-01-12 18:30 UTC (permalink / raw)
  To: Clemens; +Cc: 45780

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





^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-12 18:30     ` Juri Linkov
@ 2021-01-13 18:06       ` Clemens
  2021-01-14  9:00         ` Juri Linkov
  0 siblings, 1 reply; 12+ messages in thread
From: Clemens @ 2021-01-13 18:06 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 45780


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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-13 18:06       ` Clemens
@ 2021-01-14  9:00         ` Juri Linkov
  2021-01-14 17:21           ` Clemens
  0 siblings, 1 reply; 12+ messages in thread
From: Juri Linkov @ 2021-01-14  9:00 UTC (permalink / raw)
  To: Clemens; +Cc: 45780

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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-14  9:00         ` Juri Linkov
@ 2021-01-14 17:21           ` Clemens
  2021-01-14 18:59             ` Juri Linkov
  0 siblings, 1 reply; 12+ messages in thread
From: Clemens @ 2021-01-14 17:21 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 45780


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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-14 17:21           ` Clemens
@ 2021-01-14 18:59             ` Juri Linkov
  2021-01-14 19:43               ` Clemens
  0 siblings, 1 reply; 12+ messages in thread
From: Juri Linkov @ 2021-01-14 18:59 UTC (permalink / raw)
  To: Clemens; +Cc: 45780

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: completion--insert-strings-text-property-not-all.patch --]
[-- Type: text/x-diff, Size: 1794 bytes --]

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-14 18:59             ` Juri Linkov
@ 2021-01-14 19:43               ` Clemens
  2021-01-25 18:02                 ` Juri Linkov
  0 siblings, 1 reply; 12+ messages in thread
From: Clemens @ 2021-01-14 19:43 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 45780


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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-14 19:43               ` Clemens
@ 2021-01-25 18:02                 ` Juri Linkov
  2021-01-30 19:13                   ` Juri Linkov
  0 siblings, 1 reply; 12+ messages in thread
From: Juri Linkov @ 2021-01-25 18:02 UTC (permalink / raw)
  To: Clemens; +Cc: 45780

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: completion--insert-strings-faces.patch --]
[-- Type: text/x-diff, Size: 1477 bytes --]

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-25 18:02                 ` Juri Linkov
@ 2021-01-30 19:13                   ` Juri Linkov
  2021-01-31  9:36                     ` Clemens
  0 siblings, 1 reply; 12+ messages in thread
From: Juri Linkov @ 2021-01-30 19:13 UTC (permalink / raw)
  To: Clemens; +Cc: 45780

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.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations
  2021-01-30 19:13                   ` Juri Linkov
@ 2021-01-31  9:36                     ` Clemens
  0 siblings, 0 replies; 12+ messages in thread
From: Clemens @ 2021-01-31  9:36 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 45780

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


Nice! Thanks, for discussing and implementing it!





^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-01-31  9:36 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11 12:38 bug#45780: 28.0.50; [PATCH] Face used for affixation function annotations Clemens
2021-01-11 18:38 ` Juri Linkov
2021-01-11 20:07   ` Clemens
2021-01-12 18:30     ` Juri Linkov
2021-01-13 18:06       ` Clemens
2021-01-14  9:00         ` Juri Linkov
2021-01-14 17:21           ` Clemens
2021-01-14 18:59             ` Juri Linkov
2021-01-14 19:43               ` Clemens
2021-01-25 18:02                 ` Juri Linkov
2021-01-30 19:13                   ` Juri Linkov
2021-01-31  9:36                     ` Clemens

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).