Re: [PATCH] Completion preview common

[Eshel Yaron <me@eshelyaron.com>]

Hi Ergus,

Thank you for these comments.

Ergus <spacibba@aol.com> writes:

> Hi:
>
> On Tue, Apr 09, 2024 at 08:30:16PM +0200, Eshel Yaron wrote:
>>
>>The patch you proposed adds an option to show only the longest common
>>prefix instead of a whole candidate, but the whole candidate includes
>>the longest common prefix, and so ISTM that it provides strictly more
>>information.  That's why an option that makes Completion Preview mode
>>only show the longest common prefix strikes me as less useful than
>>letting you complete only up to that common prefix on demand, while
>>still previewing a full candidate.  If that doesn't fit your bill,
>>perhaps you can describe the use case with some more detail?
>>
>
> I don't think common preview it is less useful. Indeed what I find
> totally useless is the insertion of first candidate instead of the
> common prefix. Why the first? How far we need to rotate until we find
> the right one? isn't it faster just continue typing instead of
> next-next-next or insert the first and remove the last letters and
> re-write the ones I need?
>
> Inserting the common prefix is actually a good trick that asserts that
> the tab-insertion will be correct (but maybe incomplete), because when
> some candidate is shown it means that it is 100% sure what is valid
> after point in the whole completion list; but and also matches the
> muscular memory for bash users.

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.

> In the minibuffer (and in bash and zsh) the behavior is
> similar. Generally we only need preview+tab-insert the common part,
> because the first candidate is not good in most of the cases; actually
> it is as good and bad than any other candidate (that's why packages that
> track and sort based on history, and context exist) OTOH to browse
> candidates is generally better things like corfu, company or
> auto-complete, so, a list/tooltip to navigate with extra information (a
> preview of 5-10 candidates, a counter of the total number, a counter
> with the relative position in the list, similar candidates grouped and
> sorted in the tooltip).
>
> Inserting the common part is useful when typing, to safe time, specially
> when working with long names prefixes packages (when all the functions
> and variables have the same prefix). Inserting the first candidate for
> those is actually annoying because there will be many candidates with
> same prefix, but different suffix. And rotating candidate is usually
> slower than just type some more letter. OTOH
> com<Tab>-pr<Tab>-somesufix is faster and intuitive than
> com<M-n><M-n><M-n><M-n>..<TAB> specially because the user does not even
> know if the actual candidates list is long or short and how far the
> candidate may be.

By using this completion-preview-insert-common-prefix (for example, by
binding it to TAB or another key in completion-preview-active-mode-map)
I think we should get that "com<TAB>-pr<TAB>-somesufix" behavior that
you're after.  IIUC, the difference from what we get with the "common"
option in your patch is that in my approach the preview always shows a
full candidate, not just the longest common prefix.  Right?

>>> Certainly we can add many other features, properties, bindings and
>>> colors, but every feature implies complexity I'll like to avoid
>>> (specially in a completion feature code). The current package is
>>> simple, with few bindings and a predictable behavior. Please keep
>>> that.
>>>
>>> I added this change because it didn't imply more than 10 extra lines
>>> of execution code (ignoring comments); otherwise I would prefer to
>>> live with it as is.
>>
>>While I agree that brevity and simplicity are some of the nice
>>properties of this library, IMO providing useful features is far more
>>important then keeping the line count to a minimum.  So if there's a
>>strong use case that we don't currently support, I wouldn't mind adding
>>10, 100 or 1000 lines of code to accommodate that.
>>
> I don't mean that having a more elaborated preview is not useful; what I
> mean is that it doesn't worth it due to the code complexity and
> performance impact it generally implies. Many packages have tried this
> before.
>
> The package is useful as is, and in Emacs generally every new feature
> finds 100 requests for more and more details and configs hard to
> maintain and sadly mostly under-tested due to the high number of
> different config combinations needed. So I recommend KISS better and
> just add more features if they are actually requested by someone.
>
> But of course, you are free to implement the whole completion feature.
> Now you have the equivalents to company-preview-frontend and
> company-preview-if-just-one-frontend... my patch actually tried to mimic
> the missing company-preview-common-frontend

Thanks, I'll take a look at company-preview-common-frontend as well.

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

> 2. The `completion-all-completions`' output is a list with propertized
> string and information of the match location within the string... maybe
> it worth taking advantage of that information for the insertion or
> preview.

Maybe, what do you have in mind?

> Otherwise it is probably better to use `all-completions` instead. It
> will be faster and you don't need to de-propertize the substrings to add
> your own fonts and properties.

Note that we let-bind completion-lazy-hilit to t, so this is usually not
a problem.


Best,

Eshel