bug#68801: 30.0.50; minibuffer-visible-completions=t makes RET in completion-in-region a no-op with nothing selected

[Spencer Baugh]

Spencer Baugh <sbaugh@janestreet.com> writes:
> I'm thinking the right thing to do is just to go with my original patch
> which only binds RET when there's a selected completion.  That's simple
> and straightforward.
>
> But I'm not sure how to adapt my completion-show-help change for that: I
> want to hint to the user that they should hit RET, but RET is only bound
> when a completion is selected, which isn't the case when *Completions*
> is first displayed.  Any ideas?  Could we just hardcode "RET" in the
> help text?

The solution is obvious in retrospect, just add a defvar to force the
bindings on.

Complete patch attached, I think this closes the bug.

>From e81a3dccacd1ba701361d84f6d61fb244e8b81d6 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 27 Feb 2024 15:42:38 -0500
Subject: [PATCH] With visible-completions, only bind RET when completion is
 selected

Previously, if minibuffer-visible-completions was non-nil, we bound RET
whenever the *Completions* buffer was visible.  This meant that RET in
completion-in-region would not enter a newline, which is a somewhat
annoying behavior change from minibuffer-visible-completions=nil.

Now, we only bind RET when a completion is selected.  This means
RET will newline in completion-in-region.

So that completion help continues to suggest the correct keys,
we also add minibuffer-visible-completions--always-bind.  When
let-bound to a non-nil value, it makes the
minibuffer-visible-completions binds always active.  We let-bind
it around substitute-command-keys.

* lisp/minibuffer.el (minibuffer-visible-completions--always-bind)
(minibuffer-visible-completions--filter): Add.
(minibuffer-visible-completions-bind): Use
minibuffer-visible-completions--filter.  (bug#68801)
* lisp/simple.el (minibuffer-visible-completions--always-bind)
(completion-setup-function): Let-bind
minibuffer-visible-completions--always-bind so the completion
help is correct.
---
 lisp/minibuffer.el | 24 ++++++++++++++++++------
 lisp/simple.el     | 19 +++++++++++--------
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 708f3684d11..3a06baddeae 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3163,18 +3163,30 @@ minibuffer-visible-completions
   :type 'boolean
   :version "30.1")
 
+(defvar minibuffer-visible-completions--always-bind nil
+  "If non-nil, force the `minibuffer-visible-completions' bindings on.")
+
+(defun minibuffer-visible-completions--filter (cmd)
+  "Return CMD if `minibuffer-visible-completions' bindings should be active."
+  (if minibuffer-visible-completions--always-bind
+      cmd
+    (when-let ((window (get-buffer-window "*Completions*" 0)))
+      (when (and (eq (buffer-local-value 'completion-reference-buffer
+                                         (window-buffer window))
+                     (window-buffer (active-minibuffer-window)))
+                 (if (eq cmd #'minibuffer-choose-completion-or-exit)
+                     (with-current-buffer (window-buffer window)
+                       (get-text-property (point) 'completion--string))
+                   t))
+        cmd))))
+
 (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-let ((window (get-buffer-window "*Completions*" 0)))
-                 (when (eq (buffer-local-value 'completion-reference-buffer
-                                               (window-buffer window))
-                           (window-buffer (active-minibuffer-window)))
-                   cmd)))))
+    :filter ,#'minibuffer-visible-completions--filter))
 
 (defvar-keymap minibuffer-visible-completions-map
   :doc "Local keymap for minibuffer input with visible completions."
diff --git a/lisp/simple.el b/lisp/simple.el
index 9a33049f4ca..ac2e1f9f1c9 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10298,6 +10298,8 @@ completion-show-help
   :version "22.1"
   :group 'completion)
 
+(defvar minibuffer-visible-completions--always-bind)
+
 ;; This function goes in completion-setup-hook, so that it is called
 ;; after the text of the completion list buffer is written.
 (defun completion-setup-function ()
@@ -10338,15 +10340,16 @@ completion-setup-function
         (if minibuffer-visible-completions
             (let ((helps
                    (with-current-buffer (window-buffer 
(active-minibuffer-window))
-                     (list
-                      (substitute-command-keys
-                      (if (display-mouse-p)
-                          "Click or type 
\\[minibuffer-choose-completion-or-exit] on a completion to select it.\n"
-                         "Type \\[minibuffer-choose-completion-or-exit] on a 
completion to select it.\n"))
-                      (substitute-command-keys
-                      "Type \\[minibuffer-next-completion], 
\\[minibuffer-previous-completion], \
+                     (let ((minibuffer-visible-completions--always-bind t))
+                       (list
+                        (substitute-command-keys
+                        (if (display-mouse-p)
+                            "Click or type 
\\[minibuffer-choose-completion-or-exit] on a completion to select it.\n"
+                           "Type \\[minibuffer-choose-completion-or-exit] on a 
completion to select it.\n"))
+                        (substitute-command-keys
+                        "Type \\[minibuffer-next-completion], 
\\[minibuffer-previous-completion], \
 \\[minibuffer-next-line-completion], \\[minibuffer-previous-line-completion] \
-to move point between completions.\n\n")))))
+to move point between completions.\n\n"))))))
               (dolist (help helps)
                 (insert help)))
           (insert (substitute-command-keys
-- 
2.39.3