GNU bug report logs - #71716
[PATCH] Add new completion-preview-insert-{word,sexp} commands

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 71716 in the body.
You can then email your comments to 71716 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: Jules Tamagnan <jtamagnan <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add new completion-preview-insert-{word,sexp} commands
Date: Sat, 22 Jun 2024 02:11:18 -0700

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

Tags: patch


* Problem

Oftentimes when completing a value a user wants a small part of a
completion but not the entire thing. This happens frequently when
iterating on shell commands or on similar lines of
code. completion-preview can help with this by quickly suggesting a
sensible completion pulled from any completion-at-point function. The
problem is that accepting a full completion is often inefficient because
one might only want the first part of that completion. This leads to a
lot of deletions after the fact.

* Solution

Allow inserting of partial completions when using
completion-preview. For this I've added two new commands
completion-preview-insert-word and completion-preview-insert-sexp which
will insert the next word or sexp in the completion. For consistency
with completion-preview-insert I've refactored the code so that these
three commands share a common code path.

* Notes

 - I've added new tests for this and ensured that previous ones continue
   to pass.   
 - I've signed the copyright assignments and have contributed to emacs
   previously.

* Info

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.42, cairo version 1.18.0)
Repository revision: 988203fe980e3c80f736ad0b6aae9f288ebfa0f1
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101013
System Description: NixOS 24.11 (Vicuna)

Configured using:
 'configure
 --prefix=/nix/store/3riplzxicrgaff4jm49wa4vvvrd6yd1l-emacs-git-20240615.0
 --disable-build-details --with-modules --with-x-toolkit=gtk3
 --with-cairo --with-xft --with-compress-install
 --with-toolkit-scroll-bars --with-native-compilation
 --without-imagemagick --with-mailutils --without-small-ja-dic
 --with-tree-sitter --with-xinput2 --with-xwidgets --with-dbus
 --with-selinux'

[0001-Add-new-completion-preview-insert-word-sexp-commands.patch (text/patch, attachment)]

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

From: Eshel Yaron <me <at> eshelyaron.com>
To: Jules Tamagnan <jtamagnan <at> gmail.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Sat, 22 Jun 2024 16:05:05 +0200

Hi Jules,

Jules Tamagnan <jtamagnan <at> gmail.com> writes:

> Tags: patch

Thanks for the feature request and for the patch.

> * Problem
>
> Oftentimes when completing a value a user wants a small part of a
> completion but not the entire thing. This happens frequently when
> iterating on shell commands or on similar lines of
> code. completion-preview can help with this by quickly suggesting a
> sensible completion pulled from any completion-at-point function. The
> problem is that accepting a full completion is often inefficient because
> one might only want the first part of that completion. This leads to a
> lot of deletions after the fact.
>
> * Solution
>
> Allow inserting of partial completions when using
> completion-preview.

We currently have completion-preview-complete (M-i) for that: it inserts
the common part (prefix) of all completion candidates.

> For this I've added two new commands completion-preview-insert-word
> and completion-preview-insert-sexp which will insert the next word or
> sexp in the completion.

That sounds interesting.  The ELPA package capf-autosuggest.el provided
a similar feature, IIRC.  I'd like to get a better understanding of the
use case though: when would you use one of these commands instead of
completion-preview-complete?

> For consistency with completion-preview-insert I've refactored the
> code so that these three commands share a common code path.

Good idea, but there are two issues with the current implementation:

1. AFAICT, unlike completion-preview-insert, these new commands should
   preserve (the rest of) the completion preview.  So instead of
   dismissing the preview by disabling completion-preview-active-mode
   and then relying on the subsequent post-command-hook to recreate the
   preview, I think these commands should modify (e.g. remove a word
   from the start of) the after-string property of the preview overlay,
   and inhibit a subsequent update of the preview, like we do in
   completion-preview-complete.  That way we avoid recomputing the
   completion candidates, which may lead to a flicker in this case.

2. The temporary buffer where the motion command is executed has a
   different major mode than the original buffer, so they might have
   different notions of words/sexps.

> * Notes
>
>  - I've added new tests for this and ensured that previous ones continue
>    to pass.   
>  - I've signed the copyright assignments and have contributed to emacs
>    previously.

That's great, thanks.


Eshel

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

From: Jules Tamagnan <jtamagnan <at> gmail.com>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Sat, 22 Jun 2024 11:58:33 -0700

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

Hi Eshel,

Eshel Yaron <me <at> eshelyaron.com> writes:

> Thanks for the feature request and for the patch.

Thanks for the incredibly quick feedback and thoughtful comments. I
really appreciate it. I've attached a patch addressing the first comment
and will post an additional patch for a possible fix to the second
comment.

