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

[Spencer Baugh]

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:

>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