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

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

[Spencer Baugh]

try-completion and choose-completion have different behavior; with the
emacs22 completion style, the former preserves the text after point
(while ignoring it), whereas the latter deletes the text after point.

1. emacs -Q

2. In scratch, type "inhibit-asdf" and move point to after the "-"

3. Type "q" and M-<tab>
   The buffer now contains "inhibit-quitasdf", because the emacs22 style
   ignored the text after point and completed on "inhibit-q".

4. C-/ C-/ to change the buffer back to "inhibit-asdf"

5. M-<tab>
   The *Completions* buffer will be displayed, containing among others
   "inhibit-quit" as a completion, because the emacs22 style ignored the
   text after point and completed on "inhibit-".

6. Use M-<down> and M-<ret> to select and choose "inhibit-quit"
   The buffer now contains "inhibit-quit".

The two ways of selecting completions should behave the same.

I suggest that the behavior of not deleting the text after point is
better.  The emacs22 style takes care to not delete the text after point
in try-completion and in completion cycling; we should take similar care
in choose-completion.

This is especially noticeable when using minibuffer-visible-completions,
which make it easier to use choose-completion.  It's also especially
noticeable with corfu-mode, which usually automatically calls
choose-completion when finishing completion.

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

[Eli Zaretskii]

> Cc: dmitry@gutov.dev, juri@linkov.net
> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Wed, 15 May 2024 16:26:50 -0400
> 
> 
> try-completion and choose-completion have different behavior; with the
> emacs22 completion style, the former preserves the text after point
> (while ignoring it), whereas the latter deletes the text after point.
> 
> 1. emacs -Q
> 
> 2. In scratch, type "inhibit-asdf" and move point to after the "-"
> 
> 3. Type "q" and M-<tab>
>    The buffer now contains "inhibit-quitasdf", because the emacs22 style
>    ignored the text after point and completed on "inhibit-q".
> 
> 4. C-/ C-/ to change the buffer back to "inhibit-asdf"
> 
> 5. M-<tab>
>    The *Completions* buffer will be displayed, containing among others
>    "inhibit-quit" as a completion, because the emacs22 style ignored the
>    text after point and completed on "inhibit-".
> 
> 6. Use M-<down> and M-<ret> to select and choose "inhibit-quit"
>    The buffer now contains "inhibit-quit".
> 
> The two ways of selecting completions should behave the same.
> 
> I suggest that the behavior of not deleting the text after point is
> better.  The emacs22 style takes care to not delete the text after point
> in try-completion and in completion cycling; we should take similar care
> in choose-completion.

If making these two ways consistent means we need to change the
behavior of emacs22 style of completion, then I'm sorry, but we cannot
do that.  This style is a legacy style, and is there to provide the
legacy behavior for those who need it.

So for making the decisions in this case we need to know how did Emacs
22 and later versions behave in the above scenario, and take it from
there.  If they all behaved like you show, then this behavior must
stay unchanged, unfortunately, for backward-compatibility reasons.

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

[Dmitry Gutov]

On 16/05/2024 11:13, Eli Zaretskii wrote:
>> Cc: dmitry@gutov.dev, juri@linkov.net
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Date: Wed, 15 May 2024 16:26:50 -0400
>>
>>
>> try-completion and choose-completion have different behavior; with the
>> emacs22 completion style, the former preserves the text after point
>> (while ignoring it), whereas the latter deletes the text after point.
>>
>> 1. emacs -Q
>>
>> 2. In scratch, type "inhibit-asdf" and move point to after the "-"
>>
>> 3. Type "q" and M-<tab>
>>     The buffer now contains "inhibit-quitasdf", because the emacs22 style
>>     ignored the text after point and completed on "inhibit-q".
>>
>> 4. C-/ C-/ to change the buffer back to "inhibit-asdf"
>>
>> 5. M-<tab>
>>     The *Completions* buffer will be displayed, containing among others
>>     "inhibit-quit" as a completion, because the emacs22 style ignored the
>>     text after point and completed on "inhibit-".
>>
>> 6. Use M-<down> and M-<ret> to select and choose "inhibit-quit"
>>     The buffer now contains "inhibit-quit".
>>
>> The two ways of selecting completions should behave the same.
>>
>> I suggest that the behavior of not deleting the text after point is
>> better.  The emacs22 style takes care to not delete the text after point
>> in try-completion and in completion cycling; we should take similar care
>> in choose-completion.
> 
> If making these two ways consistent means we need to change the
> behavior of emacs22 style of completion, then I'm sorry, but we cannot
> do that.  This style is a legacy style, and is there to provide the
> legacy behavior for those who need it.

I don't think that would be required exactly.

