Re: [PATCH] Completion preview common

[Ergus <spacibba@aol.com>]

Hi:

On Wed, Apr 10, 2024 at 01:55:36PM +0200, Eshel Yaron wrote:
>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:
>>
>> 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.
>
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)

Ideally We could add a <tab><tab> approach to insert the common in the
first tab (if available) and a the rest in a second tab... But
again... this requires even more changes... not sure it worth it.

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

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

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?

Also the try-completion function only handles correctly the candidates
with the same prefix as it explains in the doc. So, in this case, the
assumption is also a simplification, and consistency assertion with what
the package is intended to do.

>> 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?
>
The output already has fontified the matching part of the string... and
the range in the match... you can just use that information as is to
know what's the suffix.

>> 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
>
Best,
Ergus