Re: [PATCH] `completing-read`: Add `group-function` support to completion metadata

[Daniel Mendler]

On 4/25/21 11:50 PM, Dmitry Gutov wrote:
> Sorry, I was basically referring to an earlier discussions where the 
> consensus was that xref-show-definitions-completiong-read doesn't play 
> very well with the default completing-read. Its completion table is odd, 
> one could say. The proposed feature simply doesn't change that.

I understand. But in which way do you think the function 
`xref-show-definitions-completiong-read` is odd? If you use completion 
styles like `flex` or `orderless` you can quickly narrow down the number 
of candidates and select. It works well for me and the grouping makes 
the view more clear.

> Perhaps if all currently planned uses of group-function are similarly 
> "odd" (and no additional uses in the core are going to be added in the 
> foreseeable future), you don't need to worry/care about having 
> :group-function added to the core, or at least not yet. Or about 
> updating the *Completions* UI.

I assume there are more commands in Emacs where grouping functionality 
is useful. Grouping is heavily used in Helm and in my Consult package, 
so having such functionality officially present in Emacs is certainly 
valuable.

> And keep it like "unofficial extension", which I'll be happy to support 
> in Xref anyway (and Xref is in ELPA Core, so users will always be able 
> to install the latest version). There are benefits to being such 
> extension: once you're a proper part of the protocol, you become much 
> more set in stone.

Yes, this would be the most minimal change - only define 
`group-function` as an official metadata which can then be used by 
commands and UIs which support it. However it would certainly be more 
encouraging to make use of the functionality if thereis support in the 
default completion UI or Icomplete.

> > > Speaking of group-function's implementation there, the 
> > > text-properties approach seems like an overkill since we can reliably 
> > > string-match anyway. But it's a minor thing.
> >
> > I've chosen the text property approach such that the group title 
> > retrieval does not lead to allocations (transform=nil). The 
> > transform=nil call is performance critical for continuously updating 
> > UIs like Icomplete, Vertico etc., since the candidates are grouped 
> > after sorting.
>
> But when the list is updated, the elements are basically recreated from 
> the external process's output every time, right? So this only helps if 
> you want to cache the result for repeated invocations of group-function 
> on the same result set.

I am not entirely sure I understand you correctly here. The candidate 
set is generated once from the external process. Then the properties are 
attached once per candidate. In the subsequent filtering/completing of 
the candidates, the candidate set and the attached properties are *not* 
regenerated. This means we save a lot of work here. In particular with 
continuously updating UIs we avoid regenerating the properties every key 
press.

> Also, xref-find-definitions usually deals with a limited number of 
> search results. But I guess some of your users set 
> xref-show-xrefs-function to xref-show-definitions-completiong-read too.

Yes, this the use case I proposed. Then you can have many more candidates.

Daniel