The problem here (IIUC) is that completion behaves differently with the 
emacs22 style depending on whether the execution path went through 
choose-completion (which is not a method of completion style but a 
common subroutine) or not (when completion--do-completion performed 
expansion).

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

[Eli Zaretskii]

> Date: Thu, 16 May 2024 20:26:31 +0300
> Cc: 70968@debbugs.gnu.org, juri@linkov.net,
>  Stefan Monnier <monnier@IRO.UMontreal.CA>
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 16/05/2024 11:13, Eli Zaretskii wrote:
> >> Cc: dmitry@gutov.dev, juri@linkov.net
> >> From: Spencer Baugh <sbaugh@janestreet.com>
> >> Date: Wed, 15 May 2024 16:26:50 -0400
> >>
> >>
> >> try-completion and choose-completion have different behavior; with the
> >> emacs22 completion style, the former preserves the text after point
> >> (while ignoring it), whereas the latter deletes the text after point.
> >>
> >> 1. emacs -Q
> >>
> >> 2. In scratch, type "inhibit-asdf" and move point to after the "-"
> >>
> >> 3. Type "q" and M-<tab>
> >>     The buffer now contains "inhibit-quitasdf", because the emacs22 style
> >>     ignored the text after point and completed on "inhibit-q".
> >>
> >> 4. C-/ C-/ to change the buffer back to "inhibit-asdf"
> >>
> >> 5. M-<tab>
> >>     The *Completions* buffer will be displayed, containing among others
> >>     "inhibit-quit" as a completion, because the emacs22 style ignored the
> >>     text after point and completed on "inhibit-".
> >>
> >> 6. Use M-<down> and M-<ret> to select and choose "inhibit-quit"
> >>     The buffer now contains "inhibit-quit".
> >>
> >> The two ways of selecting completions should behave the same.
> >>
> >> I suggest that the behavior of not deleting the text after point is
> >> better.  The emacs22 style takes care to not delete the text after point
> >> in try-completion and in completion cycling; we should take similar care
> >> in choose-completion.
> > 
> > If making these two ways consistent means we need to change the
> > behavior of emacs22 style of completion, then I'm sorry, but we cannot
> > do that.  This style is a legacy style, and is there to provide the
> > legacy behavior for those who need it.
> 
> I don't think that would be required exactly.
> 
> The problem here (IIUC) is that completion behaves differently with the 
> emacs22 style depending on whether the execution path went through 
> choose-completion (which is not a method of completion style but a 
> common subroutine) or not (when completion--do-completion performed 
> expansion).

I understand that much.  But what did these two (or their
then-equivalents) do in Emacs 22 and Emacs 23?

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

[Dmitry Gutov]

On 16/05/2024 21:25, Eli Zaretskii wrote:
>> I don't think that would be required exactly.
>>
>> The problem here (IIUC) is that completion behaves differently with the
>> emacs22 style depending on whether the execution path went through
>> choose-completion (which is not a method of completion style but a
>> common subroutine) or not (when completion--do-completion performed
>> expansion).
> I understand that much.  But what did these two (or their
> then-equivalents) do in Emacs 22 and Emacs 23?

I'm guessing they behaved incorrectly (or however we want to call the 
inconsistent behavior), but I don't have a compiled Emacs 22/23 around, 
and they might be difficult to build.

Note that we fixed bug#48356 not too long ago, which is from the same 
general area, and it probably originated from before Emacs 22/23 too.

