bug#62700: 29.0.60; minibuffer-{previous,next,choose}-completion behave unintuitively when point is not at end of buffer

[Spencer Baugh <sbaugh@janestreet.com>]

Juri Linkov <juri@linkov.net> writes:
>>>>> It just needs more testing for different categories of completion.
>>>>
>>>> Which categories do you have in mind?
>>>
>>> Actually, I can't find categories where it could fail.
>>> So your patch looks safe to push.
>>
>> Can we go ahead and push it to Emacs master, then?  I will work on the
>> changing-only-new-code backport for Emacs 29 as Eli requested.
>
> But your patch changes only new code.

Ah, I thought Eli still wanted a backport version because this changes
code which has been on Emacs 29 for over a year.

I'm happy either way.  (well, of course I prefer to not make a backport
version, but happy to do it if Eli wants one)

>>>>> Maybe you could find another heuristic for insertion of completion?
>>>>> The code is located in the same function 'minibuffer-completion-help':
>>>>>
>>>>>   (if (and (stringp start) (stringp end))
>>>>>       (progn
>>>>>         (delete-minibuffer-contents)
>>>>>         (insert start choice)
>>>>>         ;; Keep point after completion before suffix
>>>>>         (save-excursion (insert end)))
>>>>>
>>>>> Currently it keeps point before the suffix.
>>>>
>>>> I will try. Although this is a case where completion-base-position feels
>>>> more suited than completion-base-affixes...
>>>
>>> Can you get the same info about positions by calculating the
>>> lengths of prefix/choice/suffix?
>>
>> Hm I have thought about it but I can't see a simple heuristic.
>>
>> It's not actually clear what behavior we want, anyway.  When TAB
>> completes a string fully, it sends point to the end of the buffer.  This
>> happens even if completion-cycle-threshold is non-nil, and
>> completion-cycle-threashold feels like a pretty similar feature to
>> minibuffer-{previous,next}-completion. So maybe that's correct for us to
>> do here too?
>>
>> But a different behavior feels like it could also makes sense.  For
>> example, if I'm completing from ffap-|-path (| is point), I'm just
>> cycling between ffap-bib-path, ffap-c++-path, ffap-c-path, and it feels
>> like as I cycle through those, point should stay right before "-path",
>> like ffap-bib|-path, ffap-c++|-path, ffap-c|-path.  No idea how to
>> achieve this behavior though.
>
> This also makes sense: ffap-|bib-path, ffap-|c++-path, ffap-|c-path.

Agreed.

> I tried it with this tentative patch and it feels quite natural,
> so maybe could be turned into an option:
>
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index a6af65dfa14..733f7710378 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -2366,6 +2371,7 @@ minibuffer-completion-help
>    (let* ((start (or start (minibuffer--completion-prompt-end)))
>           (end (or end (point-max)))
>           (string (buffer-substring start end))
> +         (pos (1- (point)))
>           (md (completion--field-metadata start))
>           (completions (completion-all-completions
>                         string
> @@ -2493,7 +2503,8 @@ minibuffer-completion-help
>                                         (delete-minibuffer-contents)
>                                         (insert start choice)
>                                         ;; Keep point after completion before suffix
> -                                       (save-excursion (insert end)))
> +                                       (save-excursion (insert end))
> +                                       (move-to-column pos))
>                                     (unless (or (zerop (length prefix))
>                                                 (equal prefix
>                                                        (buffer-substring-no-properties

Interesting idea.  Although it breaks down with ?, I notice:
1. C-h v ff-|-p
2. ? to pop up completions
3. M-<down> to select diff-font-lock-prettify
4. Get dif|f-font-lock-prettify which seems fairly wrong

>> Anyway, the behavior with my earlier patch now feels fine to me, I don't
>> think we need any improvements to point's behavior for now.
>
> Maybe your patch still could be pushed to emacs-29 because it fixes
> the new feature.

Agreed.