Re: [PATCH] Completion preview common

[Eshel Yaron <me@eshelyaron.com>]

Ergus <spacibba@aol.com> writes:

> On Wed, Apr 10, 2024 at 01:55:36PM +0200, Eshel Yaron wrote:
>>
>>I understand the value of inserting the common prefix as you describe
>>it.  This is what the command completion-preview-insert-common-prefix
>>that I proposed up-thread does.  It lets you insert (complete up to) the
>>longest common prefix even if the preview shows (also) the first suffix.
>>
> Indeed. The problem is that in my patch the user knows what's gonna be
> inserted. While otherwise we need to as some highlight to the candidate
> to let the user more or less know.
>
> To allow this second part the change implies modifications in all the
> functions chain to somehow export the prefix when shown + 1 extra
> command to insert only the common prefix + a user side rebind of
> tab. That change seems much more complicated (compared to my patch)

Right.  I think we're on the same page.  There's a tradeoff between
keeping the library as simple as possible on the one hand, and providing
the best UX on the other.

I've implemented my suggested approach, including highlighting the
longest common prefix.  This indeed required more changes than the patch
you proposed, but nothing too complicated so far.  I plan to experiment
with this some more and settle on an implementation (perhaps closer to
your patch, if I run into unexpected issues with this approach) in a
couple of weeks.

>>> If you allow me a couple of code comments:
>>>
>>> 1. Considering how the package works maybe it is useful to let-set:
>>>
>>> (completion-styles '(basic)) before calling completion-all-completions.
>>>
>>> Because you only care and handle the candidates where the matches are in
>>> the beginning, but with substring, partial-completion, etc the matches
>>> could be anywhere, giving a longer list (with many useless)
>>> candidates.
>>
>>I'll try it out and see if it makes for a significant performance
>>improvement.  Otherwise, I think it's better not to override
>>completion-styles in case someone has some exotic completion style with
>>specific behavior that they rely on.  IOW, I'd rather avoid making any
>>assumptions about completion-styles.
>>
> The assumption is actually good considering that your package handles
> candidates like (prefix)suffix inserting suffix; but it shouldn't handle
> correctly pre(prefix)suffix where `pre` is not inserted (or even
> previewed)

Yes, in most cases the basic style is exactly what we need here.  In my
testing, let-binding completion-styles as you suggested indeed led to a
significant performance (speed) improvement in comparison with using
e.g. the 'substring' completion style.  I'll push such a change soon.

> If the user has some `flex` like style set by default it will just add
> even more noise; when what you care is just the prefix right?

Specifically the flex style also influences sorting, so it's overriding
it is not totally innocent.  But in practice I think it should be fine.


Thanks,

Eshel