It's worth looking for edge cases where we'd strongly prefer the current 
behavior, and they might exist, but so far I only know of situations 
where the change would be for the better, or the user might be okay with 
either (example at the end of https://debbugs.gnu.org/72705#35).

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

[Eli Zaretskii]

> Date: Mon, 26 Aug 2024 03:08:56 +0300
> Cc: sbaugh@janestreet.com, 70968@debbugs.gnu.org, juri@linkov.net,
>  monnier@IRO.UMontreal.CA
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 16/05/2024 21:25, Eli Zaretskii wrote:
> >> I don't think that would be required exactly.
> >>
> >> The problem here (IIUC) is that completion behaves differently with the
> >> emacs22 style depending on whether the execution path went through
> >> choose-completion (which is not a method of completion style but a
> >> common subroutine) or not (when completion--do-completion performed
> >> expansion).
> > I understand that much.  But what did these two (or their
> > then-equivalents) do in Emacs 22 and Emacs 23?
> 
> I'm guessing they behaved incorrectly (or however we want to call the 
> inconsistent behavior), but I don't have a compiled Emacs 22/23 around, 
> and they might be difficult to build.
> 
> Note that we fixed bug#48356 not too long ago, which is from the same 
> general area, and it probably originated from before Emacs 22/23 too.
> 
> It's worth looking for edge cases where we'd strongly prefer the current 
> behavior, and they might exist, but so far I only know of situations 
> where the change would be for the better, or the user might be okay with 
> either (example at the end of https://debbugs.gnu.org/72705#35).

Ping!  How should we proceed with this bug report?

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

[Dmitry Gutov]

On 07/09/2024 10:30, Eli Zaretskii wrote:
>> Date: Mon, 26 Aug 2024 03:08:56 +0300
>> Cc:sbaugh@janestreet.com,70968@debbugs.gnu.org,juri@linkov.net,
>>   monnier@IRO.UMontreal.CA
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> On 16/05/2024 21:25, Eli Zaretskii wrote:
>>>> I don't think that would be required exactly.
>>>>
>>>> The problem here (IIUC) is that completion behaves differently with the
>>>> emacs22 style depending on whether the execution path went through
>>>> choose-completion (which is not a method of completion style but a
>>>> common subroutine) or not (when completion--do-completion performed
>>>> expansion).
>>> I understand that much.  But what did these two (or their
>>> then-equivalents) do in Emacs 22 and Emacs 23?
>> I'm guessing they behaved incorrectly (or however we want to call the
>> inconsistent behavior), but I don't have a compiled Emacs 22/23 around,
>> and they might be difficult to build.
>>
>> Note that we fixed bug#48356 not too long ago, which is from the same
>> general area, and it probably originated from before Emacs 22/23 too.
>>
>> It's worth looking for edge cases where we'd strongly prefer the current
>> behavior, and they might exist, but so far I only know of situations
>> where the change would be for the better, or the user might be okay with
>> either (example at the end ofhttps://debbugs.gnu.org/72705#35).
> Ping!  How should we proceed with this bug report?

I suggest we come up with a fix (which Stefan might have some ideas 
for), then see which reasonable scenarios get broken, if any. The one 
edge case in Eglot could be fixed in the same Emacs release, I believe.

If any larger scope problems, we could add a variable/option to switch 
to the previous behavior.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Ping!  How should we proceed with this bug report?

I don't quite understand the question.

After:

Eli wrote:
> Stefan wrote:
> > I'm not sure it's terribly important to preserve this detail of the
> > behavior of `Emacs-22`.  The `emacs22` style does not aim to provide the
> > illusion you're running an old Emacs.  I named it that way because
> > I couldn't come up with a good descriptive name for it.  If it
> > misbehaves, I don't see a need to be bug-compatible, especially since
> > this doesn't affect ELisp code but users.
>
> I think it does, sorry.  Suchj old behavior is a de-facto standard.
> If we change that, we should at least have a knob to get back the old
> behavior.

I thought you had decided that the current behavior is not a bug.
I'm fine with this choice and we can close it as such.
Tho maybe we'd want to deprecate that `emacs22` style because of
those odd cases.


        Stefan

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]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Ping!  How should we proceed with this bug report?
>
> I don't quite understand the question.
>
> After:
>
> Eli wrote:
>> Stefan wrote:
>> > I'm not sure it's terribly important to preserve this detail of the
>> > behavior of `Emacs-22`.  The `emacs22` style does not aim to provide the
>> > illusion you're running an old Emacs.  I named it that way because
>> > I couldn't come up with a good descriptive name for it.  If it
>> > misbehaves, I don't see a need to be bug-compatible, especially since
>> > this doesn't affect ELisp code but users.
>>
>> I think it does, sorry.  Suchj old behavior is a de-facto standard.
>> If we change that, we should at least have a knob to get back the old
>> behavior.
>
> I thought you had decided that the current behavior is not a bug.
> I'm fine with this choice and we can close it as such.
> Tho maybe we'd want to deprecate that `emacs22` style because of
> those odd cases.

I definitely don't want to just close this bug.  I often get user
complaints about this behavior.  In my experience, for novice users,
it's a fairly significant inconsistency in the default Emacs completion
experience.

I see a few good ways to fix this in a backwards-compatible way:

A. Fix it in emacs22 with a defcustom to get back the old behavior.

B. Deprecate the emacs22 style and replace it with a new style called
   `ignore-suffix` which has this bug fixed, and which replaces emacs22
   in the default value of completion-styles.

C. Follow this idea I suggested earlier:

   Currently emacs22 is the only style that ignores the text after point
   when completing.  But, this is often useful behavior, and I'd like to
   support it in other completion styles.  Specifically, I think it
   would be nice if completion always:
   
   1. First, run the completion styles on the literal text in the
   completion region or minibuffer.
   
   2. If that returned nil, run the completion styles again, but without
   the text after point in the region or minibuffer.
   
   Step 2 when run with the basic style is equivalent to the emacs22
   style.  So, emacs22 could be removed from the default
   completion-styles, since a completion-styles of '(basic) would be
   equivalent to '(basic emacs22).

   I think this would make it straightforward to then fix
   choose-completion to behave correctly when the completion was
   generated through step 2.
   
