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