GNU bug report logs - #70381
[PATCH] New command 'completion-preview-complete'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 70381 in the body.
You can then email your comments to 70381 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Ergus <spacibba <at> aol.com>
Subject: [PATCH] New command 'completion-preview-complete'
Date: Sun, 14 Apr 2024 13:21:54 +0200

[Message part 1 (text/plain, inline)]

Tags: patch

Following a recent discussion on emacs-devel[0], this patch adds a new
command for Completion Preview mode that completes the symbol at point up
to the longest common prefix of all completion candidates.  This patch
also adds a visual indication for the longest common prefix when showing
it as part of the completion preview, so the user can tell how far the new
'completion-preview-complete' will complete before invoking this command.
For example, if the symbol at point is "foo", and the completion
candidates are "foobar" and "foobaz", then the preview shows "bar" with
the common prefix "ba" highlighted in face 'completion-preview-common'.

To provide this feature efficiently, this patch modifies the way we store
the completion candidates while the preview is visible.  This amounts to
quite a few changes, so I added some tests to make sure nothing breaks,
and a bunch of comments to clarify what each part is doing.

One thing I'm not sure about is the keybinding for the new command.  In
Company, the command that inserts the common part (like the new
'completion-preview-complete' does) is bound to TAB, but we already use
TAB for 'completion-preview-insert', which inserts the entire candidate.
(Note that these bindings are only in effect when the preview is visible.)
The analogous command to 'completion-preview-insert' in Company is bound
to RET, but I feel that binding RET may be too intrusive to do by default.
For demonstration purposes, 'completion-preview-complete' is bound to M-i
in this patch.  Suggestions for a better choice of keybinding, and any
other comments, would be most welcome :)


Thanks,

Eshel


[0] https://lists.gnu.org/archive/html/emacs-devel/2024-04/msg00154.html

[0001-New-command-completion-preview-complete.patch (text/patch, attachment)]

Message #8 received at 70381 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 70381 <at> debbugs.gnu.org, spacibba <at> aol.com
Subject: Re: bug#70381: [PATCH] New command 'completion-preview-complete'
Date: Sun, 14 Apr 2024 15:01:35 +0300

> Cc: Ergus <spacibba <at> aol.com>
> Date: Sun, 14 Apr 2024 13:21:54 +0200
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> Following a recent discussion on emacs-devel[0], this patch adds a new
> command for Completion Preview mode that completes the symbol at point up
> to the longest common prefix of all completion candidates.  This patch
> also adds a visual indication for the longest common prefix when showing
> it as part of the completion preview, so the user can tell how far the new
> 'completion-preview-complete' will complete before invoking this command.
> For example, if the symbol at point is "foo", and the completion
> candidates are "foobar" and "foobaz", then the preview shows "bar" with
> the common prefix "ba" highlighted in face 'completion-preview-common'.

Thanks.

> * lisp/completion-preview.el (completion-preview--try-table):
> Return longest common prefix and list of suffixes instead of
> list of full candidates.  Add illustrative comment.
> (completion-preview--capf-wrapper, completion-preview--update)
> (completion-preview--show, completion-preview-insert)
> (completion-preview-next-candidate): Adjust.
> (completion-preview-common): New face.
> (completion-preview-exact): Distinguish from 'c-p-common'.
> (completion-preview-complete): New command.   ^^^^^^^^^^
> (completion-preview-active-mode-map): Bind it.
> (completion-preview-mode): Mention it in docstring.
> (completion-preview-commands): Add 'c-p-complete'.
                                      ^^^^^^^^^^^^
Please don't abbreviate symbols in the log entries.  Those are
frequently used to search for changes of functions/variables, and such
abbreviations defeat those searches.

If you are annoyed by the need to type long strings, I usually find
M-/ instrumental in reducing that annoyance considerably.

> +(defface completion-preview-common
>    '((((supports :underline t))
>       :underline t :inherit completion-preview)
>      (((supports :weight bold))
>       :weight bold :inherit completion-preview)
>      (t :background "gray"))
> -  "Face for exact completion preview overlay."
> +  "Face for completions longest common prefix in the completion preview."
               ^^^^^^^^^^^
This word is redundant here.  I'd replace it with "the".

> +(defvar-local completion-preview--inhibit-update-p nil
> +  "Whether to inhibit updateing the completion preview following this command.")
                         ^^^^^^^^^
"updating"