I personally like the option C best, because I already want to do this
generalization, so that the ignore-suffix behavior also works for other
completion styles (e.g. partial-completion or substring).

But I would like to get some feedback on this idea from others first.

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

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  juri@linkov.net,  70968@debbugs.gnu.org,
>    Dmitry Gutov <dmitry@gutov.dev>
> Date: Tue, 10 Sep 2024 12:54:05 -0400
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> >> Ping!  How should we proceed with this bug report?
> >
> > I don't quite understand the question.
> >
> > After:
> >
> > Eli wrote:
> >> Stefan wrote:
> >> > I'm not sure it's terribly important to preserve this detail of the
> >> > behavior of `Emacs-22`.  The `emacs22` style does not aim to provide the
> >> > illusion you're running an old Emacs.  I named it that way because
> >> > I couldn't come up with a good descriptive name for it.  If it
> >> > misbehaves, I don't see a need to be bug-compatible, especially since
> >> > this doesn't affect ELisp code but users.
> >>
> >> I think it does, sorry.  Suchj old behavior is a de-facto standard.
> >> If we change that, we should at least have a knob to get back the old
> >> behavior.
> >
> > I thought you had decided that the current behavior is not a bug.
> > I'm fine with this choice and we can close it as such.
> > Tho maybe we'd want to deprecate that `emacs22` style because of
> > those odd cases.
> 
> I definitely don't want to just close this bug.  I often get user
> complaints about this behavior.  In my experience, for novice users,
> it's a fairly significant inconsistency in the default Emacs completion
> experience.
> 
> I see a few good ways to fix this in a backwards-compatible way:
> 
> A. Fix it in emacs22 with a defcustom to get back the old behavior.
> 
> B. Deprecate the emacs22 style and replace it with a new style called
>    `ignore-suffix` which has this bug fixed, and which replaces emacs22
>    in the default value of completion-styles.
> 
> C. Follow this idea I suggested earlier:
> 
>    Currently emacs22 is the only style that ignores the text after point
>    when completing.  But, this is often useful behavior, and I'd like to
>    support it in other completion styles.  Specifically, I think it
>    would be nice if completion always:
>    
>    1. First, run the completion styles on the literal text in the
>    completion region or minibuffer.
>    
>    2. If that returned nil, run the completion styles again, but without
>    the text after point in the region or minibuffer.
>    
>    Step 2 when run with the basic style is equivalent to the emacs22
>    style.  So, emacs22 could be removed from the default
>    completion-styles, since a completion-styles of '(basic) would be
>    equivalent to '(basic emacs22).
> 
>    I think this would make it straightforward to then fix
>    choose-completion to behave correctly when the completion was
>    generated through step 2.
>    
> I personally like the option C best, because I already want to do this
> generalization, so that the ignore-suffix behavior also works for other
> completion styles (e.g. partial-completion or substring).
> 
> But I would like to get some feedback on this idea from others first.

Option C is from my POV the least desirable one: it adds some
complicated logic, which will almost certainly produce unintended
results, as everything in this completion-related mess we have.

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.

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

[Dmitry Gutov]

On 14/09/2024 12:17, Eli Zaretskii wrote:
> B. Deprecate the emacs22 style and replace it with a new style called
>     `ignore-suffix` which has this bug fixed, and which replaces emacs22
>     in the default value of completion-styles.

Note that while this is a viable option, we're still lacking a mechanism 
which would fix the problem for the new completion style.

And then we'd have two code paths for this behavior that somehow need to 
distinguish between these styles.

With the original proposal we would delegate to the style's completion 
logic, but it would apparently behave the same between emacs22 and the 
new style.

> Option C is from my POV the least desirable one: it adds some
> complicated logic, which will almost certainly produce unintended
> results, as everything in this completion-related mess we have.

That's probably true, but whether it will make things more complicated 
or not would also depend on the overall design. With some effort, it 
could be the opposite - but with more changes across the board, overall 
not backward compatible ones.

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

I don't envy the person who's going to write documentation and has to 
describe the distinction between these two styles, that they differ in 
this particular place of the UI, and are otherwise the same.

Deprecating the old style would at least say that this complication is a 
wart which would someday go away, and we could take a shortcut in the 
implementation (e.g. hardcode the value `emacs22` in some check).

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

[Eli Zaretskii]

