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

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

[Spencer Baugh]

1. emacs -Q
2. In scratch:
(current- M-<tab>
3. Observe *Completions* pops up.
4. RET
5. *Completions* goes away and a newline is inserted.

Compare:

1. emacs -Q --eval '(setq minibuffer-visible-completions t)'
2. In scratch:
(current- M-<tab>
3. Observe *Completions* pops up.
4. RET
5. *Completions* goes away but no newline is inserted, and "No match" is
   printed.

I think the behavior with minibuffer-visible-completions=t is
unintuitive.  And it just duplicates what you can achieve with C-g
(closes *Completions*), so it's not very useful.

Even if I do:
(current- M-<tab> buf RET

which is sufficient to uniquely identify current-buffer, RET does not
complete to current-buffer, instead it just says "No match".

With minibuffer-visible-completions=t, I suggest RET with no completion
selected in completion-in-region should exit completion and insert a
newline.

Or maybe, slightly more radically, RET with no completion selected in
completion-in-region should select the first matching completion given
the current completion region?  At least one user expected that
behavior.

Another alternative is that RET doesn't close *Completions* at all,
which would match minibuffer behavior better.  I'm not a huge fan of
that, but it's more useful than the current behavior of RET.

Since there are a few alternatives, perhaps we could have a defcustom
for the RET behavior in completion-in-region.  I think the default
should be closing *Completions* and inserting a newline, since that
matches minibuffer-visible-completions=nil.

If this sounds reasonable I can write a patch implementing these.


In GNU Emacs 30.0.50 (build 24, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2024-01-29 built on
 igm-qws-u22796a Repository revision:
 cbc8bdc1386a4bc9c420d8ab06b844c6f6928c35 Repository branch: master
 Windowing system distributor 'The X.Org Foundation', version
 11.0.12011000 System Description: Rocky Linux 8.9 (Green Obsidian)

Configured using:
 'configure -C 'CFLAGS=-O0 -g3' --with-gif=ifavailable
 --with-x-toolkit=lucid'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM
XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr dabbrev emacsbug message mailcap yank-media puny
dired dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg
rfc6068 epg-config gnus-util text-property-search time-date subr-x
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren
electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd touch-screen tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
xinput2 x multi-tty move-toolbar make-network-process emacs)

Memory information:
((conses 16 65716 19738) (symbols 48 9598 0) (strings 32 22938 2513)
 (string-bytes 1 684815) (vectors 16 9358)
 (vector-slots 8 112024 10425) (floats 8 42 15) (intervals 56 264 0)
 (buffers 976 10))

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

[Juri Linkov]

> Since there are a few alternatives, perhaps we could have a defcustom
> for the RET behavior in completion-in-region.  I think the default
> should be closing *Completions* and inserting a newline, since that
> matches minibuffer-visible-completions=nil.
>
> If this sounds reasonable I can write a patch implementing these.

Sorry, I have no opinion - this is such a gray area, I never tried
to type RET with an unselected completion candidate.

But at least the current behavior makes sense since it's like
in Isearch mode where RET just exits Isearch, whereas some other
keys exit Isearch and do their usual job.

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

[Spencer Baugh]

Juri Linkov <juri@linkov.net> writes:
>> Since there are a few alternatives, perhaps we could have a defcustom
>> for the RET behavior in completion-in-region.  I think the default
>> should be closing *Completions* and inserting a newline, since that
>> matches minibuffer-visible-completions=nil.
>>
>> If this sounds reasonable I can write a patch implementing these.
>
> Sorry, I have no opinion - this is such a gray area, I never tried
> to type RET with an unselected completion candidate.
>
> But at least the current behavior makes sense since it's like
> in Isearch mode where RET just exits Isearch, whereas some other
> keys exit Isearch and do their usual job.

Fair.  So that may make sense as an optional behavior.

Still, I think the behavior of completion-in-region with
minibuffer-visible-completions=nil is more relevant, and the default
should match that.

Here's a simple patch: What if minibuffer-visible-completions just only
overrides RET when there's a selected completion?  See below.  Could
this make sense?

In general, now RET with minibuffer-visible-completions=t will do
whatever RET did when minibuffer-visible-completions=nil, except that
when there's a selected completion RET will choose it.  That seems
simple and easy to understand to me.

(Really, I think something like this behavior for RET could be made the
default, even when minibuffer-visible-completions=nil)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 45aab398078..688018fd07f 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3161,7 +3161,7 @@ minibuffer-visible-completions
   :type 'boolean
   :version "30.1")
 
-(defun minibuffer-visible-completions-bind (binding)
+(defun minibuffer-visible-completions-bind (binding &optional require-selected)
   "Use BINDING when completions are visible.
 Return an item that is enabled only when a window
 displaying the *Completions* buffer exists."
@@ -3169,9 +3169,13 @@ minibuffer-visible-completions-bind
     "" ,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)))
+                 (when (and (eq (buffer-local-value 'completion-reference-buffer
+                                                    (window-buffer window))
+                                (window-buffer (active-minibuffer-window)))
+                            (if require-selected
+                                (with-current-buffer (window-buffer window)
+                                  (get-text-property (point) 'completion--string))
+                              t))
                    cmd)))))
 
 (defvar-keymap minibuffer-visible-completions-map
@@ -3180,7 +3184,7 @@ minibuffer-visible-completions-map
   "<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-or-exit)
+  "RET"     (minibuffer-visible-completions-bind #'minibuffer-choose-completion-or-exit t)
   "C-g"     (minibuffer-visible-completions-bind #'minibuffer-hide-completions))
 \f
 ;;; Completion tables.

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

[Juri Linkov]

> Here's a simple patch: What if minibuffer-visible-completions just only
> overrides RET when there's a selected completion?  See below.  Could
> this make sense?

Yes, this makes sense.  I'll try your patch for a while.

> (Really, I think something like this behavior for RET could be made the
> default, even when minibuffer-visible-completions=nil)

I don't understand what would this mean for minibuffer-visible-completions=nil.

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

[Juri Linkov]

>> Here's a simple patch: What if minibuffer-visible-completions just only
>> overrides RET when there's a selected completion?  See below.  Could
>> this make sense?
>
> Yes, this makes sense.  I'll try your patch for a while.

Thanks for the patch, it works nicely.

Could you please write a commit message for this patch.

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

[sbaugh]

Juri Linkov <juri@linkov.net> writes:
>>> Here's a simple patch: What if minibuffer-visible-completions just only
>>> overrides RET when there's a selected completion?  See below.  Could
>>> this make sense?
>>
>> Yes, this makes sense.  I'll try your patch for a while.
>
> Thanks for the patch, it works nicely.
>
> Could you please write a commit message for this patch.

The annoying thing is that this breaks the enhancement to
completion-show-help I recently made.  When a completion is not
selected, RET isn't bound, so the help shows "Click or type M-x
minibuffer-choose-completion-or-exit on a completion to select it."

I'm not sure the right way to fix that.  Selectively binding RET based
on whether a completion is selected feels sketchy anyway, it may confuse
users because e.g. C-h c RET won't work.

Maybe instead we should always bind RET, but if no completion is
selected, we run the command that RET was bound to before
completion-in-region-mode started?

Alternatively... as a completely separate point, I'd like
completion-in-region to select the first completion candidate by
default.  I think that's useful in some cases and, for
completion-in-region, doesn't have any negatives: we couldn't do it in
the minibuffer because it would interfere with accepting the default,
but there are no defaults in completion-in-region.

If we make c-i-r select the first completion candidate by default, that
would both:

- Make the completion-show-help help render correctly with the "only
  override RET when there's a selected completion" patch.

- Partially mitigate the RET issue all on its own

Wdyt?

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

[Juri Linkov]

> The annoying thing is that this breaks the enhancement to
> completion-show-help I recently made.  When a completion is not
> selected, RET isn't bound, so the help shows "Click or type M-x
> minibuffer-choose-completion-or-exit on a completion to select it."
>
> I'm not sure the right way to fix that.  Selectively binding RET based
> on whether a completion is selected feels sketchy anyway, it may confuse
> users because e.g. C-h c RET won't work.
>
> Maybe instead we should always bind RET, but if no completion is
> selected, we run the command that RET was bound to before
> completion-in-region-mode started?

Then 'minibuffer-choose-completion-or-exit' could be more smart
to run whatever command was bound to RET initially, instead of
using the hard-coded 'minibuffer-complete-and-exit'.
But this might require juggling with keymaps.

> Alternatively... as a completely separate point, I'd like
> completion-in-region to select the first completion candidate by
> default.  I think that's useful in some cases and, for
> completion-in-region, doesn't have any negatives: we couldn't do it in
> the minibuffer because it would interfere with accepting the default,
> but there are no defaults in completion-in-region.
>
> If we make c-i-r select the first completion candidate by default, that
> would both:
>
> - Make the completion-show-help help render correctly with the "only
>   override RET when there's a selected completion" patch.
>
> - Partially mitigate the RET issue all on its own

Calling 'first-completion' makes sense even for the minibuffer,
at least optionally.

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

[sbaugh]

Juri Linkov <juri@linkov.net> writes:

>> The annoying thing is that this breaks the enhancement to
>> completion-show-help I recently made.  When a completion is not
>> selected, RET isn't bound, so the help shows "Click or type M-x
>> minibuffer-choose-completion-or-exit on a completion to select it."
>>
>> I'm not sure the right way to fix that.  Selectively binding RET based
>> on whether a completion is selected feels sketchy anyway, it may confuse
>> users because e.g. C-h c RET won't work.
>>
>> Maybe instead we should always bind RET, but if no completion is
>> selected, we run the command that RET was bound to before
>> completion-in-region-mode started?
>
> Then 'minibuffer-choose-completion-or-exit' could be more smart
> to run whatever command was bound to RET initially, instead of
> using the hard-coded 'minibuffer-complete-and-exit'.
> But this might require juggling with keymaps.

Indeed, tricky.  And since minibuffer-choose-completion-or-exit could be
bound to other keys, not just RET, we shouldn't hardcode it to run what
was bound to RET, too - it should run whatever it replaced.

Maybe if there's no completion selected, it could push this-command-keys
back on to unread-command-events, then disable
completion-in-region-mode?  Then Emacs would just do key lookup again
for us.

>> Alternatively... as a completely separate point, I'd like
>> completion-in-region to select the first completion candidate by
>> default.  I think that's useful in some cases and, for
>> completion-in-region, doesn't have any negatives: we couldn't do it in
>> the minibuffer because it would interfere with accepting the default,
>> but there are no defaults in completion-in-region.
>>
>> If we make c-i-r select the first completion candidate by default, that
>> would both:
>>
>> - Make the completion-show-help help render correctly with the "only
>>   override RET when there's a selected completion" patch.
>>
>> - Partially mitigate the RET issue all on its own
>
> Calling 'first-completion' makes sense even for the minibuffer,
> at least optionally.

What about this simple patch?

If minibuffer-visible-completions=nil, it just calls first-completion.
This doesn't meaningfully change anything, since previously M-RET would
just no-op in that situation.  And it's actually quite useful, because
it makes M-RET useful without having to ever actually do M-<down>; one
can just type text to narrow the completions and then hit M-RET.  Which
is sometimes nice.

If minibuffer-visible-completions=t, it calls first-completion only when
there's no minibuffer-default; that way a simple RET will still select
the minibuffer default.

diff --git a/lisp/simple.el b/lisp/simple.el
index 8246b9cab81..715ab672655 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10355,7 +10355,10 @@ completion-setup-function
                      "Type \\[minibuffer-choose-completion] on a completion to select it.\n")))
           (insert (substitute-command-keys
 		   "Type \\[minibuffer-next-completion] or \\[minibuffer-previous-completion] \
-to move point between completions.\n\n")))))))
+to move point between completions.\n\n"))))
+      (unless (and minibuffer-visible-completions minibuffer-default)
+        (with-minibuffer-completions-window
+          (first-completion))))))
 
 (add-hook 'completion-setup-hook #'completion-setup-function)
 

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

