[PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

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

I attached a patch which adds support for the affixation and annotation
function to the recently added icomplete-vertical-mode (Fix bug#48013).

Daniel

[-- Attachment #2: 0001-icomplete-vertical-mode-Add-support-for-affixations-.patch --]
[-- Type: text/x-diff, Size: 3427 bytes --]

From 04e7c6da34df6b60e253a35b9baa3eba4062617f Mon Sep 17 00:00:00 2001
From: Daniel Mendler <mail@daniel-mendler.de>
Date: Sat, 22 May 2021 22:41:27 +0200
Subject: [PATCH] (icomplete-vertical-mode): Add support for affixations and
 annotations

Fix bug#48013

* lisp/icomplete.el (icomplete--affixate): Add function, which
annotates the currently visible candidates, given the
`affixation-function` or `annotation-function` specified by the
completion metadata.
(icomplete-completions): Use the new function.
---
 lisp/icomplete.el | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 91bbb60013..83003fd5f3 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -666,6 +666,28 @@ icomplete-exhibit
               (put-text-property 0 1 'cursor t text)
               (overlay-put icomplete-overlay 'after-string text))))))))
 
+(defun icomplete--affixate (md prospects)
+  "Affixate PROSPECTS with prefixes and suffixes, given completion metadata MD."
+  (let ((aff-fun (or (completion-metadata-get md 'affixation-function)
+                     (plist-get completion-extra-properties :affixation-function)))
+        (ann-fun (or (completion-metadata-get md 'annotation-function)
+                     (plist-get completion-extra-properties :annotation-function))))
+    (if aff-fun
+        (funcall aff-fun prospects)
+      (if ann-fun
+          (mapcar
+           (lambda (comp)
+             (let ((suffix (or (funcall ann-fun comp) "")))
+               (list comp ""
+                     ;; The default completion UI adds the
+                     ;; `completions-annotations' face if no
+                     ;; other faces are present.
+                     (if (text-property-not-all 0 (length suffix) 'face nil suffix)
+                         suffix
+                       (propertize suffix 'face 'completions-annotations)))))
+           prospects)
+        prospects))))
+
 ;;;_ > icomplete-completions (name candidates predicate require-match)
 (defun icomplete-completions (name candidates predicate require-match)
   "Identify prospective candidates for minibuffer completion.
@@ -814,14 +836,18 @@ icomplete-completions
         ;; is cached.
         (if last (setcdr last base-size))
 	(if prospects
-	    (concat determ
-		    (if icomplete-vertical-mode " \n" "{")
-		    (mapconcat 'identity prospects (if icomplete-vertical-mode
-                                                       "\n"
-                                                       icomplete-separator))
-		    (unless icomplete-vertical-mode
-                      (concat (and limit (concat icomplete-separator ellipsis))
-                              "}")))
+            (if icomplete-vertical-mode
+	        (concat determ " \n"
+                        (mapconcat
+                         (lambda (comp)
+                           (if (consp comp)
+                               (concat (cadr comp) (car comp) (caddr comp))
+                             comp))
+                         (icomplete--affixate md prospects)
+                         "\n"))
+	      (concat determ "{"
+                      (mapconcat 'identity prospects icomplete-separator)
+                      (and limit (concat icomplete-separator ellipsis) "}")))
 	  (concat determ " [Matched]"))))))
 
 ;;; Iswitchb compatibility
-- 
2.20.1

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Thank you Daniel,

Can you give a visual example of what it looks like for a given completion
table, along with the changes to the completion table code to take
advantage of the new interface?

It seems that neither affixation or annotation were present in non-vertical
icomplete.el before your patch.  Do you have an idea why?  Should they
be added there, too?

João

PS: feel free to redirect this discussion (but maybe not crosspost) to
48013@debbugs.gnu.org.  I haven't done so as I didn't know your intent
in posting to emacs-devel (maybe you want to reach a higher audience?)

bug#48013: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

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

On 5/23/21 11:37 AM, João Távora wrote:
> Can you give a visual example of what it looks like for a given completion
> table, ...

I attached two screenshots:

- M-x
- `describe-symbol` if `completions-detailed` is set

The layout is the same as in the usual *Completions* buffer, when
`completions-format` is set to `one-column`.

> ...along with the changes to the completion table code to take
> advantage of the new interface?

See for example `help--symbol-completion-table-affixation` in
help-fns.el or `read-extended-command--affixation` in simple.el.

There are more `annotation-function`s/`affixation-function`s present in
the Emacs code base if you grep.

See also minibuffer.el for the implementation for the *Completions* buffer.

> It seems that neither affixation or annotation were present in non-vertical
> icomplete.el before your patch.  Do you have an idea why?  Should they
> be added there, too?

There is not enough space in the vertical display. The only
annotations/affixations which would make sense are single character
annotations, maybe icons. But people who like the compact Ido/Icomplete
horizontal style probably would not want this additional noise.
Furthermore it makes the completions harder to read since you get
prefix-completion-suffix, prefix-completion-suffix, where everything is
mixed up. This is different from the vertical mode, where annotations
and candidates are visually separate and easier to distinguish.

Daniel

[-- Attachment #2: describe-symbol.png --]
[-- Type: image/png, Size: 99332 bytes --]

[-- Attachment #3: Mx.png --]
[-- Type: image/png, Size: 46465 bytes --]

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/23/21 11:37 AM, João Távora wrote:
>> Can you give a visual example of what it looks like for a given completion
>> table, ...
>
> I attached two screenshots:
>
> - M-x
> - `describe-symbol` if `completions-detailed` is set

Thanks, I wasn't aware of this new framework and think this looks very
nice and is extremely useful.  I could get your second example to work,
but not the first (the M-x one).  What am I missing?

>> It seems that neither affixation or annotation were present in non-vertical
>> icomplete.el before your patch.  Do you have an idea why?  Should they
>> be added there, too?
>
> There is not enough space in the vertical display.

You mean in the horizontal one-line display right?  In the vertical
display there is space, so we are enabling it there.

> The only annotations/affixations which would make sense are single
> character annotations, maybe icons. But people who like the compact
> Ido/Icomplete horizontal style probably would not want this additional
> noise.  Furthermore it makes the completions harder to read since you
> get prefix-completion-suffix, prefix-completion-suffix, where
> everything is mixed up. This is different from the vertical mode,
> where annotations and candidates are visually separate and easier to
> distinguish.

Yes, I agree with this.  Thanks for clarifying.

After my sig, I attach a very slightly simplified (and untested) version
of icomplete-affixate, using `cond` instead of nested `ifs`.  But the
main thing I wanted to note at this point, is that I find this
"affixation" design confusing and flawed.

Reading the documentation, I don't see why we couldn't just expand
annotation-function's protocol, instead of adding a new (akwardly) named
entry point with an priority mechanism.

I also don't understand why :affixation-function is given a full list of
completions, when it is presumably meant to return a list of exactly the
same length.  Seems like a potential hazard to allow this function to do
filtering.

So, unless I am missing something (known to happen), this seems like a
misdesign which would be nice to correct before the Emacs 28 release
and/or too many external packages start relying on this.

There was probably some discussion on this, so this ship may have salied
partially.  I'm switching this discussion back to emacs-devel@gnu.org
and CCing Juri as well.

My idea is that it is not too late to:

(1) scratch affixation-function

(2) make annotation function return potentially richer propertized
completion strings.

(3) Adjust the handful of users and consumers around this function.

João.

(defun icomplete--affixate (md prospects)
  "Affixate PROSPECTS with prefixes and suffixes, given completion metadata MD."
  (let ((aff-fun (or (completion-metadata-get md 'affixation-function)
                     (plist-get completion-extra-properties :affixation-function)))
        (ann-fun (or (completion-metadata-get md 'annotation-function)
                     (plist-get completion-extra-properties :annotation-function))))
    (cond (aff-fun (funcall aff-fun prospects))
          (ann-fun
           (mapcar
            (lambda (comp)
              (let ((suffix (or (funcall ann-fun comp) "")))
                (list comp ""
                      ;; The default completion UI adds the
                      ;; `completions-annotations' face if no
                      ;; other faces are present.
                      (if (text-property-not-all 0 (length suffix) 'face nil suffix)
                          suffix
                        (propertize suffix 'face 'completions-annotations)))))
            prospects))
          (prospects))))

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/23/21 9:55 PM, João Távora wrote:
>>> Can you give a visual example of what it looks like for a given completion
>>> table, ...
>>
>> I attached two screenshots:
>>
>> - M-x
>> - `describe-symbol` if `completions-detailed` is set
> 
> Thanks, I wasn't aware of this new framework and think this looks very
> nice and is extremely useful.  I could get your second example to work,
> but not the first (the M-x one).  What am I missing?

Ah, I forgot to tell you that. The M-x annotations are guarded by
`suggest-key-bindings`.

>>> It seems that neither affixation or annotation were present in non-vertical
>>> icomplete.el before your patch.  Do you have an idea why?  Should they
>>> be added there, too?
>>
>> There is not enough space in the vertical display.
> 
> You mean in the horizontal one-line display right?  In the vertical
> display there is space, so we are enabling it there.

Yes, I meant that the horizontal display does not have enough space.

> After my sig, I attach a very slightly simplified (and untested) version
> of icomplete-affixate, using `cond` instead of nested `ifs`.  But the
> main thing I wanted to note at this point, is that I find this
> "affixation" design confusing and flawed.

I have not been involved in the initial design of this, the
`affixation-function` is there for quite a while already.

I would also be happy with an extension to the `annotation-function`,
where the returned annotation string is the suffix and can be
propertized with an additional string for the prefix.

> I also don't understand why :affixation-function is given a full list of
> completions, when it is presumably meant to return a list of exactly the
> same length.  Seems like a potential hazard to allow this function to do
> filtering.

It is not allowed by the definition of the function.

> So, unless I am missing something (known to happen), this seems like a
> misdesign which would be nice to correct before the Emacs 28 release
> and/or too many external packages start relying on this.

For what is worth, my packages already rely on this. But I would have
this changed quickly given a modified definition of the
`annotation-function`. I am not aware of other packages already using
this functionality (I searched around for this at some point).

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Ah, I forgot to tell you that. The M-x annotations are guarded by
> `suggest-key-bindings`.

Thanks, I see it.

> I have not been involved in the initial design of this, the
> `affixation-function` is there for quite a while already.

6 months, not very long in Emacs release terms.  But indeed you're right
that it wasn't last week..

> I would also be happy with an extension to the `annotation-function`,
> where the returned annotation string is the suffix and can be
> propertized with an additional string for the prefix.

Either that, or it could return a list instead of a string, and then the
list would be interpreted as "affixation function" results.

>> I also don't understand why :affixation-function is given a full list of
>> completions, when it is presumably meant to return a list of exactly the
>> same length.  Seems like a potential hazard to allow this function to do
>> filtering.
>
> It is not allowed by the definition of the function.

Yes, but users love to foot-shoot and who is going to enforce that?
Your icomplete patch doesn't, for example.  Whereas if it were a unary
function of a completion string, the hazard wouldn't exist.

In the meantime, I've read some of

   https://lists.gnu.org/archive/html/emacs-devel/2020-11/msg00613.html

and it seems one purported advantage of having `affixation-function` see
all the candidates is to be able to do layout decisions since it knows
the longest and shortest candidate.  At least this is what I read from

  https://lists.gnu.org/archive/html/emacs-devel/2020-11/msg01009.html

But I don't understand why these decision should be done in the data
backend.  The backedn should only annotate a completion with a possible
_hint_ on how to layout ("prefix" and "suffix" are hints). It's the
frontend, who is who knows what to display and where, who should be
responsible for the layout.

>> So, unless I am missing something (known to happen), this seems like a
>> misdesign which would be nice to correct before the Emacs 28 release
>> and/or too many external packages start relying on this.
>
> For what is worth, my packages already rely on this. But I would have
> this changed quickly given a modified definition of the
> `annotation-function`. I am not aware of other packages already using
> this functionality (I searched around for this at some point).

That's valuable information.  Let's wait for Juri and Stefan on this.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/23/21 11:04 PM, João Távora wrote:
>> I would also be happy with an extension to the `annotation-function`,
>> where the returned annotation string is the suffix and can be
>> propertized with an additional string for the prefix.
> 
> Either that, or it could return a list instead of a string, and then the
> list would be interpreted as "affixation function" results.

A list return value is not a good idea since that does not allow to
write forward and backward compatible code. This is actually an
advantage of the `affixation-function`. It is a non-intrusive addition.
The same holds for the recently added `group-function`, which I proposed.

>>> I also don't understand why :affixation-function is given a full list of
>>> completions, when it is presumably meant to return a list of exactly the
>>> same length.  Seems like a potential hazard to allow this function to do
>>> filtering.
>>
>> It is not allowed by the definition of the function.
> 
> Yes, but users love to foot-shoot and who is going to enforce that?
> Your icomplete patch doesn't, for example.  Whereas if it were a unary
> function of a completion string, the hazard wouldn't exist.

Of course.

> and it seems one purported advantage of having `affixation-function` see
> all the candidates is to be able to do layout decisions since it knows
> the longest and shortest candidate. 

Indeed, that was the point. Unfortunately it turns out that having
layout decisions based on all the currently visible candidates will give
bad results when scrolling. So I am never using that capability in my
packages.

You may have seen the Marginalia package by Omar Antolín Camarena and
myself, where we add rich annotations to many commands using the
`annotation/affixation-function` (https://github.com/minad/marginalia).
We are doing manual alignment there.

So from my current experience the advantage is small.

> But I don't understand why these decision should be done in the data
> backend.  The backedn should only annotate a completion with a possible
> _hint_ on how to layout ("prefix" and "suffix" are hints). It's the
> frontend, who is who knows what to display and where, who should be
> responsible for the layout.

Yes, alignment should better happen on the display level.

>>> So, unless I am missing something (known to happen), this seems like a
>>> misdesign which would be nice to correct before the Emacs 28 release
>>> and/or too many external packages start relying on this.
>>
>> For what is worth, my packages already rely on this. But I would have
>> this changed quickly given a modified definition of the
>> `annotation-function`. I am not aware of other packages already using
>> this functionality (I searched around for this at some point).
> 
> That's valuable information.  Let's wait for Juri and Stefan on this.

If it is decided to change this, I think it would be best to use text
properties for this. The `annotation-function` should continue to return
suffix strings and additionally, each suffix has an `annotation-prefix`
text property attached. That would be a more minimal API change than the
addition of the `affixation-function`, while still being fully compatible.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/23/21 11:04 PM, João Távora wrote:
>>> I would also be happy with an extension to the `annotation-function`,
>>> where the returned annotation string is the suffix and can be
>>> propertized with an additional string for the prefix.
>> 
>> Either that, or it could return a list instead of a string, and then the
>> list would be interpreted as "affixation function" results.
>
> A list return value is not a good idea since that does not allow to
> write forward and backward compatible code.

Yes, good point.

>> and it seems one purported advantage of having `affixation-function` see
>> all the candidates is to be able to do layout decisions since it knows
>> the longest and shortest candidate. 
>
> Indeed, that was the point. Unfortunately it turns out that having
> layout decisions based on all the currently visible candidates will give
> bad results when scrolling. So I am never using that capability in my
> packages.
>
> You may have seen the Marginalia package by Omar Antolín Camarena and
> myself, where we add rich annotations to many commands using the
> `annotation/affixation-function` (https://github.com/minad/marginalia).
> We are doing manual alignment there.
>
> So from my current experience the advantage is small.

Good to know that feedback.

>> But I don't understand why these decision should be done in the data
>> backend.  The backedn should only annotate a completion with a possible
>> _hint_ on how to layout ("prefix" and "suffix" are hints). It's the
>> frontend, who is who knows what to display and where, who should be
>> responsible for the layout.
>
> Yes, alignment should better happen on the display level.

Exactly, in the case in point, icomplete.el.

>>>> So, unless I am missing something (known to happen), this seems like a
>>>> misdesign which would be nice to correct before the Emacs 28 release
>>>> and/or too many external packages start relying on this.
>>>
>>> For what is worth, my packages already rely on this. But I would have
>>> this changed quickly given a modified definition of the
>>> `annotation-function`. I am not aware of other packages already using
>>> this functionality (I searched around for this at some point).
>> 
>> That's valuable information.  Let's wait for Juri and Stefan on this.
>
> If it is decided to change this, I think it would be best to use text
> properties for this. The `annotation-function` should continue to return
> suffix strings and additionally, each suffix has an `annotation-prefix`
> text property attached. That would be a more minimal API change than the
> addition of the `affixation-function`, while still being fully compatible.

Yes, let's see what Juri and Stefan have to offer here.

By the way, as a tangent to this, but spurred by your activity and
improvements to icomplete.el, I'm also thinking of enhaving icomplete.el
to allow a different type of scrolling in icomplete-vertical-mode where
the active completion isn't necessarily shown in the screen line of the
minibuffer.  So that it acts more like a classic dropdown.  Kind of
company-mode but in the minibuffer.

Another idea is to make icomplete work for
`completion-in-region-function`.

> Independent of the orthogonal discussion about the pros and cons of
> the `affixation-function` is the patch (maybe including your
> simplification) acceptable to merge? If a specification change is
> later made, Icomplete is quickly adapted.

I'd prefer if we wait a bit.  For one, adding this to
icomplete-vertical-mode would encourage more backend writers to use what
we both seem to agree is a flawed API, thus making the effort we are
discussing more difficult. So let's give Juri and Stefan some time to
opine on this, and then maybe work on a patch that overhauls
annotation-function.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/23/21 11:54 PM, João Távora wrote:
> By the way, as a tangent to this, but spurred by your activity and
> improvements to icomplete.el, I'm also thinking of enhaving icomplete.el
> to allow a different type of scrolling in icomplete-vertical-mode where
> the active completion isn't necessarily shown in the screen line of the
> minibuffer.  So that it acts more like a classic dropdown.  Kind of
> company-mode but in the minibuffer.

You may want to take a look at my Vertico package which is on GNU ELPA.
It is basically a reimplementation of Icomplete with a classic dropdown.
The inner workings are similar to Icomplete. The main technical
difference is that Icomplete rotates the candidates, while Vertico keeps
an index. But besides that, Vertico is fully compliant with the
completion infrastructure.

> Another idea is to make icomplete work for
> `completion-in-region-function`.

Icomplete already should somehow work as a
`completion-in-region-function`, but I think it is a bit brittle.

Related to my Vertico package, I also have the Corfu package, which uses
a similar implementation as Vertico, but uses a child frame to display
the completion list (Similar to Company-Box, Company-Posframe etc).

https://github.com/minad/vertico
https://github.com/minad/corfu

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/23/21 11:54 PM, João Távora wrote:
>> By the way, as a tangent to this, but spurred by your activity and
>> improvements to icomplete.el, I'm also thinking of enhaving icomplete.el
>> to allow a different type of scrolling in icomplete-vertical-mode where
>> the active completion isn't necessarily shown in the screen line of the
>> minibuffer.  So that it acts more like a classic dropdown.  Kind of
>> company-mode but in the minibuffer.
>
> You may want to take a look at my Vertico package which is on GNU ELPA.
> It is basically a reimplementation of Icomplete with a classic dropdown.
> The inner workings are similar to Icomplete. The main technical
> difference is that Icomplete rotates the candidates, while Vertico keeps
> an index.

I think rotation makes sense in vanilla icomplete, but not in
icomplete-vertical-mode.  So for the latter, I think I will just take
the cue from Vertico and use an index (or equivalent).  Since it's in
GNU ELPA, maybe even borrow some code :-)

>> Another idea is to make icomplete work for
>> `completion-in-region-function`.
>
> Icomplete already should somehow work as a
> `completion-in-region-function`, but I think it is a bit brittle.

I have a vague recollection that Someone Somewhere (tm) in one of my
extensions over at GitHub tried it out and found it could mostly work
with some tweaks.  Will search for it.

> Related to my Vertico package, I also have the Corfu package, which uses
> a similar implementation as Vertico, but uses a child frame to display
> the completion list (Similar to Company-Box, Company-Posframe etc).
>
> https://github.com/minad/vertico
> https://github.com/minad/corfu

Very interesting.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/25/21 12:46 AM, João Távora wrote:
> Daniel Mendler <mail@daniel-mendler.de> writes:
> 
>> On 5/23/21 11:54 PM, João Távora wrote:
>>> By the way, as a tangent to this, but spurred by your activity and
>>> improvements to icomplete.el, I'm also thinking of enhaving icomplete.el
>>> to allow a different type of scrolling in icomplete-vertical-mode where
>>> the active completion isn't necessarily shown in the screen line of the
>>> minibuffer.  So that it acts more like a classic dropdown.  Kind of
>>> company-mode but in the minibuffer.
>>
>> You may want to take a look at my Vertico package which is on GNU ELPA.
>> It is basically a reimplementation of Icomplete with a classic dropdown.
>> The inner workings are similar to Icomplete. The main technical
>> difference is that Icomplete rotates the candidates, while Vertico keeps
>> an index.
> 
> I think rotation makes sense in vanilla icomplete, but not in
> icomplete-vertical-mode.  So for the latter, I think I will just take
> the cue from Vertico and use an index (or equivalent).  Since it's in
> GNU ELPA, maybe even borrow some code :-)

Oh, I disagree here. I think Icomplete should continue to use rotations,
since this is what Icomplete is. Why bake in two very different modi
inside Icomplete, given that Vertico exists? Honestly, I think it would
be better than just import Vertico into Emacs then. Of course it also
lives happily on ELPA.

Baking two such different modi in Icomplete only complicates things and
I don't consider Icomplete a clean implementation actually, since it
bakes both `completion-in-region` and `completing-read` into one package
(see my criticism on the README of Vertico and Corfu). Vertico and Corfu
are deliberately separate to have the ability to optimize the
`completion-in-region` and `completing-read` UI separately.

Instead of using an index you could still use the rotations and only
reorganize the display such that the current candidate is not visualized
on the top. I think using rotations is even necessary if you want to
continue to use the default completion functions which take the first
candidate of the list into account. Of course you can change these inner
workings too (I am talking about completion-all-sorted-completions).

But I am really not sure what the point is of this in Icomplete. Maybe
it makes sense to decouple/deconstruct the minibuffer.el implementation
more such that the completion backend functionality is more separate
from the display functionality.

>>> Another idea is to make icomplete work for
>>> `completion-in-region-function`.
>>
>> Icomplete already should somehow work as a
>> `completion-in-region-function`, but I think it is a bit brittle.
> 
> I have a vague recollection that Someone Somewhere (tm) in one of my
> extensions over at GitHub tried it out and found it could mostly work
> with some tweaks.  Will search for it.

I also tried it out and it somehow works. But as argued above, I think
it is actually better to maintain this in separate packages, at least if
you start from a clean slate. This way one can do away with all these
icomplete--field-beg accessors too.

>> Related to my Vertico package, I also have the Corfu package, which uses
>> a similar implementation as Vertico, but uses a child frame to display
>> the completion list (Similar to Company-Box, Company-Posframe etc).
>>
>> https://github.com/minad/vertico
>> https://github.com/minad/corfu

Of course I will not hold you back from revamping Icomplete completely.
It is good to have multiple UIs. But you could seriously consider my
packages - I think they are exactly what you may be looking for - a
clean slate (vertical) reimplementation of Ivy/Icomplete, relying only
on the default completion facilities. I am sure you have some good
criticism regarding my code or improvement proposals.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/25/21 12:46 AM, João Távora wrote:

>> I think rotation makes sense in vanilla icomplete, but not in
>> icomplete-vertical-mode.  So for the latter, I think I will just take
>> the cue from Vertico and use an index (or equivalent).  Since it's in
>> GNU ELPA, maybe even borrow some code :-)
>
> Oh, I disagree here. I think Icomplete should continue to use rotations,
> since this is what Icomplete is.

I don't think Icomplete, or anything in Emacs, is defined by any one
thing.  Things in Emacs do some things, and then people come and add
more things, usually configurable.  Icomplete has lots of switches that
change its behaviour.

icomplete-vertical-mode is a new mode for Emacs 28, so if any users are
very attached to the rotation, they'll likely come forward and the
obvious answer is to make a toggle.

> Why bake in two very different modi inside Icomplete, given that
> Vertico exists?

I don't follow.  A lot of "vertical completers" existed before either
Icomplete or Vertico, and that didn't prevent authors of the latter to
make them.  Some people would ask you back, "why make Vertico given that
Icomplete exists?!"  I'm not among these people.  Maybe Vertico does
something special or you simply enjoy writing it and maintaining it, all
valid reasons to me.

> Honestly, I think it would be better than just import Vertico into
> Emacs then.  Of course it also lives happily on ELPA.

No objections here.

> Baking two such different modi in Icomplete only complicates things and

For whom?  For you?  icomplete-vertical-mode wasn't my idea, though I
quite like that one is able to combine it with fido-mode, for example.

> I don't consider Icomplete a clean implementation actually, since it
> bakes both `completion-in-region` and `completing-read` into one package
> (see my criticism on the README of Vertico and Corfu).

I don't have an opinion on cleanliness but icomplete has been working ok
for me.  I used to use ido-mode, which is a bit "dirtier" than
icomplete, so to speak.  But if your stuff is extra-clean, that's great.

>>>> Another idea is to make icomplete work for
>>>> `completion-in-region-function`.
>>>
>>> Icomplete already should somehow work as a
>>> `completion-in-region-function`, but I think it is a bit brittle.
>> 
>> I have a vague recollection that Someone Somewhere (tm) in one of my
>> extensions over at GitHub tried it out and found it could mostly work
>> with some tweaks.  Will search for it.
>
> I also tried it out and it somehow works.

Cool, what did you do to make it work?

> But as argued above, I think it is actually better to maintain this in
> separate packages, at least if you start from a clean slate. This way
> one can do away with all these icomplete--field-beg accessors too.

I don't see why starting from a clean slate is desired or not.  Also
don't see the problem with those accessors.  At any rate they seem
implementation details.

> Of course I will not hold you back from revamping Icomplete
> completely. It is good to have multiple UIs. But you could seriously
> consider my packages - I think they are exactly what you may be
> looking for

I'll try your packages, but I'm fine with icomplete.el.  I've been using
it for some years and I want to use it.  I don't want to revamp it
completely, just maybe make its vertical mode slightly better.  In fact,
this whole thread is about your patch to do just that :)

I also want to try it for completion-in-region, so be sure to let me
know what you did to get it working.

João

BTW: if you're into completion system widgets, one thing that's sorely
missing in Emacs IMO is a completion tooltip in C that uses the same
display as the very nice new tty menus instead of overlays or tiny
frames.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

> > Icomplete already should somehow work as a
> > `completion-in-region-function`, but I think it is a bit brittle.
> I have a vague recollection that Someone Somewhere (tm) in one of my
> extensions over at GitHub tried it out and found it could mostly work
> with some tweaks.  Will search for it.

Found it, it was in https://github.com/joaotavora/eglot/issues/577,
and the author,
Daniel Semyonov, is using `completing-read` directly. IOW it's not specific
to icomplete at all.  Here is his code.

In that issue he states that Eglot's completion table is problematic, and
he's probably right, but the general idea of making a
completion-in-region-function
from completing-read is very interesting.  It would in principle mean that every
completing-read frontend could also be used in many other places.

João

(defun init-ui-completing-read-in-region (start end col &optional pred)
  "Completion in region function utilizing `completing-read'."
  (if (minibufferp) (completion--in-region start end col pred)
    (let* ((buffer (current-buffer))
           (pos (list start end))
           (init (buffer-substring-no-properties start end))
           (all (all-completions init col pred))
           (try (try-completion init col pred)))
      (setq this-command #'completion-at-point)
      (cond ((or (eq try t) ; Sole completion.
                 (and (= (length all) 1)
                      (string= (car all) try)))
             (choose-completion-string (car all) buffer pos)
             t)
            ;; Commented out since try-completion erroneously returns nil.
            ;; ((and (null try) ; No completions.
            ;;       (> (length init) 0))
            ;;  (message "No completion")
            ;;  nil)
            (t ; Some completions.
             (choose-completion-string
              (completing-read "Completion: " col pred t init) buffer pos)
             t)))))

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Semyonov via Emacs development discussions.]

>>>>> João Távora writes:

    > Found it, it was in
    > https://github.com/joaotavora/eglot/issues/577, and the author,
    > Daniel Semyonov, is using `completing-read` directly. IOW it's not
    > specific to icomplete at all.  Here is his code.

    > In that issue he states that Eglot's completion table is
    > problematic, and he's probably right, but the general idea of
    > making a completion-in-region-function from completing-read is
    > very interesting.  It would in principle mean that every
    > completing-read frontend could also be used in many other places.

Not to derail the thread, but I do not recommend attempting to use that
function; I wrote it when I had a far worse understanding of the
completion system (and by pulling bits from various other functions I've
found online and in Emacs).  A far more robust implementation of the
same idea can be found in Daniel Mendler's Consult package (although
last I tested it also had issues with Eglot).

Regards,

  Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/23/21 11:54 PM, João Távora wrote:
> I'd prefer if we wait a bit.  For one, adding this to
> icomplete-vertical-mode would encourage more backend writers to use what
> we both seem to agree is a flawed API, thus making the effort we are
> discussing more difficult.

I thought a minute bit about this, therefore another mail. While I agree
with you that the status quo of having both an `annotation-function` and
an `affixation-function` is not beautiful API-wise, I am not sure if an
augmented `annotation-function` is actually a less "flawed" design, in
particular if one has to use a weird specification to retain the
compatibility (prefix string attached as text property to the returned
suffix string).

You can see the `affixation-function` as the successor of the
`annotation-function`, with the long term goal of replacing it entirely.
But given that, I would probably prefer a design where the function only
takes a single candidate as argument and returns a plist of annotations,
with :prefix, :suffix, and maybe more to allow extensions.

But regarding merging or not merging the patch, I don't agree with your
argument of taking this as leverage which makes the discussion more or
less difficult. The status quo is that the `affixation-function` is part
of the current `completing-read` definition and default completion UI in
Emacs master, so it should be okay to merge this. Corrections to the
definition are a separate discussion. In any case, if it comes to a
change of definition I am willing to help out with patches.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

Daniel Mendler [2021-05-24 01:04:03] wrote:
> On 5/23/21 11:54 PM, João Távora wrote:
>> I'd prefer if we wait a bit.  For one, adding this to
>> icomplete-vertical-mode would encourage more backend writers to use what
>> we both seem to agree is a flawed API, thus making the effort we are
>> discussing more difficult.
> I thought a minute bit about this, therefore another mail. While I agree
> with you that the status quo of having both an `annotation-function` and
> an `affixation-function` is not beautiful API-wise, I am not sure if an

I don't have a strong preference between re-using `annotation-function`
and adding a new method.  I think we should first figure out what is the
"ideal" API for that, and only once we have it can we see whether it can be
retro-fitted into `annotation-function`.


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Daniel Mendler [2021-05-24 01:04:03] wrote:
>> On 5/23/21 11:54 PM, João Távora wrote:
>>> I'd prefer if we wait a bit.  For one, adding this to
>>> icomplete-vertical-mode would encourage more backend writers to use what
>>> we both seem to agree is a flawed API, thus making the effort we are
>>> discussing more difficult.
>> I thought a minute bit about this, therefore another mail. While I agree
>> with you that the status quo of having both an `annotation-function` and
>> an `affixation-function` is not beautiful API-wise, I am not sure if an
>
> I don't have a strong preference between re-using `annotation-function`
> and adding a new method.  I think we should first figure out what is the
> "ideal" API for that, and only once we have it can we see whether it can be
> retro-fitted into `annotation-function`.

The ideal API for backends to say what they would like to annotation
completions with:

- would not allow the annotator backend to affect the cardinality or
  order of completions being annotated.

- would allow the the annotator backend to perform one-time setup and
  teardown for the whole set of completions.

annotation-function provides the first but not the second,
affixation-function provides the second but not the first.

I think the second may not be that useful for the use cases we have
right now, but let's say I'm wrong and it is.  In that case, I think
we're better giving another optional endpoint to the backend to perform
this setup teardown.  For example, a function of a lambda.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

> The ideal API for backends to say what they would like to annotation
> completions with:
>
> - would not allow the annotator backend to affect the cardinality or
>   order of completions being annotated.
>
> - would allow the the annotator backend to perform one-time setup and
>   teardown for the whole set of completions.
>
> annotation-function provides the first but not the second,
> affixation-function provides the second but not the first.
>
> I think the second may not be that useful for the use cases we have
> right now, but let's say I'm wrong and it is.

Fine by me.

> In that case, I think we're better giving another optional endpoint to
> the backend to perform this setup teardown.  For example, a function
> of a lambda.

I don't have a strong opinion on whether the "affixotation" function
should take a single completion or a whole list or what.  Here are the
aspects which I think matter, in this respect:

- We're not in the business of preventing people from shooting
  themselves in the foot, so I think it's fine to pass the whole list of
  completion and receive a whole list of annotated completions, even if
  that runs the risk of receiving a list in a different order or with
  different elements: we can document the expected behavior and just not
  worry about the sneaky guys.  Who knows: those sneaky guys may end up
  finding an actually good use for it.

- For performance reasons it should be OK to call the "affixotation"
  function one element (or a small number of elements) at a time, so we
  only do it for those completions which are actually displayed.
  So this function should not presume that it always receives the complete
  lists of completions.  But here again, this seems to be a small matter
  of documentation.


-- Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> - We're not in the business of preventing people from shooting
>   themselves in the foot, so I think it's fine to pass the whole list of
>   completion and receive a whole list of annotated completions, even if
>   that runs the risk of receiving a list in a different order or with
>   different elements: we can document the expected behavior and just not
>   worry about the sneaky guys.  Who knows: those sneaky guys may end up
>   finding an actually good use for it.

Extremely unlikely.  More likely the sneak would work fine for one
frontend, but break another one.

I don't have extremely strong views here either, but as an API user, I
distrust endpoints that allow silly stuff and then warn "don't!
undefined consequences" in the docs.  It's much friendlier if there is
one less way to mess up, especially when developing.

> - For performance reasons it should be OK to call the "affixotation"
>   function one element (or a small number of elements) at a time, so we
>   only do it for those completions which are actually displayed.
>   So this function should not presume that it always receives the complete
>   lists of completions.  But here again, this seems to be a small matter
>   of documentation.

If it's OK to call it one at a time, the zorglubization function should
take one completion at a time, no transcendental presumption needed.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Gregory Heytings]

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


Hi Daniel and João,

I said in bug#48013 that I was working on this, but I was waiting for my 
copyright assignment to complete before sending patches.  Anyway, 
apparently you've now taken this over, which is fine.  Alas I have no time 
to look at what you did to send useful comments this week.

Gregory

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Gregory Heytings <gregory@heytings.org> writes:

> Hi Daniel and João,
>
> I said in bug#48013 that I was working on this, but I was waiting for
> my copyright assignment to complete before sending patches.  Anyway,
> apparently you've now taken this over, which is fine.  Alas I have no
> time to look at what you did to send useful comments this week.
>
> Gregory

I've done a bit more work on icomplete.el and after seeing the
discussion evolve, I agree with Daniel that it's better to keep those
two things orthogonal.

So I've now published two branches:

- scratch/icomplete-vertical-mode-improvements

  This makes icomplete-vertical-mode much nicer IMO.  After removing the
  rotation in the vertical mode and adding a highlight color it looks a
  bit like Vertico (which I tried recently).  If people like the
  rotation in vertical mode, too, it's very easy to get it back.  It
  also adds the annotation capabilities using some of the code Daniel
  originally submitted.

  Since you, Gregory, made icomplete-vertical-mode and you, Daniel,
  worked on its annotation capabilities, I wish you'd both have a look.
  Else I will test it for a few days and then push to master.  There are
  still a lot of edges to polish, and I may have introduced some bugs,
  though hopefully not in the legacy non-vertical parts.

- scratch/annotation-function-improvements

  This overhauls annotation-function and makes the C-h f and M-x
  backends use it instead of affixation-function.  It doesn't delete
  affixation-function.

  It _may_ be useful, but I guess it's not the end of the
  affixation-function thing anyway, and I don't any much more to
  contribute at this point.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/25/21 12:54 AM, João Távora wrote:
> I think the second may not be that useful for the use cases we have
> right now, but let's say I'm wrong and it is.  In that case, I think
> we're better giving another optional endpoint to the backend to perform
> this setup teardown.  For example, a function of a lambda.

This sounds like a more complicated and impure API then the
`affixation-function` we have right now. I consider operating on lists
like `affixation-function` to be a cleaner solution. But it is of course
not as general. You may want to do setup/teardown only once, like in
minibuffer-setup/exit-hooks.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/25/21 12:54 AM, João Távora wrote:
>> I think the second may not be that useful for the use cases we have
>> right now, but let's say I'm wrong and it is.  In that case, I think
>> we're better giving another optional endpoint to the backend to perform
>> this setup teardown.  For example, a function of a lambda.
>
> This sounds like a more complicated and impure API then the
> `affixation-function` we have right now. I consider operating on lists
> like `affixation-function` to be a cleaner solution. But it is of course
> not as general. You may want to do setup/teardown only once, like in
> minibuffer-setup/exit-hooks.

I don't know if you followed my idea, but yes, a hook also works.
Anyway, the case for needing to do these one-time annotation
setups/teardowns isn't really established anyway.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/23/21 11:54 PM, João Távora wrote:

> But regarding merging or not merging the patch, I don't agree with your
> argument of taking this as leverage which makes the discussion more or
> less difficult.

I'm not taking this as "leverage", I just don't think icomplete.el
should embark into what I consider (and apparently others) a misdesigned
API.  We should strive to come up with maintainable and reliable
systems, not just merge something because it happens to work and look
nice (which is plain to see that it does).

In the branch

   scratch/icomplete-vertical-mode-related-work

which I've just pushed to the repo, I have two commits:

- One commit, titled "Overhaul annotation-function to match
affixation-function" makes annotation-function match its cousin in
capability and switches the two producers of affixation-function to use
annotation-function instead.  It uses the logic we discussed before,
returning a propertized string.  It should be backward and forward
compatible.  Modulo bugs, it's not very well tested.  After this commit,
the only consumer of annotation-function in Emacs is the *Completions*
buffer, as before.

- The other commit, titled "Make icomplete-vertical-mode behave a little
more like a dropdown", is still a work in progress.  It reworks
icomplete.el to make the vertical mode behave more like a typical
dropdown.  Alas, it's not very easy to do, especially since a looping
dropdown is hard to define in the first place.  I think, before
icomplete-vertical-mode can really be usable and behave like the fancy
extra-Emacs packages, it needs to be iterated some more.

This commit also makes use of annotation-function.  In the process it
shows how layout decisions are delegated to icomplete.el, not to the
backend, a criticism of affixation function that Dmitry and Stefan seem
to share.

Personally, given the evidence here, I'd install the first commit (after
testing) and not encourage people to use affixation function for now.
We'll see if we keep or remove it (I haven't touched it).

I'd also keep working on icomplete.el so we can clean up
icomplete-vertical-mode.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

João Távora <joaotavora@gmail.com> writes:

> Daniel Mendler <mail@daniel-mendler.de> writes:
>
>> On 5/23/21 11:54 PM, João Távora wrote:
>
>> But regarding merging or not merging the patch, I don't agree with your
>> argument of taking this as leverage which makes the discussion more or
>> less difficult.
>
> I'm not taking this as "leverage", I just don't think icomplete.el
> should embark into what I consider (and apparently others) a misdesigned
> API.  We should strive to come up with maintainable and reliable
> systems, not just merge something because it happens to work and look
> nice (which is plain to see that it does).

By the way, another reason not to merge your patch as it is originally
is that it seems to affixate too many prospects, way way more than are
shown.  My alternative patch should, in principle, only annotate the
candidates that are to be displayed.  It also does suffix alignment (but
not prefix yet).  So my view is that there are edges to polish in
multiple angles and we shouldn't rush this.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/24/21 9:13 PM, João Távora wrote:
> João Távora <joaotavora@gmail.com> writes:
> 
>> Daniel Mendler <mail@daniel-mendler.de> writes:
>>
>>> On 5/23/21 11:54 PM, João Távora wrote:
>>
>>> But regarding merging or not merging the patch, I don't agree with your
>>> argument of taking this as leverage which makes the discussion more or
>>> less difficult.
>>
>> I'm not taking this as "leverage", I just don't think icomplete.el
>> should embark into what I consider (and apparently others) a misdesigned
>> API.  We should strive to come up with maintainable and reliable
>> systems, not just merge something because it happens to work and look
>> nice (which is plain to see that it does).

Okay, I agree with what you say about maintainable and reliable systems.
I am thankful that you are putting work into the
`icomplete-vertical-mode`. And I hope we find a good solution for the
annotations.

My proposal to merge this is not just because it "looks nice". It
replicates what is already present in the default UI which is part of
the core functionality in minibuffer.el. Everything that you consider to
be wrong here should also be fixed there.

> By the way, another reason not to merge your patch as it is originally
> is that it seems to affixate too many prospects, way way more than are
> shown.  My alternative patch should, in principle, only annotate the
> candidates that are to be displayed.  It also does suffix alignment (but
> not prefix yet).  So my view is that there are edges to polish in
> multiple angles and we shouldn't rush this.

If that is indeed the case this is a fair criticism and my patch should
not be merged. However this implies that there was something wrong with
the code all along since the prospects I am annotating are concatenated
right away. Of course the concatenation should only be done for the
candidates which are visible.

Regarding the patches you are working one, I think we should first agree
on a way forward regarding the `affixation-function`. I don't think it
is a good idea to end up with the `affixation-function` in addition the
enhanced `annotation-function` with its prefix/suffix annotations. If we
go with the enhanced `annotation-function` the `affixation-function`
should be removed.

I wonder about the semantics of your 'prefix and 'suffix annotations.
Will the 'suffix take precedence over the string itself then or are they
supposed to be concatenated? Wouldn't it be sufficient to only add a
'prefix annotation? Is there a possibility to add more annotations? In
my other mail I had thought about supporting multiple annotation fields
which are then displayed in columns. Do you consider consider something
like this to be realistic?

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/24/21 9:13 PM, João Távora wrote:
>> João Távora <joaotavora@gmail.com> writes:
>> 
>>> Daniel Mendler <mail@daniel-mendler.de> writes:
>>>
>>>> On 5/23/21 11:54 PM, João Távora wrote:
>>>
>>>> But regarding merging or not merging the patch, I don't agree with your
>>>> argument of taking this as leverage which makes the discussion more or
>>>> less difficult.
>>>
>>> I'm not taking this as "leverage", I just don't think icomplete.el
>>> should embark into what I consider (and apparently others) a misdesigned
>>> API.  We should strive to come up with maintainable and reliable
>>> systems, not just merge something because it happens to work and look
>>> nice (which is plain to see that it does).
>
> Okay, I agree with what you say about maintainable and reliable systems.
> I am thankful that you are putting work into the
> `icomplete-vertical-mode`. And I hope we find a good solution for the
> annotations.
>
> My proposal to merge this is not just because it "looks nice". It
> replicates what is already present in the default UI which is part of
> the core functionality in minibuffer.el. Everything that you consider to
> be wrong here should also be fixed there.
>
>> By the way, another reason not to merge your patch as it is originally
>> is that it seems to affixate too many prospects, way way more than are
>> shown.  My alternative patch should, in principle, only annotate the
>> candidates that are to be displayed.  It also does suffix alignment (but
>> not prefix yet).  So my view is that there are edges to polish in
>> multiple angles and we shouldn't rush this.
>
> If that is indeed the case this is a fair criticism and my patch should
> not be merged. However this implies that there was something wrong with
> the code all along since the prospects I am annotating are concatenated
> right away. Of course the concatenation should only be done for the
> candidates which are visible.

I see the original code for icomplete-vertical-mode and a rough proof of
concept, workable but still a long way to go to be as sturdy as Ivy or
Helm or those new things.  Until your patch, it didn't annotate
completions, and so this wasn't a problem so far.  Your (or is it
someone else's?) Vertico package probably is much better, and
icomplete.el should take clues from it.

> Regarding the patches you are working one, I think we should first
> agree on a way forward regarding the `affixation-function`.

I think making use of annotations in icomplete.el is a good exercise for
developing a good annotation-function protocol.  That's why I put it in
the same branch where I'm reworking icomplete.el.  But you're right that
they are independent things.

> I don't think it is a good idea to end up with the
> `affixation-function` in addition the enhanced `annotation-function`
> with its prefix/suffix annotations. If we go with the enhanced
> `annotation-function` the `affixation-function` should be removed.

I tend to agree, but there's also no harm in keeping both.  That's what
happens right now.

> I wonder about the semantics of your 'prefix and 'suffix annotations.
> Will the 'suffix take precedence over the string itself then or are they
> supposed to be concatenated? Wouldn't it be sufficient to only add a
> 'prefix annotation?

Yes, it would.  But I figured, it's better not even to call the return
value of annotation-function a "suffix".  It is an annotation, for the
frontends to place where they feel is most appropriate.  If the
annotation is richer, with hints such as 'prefix and 'suffix or
'super-fancy-columnized-hints some frontends may go the extra mile and
support those.

> Is there a possibility to add more annotations? In
> my other mail I had thought about supporting multiple annotation fields
> which are then displayed in columns. Do you consider consider something
> like this to be realistic?

Yes, I do.  None of this seems really hard to do.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/25/21 1:04 AM, João Távora wrote:
>> I don't think it is a good idea to end up with the
>> `affixation-function` in addition the enhanced `annotation-function`
>> with its prefix/suffix annotations. If we go with the enhanced
>> `annotation-function` the `affixation-function` should be removed.
> 
> I tend to agree, but there's also no harm in keeping both.  That's what
> happens right now.

Okay, but I hope in the final design the `affixation-function` is gone
now or we can agree on a reworked `affixation-function` as a successor
which aims to replace the `annotation-function`. But ending up with an
augmented `annotation-function` and the new `affixation-function` is
quite obviously chaos. This should be avoided. Of course it is fine if
Juri continues to rework the `affixation-function` he introduced and you
rework the `annotation-function` to your liking. But in the end there
should be some agreement which proposal to take!

Stefan and Dmitry, your thoughts about this?

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/25/21 1:04 AM, João Távora wrote:

> Okay, but I hope in the final design the `affixation-function` is gone
> now or we can agree on a reworked `affixation-function` as a successor
> which aims to replace the `annotation-function`. But ending up with an
> augmented `annotation-function` and the new `affixation-function` is
> quite obviously chaos.

Maybe, but is it more chaotic than the current situation?  There are
already two different ways to achieve the same purpose.

> This should be avoided. Of course it is fine if Juri continues to
> rework the `affixation-function` he introduced and you rework the
> `annotation-function` to your liking. But in the end there should be
> some agreement which proposal to take!

I agree that a single point would be nice to have, but we can't get rid
of annotation-function because it's been here for a good long while.
And affixation function has some drawbacks.  So one of:

(1) fix affixation-function's drawbacks (i.e. make it a function of a
    single completion)

(2) introduce a new function, call it decoration-function of
    foobarbazing-function

(3) improve annotation-function's capabilities.

I'm fine with any of these.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/25/21 11:31 AM, João Távora wrote:
> I agree that a single point would be nice to have, but we can't get rid
> of annotation-function because it's been here for a good long while.
> And affixation function has some drawbacks.  So one of:
> 
> (1) fix affixation-function's drawbacks (i.e. make it a function of a
>     single completion)