> Date: Sat, 14 Sep 2024 18:18:46 +0300
> Cc: monnier@iro.umontreal.ca, juri@linkov.net, 70968@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> > 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.
> 
> I don't envy the person who's going to write documentation and has to 
> describe the distinction between these two styles, that they differ in 
> this particular place of the UI, and are otherwise the same.
> 
> Deprecating the old style would at least say that this complication is a 
> wart which would someday go away, and we could take a shortcut in the 
> implementation (e.g. hardcode the value `emacs22` in some check).

If documentation difficulties are the main issue with my proposal,
then you can stop worrying.

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]

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

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)

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

Stefan, Dmitry, any comments?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-ignore-after-point-completion-style.patch --]
[-- Type: text/x-patch, Size: 12976 bytes --]

From cdc7ce4b9958a0ef36e911066ecce82a6da09c02 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Mon, 16 Sep 2024 15:15:57 -0400
Subject: [PATCH] Add ignore-after-point completion style

* lisp/minibuffer.el (completion--twq-all): Use the original
completion faces where possible.
(completion-styles-alist, completion-ignore-after-point--force-nil)
(completions-ignored, completion-ignore-after-point-try-completion)
(completion-ignore-after-point-all-completions): Add
ignore-after-point completion style.  (bug#70968)
* lisp/simple.el (choose-completion-string--should-exit):
Add. (choose-completion-string): Call
choose-completion-string--should-exit.
---
 lisp/minibuffer.el | 142 ++++++++++++++++++++++++++++++++++++---------
 lisp/simple.el     |  47 +++++++++------
 2 files changed, 142 insertions(+), 47 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 72ee5d02002..e5d85ed6fc8 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -674,34 +674,42 @@ completion--twq-all
       ;; requote them, so that *Completions* can show nicer unquoted values
       ;; which only get quoted when needed by choose-completion.
       (nconc
-       (mapcar (lambda (completion)
-                 (cl-assert (string-prefix-p prefix completion 'ignore-case) t)
-                 (let* ((new (substring completion (length prefix)))
-                        (qnew (funcall qfun new))
- 			(qprefix
-                         (if (not completion-ignore-case)
-                             qprefix
-                           ;; Make qprefix inherit the case from `completion'.
-                           (let* ((rest (substring completion
-                                                   0 (length prefix)))
-                                  (qrest (funcall qfun rest)))
-                             (if (string-equal-ignore-case qprefix qrest)
-                                 (propertize qrest 'face
-                                             'completions-common-part)
-                               qprefix))))
-                        (qcompletion (concat qprefix qnew)))
-		   ;; FIXME: Similarly here, Cygwin's mapping trips this
-		   ;; assertion.
-                   ;;(cl-assert
-                   ;; (string-equal-ignore-case
-		   ;;  (funcall unquote
-		   ;;           (concat (substring string 0 qboundary)
-		   ;;                   qcompletion))
-		   ;;  (concat (substring ustring 0 boundary)
-		   ;;          completion))
-		   ;; t)
-                   qcompletion))
-               completions)
+       (mapcar
+        (if (string-equal qprefix prefix)
+            ;; There's no quoting in the prefix; quoting in the completions
+            ;; can be simpler and preserve the existing faces.
+            (lambda (completion)
+              (concat
+               (substring completion 0 (length prefix))
+               (funcall qfun (substring completion (length prefix)))))
+          (lambda (completion)
+            (cl-assert (string-prefix-p prefix completion 'ignore-case) t)
+            (let* ((new (substring completion (length prefix)))
+                   (qnew (funcall qfun new))
+                   (qprefix
+                    (if (not completion-ignore-case)
+                        qprefix
+                      ;; Make qprefix inherit the case from `completion'.
+                      (let* ((rest (substring completion
+                                              0 (length prefix)))
+                             (qrest (funcall qfun rest)))
+                        (if (string-equal-ignore-case qprefix qrest)
+                            (propertize qrest 'face
+                                        'completions-common-part)
+                          qprefix))))
+                   (qcompletion (concat qprefix qnew)))
+              ;; FIXME: Similarly here, Cygwin's mapping trips this
+              ;; assertion.
+              ;;(cl-assert
+              ;; (string-equal-ignore-case
+              ;;  (funcall unquote
+              ;;           (concat (substring string 0 qboundary)
+              ;;                   qcompletion))
+              ;;  (concat (substring ustring 0 boundary)
+              ;;          completion))
+              ;; t)
+              qcompletion)))
+        completions)
        qboundary))))
 
 ;;; Minibuffer completion
@@ -1038,6 +1046,12 @@ completion-styles-alist
      "Prefix completion that only operates on the text before point.
 I.e. when completing \"foo_bar\" (where _ is the position of point),
 it will consider all completions candidates matching the glob
