bug#66948: [PATCH] Add Completion Preview mode

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: juri@linkov.net,  dmitry@gutov.dev,  philipk@posteo.net,
>   joaotavora@gmail.com,  stefankangas@gmail.com,  66948@debbugs.gnu.org
> Date: Fri, 10 Nov 2023 08:59:56 +0100
> 
> Eshel Yaron <me@eshelyaron.com> writes:
> 
> > This is for Emacs core, I'm attaching the latest patch again.  I think
> > it mostly needs your OK.  Notably, I'd appreciate it if you could check
> > out the addition to the manual and see if you have any comments on that.
> 
> And now with the actual patch attached... sorry about that.

Thanks.  If this is for core, I think there's still work to be done
here, see the comments below.

> --- a/doc/emacs/programs.texi
> +++ b/doc/emacs/programs.texi
> @@ -1701,6 +1701,90 @@ Symbol Completion
>    In Text mode and related modes, @kbd{M-@key{TAB}} completes words
>  based on the spell-checker's dictionary.  @xref{Spelling}.
>  
> +@cindex completion preview
> +@cindex preview completion
> +@cindex suggestion preview
> +@cindex Completion Preview mode
> +@findex completion-preview-mode
> +@vindex completion-preview-mode
> +  Completion Preview mode is a minor mode that shows you symbol
> +completion suggestions as type.  When you enable Completion Preview
> +mode in a buffer (with @kbd{M-x completion-preview-mode}), Emacs
> +examines the text around point after certain commands you invoke and
> +automatically suggests a possible completion.  Emacs displays this
> +suggestion with an inline preview right after point, so you see in
> +advance exactly how the text will look if you accept the completion
> +suggestion---that's why it's called a preview.

I don't think this minor mode warrants such a long and detailed
description in the user manual.  The section where you added that just
mentions the various similar features, it doesn't describe them in
such great detail, so I think we should do the same for this new mode.
IOW, just mention it and what it does in a sentence or two, and move
the rest of the description to the Commentary section of
completion-preview.el and/or to the relevant doc strings.

> +*** New minor mode 'completion-preview-mode'.
> +This minor mode shows you symbol completion suggestions as you type,
> +using an inline preview.  New user options in the 'completion-preview'
> +customization group control exactly when Emacs displays this preview.

This fails to mention that (evidently) this mode is intended to be
used only in descendants of prog-mode, i.e. that useful completions
will be available only for editing program source.  But see below.

> +;; This library provides the Completion Preview mode.  This minor mode
> +;; displays the top completion candidate for the symbol at point in an
> +;; overlay after point.  If you want to enable Completion Preview mode
> +;; in all programming modes, add the following to your Emacs init:
> +;;
> +;;     (add-hook 'prog-mode-hook #'completion-preview-mode)

I'm not sure why this is advertised for prog-mode.  Are completions
produced for descendants of Text mode, for example?  If not, why not?
I see other apps offering completion in this style for editing
human-readable text, so why cannot we have that as well (for word at
point)?