[Spencer Baugh]

sbaugh@catern.com writes:
>>> Alternatively... as a completely separate point, I'd like
>>> completion-in-region to select the first completion candidate by
>>> default.  I think that's useful in some cases and, for
>>> completion-in-region, doesn't have any negatives: we couldn't do it in
>>> the minibuffer because it would interfere with accepting the default,
>>> but there are no defaults in completion-in-region.
>>>
>>> If we make c-i-r select the first completion candidate by default, that
>>> would both:
>>>
>>> - Make the completion-show-help help render correctly with the "only
>>>   override RET when there's a selected completion" patch.
>>>
>>> - Partially mitigate the RET issue all on its own
>>
>> Calling 'first-completion' makes sense even for the minibuffer,
>> at least optionally.
>
> What about this simple patch?
>
> If minibuffer-visible-completions=nil, it just calls first-completion.
> This doesn't meaningfully change anything, since previously M-RET would
> just no-op in that situation.  And it's actually quite useful, because
> it makes M-RET useful without having to ever actually do M-<down>; one
> can just type text to narrow the completions and then hit M-RET.  Which
> is sometimes nice.
>
> If minibuffer-visible-completions=t, it calls first-completion only when
> there's no minibuffer-default; that way a simple RET will still select
> the minibuffer default.
>
> diff --git a/lisp/simple.el b/lisp/simple.el
> index 8246b9cab81..715ab672655 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -10355,7 +10355,10 @@ completion-setup-function
>                       "Type \\[minibuffer-choose-completion] on a completion to select it.\n")))
>            (insert (substitute-command-keys
>  		   "Type \\[minibuffer-next-completion] or \\[minibuffer-previous-completion] \
> -to move point between completions.\n\n")))))))
> +to move point between completions.\n\n"))))
> +      (unless (and minibuffer-visible-completions minibuffer-default)
> +        (with-minibuffer-completions-window
> +          (first-completion))))))
>  
>  (add-hook 'completion-setup-hook #'completion-setup-function)
>  