+pattern \"foo*\" and will add back \"bar\" to the end of it.")
+    (ignore-after-point
+     completion-ignore-after-point-try-completion completion-ignore-after-point-all-completions
+     "Prefix completion that only operates on the text before point.
+I.e. when completing \"foo_bar\" (where _ is the position of point),
+it will consider all completions candidates matching the glob
 pattern \"foo*\" and will add back \"bar\" to the end of it.")
     (basic
      completion-basic-try-completion completion-basic-all-completions
@@ -3692,6 +3706,78 @@ completion-emacs22-all-completions
      point
      (car (completion-boundaries beforepoint table pred "")))))
 
+;;; ignore-after-point completion style.
+
+(defvar completion-ignore-after-point--force-nil nil
+  "When non-nil, the ignore-after-point style always returns nil.")
+
+(defface completions-ignored
+  '((t (:inherit shadow)))
+  "Face for text which was ignored by the completion style.")
+
+(defun completion-ignore-after-point-try-completion (string table pred point)
+  "Run `completion-try-completion' ignoring the part of STRING after POINT.
+
+We add the part of STRING after POINT back to the result."
+  (let ((prefix (substring string 0 point))
+        (suffix (substring string point)))
+    (when-let ((completion
+                (unless completion-ignore-after-point--force-nil
+                  (let ((completion-ignore-after-point--force-nil t))
+                    (completion-try-completion prefix table pred point)))))
+      ;; Add SUFFIX back to COMPLETION.  However, previous completion styles failed and
+      ;; this one succeeded by ignoring SUFFIX.  The success of future completion depends
+      ;; on ignoring SUFFIX.  We mostly do that by keeping point right before SUFFIX.
+      (if (eq completion t)
+          ;; Keep point in the same place, right before SUFFIX.
+          (cons string point)
+        (let ((newstring (car completion))
+              (newpoint (cdr completion)))
+          (cond
+           ((= (length newstring) newpoint)
+            ;; NEWPOINT is already right before SUFFIX.
+            (cons (concat newstring suffix) newpoint))
+           ((minibufferp completion-reference-buffer)
+            ;; Don't allow moving point, keep it right before SUFFIX.
+            (cons (concat newstring suffix) (length newstring)))
+           (t
+            ;; If we're not in a minibuffer, then we're using `completion-at-point', which
+            ;; calculates a completion region to complete over.  We can allow point to
+            ;; move and still cause SUFFIX to be omitted from the completion region, by
+            ;; including a space right before SUFFIX.
+            (cons (concat newstring
+                          ;; Don't add another space if SUFFIX already starts with one.
+                          (when (/= (aref suffix 0) ? ) " ") suffix)
+                  newpoint))))))))
+
+(defun completion-ignore-after-point-all-completions (string table pred point)
+  "Run `completion-all-completions' ignoring the part of STRING after POINT."
+  (let ((prefix (substring string 0 point))
+        (suffix (propertize (substring string point) 'face 'completions-ignored)))
+    (when-let ((completions
+                (unless completion-ignore-after-point--force-nil
+                  (let ((completion-ignore-after-point--force-nil t))
+                    (completion-all-completions prefix table pred point)))))
+      ;; Add SUFFIX back to each completion.  COMPLETIONS may be an improper list (with
+      ;; the base position in its last cdr) so we can't use `mapcar'.
+      (let ((tail completions))
+        (while (consp tail)
+          (let* ((completion (car tail))
+                 (choose-completion-will-exit
+                  (and (minibufferp completion-reference-buffer)
+                       (choose-completion-string--should-exit completion))))
+            ;; Include the suffix if, after `choose-completion' runs on COMPLETION, the
+            ;; user is still able to use and edit the suffix.
+            (unless choose-completion-will-exit
+              (let ((end-of-real-completion (length completion)))
+                (setcar tail (concat completion suffix))
+                ;; When chosen, point should go before SUFFIX.
+                (put-text-property
+                 0 1 'completion-position-after-insert end-of-real-completion
+                 (car tail)))))
+          (setq tail (cdr tail))))
+      completions)))
+
 ;;; Basic completion.
 
 (defun completion--merge-suffix (completion point suffix)
diff --git a/lisp/simple.el b/lisp/simple.el
index 1dd6bfe5b22..fe68f23c4da 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10083,6 +10083,20 @@ choose-completion-string-functions
 If all functions in the list return nil, that means to use
 the default method of inserting the completion in BUFFER.")
 
+(defun choose-completion-string--should-exit (result)
+  "Should `choose-completion-string' exit the minibuffer if RESULT is chosen?"
+  (and
+   (not completion-no-auto-exit)
+   minibuffer-completion-table
+   ;; If this is reading a file name, and the file name chosen
+   ;; is a directory, don't exit the minibuffer.
+   (let ((bounds (completion-boundaries
+                  result minibuffer-completion-table
+                  minibuffer-completion-predicate "")))
+     ;; The completion chosen leads to a new set of completions
+     ;; (e.g. it's a directory): don't exit the minibuffer yet.
+     (not (eq (car bounds) (length result))))))
+
 (defun choose-completion-string (choice &optional
                                         buffer base-position insert-function)
   "Switch to BUFFER and insert the completion choice CHOICE.
@@ -10116,10 +10130,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)))
@@ -10127,21 +10144,13 @@ choose-completion-string
 	(and (not completion-no-auto-exit)
              (minibufferp buffer)
 	     minibuffer-completion-table
-	     ;; If this is reading a file name, and the file name chosen
-	     ;; is a directory, don't exit the minibuffer.
-             (let* ((result (buffer-substring (field-beginning) (point)))
-                    (bounds
-                     (completion-boundaries result minibuffer-completion-table
-                                            minibuffer-completion-predicate
-                                            "")))
-               (if (eq (car bounds) (length result))
-                   ;; The completion chosen leads to a new set of completions
-                   ;; (e.g. it's a directory): don't exit the minibuffer yet.
-                   (let ((mini (active-minibuffer-window)))
-                     (select-window mini)
-                     (when minibuffer-auto-raise
-                       (raise-frame (window-frame mini))))
-                 (exit-minibuffer))))))))
+             (if (choose-completion-string--should-exit
+                  (buffer-substring (field-beginning) (point)))
+                 (exit-minibuffer)
+               (let ((mini (active-minibuffer-window)))
+                 (select-window mini)
+                 (when minibuffer-auto-raise
+                   (raise-frame (window-frame mini))))))))))
 
 (define-derived-mode completion-list-mode nil "Completion List"
   "Major mode for buffers showing lists of possible completions.
-- 
2.39.3

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

[Dmitry Gutov]

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

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]

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

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

[Dmitry Gutov]

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

On 15/10/2024 21:53, Spencer Baugh wrote:

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

I suppose we should ask what makes sense as the default behavior, and if 
it's going to be different between frontends, how difficult is the 
"other" approach would be to support.

Since company-mode supports the "proper" behavior for emacs22, I've had 
some time to get used to it and see if anything looks off. And IME the 
current indicators seem enough. For Elisp anyway, we a) have the 
completions themselves, b) the highlighting inside the completions for 
the characters matching the input. When the style matches the prefix 
only, all the completions will only have that many characters 
highlighted, not the chars from the suffix.

