[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] `affixation-function`: Allow only three-element lists

From: Daniel Mendler
Subject: Re: [PATCH] `affixation-function`: Allow only three-element lists
Date: Sun, 25 Apr 2021 20:02:39 +0200

On 4/25/21 7:46 PM, Juri Linkov wrote:
@@ -2110,7 +2110,9 @@ minibuffer-completion-help
                          (setq completions
-                              (funcall aff-fun completions)))
+                              (funcall aff-fun completions))
+                        (cl-assert (or (not completions)
+                                       (= 3 (length (car completions))))))

It's surprising to see that you added such artificial restriction.
I expected a change with something more like this:

and then simplify the implementation of completion--insert-strings
by removing all complexity that handled suffix-only completion strings.

Yes, this is certainly a less artificial way to restrict the results of the `affixation-function`. However this works only if you add the condition on when to add annotation faces as you proposed below.

diff --git a/lisp/simple.el b/lisp/simple.el
index 999755a642..26eb8cad7f 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2080,8 +2080,11 @@ 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)))
+                             (format " (%s)" (key-description binding)))
+                            (t ""))))
+         (put-text-property 0 (length suffix)
+                            'face 'completions-annotations suffix)
+         (list command-name "" suffix)))

Why such change only for one particular use of annotations?
Shouldn't completion--insert-strings check if the suffix is an
empty string, and then put the 'completions-annotations' face on it?

I did not do this since I wanted to avoid magic which complicates the definition of the `affixation-function`. The idea was to never add faces to the annotations supplied by the `affixation-function`.

I am looking at the `affixation-function` from the perspective of how the definition can be made as simple/predicatable as possible for the API user since there are other completion UIs and many completion tables which will make use of this. I treat the completion UI behind the API as a blackbox.

In contrast you take the perspective of how the affixation function could be fitted best into the existing completion code without introducing artificial restrictions.

I argue that it is better to take a step back from the actual code base in minibuffer.el and adopt the simpler definition.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]