> That sounds interesting.  The ELPA package capf-autosuggest.el provided
> a similar feature, IIRC.  I'd like to get a better understanding of the
> use case though: when would you use one of these commands instead of
> completion-preview-complete?

This is a functionality that I got used to when I tried using some other
packages. One issue that I have with completion on the "common" part of
candidates is that oftentimes when using thing functionality in my shell
I have so many candidates from my history that the common part is
relatively useless.

Generally I'd say that I'm so comfortable navigating with forward-word
and forward-sexp that this using M-f and C-M-f for this is second nature
to me. Given that this is also implemented in `capf-autosuggest` and
`github-copilot` I imagine that others might feel the same way.

> 1. AFAICT, unlike completion-preview-insert, these new commands should
>    preserve (the rest of) the completion preview.  So instead of
>    dismissing the preview by disabling completion-preview-active-mode
>    and then relying on the subsequent post-command-hook to recreate the
>    preview, I think these commands should modify (e.g. remove a word
>    from the start of) the after-string property of the preview overlay,
>    and inhibit a subsequent update of the preview, like we do in
>    completion-preview-complete.  That way we avoid recomputing the
>    completion candidates, which may lead to a flicker in this case.

Ahh that is a really good point, thank you.

> 2. The temporary buffer where the motion command is executed has a
>    different major mode than the original buffer, so they might have
>    different notions of words/sexps.

I was thinking about that when implementing this, even further one could
have locally changed the value of `find-word-boundary-function-table`
outside of `subword-mode`. One idea I had thought of was inserting the
complete after-string and performing character deletions until the
suffix was removed but this felt like an even worse solution. Maybe, in
the temporary buffer, we can bind `find-word-boundary-function-table` to
the parent buffer's value. I need to hop on a flight but can implement
this in a third patch.

 - Jules
 

[0002-Cont-Add-new-completion-preview-insert-word-sexp-com.patch (text/x-patch, inline)]

From 1bbcc10c5b23d63dc8454113403c2d834a69d803 Mon Sep 17 00:00:00 2001
From: Jules Tamagnan <jtamagnan <at> gmail.com>
Date: Sat, 22 Jun 2024 11:40:09 -0700
Subject: [PATCH 2/2] [Cont] Add new completion-preview-insert-{word,sexp}
 commands

