bug#59486: completion-auto-wrap disobeyed by vertical navigation

[Eli Zaretskii <eliz@gnu.org>]

> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 01 Nov 2023 19:45:50 +0200
> 
> Now here is a patch that implements the long-discussed feature of
> using arrow keys for navigating completions from the minibuffer
> only when the *Completions* buffer is visible.
> 
> It's disabled by default and can be enabled by the option
> 'minibuffer-completion-visible'.  It works nicely for
> the in-buffer completions as well:

Thanks.  I have a few comments.

> +(defcustom minibuffer-completion-visible t
> +  "Non-nil means to navigate completions with arrows from the minibuffer.
> +This has effect only when the window with the *Completions* buffer
> +is visible on the screen."

This doc string needs to be improved, as it currently doesn't explain
its effect in enough detail, IMO.  Maybe because "non-nil means to
navigate" is awkward/confusing English.

Regarding the last sentence of the doc string: does it mean that if
the *Completions* buffer is not in any window, the arrow keys in the
minibuffer will not move point in that buffer?  If so, why this
strange design?

> +(defun minibuffer-bind-visible (binding)
> +  `(menu-item
> +    "" ,binding
> +    :filter ,(lambda (cmd)
> +               (when (get-buffer-window "*Completions*" 0)
> +                 cmd))))

This function needs a doc string and possibly a better name, to match
what it actually does.

The argument zero to get-buffer-window AFAIU means that it will return
non-nil when the buffer is shown in some window on an iconified frame,
and I wonder why we would consider such a buffer "visible".

> +  "RET"     (minibuffer-bind-visible #'minibuffer-choose-completion)

AFAICT, this important binding is not mentioned or hinted in the doc
string of the new option.

> -(defun minibuffer-next-completion (&optional n)
> +(defun minibuffer-next-completion (&optional n vertical)
>    "Move to the next item in its completions window from the minibuffer.
> +When the optional argument VERTICAL is non-nil, move vertically.

The "move vertically" part contradicts the "to the next item" part,
doesn't it?  Thus, I think the added sentence should be more detailed,
and should explicitly say that the result will be a move to the next
line, not the next item (and perhaps also include a link to
next-line-completion).

> +(defun minibuffer-next-line-completion (&optional n)
> +  "Move to the next completion line from the minibuffer.

This sentence is misleading, IMO.  Assuming I understood what you
mean, I would rephrase:

  Move to the completion candidate on the next line in the completions buffer.

> +When `minibuffer-completion-auto-choose' is non-nil, then also
> +insert the selected completion to the minibuffer."

Please make a habit of talking about "completion candidate" in these
cases, not about "completion".  The latter is ambiguous, since it can
refer both to a candidate and to the action of performing completion.

> +(defun minibuffer-previous-line-completion (&optional n)
> +  "Move to the previous completion line from the minibuffer.
> +When `minibuffer-completion-auto-choose' is non-nil, then also
> +insert the selected completion to the minibuffer."

Same here.