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

[Juri Linkov <juri@linkov.net>]

>>>> 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.

>>>> 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.
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

> 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.