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