See the second attached pic for an example. It seems okay to me, but I 
suppose the experience might vary between completion types and 
programming languages, etc.

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

Makes a certain sense. Just to make a note, I think try-completion moves 
point specifically to help position before the next char which would 
help disambiguate completions. choose-completion happens when no other 
completions are relevant, so it couldn't move point similarly.

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

In practice, company-mode will require a small change anyway, since it 
doesn't do pass-through for any of the faces now. See the first attachment.

But to slice off the suffixes in the popup, which I'm currently inclined 
to do, might require more (and more complex) code than to print them 
selectively in the frontends that choose to (the "opt-in" approach). 
This is a part that gives me pause.

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

Thanks. If I were to do a test-drive to look for edge cases, is Lisp 
symbols the only kind of completion that I should try out?

[-- Attachment #2: Screenshot from 2024-10-17 03-00-36.png --]
[-- Type: image/png, Size: 12069 bytes --]

[-- Attachment #3: Screenshot from 2024-10-17 03-01-07.png --]
[-- Type: image/png, Size: 9301 bytes --]

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

[Dmitry Gutov]

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

On 24/09/2024 03:03, Dmitry Gutov wrote:
> 
> 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).

Here's a halfway PoC of the UI printing the suffix based on the metadata 
property with emacs22 (without special behavior for files yet, but it 
should be straightforward to add the same set of conditions - the 
category, completion boundaries, etc).

[-- Attachment #2: completion--emacs22-adjust-metadata-with-shadow-suffix.diff --]
[-- Type: text/x-patch, Size: 4241 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 804afe9cb43..595fdcd2288 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2568,6 +2568,10 @@ completions--after-change
       (with-selected-window window
         (completions--deselect)))))
 
