[PATCH] Completion preview common

[PATCH] Completion preview common

[Ergus]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

Hi all:

Recently I have been playing with the new completion-preview mode and
I'm very happy to see something like this finally added to emacs with a
simple and clean code.

However, there is a missing detail that I find useful when typing:
common preview completion.

The goal is to preview a common prefix when available and longer than
the prefix already inserted. This is a point in between 1) exact match
with one candidate only and 2) getting the first candidate in the whole
list.

I tried to avoid adding complexity to the code (because the thing I like
the most in the package is actually its simplicity and predictability);
but the extra functionality I think worth the few new lines.

I attach the patch for a first review.

Best,
Ergus


[-- Attachment #2: completion-preview-common.patch --]
[-- Type: text/plain, Size: 4483 bytes --]

diff --git a/lisp/completion-preview.el b/lisp/completion-preview.el
index a86c1ba1cc9..11ac929efcf 100644
--- a/lisp/completion-preview.el
+++ b/lisp/completion-preview.el
@@ -59,16 +59,21 @@ completion-preview
   :group 'completion)
 
 (defcustom completion-preview-exact-match-only nil
-  "Whether to show completion preview only when there is an exact match.
-
-If this option is non-nil, Completion Preview mode only shows the
-preview when there is exactly one completion candidate that
-matches the symbol at point.  Otherwise, if this option is nil,
-when there are multiple matching candidates the preview shows the
-first candidate, and you can cycle between the candidates with
+  "Show completion preview only when there is an exact or common prefix match.
+
+  If this option is t, Completion Preview mode only shows the preview when
+there is exactly one completion candidate that matches the symbol at
+point.
+  If the option is `common', only shows the preview if there is a common
+prefix in all the candidates and it is longer than the current input.
+  Otherwise, if this option is nil, when there are multiple matching
+candidates the preview shows the first candidate, and you can cycle
+between the candidates with
 \\[completion-preview-next-candidate] and
 \\[completion-preview-prev-candidate]."
-  :type 'boolean
+  :type '(choice (const :tag "Yes" t)
+                 (const :tag "No" nil)
+                 (const :tag "Common" common))
   :version "30.1")
 
 (defcustom completion-preview-commands '(self-insert-command
@@ -236,7 +241,10 @@ completion-preview--try-table
          (sort-fn (or (completion-metadata-get md 'cycle-sort-function)
                       (completion-metadata-get md 'display-sort-function)
                       completion-preview-sort-function))
-         (all (let ((completion-lazy-hilit t))
+         (all (let ((completion-lazy-hilit t)
+                    (completion-styles (pcase completion-preview-exact-match-only
+                                         ('common '(basic))
+                                         (_ completion-styles))))
                 (completion-all-completions string table pred
                                             (- (point) beg) md)))
          (last (last all))
@@ -244,17 +252,28 @@ completion-preview--try-table
          (prefix (substring string base)))
     (when last
       (setcdr last nil)
-      (when-let ((sorted (funcall sort-fn
-                                  (delete prefix (all-completions prefix all)))))
-        (unless (and (cdr sorted) completion-preview-exact-match-only)
-          (list (propertize (substring (car sorted) (length prefix))
-                            'face (if (cdr sorted)
-                                      'completion-preview
-                                    'completion-preview-exact)
-                            'mouse-face 'completion-preview-highlight
-                            'keymap completion-preview--mouse-map)
-                (+ beg base) end sorted
-                (substring string 0 base) exit-fn))))))
+      (when-let ((sorted (cond
+                          ((not completion-preview-exact-match-only)
+                           (funcall sort-fn
+                                    (delete prefix (all-completions prefix all))))
+                          ((eq completion-preview-exact-match-only t)
+                           (when-let ((cands (delete prefix (all-completions prefix all))))
+                             (unless (cdr cands) cands)))
+                          ((eq completion-preview-exact-match-only 'common)
+			   (when-let ((cand (try-completion prefix all)))
+                             (when (and (stringp cand)
+                                        (> (length cand) (length prefix)))
+                               (list cand))))
+                          )))
+
+        (list (propertize (substring (car sorted) (length prefix))
+                          'face (if (cdr sorted)
+                                    'completion-preview
+                                  'completion-preview-exact)
+                          'mouse-face 'completion-preview-highlight
+                          'keymap completion-preview--mouse-map)
+              (+ beg base) end sorted
+              (substring string 0 base) exit-fn)))))
 
 (defun completion-preview--capf-wrapper (capf)
   "Translate return value of CAPF to properties for completion preview overlay."

Re: [PATCH] Completion preview common

[Eshel Yaron]

Hi there,

Ergus <spacibba@aol.com> writes:

> Recently I have been playing with the new completion-preview mode and
> I'm very happy to see something like this finally added to emacs with a
> simple and clean code.
>
> However, there is a missing detail that I find useful when typing:
> common preview completion.
>
> The goal is to preview a common prefix when available and longer than
> the prefix already inserted. This is a point in between 1) exact match
> with one candidate only and 2) getting the first candidate in the whole
> list.
>
> I tried to avoid adding complexity to the code (because the thing I like
> the most in the package is actually its simplicity and predictability);
> but the extra functionality I think worth the few new lines.
>
> I attach the patch for a first review.

Thank you for your feedback and for the patch!  Completing only up the
longest common prefix instead of inserting the full candidate sounds
like a useful option, but I'm not sure that we should also restrict the
completion preview to only showing that common prefix.  How about if we
add a command that completes up to the longest common prefix, but still
show the full candidate in the preview as we do now?  That way you can
choose on a case-by-case basis whether you want to complete all the way
or just up to the longest common prefix.

To some degree, this is already possible: if your completion-styles are
set to prefix (e.g. "basic") completion, then hitting C-M-i completes to
the longest common prefix in many cases.  But we can also add a
dedicated command to completion-preview-active-mode-map that would use
the completion data that's already stored in the preview to complete up
to the longest common prefix, so something like the following:

--8<---------------cut here---------------start------------->8---
(defun completion-preview-insert-common-prefix ()
  "Complete text at point to the common prefix of all completion candidates."
  (interactive)
  (let* ((beg (completion-preview--get 'completion-preview-beg))
         (end (completion-preview--get 'completion-preview-end))
         (all (completion-preview--get 'completion-preview-cands))
         (com (substring (try-completion "" all) (- end beg))))
    (if (string-empty-p com)
        (message "Next char not unique")
      (goto-char end)
      (insert com))))
--8<---------------cut here---------------end--------------->8---

A further improvement can be to (optionally) underline the longest
common prefix in the completion preview while showing the full
candidate, similarly to how we currently underline sole candidates.
That way you can tell how far you'd get by completing to the longest
common prefix.  WDYT?

RE: [External] : [PATCH] Completion preview common

[Drew Adams]

> The goal is to preview a common prefix
> when available and longer than the prefix
> already inserted.

Related perhaps:

https://www.emacswiki.org/emacs/Icicles_-_Expanded-Common-Match_Completion

The "expanded common match" described there
is the "longest match of your input pattern
that is common to all candidates and also
contains the first input match in the first
or second candidate, whichever is longer".

This notion and behavior apply to not only
prefix completion but also regexp completion
(and substring completion).

This kind of common-match expansion is quick
but it doesn't always find the longest match.
(And of course there can be more than one
longest common match.)

The expansion typically does find a longest
common match, however, because your input
typically matches the first or the second
candidate in only one place.

And the longer the input you type, the more
likely this is.  In practice, it's only with
very short input such as 'a' that expansion
sometimes misses a longest common match.

The algorithm tries two candidates (1st and
2nd) as its starting point, to increase the
probability of finding a longest common match.

The expanded common match isn't just a common
substring among all of the candidates matched
by your input pattern.  It's a substring
common to all candidates matched by your input,
but a substring that also matches your input.

For example, with regexp completion input
`a.z' and candidates 'abz' and 'apz', there's
no expanded common match.  The substring 'a'
is common to both candidates, but it's not
matched by the (complete) input pattern.

Re: [PATCH] Completion preview common

[Ergus]

Hi:

Your idea sounds very sensible. But I don't want to add unneeded complexity to the package. To get a complete  candidates list I prefer to rely on other tools like company or Corfu. And getting the first entry in my use case is not very useful.

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.

I made some small changes a while ago in the mini buffer completion (the options completion first-tab and so on ) and tracking that prefix, insert one tab and then the other is a code that sounds simple BUT becomes terrible to follow very easily. And C-M-i is something that I cannot remember in the moment I need it (indeed, for completion I don't find usefully anything beyond tabs and arrows)

Best,
Ergus



On April 7, 2024 11:12:18 PM GMT+02:00, Eshel Yaron <me@eshelyaron.com> wrote:
>Hi there,
>
>Ergus <spacibba@aol.com> writes:
>
>> Recently I have been playing with the new completion-preview mode and
>> I'm very happy to see something like this finally added to emacs with a
>> simple and clean code.
>>
>> However, there is a missing detail that I find useful when typing:
>> common preview completion.
>>
>> The goal is to preview a common prefix when available and longer than
>> the prefix already inserted. This is a point in between 1) exact match
>> with one candidate only and 2) getting the first candidate in the whole
>> list.
>>
>> I tried to avoid adding complexity to the code (because the thing I like
>> the most in the package is actually its simplicity and predictability);
>> but the extra functionality I think worth the few new lines.
>>
>> I attach the patch for a first review.
>
>Thank you for your feedback and for the patch!  Completing only up the
>longest common prefix instead of inserting the full candidate sounds
>like a useful option, but I'm not sure that we should also restrict the
>completion preview to only showing that common prefix.  How about if we
>add a command that completes up to the longest common prefix, but still
>show the full candidate in the preview as we do now?  That way you can
>choose on a case-by-case basis whether you want to complete all the way
>or just up to the longest common prefix.
>
>To some degree, this is already possible: if your completion-styles are
>set to prefix (e.g. "basic") completion, then hitting C-M-i completes to
>the longest common prefix in many cases.  But we can also add a
>dedicated command to completion-preview-active-mode-map that would use
>the completion data that's already stored in the preview to complete up
>to the longest common prefix, so something like the following:
>
>--8<---------------cut here---------------start------------->8---
>(defun completion-preview-insert-common-prefix ()
>  "Complete text at point to the common prefix of all completion candidates."
>  (interactive)
>  (let* ((beg (completion-preview--get 'completion-preview-beg))
>         (end (completion-preview--get 'completion-preview-end))
>         (all (completion-preview--get 'completion-preview-cands))
>         (com (substring (try-completion "" all) (- end beg))))
>    (if (string-empty-p com)
>        (message "Next char not unique")
>      (goto-char end)
>      (insert com))))
>--8<---------------cut here---------------end--------------->8---
>
>A further improvement can be to (optionally) underline the longest
>common prefix in the completion preview while showing the full
>candidate, similarly to how we currently underline sole candidates.
>That way you can tell how far you'd get by completing to the longest
>common prefix.  WDYT?
>

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] Completion preview common

[Eshel Yaron]

Hi,

Ergus <spacibba@aol.com> writes:

> Your idea sounds very sensible. But I don't want to add unneeded
> complexity to the package. To get a complete candidates list I prefer
> to rely on other tools like company or Corfu. And getting the first
> entry in my use case is not very useful.

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?

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


Best,

Eshel

Re: [PATCH] Completion preview common

[Ergus]

Hi:

On Tue, Apr 09, 2024 at 08:30:16PM +0200, Eshel Yaron wrote:
>Hi,
>
>Ergus <spacibba@aol.com> writes:
>
>> Your idea sounds very sensible. But I don't want to add unneeded
>> complexity to the package. To get a complete candidates list I prefer
>> to rely on other tools like company or Corfu. And getting the first
>> entry in my use case is not very useful.
>
>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.

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.

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

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.

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.

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.

>
>Best,
>
>Eshel
>
Best,
Ergus

Re: [PATCH] Completion preview common

[Eshel Yaron]

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

Re: [PATCH] Completion preview common

[Ergus]

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

Re: [PATCH] Completion preview common

[Eshel Yaron]

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