> +      (set-text-properties 0 (length suffix)
> +                           `(face ,(if (cdr suffixes)
> +                                       'completion-preview
> +                                     'completion-preview-exact))
> +                           suffix)
> +      (set-text-properties 0 (length common)
> +                           `(face ,(if (cdr suffixes)
> +                                       'completion-preview-common
> +                                     'completion-preview-exact))
> +                           common)

Is the use of back-ticks really necessary here?

> +      (set-text-properties 0 (length suffix)
> +                           `(face ,(if (cdr sufs)
> +                                       'completion-preview
> +                                     'completion-preview-exact))
> +                           suffix)

Likewise here (and in few other places).

Message #11 received at 70381 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 70381 <at> debbugs.gnu.org, spacibba <at> aol.com
Subject: Re: bug#70381: [PATCH] New command 'completion-preview-complete'
Date: Sun, 14 Apr 2024 16:05:20 +0200

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: Ergus <spacibba <at> aol.com>
>> Date: Sun, 14 Apr 2024 13:21:54 +0200
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>
>> Following a recent discussion on emacs-devel[0], this patch adds a new
>> command for Completion Preview mode that completes the symbol at point up
>> to the longest common prefix of all completion candidates.  This patch
>> also adds a visual indication for the longest common prefix when showing
>> it as part of the completion preview, so the user can tell how far the new
>> 'completion-preview-complete' will complete before invoking this command.
>> For example, if the symbol at point is "foo", and the completion
>> candidates are "foobar" and "foobaz", then the preview shows "bar" with
>> the common prefix "ba" highlighted in face 'completion-preview-common'.
>
> Thanks.
>
>> * lisp/completion-preview.el (completion-preview--try-table):
>> Return longest common prefix and list of suffixes instead of
>> list of full candidates.  Add illustrative comment.
>> (completion-preview--capf-wrapper, completion-preview--update)
>> (completion-preview--show, completion-preview-insert)
>> (completion-preview-next-candidate): Adjust.
>> (completion-preview-common): New face.
>> (completion-preview-exact): Distinguish from 'c-p-common'.
>> (completion-preview-complete): New command.   ^^^^^^^^^^
>> (completion-preview-active-mode-map): Bind it.
>> (completion-preview-mode): Mention it in docstring.
>> (completion-preview-commands): Add 'c-p-complete'.
>                                       ^^^^^^^^^^^^
> Please don't abbreviate symbols in the log entries.  Those are
> frequently used to search for changes of functions/variables, and such
> abbreviations defeat those searches.

All right, fixed in the updated patch below.

> If you are annoyed by the need to type long strings, I usually find
> M-/ instrumental in reducing that annoyance considerably.
>
>> +(defface completion-preview-common
>>    '((((supports :underline t))
>>       :underline t :inherit completion-preview)
>>      (((supports :weight bold))
>>       :weight bold :inherit completion-preview)
>>      (t :background "gray"))
>> -  "Face for exact completion preview overlay."
>> +  "Face for completions longest common prefix in the completion preview."
>                ^^^^^^^^^^^
> This word is redundant here.  I'd replace it with "the".

Done.

>> +(defvar-local completion-preview--inhibit-update-p nil
>> +  "Whether to inhibit updateing the completion preview following this command.")
>                          ^^^^^^^^^
> "updating"

Fixed.

>> +      (set-text-properties 0 (length suffix)
>> +                           `(face ,(if (cdr suffixes)
>> +                                       'completion-preview
>> +                                     'completion-preview-exact))
>> +                           suffix)
>> +      (set-text-properties 0 (length common)
>> +                           `(face ,(if (cdr suffixes)
>> +                                       'completion-preview-common
>> +                                     'completion-preview-exact))
>> +                           common)
>
> Is the use of back-ticks really necessary here?
>
>> +      (set-text-properties 0 (length suffix)
>> +                           `(face ,(if (cdr sufs)
>> +                                       'completion-preview
>> +                                     'completion-preview-exact))
>> +                           suffix)
>
> Likewise here (and in few other places).

Not necessary, no.  So I've changed these to regular (list ...) forms.
I kept a couple of other backticks that do make the code clearer IMO.


Thanks for taking a look, here's the updated patch:

[v2-0001-New-command-completion-preview-complete.patch (text/x-patch, attachment)]

Message #14 received at 70381 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 70381 <at> debbugs.gnu.org, spacibba <at> aol.com
Subject: Re: bug#70381: [PATCH] New command 'completion-preview-complete'
Date: Thu, 18 Apr 2024 13:49:47 +0300

> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: 70381 <at> debbugs.gnu.org,  spacibba <at> aol.com
> Date: Sun, 14 Apr 2024 16:05:20 +0200
> 
> Thanks for taking a look, here's the updated patch:

LGTM, feel free to install.

Message #17 received at 70381 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 70381 <at> debbugs.gnu.org, spacibba <at> aol.com
Subject: Re: bug#70381: [PATCH] New command 'completion-preview-complete'
Date: Sat, 20 Apr 2024 14:36:55 +0300

close 70381 30.1
quit

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Eshel Yaron <me <at> eshelyaron.com>
>> Cc: 70381 <at> debbugs.gnu.org,  spacibba <at> aol.com
>> Date: Sun, 14 Apr 2024 16:05:20 +0200
>> 
>> Thanks for taking a look, here's the updated patch:
>
> LGTM, feel free to install.

Thanks, pushed to master, and closing the bug.

Ergus, if you get a chance to try out these changes, I'd love to get
your feedback.


Best,

Eshel

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.