bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

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

Tags: patch


Add completion-preserve-selection, a defcustom which allows keeping the
same selected candidate after *Completions* is updated by
minibuffer-completion-help.

This works correctly with choose-completion-deselect-if-after: If point
is after a completion (such that choose-completion-deselect-if-after=t
means it won't be treated as selected), point will still be after that
completion after updating the list.

This feature is primarily motivated by the fact that some other
completion UIs (e.g. ido, vertico, etc) effectively have this behavior:
whenever they update the list of completions, they preserve whatever
candidate is selected.

Since the default UI doesn't (yet) have support for auto-updating the
list of completions, preserving the selected candidate is less
significant.  But it might be an interesting feature on its own, and
it's very non-invasive, so maybe we can turn it on by default.

In GNU Emacs 29.2.50 (build 5, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2024-10-15 built on
 igm-qws-u22796a
Repository revision: 16a2e41014c6ea0f3cf268be8d37fe09aeac231e
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.10 (Green Obsidian)

Configured using:
 'configure --with-x-toolkit=lucid --without-gpm --without-gconf
 --without-selinux --without-imagemagick --with-modules --with-gif=no
 --with-tree-sitter --with-native-compilation=aot
 PKG_CONFIG_PATH=/usr/local/home/garnish/libtree-sitter/0.22.6-1/lib/pkgconfig/'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Optionally-preserve-selected-candidate-across-Comple.patch --]
[-- Type: text/patch, Size: 6939 bytes --]

From 7708bdbc0641c6468aba70662791d9dda043ff88 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Fri, 25 Oct 2024 16:56:30 -0400
Subject: [PATCH] Optionally preserve selected candidate across *Completions*
 update

* lisp/minibuffer.el (completion-preserve-selection):
(minibuffer-completion-help):
(minibuffer-next-completion):
* lisp/simple.el (choose-completion):
(completion-setup-function):
---
 lisp/minibuffer.el | 29 +++++++++++++++++++++++++----
 lisp/simple.el     | 39 ++++++++++++++++++++++++---------------
 2 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 44d07557f48..a18f4330514 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2538,6 +2538,16 @@ completions--fit-window-to-buffer
         (resize-temp-buffer-window win))
     (fit-window-to-buffer win completions-max-height)))
 
