bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Knut Anders Hatlen]

1. Evaluate: (setopt completions-header-format nil completion-show-help nil)

2. Type M-x followed by TAB. The *Completions* buffer pops up and shows
all available commands.

3. Type M-<down>.

Expected result: The first candidate in *Completions* is selected.

Actual result: The second candidate in *Completions* is selected.


In GNU Emacs 29.0.60 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.35, cairo version 1.16.0) of 2022-12-29 built on dell
Repository revision: 909091d7578b7225601b202fb9257dedae879e9a
Repository branch: emacs-29
System Description: Debian GNU/Linux bookworm/sid

Configured using:
 'configure --with-json --with-xml2 --with-modules
 --prefix=/usr/local/stow/emacs --with-pgtk --without-x
 --with-native-compilation --with-tree-sitter'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBOTF LIBSELINUX LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY
PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM GTK3 ZLIB

Important settings:
  value of $LANG: nn_NO.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  flyspell-mode: t
  hl-line-mode: t
  electric-pair-mode: t
  display-line-numbers-mode: t
  elide-head-mode: t
  flymake-mode: t
  winner-mode: t
  windmove-mode: t
  server-mode: t
  which-function-mode: t
  savehist-mode: t
  save-place-mode: t
  repeat-mode: t
  recentf-mode: t
  minibuffer-depth-indicate-mode: t
  marginalia-mode: t
  global-so-long-mode: t
  global-auto-revert-mode: t
  dynamic-completion-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tab-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/kah/.emacs.d/elpa/29/transient-0.3.7/transient hides /usr/local/stow/emacs/share/emacs/29.0.60/lisp/transient

Features:
(shadow sort ecomplete mail-extr emacsbug nndraft nnmh format-spec nnml
utf-7 epa-file network-stream nsm nnfolder nnnil gnus-agent gnus-srvr
gnus-score score-mode nnvirtual gnus-msg nntp gnus-cache gnus-art mm-uu
mml2015 mm-view mml-smime smime gnutls dig gnus-sum shr pixel-fill
kinsoku url-file svg dom gnus-group gnus-undo gnus-start gnus-dbus dbus
xml gnus-cloud nnimap nnmail mail-source utf7 nnoo parse-time iso8601
gnus-spec gnus-int gnus-range message sendmail yank-media puny dired
dired-loaddefs rfc822 mml mml-sec epa derived epg rfc6068 epg-config
mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045
ietf-drums mailabbrev gmm-utils mailheader gnus-win gnus nnheader
gnus-util mail-utils range mm-util mail-prsvr add-log comp comp-cstr
flyspell ispell hl-line elec-pair display-line-numbers elide-head
time-date checkdoc lisp-mnt flymake-proc flymake project compile
text-property-search comint ansi-osc ansi-color warnings thingatpt
cus-edit pp rx winner ring windmove disp-table server icons cl-extra
help-mode which-func imenu savehist saveplace repeat recentf tree-widget
wid-edit mb-depth marginalia magit-autorevert magit-git magit-section
magit-utils crm dash so-long autorevert filenotify completion cus-load
embark-autoloads boxquote-autoloads slime-autoloads marginalia-autoloads
orderless-autoloads info package browse-url url url-proxy url-privacy
url-expand url-methods url-history url-cookie generate-lisp-file
url-domsuf url-util mailcap url-handlers url-parse auth-source cl-seq
eieio eieio-core cl-macs password-cache json subr-x map byte-opt gv
bytecomp byte-compile url-vars cl-loaddefs cl-lib rmc iso-transl tooltip
cconv eldoc paren electric uniquify ediff-hook vc-hooks lisp-float-type
elisp-mode mwheel term/pgtk-win pgtk-win term/common-win pgtk-dnd
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
gtk pgtk lcms2 multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 268356 18660)
 (symbols 48 20764 2)
 (strings 32 67439 3954)
 (string-bytes 1 2195945)
 (vectors 16 37980)
 (vector-slots 8 699322 30627)
 (floats 8 291 38)
 (intervals 56 389 0)
 (buffers 984 15))

-- 
Knut Anders

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Eli Zaretskii]

> From: Knut Anders Hatlen <kahatlen@gmail.com>
> Date: Thu, 29 Dec 2022 22:25:09 +0100
> 
> 1. Evaluate: (setopt completions-header-format nil completion-show-help nil)
> 
> 2. Type M-x followed by TAB. The *Completions* buffer pops up and shows
> all available commands.
> 
> 3. Type M-<down>.
> 
> Expected result: The first candidate in *Completions* is selected.
> 
> Actual result: The second candidate in *Completions* is selected.

