bug#70968: 29.2.50; choose-completion on an emacs22-style completion deletes text after point

[Dmitry Gutov <dmitry@gutov.dev>]

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

Hi Spencer,

On 16/09/2024 22:54, Spencer Baugh wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>> I'm okay with adding a new style, per B, but why do we need to
>> deprecate emacs22 at the same time?  Let users who want this new
>> behavior customize their completion styles to use this new style
>> instead of emacs22.  That'd be fully backward compatible, and will
>> solve the problem for those who don't like the current behavior.
> 
> That's fair enough, we don't need to deprecate emacs22 at the same time,
> we can wait until the new style has been battle-tested.
> 
> I think a new style should replace emacs22 in the default
> completion-styles eventually, but that can wait.
> 
> And after working on this bug for a while, I now am convinced that the
> new style approach is straightforward, and is the best way.  (Although
> it would also work to make these changes in the emacs22 style)

I'm not quite convinced about the "new style only" approach myself, but 
anyway we can discuss a solution that could be applied to any style, 
opt-in or opt-out.

> Attached is a patch which adds a new ignore-after-point style.  The fix
> for this bug is entirely contained in the differences between
> completion-ignore-after-point-all-completions (c-iap-a-c) and
> completion-emacs22-all-completions (c-e22-a-c).
> 
> c-e22-a-c always omits the text after point, which means
> choose-completion always deletes that text, causing this bug.
> 
> c-iap-a-c includes the text after point in some cases.  Whenever the
> text after point is included, it's grayed out with a new
> completions-ignored face to indicate it was ignored.
> 
> The cases where c-iap-a-c includes the text after point are those where
> the user will have further chances to edit or complete with that text:
> 
> - When we're doing minibuffer completion and choose-completion will
>    immediately exit the minibuffer, the text after point is not included,
>    since the user won't get any further changes to use it, and it's
>    probably garbage.
> 
> - When we're doing completion-at-point (or certain kinds of minibuffer
>    completion, e.g. completing a directory in filename completion), the
>    text after point is included.  In such cases, the user can always
>    delete it themselves later, or might want to actually use it.
> 
> To make this work consistently with completion-try-completion (which
> keeps point before the ignored suffix in both the ignore-after-point and
> emacs22 styles), choose-completion-string now checks a new
> 'completion-position-after-insert text property, and moves point to that
> position.
> 
> (There are two other changes which are general improvements unrelated to
> this bug:
> 
> - The behavior of keeping point before the ignored suffix is more
>    consistent.
> 
> - Instead of hardcoding try-completion and all-completion,
>    ignore-after-point uses the configured completion styles.)

Thank you, I see a few problems with that approach (as discussed 
privately as well). To recap:

 From my POV integrating the result with company-mode is non-trivial. 
And the created visual doesn't seem optimal in a number of edge cases 
(long prefix = weird-looking popup; this probably applies to the 
*Completions* buffer as well, though maybe to a lesser extent).

Another problem is both the "all-completions" method and the insertion 
function call out to UI: choose-completion-string--should-exit 
references minibuffer-completion-table and completion-no-auto-exit 
directly, for example. I'm on the fence about coupling to the completion 
category.

Finally, the use of 'completion-position-after-insert' seems like it 
could be used separately from the "ignored text", meaning the spans 
don't have to match. So it could be a separate feature, one that's easy 
enough to implement on its own.

None of the above would be insurmountable, but here's what I think 
avoids it using the 'completion--adjust-metadata' thingy that was 
originally added for 'flex' a few releases ago: adding a metadata key 
'completion-ignore-after-point'.

The attached patch does not make a distinction for file name completion 
- it just covers the core problem - but I think any UI could use the 
addition and likewise lookup the 'file' category, and print the "hidden" 
suffix in the Completions, and decide to drop the suffix in your first 
scenario (file name completion with exit imminent).

Spencer, Stefan, WDYT?

Again, the patch is against the emacs22 style for readability, but a new 
style can be added just as well.

[-- Attachment #2: completion-ignore-after-point-metadata.diff --]
[-- Type: text/x-patch, Size: 1878 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 804afe9cb43..4ac231171b6 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2613,7 +2613,6 @@ minibuffer-completion-help
                                              (buffer-substring (point) end))))
                 (point)))
              (field-char (and (< field-end end) (char-after field-end)))
-             (base-position (list (+ start base-size) field-end))
              (all-md (completion--metadata (buffer-substring-no-properties
                                             start (point))
                                            base-size md
@@ -2623,6 +2622,12 @@ minibuffer-completion-help
              (aff-fun (completion-metadata-get all-md 'affixation-function))
              (sort-fun (completion-metadata-get all-md 'display-sort-function))
              (group-fun (completion-metadata-get all-md 'group-function))
+             (ignore-after-point (completion-metadata-get all-md
+                                                          'ignore-after-point))
+             (base-position (list (+ start base-size)
+                                  (if ignore-after-point
+                                      (point)
+                                    field-end)))
              (mainbuf (current-buffer))
              ;; If the *Completions* buffer is shown in a new
              ;; window, mark it as softly-dedicated, so bury-buffer in
@@ -3778,6 +3783,13 @@ completion-emacs22-all-completions
      point
      (car (completion-boundaries beforepoint table pred "")))))
 
+(put 'emacs22 'completion--adjust-metadata 'completion--emacs22-adjust-metadata)
+
+(defun completion--emacs22-adjust-metadata (metadata)
+  `(metadata
+    (ignore-after-point . t)
+    ,@(cdr metadata)))
+
 ;;; Basic completion.
 
 (defun completion--merge-suffix (completion point suffix)