+(defcustom completion-preserve-selection nil
+  "If non-nil, `minibuffer-completion-help' preserves the selected completion candidate.
+
+If non-nil, and point is on a completion candidate in the displayed
+*Completions* window, `minibuffer-completion-help' will put point on the
+same candidate after updating *Completions*."
+  :type '(choice (const :tag "Updating *Completions* deselects any completion candidate" t)
+                 (const :tag "Candidates in *Completions* stay selected when the list is updated" nil))
+  :version "31.1")
+
 (defcustom completion-auto-deselect t
   "If non-nil, deselect current completion candidate when you type in minibuffer.
 
@@ -2624,6 +2634,12 @@ minibuffer-completion-help
              (sort-fun (completion-metadata-get all-md 'display-sort-function))
              (group-fun (completion-metadata-get all-md 'group-function))
              (mainbuf (current-buffer))
+             (current-candidate-and-offset
+              (when-let* ((window (get-buffer-window "*Completions*" 0)))
+                (with-selected-window window
+                  (when-let* ((beg (completions--start-of-candidate-at (point))))
+
+                    (cons (get-text-property beg 'completion--string) (- (point) beg))))))
              ;; If the *Completions* buffer is shown in a new
              ;; window, mark it as softly-dedicated, so bury-buffer in
              ;; minibuffer-hide-completions will know whether to
@@ -2647,7 +2663,7 @@ minibuffer-completion-help
             ,(when temp-buffer-resize-mode
                '(preserve-size . (nil . t)))
             (body-function
-             . ,#'(lambda (_window)
+             . ,#'(lambda (window)
                     (with-current-buffer mainbuf
                       (when completion-auto-deselect
                         (add-hook 'after-change-functions #'completions--after-change nil t))
@@ -2737,7 +2753,14 @@ minibuffer-completion-help
                                                      (if (eq (car bounds) (length result))
                                                          'exact 'finished))))))
 
-                      (display-completion-list completions nil group-fun)))))
+                      (display-completion-list completions nil group-fun)
+                      (when (and completion-preserve-selection current-candidate-and-offset)
+                        (with-current-buffer standard-output
+                          (when-let* ((match (text-property-search-forward
+                                              'completion--string (car current-candidate-and-offset) t)))
+                            (goto-char (prop-match-beginning match))
+                            (forward-char (cdr current-candidate-and-offset))
+                            (set-window-point window (point)))))))))
           nil)))
     nil))
 
@@ -4905,8 +4928,6 @@ minibuffer-next-completion
   (interactive "p")
   (let ((auto-choose minibuffer-completion-auto-choose))
     (with-minibuffer-completions-window
-      (when completions-highlight-face
-        (setq-local cursor-face-highlight-nonselected-window t))
       (if vertical
           (next-line-completion (or n 1))
         (next-completion (or n 1)))
diff --git a/lisp/simple.el b/lisp/simple.el
index 2ffd6e86e56..3a142ef14b3 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10246,6 +10246,23 @@ choose-completion-deselect-if-after
 
 This makes `completions--deselect' effective.")
 
+(defun completions--start-of-candidate-at (position)
+  "Return the start position of the completion candidate at POSITION."
+  (save-excursion
+    (goto-char position)
+    (let (beg)
+      (cond
+       ((and (not (eobp))
+             (get-text-property (point) 'completion--string))
+        (setq beg (1+ (point))))
+       ((and (not (bobp))
+             (get-text-property (1- (point)) 'completion--string))
+        (setq beg (point))))
+      (when beg
+        (or (previous-single-property-change
+             beg 'completion--string)
+            beg)))))
+
 (defun choose-completion (&optional event no-exit no-quit)
   "Choose the completion at point.
 If EVENT, use EVENT's position to determine the starting position.
@@ -10269,21 +10286,11 @@ choose-completion
                (or (get-text-property (posn-point (event-start event))
                                       'completion--string)
                    (error "No completion here"))
-           (save-excursion
-             (goto-char (posn-point (event-start event)))
-             (let (beg)
-               (cond
-                ((and (not (eobp))
-                      (get-text-property (point) 'completion--string))
-                 (setq beg (1+ (point))))
-                ((and (not (bobp))
-                      (get-text-property (1- (point)) 'completion--string))
-                 (setq beg (point)))
-                (t (error "No completion here")))
-               (setq beg (or (previous-single-property-change
-                              beg 'completion--string)
-                             beg))
-               (get-text-property beg 'completion--string))))))
+             (if-let* ((candidate-start
+                        (completions--start-of-candidate-at
+                         (posn-point (event-start event)))))
+                 (get-text-property candidate-start 'completion--string)
+               (error "No completion here")))))
 
       (unless (buffer-live-p buffer)
         (error "Destination buffer is dead"))
@@ -10451,6 +10458,8 @@ completion-setup-function
       (let ((base-position completion-base-position)
             (insert-fun completion-list-insert-choice-function))
         (completion-list-mode)
+        (when completions-highlight-face
+          (setq-local cursor-face-highlight-nonselected-window t))
         (setq-local completion-base-position base-position)
         (setq-local completion-list-insert-choice-function insert-fun))
       (setq-local completion-reference-buffer mainbuf)
-- 
2.39.3

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

[Eli Zaretskii]

> Cc: Juri Linkov <juri@linkov.net>
> Date: Fri, 25 Oct 2024 17:32:38 -0400
> From:  Spencer Baugh via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Add completion-preserve-selection, a defcustom which allows keeping the
> same selected candidate after *Completions* is updated by
> minibuffer-completion-help.

Shouldn't we stop complicating the completion machinery at some point?
It is already basically impenetrable, and if someone still can reason
about what it does in any specific case, my hat's off to them; I have
long ago reached the point where the _only_ way of understanding
what's going on is to step with a debugger through the code -- and it
doesn't help that some of the code is in Lisp and some in C, so Lisp
calls into C, which calls back into Lisp, etc., thus one needs to
interrupt the stepping, fire up a different debugger, then go back.

Stefan, WDYT?  Should we close completion to further development and
accept only bugfixes?

What do Stefan Kangas and Andrea think?

> +(defcustom completion-preserve-selection nil
> +  "If non-nil, `minibuffer-completion-help' preserves the selected completion candidate.

This doc-string line is too long.

I also don't like the name: "selection" could be confused to mean the
X selection.  I'd use "selected-candidates" instead.

> +If non-nil, and point is on a completion candidate in the displayed
> +*Completions* window, `minibuffer-completion-help' will put point on the
> +same candidate after updating *Completions*."

This is completely unclear: what does minibuffer-completion-help have
to do with updating *Completions*?

> +             (current-candidate-and-offset
> +              (when-let* ((window (get-buffer-window "*Completions*" 0)))
> +                (with-selected-window window
> +                  (when-let* ((beg (completions--start-of-candidate-at (point))))
> +
> +                    (cons (get-text-property beg 'completion--string) (- (point) beg))))))

Why is this done unconditionally?  with-selected-window is not a cheap
function.

> @@ -4905,8 +4928,6 @@ minibuffer-next-completion
>    (interactive "p")
>    (let ((auto-choose minibuffer-completion-auto-choose))
>      (with-minibuffer-completions-window
> -      (when completions-highlight-face
> -        (setq-local cursor-face-highlight-nonselected-window t))

Why is this being moved to a different place?

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

> Add completion-preserve-selection, a defcustom which allows keeping the
> same selected candidate after *Completions* is updated by
> minibuffer-completion-help.

IIUC, this is a change that affects only `minibuffer-completion-help`,
which is part of the standard UI's *Completions*, right, the generic
completion infrastructure, right?

> This works correctly with choose-completion-deselect-if-after: If point
> is after a completion (such that choose-completion-deselect-if-after=t
> means it won't be treated as selected), point will still be after that
> completion after updating the list.

I can't remember what `choose-completion-deselect-if-after` is about,
sorry, but the above reads like "the code doesn't have one of the bugs
I encountered while implemented the code".  Is there more to this paragraph?

> This feature is primarily motivated by the fact that some other
> completion UIs (e.g. ido, vertico, etc) effectively have this behavior:
> whenever they update the list of completions, they preserve whatever
> candidate is selected.

Are there cases where the current behavior is preferable?
IOW, why do we need a config var and why does it default to nil?

> * lisp/minibuffer.el (completion-preserve-selection):
> (minibuffer-completion-help):
> (minibuffer-next-completion):
> * lisp/simple.el (choose-completion):
> (completion-setup-function):

I presume you know that this is incomplete.  🙂

[ BTW, I really regret not moving the `completion-list-mode` out of
  `simple.el`.  ]

> +  "If non-nil, `minibuffer-completion-help' preserves the selected completion candidate.
> +
> +If non-nil, and point is on a completion candidate in the displayed
> +*Completions* window, `minibuffer-completion-help' will put point on the
> +same candidate after updating *Completions*."

I think we should be more clear that it *tries* to preserve it.
After all, the selected candidate may simply be absent from the new set
of candidates.

> @@ -2624,6 +2634,12 @@ minibuffer-completion-help
>               (sort-fun (completion-metadata-get all-md 'display-sort-function))
>               (group-fun (completion-metadata-get all-md 'group-function))
>               (mainbuf (current-buffer))
> +             (current-candidate-and-offset
> +              (when-let* ((window (get-buffer-window "*Completions*" 0)))
> +                (with-selected-window window
> +                  (when-let* ((beg (completions--start-of-candidate-at (point))))
> +
> +                    (cons (get-text-property beg 'completion--string) (- (point) beg))))))
>               ;; If the *Completions* buffer is shown in a new
>               ;; window, mark it as softly-dedicated, so bury-buffer in
>               ;; minibuffer-hide-completions will know whether to

Hmm... are we sure here that the `*Completions*`s content is related to
the current completion session?  I don't think we want to preserve the
selection when it came from an unrelated use of completion half an
hour earlier.

> @@ -4905,8 +4928,6 @@ minibuffer-next-completion
>    (interactive "p")
>    (let ((auto-choose minibuffer-completion-auto-choose))
>      (with-minibuffer-completions-window
> -      (when completions-highlight-face
> -        (setq-local cursor-face-highlight-nonselected-window t))
>        (if vertical
>            (next-line-completion (or n 1))
>          (next-completion (or n 1)))
[...]
> @@ -10451,6 +10458,8 @@ completion-setup-function
>        (let ((base-position completion-base-position)
>              (insert-fun completion-list-insert-choice-function))
>          (completion-list-mode)
> +        (when completions-highlight-face
> +          (setq-local cursor-face-highlight-nonselected-window t))
>          (setq-local completion-base-position base-position)
>          (setq-local completion-list-insert-choice-function insert-fun))
>        (setq-local completion-reference-buffer mainbuf)

Why?


        Stefan

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

> Shouldn't we stop complicating the completion machinery at some point?

Complexity knows no bounds.

OT1H I agree that the completion infrastructure would benefit from some
cleanup and is too complex in some areas.  OTOH this specific request
doesn't affect the "completion machinery", but only the
`minibuffer-completion-help` functionality which is a fairly simple part
of the code.

> what's going on is to step with a debugger through the code -- and it
> doesn't help that some of the code is in Lisp and some in C, so Lisp
> calls into C, which calls back into Lisp, etc., thus one needs to
> interrupt the stepping, fire up a different debugger, then go back.

I luckily haven't had to step through the C code of the completion in
a long while.  But the code layout could be improved to better reflect
the architecture of the system (the separation between the "backend"
(completion tables), the "middle end" (completion styles), the standard
minibuffer UI, the in-buffer UI, the completion-list-mode).

> Stefan, WDYT?  Should we close completion to further development and
> accept only bugfixes?

🙂


        Stefan

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

[Juri Linkov]

>> Add completion-preserve-selection, a defcustom which allows keeping the
>> same selected candidate after *Completions* is updated by
>> minibuffer-completion-help.
>
> Shouldn't we stop complicating the completion machinery at some point?
> It is already basically impenetrable, and if someone still can reason
> about what it does in any specific case, my hat's off to them; I have
> long ago reached the point where the _only_ way of understanding
> what's going on is to step with a debugger through the code -- and it
> doesn't help that some of the code is in Lisp and some in C, so Lisp
> calls into C, which calls back into Lisp, etc., thus one needs to
> interrupt the stepping, fire up a different debugger, then go back.

The last sorely missing feature related to the completions window
is auto-updating the list of completions as you type.  There are several
proposed implementations, and we have to merge them sooner or later.
IIUC, this patch makes preparations to achieve this goal sooner.

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Add completion-preserve-selection, a defcustom which allows keeping the
>> same selected candidate after *Completions* is updated by
>> minibuffer-completion-help.
>
> IIUC, this is a change that affects only `minibuffer-completion-help`,
> which is part of the standard UI's *Completions*, right, the generic
> completion infrastructure, right?
>
>> This works correctly with choose-completion-deselect-if-after: If point
>> is after a completion (such that choose-completion-deselect-if-after=t
>> means it won't be treated as selected), point will still be after that
>> completion after updating the list.
>
> I can't remember what `choose-completion-deselect-if-after` is about,
> sorry, but the above reads like "the code doesn't have one of the bugs
> I encountered while implemented the code".  Is there more to this paragraph?

Eh, well, it's just that a completion can be "selected" in two different
ways:

A. if point is on the completion, then choose-completion will always
   choose it

B. if point is right after the completion, then choose-completion will
   choose it if choose-completion-deselect-if-after is nil

The location of point in the completion is preserved, so these two
different behaviors are preserved.

It's just to contrast with another possible approach, where I only
preserve what completion was selected, by moving point to its start
after updating.  That wouldn't preserve the same behavior in case B.

Will improve commit message in next version.

>> This feature is primarily motivated by the fact that some other
>> completion UIs (e.g. ido, vertico, etc) effectively have this behavior:
>> whenever they update the list of completions, they preserve whatever
>> candidate is selected.
>
> Are there cases where the current behavior is preferable?
> IOW, why do we need a config var and why does it default to nil?

It's a fair point.  We could always add the config var later if we end
up wanting it - it's very easy.

So I think it would make sense to remove the config var, and always have
this preservation behavior, if people are OK with that.  That would be
less complex.

>> * lisp/minibuffer.el (completion-preserve-selection):
>> (minibuffer-completion-help):
>> (minibuffer-next-completion):
>> * lisp/simple.el (choose-completion):
>> (completion-setup-function):
>
> I presume you know that this is incomplete.  🙂

Yes :)  Just wanted to get initial feedback.

> [ BTW, I really regret not moving the `completion-list-mode` out of
>   `simple.el`.  ]

Maybe we could still do that?  It would definitely make completion UI
changes a lot easier...

>> +  "If non-nil, `minibuffer-completion-help' preserves the selected completion candidate.
>> +
>> +If non-nil, and point is on a completion candidate in the displayed
>> +*Completions* window, `minibuffer-completion-help' will put point on the
>> +same candidate after updating *Completions*."
>
> I think we should be more clear that it *tries* to preserve it.
> After all, the selected candidate may simply be absent from the new set
> of candidates.

Will update in next version.

>> @@ -2624,6 +2634,12 @@ minibuffer-completion-help
>>               (sort-fun (completion-metadata-get all-md 'display-sort-function))
>>               (group-fun (completion-metadata-get all-md 'group-function))
>>               (mainbuf (current-buffer))
>> +             (current-candidate-and-offset
>> +              (when-let* ((window (get-buffer-window "*Completions*" 0)))
>> +                (with-selected-window window
>> +                  (when-let* ((beg (completions--start-of-candidate-at (point))))
>> +
>> +                    (cons (get-text-property beg 'completion--string) (- (point) beg))))))
>>               ;; If the *Completions* buffer is shown in a new
>>               ;; window, mark it as softly-dedicated, so bury-buffer in
>>               ;; minibuffer-hide-completions will know whether to
>
> Hmm... are we sure here that the `*Completions*`s content is related to
> the current completion session?  I don't think we want to preserve the
> selection when it came from an unrelated use of completion half an
> hour earlier.

That's why I'm doing get-buffer-window here - I figure that if
*Completions* is currently displayed in a window, it's reasonable to
preserve the selected candidate.

(The selected candidate in that window, I guess - so maybe I should use
window-point here?)

It still might not be related to the current completion session, since
the user might have just manually switched buffers to *Completions*, but
I wasn't sure there was a good way to determine that... any suggestions?

>> @@ -4905,8 +4928,6 @@ minibuffer-next-completion
>>    (interactive "p")
>>    (let ((auto-choose minibuffer-completion-auto-choose))
>>      (with-minibuffer-completions-window
>> -      (when completions-highlight-face
>> -        (setq-local cursor-face-highlight-nonselected-window t))
>>        (if vertical
>>            (next-line-completion (or n 1))
>>          (next-completion (or n 1)))
> [...]
>> @@ -10451,6 +10458,8 @@ completion-setup-function
>>        (let ((base-position completion-base-position)
>>              (insert-fun completion-list-insert-choice-function))
>>          (completion-list-mode)
>> +        (when completions-highlight-face
>> +          (setq-local cursor-face-highlight-nonselected-window t))
>>          (setq-local completion-base-position base-position)
>>          (setq-local completion-list-insert-choice-function insert-fun))
>>        (setq-local completion-reference-buffer mainbuf)
>
> Why?

Prevuously cursor-face-highlight-nonselected-window was only set by
next-completion, but minibuffer-completion-help creates a new
*Completions* buffer which doesn't have it set, so the selected
completion isn't highlighted.  Moving that to completion-setup-function
causes it to still be highlighted.

This is kind of an unrelated improvement, since this is probably nicer
anyway - if the user moves point around manually in *Completions*, IMO
that should have the same behavior as minibuffer-next-completion, but
currently it doesn't highlight the same way if they leave *Completions*
because cursor-face-highlight-nonselected-window doesn't get set.

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Juri Linkov <juri@linkov.net>
>> Date: Fri, 25 Oct 2024 17:32:38 -0400
>> From:  Spencer Baugh via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> Add completion-preserve-selection, a defcustom which allows keeping the
>> same selected candidate after *Completions* is updated by
>> minibuffer-completion-help.
>
> Shouldn't we stop complicating the completion machinery at some point?
> It is already basically impenetrable, and if someone still can reason
> about what it does in any specific case, my hat's off to them; I have
> long ago reached the point where the _only_ way of understanding
> what's going on is to step with a debugger through the code -- and it
> doesn't help that some of the code is in Lisp and some in C, so Lisp
> calls into C, which calls back into Lisp, etc., thus one needs to
> interrupt the stepping, fire up a different debugger, then go back.
>
> Stefan, WDYT?  Should we close completion to further development and
> accept only bugfixes?
>
> What do Stefan Kangas and Andrea think?

FWIW, this patch is a very surface-level improvement to the default
completion UI, it doesn't deal with the deeper complicated parts of the
completion machinery.

>> +(defcustom completion-preserve-selection nil
>> +  "If non-nil, `minibuffer-completion-help' preserves the selected completion candidate.
>
> This doc-string line is too long.
>
> I also don't like the name: "selection" could be confused to mean the
> X selection.  I'd use "selected-candidates" instead.

Will change.

>> +If non-nil, and point is on a completion candidate in the displayed
>> +*Completions* window, `minibuffer-completion-help' will put point on the
>> +same candidate after updating *Completions*."
>
> This is completely unclear: what does minibuffer-completion-help have
> to do with updating *Completions*?

That's fair.  minibuffer-completion-help is the function which updates
*Completions*, but a user doesn't necessarily know that.  Will clarify
in next version.

>> +             (current-candidate-and-offset
>> +              (when-let* ((window (get-buffer-window "*Completions*" 0)))
>> +                (with-selected-window window
>> +                  (when-let* ((beg (completions--start-of-candidate-at (point))))
>> +
>> +                    (cons (get-text-property beg 'completion--string) (- (point) beg))))))
>
> Why is this done unconditionally?  with-selected-window is not a cheap
> function.

Will avoid the with-selected-window in the next version - a
with-current-buffer is sufficient.

>> @@ -4905,8 +4928,6 @@ minibuffer-next-completion
>>    (interactive "p")
>>    (let ((auto-choose minibuffer-completion-auto-choose))
>>      (with-minibuffer-completions-window
>> -      (when completions-highlight-face
>> -        (setq-local cursor-face-highlight-nonselected-window t))
>
> Why is this being moved to a different place?

See my reply to Stefan.

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Shouldn't we stop complicating the completion machinery at some point?
>
> Complexity knows no bounds.
>
> OT1H I agree that the completion infrastructure would benefit from some
> cleanup and is too complex in some areas.  OTOH this specific request
> doesn't affect the "completion machinery", but only the
> `minibuffer-completion-help` functionality which is a fairly simple part
> of the code.
>
>> what's going on is to step with a debugger through the code -- and it
>> doesn't help that some of the code is in Lisp and some in C, so Lisp
>> calls into C, which calls back into Lisp, etc., thus one needs to
>> interrupt the stepping, fire up a different debugger, then go back.
>
> I luckily haven't had to step through the C code of the completion in
> a long while.  But the code layout could be improved to better reflect
> the architecture of the system (the separation between the "backend"
> (completion tables), the "middle end" (completion styles), the standard
> minibuffer UI, the in-buffer UI, the completion-list-mode).

Since pretty much everything completion related is in minibuffer.el, I
wonder if it would be useful to introduce a file separate from
minibuffer.el for things which *aren't* related to completion?  There
are a few of those things in minibuffer.el, and it might help to
maintain a stricter separation.

And as I mentioned elsewhere, maybe we could move completion-list-mode
into minibuffer.el?

Then we could be very clear in the commentary of minibuffer.el that
despite its name, it is the file where all the completion machinery
lives, in or out of the minibuffer.

(It would be really nice to be able to just eval minibuffer.el to pick
up changes to completion, instead of also having to eval simple.el...)

>> Stefan, WDYT?  Should we close completion to further development and
>> accept only bugfixes?
>
> 🙂
>
>
>         Stefan

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

>> Hmm... are we sure here that the `*Completions*`s content is related to
>> the current completion session?  I don't think we want to preserve the
>> selection when it came from an unrelated use of completion half an
>> hour earlier.
>
> That's why I'm doing get-buffer-window here - I figure that if
> *Completions* is currently displayed in a window, it's reasonable to
> preserve the selected candidate.
>
> (The selected candidate in that window, I guess - so maybe I should use
> window-point here?)
>
> It still might not be related to the current completion session, since
> the user might have just manually switched buffers to *Completions*, but
> I wasn't sure there was a good way to determine that... any suggestions?

Indeed, it might not be related.  E.g in my setup, `*Completions*` is
placed in a dedicated window&frame that is simply iconified/deiconified
as needed, so your heuristic would fail.

I think we need to store in the *Completions* buffer some info about the
completion session for which it was last used.  Or set some buffer-local
var that indicates that the corresponding completion session is "done"
(we can do that when we hide the *Completions* buffer, when we exit the
minibuffer or the `completion-in-region-mode`).

> This is kind of an unrelated improvement, since this is probably nicer
> anyway - if the user moves point around manually in *Completions*, IMO
> that should have the same behavior as minibuffer-next-completion, but
> currently it doesn't highlight the same way if they leave *Completions*
> because cursor-face-highlight-nonselected-window doesn't get set.

I see, thanks.


        Stefan

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Hmm... are we sure here that the `*Completions*`s content is related to
>>> the current completion session?  I don't think we want to preserve the
>>> selection when it came from an unrelated use of completion half an
>>> hour earlier.
>>
>> That's why I'm doing get-buffer-window here - I figure that if
>> *Completions* is currently displayed in a window, it's reasonable to
>> preserve the selected candidate.
>>
>> (The selected candidate in that window, I guess - so maybe I should use
>> window-point here?)
>>
>> It still might not be related to the current completion session, since
>> the user might have just manually switched buffers to *Completions*, but
>> I wasn't sure there was a good way to determine that... any suggestions?
>
> Indeed, it might not be related.  E.g in my setup, `*Completions*` is
> placed in a dedicated window&frame that is simply iconified/deiconified
> as needed, so your heuristic would fail.

(Interesting, is the code for that published somewhere?  What's the
motivation for that instead of a window?)

> I think we need to store in the *Completions* buffer some info about the
> completion session for which it was last used.  Or set some buffer-local
> var that indicates that the corresponding completion session is "done"
> (we can do that when we hide the *Completions* buffer, when we exit the
> minibuffer or the `completion-in-region-mode`).

It would be nice to store info about the completion session, but we
don't really have any concrete concept of "a completion session" right
now, right?  So it's not clear to me how we would keep track of when it
starts and ends.

Also, I think we sometimes want to avoid reusing *Completions* even
within a completion session: if we're using completion in a directory
tree containing a file /aaa/aaa, if we select "aaa" while completing on
"/a", then "aaa" shouldn't still be selected if we complete on "/aaa/a".

Do you think it would be sufficient to set a "done" buffer-local in
minibuffer-hide-completions?  That seems to be called in all the right
places.

Or maybe instead of a buffer-local, we could have
minibuffer-hide-completions do (goto-char (point-min)) so no completion
is selected anymore.  Then it becomes harmless to reuse that
*Completions* buffer.

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

>> Indeed, it might not be related.  E.g in my setup, `*Completions*` is
>> placed in a dedicated window&frame that is simply iconified/deiconified
>> as needed, so your heuristic would fail.
> (Interesting, is the code for that published somewhere?

No, it's just in my init file.

> What's the motivation for that instead of a window?)

My minibuffer is in its own frame so I need another frame to display
the completions.

> It would be nice to store info about the completion session, but we
> don't really have any concrete concept of "a completion session" right
> now, right?

That's right.  🙁

> Also, I think we sometimes want to avoid reusing *Completions* even
> within a completion session: if we're using completion in a directory
> tree containing a file /aaa/aaa, if we select "aaa" while completing on
> "/a", then "aaa" shouldn't still be selected if we complete on "/aaa/a".

I believe we can catch most of those problems by paying attention to
changes in `completion-base-position`.

[ Tho I think those cases are less problematic, because presumably the
  "old" selection happened recently enough that the users shouldn't be
  completely surprised if it's still selected (even though they may
  consider it as a misfeature).  ]

> Do you think it would be sufficient to set a "done" buffer-local in
> minibuffer-hide-completions?  That seems to be called in all the right
> places.

Yes, I believe that should do it.

> Or maybe instead of a buffer-local, we could have
> minibuffer-hide-completions do (goto-char (point-min)) so no
> completion is selected anymore.  Then it becomes harmless to reuse
> that *Completions* buffer.

Either way works for me.


        Stefan

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Or maybe instead of a buffer-local, we could have
>> minibuffer-hide-completions do (goto-char (point-min)) so no
>> completion is selected anymore.  Then it becomes harmless to reuse
>> that *Completions* buffer.
>
> Either way works for me.

OK, updated patch with this and other feedback:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Preserve-selected-candidate-across-Completions-updat.patch --]
[-- Type: text/x-patch, Size: 8798 bytes --]

From c9b1336692c095e4f8423797e58aac9fe55325fb Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 29 Oct 2024 12:16:31 -0400
Subject: [PATCH] Preserve selected candidate across *Completions* update

When *Completions* is updated and point was on some completion
candidate, move point to the same candidate after the update.

Also, a selected completion in *Completions* is now always
highlighted, even if it was selected by the user or other code
moving point rather than by minibuffer-next-completion, because
cursor-face-highlight-nonselected-window is now set in
completion-setup-function.

Other completion UIs (e.g. ido, vertico, etc) effectively have
this behavior: whenever they update the list of completions,
they preserve whatever candidate is selected.  This matters a
lot when completions are auto-updated, but is still useful
without auto-updating.  Including this behavior is a step
towards supporting auto-updating in the default completion UI.

* lisp/minibuffer.el (minibuffer-completion-help): Preserve the
selected completion candidate across updates. (bug#74019)
(minibuffer-hide-completions): Move point to BOB.
(minibuffer-next-completion): Don't set
cursor-face-highlight-nonselected-window.
* lisp/simple.el (completions--start-of-candidate-at)
(choose-completion): Extract the current-completion-finding code
into a separate function.
(completion-setup-function): Set
cursor-face-highlight-nonselected-window.
* etc/NEWS: Announce new behavior.
---
 etc/NEWS           |  8 ++++++++
 lisp/minibuffer.el | 29 +++++++++++++++++++++++------
 lisp/simple.el     | 39 ++++++++++++++++++++++++---------------
 3 files changed, 55 insertions(+), 21 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 18b6678dce9..ad85bbf116e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -86,6 +86,14 @@ different values for completion-affecting variables like
 applies for the styles configuration in 'completion-category-overrides'
 and 'completion-category-defaults'.
 
+---
+*** Selected completion candidate is preserved across *Completions* updates.
+When point is on a completion candidate in the *Completions* buffer
+(because of 'minibuffer-next-completion' or for any other reason), point
+will still be on that candidate after *Completions* is updated with a
+new list of completions.  The candidate is automatically deselected when
+the *Completions* buffer is hidden.
+
 ** Windows
 
 +++
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 44d07557f48..9b498615926 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2624,6 +2624,12 @@ minibuffer-completion-help
              (sort-fun (completion-metadata-get all-md 'display-sort-function))
              (group-fun (completion-metadata-get all-md 'group-function))
              (mainbuf (current-buffer))
+             (current-candidate-and-offset
+              (when-let* ((buffer (get-buffer "*Completions*"))
+                          (window (get-buffer-window buffer 0)))
+                (with-current-buffer buffer
+                  (when-let* ((beg (completions--start-of-candidate-at (window-point window))))
+                    (cons (get-text-property beg 'completion--string) (- (point) beg))))))
              ;; If the *Completions* buffer is shown in a new
              ;; window, mark it as softly-dedicated, so bury-buffer in
              ;; minibuffer-hide-completions will know whether to
@@ -2647,7 +2653,7 @@ minibuffer-completion-help
             ,(when temp-buffer-resize-mode
                '(preserve-size . (nil . t)))
             (body-function
-             . ,#'(lambda (_window)
+             . ,#'(lambda (window)
                     (with-current-buffer mainbuf
                       (when completion-auto-deselect
                         (add-hook 'after-change-functions #'completions--after-change nil t))
@@ -2737,7 +2743,16 @@ minibuffer-completion-help
                                                      (if (eq (car bounds) (length result))
                                                          'exact 'finished))))))
 
-                      (display-completion-list completions nil group-fun)))))
+                      (display-completion-list completions nil group-fun)
+                      (when current-candidate-and-offset
+                        (with-current-buffer standard-output
+                          (when-let* ((match (text-property-search-forward
+                                              'completion--string (car current-candidate-and-offset) t)))
+                            (goto-char (prop-match-beginning match))
+                            ;; Preserve the exact offset for the sake of
+                            ;; `choose-completion-deselect-if-after'.
+                            (forward-char (cdr current-candidate-and-offset))
+                            (set-window-point window (point)))))))))
           nil)))
     nil))
 
@@ -2746,8 +2761,12 @@ minibuffer-hide-completions
   ;; FIXME: We could/should use minibuffer-scroll-window here, but it
   ;; can also point to the minibuffer-parent-window, so it's a bit tricky.
   (interactive)
-  (let ((win (get-buffer-window "*Completions*" 0)))
-    (if win (with-selected-window win (bury-buffer)))))
+  (when-let* ((win (get-buffer-window "*Completions*" 0)))
+    (with-selected-window win
+      ;; Move point off any completions, so we don't move point there
+      ;; again the next time `minibuffer-completion-help' is called.
+      (goto-char (point-min))
+      (bury-buffer))))
 
 (defun exit-minibuffer ()
   "Terminate this minibuffer argument."
@@ -4905,8 +4924,6 @@ minibuffer-next-completion
   (interactive "p")
   (let ((auto-choose minibuffer-completion-auto-choose))
     (with-minibuffer-completions-window
-      (when completions-highlight-face
-        (setq-local cursor-face-highlight-nonselected-window t))
       (if vertical
           (next-line-completion (or n 1))
         (next-completion (or n 1)))
diff --git a/lisp/simple.el b/lisp/simple.el
index 2ffd6e86e56..3a142ef14b3 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10246,6 +10246,23 @@ choose-completion-deselect-if-after
 
 This makes `completions--deselect' effective.")
 
+(defun completions--start-of-candidate-at (position)
+  "Return the start position of the completion candidate at POSITION."
+  (save-excursion
+    (goto-char position)
+    (let (beg)
+      (cond
+       ((and (not (eobp))
+             (get-text-property (point) 'completion--string))
+        (setq beg (1+ (point))))
+       ((and (not (bobp))
+             (get-text-property (1- (point)) 'completion--string))
+        (setq beg (point))))
+      (when beg
+        (or (previous-single-property-change
+             beg 'completion--string)
+            beg)))))
+
 (defun choose-completion (&optional event no-exit no-quit)
   "Choose the completion at point.
 If EVENT, use EVENT's position to determine the starting position.
@@ -10269,21 +10286,11 @@ choose-completion
                (or (get-text-property (posn-point (event-start event))
                                       'completion--string)
                    (error "No completion here"))
-           (save-excursion
-             (goto-char (posn-point (event-start event)))
-             (let (beg)
-               (cond
-                ((and (not (eobp))
-                      (get-text-property (point) 'completion--string))
-                 (setq beg (1+ (point))))
-                ((and (not (bobp))
-                      (get-text-property (1- (point)) 'completion--string))
-                 (setq beg (point)))
-                (t (error "No completion here")))
-               (setq beg (or (previous-single-property-change
-                              beg 'completion--string)
-                             beg))
-               (get-text-property beg 'completion--string))))))
+             (if-let* ((candidate-start
+                        (completions--start-of-candidate-at
+                         (posn-point (event-start event)))))
+                 (get-text-property candidate-start 'completion--string)
+               (error "No completion here")))))
 
       (unless (buffer-live-p buffer)
         (error "Destination buffer is dead"))
@@ -10451,6 +10458,8 @@ completion-setup-function
       (let ((base-position completion-base-position)
             (insert-fun completion-list-insert-choice-function))
         (completion-list-mode)
+        (when completions-highlight-face
+          (setq-local cursor-face-highlight-nonselected-window t))
         (setq-local completion-base-position base-position)
         (setq-local completion-list-insert-choice-function insert-fun))
       (setq-local completion-reference-buffer mainbuf)
-- 
2.39.3

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

> OK, updated patch with this and other feedback:

Looks OK to me, thank you.


        Stefan

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

Spencer Baugh <sbaugh@janestreet.com> writes:
> OK, updated patch with this and other feedback:

Stefan is okay with this patch and there's been no more feedback; could
we install this now?

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

> OK, updated patch with this and other feedback:

Thanks, pushed to `master`.


        Stefan

bug#74019: [PATCH] Optionally preserve selected candidate across *Completions* update

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

> Thanks, pushed to `master`.

Closing,


        Stefan