I found selecting first-completion in the minibuffer to be annoying, so
now I'm trying with the following instead (which also fixes the
highlight to actually be shown):

(defun minibuffer-cir-first-completion ()
  "Move point to the first completion in the *Completions* window."
  (when completion-in-region-mode
    (with-selected-window (get-buffer-window standard-output 0)
      (when completions-highlight-face
        (setq-local cursor-face-highlight-nonselected-window t))
      (first-completion))))
(add-hook 'completion-setup-hook #'minibuffer-cir-first-completion 90)

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

[Juri Linkov]

> I found selecting first-completion in the minibuffer to be annoying, so
> now I'm trying with the following instead (which also fixes the
> highlight to actually be shown):
>
> (defun minibuffer-cir-first-completion ()
>   "Move point to the first completion in the *Completions* window."
>   (when completion-in-region-mode
>     (with-selected-window (get-buffer-window standard-output 0)
>       (when completions-highlight-face
>         (setq-local cursor-face-highlight-nonselected-window t))
>       (first-completion))))
> (add-hook 'completion-setup-hook #'minibuffer-cir-first-completion 90)

I noticed the problem with highlighting in your previous patch too.
Although I'm not sure why 'first-completion' didn't update
the highlighting in your previous patch.  'completion-setup-function'
enables 'completions-highlight':

      ;; Maybe enable cursor completions-highlight.
      (when completions-highlight-face
        (cursor-face-highlight-mode 1))