---
 lisp/completion-preview.el            | 37 ++++++++++++++++++++-------
 test/lisp/completion-preview-tests.el |  4 +--
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/lisp/completion-preview.el b/lisp/completion-preview.el
index 3a7fa37afe0..637778caadb 100644
--- a/lisp/completion-preview.el
+++ b/lisp/completion-preview.el
@@ -456,18 +456,37 @@ completion-preview--insert
              (end (completion-preview--get 'completion-preview-end))
              (efn (plist-get (completion-preview--get 'completion-preview-props)
                              :exit-function))
-             (ful (completion-preview--get 'after-string))
-             (aft (with-temp-buffer
-                      (insert ful)
+             (aft (completion-preview--get 'after-string))
+             (ful (with-temp-buffer
+                      (insert aft)
                       (goto-char (point-min))
                       (funcall action)
-                      (buffer-substring-no-properties (point-min) (point)))))
-        (completion-preview-active-mode -1)
+                      (cons (buffer-substring-no-properties (point-min) (point))
+                            (buffer-substring (point) (point-max)))))
+             (ins (car ful))
+             (suf (cdr ful)))
+        ;; If the completion is a full completion (there is no suffix)
+        ;; deactivate the preview
+        (when (string-empty-p suf)
+          (completion-preview-active-mode -1))
+
+        ;; Insert the new text
         (goto-char end)
-        (insert aft)
-        (when (and (functionp efn) (string= ful aft))
-          ;; If we've inserted a full completion call the exit-function
-          (funcall efn (concat (buffer-substring-no-properties beg end) aft) 'finished)))
+        (insert ins)
+
+        ;; If we are not inserting a full completion update the preview
+        (when (not (string-empty-p suf))
+          (let ((pos (point)))
+            (completion-preview--inhibit-update)
+            (overlay-put (completion-preview--make-overlay
+                          pos (propertize suf
+                                          'mouse-face 'completion-preview-highlight
+                                          'keymap completion-preview--mouse-map))
+                         'completion-preview-end pos)))
+
+        ;; If we've inserted a full completion call the exit-function
+        (when (and (functionp efn) (string-empty-p suf))
+          (funcall efn (concat (buffer-substring-no-properties beg end) ins) 'finished)))
     (user-error "No current completion preview")))
 
 (defun completion-preview-insert ()
diff --git a/test/lisp/completion-preview-tests.el b/test/lisp/completion-preview-tests.el
index dedd135da73..54ba566ad3c 100644
--- a/test/lisp/completion-preview-tests.el
+++ b/test/lisp/completion-preview-tests.el
@@ -325,7 +325,7 @@ completion-preview-insert-word
       (completion-preview-tests--check-preview "bar-1 2" 'completion-preview-common)
       (completion-preview-insert-word)
       (should (string= (buffer-string) "foobar"))
-      (should-not completion-preview--overlay)
+      (completion-preview-tests--check-preview "-1 2" 'completion-preview)
       (should-not exit-fn-called)
       (should-not exit-fn-args))))
 
@@ -347,7 +347,7 @@ completion-preview-insert-sexp
       (completion-preview-tests--check-preview "bar-1 2" 'completion-preview-common)
       (completion-preview-insert-sexp)
       (should (string= (buffer-string) "foobar-1"))
-      (should-not completion-preview--overlay)
+      (completion-preview-tests--check-preview " 2" 'completion-preview)
       (should-not exit-fn-called)
       (should-not exit-fn-args))))
 
-- 
2.45.1

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

From: Jules Tamagnan <jtamagnan <at> gmail.com>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Sat, 22 Jun 2024 15:00:03 -0700

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

Hi Eshel,

I've further tweaked the code to address the second point of
feedback. Looking at it now it seems a bit uglier for the "standard"
insert case so I'd be willing to revert that consolidation. Overall it
seems to work well both in unit tests and in my personal testing. 

In the last message I attached a patch with only my second commit. This
new patch contains of all 3 commits:
  1. The initial change
  2. The change to preserve the prefix and reduce flicker
  3. The change to support different modes and definitions of
     word. This change also includes new tests. It is worth noting that
     this will not work as a user may expect if `forward-word` or
     `forward-sexp` are bound to other functions but hopefully the
     included helper functions can allow users to define these functions
     if they need.

Best,
Jules

[full.patch (text/x-patch, attachment)]

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

From: Eshel Yaron <me <at> eshelyaron.com>
To: Jules Tamagnan <jtamagnan <at> gmail.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Sun, 23 Jun 2024 10:00:24 +0200

Hi Jules,

Jules Tamagnan <jtamagnan <at> gmail.com> writes:

> Eshel Yaron <me <at> eshelyaron.com> writes:
>
>> 2. The temporary buffer where the motion command is executed has a
>>    different major mode than the original buffer, so they might have
>>    different notions of words/sexps.
>
> I was thinking about that when implementing this, even further one could
> have locally changed the value of `find-word-boundary-function-table`
> outside of `subword-mode`.

Right.  And when considering sexps, forward-sexp-function can come into
play, which might take into account all sorts of buffer-local variables.

> One idea I had thought of was inserting the complete after-string and
> performing character deletions until the suffix was removed but this
> felt like an even worse solution.

I think that might be the way to go, actually.  Placing the after-string
insertion and subsequent deletion in an atomic change group (and using
undo-amalgamate-change-group to let the user undo everything in one go)
should hopefully work just as well, and that would alleviate the need to
chase down and replicate complex buffer state in the temporary buffer.

Jules Tamagnan <jtamagnan <at> gmail.com> writes:

> I've further tweaked the code to address the second point of feedback.

Thanks!

> Looking at it now it seems a bit uglier for the "standard" insert case
> so I'd be willing to revert that consolidation.

I think that'd be best, yes.  Let's keep completion-preview-insert
intact for the time being and see if we there's room for cleanly
consolidating it with the new commands after we get them right.

> Overall it seems to work well both in unit tests and in my personal
> testing.
>
> In the last message I attached a patch with only my second commit. This
> new patch contains of all 3 commits:

I'll give it a try, thanks.  In the future if you could squash all
changes to a single patch I think that'd make it easiest to review.


Best,

Eshel

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

From: Jules Tamagnan <jtamagnan <at> gmail.com>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Sun, 23 Jun 2024 15:08:43 -0700

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

Hi Eshel,

I just want to start off once again by saying thank you for the
thoughtful review, help, testing, and encouragement.

I'll start by responding to the previous email and then go on to explain
the two attached patches.

---

Eshel Yaron <me <at> eshelyaron.com> writes:

> Right.  And when considering sexps, forward-sexp-function can come into
> play, which might take into account all sorts of buffer-local variables.

Yeah.. I can imagine this become a frustrating game of whack-a-mole.

> I think that might be the way to go, actually.  Placing the after-string
> insertion and subsequent deletion in an atomic change group (and using
> undo-amalgamate-change-group to let the user undo everything in one go)
> should hopefully work just as well, and that would alleviate the need to
> chase down and replicate complex buffer state in the temporary buffer.

I took a stab at implementing this. I didn't fiddle with
`undo-amalgamate-change-group` but it seems like it wasn't required to
get what felt like sensible behavior.

> I think that'd be best, yes.  Let's keep completion-preview-insert
> intact for the time being and see if we there's room for cleanly
> consolidating it with the new commands after we get them right.

That sounds great no need to make things too complicated.

> I'll give it a try, thanks.

Thank you

> In the future if you could squash all changes to a single patch I
> think that'd make it easiest to review.

That sounds great. I'll keep that in mind when presenting the next
changesets.

---

Okay now onto the latest patches. Both patches have reverted the changes
to `completion-preview-insert` and both patches pass the same set of
unit tests.

The first patch
`completion-preview-partial-insertion-with-temp-buffer.patch` is the
same as the previous patch but with two critical changes: the revert,
and the addition of a new variable
`completion-preview-context-variables` which can be used to customize
the list of variables to copy into the temporary buffer.

The second patch
`completion-preview-partial-insertion-with-region-delete.patch` is the
version of the change that uses in-buffer deletion. There's not much to
say here, it seems quite a bit more robust.

I reckon the second patch is more in line with what you had in mind but
I wanted to bring the first approach to a more acceptable state.

[completion-preview-partial-insertion-with-temp-buffer.patch (text/x-patch, attachment)]

[completion-preview-partial-insertion-with-region-delete.patch (text/x-patch, attachment)]

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

Best,
Jules

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

From: Jules Tamagnan <jtamagnan <at> gmail.com>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Sun, 23 Jun 2024 17:45:05 -0700

Jules Tamagnan <jtamagnan <at> gmail.com> writes:

> The second patch
> `completion-preview-partial-insertion-with-region-delete.patch` is the
> version of the change that uses in-buffer deletion. There's not much to
> say here, it seems quite a bit more robust.

I guess one thing to mention is that the use of `full-end` and `new-end`
could be replaced by `(+ (length aft) end)` and `(point)` respectively
which I had originally avoided to reduce calculations but might better
fit the style of the code.

Another idea would be to add a test proving that
`(completion-preview--insert-partial #'end-of-buffer)` will never place
the point too far. I have tested this on my own but haven't written a
test for it.

There are surely other style comments too, maybe the use of whitespace
within a function is considered bad form?

As always open to any and all feedback. Thanks in advance.

Best,
Jules

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jules Tamagnan <jtamagnan <at> gmail.com>
Cc: 71716 <at> debbugs.gnu.org, me <at> eshelyaron.com
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Mon, 24 Jun 2024 14:49:10 +0300

> Cc: 71716 <at> debbugs.gnu.org
> From: Jules Tamagnan <jtamagnan <at> gmail.com>
> Date: Sun, 23 Jun 2024 15:08:43 -0700
> 
> +(defcustom completion-preview-context-variables '(char-script-table
> +                                                  forward-sexp-function
> +                                                  find-word-boundary-function-table
> +                                                  inhibit-field-text-motion)
> +  "List of variables which can change the functionality of `forward-word'
> +or `forward-sexp'."
> +  :type '(repeat (variable :tag "Variable" :value char-script-table))
> +  :version "30.1")

I don't think we will install new features on the emacs-30 branch, so
this :version tag should be updated.  And the previous one as well, I
guess.

> +(defun completion-preview--determine-substring (command string)
> +  "A helper function to determine what parts of a STRING come before and
> +after the point when a certain COMMAND has been performed on that STRING"

The first line of a doc string should be a single complete sentence.
That's because some help commands, like "M-x apropos", show only the
first line of the doc strings.

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

From: Eshel Yaron <me <at> eshelyaron.com>
To: Jules Tamagnan <jtamagnan <at> gmail.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Mon, 24 Jun 2024 14:43:56 +0200

Jules Tamagnan <jtamagnan <at> gmail.com> writes:

> Hi Eshel,
>
> I just want to start off once again by saying thank you for the
> thoughtful review, help, testing, and encouragement.

Gladly, I appreciate your contribution and I'm happy to help with it.

> The first patch
> `completion-preview-partial-insertion-with-temp-buffer.patch` is the
> same as the previous patch but with two critical changes: the revert,
> and the addition of a new variable
> `completion-preview-context-variables` which can be used to customize
> the list of variables to copy into the temporary buffer.
>
> The second patch
> `completion-preview-partial-insertion-with-region-delete.patch` is the
> version of the change that uses in-buffer deletion. There's not much to
> say here, it seems quite a bit more robust.
>
> I reckon the second patch is more in line with what you had in mind but
> I wanted to bring the first approach to a more acceptable state.

Thanks for that, the second patch is looking pretty good.  I'm including
some comments below, some of these are nits that I can fix myself later.
One important point is that I'm a bit hesitant about adding the sexp
variant, rather then defining only completion-preview-insert-word, and
mentioning in the documentation that other variants are trivial to
define (and how).  The reason is that I don't have a good idea of when a
completion candidate would span multiple sexps (if you have such an
example, please share it), so I'm not sure how much utility this command
would bring in practice.

> From 7fd70fb330e0623636729657b17a9cdac3841a3d Mon Sep 17 00:00:00 2001
> From: Jules Tamagnan <jtamagnan <at> gmail.com>
> Date: Sat, 22 Jun 2024 00:45:01 -0700
> Subject: [PATCH] Add new completion-preview-insert-{word,sexp} commands
>
> * lisp/completion-preview.el: Add new completion-preview-insert-word and
>   completion-preview-insert-sexp commands.
> * test/lisp/completion-preview-tests.el: Add tests for new commands.

It's best to single-quote symbols in the commit message, like 'this'.

> +(defun completion-preview--insert-partial (command)

This should be a public function (no --), to indicate that it's fine for
users to use it in their own command definitions.  So either
completion-preview-insert-partial or completion-preview-partial-insert.
(I tend to prefer the latter, but both work.)

Also, COMMAND should instead be FUN or FUNC, since the argument doesn't
have to be command, any motion function would do.  Lastly this command
should also take &rest args and pass them to the function argument, to
facilitate writing something like (c-p-partial-insert #'forward-word 2)
to complete two words.

> +  "A helper function to insert part of the completion candidate that the
> +preview is showing."

The first line of the docstring should be a full sentence.  We also want
to describe accurately enough what this function does for users to be
able to leverage it in their definitions.  I suggest:

--8<---------------cut here---------------start------------->8---
(defun completion-preview-partial-insert (fun &rest args)
  "Insert part of the current completion preview candidate.

This function calls FUN with arguments ARGS, after temporarily inserting
the entire current completion preview candidate.  FUN should move point:
if it moves point forward into the completion text, this function
inserts the prefix of the completion candidate up to that point.
Beyond moving point, FUN should not modify the current buffer."
--8<---------------cut here---------------end--------------->8---

> +  (if completion-preview-active-mode
> +      (let* ((beg (completion-preview--get 'completion-preview-beg))
> +             (end (completion-preview--get 'completion-preview-end))
> +             (efn (plist-get (completion-preview--get 'completion-preview-props)
> +                             :exit-function))
> +             (aft (completion-preview--get 'after-string))
> +             (new-end)
> +             (full-end))
> +        ;; Insert the new text
> +        (goto-char end)
> +        (insert aft)

Better strip text properties from AFT before inserting it here.

> +        (setq full-end (point))
> +
> +        ;; Use the movement command to go to a new location in the buffer
> +        (goto-char end)
> +        (funcall command)
> +        (setq new-end (point))

We should ensure that new-end isn't smaller then end, otherwise the
deletion below won't do the right thing.

> +        (if (< new-end full-end)
> +            ;; The movement command has not taken us to the end of the
> +            ;; initial insertion this means that a partial completion
> +            ;; occured.
> +            (progn
> +              (completion-preview--inhibit-update)
> +              ;; If we are not inserting a full completion update the preview
> +              (overlay-put (completion-preview--make-overlay
> +                            new-end (propertize (delete-and-extract-region full-end new-end)
> +                                                'mouse-face 'completion-preview-highlight
> +                                                'keymap completion-preview--mouse-map))
> +                           'completion-preview-end new-end))
> +          ;; The movement command has taken us to the end of the
> +          ;; completion or past it which signifies a full completion.
> +          (goto-char full-end)
> +          (completion-preview-active-mode -1)
> +          (when (functionp efn)
> +            (funcall efn (concat (buffer-substring-no-properties beg end) aft) 'finished))))
> +    (user-error "No current completion preview")))

This is a nice use of delete-and-extract-region, but the insertion and
deletion must be inside an atomic-change-group, so we don't leave AFT
inserted in case the motion function signals an error.  This is also
where we need to add an undo-amalgamate-change-group, to prevent undo
from seeing an unwanted intermediate step in case an undo-boundary is
created between the insertion and the deletion.  So the structure should
be something like:

--8<---------------cut here---------------start------------->8---
(atomic-change-group
  (let ((change-group (prepare-change-group)))
    ;; Insert,
    ;; Move,
    ;; Delete.
    (undo-amalgamate-change-group change-group)))
--8<---------------cut here---------------end--------------->8---

> +(defun completion-preview-insert-word ()
> +  "Insert the next word of the completion candidate that the preview is showing."
> +  (interactive)
> +  (completion-preview--insert-partial #'forward-word))

This should handle an optional numeric argument, like forward-word does.


Finally, we should document completion-preview-insert-word in the
Commentary section.  Here's the text I'd like to add, you can include it
the patch (and change it as you see fit) or I'll add it later:

--8<---------------cut here---------------start------------->8---
;; You can also insert only the first word of the completion candidate
;; with the command `completion-preview-insert-word'.  With a numeric
;; prefix argument, it inserts that many words instead of just the one.
;; This command is not bound by default, but you may want to bind it to
;; M-f (or remap `forward-word') in `completion-preview-active-mode-map'
;; since it's very much like a `forward-word' that also moves "into" the
;; completion preview.
--8<---------------cut here---------------end--------------->8---


Best,

Eshel

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

From: Jules Tamagnan <jtamagnan <at> gmail.com>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Mon, 24 Jun 2024 10:16:31 -0700

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

Eshel Yaron <me <at> eshelyaron.com> writes:

> One important point is that I'm a bit hesitant about adding the sexp
> variant, rather then defining only completion-preview-insert-word, and
> mentioning in the documentation that other variants are trivial to
> define (and how).  The reason is that I don't have a good idea of when
> a completion candidate would span multiple sexps (if you have such an
> example, please share it), so I'm not sure how much utility this
> command would bring in practice.

The use case that I have for the sexp variant is when completing eshell
history. Both because: parts of shell commands such as file names can be
considered sexp's, but also because eshell itself can interpret "full"
elisp forms.

On a similar note. Some similar tools additionally define a `forward-char`
variant. This is not something that I've found a need for personally but
would be willing to add for completeness.

>> From 7fd70fb330e0623636729657b17a9cdac3841a3d Mon Sep 17 00:00:00 2001
>> From: Jules Tamagnan <jtamagnan <at> gmail.com>
>> Date: Sat, 22 Jun 2024 00:45:01 -0700
>> Subject: [PATCH] Add new completion-preview-insert-{word,sexp} commands
>>
>> * lisp/completion-preview.el: Add new completion-preview-insert-word and
>>   completion-preview-insert-sexp commands.
>> * test/lisp/completion-preview-tests.el: Add tests for new commands.
>
> It's best to single-quote symbols in the commit message, like 'this'.

Thank you. Done

>> +(defun completion-preview--insert-partial (command)
>
> This should be a public function (no --), to indicate that it's fine for
> users to use it in their own command definitions.  So either
> completion-preview-insert-partial or completion-preview-partial-insert.
> (I tend to prefer the latter, but both work.)

Thank you. Done

> Also, COMMAND should instead be FUN or FUNC, since the argument doesn't
> have to be command, any motion function would do.

Thank you. Done

> Lastly this command should also take &rest args and pass them to the
> function argument, to facilitate writing something like
> (c-p-partial-insert #'forward-word 2) to complete two words.

Thank you. Done

Another idea would be to turn `c-p-partial-insert` into a macro that
uses the `interactive-form` function to generate a sensible
insert-partial function. I'm more than happy to take this tweak on as
well.

> The first line of the docstring should be a full sentence.  We also want
> to describe accurately enough what this function does for users to be
> able to leverage it in their definitions.  I suggest:
>
> (defun completion-preview-partial-insert (fun &rest args)
>   "Insert part of the current completion preview candidate.
> This function calls FUN with arguments ARGS, after temporarily inserting
> the entire current completion preview candidate.  FUN should move point:
> if it moves point forward into the completion text, this function
> inserts the prefix of the completion candidate up to that point.
> Beyond moving point, FUN should not modify the current buffer."

Thank you. Done

> Better strip text properties from AFT before inserting it here.

Thank you. Done

There were two ways of implementing this that I could think of.

 1. Insert with properties, set `suf` to delete-and-extract-region to
    preserve the properties, use `(set-text-properties end (point) nil)`
    to remove the text properties from the deletion.
 2. Insert without text properties, use `delete-region`, set `suf` to a
     substring of `aft` directly

Both seem to work equally well, I've gone with option 2 because it seems
more consistent with what you had suggested.

> We should ensure that new-end isn't smaller then end, otherwise the
> deletion below won't do the right thing.

Thank you. Done

> This is a nice use of delete-and-extract-region, but the insertion and
> deletion must be inside an atomic-change-group, so we don't leave AFT
> inserted in case the motion function signals an error.  This is also
> where we need to add an undo-amalgamate-change-group, to prevent undo
> from seeing an unwanted intermediate step in case an undo-boundary is
> created between the insertion and the deletion.  So the structure should
> be something like:
>
> (atomic-change-group
>   (let ((change-group (prepare-change-group)))
>     ;; Insert,
>     ;; Move,
>     ;; Delete.
>     (undo-amalgamate-change-group change-group)))

Thank you. Done

I'm glad to better understand how this works now.

>> +(defun completion-preview-insert-word ()
>> +  "Insert the next word of the completion candidate that the preview is showing."
>> +  (interactive)
>> +  (completion-preview--insert-partial #'forward-word))
>
> This should handle an optional numeric argument, like forward-word does.

Thank you. Done

> Finally, we should document completion-preview-insert-word in the
> Commentary section.  Here's the text I'd like to add, you can include it
> the patch (and change it as you see fit) or I'll add it later:
>
> ;; You can also insert only the first word of the completion candidate
> ;; with the command `completion-preview-insert-word'.  With a numeric
> ;; prefix argument, it inserts that many words instead of just the one.
> ;; This command is not bound by default, but you may want to bind it to
> ;; M-f (or remap `forward-word') in `completion-preview-active-mode-map'
> ;; since it's very much like a `forward-word' that also moves "into" the
> ;; completion preview.

Thank you. Done

At the end of the paragraph I've additionally added a brief note about
the `sexp` variant.

> Best,
>
> Eshel

Best,
Jules

[completion-preview-partial-insertion.patch (text/x-patch, attachment)]

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

From: Jules Tamagnan <jtamagnan <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71716 <at> debbugs.gnu.org, me <at> eshelyaron.com
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Mon, 24 Jun 2024 11:11:16 -0700

Hi Eli,

Thank you for the review, I really appreciate it.

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

>> Cc: 71716 <at> debbugs.gnu.org
>> From: Jules Tamagnan <jtamagnan <at> gmail.com>
>> Date: Sun, 23 Jun 2024 15:08:43 -0700
>> 
>> +(defcustom completion-preview-context-variables '(char-script-table
>> +                                                  forward-sexp-function
>> +                                                  find-word-boundary-function-table
>> +                                                  inhibit-field-text-motion)
>> +  "List of variables which can change the functionality of `forward-word'
>> +or `forward-sexp'."
>> +  :type '(repeat (variable :tag "Variable" :value char-script-table))
>> +  :version "30.1")
>
> I don't think we will install new features on the emacs-30 branch, so
> this :version tag should be updated.  And the previous one as well, I
> guess.

My latest patch for this change actually removed this variable entirely
but I'll keep this in mind going forward.

>> +(defun completion-preview--determine-substring (command string)
>> +  "A helper function to determine what parts of a STRING come before and
>> +after the point when a certain COMMAND has been performed on that STRING"
>
> The first line of a doc string should be a single complete sentence.
> That's because some help commands, like "M-x apropos", show only the
> first line of the doc strings.

Similarly, I've removed this function definition but will keep this in
mind. Thank you.

Best,
Jules

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

From: Eshel Yaron <me <at> eshelyaron.com>
To: Jules Tamagnan <jtamagnan <at> gmail.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Wed, 26 Jun 2024 13:41:32 +0200

Hi Jules,

Jules Tamagnan <jtamagnan <at> gmail.com> writes:

> Eshel Yaron <me <at> eshelyaron.com> writes:
>
>> One important point is that I'm a bit hesitant about adding the sexp
>> variant, rather then defining only completion-preview-insert-word, and
>> mentioning in the documentation that other variants are trivial to
>> define (and how).  The reason is that I don't have a good idea of when
>> a completion candidate would span multiple sexps (if you have such an
>> example, please share it), so I'm not sure how much utility this
>> command would bring in practice.
>
> The use case that I have for the sexp variant is when completing eshell
> history. Both because: parts of shell commands such as file names can be
> considered sexp's, but also because eshell itself can interpret "full"
> elisp forms.

Thanks, I tried it out with cape-history from cape.el and I can see how
it may be useful for such use cases.

[...]
> Another idea would be to turn `c-p-partial-insert` into a macro that
> uses the `interactive-form` function to generate a sensible
> insert-partial function. I'm more than happy to take this tweak on as
> well.

That may be a nice addition.  In particular we could have a macro that
defines a partial insertion command, and takes care of setting the
completion-predicate of the defined command such that it's only
available when the completion preview is active.

[...]
> From 74d8efceaf8f64f7cf61e36f8a5e8a4fc86e558d Mon Sep 17 00:00:00 2001
> From: Jules Tamagnan <jtamagnan <at> gmail.com>
> Date: Mon, 24 Jun 2024 08:53:23 -0700
> Subject: [PATCH] Add new completion-preview-insert-{word,sexp} commands

Thank you, pushed to master as commit b3017e7c252, after some tweaks to
the commit message.  I've also pushed a follow up commit (9cb2a204088)
with some minor refinements, see the commit message for details.  One
notable change is that completion-preview-partial-insert does not force
point to the position of the preview overlay ("end") before calling the
motion function.  This makes completion-preview-insert-word behave more
like forward-word when point is in the middle of a multi-word symbol,
with the completion preview at the end of that symbol.  I've added
another test case that demonstrates this behavior.

Could you please give it a try to make sure that everything still works
as you expect?


Thanks,

Eshel

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

From: Juri Linkov <juri <at> linkov.net>
To: Jules Tamagnan <jtamagnan <at> gmail.com>
Cc: 71716 <at> debbugs.gnu.org, Eshel Yaron <me <at> eshelyaron.com>
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Thu, 27 Jun 2024 09:33:04 +0300

> +;; This command is not bound by default, but you may want to bind it to
> +;; M-f (or remap `forward-word') in `completion-preview-active-mode-map'
> +;; since it's very much like a `forward-word' that also moves "into" the
> +;; completion preview.

Also this is very much like 'C-w' in Isearch (isearch-yank-word-or-char),
and 'C-w' does nothing without the region, only raises an error
"The mark is not set now, so there is no region".
So 'C-w' could be bound by default.

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

From: Eshel Yaron <me <at> eshelyaron.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 71716 <at> debbugs.gnu.org, Jules Tamagnan <jtamagnan <at> gmail.com>
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Thu, 27 Jun 2024 20:31:59 +0200

Hi Juri,

Juri Linkov <juri <at> linkov.net> writes:

>> +;; This command is not bound by default, but you may want to bind it to
>> +;; M-f (or remap `forward-word') in `completion-preview-active-mode-map'
>> +;; since it's very much like a `forward-word' that also moves "into" the
>> +;; completion preview.
>
> Also this is very much like 'C-w' in Isearch (isearch-yank-word-or-char),
> and 'C-w' does nothing without the region, only raises an error
> "The mark is not set now, so there is no region".
> So 'C-w' could be bound by default.

Thanks, that's an interesting suggestion.  I prefer not to bind
completion-preview-insert-word by default right away, because Completion
Preview mode should by default be unintrusive, and bindings in
completion-preview-active-mode-map override bindings that users may have
in place when the preview appears.  But if you use this binding and find
it convenient, feel free to add such a recommendation to the commentary.


Best,

Eshel

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

From: Jules Tamagnan <jtamagnan <at> gmail.com>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Thu, 27 Jun 2024 22:49:02 -0700

Eshel Yaron <me <at> eshelyaron.com> writes:

> Thank you, pushed to master as commit b3017e7c252, after some tweaks to
> the commit message.  I've also pushed a follow up commit (9cb2a204088)
> with some minor refinements, see the commit message for details.  One
> notable change is that completion-preview-partial-insert does not force
> point to the position of the preview overlay ("end") before calling the
> motion function.  This makes completion-preview-insert-word behave more
> like forward-word when point is in the middle of a multi-word symbol,
> with the completion preview at the end of that symbol.  I've added
> another test case that demonstrates this behavior.
>
> Could you please give it a try to make sure that everything still works
> as you expect?
>
> Thanks,
>
> Eshel

I've taken this change around the block since yesterday and everything
seems to be working exactly as I would expect it to. I've also reviewed
the cleanup that you did and tried to take some notes, especially on the
commit message. Thanks again for all of the work that you put into
creating this packaging in the first place, helping me through my
change, and cleaning up the rough edges in the aftermath.

Best,
Jules

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

From: Eshel Yaron <me <at> eshelyaron.com>
To: Jules Tamagnan <jtamagnan <at> gmail.com>
Cc: 71716 <at> debbugs.gnu.org
Subject: Re: bug#71716: [PATCH] Add new completion-preview-insert-{word,
 sexp} commands
Date: Fri, 28 Jun 2024 17:00:15 +0200

close 71716 31.1
quit

Jules Tamagnan <jtamagnan <at> gmail.com> writes:

> Eshel Yaron <me <at> eshelyaron.com> writes:
>
>> Thank you, pushed to master as commit b3017e7c252, after some tweaks to
>> the commit message.  I've also pushed a follow up commit (9cb2a204088)
>> with some minor refinements, see the commit message for details.  One
>> notable change is that completion-preview-partial-insert does not force
>> point to the position of the preview overlay ("end") before calling the
>> motion function.  This makes completion-preview-insert-word behave more
>> like forward-word when point is in the middle of a multi-word symbol,
>> with the completion preview at the end of that symbol.  I've added
>> another test case that demonstrates this behavior.
>>
>> Could you please give it a try to make sure that everything still works
>> as you expect?
>>
>> Thanks,
>>
>> Eshel
>
> I've taken this change around the block since yesterday and everything
> seems to be working exactly as I would expect it to.

Great, I'm therefore closing this bug.  

> I've also reviewed the cleanup that you did and tried to take some
> notes, especially on the commit message.

There was actually one more small issue that I now fixed in commit
5e3b94e1bec - in case completion-preview-insert-word would just move
word without leaving anything inserted, it would still record an
unwanted undo operation (that just inserts and deletes everything again,
basically a no-op).  Now we avoid recording anything in buffer-undo-list
if we're only moving point.

> Thanks again for all of the work that you put into creating this
> packaging in the first place, helping me through my change, and
> cleaning up the rough edges in the aftermath.

Thank you for this nice contribution!


Eshel

Previous Next

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