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

Previous Next

Full log

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

Previous Next

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