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

[Daniel Mendler <mail@daniel-mendler.de>]

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