before you called 'first-completion' at the end of 'completion-setup-function'.

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

[Spencer Baugh]

sbaugh@catern.com writes:
> Juri Linkov <juri@linkov.net> writes:
>>>> Here's a simple patch: What if minibuffer-visible-completions just only
>>>> overrides RET when there's a selected completion?  See below.  Could
>>>> this make sense?
>>>
>>> Yes, this makes sense.  I'll try your patch for a while.
>>
>> Thanks for the patch, it works nicely.
>>
>> Could you please write a commit message for this patch.
>
> The annoying thing is that this breaks the enhancement to
> completion-show-help I recently made.  When a completion is not
> selected, RET isn't bound, so the help shows "Click or type M-x
> minibuffer-choose-completion-or-exit on a completion to select it."
>
> I'm not sure the right way to fix that.  Selectively binding RET based
> on whether a completion is selected feels sketchy anyway, it may confuse
> users because e.g. C-h c RET won't work.
>
> Maybe instead we should always bind RET, but if no completion is
> selected, we run the command that RET was bound to before
> completion-in-region-mode started?
>
> Alternatively... as a completely separate point, I'd like
> completion-in-region to select the first completion candidate by
> default.

Okay, after running with my patch doing this for a while, I don't think
it's a good idea.  It makes cir too different from minibuffer
completion, and it just generally makes things a bit more unpredictable.
Maybe we can think of a way to do it later.

So anyway, this brings me back to wanting RET to insert a newline.  I
tried changing minibuffer-choose-completion-or-exit like so:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 2a8439e03dd..cbf340f1dd6 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4664,14 +4664,18 @@ minibuffer-choose-completion
 
 (defun minibuffer-choose-completion-or-exit (&optional no-exit no-quit)
   "Choose the completion from the minibuffer or exit the minibuffer.
-When `minibuffer-choose-completion' can't find a completion candidate
-in the completions window, then exit the minibuffer using its present
-contents."
+When `minibuffer-choose-completion' can't find a completion
+candidate in the completions window, then hide the completions
+window (which disables `minibuffer-visible-completions') and run
+the command which this key was previously bound to, usually
+exiting the minibuffer using its present contents."
   (interactive "P")
   (condition-case nil
       (let ((choose-completion-deselect-if-after t))
         (minibuffer-choose-completion no-exit no-quit))
-    (error (minibuffer-complete-and-exit))))
+    (error (minibuffer-hide-completions)
+           (mapc (lambda (key) (push key unread-command-events))
+                 (this-command-keys-vector)))))
 
 (defun minibuffer-complete-history ()
   "Complete the minibuffer history as far as possible.


but it doesn't work in a few situations.  Maybe there's a better way to
do it though?

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?

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

[Spencer Baugh]

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

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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-With-visible-completions-only-bind-RET-when-completi.patch --]
[-- Type: text/x-patch, Size: 5249 bytes --]

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

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

[Juri Linkov]

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

Good idea, thanks.  I will test your patch for a while.

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

[Juri Linkov]

close 68801 30.0.50
thanks

>> Complete patch attached, I think this closes the bug.
>
> Good idea, thanks.  I will test your patch for a while.

Thanks for the patch.  Now pushed and closed.

Or could reopen this request if you want also to add here
a new option to auto-select the first completion.