GNU bug report logs - #47711
27.1; Deferred highlighting support in `completion-all-completions', `vertico--all-completions`

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: João Távora <joaotavora <at> gmail.com>
Cc: dmitry <at> gutov.dev, 47711 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca, mail <at> daniel-mendler.de
Subject: bug#47711: bug#48841: bug#47711: bug#48841: bug#47711: [PATCH VERSION 2] Add new `completion-filter-completions` API and deferred highlighting
Date: Thu, 02 Nov 2023 12:58:27 +0200

> From: João Távora <joaotavora <at> gmail.com>
> Date: Thu, 2 Nov 2023 10:39:53 +0000
> Cc: dmitry <at> gutov.dev, mail <at> daniel-mendler.de, monnier <at> iro.umontreal.ca, 
> 	47711 <at> debbugs.gnu.org
> 
> > > Stefan, Eli, would you like to chime in?
> >
> > Chime in on what aspect(s) of this discussion (or the patch)?
> 
> The patch condenses the results of the discussion of last week,
> as resuscitated by Dmitry after a 2-year long hiatus.
> 
> 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?

> 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?

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".

> +(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?

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.

Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.