bug#68688: 30.0.50; next-line-completion doesn't work with multiline completions

bug#68688: 30.0.50; next-line-completion doesn't work with multiline completions

[Spencer Baugh]

1. emacs -Q
2. (completing-read ": " '("foo\nfoo" "bar\nbar"))
3. M-g M-c to switch to *Completions*
4. <down> to switch between completions
5. Note that point stays on the first completion no matter how many
times you hit <down>.

It looks like this is because the relevant part of next-line-completion
just runs forward-line until point is on a completion - it doesn't do
anything to make sure that point is on a *different* completion.


In GNU Emacs 30.0.50 (build 20, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2023-12-27 built on
 igm-qws-u22796a
Repository revision: b9c0ce0a400f18e1bba4e3491c94994d73ef9405
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 emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util time-date subr-x mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils cl-extra shortdoc
text-property-search comp-common rx help-fns radix-tree help-mode
cl-loaddefs cl-lib 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 77798 11426) (symbols 48 10074 0) (strings 32 25127 2339)
 (string-bytes 1 732356) (vectors 16 11366)
 (vector-slots 8 167679 11150) (floats 8 102 23) (intervals 56 404 18)
 (buffers 984 13))

bug#68688: 30.0.50; next-line-completion doesn't work with multiline completions

[Spencer Baugh]

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


Patch which fixes this attached.  It simply adds an additional loop
around forward-line which runs until we leave the completion at point.

BTW, I think we should have some helper functions for "get start of
completion candidate or point-min" and "get end of completion candidate
or point-max", or possibly "move to start of candidate" and "move to end
of candidate"; it's pretty hard to get right.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-next-line-completion-for-multi-line-completions.patch --]
[-- Type: text/x-patch, Size: 2212 bytes --]

From ffe03b3fe8fb805f5e4eb0a2ea956d573272e74a Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Wed, 24 Jan 2024 10:52:40 -0500
Subject: [PATCH] Fix next-line-completion for multi-line completions

Previously it would not move out of a multi-line completion, and now it will.