> +(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 overlay when there is exactly one completion candidate
           ^^^^^^^
This is an implementation detail, better left out of the doc string.

> +that matches the symbol at point, otherwise it shows the top
> +candidate also when there are multiple matching candidates."

What do you mean by "top candidate"? I can try guessing, but I think
it is better to reword this.

> +  :type 'boolean)

Every new defcustom should have a :version tag (this comment is for
all the defcustoms in this file).

> +(defcustom completion-preview-commands '(self-insert-command
> +                                         delete-backward-char
> +                                         backward-delete-char-untabify)

I think you should add insert-char to this list.

Also, did you test this minor mode when Overwrite mode is in effect?

> +(defcustom completion-preview-minimum-symbol-length 3
> +  "Minimum length of the symbol at point for showing completion preview."
> +  :type 'natnum)

Why do we need this defcustom?  IOW, why not show the completion after
a single character?

> +(defcustom completion-preview-hook
> +  '(completion-preview-require-certain-commands
> +    completion-preview-require-minimum-symbol-length)
> +  "Hook for functions that determine whether to show preview completion.
> +
> +Completion Preview mode calls each of these functions in order
> +after each command, and only displays the completion preview when
> +all of the functions return non-nil."

This feature sounds like over-engineering to me.

> +(defface completion-preview-exact
> +  '((t :underline t :inherit completion-preview))

The underline face is not universally supported, so this defface
should have fallbacks.

> +(defvar completion-preview--internal-commands
> +  '(completion-preview-next-candidate completion-preview-prev-candidate)
> +  "List of commands that manipulate the completion preview.")
> +
> +(defun completion-preview--internal-command-p ()
> +  "Return non-nil if `this-command' manipulates the completion preview."
> +  (memq this-command completion-preview--internal-commands))

Should this be a defsubst?

> +(defun completion-preview-require-certain-commands ()
> +  "Check if `this-command' is one of `completion-preview-commands'."
> +  (or (completion-preview--internal-command-p)
> +      (memq this-command completion-preview-commands)))

Likewise here.

> +(defun completion-preview-require-minimum-symbol-length ()
> +  "Check if the length of symbol at point is at least above a certain 
> threshold.
> +`completion-preview-minimum-symbol-length' determines that threshold."
> +  (pcase (bounds-of-thing-at-point 'symbol)
> +    (`(,beg . ,end)
> +     (<= completion-preview-minimum-symbol-length (- end beg)))))

Is usage of pcase really justified here, and if so, why?

> +(defun completion-preview--make-overlay (pos string)
> +  "Make a new completion preview overlay at POS showing STRING."
> +  (if completion-preview--overlay
> +      (move-overlay completion-preview--overlay pos pos)
> +    (setq completion-preview--overlay (make-overlay pos pos)))

Should this overlay be specific to the selected window?  IOW, do we
really want to see the preview in all the windows showing this buffer?

> +  (add-text-properties 0 1 '(cursor 1) string)
> +  (overlay-put completion-preview--overlay 'after-string string)

Sounds like you keep calling overlay-put and adding the same property
to the string each time this function is called, even if the overlay
already shows the same string?

> +(define-minor-mode completion-preview-active-mode
> +  "Mode for when the completion preview is active."
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
It is better to say "when the completion preview is shown".  "Active"
is ambiguous here.

> +(defun completion-preview--exit-function (func)
> +  "Return an exit function that hides the completion preview and calls FUNC."
> +  (lambda (&rest args)
> +    (completion-preview-active-mode -1)
> +    (when func (apply func args))))
       ^^^^^^^^^^
Perhaps "(when (functionp func) ..."?

> +(defun completion-preview--update ()
> +  "Update completion preview."
> +  (pcase (let ((completion-preview-insert-on-completion nil))
> +           (run-hook-with-args-until-success 'completion-at-point-functions))
> +    (`(,beg ,end ,table . ,plist)

Why use pcase here and not seq-let?

> +(defun completion-preview--show ()
> +  "Show completion preview."
> +  (when completion-preview-active-mode
> +    (let* ((beg (completion-preview--get 'completion-preview-beg))
> +           (cands (completion-preview--get 'completion-preview-cands))
> +           (index (completion-preview--get 'completion-preview-index))
> +           (cand (nth index cands))
> +           (len (length cand))
> +           (end (+ beg len))
> +           (cur (point))
> +           (face (get-text-property 0 'face (completion-preview--get 
> 'after-string))))
> +      (if (and (< beg cur end) (string-prefix-p (buffer-substring beg cur) 
> cand))
> +          (overlay-put (completion-preview--make-overlay
> +                        cur (propertize (substring cand (- cur beg))
> +                                        'face face))
> +                       'completion-preview-end cur)
> +        (completion-preview-active-mode -1))))
> +  (while-no-input (completion-preview--update)))

I'm puzzled by this function.  What does it do, and why is it needed?

> +(defun completion-preview--post-command ()
> +  "Create, update or delete completion preview post last command."
> +  (if (run-hook-with-args-until-failure 'completion-preview-hook)
> +      (or (completion-preview--internal-command-p)
> +          (completion-preview--show))
> +    (completion-preview-active-mode -1)))

This needs more comments to explain the non-trivial logic.

> +(defun completion-preview--insert ()
> +  "Completion at point function for inserting the current preview."

The purpose of this function should be described either in the doc
string or in some comment.

> +(defun completion-preview-insert ()
> +  "Insert the current completion preview."

You cannot "insert the preview".  Please reword the doc string.

> +  (interactive)
> +  (let ((completion-preview-insert-on-completion t))
> +    (completion-at-point)))

Why not just insert the string you show in the preview?

> +(defun completion-preview-prev-candidate ()
> +  "Cycle the preview to the previous completion suggestion."

You are cycling the candidates, not the preview.

> +(defun completion-preview-next-candidate (direction)
> +  "Cycle the preview to the next completion suggestion in DIRECTION.
> +
> +DIRECTION should be either 1 which means cycle forward, or -1
> +which means cycle backward.  Interactively, DIRECTION is the
> +prefix argument."            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ^^^^^^^^^^^^^^^
"...and defaults to 1".