I am fine with that.

> (2) introduce a new function, call it decoration-function of
>     foobarbazing-function

I am fine with that too as long as the `affixation-function` is then
removed. You could also call this a renaming, since
`decoration-function` better reflects the generality of the new
functionality.

> (3) improve annotation-function's capabilities.

I am fine with that too of course.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 24.05.2021 22:05, João Távora wrote:
> - One commit, titled "Overhaul annotation-function to match
> affixation-function" makes annotation-function match its cousin in
> capability and switches the two producers of affixation-function to use
> annotation-function instead.  It uses the logic we discussed before,
> returning a propertized string.  It should be backward and forward
> compatible.

TBH, I'm not quite happy with the prefix-suffix thing either: it would 
be better to find some semantic names (if possible) and leave it to the 
frontend to do the layout.

For instance, if we're talking about annotation, Company users can 
choose between two ways of alignment, at least. And with 'kind' icons, 
some users will prefer them on the left, and some (even though there is 
currently no such option) would prefer it on the right.

The current examples of affixation function usages are quite modest: one 
can be turned into an annotation, and the other into a "kind" icon.

Perhaps the annotation placement hint for the former example might make 
sense, but I wouldn't call it crucial.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

>>> I would also be happy with an extension to the `annotation-function`,
>>> where the returned annotation string is the suffix and can be
>>> propertized with an additional string for the prefix.
>>
>> Either that, or it could return a list instead of a string, and then the
>> list would be interpreted as "affixation function" results.
>
> A list return value is not a good idea since that does not allow to
> write forward and backward compatible code. This is actually an
> advantage of the `affixation-function`. It is a non-intrusive addition.
> The same holds for the recently added `group-function`, which I proposed.

I agree.

> If it is decided to change this, I think it would be best to use text
> properties for this. The `annotation-function` should continue to return
> suffix strings and additionally, each suffix has an `annotation-prefix`
> text property attached. That would be a more minimal API change than the
> addition of the `affixation-function`, while still being fully compatible.

Such ad-hoc thing as attaching an `annotation-prefix` text property on
each suffix doesn't look like a real alternative to the clean API interface
provided by `affixation-function`.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

