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

[Jules Tamagnan]

Eshel Yaron <me@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@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
Description: Full patch with addressed comments