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

[Spencer Baugh]

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.