bug#47711: bug#48841: bug#47711: bug#48841: bug#47711: [PATCH VERSION 2] Add new `completion-filter-completions` API and deferred highlighting

[João Távora]

On Thu, Nov 2, 2023 at 10:58 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > After some rounds of benchmarking and discussion, Dmitry and I
> > think the latest version of the patch should be installed.
>
> Are there any problematic aspects of the patch that need to be
> discussed or considered before installing the patch?
>
> IOW, why are you soliciting our opinions, instead of just going ahead
> and installing?

No particular reason.  Dmitry suggested that we do, and you
participated in this discussion a while back, and I know you
normally have some useful comment or two.

> > In a nutshell it solves the performance problem of overly eager
> > completion highlighting with minimal changes to the completion API.
>
> It looks to me like it adds a new feature, not just solves a
> performance problem?
>
> Some minor comments to the patch itself:
>
> > +(defvar-local completion-lazy-hilit nil
> > +  "If non-nil, request completion lazy highlighting.
> > +
> > +Completion-presenting frontends may opt to bind this variable to
> > +non-nil value in the context of completion-producing calls (such
> > +as `completion-all-completions').  This hints the intervening
> > +completion styles that they do not need to
> > +fontify (i.e. propertize with the `face' property) completion
> > +strings with highlights of the matching parts.
>
> If this is intended to be bound by frontends, why is it defvar-local?
> I thought let-binding buffer-local variables is a tricky business that
> could have unexpected results?

Good catch!  It shouldn't be defvar-local indeed, not with this latest
version.  See, glad I called you ;-)

> Also, I think this doc string should reference
> completion-lazy-hilit-fn.
>
> > +(defvar completion-lazy-hilit-fn nil
> > +  "Used by completions styles honoring `completion-lazy-hilit'.
>
> This should mention "function", since just "Used to..." doesn't convey
> that, and "-fn" could also mean "file name", not just "function".

Makes sense.

> > +(defun completion-lazy-hilit (str)
> > +  "Return a copy of completion STR that is `face'-propertized.
>                                               ^^^^^^^^^^^^^^^^^^
> Strange quoting.  "face" is not a symbol that we want to have a link
> to, is it?

It's not a symbol we'll be referencing, but it's a symbol.  I'll
rewrite it.

>
> I see a few more places in the doc strings that will "need work", but
> that can be done later.
>
> What did you want to say in NEWS about this?  If it's just a
> performance improvement, we don't normally mention them in NEWS.

But it needs frontends to opt in, and that requires an non-breaking
addition to completion API.

João