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

["João Távora" <joaotavora@gmail.com>]

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