M-<DOWN> runs the command minibuffer-next-completion, so I'm unsure
why you expect what you expect.  It looks like Emacs behaves as
documented here.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Knut Anders Hatlen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Knut Anders Hatlen <kahatlen@gmail.com>
>> Date: Thu, 29 Dec 2022 22:25:09 +0100
>> 
>> 1. Evaluate: (setopt completions-header-format nil completion-show-help nil)
>> 
>> 2. Type M-x followed by TAB. The *Completions* buffer pops up and shows
>> all available commands.
>> 
>> 3. Type M-<down>.
>> 
>> Expected result: The first candidate in *Completions* is selected.
>> 
>> Actual result: The second candidate in *Completions* is selected.
>
> M-<DOWN> runs the command minibuffer-next-completion, so I'm unsure
> why you expect what you expect.  It looks like Emacs behaves as
> documented here.

If I don't touch the completions-header-format and completion-show-help
options, M-<down> selects the first candidate first, not the second
candidate. I find that behaviour reasonable. I didn't expect that
setting those two options would have any impact on which candidate
minibuffer-next-completion selected first. I expected that it only
affected whether or not the header and the help text was printed in the
*Completions* buffer, and that the navigation worked as before.

To select the first candidate when those two options are nil, I have to
do M-<down> followed by M-<up>. My expectation was that it should be
fewer (or at least not more) keystrokes to select the first candidate
than the second candidate.

-- 
Knut Anders

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

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


>
> 1. Evaluate: (setopt completions-header-format nil completion-show-help 
> nil)
>
> 2. Type M-x followed by TAB. The *Completions* buffer pops up and shows 
> all available commands.
>
> 3. Type M-<down>.
>
> Expected result: The first candidate in *Completions* is selected.
>
> Actual result: The second candidate in *Completions* is selected.
>

Thanks for your bug report.

I attach a patch to fix that bug, can you please try it?

It's a workaround, but it's the cleanest fix I can think of.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix-selection-of-completions-with-completions-header.patch --]
[-- Type: text/x-diff; name=Fix-selection-of-completions-with-completions-header.patch, Size: 1320 bytes --]

From 10d86c8025b3387644089fa3774b2c9883fabf36 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Fri, 30 Dec 2022 21:45:24 +0000
Subject: [PATCH] Fix selection of completions with completions-header-format
 nil