>> and it seems one purported advantage of having `affixation-function` see
>> all the candidates is to be able to do layout decisions since it knows
>> the longest and shortest candidate.
>
> Indeed, that was the point. Unfortunately it turns out that having
> layout decisions based on all the currently visible candidates will give
> bad results when scrolling. So I am never using that capability in my
> packages.
>
> You may have seen the Marginalia package by Omar Antolín Camarena and
> myself, where we add rich annotations to many commands using the
> `annotation/affixation-function` (https://github.com/minad/marginalia).
> We are doing manual alignment there.

This is valuable experience.  It looks like you render rich annotations
depending on the available candidates, and reformat the output
when the number of candidates and their sizes are changing,
like the horizontal completions format in the default frontend
tries to fit as many as possible completions on the same line.

But it seems this works only for limited cases like simple and short
prefix/suffix strings.  I can't imagine how such design could handle
multi-columnar rich annotations.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/24/21 1:39 AM, Juri Linkov wrote:
>> You may have seen the Marginalia package by Omar Antolín Camarena and
>> myself, where we add rich annotations to many commands using the
>> `annotation/affixation-function` (https://github.com/minad/marginalia).
>> We are doing manual alignment there.
> 
> This is valuable experience.  It looks like you render rich annotations
> depending on the available candidates, and reformat the output
> when the number of candidates and their sizes are changing,
> like the horizontal completions format in the default frontend
> tries to fit as many as possible completions on the same line.
> 
> But it seems this works only for limited cases like simple and short
> prefix/suffix strings.  I can't imagine how such design could handle
> multi-columnar rich annotations.

Do you mean we cannot handle something like a tabular layout where
annotations can be resized on the UI level. Indeed, Marginalia cannot do
that since the UIs don't support that. Marginalia is only a best effort
thing.

If we would want to support such multi-columnar data and present it as a
tablist in the frontend the `annotation-function/affixation-function`
must be greatly extended. From this POV I also don't see
`affixation-function` as a good and all-encompassing solution.

The annotation function should be allowed to return a plist of all
columns of annotations and at this point I agree with Dmitry, that the
columns should get semantic column names. Of course this should be
extensible, the data backend should be allowed to define arbitrary such
columns. And then the UI can render some of these semantic columns
specially (e.g. as Company is doing with company-kind). But I am not in
favor of extending the `completing-read` API with these `company-*`
extensions. If such a semantic annotation extension is added, it should
be non-namespaced ideally since it is not tied to the Company backend.

On 5/24/21 1:42 AM, Juri Linkov wrote:
>> https://lists.gnu.org/archive/html/emacs-devel/2021-04/msg01320.html
>
> And later your reply was that annotation-function is the old approach,
> and that the default completion frontend still doesn't use
> :company-kind. Any plans to add support for :company-kind to
> the default frontend?

As I argued above, I don't think such support should be added. I hope we
can find a better semantic annotation solution which encompasses
company-kind, prefixes, suffixes and arbitrary rich columnar data. The
question is how this should be done such that in the end the design does
not get too complicated.

João started this discussion since he was not happy with the design put
forward by the `affixation-function`. By adding support for arbitrary
backend extensions in the frontend we will go in exactly the opposite
direction, ending up with an even more chaotic API.

The question if the `affixation-function/annotation-function` should
work on a list of candidates or on a single candidate has also been
discussed here. This is more of a technical detail, but I also want to
stress the point made by Juri in the other mail, that the backend may
have to setup some "context" to compute the annotation data lazily. If
this setup can only be done once per redisplay of the currently visible
candidates, that is beneficial. We are also using that in Marginalia for
some buffer switching to the original window, such that annotations are
computed there. For example you want to see the values of the original
buffer in the annotations of the `describe-variable` command. Of course
the backend can also perform some caching to avoid such expenses. In the
"modern" incrementally automatically updating UIs like Icomplete, Ivy,
Vertico and Selectrum the annotations are only computed on a small
subset of candidates, so these costs don't matter so much. For the
default UI or other buffer-based UIs, the computation of all candidates
and all completions is usually manually triggered or delayed a bit such
that only a small subset of all candidates are computed, such that the
same argument applies as in the case of the Icomplete-style UIs.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

> Do you mean we cannot handle something like a tabular layout where
> annotations can be resized on the UI level.

Exactly.  What I'm trying to do is to use tabulated-list
in the completions buffer.  Your recent refactoring to
completion--insert-* was a big improvement.

Now we have several rendering functions in the default frontend:

1. completion--insert-horizontal
2. completion--insert-vertical
3. completion--insert-one-column

The next step would be to allow the option 'completions-format'
to accept an arbitrary frontend rendering function.  Then a new
possible value could be added such as:

4. completion--insert-multi-column

So when affixation-function will return a list of multiple columns,
e.g. for buffers the same list of columns as used by list-buffers:
'("C" "R" "M" "Buffer" "Size" "Mode" "File") from the backend,
the new frontend rendering function completion--insert-multi-column
will accept such a list of all columns, set it to the buffer-local
variable 'tabulated-list-entries', and call 'tabulated-list-print'
to render it in the output buffer *Completions*.

Such design would follow a clean backend/frontend separation that
everyone already agreed about.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/25/21 12:07 AM, Juri Linkov wrote:
> The next step would be to allow the option 'completions-format'
> to accept an arbitrary frontend rendering function.  Then a new
> possible value could be added such as:
> 
> 4. completion--insert-multi-column
> 
> So when affixation-function will return a list of multiple columns,
> e.g. for buffers the same list of columns as used by list-buffers:
> '("C" "R" "M" "Buffer" "Size" "Mode" "File") from the backend,
> the new frontend rendering function completion--insert-multi-column
> will accept such a list of all columns, set it to the buffer-local
> variable 'tabulated-list-entries', and call 'tabulated-list-print'
> to render it in the output buffer *Completions*.

It would be great, if the backend can specify arbitrary custom columns,
maybe with some hints (desired width, if it is a prefix/suffix, etc).
Then we can define some "semantic names" as standard, which should be
understood by the frontend, as Dmitry proposed. But this can happen as a
second step. What kind of `affixation-function` are you proposing, one
that returns a plist? Would such a design be acceptable for João too?

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/25/21 12:07 AM, Juri Linkov wrote:
>> The next step would be to allow the option 'completions-format'
>> to accept an arbitrary frontend rendering function.  Then a new
>> possible value could be added such as:
>> 
>> 4. completion--insert-multi-column
>> 
>> So when affixation-function will return a list of multiple columns,
>> e.g. for buffers the same list of columns as used by list-buffers:
>> '("C" "R" "M" "Buffer" "Size" "Mode" "File") from the backend,
>> the new frontend rendering function completion--insert-multi-column
>> will accept such a list of all columns, set it to the buffer-local
>> variable 'tabulated-list-entries', and call 'tabulated-list-print'
>> to render it in the output buffer *Completions*.
>
> It would be great, if the backend can specify arbitrary custom columns,
> maybe with some hints (desired width, if it is a prefix/suffix, etc).
> Then we can define some "semantic names" as standard, which should be
> understood by the frontend, as Dmitry proposed.

To clarify, the analog in some markup format is specifying something as
"emphasis" and then the renderer usually decides that bold if it can, or
surround it in asterisks.  But the markup format doesn't usually say
"bold".  For the same reasons, what you're now calling "columns" should
probably be referred to as "fields" or something equivalently agnostic
as to the format in which they are display.

> But this can happen as a second step. What kind of
> `affixation-function` are you proposing, one that returns a plist?
> Would such a design be acceptable for João too?

I have no strong feelings on plist, versus alist, versus propertized
string, versus object, versus whatever.  Just slightly strong feelings
as to the fundamental mission of this function which is to do
annotation/affixation/whatchacallit, not filtering or reordering.

For reasons of "forward compatibility", as you called them, which is the
ability for newer backends to work on older Emacs that don't understand
these new things, if that work is done in `annotation-function` then it
needs to use propertized strings.  But if it's done in a new thing, say
called decoration-function, it doesn't.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

> I have no strong feelings on plist, versus alist, versus propertized
> string, versus object, versus whatever.  Just slightly strong feelings
> as to the fundamental mission of this function which is to do
> annotation/affixation/whatchacallit, not filtering or reordering.
>
> For reasons of "forward compatibility", as you called them, which is the
> ability for newer backends to work on older Emacs that don't understand
> these new things, if that work is done in `annotation-function` then it
> needs to use propertized strings.  But if it's done in a new thing, say
> called decoration-function, it doesn't.

As I said a month ago, while affixation-function was an improvement over
annotation-function, it's still not perfect.  I welcome a better API function,
but OTOH adding prefix/suffix text properties on the annotation strings
is not an improvement.

> To clarify, the analog in some markup format is specifying something as
> "emphasis" and then the renderer usually decides that bold if it can, or
> surround it in asterisks.  But the markup format doesn't usually say
> "bold".  For the same reasons, what you're now calling "columns" should
> probably be referred to as "fields" or something equivalently agnostic
> as to the format in which they are display.

While looking for an analog, a better analogy would be the classic
MVC architecture:

- M: data comes from the list of completion strings;
- V: render functions like completion--insert-* or icomplete-completions;
- C: API callers that provide metadata functions that specify
  how to process data for displaying, e.g. they define display-sort-function
  where the prefix 'display-' hints that it affects the view,
  so the frontend has no own choice how to sort completions.

Following such pattern, affixation-function could return more fields
(e.g. when completions-format is customized to the value 'multi-column')
in the same format as tabulated-list (and an indication in which column
there are completion candidates) for the frontend to display in columns.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Juri Linkov <juri@linkov.net> writes:

> As I said a month ago, while affixation-function was an improvement over
> annotation-function, it's still not perfect.  I welcome a better API function,
> but OTOH adding prefix/suffix text properties on the annotation strings
> is not an improvement.

Sure it's not beautiful, but it's an improvement to annotation-function.
If affixation-function were a function of a single completion, it would
be fine.  For a hint at a good design, look at the language server
protocol.  It returns a large list of completions, and then you can
"resolve" a completion to get many more of its properties.  So
resolution-function is an option.

> Following such pattern, affixation-function could return more fields
> (e.g. when completions-format is customized to the value 'multi-column')
> in the same format as tabulated-list (and an indication in which column
> there are completion candidates) for the frontend to display in
> columns.

If you prefer to see it that way, I have no objection, though I prefer
to think of backend and frontend simply (i.e. your C doesn't match
anything here very accurately).

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

> Sure it's not beautiful, but it's an improvement to annotation-function.
> If affixation-function were a function of a single completion, it would
> be fine.  For a hint at a good design, look at the language server
> protocol.  It returns a large list of completions, and then you can
> "resolve" a completion to get many more of its properties.  So
> resolution-function is an option.

resolution-function sounds good.  Maybe this is not the most
suitable comparison but since you mentioned the word "resolve"
that evoked by association: resolver functions used in GraphQL.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

>> Sure it's not beautiful, but it's an improvement to annotation-function.
>> If affixation-function were a function of a single completion, it would
>> be fine.  For a hint at a good design, look at the language server
>> protocol.  It returns a large list of completions, and then you can
>> "resolve" a completion to get many more of its properties.  So
>> resolution-function is an option.
>
> resolution-function sounds good.  Maybe this is not the most
> suitable comparison but since you mentioned the word "resolve"
> that evoked by association: resolver functions used in GraphQL.

Then instead of such ad-hoc meta functions:

  :annotation-function
  (lambda (str) (if (fboundp (intern-soft str)) " <f>"))
  :company-kind #'elisp--company-kind
  :company-doc-buffer #'elisp--company-doc-buffer
  :company-docsig #'elisp--company-doc-string
  :company-location #'elisp--company-location))

we could have one function

  :resolution-function
  (lambda (str fields)
    (mapcar (lambda (field)
              (pcase field
                ('suffix (if (fboundp (intern-soft str)) " <f>"))
                ('kind (elisp--company-kind str))
                ('doc-buffer (elisp--company-doc-buffer str))
                ('docsig (elisp--company-doc-string str))
                ('location (elisp--company-location str))))
            fields))

called with e.g.

  (funcall (completion-metadata-get md 'resolution-function)
           '(prefix suffix location kind))

When 'prefix' is not implemented by the backend, it just returns 'nil'.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 27.05.2021 00:45, Juri Linkov wrote:
> we could have one function
> 
>    :resolution-function
>    (lambda (str fields)
>      (mapcar (lambda (field)
>                (pcase field
>                  ('suffix (if (fboundp (intern-soft str)) " <f>"))
>                  ('kind (elisp--company-kind str))
>                  ('doc-buffer (elisp--company-doc-buffer str))
>                  ('docsig (elisp--company-doc-string str))
>                  ('location (elisp--company-location str))))
>              fields))

The downside should be obvious: you waste extra CPU and memory when you 
only need some of these values, and not all of them.

Which is the case most of the time, IME. That is, for one command I only 
need 'doc-buffer', for another - 'location', 'suffix' and 'kind' might 
be displayed together (though in your example they are duplicating one 
another) on one idle timer, 'docsig' is used in another idle timer.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, May 26, 2021 at 11:20 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 27.05.2021 00:45, Juri Linkov wrote:
> > we could have one function
> >
> >    :resolution-function
> >    (lambda (str fields)
> >      (mapcar (lambda (field)
> >                (pcase field
> >                  ('suffix (if (fboundp (intern-soft str)) " <f>"))
> >                  ('kind (elisp--company-kind str))
> >                  ('doc-buffer (elisp--company-doc-buffer str))
> >                  ('docsig (elisp--company-doc-string str))
> >                  ('location (elisp--company-location str))))
> >              fields))
>
> The downside should be obvious: you waste extra CPU and memory when you
> only need some of these values, and not all of them.

I don't see why, though.  Isn't the `fields` arg supposed to solve just
that problem?  Or am I missing something?

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 27.05.2021 02:17, João Távora wrote:
> On Wed, May 26, 2021 at 11:20 PM Dmitry Gutov<dgutov@yandex.ru>  wrote:
>> On 27.05.2021 00:45, Juri Linkov wrote:
>>> we could have one function
>>>
>>>     :resolution-function
>>>     (lambda (str fields)
>>>       (mapcar (lambda (field)
>>>                 (pcase field
>>>                   ('suffix (if (fboundp (intern-soft str)) " <f>"))
>>>                   ('kind (elisp--company-kind str))
>>>                   ('doc-buffer (elisp--company-doc-buffer str))
>>>                   ('docsig (elisp--company-doc-string str))
>>>                   ('location (elisp--company-location str))))
>>>               fields))
>> The downside should be obvious: you waste extra CPU and memory when you
>> only need some of these values, and not all of them.
> I don't see why, though.  Isn't the `fields` arg supposed to solve just
> that problem?  Or am I missing something?

OK, that was poor reading on my part (missed 'pcase').

What would such structure buy us, though?

My objections would be less fundamental here, but:

- Harder to mix and match different implementations for these fields 
(look how elisp-completion-at-point is implemented, using different sets 
of extra-properties in different branches of its execution).

- Harder to know whether a particular field is implemented/present. 
Currently you fetch the necessary function with 'plist-get', and you can 
test the returned value for non-nil. Without that, each function cannot 
return nil to mean something different than its absense. So you can't 
have 'true' as default value for any of those fields.

Also, the above piece of code looks awfully similar to a Company backend 
or frontend definition, which you have disparaged on multiple occasions.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Thu, May 27, 2021 at 2:06 AM Dmitry Gutov <dgutov@yandex.ru> wrote:

> >> The downside should be obvious: you waste extra CPU and memory when you
> >> only need some of these values, and not all of them.
> > I don't see why, though.  Isn't the `fields` arg supposed to solve just
> > that problem?  Or am I missing something?
> OK, that was poor reading on my part (missed 'pcase').
> Also, the above piece of code looks awfully similar to a Company backend
> or frontend definition, which you have disparaged on multiple occasions.

[ Uncalled for, right?  Let's keep cool with the animosity and
the vague accusations, stick to the topic. ]

Here, I was referring specifically to your "obvious" objection, which
seemed nonsensical and ended up just being a reading mistake, that's
fine.

As to ways to improve on Juri's idea, I would maybe suggest a generic function
of string and a single field.  Much like in `affixation-function`,
there's no need to ask
the backend the `mapcar`.  To know what a backend implements would
mean asking that generic function for its methods.

And if you want to "mix and match", a generic function with the backend
as its arg and a hierarchy of backends seems the most elegant choice.
So `:resolution-function` wouldn't be written like that at all, the
backend would
write methods for that `completion-resolution` gf.
But that's redesigning way too much and has numerous other implications
and experience tells me it's not going anywhere and we'd not solve
the annotation problem which is the topic here.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 27.05.2021 10:29, João Távora wrote:
> On Thu, May 27, 2021 at 2:06 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
> 
>>>> The downside should be obvious: you waste extra CPU and memory when you
>>>> only need some of these values, and not all of them.
>>> I don't see why, though.  Isn't the `fields` arg supposed to solve just
>>> that problem?  Or am I missing something?
>> OK, that was poor reading on my part (missed 'pcase').
>> Also, the above piece of code looks awfully similar to a Company backend
>> or frontend definition, which you have disparaged on multiple occasions.
> 
> [ Uncalled for, right?  Let's keep cool with the animosity and
> the vague accusations, stick to the topic. ]

I don't exactly mean to initiate a shouting match here. Someone liking 
(or not) a particular API design can be pertinent, because people are 
going to have to use it, after all.

> Here, I was referring specifically to your "obvious" objection, which
> seemed nonsensical and ended up just being a reading mistake, that's
> fine.

Yup.

> As to ways to improve on Juri's idea, I would maybe suggest a generic function
> of string and a single field.  Much like in `affixation-function`,
> there's no need to ask
> the backend the `mapcar`.  To know what a backend implements would
> mean asking that generic function for its methods.

I don't think this can work without doing the long-discussed migration 
of the completion-table API (or c-a-p-f API) to generic functions.

Upon which we'll lose the capability to mix-and-match using immediate 
values and refer to containing lexical scopes in implementations anyway 
(while gaining in comprehensibility, structure and better code navigation).

Yet still, even when using generic functions, why would we create an 
additional dispatcher mechanism when cl-generic can already do that for 
us? To reduce the number of methods in the API description?

> And if you want to "mix and match", a generic function with the backend
> as its arg and a hierarchy of backends seems the most elegant choice.
> So `:resolution-function` wouldn't be written like that at all, the
> backend would
> write methods for that `completion-resolution` gf.
> But that's redesigning way too much and has numerous other implications
> and experience tells me it's not going anywhere and we'd not solve
> the annotation problem which is the topic here.

Yes, it's orthogonal to the more pressing questions of how information 
returned by 'affixation', etc, should look.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Thu, May 27, 2021 at 2:15 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 27.05.2021 10:29, João Távora wrote:
> > On Thu, May 27, 2021 at 2:06 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
> >
> >>>> The downside should be obvious: you waste extra CPU and memory when you
> >>>> only need some of these values, and not all of them.
> >>> I don't see why, though.  Isn't the `fields` arg supposed to solve just
> >>> that problem?  Or am I missing something?
> >> OK, that was poor reading on my part (missed 'pcase').
> >> Also, the above piece of code looks awfully similar to a Company backend
> >> or frontend definition, which you have disparaged on multiple occasions.
> > [ Uncalled for, right?  Let's keep cool with the animosity and
> > the vague accusations, stick to the topic. ]
> I don't exactly mean to initiate a shouting match here.

Good, then let's not bring up past ones for absolutely no reason.

> I don't think this can work without doing the long-discussed migration

I don't understand this.  I was just suggesting that using a small
variation of Juri's idea, the backend may want to use:

   :resolution-function #'im-backend-x-and-this-is-my-gf

Instead of:

   :resolution-function (lambda (str field) (pcase field ...))

Then, you could easily inspect the methods of
'im-backend-x-and-this-is-my-gf' to see what fields it supports.  The
signature of the gf is:

   (defgeneric im-backend-x-and-this-is-my-gf (str field) ...)

Additionally, and anticipating a possible follow-up question, to share
information between different methods of the same GF (for performance
reasons and only if they indeed exist) one can use a second API point
that is called to let-bind special variables accessed with an argless
function.  I've used this pattern successfully in CLOS many times.

> Yet still, even when using generic functions, why would we create an
> additional dispatcher mechanism when cl-generic can already do that for
> us? To reduce the number of methods in the API description?

We wouldn't.  I _am_ suggesting using cl-generic:

I also tried to express that if we redesigned the method, instead of
'im-backend-x-and-this-is-my-gf', you could have 'this-is-the-resolution-gf'
with a signature that takes the table (aka backend) as an argument:

   (defgeneric this-is-the-resolution-gf (backend field str) ...)

And if backends were CLOS object you could leverage inheritance to mix
and match stuff between different backends.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 27.05.2021 17:19, João Távora wrote:

> I  was just suggesting that using a small
> variation of Juri's idea, the backend may want to use:
> 
>     :resolution-function #'im-backend-x-and-this-is-my-gf
> 
> Instead of:
> 
>     :resolution-function (lambda (str field) (pcase field ...))
> 
> Then, you could easily inspect the methods of
> 'im-backend-x-and-this-is-my-gf' to see what fields it supports.  The
> signature of the gf is:
> 
>     (defgeneric im-backend-x-and-this-is-my-gf (str field) ...)

I was talking about detecting the lack of such support programmatically. 
At the moment we can do that with (null (plist-get ...)). With generic 
functions, one can get that by catching cl-no-applicable-method.

I was thinking the definitions in your proposal would look like

(defgeneric im-backend-resolve ((head 'some str) field)
   <the field is dispatched dynamically on>)

, but the definitions could also look like

(defgeneric im-backend-resolve ((head 'some str) (eql 'a field) str)
   <the implementation for field-a>)

(defgeneric im-backend-resolve ((head 'some str) (eql 'b field) str)
   <the implementation for field-a>)

...then we'll get the same error if the field is not implemented.

But why choose that instead of

(defgeneric im-backend-field-a ((head 'some str) str)
   <the implementation for field-a>)

(defgeneric im-backend-field-b ((head 'some str) str)
   <the implementation for field-a>)

?

> Additionally, and anticipating a possible follow-up question, to share
> information between different methods of the same GF (for performance
> reasons and only if they indeed exist) one can use a second API point
> that is called to let-bind special variables accessed with an argless
> function.  I've used this pattern successfully in CLOS many times.

Passing context info through dynamic vars?

>> Yet still, even when using generic functions, why would we create an
>> additional dispatcher mechanism when cl-generic can already do that for
>> us? To reduce the number of methods in the API description?
> 
> We wouldn't.  I _am_ suggesting using cl-generic:
> 
> I also tried to express that if we redesigned the method, instead of
> 'im-backend-x-and-this-is-my-gf', you could have 'this-is-the-resolution-gf'
> with a signature that takes the table (aka backend) as an argument:
> 
>     (defgeneric this-is-the-resolution-gf (backend field str) ...)
> 
> And if backends were CLOS object you could leverage inheritance to mix
> and match stuff between different backends.

The difference is that all combinations would need to be anticipated in 
backend definitions in advance.

Anyway, like I said, this discussion seems premature. There is little 
point in complicating the c-a-p-f API with generic functions unless we 
also use them for core functionality.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Thu, May 27, 2021 at 7:52 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 27.05.2021 17:19, João Távora wrote:

> I was talking about detecting the lack of such support programmatically.
> At the moment we can do that with (null (plist-get ...)). With generic
> functions, one can get that by catching cl-no-applicable-method.

That's the "poor man's" way to do it.  I don't know if eieio supports
it but CLOS and the AMOP have many more ways to inspect
a generic function.  Anyway, if this is extremely necessary we could add
it to eieio.el.

> I was thinking the definitions in your proposal would look like
>
> (defgeneric im-backend-resolve ((head 'some str) field)
>    <the field is dispatched dynamically on>)
>
> , but the definitions could also look like

That's not how methods on generic functions are
defined, at least not in CLOS, but anyway this is what I mean.

(defmethod im-backend-resolve (str (field (eql 'a-very-interesting field))
  ...
   )

The frontend will call :resolution-function multiple times for the fields
it can layout.  A default method can return nil, or it can signal, or
something else, depending on the API.  (I think returning nil for "I
don't have this field" is the right choice here).

But bear in mind that using generic functions is just one of the backend's
options to implement this.  If backend wants to use regular functions like
Juri suggested, any other dispatching method, it can.  The frontend
has no say on the particular dispatching technique, and that's a good
thing.

This contrasts with the other, more complex and ambitious option,
where the backend adds methods to a generic function that does not
belong to it. That generic functions would have three arguments instead
of two, of course.  If some of those arguments are CLOS classes, the
standard method combinations can do very fancy things in terms
of code reuse.  But I don't recommend this approach now to solve _this_
problem.

> > Additionally, and anticipating a possible follow-up question, to share
> > information between different methods of the same GF (for performance
> > reasons and only if they indeed exist) one can use a second API point
> > that is called to let-bind special variables accessed with an argless
> > function.  I've used this pattern successfully in CLOS many times.
> Passing context info through dynamic vars?

They are an implementation detail, but also the easiest way to
establish context. Normally I make them internal, and have
clients read, not write them, via an exported function.  As such,
that function can use any implementation detail it wants.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/26/21 11:45 PM, Juri Linkov wrote:
> Then instead of such ad-hoc meta functions:
> 
>   :annotation-function
>   (lambda (str) (if (fboundp (intern-soft str)) " <f>"))
>   :company-kind #'elisp--company-kind
>   :company-doc-buffer #'elisp--company-doc-buffer
>   :company-docsig #'elisp--company-doc-string
>   :company-location #'elisp--company-location))
> 
> we could have one function
> 
>   :resolution-function
>   (lambda (str fields)
>     (mapcar (lambda (field)
>               (pcase field
>                 ('suffix (if (fboundp (intern-soft str)) " <f>"))
>                 ('kind (elisp--company-kind str))
>                 ('doc-buffer (elisp--company-doc-buffer str))
>                 ('docsig (elisp--company-doc-string str))
>                 ('location (elisp--company-location str))))
>             fields))
> 
> called with e.g.
> 
>   (funcall (completion-metadata-get md 'resolution-function)
>            '(prefix suffix location kind))
> 
> When 'prefix' is not implemented by the backend, it just returns 'nil'.

The proposal of a `resolution-function` is an over generalization.
Throwing orthogonal functionality together in a single function will
hurt composability and will make the design less flexible. There is
already `metadata` which can be extended. Therefore there is no need to
add such an arbitrary query functionality in the form of a
`resolution-function`. The additional fields 'docsig, 'location and
'doc-buffer should not be added here.

A better design for the problem of annotations, which we are actually
discussing here is a `decoration-function` as proposed João. This
decoration function has the sole purpose of providing additional
decorations. Unrelated features should be provided by unrelated
additional functions. The decoration function should return a
plist/alist of all decoration fields. Then one may want to add a
`fields` argument to restrict the set fields in case the UI only needs
the prefix and suffix. A tabular list UI can then present all the fields
as returned by the decoration function.

If a design like the `resolution-function` from above is used, the UI
would have to somehow check the type of each field value to determine if
and how it should be displayed. This does not look like a good and
simple solution to me.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:
>
> A better design for the problem of annotations, which we are actually
> discussing here is a `decoration-function` as proposed João. This
> decoration function has the sole purpose of providing additional
> decorations. Unrelated features should be provided by unrelated
> additional functions. The decoration function should return a
> plist/alist of all decoration fields. Then one may want to add a
> `fields` argument to restrict the set fields in case the UI only needs
> the prefix and suffix. A tabular list UI can then present all the fields
> as returned by the decoration function.

There has to be a misunderstanding here, because naming aside, this is
exactly how I read Juri's proposal of a resolution-function, which I'm
fine to call decoration-function or fancification-function etc.

The only tiny thing I'm not in agreement with is the signature.  Mind
that it works fine with Juri's idea, but it requires the backend
programmer to be careful of its mapcar (this is exactly the same
foot-shoot situation that Stefan understates and I overstate).

But that's not a showstopper, I propose this small variation.
In the backend:

    :resolution-function
    (lambda (str field)
      (pcase field
             ('suffix (if (fboundp (intern-soft str)) " <f>"))
             ('kind (elisp--company-kind str))
             ('doc-buffer (elisp--company-doc-buffer str))
             ('docsig (elisp--company-doc-string str))
             ('location (elisp--company-location str))))

The frontends then decides what value of FIELD to pass to their multiple
invocations of this function for each completion.

> If a design like the `resolution-function` from above is used, the UI
> would have to somehow check the type of each field value to determine if
> and how it should be displayed. This does not look like a good and
> simple solution to me.

Again, I can't understand this.  In the frontend:

    (let ((res-fun
             (or (completion-metadata-get md resolution-function)
                 (plist-get completion-extra-properties :resolution-function))))
      (let* ((fields-i-know-how-to-handle '(suffix location))
             (str candidate-i-am-about-to-decorate)
             (field-values
               (mapcar (lambda (f) (funcall res-fun str)) fields-i-know-how-to-handle)))
         ...))


João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/28/21 10:34 AM, João Távora wrote:
> Daniel Mendler <mail@daniel-mendler.de> writes:
>>
>> A better design for the problem of annotations, which we are actually
>> discussing here is a `decoration-function` as proposed João. This
>> decoration function has the sole purpose of providing additional
>> decorations. Unrelated features should be provided by unrelated
>> additional functions. The decoration function should return a
>> plist/alist of all decoration fields. Then one may want to add a
>> `fields` argument to restrict the set fields in case the UI only needs
>> the prefix and suffix. A tabular list UI can then present all the fields
>> as returned by the decoration function.
> 
> There has to be a misunderstanding here, because naming aside, this is
> exactly how I read Juri's proposal of a resolution-function, which I'm
> fine to call decoration-function or fancification-function etc.

No, there is no misunderstanding here. The example sent by Juri contains
'doc-buffer, 'docsig and 'location, which are all Company extensions and
which cannot be considered annotations or decorations.

The name `resolution-function` does not suggest that the function is
supposed to return annotations or decorations. It can return anything -
this is the over-generalization I am talking about.

> Again, I can't understand this.  In the frontend:
> 
>     (let ((res-fun
>              (or (completion-metadata-get md resolution-function)
>                  (plist-get completion-extra-properties :resolution-function))))
>       (let* ((fields-i-know-how-to-handle '(suffix location))
>              (str candidate-i-am-about-to-decorate)
>              (field-values
>                (mapcar (lambda (f) (funcall res-fun str)) fields-i-know-how-to-handle)))
>          ...))

This is not an extensible design. I would like that the frontend has the
ability to show arbitrary fields, not only fields it already knows of.
Then the completion UI can for example display the supplied annotations
in a tablist as columns. The backend can decide and extend the list of
annotation fields itself. This makes sense since the UI cannot foresee
all kinds of semantic annotation fields up front.

So to summarize - I consider the current design lacking:

1. It is not extensible. The backend cannot define fields.
2. It is over-generalized, since the function returns not only
annotations but also arbitrary data.

There is no point in the over-generalized design, since metadata is
already extensible with functions or data. Going via the
`resolution-function` only introduces a needless complication and
indirection.

I am proposing the addition of a `decoration-function` which has the
single concern of providing annotations/decorations. The simplest design
from is something like a generalized `affixation-function`:

(funcall decoration-function "candidate") --> (:candidate
"formatted-candidate" :prefix "prefix" :suffix "default suffix?"
:my-annotation1 "suffix" :my-annotation2 "suffix2" ...)

Then the UI can present prefix, formatted candidate and suffix and the
additional annotations in a tablist. Maybe it is not necessary to
specify a `:suffix` field - the additional annotations could be treated
as suffixes in the default UI.

I hope we can back to a simpler design here, which solves the problems
we are having at the moment - adding annotations or even only prefixes
and suffixes.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/28/21 10:34 AM, João Távora wrote:
>> Daniel Mendler <mail@daniel-mendler.de> writes:
>>>
>>> A better design for the problem of annotations, which we are actually
>>> discussing here is a `decoration-function` as proposed João. This
>>> decoration function has the sole purpose of providing additional
>>> decorations. Unrelated features should be provided by unrelated
>>> additional functions. The decoration function should return a
>>> plist/alist of all decoration fields. Then one may want to add a
>>> `fields` argument to restrict the set fields in case the UI only needs
>>> the prefix and suffix. A tabular list UI can then present all the fields
>>> as returned by the decoration function.
>> 
>> There has to be a misunderstanding here, because naming aside, this is
>> exactly how I read Juri's proposal of a resolution-function, which I'm
>> fine to call decoration-function or fancification-function etc.
>
> No, there is no misunderstanding here. The example sent by Juri contains
> 'doc-buffer, 'docsig and 'location, which are all Company extensions and
> which cannot be considered annotations or decorations.

Really?  The names of the function mention Company, because at the time
company frontend was their only consumer of them, but the point here is
to make other frontends consume them.  Anyway, as I understood it they
were only examples.

>
> The name `resolution-function` does not suggest that the function is
> supposed to return annotations or decorations.

Ok, change the name.  Name it `decoration-function` or `annotation-function`.

> It can return anything - this is the over-generalization I am talking
> about.

I like that it can return arbitrary pieces of information attached to a
single completion.  That is a generalization, I don't understand the
"over" problem.

>> Again, I can't understand this.  In the frontend:
>> 
>>     (let ((res-fun
>>              (or (completion-metadata-get md resolution-function)
>>                  (plist-get completion-extra-properties :resolution-function))))
>>       (let* ((fields-i-know-how-to-handle '(suffix location))
>>              (str candidate-i-am-about-to-decorate)
>>              (field-values
>>                (mapcar (lambda (f) (funcall res-fun str)) fields-i-know-how-to-handle)))
>>          ...))
>
> This is not an extensible design. I would like that the frontend has the
> ability to show arbitrary fields, not only fields it already knows of.

That's generally impossible.  How can a frontend know how to display the
field 'magical-animated-gif-boomerang-video-url' I'm thinking of
attaching to the completions of my completion backend?  Eventually, very
fancy frontends will be able to do that, and it's my job to convince
them that this is a super-important piece of information to show the
user, but some frontends can't or won't show video for example.

To restate, now that I have Icomplete mostly fixed for verticality and
nice scrolling behaviour and also reworked for easy annotation support,
I think Juri's idea is a fine one (with the cardinality adjustment --
one field per call).

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/28/21 12:09 PM, João Távora wrote:
> Really?  The names of the function mention Company, because at the time
> company frontend was their only consumer of them, but the point here is
> to make other frontends consume them.  Anyway, as I understood it they
> were only examples.

Well, the point here is that these are not annotations, which are not
meant to be displayed. So what are we talking here about.

>> The name `resolution-function` does not suggest that the function is
>> supposed to return annotations or decorations.
> 
> Ok, change the name.  Name it `decoration-function` or `annotation-function`.

Then I am more happy with the proposal.

>> It can return anything - this is the over-generalization I am talking
>> about.
> 
> I like that it can return arbitrary pieces of information attached to a
> single completion.  That is a generalization, I don't understand the
> "over" problem.
>
>> This is not an extensible design. I would like that the frontend has the
>> ability to show arbitrary fields, not only fields it already knows of.
> 
> That's generally impossible.  How can a frontend know how to display the
> field 'magical-animated-gif-boomerang-video-url' I'm thinking of
> attaching to the completions of my completion backend?  Eventually, very
> fancy frontends will be able to do that, and it's my job to convince
> them that this is a super-important piece of information to show the
> user, but some frontends can't or won't show video for example.

It is not impossible as long as we require the annotations to be
strings. The affixation function also returns strings.

Alternatively we can specify that the function may return other types
which are then up to the interpretation of the UI.

There should be a possibility for the UI to request all annotations and
then it can display the fields it supports.

Did you see my submission of the GNU ELPA Marginalia package? The
package annotates the candidates with multiple annotations which are
currently displayed in columns. But this formatting happens inside
Marginalia. It would be great if Marginalia could instead return an
arbitrary list of annotations which are then formatted by the frontend,
for example in a table. The list of annotation fields is not fixed.

> To restate, now that I have Icomplete mostly fixed for verticality and
> nice scrolling behaviour and also reworked for easy annotation support,
> I think Juri's idea is a fine one (with the cardinality adjustment --
> one field per call).
From my side the changes in your icomplete-vertical-mode-improvements
branch are fine.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/28/21 12:09 PM, João Távora wrote:
>> Really?  The names of the function mention Company, because at the time
>> company frontend was their only consumer of them, but the point here is
>> to make other frontends consume them.  Anyway, as I understood it they
>> were only examples.
>
> Well, the point here is that these are not annotations, which are not
> meant to be displayed. So what are we talking here about.

Let's see an example: we are talking about elisp--company-doc-string for
'docsig' or elisp--company-location for 'location', to pick two from
Juri's original example.

Both can be displayed, for some meaning of display, it's up to the
frontend to decide _how_ to display it.  In Icomplete:

- 'docsig' could be displayed right-aligned for example, depending on
  available space.

- 'location' could be linkeds to a special keybinding and command that
  would pop a buffer with that location.

Other frontends could do other things with it.  The Company frontend
already has quite specific ways to handle these things, I suppose.

>> That's generally impossible.  How can a frontend know how to display the
>> field 'magical-animated-gif-boomerang-video-url' I'm thinking of
>> attaching to the completions of my completion backend?  Eventually, very
>> fancy frontends will be able to do that, and it's my job to convince
>> them that this is a super-important piece of information to show the
>> user, but some frontends can't or won't show video for example.
>
> It is not impossible as long as we require the annotations to be
> strings. The affixation function also returns strings.

Yes, but this does more than merely affix strings before and after other
strings.  If we insist on "affixation function", we are constraining the
layout from within the backend, and a good design principle is not good
to mix models and presentation ("MCV" design if you're into ~90's
acronyms).

But if you're skeptic, don't take this weak argument of authority,
observes how it really solves our problem here (and many other
problems).

> Alternatively we can specify that the function may return other types
> which are then up to the interpretation of the UI.
>
> There should be a possibility for the UI to request all annotations and
> then it can display the fields it supports.

Why would the UI request annotations for fields it _doesn't_ support?
In that case I have to side with Dmitry, who says it's a waste of
processing.  Why would Icomplete request 'doc-buffer', which is
potentially slow and needs a network request in some backend, if it
doesn't need to show it, or even know how to show it?

> Did you see my submission of the GNU ELPA Marginalia package? The
> package annotates the candidates with multiple annotations which are
> currently displayed in columns. But this formatting happens inside
> Marginalia. It would be great if Marginalia could instead return an
> arbitrary list of annotations which are then formatted by the frontend,
> for example in a table. The list of annotation fields is not fixed.

I don't know Marginalia, sorry.  You want a catch-all for arbitrary
string annotations that some frontends will do their best to display in
a table? If so, in this scheme, make a field called 'lotsa-strings (or
some better name, maybe "marginalia" is a good one).  Then convince
frontends (maybe starting with the ones you control) to give prominence
to that field over other ones.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/28/21 1:41 PM, João Távora wrote:
>> There should be a possibility for the UI to request all annotations and
>> then it can display the fields it supports.
> 
> Why would the UI request annotations for fields it _doesn't_ support?
> In that case I have to side with Dmitry, who says it's a waste of
> processing.  Why would Icomplete request 'doc-buffer', which is
> potentially slow and needs a network request in some backend, if it
> doesn't need to show it, or even know how to show it?

I would like the backend to have the ability to define arbitrary fields
which are strings and can be printed by the frontend for example in a
table. There is a difference between supporting specific fields or
specific field types. I would like to have the ability for the frontend
to specify arbitrary fields of field type string. Then a frontend can
display those in a tablist for example.

Juri talked about exactly this use case. He considered adding a
formatting function for the default completion UI which uses the tablist.

The "waste of processing" argument by Dmitry is okay - I am not against
a way to limit the number of returned fields. But there should also be a
way to request all available fields, such that a UI can show the ones it
supports (but not based on name but based on type!).

>> Did you see my submission of the GNU ELPA Marginalia package? The
>> package annotates the candidates with multiple annotations which are
>> currently displayed in columns. But this formatting happens inside
>> Marginalia. It would be great if Marginalia could instead return an
>> arbitrary list of annotations which are then formatted by the frontend,
>> for example in a table. The list of annotation fields is not fixed.
> 
> I don't know Marginalia, sorry.  You want a catch-all for arbitrary
> string annotations that some frontends will do their best to display in
> a table? If so, in this scheme, make a field called 'lotsa-strings (or
> some better name, maybe "marginalia" is a good one).  Then convince
> frontends (maybe starting with the ones you control) to give prominence
> to that field over other ones.

What about taking a look then? Convincing the frontends on a
case-by-case basis is not a workable solution. You are the good example
here, your answer is "I don't know Marginalia", so how are you supposed
to be convinced?

All I am asking is a possibility for the backend to return a list of
string fields which can then be displayed by frontends which support
such a tabular view, or maybe a tooltip.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 5/28/21 1:41 PM, João Távora wrote:
>>> There should be a possibility for the UI to request all annotations and
>>> then it can display the fields it supports.
>> 
>> Why would the UI request annotations for fields it _doesn't_ support?
>> In that case I have to side with Dmitry, who says it's a waste of
>> processing.  Why would Icomplete request 'doc-buffer', which is
>> potentially slow and needs a network request in some backend, if it
>> doesn't need to show it, or even know how to show it?
>
> I would like the backend to have the ability to define arbitrary fields
> which are strings and can be printed by the frontend for example in a
> table. There is a difference between supporting specific fields or
> specific field types. I would like to have the ability for the frontend
> to specify arbitrary fields of field type string. Then a frontend can
> display those in a tablist for example.

OK, I think understand and I already addressed this.

The frontend/backend protocol being discussed doesn't preclude your
intentions, in any way, I think As I said, make a "marginalia" field,
specified to take the value of a list of strings.  But then you have to
convince frontends to support it.  Icomplete supports 'prefix and
'suffix right now, make a patch to support 'marginalia.  We'll figure
out the relatively priority if a backend wants to return both
'marginalia and 'prefix.

> The "waste of processing" argument by Dmitry is okay - I am not against
> a way to limit the number of returned fields. 

It's more than just "okay": it's pretty fundamental. I don't think it's
a question of limiting the number of returned fields, if the frontend is
unlucky one of those fields may take ages to calculate based on the
processing needed (many backends operate through networks or processes
to request information about completions).

> What about taking a look then? Convincing the frontends on a
> case-by-case basis is not a workable solution. You are the good example
> here, your answer is "I don't know Marginalia", so how are you supposed
> to be convinced?

Heh, :-) I think it is rather you who have to state the relevance of
that item to this discussion.  There are lots of packages out there.
More than "what does it do" we want to know how it is positively or
adversely impacted by the decisions taken here, if at all.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/28/21 2:15 PM, João Távora wrote:
> OK, I think understand and I already addressed this.
> 
> The frontend/backend protocol being discussed doesn't preclude your
> intentions, in any way, I think As I said, make a "marginalia" field,
> specified to take the value of a list of strings.  But then you have to
> convince frontends to support it.  Icomplete supports 'prefix and
> 'suffix right now, make a patch to support 'marginalia.  We'll figure
> out the relatively priority if a backend wants to return both
> 'marginalia and 'prefix.

Unfortunately this is not a satisfactory solution. I would like to
decouple the components. It is better if the frontend does not have to
know everything and all the field names of the backend.

I am not speaking against special casing certain named fields in the
frontend if the frontend wants to give those fields special support.
This is perfectly fine as in the case of the Company fields.

I am talking about the common case of arbitrary fields of type
string/number/..., which can be supported automatically by a frontend
which can show (possibly truncated) strings.

So no, you did not address this. You enforce a strong coupling between
backend and frontend.

>> The "waste of processing" argument by Dmitry is okay - I am not against
>> a way to limit the number of returned fields. 
> 
> It's more than just "okay": it's pretty fundamental. I don't think it's
> a question of limiting the number of returned fields, if the frontend is
> unlucky one of those fields may take ages to calculate based on the
> processing needed (many backends operate through networks or processes
> to request information about completions).

It is still a technical detail. But since we agree on this one, there is
no need to discuss this further. Of course there can be expensive
annotations, where this matters a lot.

For example in Marginalia we take some care to not compute particularly
expensive annotations. But it is still way too costly to compute all
annotations of all candidates at once. Marginalia works best when the
frontend requests only a small number of annotations for the small
subset of candidates being displayed, as is the case in Vertico or your
newly revamped Icomplete-vertical-mode.

>> What about taking a look then? Convincing the frontends on a
>> case-by-case basis is not a workable solution. You are the good example
>> here, your answer is "I don't know Marginalia", so how are you supposed
>> to be convinced?
> 
> Heh, :-) I think it is rather you who have to state the relevance of
> that item to this discussion.  There are lots of packages out there.
> More than "what does it do" we want to know how it is positively or
> adversely impacted by the decisions taken here, if at all.

Of course, but as soon as I point you to a package specifically you can
take a quick look ;) It is just a datapoint in the discussion.

Marginalia would not be adversely affected by the `decoration-function`.
But there would also be zero positive impact in comparison to the
`affixation-function`. From what I've seen, Marginalia could use a
`decoration-function` returning a suffix and prefix, which would result
in just a slightly different implementation than the current  one using
the `affixation-function`.

If the `decoration-function` would be allowed to return arbitrary
fields, then the UI could take care to list all these fields in a
tablist. Of course there are other possible UIs. But let's just pick
that one.

For example in the case of `describe-symbol`, there would be these columns:

* keybinding (only supplied for commands)
* type (customizable-variable, command, function, variable, face, ...)
* description (first line of docstring)
* maybe more?

So it would be a positive impact if candidates could be annotated with
arbitrary fields, which the frontend does not know beforehand. The only
thing the frontend must support is the type of the fields. There could
be frontends supporting strings, numbers, images, audio, ...

From how I understood Juri in a mail a while ago, he was talking about
exactly such a tablist frontend. So I would like to hear his opinion on
this question.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

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

On Fri, May 28, 2021, 13:32 Daniel Mendler <mail@daniel-mendler.de> wrote:

>
> Unfortunately this is not a satisfactory solution. I would like to
> decouple the components. It is better if the frontend does not have to
> know everything and all the field names of the backend.
>

Of course. And this doesn't happen.

>
If some filtering inside the marginalia is needed, just make the marginalia
field be a plist instead of a flat list.
Dmitry's completion-info-columns-function is the same idea in a separate
API. Also works, but I prefer this one, as columns is constraining design
on the backend side.

So no, you did not address this. You enforce a strong coupling between
> backend and frontend.
>

Check again.

>> The "waste of processing" argument by Dmitry is okay - I am not against
> >> a way to limit the number of returned fields.
> >
> > It's more than just "okay": it's pretty fundamental. I don't think it's
> > a question of limiting the number of returned fields, if the frontend is
> > unlucky one of those fields may take ages to calculate based on the
> > processing needed (many backends operate through networks or processes
> > to request information about completions).
>
> It is still a technical detail. But since we agree on this one, there is
> no need to discuss this further.


I don't know if you understand that same frontends might misfunction if
they request your full list of marginalia data and the backend has included
a slow field there. So the free-form marginalia or table plist should be
specified to be very quick to calculate.

For example in Marginalia we take some care to not compute particularly
> expensive annotations.


Which is a drawback. A frontend with async capabilities can choose to
request the expensive ones, too, for example.

But it is still way too costly to compute all
> annotations of all candidates at once. Marginalia works best when the
> frontend requests only a small number of annotations for the small
> subset of candidates being displayed, as is the case in Vertico or your
> newly revamped Icomplete-vertical-mode.
>

Yes but this is besides the point.

> Heh, :-) I think it is rather you who have to state the relevance of
> > that item to this discussion.  There are lots of packages out there.
> > More than "what does it do" we want to know how it is positively or
> > adversely impacted by the decisions taken here, if at all.
>
> Of course, but as soon as I point you to a package specifically you can
> take a quick look ;) It is just a datapoint in the discussion.
>

I did take a quick look, but I was confused. I couldn't confidently make
any assertion about it based on the knowledge I gathered from looking at
its README.

[-- Attachment #2: Type: text/html, Size: 4854 bytes --]

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/28/21 3:17 PM, João Távora wrote:
> If some filtering inside the marginalia is needed, just make the
> marginalia field be a plist instead of a flat list.
> Dmitry's completion-info-columns-function is the same idea in a separate
> API. Also works, but I prefer this one, as columns is constraining
> design on the backend side.
> 
>     So no, you did not address this. You enforce a strong coupling between
>     backend and frontend.
> 
> 
> Check again. 

You are talking about the introduction of a `marginalia` field. Such a
field should not be necessary. The frontend should not have to know
about the existence of a `marginalia` field.

What you probably want to say here is that the name does not matter and
we could also call this generically `auxiliary-annotation-columns` and
then the frontends could support this field containing another list of
fields if the frontend likes that or not.

>     For example in Marginalia we take some care to not compute particularly
>     expensive annotations.
> 
> 
> Which is a drawback. A frontend with async capabilities can choose to
> request the expensive ones, too, for example. 

Of course. I am not against such a query based API. But one should take
into account that other expensive metadata can be provided via other
query functions.

There is no advantage in requesting some `expensive-field` from a
`resolution/decoration-function` in contrast to calling an
`expensive-field-function` which is specified in the metadata.

I fail to see what the hypothetical `resolution/decoration-function`
improves. The use case with the columns I described won't be supported
out of the box. Prefix/suffix is supported with the only change over the
affixation function that the prefix/suffix must be requested explicitly.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

> For example in the case of `describe-symbol`, there would be these columns:
>
> * keybinding (only supplied for commands)
> * type (customizable-variable, command, function, variable, face, ...)
> * description (first line of docstring)
> * maybe more?
>
> So it would be a positive impact if candidates could be annotated with
> arbitrary fields, which the frontend does not know beforehand. The only
> thing the frontend must support is the type of the fields. There could
> be frontends supporting strings, numbers, images, audio, ...
>
> From how I understood Juri in a mail a while ago, he was talking about
> exactly such a tablist frontend. So I would like to hear his opinion on
> this question.

1. A simple call with a string candidate could return its supported fields:

(funcall resolution/decoration-function "candidate")
⇒
(:prefix "prefix" :suffix "suffix" :my-annotation1 "suffix1" :my-annotation2 "suffix2" ...)

2. A call with nil instead of the string candidate could return
the metadata of all supported fields, e.g.:

(funcall resolution/decoration-function nil)
⇒
((:name :prefix
  :type 'string)
 (:name :suffix
  :type 'string)
 (:name :my-annotation1
  :type 'number
  :tags '(:expensive-field :unstable))
 (:name :my-annotation2
  :type 'image)
 (:name :tablist
  :type 'list))

3. A call with an additional field could restrict the output only
to that field:

(funcall resolution/decoration-function "candidate" :prefix)
⇒
"prefix"

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/28/21 8:46 PM, Juri Linkov wrote:
> (funcall resolution/decoration-function "candidate")
> ⇒
> (:prefix "prefix" :suffix "suffix" :my-annotation1 "suffix1" :my-annotation2 "suffix2" ...)
> 
> 2. A call with nil instead of the string candidate could return
> the metadata of all supported fields, e.g.:
> 
> (funcall resolution/decoration-function nil)
> ⇒
> ((:name :prefix
>   :type 'string)
>  (:name :suffix
>   :type 'string)
>  (:name :my-annotation1
>   :type 'number
>   :tags '(:expensive-field :unstable))
>  (:name :my-annotation2
>   :type 'image)
>  (:name :tablist
>   :type 'list))
> 
> 3. A call with an additional field could restrict the output only
> to that field:
> 
> (funcall resolution/decoration-function "candidate" :prefix)
> ⇒
> "prefix"

What about adding a descriptor to the `metadata` alist itself for the
field type information? I think we could go with a slightly simpler
definition.

1. Add a `decoration-function` to the metadata. The function takes two
arguments, a candidate string and a list of field symbols. It returns
returns a plist of the requested fields.
2. Add a `decoration-fields` list to the metadata, which describes all
the available decoration fields with the necessary field type
information in a format similar to the one you described above.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 28.05.2021 14:55, Daniel Mendler wrote:
> But there should also be a
> way to request all available fields, such that a UI can show the ones it
> supports (but not based on name but based on type!).
I get that it could be seen as convenient, but why can't the frontend 
just request the fields it supports, one after the other?

IME the overhead for the additional function calls is negligible, as 
long as we're rendering only a dozen items or so, and not the full 
collection (with thousands of items or more).

But maybe you want a collection of particular bits of data, 
pre-formatted for display in a table, and basically not useful for 
anything else. You could call it :info-columns-function, something like 
that. Which would allow frontends not even care about which columns are 
there, or what they contain, and just print them in the indicated order.

I'm not a fan of having a backend drive presentation like that, but as 
long as nobody argues for elimination of "functional" attributes in 
favor of the info presented in info-columns-function (meaning, I could 
mostly ignore that addition), I'm not going to fight it.

But also consider the possibility of having that logic extracted:

- Have c-a-p-f elements only include the "functional" attributes.
- Add a middle layer, something like an option 
completion-info-columns-function, which takes (COMPLETION 
EXTRA-PROPERTIES) and returns a list of columnar instructions in the 
format you originally wanted. EXTRA-PROPERTIES is the value of 
completion-extra-properties when called by the default UI.

That would allow, for example, to show the key bindings and descriptions 
always in the same place, across all completion functions.

A possible drawback is that a completion function cannot unilaterally 
add a new column without a corresponding change in 
completion-info-columns-function. But that might arguably be a good thing.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/28/21 2:44 PM, Dmitry Gutov wrote:
> On 28.05.2021 14:55, Daniel Mendler wrote:
>> But there should also be a
>> way to request all available fields, such that a UI can show the ones it
>> supports (but not based on name but based on type!).
> I get that it could be seen as convenient, but why can't the frontend 
> just request the fields it supports, one after the other?

I am not particularly fixated on the specifics of how the frontend
retrieves the fields from the backend. I want something different - I
want the frontend to have the ability to request all field of a certain
type, for example strings.

The frontend cannot request fields by names of which it does not know
the name. This is the flaw of the current proposal.

Then a tablist frontend could easily display those and allow sorting and
filtering by column. You may be aware of Embark, which already provides
a tablist frontend, but only with the columns candidate and suffix as of
now. It would be great to have more columns there, which are specific to
the backend.

> IME the overhead for the additional function calls is negligible, as 
> long as we're rendering only a dozen items or so, and not the full 
> collection (with thousands of items or more).
> 
> But maybe you want a collection of particular bits of data, 
> pre-formatted for display in a table, and basically not useful for 
> anything else. You could call it :info-columns-function, something like 
> that. Which would allow frontends not even care about which columns are 
> there, or what they contain, and just print them in the indicated order.
> 
> I'm not a fan of having a backend drive presentation like that, but as 
> long as nobody argues for elimination of "functional" attributes in 
> favor of the info presented in info-columns-function (meaning, I could 
> mostly ignore that addition), I'm not going to fight it.

Why do you think backend drivers the presentation then? The backend says
I have these fields associated with the particular candidate, please
display them as you wish, for example As a spreadsheet/table or as an
additional popup in Company.

The current proposal is such that the frontend must explicitly request
the fields it would like to see, which precludes the use case I
described above.

> But also consider the possibility of having that logic extracted:
> 
> - Have c-a-p-f elements only include the "functional" attributes.
> - Add a middle layer, something like an option 
> completion-info-columns-function, which takes (COMPLETION 
> EXTRA-PROPERTIES) and returns a list of columnar instructions in the 
> format you originally wanted. EXTRA-PROPERTIES is the value of 
> completion-extra-properties when called by the default UI.

This sounds a bit complicated.

> A possible drawback is that a completion function cannot unilaterally 
> add a new column without a corresponding change in 
> completion-info-columns-function. But that might arguably be a good thing.

From my experience this is not a good thing. The completion table should
have the ability to add arbitrary annotation columns depending on the
data. There is a use case for this (Marginalia or ivy-rich) and from
what I've heard, people like to have such rich annotations. It helps a
lot with the self documentation aspect of Emacs.

And now if we would have an additional tablist frontend, the UI would be
even better, since people could take a look at the annotation fields and
sort/filter/hide each column separately like in a usual spreadsheet.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 28.05.2021 16:14, Daniel Mendler wrote:

> I am not particularly fixated on the specifics of how the frontend
> retrieves the fields from the backend. I want something different - I
> want the frontend to have the ability to request all field of a certain
> type, for example strings.
> 
> The frontend cannot request fields by names of which it does not know
> the name. This is the flaw of the current proposal.

IOW, I guessed correctly, and you want the backend to make a 
presentation-level choice for you: the list of columns to display.

> Then a tablist frontend could easily display those and allow sorting and
> filtering by column. You may be aware of Embark, which already provides
> a tablist frontend, but only with the columns candidate and suffix as of
> now. It would be great to have more columns there, which are specific to
> the backend.

Aware: yes. Seen the screenshots, haven't tried it myself so far.

> Why do you think backend drivers the presentation then? The backend says
> I have these fields associated with the particular candidate, please
> display them as you wish, for example As a spreadsheet/table or as an
> additional popup in Company.

That's "presentation" in my book, especially if the backend also 
specifies positioning, which I think has been discussed in this thread 
as well.

Or consider, for example, that you can have a file associated with each 
completion (perhaps you're showing a list of definitions or references).

Someone might like to see the file in one of the columns. Do you show 
it? How do you display it? Do you show the basename? Do you try to 
"uniquify" those values? These are all presentation-level choices.

> The current proposal is such that the frontend must explicitly request
> the fields it would like to see, which precludes the use case I
> described above.
> 
>> But also consider the possibility of having that logic extracted:
>>
>> - Have c-a-p-f elements only include the "functional" attributes.
>> - Add a middle layer, something like an option
>> completion-info-columns-function, which takes (COMPLETION
>> EXTRA-PROPERTIES) and returns a list of columnar instructions in the
>> format you originally wanted. EXTRA-PROPERTIES is the value of
>> completion-extra-properties when called by the default UI.
> 
> This sounds a bit complicated.

It's an extra indirection, yes.

But I think some users might appreciate the ability to customize the set 
and ordering of the columns in their completions.

And backend authors might appreciate not having to make those choices.

>> A possible drawback is that a completion function cannot unilaterally
>> add a new column without a corresponding change in
>> completion-info-columns-function. But that might arguably be a good thing.
> 
>  From my experience this is not a good thing. The completion table should
> have the ability to add arbitrary annotation columns depending on the
> data.

Well, it definitely shouldn't choose the presentation of "kind" 
annotations on its own, like in your earlier example.

> There is a use case for this (Marginalia or ivy-rich) and from
> what I've heard, people like to have such rich annotations. It helps a
> lot with the self documentation aspect of Emacs.

Worst case, Marginalia could define its own 
completion-info-columns-function. But yes, it would add an extra bit of 
rigidity to the system.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/28/21 3:57 PM, Dmitry Gutov wrote:
> On 28.05.2021 16:14, Daniel Mendler wrote:
> 
>> I am not particularly fixated on the specifics of how the frontend
>> retrieves the fields from the backend. I want something different - I
>> want the frontend to have the ability to request all field of a certain
>> type, for example strings.
>>
>> The frontend cannot request fields by names of which it does not know
>> the name. This is the flaw of the current proposal.
> 
> IOW, I guessed correctly, and you want the backend to make a 
> presentation-level choice for you: the list of columns to display.

Not according to my interpretion. If I have a database with a list of
columns, does the database specify the presentation? All I am saying is
that the backend should offer a list of available fields to the
frontend. What about the possibility for the backend to return some kind
of description of the available data?

(:kind 'icon :keybinding 'string :description 'string ...)

But maybe that's also overkill.

>> Why do you think backend drivers the presentation then? The backend says
>> I have these fields associated with the particular candidate, please
>> display them as you wish, for example As a spreadsheet/table or as an
>> additional popup in Company.
> 
> That's "presentation" in my book, especially if the backend also 
> specifies positioning, which I think has been discussed in this thread 
> as well.
> 
> Or consider, for example, that you can have a file associated with each 
> completion (perhaps you're showing a list of definitions or references).
> 
> Someone might like to see the file in one of the columns. Do you show 
> it? How do you display it? Do you show the basename? Do you try to 
> "uniquify" those values? These are all presentation-level choices.

Okay, I think I understand what you mean. Of course there is already a
bit of presentation performed by the backend depending on how it returns
the data, in particular if the backend returns generic fields of type
string. In order to fix this one would have to distinguish 'string from
'file field types.

>>> - Have c-a-p-f elements only include the "functional" attributes.
>>> - Add a middle layer, something like an option
>>> completion-info-columns-function, which takes (COMPLETION
>>> EXTRA-PROPERTIES) and returns a list of columnar instructions in the
>>> format you originally wanted. EXTRA-PROPERTIES is the value of
>>> completion-extra-properties when called by the default UI.
>>
>> This sounds a bit complicated.
> 
> It's an extra indirection, yes.
> 
> But I think some users might appreciate the ability to customize the set 
> and ordering of the columns in their completions.
> 
> And backend authors might appreciate not having to make those choices.

This is a serious limitation of Marginalia as of now. One could handle
this is on the UI level. An intermediate layer is more generic and would
work with different UIs. This is probably better, I agree with you about
that.

>>  From my experience this is not a good thing. The completion table should
>> have the ability to add arbitrary annotation columns depending on the
>> data.
> 
> Well, it definitely shouldn't choose the presentation of "kind" 
> annotations on its own, like in your earlier example.

My idea is rather that the completion table returns a field of type,
which can then be interpreted depending on its type by the UI. The
question is how fine grained these types should be. As you argued string
is not generic enough, one would require a special 'kind type and a
'filename type. In the case of the 'kind type the discussion becomes a
bit mood since then one has a field 'kind of type 'kind. But this is not
the common case, I think one could catch many annotations with some
common types.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 28.05.2021 17:10, Daniel Mendler wrote:

> What about the possibility for the backend to return some kind
> of description of the available data?
> 
> (:kind 'icon :keybinding 'string :description 'string ...)
> 
> But maybe that's also overkill.

I could get behind something like this. Though it does look like extra 
work for backend writers.

My thinking was more along the lines of having the attribute types 
simply be described in the docs (then only the "known" attributes would 
be rendered by different frontends), but you and the Marginalia 
community can probably make a better choice between these options than 
myself (*).

Note that this is still compatible with my suggestions of 
completion-info-columns-function, the latter could still be used for 
some presentation-level choices (like which columns to display and in 
which order; but also be able to show arbitrary columns). But it becomes 
less necessary for sure.

(*) From where I'm standing, there should be a limited set of columns 
you'll want to show in such table. Then we could basically document them 
all at once and forego the introspection capability. But I definitely 
can be wrong about this.

> Okay, I think I understand what you mean. Of course there is already a
> bit of presentation performed by the backend depending on how it returns
> the data, in particular if the backend returns generic fields of type
> string. In order to fix this one would have to distinguish 'string from
> 'file field types.

Another use for field types could be to render some of the fields 
differently, e.g. apply the help-key-binding face to key bindings.

>>>   From my experience this is not a good thing. The completion table should
>>> have the ability to add arbitrary annotation columns depending on the
>>> data.
>>
>> Well, it definitely shouldn't choose the presentation of "kind"
>> annotations on its own, like in your earlier example.
> 
> My idea is rather that the completion table returns a field of type,
> which can then be interpreted depending on its type by the UI. The
> question is how fine grained these types should be. As you argued string
> is not generic enough, one would require a special 'kind type and a
> 'filename type. In the case of the 'kind type the discussion becomes a
> bit mood since then one has a field 'kind of type 'kind. But this is not
> the common case, I think one could catch many annotations with some
> common types.

Yes, probably.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/28/21 4:57 PM, Dmitry Gutov wrote:
> On 28.05.2021 17:10, Daniel Mendler wrote:
> 
>> What about the possibility for the backend to return some kind
>> of description of the available data?
>>
>> (:kind 'icon :keybinding 'string :description 'string ...)
>>
>> But maybe that's also overkill.
> 
> I could get behind something like this. Though it does look like extra 
> work for backend writers.

There is certainly some extra work. On the other hand the backend has to
do a lot of unnecessary formatting which could be pushed to the frontend
layer. The formatting happens for example in Marginalia, which hooks
into the backend completion table.

> My thinking was more along the lines of having the attribute types 
> simply be described in the docs (then only the "known" attributes would 
> be rendered by different frontends), but you and the Marginalia 
> community can probably make a better choice between these options than 
> myself (*).
> 
> Note that this is still compatible with my suggestions of 
> completion-info-columns-function, the latter could still be used for 
> some presentation-level choices (like which columns to display and in 
> which order; but also be able to show arbitrary columns). But it becomes 
> less necessary for sure.
> 
> (*) From where I'm standing, there should be a limited set of columns 
> you'll want to show in such table. Then we could basically document them 
> all at once and forego the introspection capability. But I definitely 
> can be wrong about this.

Yes, this is the difference between the UIs. For vertical
completing-read UIs (default completion with one-column, Vertico, etc)
there is plenty of space to show a lot of helpful information. For
popups this is not the case, there you only want a small set of columns,
which could be hard-coded.

From my perspective it would be a significant improvement if backends
could add arbitrary fields (with some associated field type, the
frontend can understand or ignore), in contrast to only allowing a fixed
set of fields.

From what I've seen the Icomplete patch by João is quite independent of
this discussion here and could go in as is. We should probably compare
different prototypes for an enhanced affixation/decoration function to
see how the result could actually look and how well it works in the
different scenarios.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

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

Daniel Mendler <mail@daniel-mendler.de> writes:

> From my side the changes in your icomplete-vertical-mode-improvements
> branch are fine.

I didn't get a terrible amount of feedback, but I spent the week testing
that branch, making small adjustments, and verifying that the default
Icomplete and Fido behaviour are unchanged.  So I've now pushed the
icomplete-vertical-mode rework to master.  I quite like it, to be
honest.  I also added little filtered/total indicator idea found in
Vertico and many other completers.

Here's a gif showing how to use it with Fido, my preferred option.  To
try it out, just M-x fido-mode and M-x fido-vertical-mode together.

Thanks,
João


[-- Attachment #2: Peek 2021-06-01 10-52.gif --]
[-- Type: image/gif, Size: 278785 bytes --]

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 6/1/21 11:56 AM, João Távora wrote:
>> From my side the changes in your icomplete-vertical-mode-improvements
>> branch are fine.
> 
> I didn't get a terrible amount of feedback, but I spent the week testing
> that branch, making small adjustments, and verifying that the default
> Icomplete and Fido behaviour are unchanged.  So I've now pushed the
> icomplete-vertical-mode rework to master.  I quite like it, to be
> honest.  I also added little filtered/total indicator idea found in
> Vertico and many other completers.
> 
> Here's a gif showing how to use it with Fido, my preferred option.  To
> try it out, just M-x fido-mode and M-x fido-vertical-mode together.

Thanks! I just gave it another try. It looks good and feels similar to
my Vertico. It works also well with Marginalia.

* I observed an issue with the last candidate with `icomplete-scroll=t`
- The candidate is one line off the minibuffer screen.

* Is it possible to disable the index/count indicator? In Vertico I use
`vertico-count-format` for that, if nil, the indicator is disabled.

* Do you plan to add more scroll commands, scroll forward/backward by
one page as Vertico has it? See
https://github.com/minad/vertico#key-bindings for the Vertico keybindings.

* Do you plan to support the `group-function`?

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 6/1/21 11:56 AM, João Távora wrote:
>>> From my side the changes in your icomplete-vertical-mode-improvements
>>> branch are fine.
>> 
>> I didn't get a terrible amount of feedback, but I spent the week testing
>> that branch, making small adjustments, and verifying that the default
>> Icomplete and Fido behaviour are unchanged.  So I've now pushed the
>> icomplete-vertical-mode rework to master.  I quite like it, to be
>> honest.  I also added little filtered/total indicator idea found in
>> Vertico and many other completers.
>> 
>> Here's a gif showing how to use it with Fido, my preferred option.  To
>> try it out, just M-x fido-mode and M-x fido-vertical-mode together.
>
> Thanks! I just gave it another try. It looks good and feels similar to
> my Vertico. It works also well with Marginalia.
>
> * I observed an issue with the last candidate with `icomplete-scroll=t`
> - The candidate is one line off the minibuffer screen.

Hmm, I don't see that problem.  Are you sure you're using the version in
master?  Else you may be seeing a bug that was in
scratch/icomplete-vertical-mode-improvements (which should now be
scratched, as the name implies).


> * Is it possible to disable the index/count indicator? In Vertico I use
> `vertico-count-format` for that, if nil, the indicator is disabled.

I normally don't add a knob for things until someone actually requests
it.  If you're requesting it, then it can be added (and you can too).

> * Do you plan to add more scroll commands, scroll forward/backward by
> one page as Vertico has it? See
> https://github.com/minad/vertico#key-bindings for the Vertico
> keybindings.

Could be added, yes.

> * Do you plan to support the `group-function`?

Maybe, but nothing in Emacs uses it so I can't test.  You can add it
too, of course.  I don't understand one thing about that protocol: why
the extra TRANSFORM argument flag?  Why not just have that function
return a tuple with the group title and transformed name?

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 6/1/21 2:00 PM, João Távora wrote:
>> * I observed an issue with the last candidate with `icomplete-scroll=t`
>> - The candidate is one line off the minibuffer screen.
> 
> Hmm, I don't see that problem.  Are you sure you're using the version in
> master?  Else you may be seeing a bug that was in
> scratch/icomplete-vertical-mode-improvements (which should now be
> scratched, as the name implies).

Yes, I am using the newest master version. But the issue seems to be
related somehow to my configuration, since I don't see it with emacs -Q.
I have to investigate which setting does this.

>> * Is it possible to disable the index/count indicator? In Vertico I use
>> `vertico-count-format` for that, if nil, the indicator is disabled.
> 
> I normally don't add a knob for things until someone actually requests
> it.  If you're requesting it, then it can be added (and you can too).

I usually take the same stance as you and I also added this only after a
request to Vertico. I can add a small patch making this configurable.

>> * Do you plan to support the `group-function`?
> 
> Maybe, but nothing in Emacs uses it so I can't test.  You can add it
> too, of course. 

There is `read-char-by-name` one can use for testing. Juri added the
`group-function` there.

> I don't understand one thing about that protocol: why
> the extra TRANSFORM argument flag?  Why not just have that function
> return a tuple with the group title and transformed name?

The design of the `group-function` has been extensively discussed. The
flag is used instead of a tuple for performance reasons. The computation
of the transformed candidate string may require string allocations,
which are expensive if performed for every candidate. By using a flag we
only have to transform the displayed candidates. There is the function
`minibuffer--group-by` which takes the *full* sorted candidate list and
groups the candidates. This function invokes the `group-function` with
`TRANSFORM=nil` once for each filtered candidate. UIs like Icomplete and
Vertico are the main beneficiaries from this API design. It is also nice
for default completion of course, but there the optimization does not
play such a big role.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 6/1/21 2:00 PM, João Távora wrote:
>>> * I observed an issue with the last candidate with `icomplete-scroll=t`
>>> - The candidate is one line off the minibuffer screen.
> Yes, I am using the newest master version. But the issue seems to be
> related somehow to my configuration, since I don't see it with emacs -Q.
> I have to investigate which setting does this.

Let me know about your findings.

>>> * Do you plan to support the `group-function`?
>> 
>> Maybe, but nothing in Emacs uses it so I can't test.  You can add it
>> too, of course. 
>
> There is `read-char-by-name` one can use for testing. Juri added the
> `group-function` there.

I didn't know how to use it, so I just M-: (read-char-by-name "thingy").
It worked decently, I guess, but I couldn't find any evidence of
"grouping".  What is the group function supposed to do here?  Does
Vertico use it?  What does it do with it?

>> I don't understand one thing about that protocol: why
>> the extra TRANSFORM argument flag?  Why not just have that function
>> return a tuple with the group title and transformed name?
>
> The design of the `group-function` has been extensively discussed. The
> flag is used instead of a tuple for performance reasons. The computation
> of the transformed candidate string may require string allocations,
> which are expensive if performed for every candidate.

That's right, I see your point.

They could return a lambda, which is cheap to allocate, to defer the
calculation.  Or just nil or #'identity if it's supposed to stay
unchanged.

   diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
   index ec21b7b93b..5c00e7bb81 100644
   --- a/lisp/minibuffer.el
   +++ b/lisp/minibuffer.el
   @@ -1450,11 +1450,14 @@ minibuffer--group-by
      "Group ELEMS by GROUP-FUN and sort groups by SORT-FUN."
      (let ((groups))
        (dolist (cand elems)
   -      (let* ((key (funcall group-fun cand nil))
   +      (let* ((key-and-transform (funcall group-fun cand))
   +             (key (car key-and-transform))
                 (group (assoc key groups)))
   -        (if group
   -            (setcdr group (cons cand (cdr group)))
   -          (push (list key cand) groups))))
   +        (cond (group
   +               (put-text-property 0 1 'completion--transform (cdr key-and-transform)
   +                                  cand)
   +               (setcdr group (cons cand (cdr group))))
   +              (push (list key cand) groups))))
        (setq groups (nreverse groups)
              groups (mapc (lambda (x)
                             (setcdr x (nreverse (cdr x))))

The protocol is slightly simpler since it returns the same type every
time.  More importantly, this avoids threading group-fn down a bunch of
functions in minibuffer.el, where it seems to be adding an awful lot of
complexity.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 6/1/21 4:30 PM, João Távora wrote:
>> The design of the `group-function` has been extensively discussed. The
>> flag is used instead of a tuple for performance reasons. The computation
>> of the transformed candidate string may require string allocations,
>> which are expensive if performed for every candidate.
> 
> That's right, I see your point.
> 
> They could return a lambda, which is cheap to allocate, to defer the
> calculation.  Or just nil or #'identity if it's supposed to stay
> unchanged.
> ...
> The protocol is slightly simpler since it returns the same type every
> time.  More importantly, this avoids threading group-fn down a bunch of
> functions in minibuffer.el, where it seems to be adding an awful lot of
> complexity.

No, I am against changing the group function. I am using this efficient
definition in my packages and I rely on the performance characteristics.

The current implementation is simple enough and also avoids allocation
of the cons pair and the allocation of the lambda as in your proposal.
The efficiency is crucial here. Furthermore I argue that the current
implementation is simpler to understand for the completion table authors
than your counter proposal, which introduces a deferred computation with
the lambda.

There was a long discussion with Stefan, Dmitry,  Juri and Eli where
multiple different designs for the `group-function` have been considered
and we settled on the current design.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 6/1/21 4:30 PM, João Távora wrote:
>>> The design of the `group-function` has been extensively discussed. The
>>> flag is used instead of a tuple for performance reasons. The computation
>>> of the transformed candidate string may require string allocations,
>>> which are expensive if performed for every candidate.
>> 
>> That's right, I see your point.
>> 
>> They could return a lambda, which is cheap to allocate, to defer the
>> calculation.  Or just nil or #'identity if it's supposed to stay
>> unchanged.
>> ...
>> The protocol is slightly simpler since it returns the same type every
>> time.  More importantly, this avoids threading group-fn down a bunch of
>> functions in minibuffer.el, where it seems to be adding an awful lot of
>> complexity.
>
> No, I am against changing the group function. I am using this efficient
> definition in my packages and I rely on the performance
> characteristics.

But do you have any evidence to suggest that making a cons and
allocating a function is detrimental to this??  I just tried this small
patch to the function you suggested as a potential hog, since it
iterates all the completions.

    diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
    index ec21b7b93b..32737ff1ce 100644
    --- a/lisp/minibuffer.el
    +++ b/lisp/minibuffer.el
    @@ -1450,11 +1450,15 @@ minibuffer--group-by
       "Group ELEMS by GROUP-FUN and sort groups by SORT-FUN."
       (let ((groups))
         (dolist (cand elems)
    -      (let* ((key (funcall group-fun cand nil))
    +      (let* ((pair (cons (funcall group-fun cand nil)
    +                         (lambda () (list "oh no a capture"
    +                                          cand))))
    +             (key (car pair))
                  (group (assoc key groups)))
             (if group
                 (setcdr group (cons cand (cdr group)))
    -          (push (list key cand) groups))))
    +          (push (list key cand) groups))
    +        (put-text-property 0 1 'transform (cdr pair) cand)))
         (setq groups (nreverse groups)
               groups (mapc (lambda (x)
                              (setcdr x (nreverse (cdr x))))

(length joaot/test); 44547 taken from the `read-char-by-name' table

(benchmark-run 10000000
  '(minibuffer--group-by #'mule--ucs-names-group ; group-fn of that table
                         #'identity
                         joaot/test))
                         
;; Before the patch
;;
;; (0.10353484400000001 0 0.0)
;; (0.08804328700000008 0 0.0)
;; (0.11191811499999996 0 0.0)
;;
;; After
;;
;; (0.09785707300000002 0 0.0)
;; (0.10107326400000005 0 0.0)
;; (0.09778669299999998 0 0.0)

So basically the same.  Cons and lambda's are cheap.

Also, for good measure, I also changed that function to allocate a 30
char string for each completion (instead of the lambda).  Also quite
snappy (though there's maybe an effect showing).

;; (0.10938493999999999 0 0.0)
;; (0.10871186500000007 0 0.0)
;; (0.11186114000000003 0 0.0)

> The current implementation is simple enough and also avoids allocation
> of the cons pair and the allocation of the lambda as in your proposal.
> The efficiency is crucial here.

I believe you, but I haven't seen any evidence (yet) to back up the
claim.  But instead of me whistling premature optimization song, feel
free to point me to some numbers or something.

> Furthermore I argue that the current implementation is simpler to
> understand for the completion table authors than your counter
> proposal, which introduces a deferred computation with the lambda.

It's like the affixation-function thing, I don't think it makes sense
for client code to do these IFs, much as I dont' think it makes sense
for them to do MAPCARs.

But to be clear, the main advantage here in my opinion is the cleanup in
minibuffer.el, which is somehwat messy in my opinion (not only from
group-fn's fault, of course).

> There was a long discussion with Stefan, Dmitry,  Juri and Eli where
> multiple different designs for the `group-function` have been considered
> and we settled on the current design.

It could have been discussed with your favourite divinity, for all I
care.  If I see something worthy of improvement for the next version,
I'm going to suggest it here.  Especially in the case of a new feature
in master. "It's been discussed with so and so" is not a worthless
argument, but it's not very valuable either.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 6/1/21 5:49 PM, João Távora wrote:
> But do you have any evidence to suggest that making a cons and
> allocating a function is detrimental to this??  I just tried this small
> patch to the function you suggested as a potential hog, since it
> iterates all the completions.
>
> So basically the same.  Cons and lambda's are cheap.

The allocations itself are cheap but they put unnecessary pressure on
the GC. The current version avoids this. This matters when scrolling
through long candidate lists.

>> The current implementation is simple enough and also avoids allocation
>> of the cons pair and the allocation of the lambda as in your proposal.
>> The efficiency is crucial here.
> 
> I believe you, but I haven't seen any evidence (yet) to back up the
> claim.  But instead of me whistling premature optimization song, feel
> free to point me to some numbers or something.

The current version is proven in my packages and performs well. I don't
see a point in replacing it to a more complicated and less efficient
version.

>> Furthermore I argue that the current implementation is simpler to
>> understand for the completion table authors than your counter
>> proposal, which introduces a deferred computation with the lambda.
> 
> It's like the affixation-function thing, I don't think it makes sense
> for client code to do these IFs, much as I dont' think it makes sense
> for them to do MAPCARs.

No it is not. It is more like the decoration function of which you are
also in favor. The type is this:

group-function : string -> bool -> string

The bool corresponds to the fields to return in the decoration function.
Either return the title or return the transformed candidate.

> But to be clear, the main advantage here in my opinion is the cleanup in
> minibuffer.el, which is somehwat messy in my opinion (not only from
> group-fn's fault, of course).

That's the wrong way to look at it. The main thing which should be
optimized here is the performance and the ease of usage for the
completion table authors. Furthermore I disagree with you that the
minibuffer.el code is messy. The same applies to the Vertico code, where
I implemented group function support. It is also reasonably clean.
Please take a look at it.

The suggestion of yours to mutate the candidates by putting a property
instead of threading the group function through the call sites is much
worse.

>> There was a long discussion with Stefan, Dmitry,  Juri and Eli where
>> multiple different designs for the `group-function` have been considered
>> and we settled on the current design.
> 
> It could have been discussed with your favourite divinity, for all I
> care.  If I see something worthy of improvement for the next version,
> I'm going to suggest it here.  Especially in the case of a new feature
> in master. "It's been discussed with so and so" is not a worthless
> argument, but it's not very valuable either.

Your version is not an improvement. Please read through the old
discussion and consider my arguments. I also don't see a point in
rediscussing this as long as you don't make a proposal which is a clear
improvement.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 6/1/21 5:49 PM, João Távora wrote:
>> But do you have any evidence to suggest that making a cons and
>> allocating a function is detrimental to this??  I just tried this small
>> patch to the function you suggested as a potential hog, since it
>> iterates all the completions.
>>
>> So basically the same.  Cons and lambda's are cheap.
>
> The allocations itself are cheap but they put unnecessary pressure on
> the GC. The current version avoids this. This matters when scrolling
> through long candidate lists.

I've now benchmarked correctly (the previous benchmarks were completely
bogus)

;; current
(20.18251486 3 0.3386851209999975)
(19.646946365 3 0.3477981569999997)
(20.10486081 3 0.34829881900000004)

;; allocate extra cons and non-capturing function
(21.353061817 3 0.3557993659999994)
(21.124979874999998 3 0.361779398000003)
(21.009165698 3 0.36675408300000356)

;; allocate extra cons and closure function
(22.860096656 4 0.49817515299999826)
(22.091684587000003 4 0.4981799040000041)
(21.948139371 4 0.505049483999997)

;; allocate extra cons and string
(24.102438947 4 0.5202420680000017)

This was the test:

   (length joaot/test) ;; 44547 taken from the `read-char-by-name' table
   (let ((test (mapcar #'substring-no-properties joaot/test)))
     (garbage-collect)
     (benchmark-run 1
       (minibuffer--group-by #'mule--ucs-names-group
                             #'identity
                             test)
       (setq test nil)
       (garbage-collect)))

I really doubt this "matters when scrolling", as grouping would be
called once per filtering, and scrolling is just getting at each
candidate's transform.  If anything, if you take the last option, it
could _speed up_ scrolling (and the cost of some more initial latency).

>>> The current implementation is simple enough and also avoids allocation
>>> of the cons pair and the allocation of the lambda as in your proposal.
>>> The efficiency is crucial here.
>> 
>> I believe you, but I haven't seen any evidence (yet) to back up the
>> claim.  But instead of me whistling premature optimization song, feel
>> free to point me to some numbers or something.
>
> The current version is proven in my packages and performs well. I don't
> see a point in replacing it to a more complicated and less efficient
> version.

I've told you the point.  A simpler minibuffer.el code, possibly with
less branching and logic, which might even be faster.

>>> Furthermore I argue that the current implementation is simpler to
>>> understand for the completion table authors than your counter
>>> proposal, which introduces a deferred computation with the lambda.
>> 
>> It's like the affixation-function thing, I don't think it makes sense
>> for client code to do these IFs, much as I dont' think it makes sense
>> for them to do MAPCARs.
>
> No it is not.

Look, this is just my opinion, you don't need to concur with it it, but
there's no need to say "no it's not" to an opinion.

> completion table authors. Furthermore I disagree with you that the
> minibuffer.el code is messy.

Surely, you jest, lol.  minibuffer-completion-help is a mess.  That's
ok!  Most of minibuffer.el is a mess.  There's no good code, in general,
so let's not get emotionally attached to ours.

> The suggestion of yours to mutate the candidates by putting a property
> instead of threading the group function through the call sites is much
> worse.

In what way?  Really, all other things being equal, in what way is
threading arguments through lots of functions, bloating the arglist,
implementations and the docstrings of said functions, calling group-fun
multiple times and in multiple places in minibuffer.el better than _not_
doing those things?

The only argument is if there was a terrible performance cost, which was
indeed the first and only real argument you advanced.  But I've not seen
evidence of that reality.

> Your version is not an improvement. Please read through the old
> discussion and consider my arguments. I also don't see a point in
> rediscussing this as long as you don't make a proposal which is a clear
> improvement.

Look, I get it that you oppose this suggestion and are happy with your
implementation.  That's fine it's your right to be.  But there's no
point in so aggresively gainsaying a suggestion while not offering any
material arguments.  APIs in master aren't meant to cristalize, they are
meant to be iterated.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 6/1/21 8:47 PM, João Távora wrote:
> I really doubt this "matters when scrolling", as grouping would be
> called once per filtering, and scrolling is just getting at each
> candidate's transform.  If anything, if you take the last option, it
> could _speed up_ scrolling (and the cost of some more initial latency).

Yes, you are right about that. It matters when refiltering the candidate
list, which happens quite often in Vertico or other continuously
updating UIs.

>>>> The current implementation is simple enough and also avoids allocation
>>>> of the cons pair and the allocation of the lambda as in your proposal.
>>>> The efficiency is crucial here.
>>>
>>> I believe you, but I haven't seen any evidence (yet) to back up the
>>> claim.  But instead of me whistling premature optimization song, feel
>>> free to point me to some numbers or something.
>>
>> The current version is proven in my packages and performs well. I don't
>> see a point in replacing it to a more complicated and less efficient
>> version.
> 
> I've told you the point.  A simpler minibuffer.el code, possibly with
> less branching and logic, which might even be faster.

I doubt that you manage to make it simpler. If you indeed manage to make
you simpler I will propose a counter patch based on the current
definition which is as simple.

Generally, we should not base our design on the internals of the UI but
rather on the ease of use and the performance of the API. The internals
can certainly be made nicer, but this is an orthogonal discussions. I
welcome simplifications and improvements. I also submitted some patches
in this direction myself. In the process of adding the `group-function`
I made such simplifications.

>>>> Furthermore I argue that the current implementation is simpler to
>>>> understand for the completion table authors than your counter
>>>> proposal, which introduces a deferred computation with the lambda.
>>>
>>> It's like the affixation-function thing, I don't think it makes sense
>>> for client code to do these IFs, much as I dont' think it makes sense
>>> for them to do MAPCARs.
>>
>> No it is not.
> 
> Look, this is just my opinion, you don't need to concur with it it, but
> there's no need to say "no it's not" to an opinion.

There is also no need to rediscuss a fine solution without making a
better and proper counter proposal. I told you that I care about this
being efficient - meaning no allocations. If your proposal fails to
address this, I will oppose it.

>> completion table authors. Furthermore I disagree with you that the
>> minibuffer.el code is messy.
> 
> Surely, you jest, lol.  minibuffer-completion-help is a mess.  That's
> ok!  Most of minibuffer.el is a mess.  There's no good code, in general,
> so let's not get emotionally attached to ours.

I am talking about the added complexity due to the `group-function` of
course. I am not emotionally attached to the minibuffer code and neither
to my code. I suggest you keep the discussion on a technical level.

>> The suggestion of yours to mutate the candidates by putting a property
>> instead of threading the group function through the call sites is much
>> worse.
> 
> In what way?  Really, all other things being equal, in what way is
> threading arguments through lots of functions, bloating the arglist,
> implementations and the docstrings of said functions, calling group-fun
> multiple times and in multiple places in minibuffer.el better than _not_
> doing those things?

Please present your counter proposal which is as efficient and which is
simpler. Then we can discuss. But I will then make another proposal
which will simplify minibuffer.el as much as your proposal.

> The only argument is if there was a terrible performance cost, which was
> indeed the first and only real argument you advanced.  But I've not seen
> evidence of that reality.

I benchmarked this heavily in my Vertico UI. So I can assure you that I
looked into this. I rely on this for the performance of Vertico and for
the performance of other continuously updating UIs.

The current group-function is implemented in Selectrum (MELPA), Vertico
(GNU ELPA), Icomplete-vertical (GNU MELPA) and used by Emacs and my
Consult package so far. It is a proven implementation.

I don't think it is justified to reiterate the whole discussion. At some
point it must be possible to rely on the code which made it into Emacs
master.

>> Your version is not an improvement. Please read through the old
>> discussion and consider my arguments. I also don't see a point in
>> rediscussing this as long as you don't make a proposal which is a clear
>> improvement.
> 
> Look, I get it that you oppose this suggestion and are happy with your
> implementation.  That's fine it's your right to be.  But there's no
> point in so aggresively gainsaying a suggestion while not offering any
> material arguments.  APIs in master aren't meant to cristalize, they are
> meant to be iterated.

I have made many material arguments and you ignore them. The status quo
is - we have the 'group-function` in the current form. While not being
emotionally attached to it, I think it is a fine implementation.

If you want to change it, make a better proposal. I will oppose it as
long as it does not address my performance concerns. If it addresses
these concerns I am of course open to improvements. I am the last person
to say no to a technical argument. You are the one who did not bring a
good one here, except some criticism regarding the current way
minibuffer.el is implemented. This can be improved and I think we can do
that in a way keeping the current definition of the `group-function`.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 6/1/21 8:47 PM, João Távora wrote:
>> I really doubt this "matters when scrolling", as grouping would be
>> called once per filtering, and scrolling is just getting at each
>> candidate's transform.  If anything, if you take the last option, it
>> could _speed up_ scrolling (and the cost of some more initial latency).
> Yes, you are right about that. It matters when refiltering the candidate
> list, which happens quite often in Vertico or other continuously
> updating UIs.

Indeed, and losing 20 seconds (example above) for each refiltering is
normally prohibitive.  That's why icomplete (and probably all these
other ui's have while-no-input), effectively interrupting the
processing.  So it also doesn't matter when rapidly typing.  It's only
when the user "settles" on a value.

>> I've told you the point.  A simpler minibuffer.el code, possibly with
>> less branching and logic, which might even be faster.
> I doubt that you manage to make it simpler. If you indeed manage to
> make [it] simpler I will propose a counter patch based on the current
> definition which is as simple.

If I or you or anyone else manages to make X or Y simpler, we judge that
work based its merits and drawbacks.  Tends to work better.

> There is also no need to rediscuss a fine solution without making a
> better and proper counter proposal.

My proposal's merits are a simpler protocol and simplified
minibuffer.el.  Its drawbacks are what I think are a negligble
performance hit in non-essential scenarios.  You may not agree with this
assessment, but there's nothing "improper" about it.

> I have made many material arguments and you ignore them.

A bizarre claim: you pointed to performance loss and to a specific
function that iterates all completions, and I spent the effort to look
at that function, benchmark variations on it and share my results with
you.  I found that even with a very large set of completions only a
small amount of time is lost (around 3-5%) even if you happen hit an
unlucky GC.

Sure you may think that's unacceptable. But when? In what circumstances?
For example, you once said it "matters when scrolling", but then we
established that it doesn't.

> I benchmarked this heavily in my Vertico UI. So I can assure you that I
> looked into this. I rely on this for the performance of Vertico and for
> the performance of other continuously updating UIs.

I'd rather learn details of those experiments and their results.  The
only thing I have are measurements and methodology that I shared with
you and which don't point to anything serious.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

> ;; current
> (20.18251486 3 0.3386851209999975)
> (19.646946365 3 0.3477981569999997)
> (20.10486081 3 0.34829881900000004)
>
> ;; allocate extra cons and non-capturing function
> (21.353061817 3 0.3557993659999994)
> (21.124979874999998 3 0.361779398000003)
> (21.009165698 3 0.36675408300000356)

So, the current design is better?
I see no need to re-hash the whole discussion around it, unless you find
an actual concrete problem with it (rather than an aesthetic dislike).

Let's move on.  If we can't agree on this design which has no clear
flaw, I can't imagine we'll ever come to an agreement for the
replacement of `affixation-function` so we'll be stuck with the current
design and its real technical flaw (which makes does the layout in the
backend, and hence inevitably gets it wrong for some UIs).


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Tue, Jun 1, 2021 at 9:22 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > ;; current
> > (20.18251486 3 0.3386851209999975)
> > (19.646946365 3 0.3477981569999997)
> > (20.10486081 3 0.34829881900000004)
> >
> > ;; allocate extra cons and non-capturing function
> > (21.353061817 3 0.3557993659999994)
> > (21.124979874999998 3 0.361779398000003)
> > (21.009165698 3 0.36675408300000356)
>
> So, the current design is better?

It's neglibily faster IMO (and only in this particular stage).  Could
be slower later on, where the group function is called again on the
same completion twice more.

> I see no need to re-hash the whole discussion around it, unless you find
> an actual concrete problem with it (rather than an aesthetic dislike).

A confusing API and a complicated minibuffer.el are concrete problems
IMHO.

> Let's move on.  If we can't agree on this design which has no clear
> flaw, I can't imagine we'll ever come to an agreement for the
> replacement of `affixation-function`

You may not think it worth it, but the complicated minibuffer.el hampers
my efforts to experiment with replacements for affixation-function. Nothing
insurmountable but I for one like tighter API's and code.

João Távora

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

>> I see no need to re-hash the whole discussion around it, unless you find
>> an actual concrete problem with it (rather than an aesthetic dislike).
> A confusing API and a complicated minibuffer.el are concrete problems IMHO.

These are judgment calls, i.e. aesthetic dislikes.
The long discussion we've had about ti shows that there's a lot of
disagreeing tastes around this topic, so I'm pretty sure we won't find
one that satisfies everyone.

>> Let's move on.  If we can't agree on this design which has no clear
>> flaw, I can't imagine we'll ever come to an agreement for the
>> replacement of `affixation-function`
> You may not think it worth it, but the complicated minibuffer.el hampers
> my efforts to experiment with replacements for affixation-function.  Nothing
> insurmountable but I for one like tighter API's and code.

I highly doubt changing the API for `group-function` would make any
measurable difference in this respect since it's a pretty small part of
"the complicated minibuffer.el".


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, Jun 2, 2021 at 3:40 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> These are judgment calls, i.e. aesthetic dislikes.
> The long discussion we've had about ti shows that there's a lot of
> disagreeing tastes around this topic, so I'm pretty sure we won't find
> one that satisfies everyone.

I admit I don't know how long or painful the discussion was for all of you,
but so far the only argument against doing what I suggest is performance,
and it seems weak according to my measurements (which are the only
ones I've seen).

So I miss how ideas for less code and stabler return type of an API
call fall into bikeshedding territory.  Those would be more struct vs
deflcass vs list vs propertized string for me, for example. But even
there, might not be bikeshedding, it depends on the intentions.

> I highly doubt changing the API for `group-function` would make any
> measurable difference in this respect since it's a pretty small part of
> "the complicated minibuffer.el".

They make a difference in the part I was trying to touch.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

> I admit I don't know how long or painful the discussion was for all of you,
> but so far the only argument against doing what I suggest is performance,
> and it seems weak according to my measurements (which are the only
> ones I've seen).

All the arguments for or against other alternatives are/were
similarly weak.  Hence this bikeshedding.

The fact that you perceive the difference as very important is the
result of the bikeshedding effect, nothing more.
Or call it NIH, if you want.  The end result is the same: please move on.

We can revisit this decision in 5 or 10 years when we have more
experience with all those new completion-table features, if you want.
But for now, `group-function` is good enough as it is.

>> I highly doubt changing the API for `group-function` would make any
>> measurable difference in this respect since it's a pretty small part of
>> "the complicated minibuffer.el".
> They make a difference in the part I was trying to touch.

It's easy to write a local adapter which adapts the current calling
convention to the one you propose or vice-versa, so the API itself can't
be the real reason for a significant difference.


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 01.06.2021 21:47, João Távora wrote:
> In what way?  Really, all other things being equal, in what way is
> threading arguments through lots of functions, bloating the arglist,
> implementations and the docstrings of said functions, calling group-fun
> multiple times and in multiple places in minibuffer.el better than_not_
> doing those things?

Why do you think it forces you to "thread" them?

Firs of all, you can construct the same composite values you wanted 
using the current calling convention of the group-function. And then use 
them the same way.

Whether this results in better minibuffer.el code is debatable: your 
diff has more additions than deletions, and one rather long line.

OTOH, someone in a different codebase might appreciate that the 
group-function decouples the mapping to group names from the 
transformations of the items, and thus one can do the latter closer to 
where the items are printed, instead of having to "thread" the 
transformed values to their place of use (from where the grouping logic 
was called).

Using a text property is a handy workaround, but ideally one shouldn't 
have to use it.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, Jun 2, 2021 at 12:04 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 01.06.2021 21:47, João Távora wrote:
> > In what way?  Really, all other things being equal, in what way is
> > threading arguments through lots of functions, bloating the arglist,
> > implementations and the docstrings of said functions, calling group-fun
> > multiple times and in multiple places in minibuffer.el better than_not_
> > doing those things?
>
> Why do you think it forces you to "thread" them?

The current implementation in minibuffer.el threads the function down
to the point where it is called twice again (once with the same
second argument -- a possible inefficiency -- and then with a non-nil
second argument). If the results of a single call were stored in
the candidate string, this wouldn't be necessary.

> Firs of all, you can construct the same composite values you wanted
> using the current calling convention of the group-function. And then use
> them the same way.

Indeed.  Indeed that does show that we simplify the code and can keep
the current calling convention.  But then why should we? Its akwardness
becomes even clearer there: calling the same function twice in quick
succession with different second arg for two different types of return value.
To save a cons?

> Whether this results in better minibuffer.el code is debatable: your
> diff has more additions than deletions, and one rather long line.

Oh sure, that function is slightly more complicated, but if you look at

completion--insert-strings
completion--insert-horizontal
completion--insert-vertical
completion--insert-one-column
display-completion-list
minibuffer-completion-help

You'll find they are all simplified.

> OTOH, someone in a different codebase might appreciate that the
> group-function decouples the mapping to group names from the
> transformations of the items, and thus one can do the latter closer to
> where the items are printed, instead of having to "thread" the
> transformed values to their place of use (from where the grouping logic
> was called).

Exactly, but my proposal doesn't prevent doing this near the
print locus.

> Using a text property is a handy workaround, but ideally one shouldn't
> have to use it.

There's nothing wrong with it? If you see a transformation function as
a property of a candidate (which it is in reality -- it's given by a fucntion of
a candidate), then it's something that fits naturally.  If a candidate was
some non-string object, I doubt that there would be any doubt as to what
to do here.  But just because it's a string doesn't change things that much.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, Jun 2, 2021 at 12:22 AM João Távora <joaotavora@gmail.com> wrote:
>
> On Wed, Jun 2, 2021 at 12:04 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
> >
> > On 01.06.2021 21:47, João Távora wrote:
> > > In what way?  Really, all other things being equal, in what way is
> > > threading arguments through lots of functions, bloating the arglist,
> > > implementations and the docstrings of said functions, calling group-fun
> > > multiple times and in multiple places in minibuffer.el better than_not_
> > > doing those things?
> >
> > Why do you think it forces you to "thread" them?
>
> The current implementation in minibuffer.el threads the function down
> to the point where it is called twice again (once with the same
> second argument -- a possible inefficiency -- and then with a non-nil
> second argument). If the results of a single call were stored in
> the candidate string, this wouldn't be necessary.

By the way, another classic way to avoid threading in Lisp
is binding a dynamic variable.  We do that loads (but I
still thing storing a property is cleaner).

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 02.06.2021 02:22, João Távora wrote:

>> Firs of all, you can construct the same composite values you wanted
>> using the current calling convention of the group-function. And then use
>> them the same way.
> 
> Indeed.  Indeed that does show that we simplify the code and can keep
> the current calling convention.  But then why should we? Its akwardness
> becomes even clearer there: calling the same function twice in quick
> succession with different second arg for two different types of return value.

I think the awkwardness comes from the general organization of 
minibuffer.el rather that from this particular addition.

>> Whether this results in better minibuffer.el code is debatable: your
>> diff has more additions than deletions, and one rather long line.
> 
> Oh sure, that function is slightly more complicated, but if you look at
> 
> completion--insert-strings
> completion--insert-horizontal
> completion--insert-vertical
> completion--insert-one-column
> display-completion-list
> minibuffer-completion-help
> 
> You'll find they are all simplified.

Yeah, OK, that's a recent change I haven't read yet. But couldn't any of 
these functions fetch the current group-fun based on the minibuffer 
contents and the current global values?

completion-all-sorted-completions and minibuffer-completion-help have 
relevant code which could be extracted into a helper.

A new dynamic var, as you wrote in a later email, is also an option.

>> OTOH, someone in a different codebase might appreciate that the
>> group-function decouples the mapping to group names from the
>> transformations of the items, and thus one can do the latter closer to
>> where the items are printed, instead of having to "thread" the
>> transformed values to their place of use (from where the grouping logic
>> was called).
> 
> Exactly, but my proposal doesn't prevent doing this near the
> print locus.

One might do the grouping and the printing at different stages (in 
different functions).

>> Using a text property is a handy workaround, but ideally one shouldn't
>> have to use it.
> 
> There's nothing wrong with it? If you see a transformation function as
> a property of a candidate (which it is in reality -- it's given by a fucntion of
> a candidate), then it's something that fits naturally.  If a candidate was
> some non-string object, I doubt that there would be any doubt as to what
> to do here.  But just because it's a string doesn't change things that much.

It's a property (method) of a completion table, isn't it?

The way I see it, we actually combine there two distinct actions 
(grouping and transform), only in the interest of less typing or 
whatever. But as the actions are distinct, it makes sense to be able to 
ask for one or the other.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, Jun 2, 2021 at 12:46 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
> > Indeed.  Indeed that does show that we simplify the code and can keep
> > the current calling convention.  But then why should we? Its akwardness
> > becomes even clearer there: calling the same function twice in quick
> > succession with different second arg for two different types of return value.
>
> I think the awkwardness comes from the general organization of
> minibuffer.el rather than from this particular addition.

Yes, yes.  Just that this particular addition pulls things in the wrong
direction, in my opinion.

Another pain point in minibuffer is the representation of candidates.
Sometimes they  are strings, sometimes they are lists of strings with
prefixes and suffixes, lots of silly "if consp".  Really should all be slots
in a completion object.   If that object is a string, text properties are a
good choice (but we could even abstract the implementation away,
though it could be slow).

> >> Whether this results in better minibuffer.el code is debatable: your
> >> diff has more additions than deletions, and one rather long line.
> >
> > Oh sure, that function is slightly more complicated, but if you look at
> >
> > completion--insert-strings
> > completion--insert-horizontal
> > completion--insert-vertical
> > completion--insert-one-column
> > display-completion-list
> > minibuffer-completion-help
> >
> > You'll find they are all simplified.
>
> Yeah, OK, that's a recent change I haven't read yet. But couldn't any of
> these functions fetch the current group-fun based on the minibuffer
> contents and the current global values?

Yes, but in that case, a dynamic variable is better than repeating
that little md dance.

> > Exactly, but my proposal doesn't prevent doing this near the
> > print locus.
> One might do the grouping and the printing at different stages (in
> different functions).

Yes.

> It's a property (method) of a completion table, isn't it?

We're talking about different things. The group function is a property
of the completion table.  Each invocation transforms a completion,
meaning it produces a property of a completion.  An that property could
be stored in the completion, whatever the format.

> The way I see it, we actually combine there two distinct actions
> (grouping and transform), only in the interest of less typing or
> whatever. But as the actions are distinct, it makes sense to be able to
> ask for one or the other.

They're related in the sense that one of them doesn't make
sense without the other.  So it's not only in the interest of
typing, I'd say.

-- 
João Távora

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 02.06.2021 03:02, João Távora wrote:
> On Wed, Jun 2, 2021 at 12:46 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
>>> Indeed.  Indeed that does show that we simplify the code and can keep
>>> the current calling convention.  But then why should we? Its akwardness
>>> becomes even clearer there: calling the same function twice in quick
>>> succession with different second arg for two different types of return value.
>>
>> I think the awkwardness comes from the general organization of
>> minibuffer.el rather than from this particular addition.
> 
> Yes, yes.  Just that this particular addition pulls things in the wrong
> direction, in my opinion.
> 
> Another pain point in minibuffer is the representation of candidates.
> Sometimes they  are strings, sometimes they are lists of strings with
> prefixes and suffixes, lots of silly "if consp".  Really should all be slots
> in a completion object.

Maybe, maybe not. Sometimes we're dealing with a lot of completions, 
though (like 10s of thousands). It would not be great to see some extra 
latency because of a "cleanup" like that.

The cases you are referring to could be solved with a "completions and 
some extra info" struct or several, I think. Where one of the fields is 
the list of completion strings.

> If that object is a string, text properties are a
> good choice (but we could even abstract the implementation away,
> though it could be slow).

Text properties are a good choice in a lot of cases. If we try to turn 
each completion into a struct, that would limit fields that could be 
stored. Add some versioning concerns, etc.

>>>> Whether this results in better minibuffer.el code is debatable: your
>>>> diff has more additions than deletions, and one rather long line.
>>>
>>> Oh sure, that function is slightly more complicated, but if you look at
>>>
>>> completion--insert-strings
>>> completion--insert-horizontal
>>> completion--insert-vertical
>>> completion--insert-one-column
>>> display-completion-list
>>> minibuffer-completion-help
>>>
>>> You'll find they are all simplified.
>>
>> Yeah, OK, that's a recent change I haven't read yet. But couldn't any of
>> these functions fetch the current group-fun based on the minibuffer
>> contents and the current global values?
> 
> Yes, but in that case, a dynamic variable is better than repeating
> that little md dance.

If you want to move group-fun from function arguments to a dynamic var, 
I'm not going not object. It seems reasonable to make it less prominent 
in the code, since it's an optional feature.

But speaking of pulling things in the right direction, I think one of 
the main troubles on minibuffer.el is a lot of the implicit, untyped 
global state. And that change would add to it. The "little md dance", on 
the other hand, makes things explicit.

>> It's a property (method) of a completion table, isn't it?
> 
> We're talking about different things. The group function is a property
> of the completion table.  Each invocation transforms a completion,
> meaning it produces a property of a completion.  An that property could
> be stored in the completion, whatever the format.

You are describing the action of caching. Which is a fine thing for the 
caller to do, but there's no need to make the API less flexible than it 
is now.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, Jun 2, 2021 at 12:48 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 02.06.2021 03:02, João Távora wrote:
> > On Wed, Jun 2, 2021 at 12:46 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
> >>> Indeed.  Indeed that does show that we simplify the code and can keep
> >>> the current calling convention.  But then why should we? Its akwardness
> >>> becomes even clearer there: calling the same function twice in quick
> >>> succession with different second arg for two different types of return value.
> >>
> >> I think the awkwardness comes from the general organization of
> >> minibuffer.el rather than from this particular addition.
> >
> > Yes, yes.  Just that this particular addition pulls things in the wrong
> > direction, in my opinion.
> >
> > Another pain point in minibuffer is the representation of candidates.
> > Sometimes they  are strings, sometimes they are lists of strings with
> > prefixes and suffixes, lots of silly "if consp".  Really should all be slots
> > in a completion object.
>
> Maybe, maybe not. Sometimes we're dealing with a lot of completions,
> though (like 10s of thousands). It would not be great to see some extra
> latency because of a "cleanup" like that.

[ Mind you I'm not proposing completions-as-structs in earnest right
now, I think strings as containers of completions are fine for now.
Just food for thought. ]

If each completion was, say a struct with the completion text, and
some standard fields, I would expect to see a speedup, since struct
access is very fast.

The downside is the loss of flexibility, and that's why when taking
this approach it is common to make these structs also carry a 'plist' slot
where performance isn't paramount.

In terms of usability and speed of these approaches (propertized string
or struct or something else) would be faster than "sometimes string,
sometimes cons, let's just test it here and there and everywhere".

> The cases you are referring to could be solved with a "completions and
> some extra info" struct or several, I think. Where one of the fields is
> the list of completion strings.

OK, but that's on another level. We do have per-completion properties which
could possibly not be modeled as string properties.  It's much faster
to access say, the completion score for a given company from a slot,
rather than from a text property.  Other less important things could stay in
the string property list (or in a plist of said hypothetical struct,
which should
be about the same in terms of cost).

> > If that object is a string, text properties are a
> > good choice (but we could even abstract the implementation away,
> > though it could be slow).
> Text properties are a good choice in a lot of cases. If we try to turn
> each completion into a struct, that would limit fields that could be
> stored.

See above.  Keep using the string plist for that, or use a plist

> Add some versioning concerns, etc.

Defstruct is like a macro yes.  So you handle it like you do macros,
recompilation of its users is needed, unless you add slots in order.
But what exactly is the "versioning concern etc" scenario that
you're envisoning?  Is it older backends compiled for older Emacs working
with newer Emacsen?  As long as you don't mess with the order of slots,
you should be fine.  And that's only if you decide to make the slots
public, which doesn't necessarily need to happen.

> But speaking of pulling things in the right direction, I think one of
> the main troubles on minibuffer.el is a lot of the implicit, untyped
> global state. And that change would add to it. The "little md dance", on
> the other hand, makes things explicit.

The little md dance is the same.  In the end it relies on global state
as well, unless you thread the whole thing down again.

Anyway, I agree with you and that's basically why I prefer keeping
properties of completion objects in the completion object.  I'm not a
fan of the global state either for exactly the reasons you list.

> You are describing the action of caching.

Yes, but caching wouldn't be done by the backend/table/API consumer.

> Which is a fine thing for the
> caller to do, but there's no need to make the API less flexible than it
> is now.

It wouldn't be less flexible.  Having a "group fun" take a single completion
string and no other args and return its group and a transformer function isn't
less flexible.  Alternatively having a "group fun" take a completion string
and fills in its "group" and "transform" properties is also no less flexible.
And both these options are, IMO, better than having a "group fun" take a
second boolean parameter telling it which of two actions to perform, because
of (yet) unproven performance concerns.  It seems pretty clear that that's
letting  the implementation spill into the API.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 02.06.2021 15:59, João Távora wrote:

> In terms of usability and speed of these approaches (propertized string
> or struct or something else) would be faster than "sometimes string,
> sometimes cons, let's just test it here and there and everywhere".

Not necessarily: a struct is extra allocation and extra indirection to 
get the string. Also could slow down the hot path (the 'all-completions' 
call).

>> The cases you are referring to could be solved with a "completions and
>> some extra info" struct or several, I think. Where one of the fields is
>> the list of completion strings.
> 
> OK, but that's on another level. We do have per-completion properties which
> could possibly not be modeled as string properties.  It's much faster
> to access say, the completion score for a given company from a slot,
> rather than from a text property.  Other less important things could stay in
> the string property list (or in a plist of said hypothetical struct,
> which should
> be about the same in terms of cost).

Another problem with such structs is it encourages "materializing" 
properties for all completions ahead of time, instead of doing it 
lazily. Or if we put a function in every its optional slot, that would 
be both non-obvious and smell of over-engineering.

>> But speaking of pulling things in the right direction, I think one of
>> the main troubles on minibuffer.el is a lot of the implicit, untyped
>> global state. And that change would add to it. The "little md dance", on
>> the other hand, makes things explicit.
> 
> The little md dance is the same.  In the end it relies on global state
> as well, unless you thread the whole thing down again.

Everything ultimately relies on the global state, but this way one can 
trace the logic from the call site to find out how the group function is 
retrieved. Both alternatives (getting the transformer from the text 
property and binding group-fun to a dynamic var) rely on "magical" 
knowledge of sorts, without a comparable handy way to find how the value 
was computed (Grep will help, of course, but it leaves more opportunity 
to set the wrong value, or fail to set it in some contexts).

> Anyway, I agree with you and that's basically why I prefer keeping
> properties of completion objects in the completion object.  I'm not a
> fan of the global state either for exactly the reasons you list.

Threading MD, like Stefan suggested later, seems like a good alternative 
to me.

>> Which is a fine thing for the
>> caller to do, but there's no need to make the API less flexible than it
>> is now.
> 
> It wouldn't be less flexible.  Having a "group fun" take a single completion
> string and no other args and return its group and a transformer function isn't
> less flexible.  Alternatively having a "group fun" take a completion string
> and fills in its "group" and "transform" properties is also no less flexible.
> And both these options are, IMO, better than having a "group fun" take a
> second boolean parameter telling it which of two actions to perform, because
> of (yet) unproven performance concerns.  It seems pretty clear that that's
> letting  the implementation spill into the API.

Now that I understand it better, I say that your proposal is fine (the 
first one in the paragraph above, at least), but I (obviously) don't 
agree that that the current one is as ugly as you paint it. It's also 
more in line with the current shape of the c-a-p-f API (have a 
xyz-function return a struct where one of the values if also a function 
is a bit out there).

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, Jun 2, 2021 at 7:29 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
> Not necessarily: a struct is extra allocation and extra indirection to
> get the string. Also could slow down the hot path (the 'all-completions'
> call).

The extra indirection is the cost of an aref, so a tiny increment
to a constant factor that I beliieve is quite large.  You'd pay that with
the saved IFs alone, I suppose. The allocation is about the same,  since
you're allocating less string properties.  This is just a box, and a string
is already a box.

> Another problem with such structs is it encourages "materializing"
> properties for all completions ahead of time, instead of doing it
> lazily. Or if we put a function in every its optional slot, that would
> be both non-obvious and smell of over-engineering.

Off, no don't do that.  If you want indirection, don't use structs, use eieio
objects (CLOS-likes).  You only use structs where you need fast access to.

Indeed structs really shine when very fast access to very commonly
used properties are is required. But we have only minor hints of
these so we'd have to profile.   Structs were just an example, I just
talking about an ADT.

> Now that I understand it better, I say that your proposal is fine (the
> first one in the paragraph above, at least), but I (obviously) don't
> agree that that the current one is as ugly as you paint it.

I don't paint it as particularly ugly (or rather, for me, everything is ugly,
there is only uglier and less ugly). I was just noting an alternative and
looking at its pros and cons.

> It's also
> more in line with the current shape of the c-a-p-f API (have a
> xyz-function return a struct where one of the values if also a function
> is a bit out there).

Are higher-order functions in Elisp APIs really that uncommon? It's the
daily bread in lots of stuff I work on, like capf  for one (maybe that's what
you don't like about it?)  Since lexical binding, they are really really
cool to use in my opinion.  Not gratutiously of course, but for it's
good not to shun them.

João

complexity in minibuffer (was: (icomplete-vertical-mode): Add support for affixations and, annotations)

[Stefan Monnier]

> Another pain point in minibuffer is the representation of candidates.
> Sometimes they  are strings, sometimes they are lists of strings with
> prefixes and suffixes, lots of silly "if consp".

I must say I'm not sure what you're referring to here.
So I went searching for `consp` in minibuffer.el, and IIUC you're
talking about the code which takes a set of completions and displays it
in *Completions*.  E.g. the code in `completion--insert-*`.
Is that right?

[ If so, it explains why I wasn't sure what you referred to, since it's
  not part of the code with which I'm familiar ;-)  ]

Indeed, I wonder why we have that.

I know `display-completion-list` is documented to work with lists of
various things (strings, symbols, and lists of strings, IIRC), but
that's an old "feature" of `display-completion-list` that's not used by
the current completion code, AFAIK (and potentially not used by any code
at all).

So we could have a quick loop through the completion candidates at the
entry of `display-completion-list` which converts each candidate to
a plain string and then calls a streamlined `completion--insert-strings`
where we don't need those `consp` any more.
[ And in most cases we could call `completion--insert-strings` directly
  rather than go through the old `display-completion-list`.  ]

WDYT?  Am I missing something?


        Stefan

Re: complexity in minibuffer (was: (icomplete-vertical-mode): Add support for affixations and, annotations)

[João Távora]

On Wed, Jun 2, 2021 at 3:19 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> I must say I'm not sure what you're referring to here.
> So I went searching for `consp` in minibuffer.el, and IIUC you're
> talking about the code which takes a set of completions and displays it
> in *Completions*.  E.g. the code in `completion--insert-*`.
> Is that right?

Yes (but on only)  And those were the functions who now get an optional
group-fun arg (also often 'if'ed against sometimes only threaded).

> [ If so, it explains why I wasn't sure what you referred to, since it's
>   not part of the code with which I'm familiar ;-)  ]
>
> Indeed, I wonder why we have that.

I think it has to do with the way Juri (or someone else) implemented
affixation function and annotation function, which was what I was trying
to touch to experiment with resolution-function ideas. Sometimes as completion
is represented by a cons (string annotation-prefix annotation-suffix)
sometimes only a string.  It could be a string always, with properties.
Or a struct.  But let's leave it at string and never cons.

> So we could have a quick loop through the completion candidates at the
> entry of `display-completion-list` which converts each candidate to
> a plain string and then calls a streamlined `completion--insert-strings`
> where we don't need those `consp` any more.
> [ And in most cases we could call `completion--insert-strings` directly
>   rather than go through the old `display-completion-list`.  ]
>
> WDYT?  Am I missing something?

Works, but can't we just find we're making the (presumably) useless cons
and just do a propertized string there?   I'd prefer to take a clearer look at
where this messiness was introduced and clean it there.  Which
is commonly why I look at other nearby cleanup opportunities and bring
them up here.  From where I stand this time (code only in master) is when
to strike the iron with the "aesthetics" hammer, which for me normally
translates to "use less code that does the same (and usually quicker)".

João

Re: complexity in minibuffer

[Stefan Monnier]

> Yes (but on only)  And those were the functions who now get an optional
> group-fun arg (also often 'if'ed against sometimes only threaded).

I suspect they should get an `md` argument instead (which will include
the `group-function`, of course).

> I think it has to do with the way Juri (or someone else) implemented
> affixation function and annotation function, which was what I was trying
> to touch to experiment with resolution-function ideas. Sometimes as completion
> is represented by a cons (string annotation-prefix annotation-suffix)
> sometimes only a string.  It could be a string always, with properties.
> Or a struct.  But let's leave it at string and never cons.

I don't have a strong opinion here, except that I think it would make
sense to handle `group-function` and `annotation/affixation-function` at
the same place.  This will likely make much more of a difference than
the details of the way those functions are called.  There's a chance it
will let us only use strings (either the actual "raw candidate", or its
pretty-printed rendering) and never need to pass sets of lists
of strings.


        Stefan

Re: complexity in minibuffer

[João Távora]

On Wed, Jun 2, 2021 at 4:06 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > Yes (but on only)  And those were the functions who now get an optional
> > group-fun arg (also often 'if'ed against sometimes only threaded).
>
> I suspect they should get an `md` argument instead (which will include
> the `group-function`, of course).

That works, of course.  Many things work, but this is not the simplest.

By that time, you have already traversed the completion list many times.
What's the problem with having the completions themselves contain
all that's needed to render them? A negligible amount of allocation in
a ocean of allocations? Why do we have to pass down the API call
all to way to the display?

Let's have it so that at a certain point, a collection of completions
(whatever the format of collection and/or completion) is  enough to draw
them, no more dependency on global state or multiple funky arguments
that are hard to mock.  Less if's. Great for automated testing, too.

> > I think it has to do with the way Juri (or someone else) implemented
> > affixation function and annotation function, which was what I was trying
> > to touch to experiment with resolution-function ideas. Sometimes as completion
> > is represented by a cons (string annotation-prefix annotation-suffix)
> > sometimes only a string.  It could be a string always, with properties.
> > Or a struct.  But let's leave it at string and never cons.
>
> I don't have a strong opinion here, except that I think it would make
> sense to handle `group-function` and `annotation/affixation-function` at
> the same place.

You do have to call the first on all of the completions and the latter only
on a subset to be displayed.  That's a constraint of the problem itself.

João

Re: complexity in minibuffer

[Dmitry Gutov]

On 02.06.2021 18:20, João Távora wrote:
> By that time, you have already traversed the completion list many times.
> What's the problem with having the completions themselves contain
> all that's needed to render them?

In any of those "many times", did you already know which completions 
will be displayed, and which will not?

Some transformations will be more costly than others (even if the 
current examples are pretty fast), so we shouldn't call them unless we 
know a given completion will be displayed.

Re: complexity in minibuffer

[João Távora]

On Wed, Jun 2, 2021 at 4:29 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 02.06.2021 18:20, João Távora wrote:
> > By that time, you have already traversed the completion list many times.
> > What's the problem with having the completions themselves contain
> > all that's needed to render them?
>
> In any of those "many times", did you already know which completions
> will be displayed, and which will not?
>
> Some transformations will be more costly than others (even if the
> current examples are pretty fast), so we shouldn't call them unless we
> know a given completion will be displayed.

You're again misunderstanding my suggestion, and bringing
up the same concern, which I've already answered. Let me
try again. Call the total group of completions A. Invoke the group func
for every c in A, yes.  _Don'_t transform every completion until you know
which ones to display, call this group D.  Instead allocate a transform
function and attach it to each c in A.  Then call this transform for each
c in D.

Currently, one calls group func for every c in A, to get the
group only.  Then one calls it again for c in D to get the group again
(call this invocation X) and finally call it again for c in D to get the
transformation.

My suggestion can also scrap the double invocation X (getting
the group may be expensive, for some backends).

João

Re: complexity in minibuffer

[Dmitry Gutov]

On 02.06.2021 18:37, João Távora wrote:

>> Some transformations will be more costly than others (even if the
>> current examples are pretty fast), so we shouldn't call them unless we
>> know a given completion will be displayed.
> 
> You're again misunderstanding my suggestion, and bringing
> up the same concern, which I've already answered. Let me
> try again. Call the total group of completions A. Invoke the group func
> for every c in A, yes.  _Don'_t transform every completion until you know
> which ones to display, call this group D.  Instead allocate a transform
> function and attach it to each c in A.  Then call this transform for each
> c in D.

I think I get it now, thanks: key-and-transform, which you proposed 
group-function would return, would have a transformer _function_ in its 
CDR. So overall it should be approximately as powerful as the current 
approach. An allocation of 10000 lambdas in the interpreter takes about 
4 ms here, which is not too much.

I could still describe some downsides (opaque properties on strings: 
harder to examine them; longer backtraces - that is something you have 
criticized in another proposal of mine, in a different discussion).

Overall it's a valid idea, not without its elegance, but I don't think 
it's a definite enough improvement to ask everybody to rewrite their 
code, which uses the current definition of group-function.

> My suggestion can also scrap the double invocation X (getting
> the group may be expensive, for some backends).

If that ever turns out to be a problem, we can of course use text 
properties and/or a hash table somewhere. But this is an argument about 
the implementation strategy for minibuffer.el, whereas the 
group-function convention affects more than that.

Re: complexity in minibuffer

[João Távora]

On Wed, Jun 2, 2021 at 7:11 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> I think I get it now, thanks: key-and-transform, which you proposed
> group-function would return, would have a transformer _function_ in its
> CDR. So overall it should be approximately as powerful as the current
> approach. An allocation of 10000 lambdas in the interpreter takes about
> 4 ms here, which is not too much.

Yes, you got it.

Your numbers, in line with my measurements here.  But it varies (not
much) if the lambdas are closures or mere functions (where I suppose
the compiler would only allocate one of them).

Mind you that we could _also_  allow the backend to return the
transformation immediately if it's cheap, happens to be already
available somehow. That, in turn, would speed up scrolling, for example.

> Overall it's a valid idea, not without its elegance, but I don't think
> it's a definite enough improvement to ask everybody to rewrite their
> code, which uses the current definition of group-function.

Right, it was just a passing suggestion, an idea among many,
as I was looking at minibuffer.el for ways to rework
annotation/completions display and stuff.  Really wound up defending
waaay more than I expected.

As far as I understand only minibuffer.el and vertico.el use group-fn.
It's quite new and in master so that kind of impact would be gigantic.

João

Re: complexity in minibuffer

[Stefan Monnier]

> By that time, you have already traversed the completion list many times.
> What's the problem with having the completions themselves contain
> all that's needed to render them?

If you have 10K completions, the time need to compute their rendering
becomes very significant, so we want to only compute the rendering for
those completion which actually reach the glass.

> Let's have it so that at a certain point, a collection of completions
> (whatever the format of collection and/or completion) is  enough to draw
> them, no more dependency on global state or multiple funky arguments
> that are hard to mock.  Less if's. Great for automated testing, too.

That can be done once you know which completions can be displayed, yes.

Basically, we need a function that takes a (potentially large) set of
"raw candidates" and turns it into the (hopefully smallish) set of
rendered candidates which we'll actually display.

>> I don't have a strong opinion here, except that I think it would make
>> sense to handle `group-function` and `annotation/affixation-function` at
>> the same place.
> You do have to call the first on all of the completions and the latter only
> on a subset to be displayed.  That's a constraint of the problem itself.

Not necessarily: you could also decide to only group those completions
which are displayed.


        Stefan

Re: complexity in minibuffer

[João Távora]

On Wed, Jun 2, 2021 at 4:38 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > By that time, you have already traversed the completion list many times.
> > What's the problem with having the completions themselves contain
> > all that's needed to render them?
>
> If you have 10K completions, the time need to compute their rendering
> becomes very significant, so we want to only compute the rendering for
> those completion which actually reach the glass.

Same answer as to Dmitry.  Did you miss the fact from a lot of email ago
that I'm proposing attaching a function to each completions to be called
just-in-time?  The only extra cost is the attachment of this function to the
completion.

The benchmarks I did were with 45K completions and showed a small
overhead, just for the group-fun calling bit.  So likely even more
diluted once you consider each completion iteration.

> Not necessarily: you could also decide to only group those completions
> which are displayed.

Hmm, that would be great yes, but as far as I understand grouping changes
order so you can't know the second without doing the first.  Like sorting.
Unless you mean "decide" as in "dramatically changing the semantics of
grouping".

João

Re: complexity in minibuffer

[Daniel Mendler]

On 6/2/21 5:45 PM, João Távora wrote:
>> If you have 10K completions, the time need to compute their rendering
>> becomes very significant, so we want to only compute the rendering for
>> those completion which actually reach the glass.
> 
> Same answer as to Dmitry.  Did you miss the fact from a lot of email ago
> that I'm proposing attaching a function to each completions to be called
> just-in-time?  The only extra cost is the attachment of this function to the
> completion.

Do you prefer attaching the annotation functions to each completion
candidate over passing down the functions or the metadata to the display
functions directly as argument? This sounds like an unnecessary mutation
of the candidates and it is more complicated.

If you ask me, threading the argument down is the better approach. And
as you mentioned there is also the option to use a dynamically bound
variable, in case you want to avoid passing the metadata down explicitly
as an argument. I prefer to use an explicit function argument, over
introducing global variables, but this is clearly a matter of taste.

Daniel

Re: complexity in minibuffer

[João Távora]

> This sounds like an unnecessary mutation
> of the candidates and it is more complicated.

This is what I value:

- Rendering is less coupled to state or extra function arguments so
  it's good for debugging, unit testing and eventually rewriting the
  completion data structures.
- Each of the intervening rendering functions has less code.

You may deem the above "unnecessary", that's fine.  I find it useful
and _less_ complicated as a whole.

> If you ask me, threading the argument down is the better approach.

You have made that abundantly clear many times now.

João

Re: complexity in minibuffer

[Daniel Mendler]

On 6/2/21 5:38 PM, Stefan Monnier wrote:
>> By that time, you have already traversed the completion list many times.
>> What's the problem with having the completions themselves contain
>> all that's needed to render them?
> 
> If you have 10K completions, the time need to compute their rendering
> becomes very significant, so we want to only compute the rendering for
> those completion which actually reach the glass.

Yes, it is crucial to only compute
annotations/affixations/group-transformed-candidates only for the
visible candidates, since all these computations may be expensive.

I think we should make this part of the `completing-read` definition,
that these functions are allowed to do more expensive work, since they
are always called on a small subset of candidates, which are going to be
displayed.

>>> I don't have a strong opinion here, except that I think it would make
>>> sense to handle `group-function` and `annotation/affixation-function` at
>>> the same place.
>> You do have to call the first on all of the completions and the latter only
>> on a subset to be displayed.  That's a constraint of the problem itself.
> 
> Not necessarily: you could also decide to only group those completions
> which are displayed.

For the default completion UI this is the case. But this is not how
grouping works for UIs like Vertico, Selectrum and Icomplete-vertical,
where the first grouping step has to happen after sorting on all
candidates. The grouping candidate transformation can then come later
only for the visible candidates and can then be comparatively more
expensive.

Daniel

Re: complexity in minibuffer

[Dmitry Gutov]

On 02.06.2021 18:06, Stefan Monnier wrote:
> I suspect they should get an `md` argument instead (which will include
> the `group-function`, of course).

Good idea: this way the argument is not tied to an optional feature, but 
would OTOH provide renderers' access to any such feature that we might 
add in the future.

Re: complexity in minibuffer

[João Távora]

On Wed, Jun 2, 2021 at 7:31 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 02.06.2021 18:06, Stefan Monnier wrote:
> > I suspect they should get an `md` argument instead (which will include
> > the `group-function`, of course).
>
> Good idea: this way the argument is not tied to an optional feature, but
> would OTOH provide renderers' access to any such feature that we might
> add in the future.

Agree  we have to give _some_ state of the completion operation
to the renderer, not just the completions themselves.   But lets
concentrate that state as much as possible.  Ideally, as I think
you once suggested, we'd have a "completion operation instance"
object/ADT that contains the collection of completions and all the
paraphernalia used to manipulate it.  We'd pass _that_ around,
instead of md's, minibuffer-* special vars, completion-extra-properties
and whatnot.  Not sure if easy to realize but at least that's what
I would aim for.

João

Re: complexity in minibuffer

[Dmitry Gutov]

On 02.06.2021 22:03, João Távora wrote:
> On Wed, Jun 2, 2021 at 7:31 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>>
>> On 02.06.2021 18:06, Stefan Monnier wrote:
>>> I suspect they should get an `md` argument instead (which will include
>>> the `group-function`, of course).
>>
>> Good idea: this way the argument is not tied to an optional feature, but
>> would OTOH provide renderers' access to any such feature that we might
>> add in the future.
> 
> Agree  we have to give _some_ state of the completion operation
> to the renderer, not just the completions themselves.   But lets
> concentrate that state as much as possible.  Ideally, as I think
> you once suggested, we'd have a "completion operation instance"
> object/ADT that contains the collection of completions and all the
> paraphernalia used to manipulate it.  We'd pass _that_ around,
> instead of md's, minibuffer-* special vars, completion-extra-properties
> and whatnot.  Not sure if easy to realize but at least that's what
> I would aim for.

Ideally, perhaps, but I don't think we're at the stage where we can 
confidently implement that yet. This completion context with the session 
cache, etc.

We still might pass MD even in that case, because it's not just a 
function of the current completion, but of the current field as well. 
Depend on what the info a particular function will want/need.

Re: complexity in minibuffer

[Juri Linkov]

> I think it has to do with the way Juri (or someone else) implemented
> affixation function and annotation function, which was what I was trying
> to touch to experiment with resolution-function ideas. Sometimes as completion
> is represented by a cons (string annotation-prefix annotation-suffix)
> sometimes only a string.  It could be a string always, with properties.
> Or a struct.  But let's leave it at string and never cons.

I only added the third element to the list of '(candidate prefix suffix)'.
So either turning it into a struct, or adding text properties would be fine.
But there is no room to extend annotation-function since as can be seen
in your branch annotation-function with text properties would return such
self-referencing ugliness:

  (propertize annotation 'suffix annotation)

Or do you intend resolution-function to do the same but return
a candidate string with text properties?

Re: complexity in minibuffer

[João Távora]

On Wed, Jun 2, 2021 at 11:56 PM Juri Linkov <juri@linkov.net> wrote:
>
> I only added the third element to the list of '(candidate prefix suffix)'.

OK Thanks.  Indeed I didn't dig down.  So the problem was there before.

> So either turning it into a struct, or adding text properties would be fine.
> But there is no room to extend annotation-function since as can be seen
> in your branch annotation-function with text properties would return such
> self-referencing ugliness:
>
>   (propertize annotation 'suffix annotation)

It's not pretty, but not harmful either.  But you can ignore it since
it's pretty much settled we're going past annotation-function anyway.

> Or do you intend resolution-function to do the same but return
> a candidate string with text properties?

Do you mean the difference between having
resolution-function(cand field) return the value for field or
or attach the value for field to cand. Both can work,
but I think I prefer the first.

João



-- 
João Távora

Re: complexity in minibuffer

[Juri Linkov]

>> Or do you intend resolution-function to do the same but return
>> a candidate string with text properties?
>
> Do you mean the difference between having
> resolution-function(cand field) return the value for field or
> or attach the value for field to cand. Both can work,
> but I think I prefer the first.

I thought you prefer attaching all fields to the candidate string,
but either is fine.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

> There was a long discussion with Stefan, Dmitry,  Juri and Eli where
> multiple different designs for the `group-function` have been considered
> and we settled on the current design.

As a "static typing" guy, I'm not fond of functions which return
different types depending on their argument (tho that's exactly what
depending typing is for, admittedly), but I think the current design is
indeed close enough to the best possible choice that it's better to
leave this bikeshed with its current color.


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 6/1/21 5:58 PM, Stefan Monnier wrote:
>> There was a long discussion with Stefan, Dmitry,  Juri and Eli where
>> multiple different designs for the `group-function` have been considered
>> and we settled on the current design.
> 
> As a "static typing" guy, I'm not fond of functions which return
> different types depending on their argument (tho that's exactly what
> depending typing is for, admittedly), but I think the current design is
> indeed close enough to the best possible choice that it's better to
> leave this bikeshed with its current color.

Just to make this clear: The type of the function is not dependent. It
is a simple type.

group-function : string -> bool -> string

There are not many other possible encodings for the group function which
are better or simpler. The good thing about the current design is that
it is both simple and efficient, since it does not lead to unnecessary
allocations.

Daniel

Re:Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[tumashu]

>> * Is it possible to disable the index/count indicator? In Vertico I use
>> `vertico-count-format` for that, if nil, the indicator is disabled.
>
>I normally don't add a knob for things until someone actually requests
>it.  If you're requesting it, then it can be added (and you can too).

I suggest add this variable.

by the way, I have found that  prompt will flicker, for:  "x/xxx M-x"  ->  "M-x"  ->  "x/xxx M-x" -> "M-x"  ...

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

tumashu  <tumashu@163.com> writes:

>>I normally don't add a knob for things until someone actually requests
>>it.  If you're requesting it, then it can be added (and you can too).
> I suggest add this variable.

OK.  Patches welcome.

> by the way, I have found that prompt will flicker, for: "x/xxx M-x" ->
> "M-x" -> "x/xxx M-x" -> "M-x" ...

During potentially expensive filtering it is hidden.  It's better than
showing incorrect information, in my opinion.  It reappears once the
totals are known.  Would you rather if "flickered" to "?/?"  ?  Or would
you rather redisplay is inhibited until the totals are known?  Other
completers may use this last option, but I personally dislike sluggish
typing response.  And the whole constant updating thing is about
"flickering" anyway.

Anyway, avoiding intermediate rediplay seems possible but slightly
harder to implement in icomplete.el.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 02.06.2021 10:48, João Távora wrote:
> Would you rather if "flickered" to "?/?"  ?  Or would
> you rather redisplay is inhibited until the totals are known?  Other
> completers may use this last option, but I personally dislike sluggish
> typing response.

You can try following the recent change in Company for asynchronous 
backends, where we do redisplay soon after the user typed, briefly 
showing "outdated" completions with the new input. And then updating it 
as soon as the new result is computed.

icomplete's implementation is this approach should be a lot easier to 
do, too. This seems to work okay:

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 91bbb60013..29138e089e 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -154,7 +154,8 @@ icomplete--initial-input

  (defun icomplete-pre-command-hook ()
   (let ((non-essential t))
-   (icomplete-tidy)))
+   ;; (icomplete-tidy)
+   ))

  (defun icomplete-post-command-hook ()
    (let ((non-essential t)) ;E.g. don't prompt for password!


I would also suggest lowering the default of icomplete-compute-delay to 
0.1 or even 0.05.

With 300ms delay, the new behavior does look a little weird.

> And the whole constant updating thing is about
> "flickering" anyway.

Experience shows the above is a lot less jarring then hiding all 
completions ("blinking" with the empty background) and then showing them 
again.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Wed, Jun 2, 2021 at 11:40 AM Dmitry Gutov <dgutov@yandex.ru> wrote:

> > And the whole constant updating thing is about
> > "flickering" anyway.
> Experience shows the above is a lot less jarring then hiding all
> completions ("blinking" with the empty background) and then showing them
> again.

I don't mind what you call flickering at all, but to each his own,
as usual.

> icomplete's implementation is this approach should be a lot easier to
> do, too. This seems to work okay:
>
> diff --git a/lisp/icomplete.el b/lisp/icomplete.el
> index 91bbb60013..29138e089e 100644
> --- a/lisp/icomplete.el
> +++ b/lisp/icomplete.el
> @@ -154,7 +154,8 @@ icomplete--initial-input
>
>   (defun icomplete-pre-command-hook ()
>    (let ((non-essential t))
> -   (icomplete-tidy)))
> +   ;; (icomplete-tidy)
> +   ))

Great! i thought it would be more complicated, to be honest.

>   (defun icomplete-post-command-hook ()
>     (let ((non-essential t)) ;E.g. don't prompt for password!
>
>
> I would also suggest lowering the default of icomplete-compute-delay to
> 0.1 or even 0.05.
>
> With 300ms delay, the new behavior does look a little weird.

Can you remind us what that delay is doing there again?
Anyway, it should probably consult a customization knob.  Or
maybe the new behaviour could decide based on
icomplete-compute-delay If someone is customizing that to a
low value, turn on the anti-flicker and suffer some little latency.
If the normal value, keep the current flickering and 0-ish latency.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 02.06.2021 14:04, João Távora wrote:

>>    (defun icomplete-post-command-hook ()
>>      (let ((non-essential t)) ;E.g. don't prompt for password!
>>
>>
>> I would also suggest lowering the default of icomplete-compute-delay to
>> 0.1 or even 0.05.
>>
>> With 300ms delay, the new behavior does look a little weird.
> 
> Can you remind us what that delay is doing there again?

It waits before fetching completions if input it too short. To save on 
CPU usage or whatever.

> Anyway, it should probably consult a customization knob.  Or
> maybe the new behaviour could decide based on
> icomplete-compute-delay If someone is customizing that to a
> low value, turn on the anti-flicker and suffer some little latency.
> If the normal value, keep the current flickering and 0-ish latency.

The proposed change doesn't add any latency. And it would be helpful to 
have it working OOTB. But maybe disable if the user customizes the delay 
to be higher.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, Jun 2, 2021 at 12:28 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> It waits before fetching completions if input it too short. To save on
> CPU usage or whatever.

Makes sense.  Even though it can be interrupted, doesn't mean we
should start spinning up fans.

> The proposed change doesn't add any latency. And it would be helpful to
> have it working OOTB. But maybe disable if the user customizes the delay
> to be higher.

Indeed it doesn't, just tried it. So I have even less objection to doing it.
Do you have a take on the reasons for this `icomplete-tidy` call in
pre-command-hook?

Anyway, the weirdness (and less flickering) could be solved by
replacing xx/xx with of the same length ??/?? instead of deleting the
overlay outright. But I'm fine with the weirdness too, to be honest.
My main concern was typing latency, and it doesn't happen.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 02.06.2021 14:33, João Távora wrote:
> On Wed, Jun 2, 2021 at 12:28 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
> 
>> It waits before fetching completions if input it too short. To save on
>> CPU usage or whatever.
> 
> Makes sense.  Even though it can be interrupted, doesn't mean we
> should start spinning up fans.

OTOH, ido-mode has lived with the "spinning up fans" behavior for ages, 
and we don't hear much complaints about that. Or fan noise, really.

>> The proposed change doesn't add any latency. And it would be helpful to
>> have it working OOTB. But maybe disable if the user customizes the delay
>> to be higher.
> 
> Indeed it doesn't, just tried it. So I have even less objection to doing it.
> Do you have a take on the reasons for this `icomplete-tidy` call in
> pre-command-hook?

All the justifications for the current behavior you have come up with 
already, I suppose. Here and in the related Company discussion.

Simpler mental model, a guarantee of no "inconsistent" display, I 
suppose. But the flicking is real annoying, it's one of the things that 
has stopped my from migrating to fido-mode (somewhat lower performance 
is the other).

> Anyway, the weirdness (and less flickering) could be solved by
> replacing xx/xx with of the same length ??/?? instead of deleting the
> overlay outright. But I'm fine with the weirdness too, to be honest.
> My main concern was typing latency, and it doesn't happen.

It might make sense to keep them at the previous values, to emphasize 
that the output simply hasn't been updated yet. And to keep the prompt 
from moving around twice.

As long as the delay is not too high (meaning, we do make it lower than 
100ms), that behavior looks natural enough. The user just gets used to 
"asynchronous" updates.

Here's one edge case that might warrant some looking into:

- fido-mode on (but not -vertical-mode).
- Have, like, only 2 buffers open.
- C-x b

Start typing an existing buffer's name, in the way that would create a 
unique prefix match. icomplete will render it with cutoff prefix, like

   ico[mplete.el]

where "ico" is input. If I backspace and type "o" again, there will be 
an interval of time when two "o" will be shown. That looks kinda weird, 
but probably still better than having it blink. Hopefully, this behavior 
could still be improved, though.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, Jun 2, 2021 at 1:31 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> OTOH, ido-mode has lived with the "spinning up fans" behavior for ages,
> and we don't hear much complaints about that. Or fan noise, really.

I wonder how many people are still actually using ido today.  It
used to be extremely popular, I used it for ages.

> Simpler mental model, a guarantee of no "inconsistent" display, I
> suppose. But the flicking is real annoying, it's one of the things that
> has stopped my from migrating to fido-mode (somewhat lower performance
> is the other).

You've shown you can get rid of the flickering, be my guest (my
gripe would be input latency, but there seems to be none).

Are you using ido-enable-flex-matching?   You can try making fido use
substring completion style to see if it helps. substring completion what
bare ido uses:

  (defun joaot/my-fido-style () (when fido-mode (setq
completion-styles '(substring))))
  (add-hook 'minibuffer-setup-hook 'joaot/my-fido-style 100)

If there's still a performance discrepancy, maybe a controlled benchmark
is in order.

> where "ico" is input. If I backspace and type "o" again, there will be
> an interval of time when two "o" will be shown. That looks kinda weird,
> but probably still better than having it blink. Hopefully, this behavior
> could still be improved, though.

I'm fine if you fix this and fine if you leave it. Personally, I've switched to
fido-mode +  fido-vertical-mode  which doesn't do the [mplete.el] dance.
Personally I'm  also fine to remove this complexity altogether, even for
non-vertical Icomplete/Fido, but I think at least Stefan likes it (possibly
others, but no record thereof).

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 02.06.2021 16:16, João Távora wrote:

> I wonder how many people are still actually using ido today.  It
> used to be extremely popular, I used it for ages.

Still fairly popular, though the newer completion frameworks have 
popularized different bindings, alas.

>> Simpler mental model, a guarantee of no "inconsistent" display, I
>> suppose. But the flicking is real annoying, it's one of the things that
>> has stopped my from migrating to fido-mode (somewhat lower performance
>> is the other).

By the way, another thing I'm kind of missing is different faces. One 
for input, and another for directories in the completions list.

That's less critical, though.

> Are you using ido-enable-flex-matching?

Sure. Also see bug#12796.

> If there's still a performance discrepancy, maybe a controlled benchmark
> is in order.

I've filed bug#48841 with some details.

It's apples to oranges, of course (Ido doesn't do any "smart" sorting), 
but what can we do.

>> where "ico" is input. If I backspace and type "o" again, there will be
>> an interval of time when two "o" will be shown. That looks kinda weird,
>> but probably still better than having it blink. Hopefully, this behavior
>> could still be improved, though.
> 
> I'm fine if you fix this and fine if you leave it. Personally, I've switched to
> fido-mode +  fido-vertical-mode  which doesn't do the [mplete.el] dance.

The new icomplete-vertical-mode is pretty nice, but ivy-posframe still 
kicks it out of the park for my usage.

> Personally I'm  also fine to remove this complexity altogether, even for
> non-vertical Icomplete/Fido, but I think at least Stefan likes it (possibly
> others, but no record thereof).

Yeah, removing features is hard.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> If there's still a performance discrepancy, maybe a controlled benchmark
>> is in order.
>
> I've filed bug#48841 with some details.
>
> It's apples to oranges, of course (Ido doesn't do any "smart"
> sorting), but what can we do.

OK, I will take a look.

>>> where "ico" is input. If I backspace and type "o" again, there will be
>>> an interval of time when two "o" will be shown. That looks kinda weird,
>>> but probably still better than having it blink. Hopefully, this behavior
>>> could still be improved, though.
>> I'm fine if you fix this and fine if you leave it. Personally, I've
>> switched to
>> fido-mode +  fido-vertical-mode  which doesn't do the [mplete.el] dance.
>
> The new icomplete-vertical-mode is pretty nice, but ivy-posframe still
> kicks it out of the park for my usage.

What are the main differences, summarized?  Does it work in a tty emacs?

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 05.06.2021 12:21, João Távora wrote:
> What are the main differences, summarized?

It's just a way to show the completion rectangle above the middle of the 
frame (or in any other predefined position inside the frame), instead of 
having all the windows jump when it appears. Using a child frame.

> Does it work in a tty emacs?

Definitely not. Someday we'll have a popup library that works in the 
terminal and across windows, but not today.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 05.06.2021 12:21, João Távora wrote:
>> What are the main differences, summarized?
>
> It's just a way to show the completion rectangle above the middle of
> the frame (or in any other predefined position inside the frame),
> instead of having all the windows jump when it appears. Using a child
> frame.

Ah ok, I figured.  But not all the windows jump with fido-mode or
multi-line echo area's: only the windows that reach to the bottom of
the frame do, I think.  Personally, I'm so used to this that it doesn't
bother me a bit.

>> Does it work in a tty emacs?
>
> Definitely not. Someday we'll have a popup library that works in the
> terminal and across windows, but not today.

Yes, that is the solution.  The TTY mouse menus are extremely good now.
Some time ago, I had a look at the C code and it didn't seem
transcendental, but I have too much on my plate right now.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

> I'm fine if you fix this and fine if you leave it. Personally, I've switched to
> fido-mode +  fido-vertical-mode  which doesn't do the [mplete.el] dance.
> Personally I'm  also fine to remove this complexity altogether, even for
> non-vertical Icomplete/Fido, but I think at least Stefan likes it (possibly
> others, but no record thereof).

I'm not very much attached to the "[mplete.el] dance", FWIW.
I do think it helps cram a fair bit more candidates in the small amount
of space usually available in horizontal icomplete, so I suspect other
users may care, but I'm not one of them because I don't use icomplete
any more (well, I do enable it occasionally, but it's fairly rare).


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I'm not very much attached to the "[mplete.el] dance", FWIW.
> I do think it helps cram a fair bit more candidates in the small amount
> of space usually available in horizontal icomplete, so I suspect other
> users may care, but I'm not one of them because I don't use icomplete
> any more (well, I do enable it occasionally, but it's fairly rare).

So either we implement cloud-based business intelligence event tracking
thingies or just remove it and whistle unsuspiciously

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Ergus]

Hi:

Just a detail

When using icomplete-vertical, probably fido-mode should bind
icomplete-forward-completions and icomplete-backward-completions to
<down> <up> instead of <right> <left> similar to C-n/C-p 



On Sat, Jun 05, 2021 at 12:23:00AM -0400, Stefan Monnier wrote:
>> I'm fine if you fix this and fine if you leave it. Personally, I've switched to
>> fido-mode +  fido-vertical-mode  which doesn't do the [mplete.el] dance.
>> Personally I'm  also fine to remove this complexity altogether, even for
>> non-vertical Icomplete/Fido, but I think at least Stefan likes it (possibly
>> others, but no record thereof).
>
>I'm not very much attached to the "[mplete.el] dance", FWIW.
>I do think it helps cram a fair bit more candidates in the small amount
>of space usually available in horizontal icomplete, so I suspect other
>users may care, but I'm not one of them because I don't use icomplete
>any more (well, I do enable it occasionally, but it's fairly rare).
>
>
>        Stefan
>
>

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 05.06.2021 07:23, Stefan Monnier wrote:
>> I'm fine if you fix this and fine if you leave it. Personally, I've switched to
>> fido-mode +  fido-vertical-mode  which doesn't do the [mplete.el] dance.
>> Personally I'm  also fine to remove this complexity altogether, even for
>> non-vertical Icomplete/Fido, but I think at least Stefan likes it (possibly
>> others, but no record thereof).
> 
> I'm not very much attached to the "[mplete.el] dance", FWIW.
> I do think it helps cram a fair bit more candidates in the small amount
> of space usually available in horizontal icomplete,

It's actually disabled in fido-mode, except for the "sole remaining 
match" case, so we're not really cramming them in. So it's an aesthetic 
choice at this point.

> so I suspect other
> users may care, but I'm not one of them because I don't use icomplete
> any more (well, I do enable it occasionally, but it's fairly rare).

Did you go back to the standard completion UI?

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

>> so I suspect other users may care, but I'm not one of them because
>> I don't use icomplete any more (well, I do enable it occasionally,
>> but it's fairly rare).
> Did you go back to the standard completion UI?

Yup ;-)


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

> Indeed it doesn't, just tried it. So I have even less objection to doing it.
> Do you have a take on the reasons for this `icomplete-tidy` call in
> pre-command-hook?

I think it was/is the fear that the overlay could interfere with
the command.  AFAIK it was written under the assumption that the
`icomplete-tidy` call would be automatically "undone" by the
`post-command-hook` before the next redisplay.


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 02.06.2021 16:38, Stefan Monnier wrote:
> I think it was/is the fear that the overlay could interfere with
> the command.

Same as the reason for Company's frontends doing that, then (only more 
founded in that case). If somebody reports a real problem caused by 
this, we can copy the corresponding fix more thoroughly:

- Remove the after-string from the overlay (moving its value to some 
other property) in pre-command.
- Restore it right away in post-command. And then update after the 
computation is made.

But we probably don't need this additional complexity.

> AFAIK it was written under the assumption that the
> `icomplete-tidy` call would be automatically "undone" by the
> `post-command-hook` before the next redisplay.

Are you okay with the proposed change, then?

And how do you feel about reducing icomplete-compute-delay to 100ms at most?

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

>> AFAIK it was written under the assumption that the
>> `icomplete-tidy` call would be automatically "undone" by the
>> `post-command-hook` before the next redisplay.
> Are you okay with the proposed change, then?

I think it's worth a try.

> And how do you feel about reducing icomplete-compute-delay to 100ms at most?

I don't have an opinion on that.

I do think that 0.05s would be worse than 0s, OTOH (in the sense that
I think it would have the downside that it's not immediate and it
requires 2 redisplays, but without the upside of skipping some
computation while the user is typing since 0.05s is short enough that it
will still trigger between keys in many/most cases).


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, Jun 2, 2021 at 3:54 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> >> AFAIK it was written under the assumption that the
> >> `icomplete-tidy` call would be automatically "undone" by the
> >> `post-command-hook` before the next redisplay.
> > Are you okay with the proposed change, then?
>
> I think it's worth a try.
>
> > And how do you feel about reducing icomplete-compute-delay to 100ms at most?

For me maybe also not much less than that. For the first few chars, I
usuallly type fast enough, and I appreciate this CPU saving measure.

150ms down from 300ms maybe a good compromise to cut down on the
xx/xx transient inconsistency and still have the CPU saving thing.

Even better, as I suggested, is to make that proposed change be
conditional on the delay.  Then people wanting no flicker just reduce
the delay, which is quite intuitive.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 02.06.2021 18:07, João Távora wrote:
> On Wed, Jun 2, 2021 at 3:54 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>>
>>>> AFAIK it was written under the assumption that the
>>>> `icomplete-tidy` call would be automatically "undone" by the
>>>> `post-command-hook` before the next redisplay.
>>> Are you okay with the proposed change, then?
>>
>> I think it's worth a try.
>>
>>> And how do you feel about reducing icomplete-compute-delay to 100ms at most?
> 
> For me maybe also not much less than that. For the first few chars, I
> usuallly type fast enough, and I appreciate this CPU saving measure.
> 
> 150ms down from 300ms maybe a good compromise to cut down on the
> xx/xx transient inconsistency and still have the CPU saving thing.

OK, so I went ahead with this change.

And also took the liberty of changing icomplete-max-delay-chars to 2: in 
this day and age, we can certainly deal with longer lists than we could 
in 1998.

But we can undo either change, of course, upon negative feedback.

> Even better, as I suggested, is to make that proposed change be
> conditional on the delay.  Then people wanting no flicker just reduce
> the delay, which is quite intuitive.

I'm not sure who would want to change the delay to a higher value, and 
why (people are welcome to tell us).

But even if someone decides to go back to 300 ms, I suspect they would 
still benefit from the new behavior.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:
> On 02.06.2021 18:07, João Távora wrote:

> OK, so I went ahead with this change.
>
> And also took the liberty of changing icomplete-max-delay-chars to 2:
> in this day and age, we can certainly deal with longer lists than we
> could in 1998.
>
> But we can undo either change, of course, upon negative feedback.

Fair enough.

>> Even better, as I suggested, is to make that proposed change be
>> conditional on the delay.  Then people wanting no flicker just reduce
>> the delay, which is quite intuitive.
>
> I'm not sure who would want to change the delay to a higher value, and
> why (people are welcome to tell us).
>
> But even if someone decides to go back to 300 ms, I suspect they would
> still benefit from the new behavior.

Ditto.  I don't think I'll be changing that default, personally.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Gregory Heytings]

>>> I didn't get a terrible amount of feedback, but I spent the week 
>>> testing that branch, making small adjustments, and verifying that the 
>>> default Icomplete and Fido behaviour are unchanged.  So I've now 
>>> pushed the icomplete-vertical-mode rework to master.
>>
>> Okay, so I guess it's too late to send you feedback...  I asked you to 
>> wait a few days more, and promised to send you feedback this week.
>
> You can still send feedback, of course.  Just because it's in master 
> doesn't mean it's written in stone.
>

That's most often the case, but let's see whether this one is an 
exception.

>
> I did wait a few days more to your request but I didn't think it made 
> sense to wait many more as I need to move on to other developments that 
> kind of depend on it. Anyway, I think you'll like the new mode, but let 
> me know what you think.
>

I've now spent some time to test the feature and to look at your code, and 
here are a few comments:

1. Your code is not modular enough, for example, IMO it would be much 
better if the the annotation function and the "candidate number/total 
number of candidates" indication were (user-configurable) hooks, instead 
of hard-coding these operations in the code.  Not everyone wants to see 
these numbers, not everyone wants to see annotations even if they are 
available, some might want to add annotations to the candidates list with 
their own chosen formatting, and so forth.

2. I don't understand why you used "icomplete-scroll" for the variable 
name, and why it is not a defcustom.  IMO it should have been something 
more explicit like "icomplete-vertical-scroll-candidate-list".  And with a 
hook, this variable would not be necessary.

3. As I feared, the code to scroll the candidates list does not work 
correctly alas.  I've put lots of efforts to make "icomplete-vertical" 
work correctly in virtually every possible case, so I find this very 
regrettable.  For example, with frame-height = 47, 10 candidates are 
displayed, yet the candidates list rotates on the 7th candidate (instead 
of the 11th one).  With frame-height = 30, 6 candidates are displayed, and 
the candidates list rotates on the 6th candidate (instead of the 7th one). 
With frame-height = 22, 4 candidates are displayed, and the candidates 
list correctly rotates on the 5th candidate.  With frame-height = 15, two 
candidates are displayed, yet the candidates list rotates on the 4th 
candidate (instead of the 3th one, and in this case you don't even see the 
current candidate anymore).

If I start working on this (again), I would likely change most of your 
code into something else, and it would take some time, because it's a 
difficult problem.  Would you agree with this?

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 6/1/21 4:47 PM, Gregory Heytings wrote:
> 1. Your code is not modular enough, for example, IMO it would be much 
> better if the the annotation function and the "candidate number/total 
> number of candidates" indication were (user-configurable) hooks, instead 
> of hard-coding these operations in the code.  Not everyone wants to see 
> these numbers, not everyone wants to see annotations even if they are 
> available, some might want to add annotations to the candidates list with 
> their own chosen formatting, and so forth.

Regarding the annotations I disagree. The default *Completions* UI also
annotates by default with not possibility for the user to turn this off,
except by overwriting the `annotation-function` itself for example by
advising `completion-metadata-get`.

> 2. I don't understand why you used "icomplete-scroll" for the variable 
> name, and why it is not a defcustom.  IMO it should have been something 
> more explicit like "icomplete-vertical-scroll-candidate-list".  And with a 
> hook, this variable would not be necessary.

Agree, a `defcustom` is better.

> 3. As I feared, the code to scroll the candidates list does not work 
> correctly alas.  I've put lots of efforts to make "icomplete-vertical" 
> work correctly in virtually every possible case, so I find this very 
> regrettable.  For example, with frame-height = 47, 10 candidates are 
> displayed, yet the candidates list rotates on the 7th candidate (instead 
> of the 11th one).  With frame-height = 30, 6 candidates are displayed, and 
> the candidates list rotates on the 6th candidate (instead of the 7th one). 
> With frame-height = 22, 4 candidates are displayed, and the candidates 
> list correctly rotates on the 5th candidate.  With frame-height = 15, two 
> candidates are displayed, yet the candidates list rotates on the 4th 
> candidate (instead of the 3th one, and in this case you don't even see the 
> current candidate anymore).

Okay, I didn't see something out of the ordinary, but I've only given it
a bit of light testing earlier. Hopefully these issues can be fixed.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Gregory Heytings]

>
> Regarding the annotations I disagree. The default *Completions* UI also 
> annotates by default with not possibility for the user to turn this off, 
> except by overwriting the `annotation-function` itself for example by 
> advising `completion-metadata-get`.
>

And this is (for me, and likely for others) one of the differences between 
the *Completions* buffer and the minibuffer.  I'm not against making it 
possible to display annotations in the minibuffer for those who want to 
see them there, but hardcoding them is IMO wrong.  And as I said users 
might want to display these annotations with their own formatting, e.g. to 
display only some but not all of them in the minibuffer.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 6/1/21 4:58 PM, Gregory Heytings wrote:
>> Regarding the annotations I disagree. The default *Completions* UI also 
>> annotates by default with not possibility for the user to turn this off, 
>> except by overwriting the `annotation-function` itself for example by 
>> advising `completion-metadata-get`.
>
> And this is (for me, and likely for others) one of the differences between 
> the *Completions* buffer and the minibuffer.  I'm not against making it 
> possible to display annotations in the minibuffer for those who want to 
> see them there, but hardcoding them is IMO wrong.

I am not against offering a customization option in the UI to
enable/disable annotations. Note that there is also the
`completions-detailed` variable, which guards some uses of the
`affixation-function`. The `annotation-functions` seem to be unguarded.

The design which I would prefer would be to check the
`completions-detailed` variable once in the completion UI (Icomplete,
default *Completions*) and depending on the value show the annotations
or not.

> And as I said users might want to display these annotations with their
> own formatting, e.g. to display only some but not all of them in the
> minibuffer.

This is not a good idea in my opinion, at least for the
`annotation-functions`. Annotations are only strings, there is not much
formatting the user can do here. But more general use cases could be
supported by a `decoration-function` (replacement for the current
`affixation-function`) we discussed a few days ago. The
`decoration-function` would return a list of semantic
annotation/decoration fields given a candidate.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Gregory Heytings]

>> And as I said users might want to display these annotations with their 
>> own formatting, e.g. to display only some but not all of them in the 
>> minibuffer.
>
> This is not a good idea in my opinion, at least for the 
> `annotation-functions`. Annotations are only strings, there is not much 
> formatting the user can do here.
>

That's your opinion.  Some users might want to display only the prefix, or 
only the suffix, or the prefix and the suffix after the completion 
candidate, or display the suffix with only one space between the candidate 
and the suffix, or filter prefixes and suffixes (IOW display only some but 
not all of them), and so forth.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 6/1/21 5:33 PM, Gregory Heytings wrote:
> 
>>> And as I said users might want to display these annotations with their 
>>> own formatting, e.g. to display only some but not all of them in the 
>>> minibuffer.
>>
>> This is not a good idea in my opinion, at least for the 
>> `annotation-functions`. Annotations are only strings, there is not much 
>> formatting the user can do here.
>>
> 
> That's your opinion.  Some users might want to display only the prefix, or 
> only the suffix, or the prefix and the suffix after the completion 
> candidate, or display the suffix with only one space between the candidate 
> and the suffix, or filter prefixes and suffixes (IOW display only some but 
> not all of them), and so forth.

The annotation function only supports suffixes. Allowing the user to
decide to display or filter the annotations is not a reasonable use
case. I've written numerous commands relying on annotations/affixations
and so far I've not heard this request of yours. This means that there
is no wide demand for such an override feature and there is no need to
offer this configuration option. But you can of course implement this in
your own user configuration by overwriting the
`annotation/affixation-function`.

If you want to convince me, please present an example command where you
want to filter/modify the annotations as a user and which modification
you want to apply. If there is a reasonable use case, I am not against
offering tweaking capabilities. But if it is a very special fringe use
case or otherwise heavy customization, you can already use advices. You
may have seen our Marginalia package? This package does exactly that, it
overwrites the annotation function with a more advanced version, which
shows much more data and wastes more CPU cycles ;)

The question is really if we need something on the level of the user
configuration or if the existing Elisp facilities suffice. Since all
these overrides you have in mind are of course already possible.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Gregory Heytings <gregory@heytings.org> writes:

>>
>> Regarding the annotations I disagree. The default *Completions* UI
>> also annotates by default with not possibility for the user to turn
>> this off, except by overwriting the `annotation-function` itself for
>> example by advising `completion-metadata-get`.
>>
>
> And this is (for me, and likely for others) one of the differences
> between the *Completions* buffer and the minibuffer.  I'm not against
> making it possible to display annotations in the minibuffer for those
> who want to see them there, but hardcoding them is IMO wrong.  And as
> I said users might want to display these annotations with their own
> formatting, e.g. to display only some but not all of them in the
> minibuffer.

Nothing is hardcoded and icomplete-vertical-mode is a new feature.  So I
woundn't go crazy on customization for customization sake.  Daniel's
proposal was simple enough and it was a no brainer to take.  We're still
discussing the API to do annotations anyway.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 6/1/21 5:09 PM, João Távora wrote:
> Gregory Heytings <gregory@heytings.org> writes:
> 
>>>
>>> Regarding the annotations I disagree. The default *Completions* UI
>>> also annotates by default with not possibility for the user to turn
>>> this off, except by overwriting the `annotation-function` itself for
>>> example by advising `completion-metadata-get`.
>>>
>>
>> And this is (for me, and likely for others) one of the differences
>> between the *Completions* buffer and the minibuffer.  I'm not against
>> making it possible to display annotations in the minibuffer for those
>> who want to see them there, but hardcoding them is IMO wrong.  And as
>> I said users might want to display these annotations with their own
>> formatting, e.g. to display only some but not all of them in the
>> minibuffer.
> 
> Nothing is hardcoded and icomplete-vertical-mode is a new feature.  So I
> woundn't go crazy on customization for customization sake.  Daniel's
> proposal was simple enough and it was a no brainer to take.  We're still
> discussing the API to do annotations anyway.

I agree with not going overboard with customization possibilities. I am
in particular worried by the duplication of customization variables.
There is `completions-detailed` which is pretty much unused for example.
Adding another guard variable on top does not sound good. But I think a
global guard variable (per completion UI) to disable annotations in that
particular UI is something to consider.

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

[Just sent you this reply off-list, here it is on-list]

Gregory Heytings <gregory@heytings.org> writes:

>> Just because it's in master doesn't mean it's written in stone.
> That's most often the case, but let's see whether this one is an
> exception.

What, really?  Lol, then what are all these commits to master doing?
:-)

> 1. Your code is not modular enough, for example, IMO it would be much
> better if the the annotation function and the candidate number/total
> number of candidates were (user-configurable) hooks, instead of
> hard-coding these operations in the code.

Why do you think the code is not modular enough? Previously, it mixed
vertical and non-vertical mode with a bunch of ifs, now vertical mode is
handled in its own function.  Not ideal, but at least more modular to
me.

Maybe you're conflating the ability to configure a system with the
modularity of its implementation in code.

> Not everyone wants to see
> these numbers, not everyone wants to see annotations even if they are
> available, some might want to add annotations to the candidates list
> with their own chosen formatting, and so forth.

Who do you think doesn't want to see those numbers?  You personally?  If
so, add a patch (I think Daniel is working on this).  But keep the
numbers on by default in fido-mode + fido-vertical-mode.  It's a new
feature so we can what we think is the best default.

Overlay, to clarify it seems you regard icomplete-vertical-mode as a
long-supported feature of Emacs, it's not.  It's a new thing for Emacs
28.  Your implementation, which I reworked and merged some time ago, was
very simple (a good thing!) but also primitive in functionality (when
compared to other completers).  I was a good proof of concept but it
makes sense to rework it.

> 2. I don't understand why you used "icomplete-scroll" for the variable
> name, and why it is not a defcustom.  IMO it should have been
> something more explicit like
> "icomplete-vertical-scroll-candidate-list".  With a hook, this
> variable would not be necessary.

A hook is a variable, and considerably more complex than a boolean.  Why
is that better here and how would it solve the problem?

If you want to change the variable name, it's vertainly possible, but
really your proposal is mouthful of words, IMHO.

I don't make stuff customizable unless people request the customization,
i.e. I don't add customization knobs gratutiously.  But if you really
want to make it a defcustom, of course it can be added.

As for fido-mode, which is an opinionated completer, it overrides it (as
it already does a lot of other defcustoms).

> 3. As I feared, the code to scroll the candidates list does not work
> correctly alas.  I've put lots of efforts to make "icomplete-vertical"
> work correctly in virtually every possible case, so I find this very
> regrettable.  For example, with frame-height = 47, 10 candidates are
> displayed, yet the candidates list rotates on the 7th candidate
> (instead of the 11th one).

This is by design.  Works like Vertico and many other completers.  But
are you experimenting with:

  fido-mode + icomplete-vertical-mode

or

  icomplete-mode + icomplete-vertical-mode

?

The former uses scrolling by default, the latter doesn't.  It uses
rotation, and always "rotates" in the first character.  So I am confused
when you speak of rotations.

> With frame-height = 30, 6 candidates are
> displayed, and the candidates list rotates on the 6th candidate
> (instead of the 7th one).

Can't reproduce this, I see it scroll (not rotate) on the third visual
candidate.

> With frame-height = 22, 4 candidates are displayed, and the candidates
> list correctly rotates on the 5th candidate.  With frame-height = 15,
> two candidates are displayed, yet the candidates list rotates on the
> 4th candidate (instead of the 3th one, and in this case you don't even
> see the current candidate anymore).

I also can't reproduce these two cases, not with Emacs -Q.  Maybe you're
using some extra configuration?  What?

> If I start working on this (again), I would likely change most of your
> code into something else, and it would take some time, because it's a
> difficult problem.  Would you agree with this?

First say what you would want to change it to, and why.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Gregory Heytings]

>
> Why do you think the code is not modular enough? Previously, it mixed 
> vertical and non-vertical mode with a bunch of ifs, now vertical mode is 
> handled in its own function.  Not ideal, but at least more modular to 
> me.
>
> You seem to be conflating the ability to configure a system with the 
> modularity of its implementation in code.
>

I did not say it's "less modular", I said it is "not modular enough".

>
> Who do you think doesn't want to see those numbers?  You personally?
>

I do not want to see these numbers indeed, and again I'd be surprised if I 
were the only one.  IMO displaying that information wastes CPU cycles for 
no good reason.

>
> A hook is a variable, and considerably more complex than a boolean. 
> Why is that better here and how would it solve the problem?
>

It is better because other users can define other functions in their init 
files and use them instead of the default ones.

>> As I feared, the code to scroll the candidates list does not work 
>> correctly alas.  I've put lots of efforts to make "icomplete-vertical" 
>> work correctly in virtually every possible case, so I find this very 
>> regrettable.  For example, with frame-height = 47, 10 candidates are 
>> displayed, yet the candidates list rotates on the 7th candidate 
>> (instead of the 11th one).
>
> This is by design.  Works like Vertico and many other completers.
>

We can and should do better than other completers.

>
> Can't reproduce this, I see it scroll on the third visual candidate.
>
> [...]
>
> I also can't reproduce these two cases, not with Emacs -Q.  Maybe you're 
> using some extra configuration?  What?
>

No, I'm using emacs -Q (on Debian GNU/Linux with GTK in case it matters).

>> If I start working on this (again), I would likely change most of your 
>> code into something else, and it would take some time, because it's a 
>> difficult problem.  Would you agree with this?
>
> First say what you would want to change it to, and why.
>

I explained it above.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

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

Gregory Heytings <gregory@heytings.org> writes:

> I did not say it's "less modular", I said it is "not modular enough".

OK, so at least we agree it's an improvement.  And I also agree it's not
modular enough, but mostly in the sense that nothing is ever is.

>> Who do you think doesn't want to see those numbers?  You personally?
> I do not want to see these numbers indeed, and again I'd be surprised
> if I were the only one.  IMO displaying that information wastes CPU
> cycles for no good reason.

OK.  Then patches welcome I guess.  Do you have any indication of how
many CPU cycles to do that measuring in comparison to the number spent
without them?

>> A hook is a variable, and considerably more complex than a
>> boolean. Why is that better here and how would it solve the problem?
> It is better because other users can define other functions in their
> init files and use them instead of the default ones.

What other functions?  What is the protocol for this hook you are
proposing?

>>> frame-height = 47, 10 candidates are displayed, yet the candidates
>>> list rotates on the 7th candidate (instead of the 11th one).
>> This is by design.  Works like Vertico and many other completers.
> We can and should do better than other completers.

But what's wrong with this approach?  How is it bad and how could it be
better?

>> I also can't reproduce these two cases, not with Emacs -Q.  Maybe
>> you're using some extra configuration?  What?
> No, I'm using emacs -Q (on Debian GNU/Linux with GTK in case it
> matters).

I need a recipe so I can understasnd what you're doing and ensure
neither of us is seeing poltergeist.  Here's what I'm doing:

  emacs -Q -g 120x47 -f fido-mode -f fido-vertical-mode
  Type C-x C-f
  Scroll down a bit

I attach a gif.


[-- Attachment #2: Peek 2021-06-01 23-58.gif --]
[-- Type: image/gif, Size: 374499 bytes --]

[-- Attachment #3: Type: text/plain, Size: 569 bytes --]


>>> If I start working on this (again), I would likely change most of
>>> your code into something else, and it would take some time, because
>>> it's a difficult problem.  Would you agree with this?
>>
>> First say what you would want to change it to, and why.
> I explained it above.

Make icomplete-scroll a defcustom if you must.  Also fine with making
the x/y indicator a formattable thing.  Didn't understand what you want
to do with hooks.  Don't understand why this entails changing "most of
my code", but I'll wait for the patches.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 24.05.2021 13:26, Daniel Mendler wrote:
> Of course this should be
> extensible, the data backend should be allowed to define arbitrary such
> columns. And then the UI can render some of these semantic columns
> specially (e.g. as Company is doing with company-kind).

That works for me, though I would approach this from the other direction 
and define an xyz-function for each column.

> But I am not in
> favor of extending the `completing-read` API with these `company-*`
> extensions. If such a semantic annotation extension is added, it should
> be non-namespaced ideally since it is not tied to the Company backend.

Yes, sure. Don't make them prefixed and document those attributes 
properly. This is orthogonal to all the other decisions.

> As I argued above, I don't think such support should be added. I hope we
> can find a better semantic annotation solution which encompasses
> company-kind, prefixes, suffixes and arbitrary rich columnar data. The
> question is how this should be done such that in the end the design does
> not get too complicated.

So far your columnar proposals (IIUC) have been quite different from 
what company-kind offers. And it more and more looks like something 
Company couldn't support.

Which is fine by itself, I suppose, but it also moves c-a-p-f away from 
the idea of a frontend-agnostic API.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 24.05.2021 13:26, Daniel Mendler wrote:
> This is more of a technical detail, but I also want to
> stress the point made by Juri in the other mail, that the backend may
> have to setup some "context" to compute the annotation data lazily. If
> this setup can only be done once per redisplay of the currently visible
> candidates, that is beneficial.

IME when annotations are computed only for a limited subset of 
completions (as is the case now), it doesn't matter too much if they 
have to be computed twice, 'cause the time it takes is still negligible.

But of course a frontend can set up a context itself (some hash-table 
inside its internals) which tracks which completions have which 
annotations, so it doesn't have to ask twice.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

Independent of the orthogonal discussion about the pros and cons of the
`affixation-function` is the patch (maybe including your simplification)
acceptable to merge? If a specification change is later made, Icomplete
is quickly adapted.

Thanks!

Daniel

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 24.05.2021 00:04, João Távora wrote:
> But I don't understand why these decision should be done in the data
> backend.  The backedn should only annotate a completion with a possible
> _hint_  on how to layout ("prefix" and "suffix" are hints). It's the
> frontend, who is who knows what to display and where, who should be
> responsible for the layout.

I've made some similar arguments in a recent thread:

https://lists.gnu.org/archive/html/emacs-devel/2021-04/msg01320.html

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

Dmitry Gutov [2021-05-24 01:42:26] wrote:
> On 24.05.2021 00:04, João Távora wrote:
>> But I don't understand why these decision should be done in the data
>> backend.  The backedn should only annotate a completion with a possible
>> _hint_  on how to layout ("prefix" and "suffix" are hints). It's the
>> frontend, who is who knows what to display and where, who should be
>> responsible for the layout.
>
> I've made some similar arguments in a recent thread:
>
> https://lists.gnu.org/archive/html/emacs-devel/2021-04/msg01320.html

+1


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

>> But I don't understand why these decision should be done in the data
>> backend.  The backedn should only annotate a completion with a possible
>> _hint_  on how to layout ("prefix" and "suffix" are hints). It's the
>> frontend, who is who knows what to display and where, who should be
>> responsible for the layout.
>
> I've made some similar arguments in a recent thread:
>
> https://lists.gnu.org/archive/html/emacs-devel/2021-04/msg01320.html

And later your reply was that annotation-function is the old approach, and
that the default completion frontend still doesn't use :company-kind.
Any plans to add support for :company-kind to the default frontend?

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 24.05.2021 02:42, Juri Linkov wrote:
> And later your reply was that annotation-function is the old approach, and
> that the default completion frontend still doesn't use :company-kind.
> Any plans to add support for :company-kind to the default frontend?

I can try and write an experimental patch if people are interested.

But it would be better if it was then brought over the "finish line" by 
someone who regularly uses 'M-x completion-at-point'. Because this 
feature, applied to the UI in a general fashion, brings usability 
questions and I'm sure will trigger some old-timer concerns.

Consider Elisp completion. Currently we only show <f> annotation when 
the context is such that it is not certain that the completion is a 
function (e.g. the completions set is symbols which can be anything). 
But when all completions are functions, we don't show <f>.

The current company-kind implementation is informed by the expectations 
of IDE users, where the kind is always shown. We also split functions 
into special forms and "regular" functions, annotating the former as 
"keywords", to make it not entirely useless even in this context.

I do believe this is beneficial, but I'm also sure that not every user 
is going to like seeing <f> or <k> in the first column for all 
completions, always. Not at first, at least.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

> I also don't understand why :affixation-function is given a full list of
> completions, when it is presumably meant to return a list of exactly the
> same length.

This has the advantage in functions like read-extended-command--affixation,
where minibuffer-selected-window is selected only once with with-selected-window,
then all completions are processed in the same buffer, without the need
to switch buffers for every completion in a long list, as annotation-function
would require to do.

> Seems like a potential hazard to allow this function to do filtering.

OTOH, not allowing the function to do filtering is too much hand-holding
for users who want the freedom to transform the completions to their likings.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

>> I also don't understand why :affixation-function is given a full list of
>> completions, when it is presumably meant to return a list of exactly the
>> same length.
>
> This has the advantage in functions like read-extended-command--affixation,
> where minibuffer-selected-window is selected only once with with-selected-window,
> then all completions are processed in the same buffer, without the need
> to switch buffers for every completion in a long list, as annotation-function
> would require to do.

Why does the function use `with-selected-window`?  I can't see why it
would need to mess with windows.  As for switching buffer, the only
reason I can see to switch buffer is for `where-is-internal`, which
could be fixed by pre-computing the set of keymaps (see patch below)
which also makes sure we use the right buffer whereas
`minibuffer-selected-window` could theoretically return another active
minibuffer than "ours".


        Stefan


diff --git a/lisp/simple.el b/lisp/simple.el
index 2a90a076315..f0eff99a358 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2003,9 +2003,14 @@ read-extended-command
 	         "M-x "))
        (lambda (string pred action)
          (if (and suggest-key-bindings (eq action 'metadata))
-	     '(metadata
-	       (affixation-function . read-extended-command--affixation)
-	       (category . command))
+	     ;; FIXME: Actually, we know that `overriding-local-map' is nil at
+	     ;; this point.
+	     (let ((keymaps (current-active-maps overriding-local-map)))
+	       `(metadata
+	         (affixation-function
+	          . ,(lambda (arg)
+	               (read-extended-command--affixation keymaps arg)))
+	         (category . command)))
            (let ((pred
                   (if (memq action '(nil t))
                       ;; Exclude from completions obsolete commands
@@ -2093,25 +2098,24 @@ command-completion-button-p
     (and (get-text-property (point) 'button)
          (eq (get-text-property (point) 'category) category))))
 
-(defun read-extended-command--affixation (command-names)
-  (with-selected-window (or (minibuffer-selected-window) (selected-window))
-    (mapcar
-     (lambda (command-name)
-       (let* ((fun (and (stringp command-name) (intern-soft command-name)))
-              (binding (where-is-internal fun overriding-local-map t))
-              (obsolete (get fun 'byte-obsolete-info))
-              (alias (symbol-function fun))
-              (suffix (cond ((symbolp alias)
-                             (format " (%s)" alias))
-                            (obsolete
-                             (format " (%s)" (car obsolete)))
-                            ((and binding (not (stringp binding)))
-                             (format " (%s)" (key-description binding)))
-                            (t ""))))
-         (put-text-property 0 (length suffix)
-                            'face 'completions-annotations suffix)
-         (list command-name "" suffix)))
-     command-names)))
+(defun read-extended-command--affixation (keymaps command-names)
+  (mapcar
+   (lambda (command-name)
+     (let* ((fun (and (stringp command-name) (intern-soft command-name)))
+            (binding (where-is-internal fun keymaps t))
+            (obsolete (get fun 'byte-obsolete-info))
+            (alias (symbol-function fun))
+            (suffix (cond ((symbolp alias)
+                           (format " (%s)" alias))
+                          (obsolete
+                           (format " (%s)" (car obsolete)))
+                          ((and binding (not (stringp binding)))
+                           (format " (%s)" (key-description binding)))
+                          (t ""))))
+       (put-text-property 0 (length suffix)
+                          'face 'completions-annotations suffix)
+       (list command-name "" suffix)))
+   command-names))
 
 (defcustom suggest-key-bindings t
   "Non-nil means show the equivalent key-binding when M-x command has one.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Daniel Mendler]

On 5/24/21 5:23 AM, Stefan Monnier wrote:
>>> I also don't understand why :affixation-function is given a full list of
>>> completions, when it is presumably meant to return a list of exactly the
>>> same length.
>>
>> This has the advantage in functions like read-extended-command--affixation,
>> where minibuffer-selected-window is selected only once with with-selected-window,
>> then all completions are processed in the same buffer, without the need
>> to switch buffers for every completion in a long list, as annotation-function
>> would require to do.
> 
> Why does the function use `with-selected-window`?  I can't see why it
> would need to mess with windows.  As for switching buffer, the only
> reason I can see to switch buffer is for `where-is-internal`, which
> could be fixed by pre-computing the set of keymaps (see patch below)
> which also makes sure we use the right buffer whereas
> `minibuffer-selected-window` could theoretically return another active
> minibuffer than "ours".

That is of course correct that it is better to add some internal
pre-computation. In the case of Marginalia we also have an extra caching
for the keybindings, since the `where-is-internal` function is
relatively slow. This matters when computing the keybinding annotations
on many candidates.

The context in which the `annotation/affixation-function` is computed is
not entirely clear across the set of available completion UIs, but it is
mostly the minibuffer. The solutions used in the various
annotation/affixation functions which are around are not a clean
technical solution but rather some best effort to show something useful
and hopefully correct in many scenarios. The most extensive use case are
probably our Marginalia package and the comparable ivy-rich package
(Note that ivy-rich does not make use of Ivy-specific infrastructure).

Daniel

Caching where-is-internal (was: (icomplete-vertical-mode): Add support for affixations and, annotations)

[Stefan Monnier]

> That is of course correct that it is better to add some internal
> pre-computation. In the case of Marginalia we also have an extra caching
> for the keybindings, since the `where-is-internal` function is
> relatively slow. This matters when computing the keybinding annotations
> on many candidates.

`where-is-internal` is supposed to do its own caching already (I added
that around the time of Emacs-21 because it was making construction of
the menubar too slow).  So if you see problems on that side, please
report them.


        Stefan

Re: Caching where-is-internal (was: (icomplete-vertical-mode): Add support for affixations and, annotations)

[Daniel Mendler]

On 5/24/21 6:22 PM, Stefan Monnier wrote:
> `where-is-internal` is supposed to do its own caching already (I added
> that around the time of Emacs-21 because it was making construction of
> the menubar too slow).  So if you see problems on that side, please
> report them.

I think the problem was that `where-is-internal` can generate a lot of
garbage memory. In Marginalia we have this code:

~~~
(defun marginalia-annotate-binding (cand)
  "Annotate command CAND with keybinding."
  ;; Precomputing the keybinding of every command is faster than
  ;; looking it up every time using `where-is-internal'.
  ;; `where-is-internal' generates a lot of garbage, leading to
  ;; garbage collecting pauses when interacting with the minibuffer.
  ;; See https://github.com/minad/marginalia/issues/16.
  (unless marginalia--annotate-binding-hash
    (setq marginalia--annotate-binding-hash
          (make-hash-table :size 1025))
    (mapatoms
     (lambda (sym)
       (when-let (key (and (commandp sym)
                           (where-is-internal sym nil t)))
         (puthash sym key marginalia--annotate-binding-hash)))))
  (when-let* ((sym (intern-soft cand))
              (binding (gethash sym marginalia--annotate-binding-hash)))
    (propertize (format " (%s)" (key-description binding))
                'face 'marginalia-key)))
~~~

But I cannot tell you much about the exact reasons. It just turned out
that calling `where-is-internal` once on all symbols was faster than
calling it occasionally.

In Vertico/Selectrum etc, the `marginalia-annotate-binding` function is
called for each candidate and the difference was noticeable when
scrolling through the candidate list.

The funny thing here is that the initial (costly?) computation of the
`marginalia--annotate-binding-hash` does not lead to a noticeable slowdown.

Daniel

Re: Caching where-is-internal

[Stefan Monnier]

> I think the problem was that `where-is-internal` can generate a lot of
> garbage memory.  In Marginalia we have this code:

It would be great if you could track it down further.
Maybe it's simply that the cache ends up flushed between invocations
(in your use case), defeating its purpose.

>     (mapatoms
>      (lambda (sym)
>        (when-let (key (and (commandp sym)
>                            (where-is-internal sym nil t)))
>          (puthash sym key marginalia--annotate-binding-hash)))))

`where-is-internal` has almost exactly this same hash-table internally, so
this basically copies the hash-table (in a somewhat inefficient way).
:-(

> But I cannot tell you much about the exact reasons. It just turned out
> that calling `where-is-internal` once on all symbols was faster than
> calling it occasionally.

I suspect a "spurious cache flush".  Maybe we should export the code
that turns a set of keymaps into a (reversed) hash-table.

> The funny thing here is that the initial (costly?) computation of the
> `marginalia--annotate-binding-hash` does not lead to a noticeable slowdown.

The first call to `where-is-internal` will traverse all the keymaps and
build the hash-table (which doesn't cost much more than just a plain
uncached `where-is-internal`), and the subsequent ones will hit the
hash-table instead.


        Stefan

Re: Caching where-is-internal

[Daniel Mendler]

On 5/24/21 9:53 PM, Stefan Monnier wrote:
>> The funny thing here is that the initial (costly?) computation of the
>> `marginalia--annotate-binding-hash` does not lead to a noticeable slowdown.
> 
> The first call to `where-is-internal` will traverse all the keymaps and
> build the hash-table (which doesn't cost much more than just a plain
> uncached `where-is-internal`), and the subsequent ones will hit the
> hash-table instead.

This makes sense. Thanks. Then we have to figure out why this spurious
cache flush happens. At the time when I wrote this code I should have
reported the issue right away or investigated a bit more deeply. It is
good to revisit this now in the light of the current discussion.

Daniel

Re: Caching where-is-internal

[Stefan Monnier]

> This makes sense. Thanks. Then we have to figure out why this spurious
> cache flush happens.

The cache checks that the set of active keymaps is the same as
last time `where-is-internal` was called.
So if the set of active keymaps is different, the cache will be flushed.

Otherwise, the cache is also flushed by `set-keymap-parent` and
`define-key` (search for `where_is_cache` in src/keymap.c for details).


        Stefan

Re: Caching where-is-internal

[Daniel Mendler]

On 5/24/21 10:33 PM, Stefan Monnier wrote:
>> This makes sense. Thanks. Then we have to figure out why this spurious
>> cache flush happens.
> 
> The cache checks that the set of active keymaps is the same as
> last time `where-is-internal` was called.
> So if the set of active keymaps is different, the cache will be flushed.
> 
> Otherwise, the cache is also flushed by `set-keymap-parent` and
> `define-key` (search for `where_is_cache` in src/keymap.c for details).

Thanks. But it seems I cannot reproduce the problem anymore in
Selectrum. Maybe this was due to some temporary buffers used by
Selectrum back then, which have been removed for good now. Maybe this
has fixed this caching issue. My Vertico UI also does not have the
issue. I removed the binding cache from Marginalia.

Daniel

Re: Caching where-is-internal

[Stefan Monnier]

> Thanks. But it seems I cannot reproduce the problem anymore in
> Selectrum. Maybe this was due to some temporary buffers used by
> Selectrum back then, which have been removed for good now. Maybe this
> has fixed this caching issue. My Vertico UI also does not have the
> issue. I removed the binding cache from Marginalia.

Even better.  I'll be expecting your call for when it rears its ugly
head again, tho,


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

> Why does the function use `with-selected-window`?  I can't see why it
> would need to mess with windows.  As for switching buffer, the only
> reason I can see to switch buffer is for `where-is-internal`, which
> could be fixed by pre-computing the set of keymaps (see patch below)
> which also makes sure we use the right buffer whereas
> `minibuffer-selected-window` could theoretically return another active
> minibuffer than "ours".
>[...]
> +	     (let ((keymaps (current-active-maps overriding-local-map)))
> +	       `(metadata
> +	         (affixation-function
> +	          . ,(lambda (arg)
> +	               (read-extended-command--affixation keymaps arg)))

I tried it out, but it doesn't work, because the above code is executed
in the minibuffer instead of the original buffer.

It doesn't need to select another window indeed,
but still needs to switch buffers, maybe current-active-maps
in the above wrapped with something like

  (with-current-buffer (window-buffer (minibuffer-selected-window))
    (current-active-maps ...))

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Juri Linkov <juri@linkov.net> writes:

>> Why does the function use `with-selected-window`?  I can't see why it
>> would need to mess with windows.  As for switching buffer, the only
>> reason I can see to switch buffer is for `where-is-internal`, which
>> could be fixed by pre-computing the set of keymaps (see patch below)
>> which also makes sure we use the right buffer whereas
>> `minibuffer-selected-window` could theoretically return another active
>> minibuffer than "ours".
>>[...]
>> +	     (let ((keymaps (current-active-maps overriding-local-map)))
>> +	       `(metadata
>> +	         (affixation-function
>> +	          . ,(lambda (arg)
>> +	               (read-extended-command--affixation keymaps arg)))
>
> I tried it out, but it doesn't work, because the above code is executed
> in the minibuffer instead of the original buffer.
>
> It doesn't need to select another window indeed,
> but still needs to switch buffers,

And why not do this switch in minibuffer.el, in
minibuffer-completion-help?  If the minibuffer to switch to is indeed
the the one in the selected minibuffer window, then it makes sense to
me.  This would, in principle, remove the argument for having the
affixation-function.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

> I tried it out, but it doesn't work, because the above code is executed
> in the minibuffer instead of the original buffer.
>
> It doesn't need to select another window indeed,
> but still needs to switch buffers, maybe current-active-maps
> in the above wrapped with something like
>
>   (with-current-buffer (window-buffer (minibuffer-selected-window))
>     (current-active-maps ...))

Indeed, thanks for catching that.


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Juri Linkov <juri@linkov.net> writes:

>> I also don't understand why :affixation-function is given a full list of
>> completions, when it is presumably meant to return a list of exactly the
>> same length.
>
> This has the advantage in functions like read-extended-command--affixation,
> where minibuffer-selected-window is selected only once with with-selected-window,
> then all completions are processed in the same buffer, without the need
> to switch buffers for every completion in a long list, as annotation-function
> would require to do. 

I've seen this in read-extended-command--affixation.el indeed. I don't
understand:

- the need for this window switch, but I might be missing something;
- if the need is to process in the same buffer, why one switches
  windows;
- why it can't and shouldn't be done in the funcall locus by the frontend
  (in this case minibuffer-completion-help)

>> Seems like a potential hazard to allow this function to do filtering.
>
> OTOH, not allowing the function to do filtering is too much hand-holding
> for users who want the freedom to transform the completions to their
> likings.

But if they exercise this freedom fully they're going to break the
layout of the frontend, like icomplete.el, which potentially does layout
calculations.  Probably company.el or any other frontend is the same.
So not so much holding hands, more like saving feet from being shot :-).

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Juri Linkov]

> I've seen this in read-extended-command--affixation.el indeed. I don't
> understand:
>
> - the need for this window switch, but I might be missing something;
> - if the need is to process in the same buffer, why one switches
>   windows;

It can switch buffers instead of selecting windows.

> - why it can't and shouldn't be done in the funcall locus by the frontend
>   (in this case minibuffer-completion-help)

Because this requirement is specific only to read-extended-command--affixation,
other uses might require other buffers to be current, so this doesn't remove
the argument for having the affixation-function.

> But if they exercise this freedom fully they're going to break the
> layout of the frontend, like icomplete.el, which potentially does layout
> calculations.  Probably company.el or any other frontend is the same.
> So not so much holding hands, more like saving feet from being shot :-).

For many years we allowed the users to shoot themselves in the foot,
and yet nobody did it :-)

With the existing function 'display-sort-function' you can easily
add new elements to the list of completions or remove the existing
candidates.  And that is not a problem in practice.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

> With the existing function 'display-sort-function' you can easily
> add new elements to the list of completions or remove the existing
> candidates.  And that is not a problem in practice.

Indeed.  I think we should aim for an API that's easy to use correctly.
If it makes incorrect uses difficult, that's good, but that shouldn't be
the driving goal.

If you want to make incorrect uses hard or even impossible, there are
dependently typed programming languages for that ;-)


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> With the existing function 'display-sort-function' you can easily
>> add new elements to the list of completions or remove the existing
>> candidates.  And that is not a problem in practice.
> Indeed.  I think we should aim for an API that's easy to use correctly.
> If it makes incorrect uses difficult, that's good, but that shouldn't be
> the driving goal.

I see, broken windows.  You keep asking for good API's but when things
are proposed in that direction "that's good" but doesn't matter.

I'm not exactly a fan of d-s-f, but likelyhood that it messes up is less
since it's likely called unconditionally on all returned completions by
the frontend.  Indeed I wonder why d-s-f exists at all.

As to the current shape affixation-function, I'm not mortally against it
just that its misdesign is a bit jarring, but so are so many other parts
of the completion system, so I guess, broken windows.  It's just that
this is supposed to be a new part.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Stefan Monnier]

> I see, broken windows.  You keep asking for good API's but when things
> are proposed in that direction "that's good" but doesn't matter.

[ Remember that the whole design of Emacs and ELisp goes against the
  usual software engineering practice of using encapsulation and
  abstraction to modularize the code so that one part can't mess with
  the other.

  Instead, Emacs's idea of empowering users means that for one part to
  be able to mess with another is considered a feature rather than
  a bug, and the "abstraction boundaries" are enforced by conventions
  rather than by technical means.

  As a dependent-types kinda guy, I sometimes find it odd as well, but
  I think it has served Emacs surprisingly well, so I tend to try and
  keep following the same design, despite my natural leaning
  against it.  ]

> As to the current shape affixation-function, I'm not mortally against it
> just that its misdesign is a bit jarring, but so are so many other parts
> of the completion system, so I guess, broken windows.  It's just that
> this is supposed to be a new part.

The part of `affixation-function` which I find problematic is the fact
that it does the layout itself, whereas I think it would be better to do
it in the UI (so it can choose to use columns or not, to put it at the
beginning or at the end, etc...).

The fact that it takes a whole list rather than a single element is more
in the "bikeshedcolor" category for me.


        Stefan

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Dmitry Gutov]

On 25.05.2021 23:01, Stefan Monnier wrote:
> The fact that it takes a whole list rather than a single element is more
> in the "bikeshedcolor" category for me.

 From where I'm standing, this color of the bike shed would make it 
harder to use (if I ever found a way to support it in Company, that is).

If affixation-function returns a list including extra data, I will then 
have to convert it to some structure that allows lookup by completion 
string (e.g. a hash table), and then pass that table on to the place the 
completions are rendered.

display-sort-function is different, because you can use it in one place, 
have your list re-sorted, and then go on with the rendering of the 
resulting collection. It doesn't force me to do any extra accounting.

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I see, broken windows.  You keep asking for good API's but when things
>> are proposed in that direction "that's good" but doesn't matter.
>
> [ Remember that the whole design of Emacs and ELisp goes against the
>   usual software engineering practice of using encapsulation and
>   abstraction to modularize the code so that one part can't mess with
>   the other.

No it doesn't :-) At least I don't design Lisp (Elisp or CL) systems
that way (or Ruby or JS, for more examples).  Encapsulation or
abstraction aren't property of a specific language.  Elisp can't
strictly enforce many things the way a C++ compiler refuses to compile,
but that doesn't mean there aren't a bunch of compile-time and run-time
assertions, much less that they are are useless.  Among many examples,
just because Emacs Lisp uses a shared symbol namespace (sigh...) hasn't
stopped us from making the '--' convention or, say, carefully deciding
if a variable name should end in "-functions" or "-function".  This is
nor bikeshedding for me, it's a daily part of design.

>   Instead, Emacs's idea of empowering users means that for one part to
>   be able to mess with another is considered a feature rather than
>   a bug, and the "abstraction boundaries" are enforced by conventions
>   rather than by technical means.

By both.  There's always a way to subvert API intent by the programmer.
That's fine, but it doesn't mean it should be easy to.  Pretty sure one
can frobnicate the order of the candidates in a single-arg annotation
function, but it's going to be a lot more code.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Manuel Uberti]

Hi,

I gave icomplete-vertical-mode a try, setting icomplete-scroll to t because I am 
not really comfortable with rotating candidates after the great experience with 
Helm, Ivy, Selectrum, and Vertico.

I used icomplete-vertical-mode with Consult, and tested it with commands such as 
consult-ripgrep and project-find-file, which are two of the most used commands 
for me when I work.

Great experience so far. I too would prefer to disable the counters before the 
prompt (IIRC, it was me asking Daniel to make it optional on Vertico), but it's 
a minor thing. Great job!


All the best

-- 
Manuel Uberti
www.manueluberti.eu

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

On Wed, Jun 2, 2021 at 9:25 AM Manuel Uberti
<manuel.uberti@inventati.org> wrote:

> Great experience so far. I too would prefer to disable the counters before the
> prompt (IIRC, it was me asking Daniel to make it optional on Vertico), but it's
> a minor thing. Great job!

By the way, are you using fido-mode or icomplete-mode.

If using M-x fido-mode and M-x fido-vertical-mode you should get an
out-of-the-box similar experience to Vertico and Ido without other
need of configuration.    You may not like it, of course, just wondering
if you had tried it.

João

Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[Manuel Uberti]

On 02/06/21 13:07, João Távora wrote:
> By the way, are you using fido-mode or icomplete-mode.
> 
> If using M-x fido-mode and M-x fido-vertical-mode you should get an
> out-of-the-box similar experience to Vertico and Ido without other
> need of configuration.    You may not like it, of course, just wondering
> if you had tried it.

This is how I have set it up in my init.el:

(add-hook 'after-init-hook #'icomplete-mode)
(add-hook 'icomplete-mode-hook #'icomplete-vertical-mode)

(setq icomplete-compute-delay 0.2
       icomplete-delay-completions-threshold 100
       icomplete-hide-common-prefix nil
       icomplete-max-delay-chars 2
       icomplete-scroll t
       icomplete-show-matches-on-no-input t
       icomplete-tidy-shadowed-file-names t)

(setq read-buffer-completion-ignore-case t
       read-file-name-completion-ignore-case t)

(setq completion-styles '(basic partial-completion orderless))

I can give fido-vertical-mode a try as well, although I see I am already using 
some of its settings.

-- 
Manuel Uberti
www.manueluberti.eu