* lisp/simple.el (next-line-completion): Move out of the completion
before going forward lines.  (bug#68688)
---
 lisp/simple.el | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/lisp/simple.el b/lisp/simple.el
index 313241a7d34..35fd76b8d1a 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9937,6 +9937,17 @@ next-line-completion
 
     (while (> n 0)
       (setq found nil pos nil column (current-column) line (line-number-at-pos))
+      ;; If in a completion and not at the end, move lines forward
+      ;; until we leave it or reach the end.
+      (when (and (get-text-property (point) 'mouse-face)
+                 (not (eobp))
+                 (get-text-property (1+ (point)) 'mouse-face))
+        (let ((end (or (next-single-property-change (point) 'mouse-face) (point-max))))
+          (while (and
+                  (< (point) end)
+                  (eq (forward-line 1) 0)
+                  (not (eobp))
+                  (eq (move-to-column column) column)))))
       (while (and (not found)
                   (eq (forward-line 1) 0)
                   (not (eobp))
@@ -9961,6 +9972,15 @@ next-line-completion
 
     (while (< n 0)
       (setq found nil pos nil column (current-column) line (line-number-at-pos))
+      ;; If in a completion and not at the start, move lines backward
+      ;; until we leave it or reach the start.
+      (when (and (get-text-property (point) 'mouse-face)
+                 (not (bobp))
+                 (get-text-property (1- (point)) 'mouse-face))
+        (let ((start (previous-single-property-change (point) 'mouse-face)))
+          (while (and (> (point) start)
+                      (eq (forward-line -1) 0)
+                      (eq (move-to-column column) column)))))
       (while (and (not found)
                   (eq (forward-line -1) 0)
                   (eq (move-to-column column) column))
-- 
2.39.3

bug#68688: 30.0.50; next-line-completion doesn't work with multiline completions

[Juri Linkov]

> Patch which fixes this attached.

Thanks for fixing the multi-line case.  Could you please
add a test in minibuffer-tests.el.  Maybe easier is
to replace '("aa" "ab" "ac") with '("aa" "a\nb" "ac").
Or a new test would be better, I don't know.

> It simply adds an additional loop around forward-line which runs
> until we leave the completion at point.

Alternatively without a loop maybe simpler would be just to move
to the end of the current completion before calling forward-line?

> BTW, I think we should have some helper functions for "get start of
> completion candidate or point-min" and "get end of completion candidate
> or point-max", or possibly "move to start of candidate" and "move to end
> of candidate"; it's pretty hard to get right.

Completely agreed.  We need more helper functions for all 4 functions:
previous-completion, next-completion, previous-line-completion,
next-line-completion.

BTW, do you think that these parts below are not needed anymore
since you implemented a hidden placeholder at the top
in case of no header?

diff --git a/lisp/simple.el b/lisp/simple.el
index 692c0dacefc..ca3599e6cb3 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9913,11 +9913,11 @@ first-completion
   "Move to the first item in the completions buffer."
   (interactive)
   (goto-char (point-min))
-  (if (get-text-property (point) 'mouse-face)
-      (unless (get-text-property (point) 'first-completion)
-        (let ((inhibit-read-only t))
-          (add-text-properties (point) (min (1+ (point)) (point-max))
-                               '(first-completion t))))
+  (if t ;; (get-text-property (point) 'mouse-face)
+      ;; (unless (get-text-property (point) 'first-completion)
+      ;;   (let ((inhibit-read-only t))
+      ;;     (add-text-properties (point) (min (1+ (point)) (point-max))
+      ;;                          '(first-completion t))))
     (when-let ((pos (next-single-property-change (point) 'mouse-face)))
       (goto-char pos))))
 
@@ -9950,13 +9950,13 @@ 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)))
+      ;; (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))
@@ -10019,13 +10019,13 @@ next-line-completion
 Also see the `completion-auto-wrap' variable."
   (interactive "p")
   (let (line column pos found)
-    (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)))
+    ;; (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.

bug#68688: 30.0.50; next-line-completion doesn't work with multiline completions

[Spencer Baugh]

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

Juri Linkov <juri@linkov.net> writes:
>> Patch which fixes this attached.
>
> Thanks for fixing the multi-line case.  Could you please
> add a test in minibuffer-tests.el.  Maybe easier is
> to replace '("aa" "ab" "ac") with '("aa" "a\nb" "ac").
> Or a new test would be better, I don't know.

Sure.  New test is easier since "a\nb" sorts before "aa", so adding the
newline changes the test results.

>> It simply adds an additional loop around forward-line which runs
>> until we leave the completion at point.
>
> Alternatively without a loop maybe simpler would be just to move
> to the end of the current completion before calling forward-line?

I was unsure whether that would work correctly in all cases.  But it
seems to pass tests, so let's just try it.  If it does break, the bug
report will give us some new test cases :)

>> BTW, I think we should have some helper functions for "get start of
>> completion candidate or point-min" and "get end of completion candidate
>> or point-max", or possibly "move to start of candidate" and "move to end
>> of candidate"; it's pretty hard to get right.
>
> Completely agreed.  We need more helper functions for all 4 functions:
> previous-completion, next-completion, previous-line-completion,
> next-line-completion.

Cool, I made two helpers in the latest version of my patch.  Using only
in next-line-completion for now, but we can use it more in the other
completion function.

> BTW, do you think that these parts below are not needed anymore
> since you implemented a hidden placeholder at the top
> in case of no header?

Yes, I think all the code for 'first-completion can be removed.

Updated patch for next-line-completion:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-next-line-completion-for-multi-line-completions.patch --]
[-- Type: text/x-patch, Size: 3767 bytes --]

From cbc8bdc1386a4bc9c420d8ab06b844c6f6928c35 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Wed, 24 Jan 2024 10:52:40 -0500
Subject: [PATCH] Fix next-line-completion for multi-line completions

Previously it would not move out of a multi-line completion, and now it will.

* lisp/simple.el (next-line-completion): Move to the completion start
or end before going forward or backward lines.  (bug#68688)
---
 lisp/simple.el                | 20 +++++++++++++++++---
 test/lisp/minibuffer-tests.el | 14 ++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 692c0dacefc..4ffe159dc88 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9940,6 +9940,20 @@ previous-completion
   (interactive "p")
   (next-completion (- n)))
 
+(defun completion--move-to-candidate-start ()
+  "If in a completion candidate, move point to its start."
+  (when (and (get-text-property (point) 'mouse-face)
+             (not (bobp))
+             (get-text-property (1- (point)) 'mouse-face))
+    (goto-char (previous-single-property-change (point) 'mouse-face))))
+
+(defun completion--move-to-candidate-end ()
+  "If in a completion candidate, move point to its end."
+  (when (and (get-text-property (point) 'mouse-face)
+             (not (eobp))
+             (get-text-property (1+ (point)) 'mouse-face))
+    (goto-char (or (next-single-property-change (point) 'mouse-face) (point-max)))))
+
 (defun next-completion (n)
   "Move to the next item in the completions buffer.
 With prefix argument N, move N items (negative N means move
@@ -10029,9 +10043,7 @@ next-line-completion
 
     (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)))
+        (completion--move-to-candidate-start)
       ;; Try to move to the previous completion.
       (setq pos (previous-single-property-change (point) 'mouse-face))
       (if pos
@@ -10046,6 +10058,7 @@ next-line-completion
 
     (while (> n 0)
       (setq found nil pos nil column (current-column) line (line-number-at-pos))
+      (completion--move-to-candidate-end)
       (while (and (not found)
                   (eq (forward-line 1) 0)
                   (not (eobp))
@@ -10070,6 +10083,7 @@ next-line-completion
 
     (while (< n 0)
       (setq found nil pos nil column (current-column) line (line-number-at-pos))
+      (completion--move-to-candidate-start)
       (while (and (not found)
                   (eq (forward-line -1) 0)
                   (eq (move-to-column column) column))
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index c1fe3032cb5..d104858b0d0 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -465,6 +465,20 @@ completion-auto-wrap-test
       (previous-line-completion 4)
       (should (equal "ac" (get-text-property (point) 'completion--string))))))
 
+(ert-deftest completion-next-line-multline-test ()
+  (let ((completion-auto-wrap t))
+    (completing-read-with-minibuffer-setup
+     '("a\na" "a\nb" "ac")
+     (insert "a")
+     (minibuffer-completion-help)
+     (switch-to-completions)
+     (goto-char (point-min))
+     (next-line-completion 5)
+     (should (equal "a\nb" (get-text-property (point) 'completion--string)))
+     (goto-char (point-min))
+     (previous-line-completion 5)
+     (should (equal "a\nb" (get-text-property (point) 'completion--string))))))
+
 (ert-deftest completions-header-format-test ()
   (let ((completion-show-help nil)
         (completions-header-format nil))
-- 
2.39.3

bug#68688: 30.0.50; next-line-completion doesn't work with multiline completions

[Juri Linkov]

>> Alternatively without a loop maybe simpler would be just to move
>> to the end of the current completion before calling forward-line?
>
> I was unsure whether that would work correctly in all cases.  But it
> seems to pass tests, so let's just try it.  If it does break, the bug
> report will give us some new test cases :)

It might fail on a multi-column and at the same time multi-line layout.
But currently this is just a hypothetical case.  I remember there was
an idea to use tabulated-list-mode in the completions buffer that
could simplify next-completion but at the cost of breaking backward
compatibility with existing commands that rely on text properties.

>> Completely agreed.  We need more helper functions for all 4 functions:
>> previous-completion, next-completion, previous-line-completion,
>> next-line-completion.
>
> Cool, I made two helpers in the latest version of my patch.  Using only
> in next-line-completion for now, but we can use it more in the other
> completion function.

Using helpers more would be nice.

>> BTW, do you think that these parts below are not needed anymore
>> since you implemented a hidden placeholder at the top
>> in case of no header?
>
> Yes, I think all the code for 'first-completion can be removed.
>
> Updated patch for next-line-completion:

Thanks for the patch.  Now pushed to master.

bug#68688: 30.0.50; next-line-completion doesn't work with multiline completions

[Juri Linkov]

While testing, I found an old bug where group lines prevented from
moving horizontally, such as moving from aa2 to ac2:

  > Group 1
              aa1   aa2   aa3
              aa4
  > Group 2
              ab1
  > Group 3
              ac1   ac2

The fix was very simple, and I updated the group test as well.

bug#68688: 30.0.50; next-line-completion doesn't work with multiline completions

[Juri Linkov]

close 68688 30.0.50
thanks

>> Updated patch for next-line-completion:
>
> Thanks for the patch.  Now pushed to master.

It seems this bug report could be closed now.