* lisp/minibuffer.el (display-completion-list): Insert an
invisible line when completions-header-format is nil.
Fixes bug#60411.
---
 lisp/minibuffer.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 7a720cf2c0..95b8962759 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2275,8 +2275,12 @@ display-completion-list
 
     (with-current-buffer standard-output
       (goto-char (point-max))
-      (when completions-header-format
-        (insert (format completions-header-format (length completions))))
+      (if completions-header-format
+          (insert (format completions-header-format (length completions)))
+        ;; Insert an invisible line, otherwise the first call to
+        ;; 'minibuffer-next-completion' might select the second
+        ;; completion candidate.  See bug#60411.
+        (insert (propertize "\n" 'invisible t)))
       (completion--insert-strings completions group-fun)))
 
   (run-hooks 'completion-setup-hook)
-- 
2.39.0

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Knut Anders Hatlen]

Gregory Heytings <gregory@heytings.org> writes:

>>
>> 1. Evaluate: (setopt completions-header-format nil
>> completion-show-help nil)
>>
>> 2. Type M-x followed by TAB. The *Completions* buffer pops up and
>> shows all available commands.
>>
>> 3. Type M-<down>.
>>
>> Expected result: The first candidate in *Completions* is selected.
>>
>> Actual result: The second candidate in *Completions* is selected.
>>
>
> Thanks for your bug report.
>
> I attach a patch to fix that bug, can you please try it?
>
> It's a workaround, but it's the cleanest fix I can think of.

This seems to work. Thanks!

For completeness, you may consider also handling the case where
completions-header-format is an empty string, which is another not too
unreasonable way to disable the header. minibuffer-next-completion still
skips the first candidate for that case.

-- 
Knut Anders

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

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


>
> For completeness, you may consider also handling the case where 
> completions-header-format is an empty string, which is another not too 
> unreasonable way to disable the header. minibuffer-next-completion still 
> skips the first candidate for that case.
>

Thanks for your feedback!  Indeed, that's another bug, which makes a fix 
elsewhere in the code even less likely.  It is fixed in the attached 
patch.  Can you try it?

Eli, do you have objections to that patch?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix-handling-of-completions-header-format.patch --]
[-- Type: text/x-diff; name=Fix-handling-of-completions-header-format.patch, Size: 1471 bytes --]

From d0443e0215196b1576800ae1d5f0f5357a8a8dad Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 31 Dec 2022 14:50:13 +0000
Subject: [PATCH] Fix handling of completions-header-format

* lisp/minibuffer.el (display-completion-list): Insert an
invisible line when completions-header-format is not a string (in
particular, nil) or is an empty string.  Fixes bug#60411.
Also, do not use completions-header-format when it is not a
string.
---
 lisp/minibuffer.el | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 7a720cf2c0..6c7413c555 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2275,8 +2275,14 @@ display-completion-list
 
     (with-current-buffer standard-output
       (goto-char (point-max))
-      (when completions-header-format
+      (when (stringp completions-header-format)
         (insert (format completions-header-format (length completions))))
+      (when (or (not (stringp completions-header-format))
+                (string= completions-header-format ""))
+        ;; Insert an invisible line, otherwise the first call to
+        ;; 'minibuffer-next-completion' might select the second
+        ;; completion candidate.  See bug#60411.
+        (insert (propertize "\n" 'invisible t)))
       (completion--insert-strings completions group-fun)))
 
   (run-hooks 'completion-setup-hook)
-- 
2.39.0

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Knut Anders Hatlen]

Gregory Heytings <gregory@heytings.org> writes:

>>
>> For completeness, you may consider also handling the case where
>> completions-header-format is an empty string, which is another not
>> too unreasonable way to disable the header.
>> minibuffer-next-completion still skips the first candidate for that
>> case.
>>
>
> Thanks for your feedback!  Indeed, that's another bug, which makes a
> fix elsewhere in the code even less likely.  It is fixed in the
> attached patch.  Can you try it?

Yes, this patch works fine in the scenarios that I have found
problematic. Thank you!

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>>> For completeness, you may consider also handling the case where 
>>> completions-header-format is an empty string, which is another not too 
>>> unreasonable way to disable the header. minibuffer-next-completion 
>>> still skips the first candidate for that case.
>>
>> Thanks for your feedback!  Indeed, that's another bug, which makes a 
>> fix elsewhere in the code even less likely.  It is fixed in the 
>> attached patch.  Can you try it?
>
> Yes, this patch works fine in the scenarios that I have found 
> problematic. Thank you!
>

Great!  Thanks again.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Eli Zaretskii]

> Cc: 60411@debbugs.gnu.org
> Date: Sat, 31 Dec 2022 15:02:57 +0000
> From: Gregory Heytings <gregory@heytings.org>
> 
> > For completeness, you may consider also handling the case where 
> > completions-header-format is an empty string, which is another not too 
> > unreasonable way to disable the header. minibuffer-next-completion still 
> > skips the first candidate for that case.
> >
> 
> Thanks for your feedback!  Indeed, that's another bug, which makes a fix 
> elsewhere in the code even less likely.  It is fixed in the attached 
> patch.  Can you try it?
> 
> Eli, do you have objections to that patch?

It becomes less and less to my liking, TBH: too much ad-hocery.  Juri,
Stefan, WDYT? is this safe enough for the release branch?

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> @@ -2275,8 +2275,14 @@ display-completion-list
>  
>      (with-current-buffer standard-output
>        (goto-char (point-max))
> -      (when completions-header-format
> +      (when (stringp completions-header-format)
>          (insert (format completions-header-format (length completions))))
> +      (when (or (not (stringp completions-header-format))
> +                (string= completions-header-format ""))
> +        ;; Insert an invisible line, otherwise the first call to
> +        ;; 'minibuffer-next-completion' might select the second
> +        ;; completion candidate.  See bug#60411.
> +        (insert (propertize "\n" 'invisible t)))
>        (completion--insert-strings completions group-fun)))
>  
>    (run-hooks 'completion-setup-hook)

Yuck.  I get the impression that it would be cleaner to fix "the first
call to `minibuffer-next-completion`" instead.


        Stefan

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>
> Yuck.  I get the impression that it would be cleaner to fix "the first 
> call to `minibuffer-next-completion`" instead.
>

That was my impression, too.  Until I tried to fix "the first call to 
`minibuffer-next-completion`".  But I may have missed something, of 
course.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Yuck.  I get the impression that it would be cleaner to fix "the first
>> call to `minibuffer-next-completion`" instead.
> That was my impression, too.  Until I tried to fix "the first call to
> `minibuffer-next-completion`".  But I may have missed something, of course.

Hmm...  I think the crux of the matter is that the state in which we are
at the beginning (when creating the *Completions* buffer) is
unclear/accidental (is the first completion already selected or not?).

This is made more muddy by

      (when completions-highlight-face
        (setq-local cursor-face-highlight-nonselected-window t))

which changes the behavior of *Completions* depending on whether
`minibuffer-next-completion` has been used or not since the
*Completions* buffer was setup.
[ "Fixing" this might require extending
  `cursor-face-highlight-nonselected-window` so that it can indicate
  whether to highlight when either *Completion* or the minibuffer is
  selected).  ]

It's not clear to me how to "make this right", but maybe a "better ugly
hack" is to work with the above `setq-local`, i.e. if
`cursor-face-highlight-nonselected-window` is still nil (in which case,
the cursor-face hilighting should be currently off), consider that
`minibuffer-next-completion` should move to the *first* completion
rather than to the next.


        Stefan

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>
> I think the crux of the matter is that the state in which we are at the 
> beginning (when creating the *Completions* buffer) is unclear/accidental 
> (is the first completion already selected or not?).
>

Exactly.

>
> (when completions-highlight-face
>   (setq-local cursor-face-highlight-nonselected-window t))
>
> It's not clear to me how to "make this right", but maybe a "better ugly 
> hack" is to work with the above `setq-local`, i.e. if 
> `cursor-face-highlight-nonselected-window` is still nil (in which case, 
> the cursor-face hilighting should be currently off), consider that 
> `minibuffer-next-completion` should move to the *first* completion 
> rather than to the next.
>

I thought about that solution, but what if someone sets 
completion-highlight-face to nil?  I also tried to add another 
buffer-local variable to distinguish the first and later calls to 
minibuffer-next-completion, but that didn't work in all cases either.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Juri Linkov]

>> I think the crux of the matter is that the state in which we are at the
>> beginning (when creating the *Completions* buffer) is unclear/accidental
>> (is the first completion already selected or not?).
>
> Exactly.
>
>> It's not clear to me how to "make this right", but maybe a "better ugly
>> hack" is to work with the above `setq-local`, i.e. if
>> `cursor-face-highlight-nonselected-window` is still nil (in which case,
>> the cursor-face hilighting should be currently off), consider that
>> `minibuffer-next-completion` should move to the *first* completion rather
>> than to the next.
>
> I thought about that solution, but what if someone sets
> completion-highlight-face to nil?  I also tried to add another buffer-local
> variable to distinguish the first and later calls to
> minibuffer-next-completion, but that didn't work in all cases either.

Then I guess your insert-invisible-\n patch is the simplest way
to enforce such a long-standing rule that no candidate is selected
in the completions buffer initially, even when it has no visible header.
And definitely this is the safest solution for the release branch.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>
> Then I guess your insert-invisible-\n patch is the simplest way to 
> enforce such a long-standing rule that no candidate is selected in the 
> completions buffer initially, even when it has no visible header. And 
> definitely this is the safest solution for the release branch.
>

Stefan and Eli, do you agree with that conclusion?

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Eli Zaretskii]

> Cc: Knut Anders Hatlen <kahatlen@gmail.com>, 60411@debbugs.gnu.org,
>  Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 05 Jan 2023 21:04:03 +0000
> From: Gregory Heytings <gregory@heytings.org>
> 
> > Then I guess your insert-invisible-\n patch is the simplest way to 
> > enforce such a long-standing rule that no candidate is selected in the 
> > completions buffer initially, even when it has no visible header. And 
> > definitely this is the safest solution for the release branch.
> >
> 
> Stefan and Eli, do you agree with that conclusion?

I admit that I've lost the line of reasoning here (too much of the
previous context is being elided, forcing me to re-read the entire
discussion).  Which code is proposed for the release branch, and how
will Emacs behave with that code in this particular use case?

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

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


>> Stefan and Eli, do you agree with that conclusion?
>
> I admit that I've lost the line of reasoning here (too much of the 
> previous context is being elided, forcing me to re-read the entire 
> discussion).  Which code is proposed for the release branch, and how 
> will Emacs behave with that code in this particular use case?
>

It's this patch.  It adds an invisible empty line at the beginning of 
*Completions* when completions-header-format is not a string (in 
particular, nil) or an empty string.

[-- Attachment #2: Fix-handling-of-completions-header-format.patch --]
[-- Type: text/x-diff, Size: 1471 bytes --]

From d0443e0215196b1576800ae1d5f0f5357a8a8dad Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 31 Dec 2022 14:50:13 +0000
Subject: [PATCH] Fix handling of completions-header-format

* lisp/minibuffer.el (display-completion-list): Insert an
invisible line when completions-header-format is not a string (in
particular, nil) or is an empty string.  Fixes bug#60411.
Also, do not use completions-header-format when it is not a
string.
---
 lisp/minibuffer.el | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 7a720cf2c0..6c7413c555 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2275,8 +2275,14 @@ display-completion-list
 
     (with-current-buffer standard-output
       (goto-char (point-max))
-      (when completions-header-format
+      (when (stringp completions-header-format)
         (insert (format completions-header-format (length completions))))
+      (when (or (not (stringp completions-header-format))
+                (string= completions-header-format ""))
+        ;; Insert an invisible line, otherwise the first call to
+        ;; 'minibuffer-next-completion' might select the second
+        ;; completion candidate.  See bug#60411.
+        (insert (propertize "\n" 'invisible t)))
       (completion--insert-strings completions group-fun)))
 
   (run-hooks 'completion-setup-hook)
-- 
2.39.0

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Eli Zaretskii]

> Date: Fri, 06 Jan 2023 08:22:39 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: kahatlen@gmail.com, 60411@debbugs.gnu.org, monnier@iro.umontreal.ca, 
>     juri@linkov.net
> 
> >> Stefan and Eli, do you agree with that conclusion?
> >
> > I admit that I've lost the line of reasoning here (too much of the 
> > previous context is being elided, forcing me to re-read the entire 
> > discussion).  Which code is proposed for the release branch, and how 
> > will Emacs behave with that code in this particular use case?
> >
> 
> It's this patch.  It adds an invisible empty line at the beginning of 
> *Completions* when completions-header-format is not a string (in 
> particular, nil) or an empty string.

What is the significance of these two special values of
completions-header-format?  Is it that you want to test whether the
first candidates starts at buffer position 1 in the *Completions*
buffer?  Then why not test for that explicitly?

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>>>> Stefan and Eli, do you agree with that conclusion?
>>>
>>> I admit that I've lost the line of reasoning here (too much of the 
>>> previous context is being elided, forcing me to re-read the entire 
>>> discussion).  Which code is proposed for the release branch, and how 
>>> will Emacs behave with that code in this particular use case?
>>
>> It's this patch.  It adds an invisible empty line at the beginning of 
>> *Completions* when completions-header-format is not a string (in 
>> particular, nil) or an empty string.
>
> What is the significance of these two special values of 
> completions-header-format?  Is it that you want to test whether the 
> first candidates starts at buffer position 1 in the *Completions* 
> buffer?  Then why not test for that explicitly?
>

When completions-header-format is nil or an empty string, no completion 
header "NNN possible completions:\n" is inserted in *Completions*. 
Because of this, minibuffer-next-completion (bound to M-<down> by 
default), which is supposed to select the first completion when it is 
called for the first time, erroneously selects the second completion. 
The fact that "something" (the completion header and/or the help text 
"Click on a completion to select it...") must exist in *Completions* 
before the first completion candidate is more or less hardcoded in the 
logic of minibuffer-next-completion.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Eli Zaretskii]

> Date: Fri, 06 Jan 2023 09:01:08 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: kahatlen@gmail.com, 60411@debbugs.gnu.org, monnier@iro.umontreal.ca, 
>     juri@linkov.net
> 
> 
> >>>> Stefan and Eli, do you agree with that conclusion?
> >>>
> >>> I admit that I've lost the line of reasoning here (too much of the 
> >>> previous context is being elided, forcing me to re-read the entire 
> >>> discussion).  Which code is proposed for the release branch, and how 
> >>> will Emacs behave with that code in this particular use case?
> >>
> >> It's this patch.  It adds an invisible empty line at the beginning of 
> >> *Completions* when completions-header-format is not a string (in 
> >> particular, nil) or an empty string.
> >
> > What is the significance of these two special values of 
> > completions-header-format?  Is it that you want to test whether the 
> > first candidates starts at buffer position 1 in the *Completions* 
> > buffer?  Then why not test for that explicitly?
> >
> 
> When completions-header-format is nil or an empty string, no completion 
> header "NNN possible completions:\n" is inserted in *Completions*. 
> Because of this, minibuffer-next-completion (bound to M-<down> by 
> default), which is supposed to select the first completion when it is 
> called for the first time, erroneously selects the second completion. 
> The fact that "something" (the completion header and/or the help text 
> "Click on a completion to select it...") must exist in *Completions* 
> before the first completion candidate is more or less hardcoded in the 
> logic of minibuffer-next-completion.

Then why not change that logic in minibuffer-next-completion to be
smarter about this?

What you suggest is too ad-hoc-ish, and any future change to the
possible values of completions-header-format will risk breaking the
condition you propose.

Or maybe what minibuffer-next-completion does should be rethought?
Why does it assume that the first candidate is on the second line?
what if it's on the third line instead?

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>
> Then why not change that logic in minibuffer-next-completion to be 
> smarter about this?
>

I (and Stefan) already tried this, it doesn't seem to be feasible with a 
small and safe change.

>
> What you suggest is too ad-hoc-ish, and any future change to the 
> possible values of completions-header-format will risk breaking the 
> condition you propose.
>

I agree about the adhocishness, but I don't see how it could be done 
differently.

>
> Or maybe what minibuffer-next-completion does should be rethought? Why 
> does it assume that the first candidate is on the second line? what if 
> it's on the third line instead?
>

It does not need to be on the second line, it's only that it cannot be at 
BOB.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Eli Zaretskii]

> Date: Fri, 06 Jan 2023 12:13:54 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: kahatlen@gmail.com, 60411@debbugs.gnu.org, monnier@iro.umontreal.ca, 
>     juri@linkov.net
> 
> > Then why not change that logic in minibuffer-next-completion to be 
> > smarter about this?
> 
> I (and Stefan) already tried this, it doesn't seem to be feasible with a 
> small and safe change.

That's very surprising to hear.  AFAIU, it just looks for some special
text property (in next-completion).  So it sounds like a very simple
breakage of logic, where "next" means "the first one" when you are
exactly at BOB.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>
> That's very surprising to hear.  AFAIU, it just looks for some special 
> text property (in next-completion).  So it sounds like a very simple 
> breakage of logic, where "next" means "the first one" when you are 
> exactly at BOB.
>

The problem is that minibuffer-next-completion is supposed to move to the 
first completion when it is called for the first time, to the next 
completion when it is called for the Nth time, and to the first completion 
again when it is called for the Mth time (where M is the number of 
completion candidates).

If point is at BOB, and if there is nothing before the first completion, 
next-completion finds that there is a text property there, and therefore 
moves to the end of the current completion (the first one) and to the 
beginning of the next completion, with the two calls to 
next-single-property-change.

There is nothing in the current logic of the code with which it is 
possible to make a distinction between "this is the first call" and "this 
is not the first call".

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Eli Zaretskii]

> Date: Fri, 06 Jan 2023 12:39:46 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: kahatlen@gmail.com, 60411@debbugs.gnu.org, monnier@iro.umontreal.ca, 
>     juri@linkov.net
> 
> 
> >
> > That's very surprising to hear.  AFAIU, it just looks for some special 
> > text property (in next-completion).  So it sounds like a very simple 
> > breakage of logic, where "next" means "the first one" when you are 
> > exactly at BOB.
> >
> 
> The problem is that minibuffer-next-completion is supposed to move to the 
> first completion when it is called for the first time, to the next 
> completion when it is called for the Nth time, and to the first completion 
> again when it is called for the Mth time (where M is the number of 
> completion candidates).

Right.

> If point is at BOB, and if there is nothing before the first completion, 
> next-completion finds that there is a text property there, and therefore 
> moves to the end of the current completion (the first one) and to the 
> beginning of the next completion, with the two calls to 
> next-single-property-change.
> 
> There is nothing in the current logic of the code with which it is 
> possible to make a distinction between "this is the first call" and "this 
> is not the first call".

Didn't you just say that the difference between "first" and "next" is
"the first call"?  Can't we make the logic be based on that instead of
assuming that the format produces an empty string under certain
conditions?

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>
> Didn't you just say that the difference between "first" and "next" is 
> "the first call"?  Can't we make the logic be based on that instead of 
> assuming that the format produces an empty string under certain 
> conditions?
>

As far as I can see, no.  What is also possible, if it's the condition 
that you don't like, is to insert that invisible line unconditionally, 
like this:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index f47299bd0d..09125772f3 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2275,6 +2275,10 @@ display-completion-list

      (with-current-buffer standard-output
        (goto-char (point-max))
+      ;; Insert an invisible line, otherwise the first call to
+      ;; 'minibuffer-next-completion' might select the second
+      ;; completion candidate.  See bug#60411.
+      (insert (propertize "\n" 'invisible t))
        (when completions-header-format
          (insert (format completions-header-format (length completions))))
        (completion--insert-strings completions group-fun)))

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Eli Zaretskii]

> Date: Fri, 06 Jan 2023 13:10:18 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: kahatlen@gmail.com, 60411@debbugs.gnu.org, monnier@iro.umontreal.ca, 
>     juri@linkov.net
> 
> > Didn't you just say that the difference between "first" and "next" is 
> > "the first call"?  Can't we make the logic be based on that instead of 
> > assuming that the format produces an empty string under certain 
> > conditions?
> >
> 
> As far as I can see, no.  What is also possible, if it's the condition 
> that you don't like, is to insert that invisible line unconditionally, 
> like this:

If we cannot come up with any better ideas (which again surprises me),
then unconditionally adding such a newline is better.

But let's wait for Stefan to chime in.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>>> Didn't you just say that the difference between "first" and "next" is 
>>> "the first call"?  Can't we make the logic be based on that instead of 
>>> assuming that the format produces an empty string under certain 
>>> conditions?
>>
>> As far as I can see, no.  What is also possible, if it's the condition 
>> that you don't like, is to insert that invisible line unconditionally, 
>> like this:
>
> If we cannot come up with any better ideas (which again surprises me), 
> then unconditionally adding such a newline is better.
>

Okay.  I just had another look at this bug, and the patch below seems to 
work, too.  I find it much trickier and adhocish, though.

diff --git a/lisp/simple.el b/lisp/simple.el
index 63479e9ce0..75f9956548 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9698,6 +9698,14 @@ next-completion
    (let ((tabcommand (member (this-command-keys) '("\t" [backtab])))
          pos)
      (catch 'bound
+      (when (and (= n 1)
+                 (= (point) (point-min))
+                 (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 0))
+
        (while (> n 0)
          (setq pos (point))
          ;; If in a completion, move to the end of it.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Eli Zaretskii]

> Date: Fri, 06 Jan 2023 17:07:21 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: kahatlen@gmail.com, 60411@debbugs.gnu.org, monnier@iro.umontreal.ca, 
>     juri@linkov.net
> 
> > If we cannot come up with any better ideas (which again surprises me), 
> > then unconditionally adding such a newline is better.
> >
> 
> Okay.  I just had another look at this bug, and the patch below seems to 
> work, too.  I find it much trickier and adhocish, though.

I find it way better than the alternatives.  Stefan?

Thanks.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Okay.  I just had another look at this bug, and the patch below seems to 
>> work, too.  I find it much trickier and adhocish, though.
> I find it way better than the alternatives.  Stefan?

The code is more complex than the alternative but it solves the problem
where it occurs, so I like it better as well.
(I'd replace (= (point) (point-min)) with (bobp), tho).


        Stefan

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>>> Okay.  I just had another look at this bug, and the patch below seems 
>>> to work, too.  I find it much trickier and adhocish, though.
>>
>> I find it way better than the alternatives.  Stefan?
>
> The code is more complex than the alternative but it solves the problem 
> where it occurs, so I like it better as well.
>
> (I'd replace (= (point) (point-min)) with (bobp), tho).
>

Okay, so I'll install that patch in a day or two, unless someone objects.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Juri Linkov]

> Okay.  I just had another look at this bug, and the patch below seems to
> work, too.  I find it much trickier and adhocish, though.

Indeed.  But I'm not opposed to this patch.

> +      (when (and (= n 1)
> +                 (= (point) (point-min))
> +                 (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 0))

Please remove (= n 1) and replace (setq n 0) with (setq n (1- n))
to support the prefix arg with ‘M-N M-n’.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Juri Linkov]

>> +      (when (and (= n 1)
>> +                 (= (point) (point-min))
>> +                 (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 0))
>
> Please remove (= n 1) and replace (setq n 0) with (setq n (1- n))
> to support the prefix arg with ‘M-N M-n’.

When n > 0.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

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


>> Please remove (= n 1) and replace (setq n 0) with (setq n (1- n)) to 
>> support the prefix arg with ‘M-N M-n’.
>
> When n > 0.
>

You mean this, right?

diff --git a/lisp/simple.el b/lisp/simple.el
index 63479e9ce0..0221881641 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9698,6 +9698,14 @@ next-completion
    (let ((tabcommand (member (this-command-keys) '("\t" [backtab])))
          pos)
      (catch 'bound
+      (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)))
+
        (while (> n 0)
          (setq pos (point))
          ;; If in a completion, move to the end of it.

It took me a few minutes to understand what you meant by "support the 
prefix arg" in this case.  And indeed with the previous version of the 
patch, M-x M-2 M-<down> selects the third completion candidate (2C-split), 
whereas with this patch the second one (2C-command) is selected.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Juri Linkov]

>>> Please remove (= n 1) and replace (setq n 0) with (setq n (1- n)) to
>>> support the prefix arg with ‘M-N M-n’.
>>
>> When n > 0.
>
> You mean this, right?

Exactly.

> It took me a few minutes to understand what you meant by "support the
> prefix arg" in this case.  And indeed with the previous version of the
> patch, M-x M-2 M-<down> selects the third completion candidate (2C-split),
> whereas with this patch the second one (2C-command) is selected.

Sorry for two mistakes in one message.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>> You mean this, right?
>
> Exactly.
>

Thanks!

>
> Sorry for two mistakes in one message.
>

No worries (of course)!

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Juri Linkov]

close 60411 29.0.60
thanks

> diff --git a/lisp/simple.el b/lisp/simple.el
> index 63479e9ce0..0221881641 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -9698,6 +9698,14 @@ next-completion
>    (let ((tabcommand (member (this-command-keys) '("\t" [backtab])))
>          pos)
>      (catch 'bound
> +      (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)))
> +
>        (while (> n 0)
>          (setq pos (point))
>          ;; If in a completion, move to the end of it.

Thanks, now pushed to emacs-29 and closed.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > Then why not change that logic in minibuffer-next-completion to be 
>> > smarter about this?
>> I (and Stefan) already tried this, it doesn't seem to be feasible with a 
>> small and safe change.
> That's very surprising to hear.  AFAIU, it just looks for some special
> text property (in next-completion).  So it sounds like a very simple
> breakage of logic, where "next" means "the first one" when you are
> exactly at BOB.

The problem is how to determine "this is the first time".  Currently we
encode that information indirectly by the fact that point as at BOB
(and is not on an actual completion).

We could try and add a boolean buffer-local variable to remember if
we've already used `minibuffer-next-completion`.  Gregory said he tried
and bumped into further problems.  It would arguably be cleaner to do
that (and fix whichever other problem shows up), but I haven't had the
time to look into that.

I suspect in the mean time Gregory's hack might be an OK workaround
(invisible text tends to come with its own problems, so I'd prefer if
we install it conditionally rather than unconditionally, BTW), but
it should have a comment with a FIXME.


        Stefan

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Gregory Heytings <gregory@heytings.org>,  kahatlen@gmail.com,
>   60411@debbugs.gnu.org,  juri@linkov.net
> Date: Fri, 06 Jan 2023 12:51:41 -0500
> 
> >> > Then why not change that logic in minibuffer-next-completion to be 
> >> > smarter about this?
> >> I (and Stefan) already tried this, it doesn't seem to be feasible with a 
> >> small and safe change.
> > That's very surprising to hear.  AFAIU, it just looks for some special
> > text property (in next-completion).  So it sounds like a very simple
> > breakage of logic, where "next" means "the first one" when you are
> > exactly at BOB.
> 
> The problem is how to determine "this is the first time".  Currently we
> encode that information indirectly by the fact that point as at BOB
> (and is not on an actual completion).

Gregory just suggested a patch which does that in a fairly simple and
straightforward way.

bug#60411: 29.0.60; minibuffer-next-completion skips first candidate when completions-header-format and completion-show-help are nil

[Gregory Heytings]

>
> We could try and add a boolean buffer-local variable to remember if 
> we've already used `minibuffer-next-completion`.  Gregory said he tried 
> and bumped into further problems.  It would arguably be cleaner to do 
> that (and fix whichever other problem shows up), but I haven't had the 
> time to look into that.
>

In fact, the problem is that buffer-local is not local enough in this 
case, because *Completions* can be reused.  Somehow I did not think of a 
"text-local" variable earlier.