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

[Spencer Baugh via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

Dmitry Gutov <dmitry@gutov.dev> writes:

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

I think this is preferable, though, and even if it isn't, it should
still be supported.

This keeps it explicit to the user exactly what text will be inserted
into the buffer upon accepting a completion.

Consider these two cases of completion (| representing point):

A. switch-to-|asdf

B. switch-to-|buffer

In both cases, switch-to-next-buffer is a potential completion, provided
by "emacs22" in the case A and "basic" in case B.

But in case A, if switch-to-next-buffer is chosen, the "asdf" should be
preserved, and in case B, if switch-to-next-buffer, the "buffer" should
be deleted.

Because they have different behavior, they should appear differently to
the user.  That's why I think case A should show
"switch-to-next-bufferasdf" in the *Completions* buffer, with "asdf"
grayed out via a "completions-ignored" face.

If a user decides they don't want "asdf" text to be shown in case A,
then they can customize that.  If a frontend decides it doesn't want to
show that text, it can omit the text.

But it should at least be *possible* for the text to be shown.
Otherwise there's no way to distinguish the two cases.

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

That's not an important aspect, I've removed that dependency in my
latest version.

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

Yes, and I indeed think this feature is useful on its own, because it
allows choose-completion to be fully equivalent to
completion-try-completion.  Right now completion-try-completion can
change point, but choose-completion can't.  That's limiting for a bunch
of reasons, and the inability to fix this bug purely in a completion
style is one of them.

So I think we should just go ahead and implement this feature.  And if
we do so, it makes it possible to fix this bug with only changes inside
a completion style.

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

Your patch is simple, and it works for default completion, but two
issues:

- Your patch doesn't distinguish the two cases A and B that I described above.
  
- Your patch will require company-mode and other frontends to change.
  My patch does not strictly require that.

  But if we're already requiring that, I think we should take the
  opportunity to add the more powerful feature
  completion-position-after-insert.

Here's a simplified version of my earlier patch, which modifies the
emacs22 style to make it easier to discuss.  I think this is equally
simple as your patch, but it:

- Distinguishes the cases A and B by including the ignored text in the
  completion, just grayed out.

- Fixes the bug for other frontends without requiring them to change
  (they get additional benefit when they change to support
  completion-position-after-insert)

Note that due to a separate bug in completion--twq-all, used in filename
completion, the graying-out face is dropped from the completion
candidates before they reach *Completions*; so if you try this, try it
by e.g. completing Lisp symbols.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Preserve-suffix-in-emacs22-style.patch --]
[-- Type: text/x-patch, Size: 2567 bytes --]

From dabd42adc78ef3c3e8f40f913325941246993628 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Wed, 18 Sep 2024 08:52:54 -0400
Subject: [PATCH] Preserve suffix in emacs22 style

---
 lisp/minibuffer.el | 14 ++++++++++++--
 lisp/simple.el     | 11 +++++++----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 804afe9cb43..a28041751aa 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3771,10 +3771,20 @@ completion-emacs22-try-completion
           (setq suffix (substring suffix 1)))
       (cons (concat completion suffix) (length completion))))))
 
+(defface completions-ignored
+  '((t (:inherit shadow)))
+  "Face for text which was ignored by the completion style.")
+
 (defun completion-emacs22-all-completions (string table pred point)
-  (let ((beforepoint (substring string 0 point)))
+  (let ((beforepoint (substring string 0 point))
+        (suffix (propertize (substring string point) 'face 'completions-ignored)))
     (completion-hilit-commonality
-     (all-completions beforepoint table pred)
+     (mapcar
+      (lambda (elem)
+        (let ((s (concat elem suffix)))
+          (put-text-property 0 1 'completion-position-after-insert (length elem) s)
+          s))
+      (all-completions beforepoint table pred))
      point
      (car (completion-boundaries beforepoint table pred "")))))
 
diff --git a/lisp/simple.el b/lisp/simple.el
index e35cfe0479b..881eb51e57a 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10371,10 +10371,13 @@ choose-completion-string
         ;; comes from buffer-substring-no-properties.
         ;;(remove-text-properties 0 (length choice) '(mouse-face nil) choice)
 	;; Insert the completion into the buffer where it was requested.
-        (funcall (or insert-function completion-list-insert-choice-function)
-                 (or (car base-position) (point))
-                 (or (cadr base-position) (point))
-                 choice)
+        (let ((beg (or (car base-position) (point)))
+              (end (or (cadr base-position) (point))))
+          (funcall (or insert-function completion-list-insert-choice-function)
+                   beg end choice)
+          (unless (string-empty-p choice)
+            (when-let ((pos (get-text-property 0 'completion-position-after-insert choice)))
+              (goto-char (+ pos beg)))))
         ;; Update point in the window that BUFFER is showing in.
 	(let ((window (get-buffer-window buffer t)))
 	  (set-window-point window (point)))
-- 
2.39.3