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

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 219 bytes --]

In a multi-column layout, the keys <left> and <right>
wrap to the beginning/end of the completions buffer,
but <up> and <down> don't.  Here is a patch that supports
completion-auto-wrap for wrapping to the top/bottom:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: next-line-completion.patch --]
[-- Type: text/x-diff, Size: 3612 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 0f44b14948c..459af2fcf55 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9572,6 +9574,8 @@ completion-list-mode-map
     (define-key map "\C-m" 'choose-completion)
     (define-key map "\e\e\e" 'delete-completion-window)
     (define-key map [remap keyboard-quit] #'delete-completion-window)
+    (define-key map [up] 'previous-line-completion)
+    (define-key map [down] 'next-line-completion)
     (define-key map [left] 'previous-completion)
     (define-key map [right] 'next-completion)
     (define-key map [?\t] 'next-completion)
@@ -9736,6 +9740,77 @@ next-completion
     (when (/= 0 n)
       (switch-to-minibuffer))))
 
+(defun previous-line-completion (&optional n)
+  "Move to the item on the previous line in the completion list.
+With prefix argument N, move back N items line-wise (negative N
+means move forward).
+
+Also see the `completion-auto-wrap' variable."
+  (interactive "p")
+  (next-line-completion (- n)))
+
+(defun next-line-completion (&optional n)
+  "Move to the item on the next line in the completion list.
+With prefix argument N, move N items line-wise (negative N
+means move backward).
+
+Also see the `completion-auto-wrap' variable."
+  (interactive "p")
+  (let ((column (current-column))
+        pos)
+    (catch 'bound
+      (while (> n 0)
+        (setq pos nil)
+        (save-excursion
+          (while (and (not pos) (not (eobp)))
+            (forward-line 1)
+            (when (and (not (eobp))
+                       (eq (move-to-column column) column)
+                       (get-text-property (point) 'mouse-face))
+              (setq pos (point)))))
+        (if pos
+            (goto-char pos)
+          (when completion-auto-wrap
+            (let ((column (current-column)))
+              (save-excursion
+                (goto-char (point-min))
+                (when (and (eq (move-to-column column) column)
+                           (get-text-property (point) 'mouse-face))
+                  (setq pos (point)))
+                (while (and (not pos) (not (eobp)))
+                  (forward-line 1)
+                  (when (and (eq (move-to-column column) column)
+                             (get-text-property (point) 'mouse-face))
+                    (setq pos (point)))))
+              (if pos (goto-char pos)))))
+        (setq n (1- n)))
+
+      (while (< n 0)
+        (setq pos nil)
+        (save-excursion
+          (while (and (not pos) (not (bobp)))
+            (forward-line -1)
+            (when (and (not (bobp))
+                       (eq (move-to-column column) column)
+                       (get-text-property (point) 'mouse-face))
+              (setq pos (point)))))
+        (if pos
+            (goto-char pos)
+          (when completion-auto-wrap
+            (let ((column (current-column)))
+              (save-excursion
+                (goto-char (point-max))
+                (when (and (eq (move-to-column column) column)
+                           (get-text-property (point) 'mouse-face))
+                  (setq pos (point)))
+                (while (and (not pos) (not (bobp)))
+                  (forward-line -1)
+                  (when (and (eq (move-to-column column) column)
+                             (get-text-property (point) 'mouse-face))
+                    (setq pos (point)))))
+              (if pos (goto-char pos)))))
+        (setq n (1+ n))))))
+
 (defun choose-completion (&optional event no-exit no-quit)
   "Choose the completion at point.
 If EVENT, use EVENT's position to determine the starting position.

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]

> In a multi-column layout, the keys <left> and <right>
> wrap to the beginning/end of the completions buffer,
> but <up> and <down> don't.  Here is a patch that supports
> completion-auto-wrap for wrapping to the top/bottom:

Now pushed.  And here are the corresponding commands for
navigating the completions buffer from the minibuffer:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: minibuffer-next-completion.patch --]
[-- Type: text/x-diff, Size: 1914 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 6bb0fa3ae98..ea1e88c7234 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4452,7 +4456,7 @@ minibuffer-completion-auto-choose
   :type 'boolean
   :version "29.1")
 
-(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 `minibuffer-completion-auto-choose' is non-nil, then also
 insert the selected completion to the minibuffer."
@@ -4461,7 +4465,9 @@ minibuffer-next-completion
     (with-minibuffer-completions-window
       (when completions-highlight-face
         (setq-local cursor-face-highlight-nonselected-window t))
-      (next-completion (or n 1))
+      (if vertical
+          (next-line-completion (or n 1))
+        (next-completion (or n 1)))
       (when auto-choose
         (let ((completion-use-base-affixes t))
           (choose-completion nil t t))))))
@@ -4473,6 +4479,20 @@ minibuffer-previous-completion
   (interactive "p")
   (minibuffer-next-completion (- (or n 1))))
 
+(defun minibuffer-next-line-completion (&optional n)
+  "Move to the next completion line from the minibuffer.
+When `minibuffer-completion-auto-choose' is non-nil, then also
+insert the selected completion to the minibuffer."
+  (interactive "p")
+  (minibuffer-next-completion (or n 1) t))
+
+(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."
+  (interactive "p")
+  (minibuffer-next-completion (- (or n 1)) t))
+
 (defun minibuffer-choose-completion (&optional no-exit no-quit)
   "Run `choose-completion' from the minibuffer in its completions window.
 With prefix argument NO-EXIT, insert the completion at point to the

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

[Juri Linkov]

> And here are the corresponding commands for navigating
> the completions buffer from the minibuffer:
>
> +(defun minibuffer-next-line-completion (&optional n)
> +(defun minibuffer-previous-line-completion (&optional n)

It seems there commands can't be bound in the minibuffer.
Ideally, these keybindings could be added to
minibuffer-local-completion-map:

  "M-<left>"  #'minibuffer-previous-completion
  "M-<right>" #'minibuffer-next-completion
  "M-<up>"    #'minibuffer-previous-line-completion
  "M-<down>"  #'minibuffer-next-line-completion

But maybe 'M-<left>' and 'M-<right>' can't be taken from moving by words
even when 'C-<left>' and 'C-<right>' are duplicate keys that are doing
the same.  Or to bind 'M-<left>' and 'M-<right>' only in
completion-in-region-mode-map that is more transient by nature
and is active only as long as the completions buffer is shown.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 24 Nov 2022 09:59:52 +0200
> 
> > And here are the corresponding commands for navigating
> > the completions buffer from the minibuffer:
> >
> > +(defun minibuffer-next-line-completion (&optional n)
> > +(defun minibuffer-previous-line-completion (&optional n)
> 
> It seems there commands can't be bound in the minibuffer.
> Ideally, these keybindings could be added to
> minibuffer-local-completion-map:
> 
>   "M-<left>"  #'minibuffer-previous-completion
>   "M-<right>" #'minibuffer-next-completion
>   "M-<up>"    #'minibuffer-previous-line-completion
>   "M-<down>"  #'minibuffer-next-line-completion
> 
> But maybe 'M-<left>' and 'M-<right>' can't be taken from moving by words
> even when 'C-<left>' and 'C-<right>' are duplicate keys that are doing
> the same.  Or to bind 'M-<left>' and 'M-<right>' only in
> completion-in-region-mode-map that is more transient by nature
> and is active only as long as the completions buffer is shown.

Please don't usurp M-<LEFT> and M-<RIGHT>, as they are needed on TTY frames
where C-<LEFT> and C-<RIGHT> are not available.

I don't really understand the difference between minibuffer-previous-* and
minibuffer-previous-line-* commands (the available documentation is minimal
and doesn't explain this difference), so it's hard to suggest an
alternative.  But one immediate alternative is to use <LEFT> and <RIGHT> for
the minibuffer-previous-* family, like previous-completion does.

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

[Juri Linkov]

> I don't really understand the difference between minibuffer-previous-* and
> minibuffer-previous-line-* commands (the available documentation is minimal
> and doesn't explain this difference), so it's hard to suggest an alternative.

minibuffer-previous-* are intended to navigate the completion list
horizontally while the minibuffer is active, and minibuffer-previous-line-*
vertically.

> But one immediate alternative is to use <LEFT> and <RIGHT> for
> the minibuffer-previous-* family, like previous-completion does.

<LEFT> and <RIGHT> are useful to move point in the minibuffer.
But do you think it would be okay to use <LEFT> and <RIGHT>
for navigation of completions from the minibuffer after
adding a new user option, when is non-nil (disabled by default)?

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 59486@debbugs.gnu.org
> Date: Thu, 24 Nov 2022 20:30:00 +0200
> 
> > I don't really understand the difference between minibuffer-previous-* and
> > minibuffer-previous-line-* commands (the available documentation is minimal
> > and doesn't explain this difference), so it's hard to suggest an alternative.
> 
> minibuffer-previous-* are intended to navigate the completion list
> horizontally while the minibuffer is active, and minibuffer-previous-line-*
> vertically.

Then the names could be simplified as minibuffer-up-line and
minibuffer-down-line.  It will also make the names less confusing, IMO.

> > But one immediate alternative is to use <LEFT> and <RIGHT> for
> > the minibuffer-previous-* family, like previous-completion does.
> 
> <LEFT> and <RIGHT> are useful to move point in the minibuffer.

Then maybe PgUp and PgDn?

> But do you think it would be okay to use <LEFT> and <RIGHT>
> for navigation of completions from the minibuffer after
> adding a new user option, when is non-nil (disabled by default)?

If the arrow keys are already taken, having an option that will change their
effect would be confusing, I think.

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

[Juri Linkov]

>> minibuffer-previous-* are intended to navigate the completion list
>> horizontally while the minibuffer is active, and minibuffer-previous-line-*
>> vertically.
>
> Then the names could be simplified as minibuffer-up-line and
> minibuffer-down-line.  It will also make the names less confusing, IMO.

It's not only about the minibuffer, but also about completions,
so the commands names should also include the word "completion".

>> > But one immediate alternative is to use <LEFT> and <RIGHT> for
>> > the minibuffer-previous-* family, like previous-completion does.
>> 
>> <LEFT> and <RIGHT> are useful to move point in the minibuffer.
>
> Then maybe PgUp and PgDn?

PgUp and PgDn are used to scroll the completions buffer from the minibuffer.

>> But do you think it would be okay to use <LEFT> and <RIGHT>
>> for navigation of completions from the minibuffer after
>> adding a new user option, when is non-nil (disabled by default)?
>
> If the arrow keys are already taken, having an option that will change their
> effect would be confusing, I think.

It does the same as icomplete-vertical-mode where the arrow keys
are not confusing.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 59486@debbugs.gnu.org
> Date: Fri, 25 Nov 2022 09:47:29 +0200
> 
> >> minibuffer-previous-* are intended to navigate the completion list
> >> horizontally while the minibuffer is active, and minibuffer-previous-line-*
> >> vertically.
> >
> > Then the names could be simplified as minibuffer-up-line and
> > minibuffer-down-line.  It will also make the names less confusing, IMO.
> 
> It's not only about the minibuffer, but also about completions,
> so the commands names should also include the word "completion".

minibuffer-up-completions-line is still shorter and less confusing.

> >> > But one immediate alternative is to use <LEFT> and <RIGHT> for
> >> > the minibuffer-previous-* family, like previous-completion does.
> >> 
> >> <LEFT> and <RIGHT> are useful to move point in the minibuffer.
> >
> > Then maybe PgUp and PgDn?
> 
> PgUp and PgDn are used to scroll the completions buffer from the minibuffer.

That's a bad idea, IMO: scrolling another window should use something like
M-PgDn and C-M-v, like we do with other windows elsewhere.

> > If the arrow keys are already taken, having an option that will change their
> > effect would be confusing, I think.
> 
> It does the same as icomplete-vertical-mode where the arrow keys
> are not confusing.

icomplete-vertical-mode is a an opt-in feature, so what it does is less
relevant to the issue at point, IMO.

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

[Juri Linkov]

>> >> minibuffer-previous-* are intended to navigate the completion list
>> >> horizontally while the minibuffer is active, and minibuffer-previous-line-*
>> >> vertically.
>> >
>> > Then the names could be simplified as minibuffer-up-line and
>> > minibuffer-down-line.  It will also make the names less confusing, IMO.
>>
>> It's not only about the minibuffer, but also about completions,
>> so the commands names should also include the word "completion".
>
> minibuffer-up-completions-line is still shorter and less confusing.

'minibuffer-previous-line-completion' is named after 'previous-line'
that is not named 'up-line'.  There are 'left-char', 'left-word', etc.
only in horizontal direction, but no 'up-line' in vertical direction.
So it makes no sense to propagate this inconsistency to completion
command names where there are only 'previous-completion', but no
'left-completion'.

>> It does the same as icomplete-vertical-mode where the arrow keys
>> are not confusing.
>
> icomplete-vertical-mode is a an opt-in feature, so what it does is less
> relevant to the issue at point, IMO.

The proposed feature is also opt-in.

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 256 bytes --]

> In a multi-column layout, the keys <left> and <right>
> wrap to the beginning/end of the completions buffer,
> but <up> and <down> don't.  Here is a patch that supports
> completion-auto-wrap for wrapping to the top/bottom:

Ok, here is a better patch:


[-- Attachment #2: next-line-completion.patch --]
[-- Type: text/x-diff, Size: 5691 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index 9c0f28e3fa9..cabdfe50a19 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -999,8 +999,15 @@ A major mode based on the tree-sitter library for editing Elixir files.
 *** New major mode 'lua-ts-mode'.
 A major mode based on the tree-sitter library for editing Lua files.
 
+** Minibuffer and Completions
+
+*** New commands 'previous-line-completion' and 'next-line-completion'.
+Bound to [up] and [down] respectively, they navigate the *Completions*
+buffer vertically, wrapping at the top/bottom when 'completion-auto-wrap'
+is non-nil.
+
 +++
-** New global minor mode 'minibuffer-regexp-mode'.
+*** New global minor mode 'minibuffer-regexp-mode'.
 This is a minor mode for editing regular expressions in the minibuffer.
 It highlights parens via ‘show-paren-mode’ and ‘blink-matching-paren’ in
 a user-friendly way, avoids reporting alleged paren mismatches and makes
diff --git a/lisp/simple.el b/lisp/simple.el
index ec14bec9e07..b0782d1f8ae 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9820,6 +9827,8 @@ completion-list-mode-map
     (define-key map "\C-m" 'choose-completion)
     (define-key map "\e\e\e" 'delete-completion-window)
     (define-key map [remap keyboard-quit] #'delete-completion-window)
+    (define-key map [up] 'previous-line-completion)
+    (define-key map [down] 'next-line-completion)
     (define-key map [left] 'previous-completion)
     (define-key map [right] 'next-completion)
     (define-key map [?\t] 'next-completion)
@@ -9882,7 +9891,8 @@ delete-completion-window
 
 (defcustom completion-auto-wrap t
   "Non-nil means to wrap around when selecting completion options.
-This affects the commands `next-completion' and `previous-completion'.
+This affects the commands `next-completion', `previous-completion',
+`next-line-completion' and `previous-line-completion'.
 When `completion-auto-select' is t, it wraps through the minibuffer
 for the commands bound to the TAB key."
   :type 'boolean
@@ -10000,6 +10010,87 @@ next-completion
     (when (/= 0 n)
       (switch-to-minibuffer))))
 
+(defun previous-line-completion (&optional n)
+  "Move to the item on the previous line in the completion list.
+With prefix argument N, move back N items line-wise (negative N
+means move forward).
+
+Also see the `completion-auto-wrap' variable."
+  (interactive "p")
+  (next-line-completion (- n)))
+
+(defun next-line-completion (&optional n)
+  "Move to the item on the next line in the completion list.
+With prefix argument N, move N items line-wise (negative N
+means move backward).
+
+Also see the `completion-auto-wrap' variable."
+  (interactive "p")
+  (let (line column pos)
+    (when (and (bobp)
+               (> n 0)
+               (get-text-property (point) 'mouse-face)
+               (not (get-text-property (point) 'first-completion)))
+      (let ((inhibit-read-only t))
+        (add-text-properties (point) (1+ (point)) '(first-completion t)))
+      (setq n (1- n)))
+
+    (if (get-text-property (point) 'mouse-face)
+        ;; If in a completion, move to the start of it.
+        (when (and (not (bobp))
+                   (get-text-property (1- (point)) 'mouse-face))
+          (goto-char (previous-single-property-change (point) 'mouse-face)))
+      ;; Try to move to the previous completion.
+      (setq pos (previous-single-property-change (point) 'mouse-face))
+      (if pos
+          ;; Move to the start of the previous completion.
+          (progn
+            (goto-char pos)
+            (unless (get-text-property (point) 'mouse-face)
+              (goto-char (previous-single-property-change
+                          (point) 'mouse-face nil (point-min)))))
+        (cond ((> n 0) (setq n (1- n)) (first-completion))
+              ((< n 0) (first-completion)))))
+
+    (while (> n 0)
+      (setq pos nil column (current-column) line (line-number-at-pos))
+      (when (and (or (not (eq (forward-line 1) 0))
+                     (eobp)
+                     (not (eq (move-to-column column) column))
+                     (not (get-text-property (point) 'mouse-face)))
+                 completion-auto-wrap)
+        (save-excursion
+          (goto-char (point-min))
+          (when (and (eq (move-to-column column) column)
+                     (get-text-property (point) 'mouse-face))
+            (setq pos (point)))
+          (while (and (not pos) (> line (line-number-at-pos)))
+            (forward-line 1)
+            (when (and (eq (move-to-column column) column)
+                       (get-text-property (point) 'mouse-face))
+              (setq pos (point)))))
+        (if pos (goto-char pos)))
+      (setq n (1- n)))
+
+    (while (< n 0)
+      (setq pos nil column (current-column) line (line-number-at-pos))
+      (when (and (or (not (eq (forward-line -1) 0))
+                     (not (eq (move-to-column column) column))
+                     (not (get-text-property (point) 'mouse-face)))
+                 completion-auto-wrap)
+        (save-excursion
+          (goto-char (point-max))
+          (when (and (eq (move-to-column column) column)
+                     (get-text-property (point) 'mouse-face))
+            (setq pos (point)))
+          (while (and (not pos) (< line (line-number-at-pos)))
+            (forward-line -1)
+            (when (and (eq (move-to-column column) column)
+                       (get-text-property (point) 'mouse-face))
+              (setq pos (point)))))
+        (if pos (goto-char pos)))
+      (setq n (1+ n)))))
+
 (defun choose-completion (&optional event no-exit no-quit)
   "Choose the completion at point.
 If EVENT, use EVENT's position to determine the starting position.

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

>> In a multi-column layout, the keys <left> and <right>
>> wrap to the beginning/end of the completions buffer,
>> but <up> and <down> don't.  Here is a patch that supports
>> completion-auto-wrap for wrapping to the top/bottom:
>
> Ok, here is a better patch:

Pushed to master.

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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: minibuffer-completion-visible.patch --]
[-- Type: text/x-diff, Size: 4743 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 2120e31775e..2601e705aa0 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2707,8 +2716,14 @@ completion-in-region-mode
 	  completion-in-region-mode-predicate)
     (setq-local minibuffer-completion-auto-choose nil)
     (add-hook 'post-command-hook #'completion-in-region--postch)
-    (push `(completion-in-region-mode . ,completion-in-region-mode-map)
-          minor-mode-overriding-map-alist)))
+    (let* ((keymap completion-in-region-mode-map)
+           (keymap (if minibuffer-completion-visible
+                       (make-composed-keymap
+                        (list minibuffer-local-visible-completion-map
+                              keymap))
+                     keymap)))
+      (push `(completion-in-region-mode . ,keymap)
+            minor-mode-overriding-map-alist))))
 
 ;; Define-minor-mode added our keymap to minor-mode-map-alist, but we want it
 ;; on minor-mode-overriding-map-alist instead.
@@ -2953,7 +2972,32 @@ minibuffer-mode
   :interactive nil
   ;; Enable text conversion, but always make sure `RET' does
   ;; something.
-  (setq text-conversion-style 'action))
+  (setq text-conversion-style 'action)
+  (when minibuffer-completion-visible
+    (setq-local minibuffer-completion-auto-choose nil)))
+
+(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."
+  :type 'boolean
+  :version "30.1")
+
+(defun minibuffer-bind-visible (binding)
+  `(menu-item
+    "" ,binding
+    :filter ,(lambda (cmd)
+               (when (get-buffer-window "*Completions*" 0)
+                 cmd))))
+
+(defvar-keymap minibuffer-local-visible-completion-map
+  :doc "Local keymap for minibuffer input with visible completions."
+  "<left>"  (minibuffer-bind-visible #'minibuffer-previous-completion)
+  "<right>" (minibuffer-bind-visible #'minibuffer-next-completion)
+  "<up>"    (minibuffer-bind-visible #'minibuffer-previous-line-completion)
+  "<down>"  (minibuffer-bind-visible #'minibuffer-next-line-completion)
+  "RET"     (minibuffer-bind-visible #'minibuffer-choose-completion)
+  "C-g"     (minibuffer-bind-visible #'minibuffer-hide-completions))
 
 ;;; Completion tables.
 
@@ -4370,6 +4414,11 @@ completing-read-default
                     ;; in minibuffer-local-filename-completion-map can
                     ;; override bindings in base-keymap.
                     base-keymap)))
+         (keymap (if minibuffer-completion-visible
+                     (make-composed-keymap
+                      (list minibuffer-local-visible-completion-map
+                            keymap))
+                   keymap))
          (buffer (current-buffer))
          (c-i-c completion-ignore-case)
          (result
@@ -4489,8 +4538,9 @@ minibuffer-completion-auto-choose
   :type 'boolean
   :version "29.1")
 
-(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.
 When `minibuffer-completion-auto-choose' is non-nil, then also
 insert the selected completion to the minibuffer."
   (interactive "p")
@@ -4498,7 +4548,9 @@ minibuffer-next-completion
     (with-minibuffer-completions-window
       (when completions-highlight-face
         (setq-local cursor-face-highlight-nonselected-window t))
-      (next-completion (or n 1))
+      (if vertical
+          (next-line-completion (or n 1))
+        (next-completion (or n 1)))
       (when auto-choose
         (let ((completion-use-base-affixes t))
           (choose-completion nil t t))))))
@@ -4510,6 +4562,20 @@ minibuffer-previous-completion
   (interactive "p")
   (minibuffer-next-completion (- (or n 1))))
 
+(defun minibuffer-next-line-completion (&optional n)
+  "Move to the next completion line from the minibuffer.
+When `minibuffer-completion-auto-choose' is non-nil, then also
+insert the selected completion to the minibuffer."
+  (interactive "p")
+  (minibuffer-next-completion (or n 1) t))
+
+(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."
+  (interactive "p")
+  (minibuffer-next-completion (- (or n 1)) t))
+
 (defun minibuffer-choose-completion (&optional no-exit no-quit)
   "Run `choose-completion' from the minibuffer in its completions window.
 With prefix argument NO-EXIT, insert the completion at point to the

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

[Eli Zaretskii]

> 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.

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 3328 bytes --]

>> +(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.

Now improved in a new patch.

> 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 the *Completions* buffer is not in any window, the arrow keys
move point in the minibuffer.  Now explained this in the docstring.

>> +(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.

Done.

> 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".

(get-buffer-window "*Completions*" 0) is used everywhere in minibuffer.el
and simple.el, so the argument zero is for compatibility with other
minibuffer completion commands.

>> +  "RET"     (minibuffer-bind-visible #'minibuffer-choose-completion)
>
> AFAICT, this important binding is not mentioned or hinted in the doc
> string of the new option.

Now mentioned.

>> -(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?

The next item can be on the axis x or y.

> 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).

Ok, improved with a link.

>> +(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.

This line is too long and doesn't mention the minibuffer.
So I added it to the rest of the docstring.

>> +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.

Ok, fixed.

>> +(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.

Here is a new patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: minibuffer-visible-completions.patch --]
[-- Type: text/x-diff, Size: 6692 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 2120e31775e..56ba7e235f2 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2707,8 +2716,14 @@ completion-in-region-mode
 	  completion-in-region-mode-predicate)
     (setq-local minibuffer-completion-auto-choose nil)
     (add-hook 'post-command-hook #'completion-in-region--postch)
-    (push `(completion-in-region-mode . ,completion-in-region-mode-map)
-          minor-mode-overriding-map-alist)))
+    (let* ((keymap completion-in-region-mode-map)
+           (keymap (if minibuffer-visible-completions
+                       (make-composed-keymap
+                        (list minibuffer-visible-completions-map
+                              keymap))
+                     keymap)))
+      (push `(completion-in-region-mode . ,keymap)
+            minor-mode-overriding-map-alist))))
 
 ;; Define-minor-mode added our keymap to minor-mode-map-alist, but we want it
 ;; on minor-mode-overriding-map-alist instead.
@@ -2953,7 +2972,41 @@ minibuffer-mode
   :interactive nil
   ;; Enable text conversion, but always make sure `RET' does
   ;; something.
-  (setq text-conversion-style 'action))
+  (setq text-conversion-style 'action)
+  (when minibuffer-visible-completions
+    (setq-local minibuffer-completion-auto-choose nil)))
+
+(defcustom minibuffer-visible-completions t
+  "When non-nil, visible completions can be navigated from the minibuffer.
+This means that when the *Completions* buffer is visible in a window,
+then you can use the arrow keys in the minibuffer to move the cursor
+in the *Completions* buffer.  Then you can type `RET',
+and the candidate highlighted the *Completions* buffer
+will be accepted.
+But when the *Completions* buffer is not displayed on the screen,
+then the arrow keys move point in the minibuffer as usual, and
+`RET' accepts the input typed in the minibuffer."
+  :type 'boolean
+  :version "30.1")
+
+(defun minibuffer-visible-completions-bind (binding)
+  "Use BINDING when completions are visible.
+Return an item that is enabled only when a window
+displaying the *Completions* buffer exists."
+  `(menu-item
+    "" ,binding
+    :filter ,(lambda (cmd)
+               (when (get-buffer-window "*Completions*" 0)
+                 cmd))))
+
+(defvar-keymap minibuffer-visible-completions-map
+  :doc "Local keymap for minibuffer input with visible completions."
+  "<left>"  (minibuffer-visible-completions-bind #'minibuffer-previous-completion)
+  "<right>" (minibuffer-visible-completions-bind #'minibuffer-next-completion)
+  "<up>"    (minibuffer-visible-completions-bind #'minibuffer-previous-line-completion)
+  "<down>"  (minibuffer-visible-completions-bind #'minibuffer-next-line-completion)
+  "RET"     (minibuffer-visible-completions-bind #'minibuffer-choose-completion)
+  "C-g"     (minibuffer-visible-completions-bind #'minibuffer-hide-completions))
 
 ;;; Completion tables.
 
@@ -4370,6 +4423,11 @@ completing-read-default
                     ;; in minibuffer-local-filename-completion-map can
                     ;; override bindings in base-keymap.
                     base-keymap)))
+         (keymap (if minibuffer-visible-completions
+                     (make-composed-keymap
+                      (list minibuffer-visible-completions-map
+                            keymap))
+                   keymap))
          (buffer (current-buffer))
          (c-i-c completion-ignore-case)
          (result
@@ -4489,16 +4547,21 @@ minibuffer-completion-auto-choose
   :type 'boolean
   :version "29.1")
 
-(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
+to the next item on the next line using `next-line-completion'.
+Otherwise, move to the next item horizontally using `next-completion'.
 When `minibuffer-completion-auto-choose' is non-nil, then also
-insert the selected completion to the minibuffer."
+insert the selected completion candidate to the minibuffer."
   (interactive "p")
   (let ((auto-choose minibuffer-completion-auto-choose))
     (with-minibuffer-completions-window
       (when completions-highlight-face
         (setq-local cursor-face-highlight-nonselected-window t))
-      (next-completion (or n 1))
+      (if vertical
+          (next-line-completion (or n 1))
+        (next-completion (or n 1)))
       (when auto-choose
         (let ((completion-use-base-affixes t))
           (choose-completion nil t t))))))
@@ -4506,17 +4569,35 @@ minibuffer-next-completion
 (defun minibuffer-previous-completion (&optional n)
   "Move to the previous item in its completions window from the minibuffer.
 When `minibuffer-completion-auto-choose' is non-nil, then also
-insert the selected completion to the minibuffer."
+insert the selected completion candidate to the minibuffer."
   (interactive "p")
   (minibuffer-next-completion (- (or n 1))))
 
+(defun minibuffer-next-line-completion (&optional n)
+  "Move to the next completion line from the minibuffer.
+This means to move to the completion candidate on the next line
+in the *Completions* buffer while point stays in the minibuffer.
+When `minibuffer-completion-auto-choose' is non-nil, then also
+insert the selected completion candidate to the minibuffer."
+  (interactive "p")
+  (minibuffer-next-completion (or n 1) t))
+
+(defun minibuffer-previous-line-completion (&optional n)
+  "Move to the previous completion line from the minibuffer.
+This means to move to the completion candidate on the previous line
+in the *Completions* buffer while point stays in the minibuffer.
+When `minibuffer-completion-auto-choose' is non-nil, then also
+insert the selected completion candidate to the minibuffer."
+  (interactive "p")
+  (minibuffer-next-completion (- (or n 1)) t))
+
 (defun minibuffer-choose-completion (&optional no-exit no-quit)
   "Run `choose-completion' from the minibuffer in its completions window.
-With prefix argument NO-EXIT, insert the completion at point to the
-minibuffer, but don't exit the minibuffer.  When the prefix argument
+With prefix argument NO-EXIT, insert the completion candidate at point to
+the minibuffer, but don't exit the minibuffer.  When the prefix argument
 is not provided, then whether to exit the minibuffer depends on the value
 of `completion-no-auto-exit'.
-If NO-QUIT is non-nil, insert the completion at point to the
+If NO-QUIT is non-nil, insert the completion candidate at point to the
 minibuffer, but don't quit the completions window."
   (interactive "P")
     (with-minibuffer-completions-window

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

[Juri Linkov]

>> 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".
>
> (get-buffer-window "*Completions*" 0) is used everywhere in minibuffer.el
> and simple.el, so the argument zero is for compatibility with other
> minibuffer completion commands.

During testing I discovered that the condition should be more complex.
The problem is that the logic should be bound only to the minibuffer
that showed the completions, not in other minibuffers within a set of
recursive minibuffers.  An example: `M-x TAB C-h v down RET'
raised an error "Minibuffer is not active for completion".
This error comes from `choose-completion-string',
so I copied the same logic from `choose-completion-string':

  (when-let ((window (get-buffer-window "*Completions*" 0)))
    (when (eq (buffer-local-value 'completion-reference-buffer
                                  (window-buffer window))
              (window-buffer (active-minibuffer-window)))
      cmd))

>> Same here.
>
> Here is a new patch:

Now pushed the fixed patch.

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

[Juri Linkov]

close 59486 30.0.50
thanks

>> Here is a new patch:
>
> Now pushed the fixed patch.

Looks like everything is done here, so closing.