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.