+(defface completions-ignored
+  '((t (:inherit shadow)))
+  "Face for text which was ignored by the completion style.")
+
 (defun minibuffer-completion-help (&optional start end)
   "Display a list of possible completions of the current minibuffer contents."
   (interactive)
@@ -2613,7 +2617,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 +2626,13 @@ 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 'completion-ignore-after-point))
+             (base-position (list (+ start base-size)
+                                  (if ignore-after-point
+                                      (point)
+                                    field-end)))
+             (ignore-suffix (completion-metadata-get all-md 'completion-ignore-after-point))
              (mainbuf (current-buffer))
              ;; If the *Completions* buffer is shown in a new
              ;; window, mark it as softly-dedicated, so bury-buffer in
@@ -2668,6 +2678,14 @@ minibuffer-completion-help
                                             ('historical (minibuffer-sort-by-history completions))
                                             (_ (funcall completions-sort completions)))))
 
+                      (when ignore-suffix
+                        (let ((hidden-suffix (propertize (buffer-substring (point) end)
+                                                         'face
+                                                         'completions-ignored)))
+                          (setq completions
+                                (mapcar (lambda (s) (concat s hidden-suffix))
+                                        completions))))
+
                       ;; After sorting, group the candidates using the
                       ;; `group-function'.
                       (when group-fun
@@ -3778,6 +3796,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
+    (completion-ignore-after-point . t)
+    ,@(cdr metadata)))
+
 ;;; Basic completion.
 
 (defun completion--merge-suffix (completion point suffix)
diff --git a/lisp/simple.el b/lisp/simple.el
index e35cfe0479b..a495231c211 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10264,6 +10264,7 @@ choose-completion
           (base-position completion-base-position)
           (insert-function completion-list-insert-choice-function)
           (completion-no-auto-exit (if no-exit t completion-no-auto-exit))
+          (ignore-face nil)
           (choice
            (if choose-completion-deselect-if-after
                (or (get-text-property (posn-point (event-start event))
@@ -10285,6 +10286,10 @@ choose-completion
                              beg))
                (get-text-property beg 'completion--string))))))
 
+      (when (get-text-property (1- (length choice)) 'face choice)
+        (setq ignore-face (previous-single-property-change (1- (length choice)) 'face choice))
+        (setq choice (substring choice 0 ignore-face)))
+
       (unless (buffer-live-p buffer)
         (error "Destination buffer is dead"))
       (unless no-quit

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I suggest that the behavior of not deleting the text after point is
>> better.  The emacs22 style takes care to not delete the text after point
>> in try-completion and in completion cycling; we should take similar care
>> in choose-completion.

I agree this inconsistency is a bug.
I suspect it may have been a result of implementation limitations.

A typical use case is

    C-x C-w bar
    ... realize you're not in the right subdir ...
    M-b ?
    ... choose some subdirectory in which you want to save `bar`
    ... Oops, oh no, `bar` has disappeared!

> If making these two ways consistent means we need to change the
> behavior of emacs22 style of completion, then I'm sorry, but we cannot
> do that.  This style is a legacy style, and is there to provide the
> legacy behavior for those who need it.

I'm not sure it's terribly important to preserve this detail of the
behavior of `Emacs-22`.  The `emacs22` style does not aim to provide the
illusion you're running an old Emacs.  I named it that way because
I couldn't come up with a good descriptive name for it.  If it
misbehaves, I don't see a need to be bug-compatible, especially since
this doesn't affect ELisp code but users.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Spencer Baugh <sbaugh@janestreet.com>,  dmitry@gutov.dev,
>   70968@debbugs.gnu.org,  juri@linkov.net
> Date: Thu, 16 May 2024 13:40:35 -0400
> 
> > If making these two ways consistent means we need to change the
> > behavior of emacs22 style of completion, then I'm sorry, but we cannot
> > do that.  This style is a legacy style, and is there to provide the
> > legacy behavior for those who need it.
> 
> I'm not sure it's terribly important to preserve this detail of the
> behavior of `Emacs-22`.  The `emacs22` style does not aim to provide the
> illusion you're running an old Emacs.  I named it that way because
> I couldn't come up with a good descriptive name for it.  If it
> misbehaves, I don't see a need to be bug-compatible, especially since
> this doesn't affect ELisp code but users.

I think it does, sorry.  Suchj old behavior is a de-facto standard.
If we change that, we should at least have a knob to get back the old
behavior.

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

[Spencer Baugh]

Here is an idea:

Currently emacs22 is the only style that ignores the text after point
when completing.  But, this is often useful behavior, and I'd like to
support it in other completion styles as a fallback.  Specifically, I
think it would be nice if completion always:

1. Try to complete including the suffix after point (like the basic,
partial-completion, substring styles do)

2. If no completion matches, try to complete again without the suffix
after point

What if we implemented this in the completion machinery itself?  If no
style matches, run through all the styles again without the suffix after
point.  This would allow e.g. partial-completion behavior when
completing in the middle of a symbol, which currently doesn't work.

This would basically remove the emacs22 style, since it would be a
behavior implemented at a higher level.  Then each completion frontend
could implement this "first with suffix then without suffix" behavior
separately, which would make choose-completion and similar calls easier
to make work.

Stefan, I wonder if you have thoughts on this idea?