unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
@ 2021-05-11 17:23 Daniel Mendler
  2022-03-13 17:56 ` Juri Linkov
  2022-03-14  3:10 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 2 replies; 24+ messages in thread
From: Daniel Mendler @ 2021-05-11 17:23 UTC (permalink / raw)
  To: 48356; +Cc: Stefan Monnier, JD Smith

When selecting a candidate the suffix after the completion boundary is
discarded by `choose-completion`/`choose-completion-string`.
`choose-completion` is invoked when a candidate in the *Completions*
buffer is selected with the mouse or RET.

For example when completing a file path "~/emacs/master/li|/calc", where
"|" is the cursor, and then the candidate "lisp" is selected in the
*Completions* buffer, the result is "~/emacs/master/lisp/". The prefix
"~/emacs/master/" is prepended to the selected candidate, but the suffix
"/calc" is discarded.

`choose-completion-string` contains logic which checks if the resulting
string equals the car of the completion boundary. In that case the
minibuffer is not exited.

I propose the following change to the existing logic: When
selecting a candidate with `choose-completion` and a suffix is present,
the minibuffer should not be exited (completion continues) and the
suffix is preserved.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2021-05-11 17:23 bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary Daniel Mendler
@ 2022-03-13 17:56 ` Juri Linkov
  2022-03-13 20:35   ` bug#48356: [External] : " Drew Adams
  2022-03-14  3:10 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 24+ messages in thread
From: Juri Linkov @ 2022-03-13 17:56 UTC (permalink / raw)
  To: Daniel Mendler; +Cc: 48356, Stefan Monnier, JD Smith

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

forcemerge 48356 49931
thanks

> When selecting a candidate the suffix after the completion boundary is
> discarded by `choose-completion`/`choose-completion-string`.
> `choose-completion` is invoked when a candidate in the *Completions*
> buffer is selected with the mouse or RET.
>
> For example when completing a file path "~/emacs/master/li|/calc", where
> "|" is the cursor, and then the candidate "lisp" is selected in the
> *Completions* buffer, the result is "~/emacs/master/lisp/". The prefix
> "~/emacs/master/" is prepended to the selected candidate, but the suffix
> "/calc" is discarded.
>
> `choose-completion-string` contains logic which checks if the resulting
> string equals the car of the completion boundary. In that case the
> minibuffer is not exited.

Strange, in your test case above, the minibuffer is not exited already.

> I propose the following change to the existing logic: When
> selecting a candidate with `choose-completion` and a suffix is present,
> the minibuffer should not be exited (completion continues) and the
> suffix is preserved.

Here is a better patch than was posted to bug#49931
to preserve the suffix.

It correctly handles at least three different cases:

1. When manually adding a suffix in the minibuffer after completions
   were displayed, choose-completion discards that suffix.

2. In file name completion in the above case the suffix is preserved.

3. 'M-! command filename TAB' and choosing a completion preserves both
   prefix and suffix.

This works only after customizing 'completion-use-base-affixes' to t:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: completion-base-affixes.patch --]
[-- Type: text/x-diff, Size: 5170 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 36b8d80841..5685f078ad 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2227,6 +2312,8 @@ minibuffer-completion-help
       (let* ((last (last completions))
              (base-size (or (cdr last) 0))
              (prefix (unless (zerop base-size) (substring string 0 base-size)))
+             (base-prefix (buffer-substring (minibuffer--completion-prompt-end) (+ start base-size)))
+             (base-suffix (buffer-substring (point) (point-max)))
              (all-md (completion--metadata (buffer-substring-no-properties
                                             start (point))
                                            base-size md
@@ -2320,11 +2407,18 @@ minibuffer-completion-help
                                    ;; completion-all-completions does not give us the
                                    ;; necessary information.
                                    end))
+                        (setq-local completion-base-affixes (list base-prefix base-suffix))
                         (setq-local completion-list-insert-choice-function
                              (let ((ctable minibuffer-completion-table)
                                    (cpred minibuffer-completion-predicate)
                                    (cprops completion-extra-properties))
                                (lambda (start end choice)
+                                 (if (and (stringp start) (stringp end))
+                                     (progn
+                                       (delete-minibuffer-contents)
+                                       (insert start choice)
+                                       ;; Keep point after completion before suffix
+                                       (save-excursion (insert end)))
                                  (unless (or (zerop (length prefix))
                                              (equal prefix
                                                     (buffer-substring-no-properties
@@ -2333,7 +2427,7 @@ minibuffer-completion-help
                                                      start)))
                                    (message "*Completions* out of date"))
                                  ;; FIXME: Use `md' to do quoting&terminator here.
-                                 (completion--replace start end choice)
+                                 (completion--replace start end choice))
                                  (let* ((minibuffer-completion-table ctable)
                                         (minibuffer-completion-predicate cpred)
                                         (completion-extra-properties cprops)
diff --git a/lisp/simple.el b/lisp/simple.el
index accc119e2b..52cf54c563 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9071,6 +9075,19 @@ completion-base-position
 where the completion should be inserted and END (if non-nil) is the end
 of the text to replace.  If END is nil, point is used instead.")
 
+(defvar completion-base-affixes nil
+  "Base context of the text corresponding to the shown completions.
+This variable is used in the *Completions* buffers.
+Its value is a list of the form (PREFIX SUFFIX) where PREFIX is the text
+before the place where completion should be inserted and SUFFIX is the text
+after the completion.")
+
+(defcustom completion-use-base-affixes nil
+  "Non-nil means to restore original prefix and suffix in the minibuffer."
+  :type 'boolean
+  :version "29.1"
+  :group 'completion)
+
 (defvar completion-list-insert-choice-function #'completion--replace
   "Function to use to insert the text chosen in *Completions*.
 Called with three arguments (BEG END TEXT), it should replace the text
@@ -9151,6 +9168,7 @@ next-completion
   (with-current-buffer (window-buffer (posn-window (event-start event)))
     (let ((buffer completion-reference-buffer)
           (base-position completion-base-position)
+          (base-affixes completion-base-affixes)
           (insert-function completion-list-insert-choice-function)
           (choice
            (save-excursion
@@ -9184,7 +9203,8 @@ choose-completion
       (with-current-buffer buffer
         (choose-completion-string
          choice buffer
-         (or base-position
+         (or (and completion-use-base-affixes base-affixes)
+             base-position
              ;; If all else fails, just guess.
              (list (choose-completion-guess-base-position choice)))
          insert-function)))))
@@ -9344,9 +9372,11 @@ completion-setup-function
                 (buffer-substring (minibuffer-prompt-end) (point)))))))
     (with-current-buffer standard-output
       (let ((base-position completion-base-position)
+            (base-affixes completion-base-affixes)
             (insert-fun completion-list-insert-choice-function))
         (completion-list-mode)
         (setq-local completion-base-position base-position)
+        (setq-local completion-base-affixes base-affixes)
         (setq-local completion-list-insert-choice-function insert-fun))
       (setq-local completion-reference-buffer mainbuf)
       (if base-dir (setq default-directory base-dir))

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* bug#48356: [External] : bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2022-03-13 17:56 ` Juri Linkov
@ 2022-03-13 20:35   ` Drew Adams
  0 siblings, 0 replies; 24+ messages in thread
From: Drew Adams @ 2022-03-13 20:35 UTC (permalink / raw)
  To: Juri Linkov, Daniel Mendler
  Cc: 48356@debbugs.gnu.org, Stefan Monnier, JD Smith

FWIW, my opinion (no doubt a minority of one)
is that all such approaches, including what's
in vanilla Emacs now (since `minibuffer.el',
which I guess means Emacs 22/23), are inferior
to the original vanilla behavior.

Icicles uses that original behavior, in which
it _makes no difference where point is_ in
the minibuffer content.

That is, the entire minibuffer content is the
pattern to be matched (whether for completion
or reading by `read-from-minibuffer' etc.).

I find this more flexible & saner - doesn't
matter where point is.  Whether or not you've
made an edit in the middle of the content,
e.g. yanking or typing or deleting there, all
of the current text is used.

If someone really ever wants the text that
follows point not to be taken into account
then it's simple enough to hit a key to remove
it (and it can be restored with undo etc.).

You may say it's also simple enough otherwise
to move point to the end of input (e.g. `M-v').
Fair enough, but I think that's more bother.

It's more common, I think, to edit text in the
middle somewhere, and then either keep the
text that follows point or kill/delete it.

There's never any need to move point just to
get the pattern you want.  You never need to
pay any attention to point in the minibuffer.
___

[I suppose that in some sense this is kind of
a bottle half-full/half-empty choice.  Maybe
akin to views on `delete-selection-mode':
convenience of not having to first use `C-w'
(to replace) versus convenience of not having
to first use `C-g' (to not replace).]







^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2021-05-11 17:23 bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary Daniel Mendler
  2022-03-13 17:56 ` Juri Linkov
@ 2022-03-14  3:10 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-03-14 18:53   ` Juri Linkov
  1 sibling, 1 reply; 24+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-03-14  3:10 UTC (permalink / raw)
  To: Daniel Mendler; +Cc: 48356, JD Smith

> For example when completing a file path "~/emacs/master/li|/calc", where
> "|" is the cursor, and then the candidate "lisp" is selected in the
> *Completions* buffer, the result is "~/emacs/master/lisp/". The prefix
> "~/emacs/master/" is prepended to the selected candidate, but the suffix
> "/calc" is discarded.

Yup.  That's listed under "bugs" at the beginning of minibuffer.el:

    ;; - choose-completion can't automatically figure out the boundaries
    ;;   corresponding to the displayed completions because we only
    ;;   provide the start info but not the end info in
    ;;   completion-base-position.

> ... and the suffix is preserved.

Of course, the suffix should *not* be preserved when the minibuffer was
"r|e-buf".


        Stefan






^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2022-03-14  3:10 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-03-14 18:53   ` Juri Linkov
  2022-03-14 20:55     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 24+ messages in thread
From: Juri Linkov @ 2022-03-14 18:53 UTC (permalink / raw)
  To: 48356; +Cc: mail, monnier, jdtsmith

>> For example when completing a file path "~/emacs/master/li|/calc", where
>> "|" is the cursor, and then the candidate "lisp" is selected in the
>> *Completions* buffer, the result is "~/emacs/master/lisp/". The prefix
>> "~/emacs/master/" is prepended to the selected candidate, but the suffix
>> "/calc" is discarded.
>
> Yup.  That's listed under "bugs" at the beginning of minibuffer.el:
>
>     ;; - choose-completion can't automatically figure out the boundaries
>     ;;   corresponding to the displayed completions because we only
>     ;;   provide the start info but not the end info in
>     ;;   completion-base-position.
>
>> ... and the suffix is preserved.
>
> Of course, the suffix should *not* be preserved when the minibuffer was
> "r|e-buf".

IMHO, the root of the problem is in completion-all-completions
that returns in the last `cdr' only the start of completions,
but not the end of completions.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2022-03-14 18:53   ` Juri Linkov
@ 2022-03-14 20:55     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-03-15  2:14       ` Daniel Mendler
  0 siblings, 1 reply; 24+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-03-14 20:55 UTC (permalink / raw)
  To: Juri Linkov; +Cc: mail, 48356, jdtsmith

> IMHO, the root of the problem is in completion-all-completions
> that returns in the last `cdr' only the start of completions,
> but not the end of completions.

Agreed.


        Stefan






^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2022-03-14 20:55     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-03-15  2:14       ` Daniel Mendler
  2022-03-15  7:53         ` Juri Linkov
  0 siblings, 1 reply; 24+ messages in thread
From: Daniel Mendler @ 2022-03-15  2:14 UTC (permalink / raw)
  To: Stefan Monnier, Juri Linkov; +Cc: 48356, jdtsmith

On 3/14/22 21:55, Stefan Monnier wrote:
>> IMHO, the root of the problem is in completion-all-completions
>> that returns in the last `cdr' only the start of completions,
>> but not the end of completions.
> 
> Agreed.

If I recall correctly, someone wrote a patch which fixed the root cause.

Daniel





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2022-03-15  2:14       ` Daniel Mendler
@ 2022-03-15  7:53         ` Juri Linkov
  2022-03-20 20:34           ` Juri Linkov
  0 siblings, 1 reply; 24+ messages in thread
From: Juri Linkov @ 2022-03-15  7:53 UTC (permalink / raw)
  To: Daniel Mendler; +Cc: 48356, Stefan Monnier, JD Smith

>>> IMHO, the root of the problem is in completion-all-completions
>>> that returns in the last `cdr' only the start of completions,
>>> but not the end of completions.
>>
>> Agreed.
>
> If I recall correctly, someone wrote a patch which fixed the root cause.

The only patch that removes these comments

        ;; FIXME: We need to additionally return the info needed for the
        ;; second part of completion-base-position.

        ;; FIXME: We should pay attention to completion
        ;; boundaries here, but currently
        ;; completion-all-completions does not give us the
        ;; necessary information.

is https://lists.gnu.org/archive/html/emacs-devel/2021-08/msg00412.html
in bug#47711 and bug#48841.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2022-03-15  7:53         ` Juri Linkov
@ 2022-03-20 20:34           ` Juri Linkov
  2024-04-08 21:59             ` Dmitry Gutov
  0 siblings, 1 reply; 24+ messages in thread
From: Juri Linkov @ 2022-03-20 20:34 UTC (permalink / raw)
  To: Daniel Mendler; +Cc: 48356, Stefan Monnier, JD Smith

>>>> IMHO, the root of the problem is in completion-all-completions
>>>> that returns in the last `cdr' only the start of completions,
>>>> but not the end of completions.
>>>
>>> Agreed.
>>
>> If I recall correctly, someone wrote a patch which fixed the root cause.
>
> The only patch that removes these comments
>
>         ;; FIXME: We need to additionally return the info needed for the
>         ;; second part of completion-base-position.
>
>         ;; FIXME: We should pay attention to completion
>         ;; boundaries here, but currently
>         ;; completion-all-completions does not give us the
>         ;; necessary information.
>
> is https://lists.gnu.org/archive/html/emacs-devel/2021-08/msg00412.html
> in bug#47711 and bug#48841.

I wonder what is the fate of this patch?  There was a long discussion
without a clear outcome.  Maybe it's possible to split the patch
to smaller parts where a separate patch would add the feature needed here
to return the end position of the completion boundaries?





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2022-03-20 20:34           ` Juri Linkov
@ 2024-04-08 21:59             ` Dmitry Gutov
  2024-04-08 22:27               ` Dmitry Gutov
  2024-04-08 23:50               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 2 replies; 24+ messages in thread
From: Dmitry Gutov @ 2024-04-08 21:59 UTC (permalink / raw)
  To: Juri Linkov, Daniel Mendler; +Cc: 48356, Stefan Monnier, JD Smith

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

On 20/03/2022 22:34, Juri Linkov wrote:
>>>>> IMHO, the root of the problem is in completion-all-completions
>>>>> that returns in the last `cdr' only the start of completions,
>>>>> but not the end of completions.
>>>> Agreed.
>>> If I recall correctly, someone wrote a patch which fixed the root cause.
>> The only patch that removes these comments
>>
>>          ;; FIXME: We need to additionally return the info needed for the
>>          ;; second part of completion-base-position.
>>
>>          ;; FIXME: We should pay attention to completion
>>          ;; boundaries here, but currently
>>          ;; completion-all-completions does not give us the
>>          ;; necessary information.
>>
>> ishttps://lists.gnu.org/archive/html/emacs-devel/2021-08/msg00412.html
>> in bug#47711 and bug#48841.
> I wonder what is the fate of this patch?  There was a long discussion
> without a clear outcome.  Maybe it's possible to split the patch
> to smaller parts where a separate patch would add the feature needed here
> to return the end position of the completion boundaries?

It seems there is a range of solutions we could take here.

On the one side would be a replacement of the -all-completions API with 
something like completion-filter-completions as proposed by Daniel. It's 
still a reasonable choice, but a rather breaking one, and the patch 
would need to be majorly rewritten anyway, given the amount of changes 
in the area since it was submitted.

On the other, we could preserve the current convention again, and add a 
new dynamic variable which would be assigned in every relevant 
completion style, to store the rightmost-position (the length of 
suffix). Then the callers would fetch it from that variable. Similar to 
what we do with completion-lazy-hilit-fn now.

Or some variations of that.

But what I don't quite see yet, is why wouldn't the caller be able to 
compute the bounds cheaply enough? We could offer an accessor function. 
See the new logic in the attached patch (but imagine it extracted to a 
named function).

Note that it doesn't work too well now, because in the example like

   ~/v/e-|/src

the completions include the trailing slash. And base-suffix includes a 
starting slash as well (according to boundaries returned by the 
completion table). So when I choose one of the completions using 
minibuffer-next-completion, the minibuffer contents look like

   ~/v/emacs-master//src

...which translates to "/" because of the double slash -- the filesystem 
root directory (*). But that's the same data which would be used by any 
other proposed solution, too. So maybe it should be either be fixed in 
the completion table (avoid adding trailing slash when the last boundary 
is already followed by slash?), or the insertion code should do some 
additional post-processing of the completion string.

(*) And then, when I press tab while point is between slashes -- /|/ -- 
it jumps to the beginning of the input, but that's a secondary problem.

[-- Attachment #2: base-suffix.diff --]
[-- Type: text/x-patch, Size: 1515 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 0a844c538b4..33c175aa3c6 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2582,16 +2582,13 @@ minibuffer-completion-help
              (minibuffer-completion-base (substring string 0 base-size))
              (base-prefix (buffer-substring (minibuffer--completion-prompt-end)
                                             (+ start base-size)))
-             (base-suffix
-              (if (or (eq (alist-get 'category (cdr md)) 'file)
-                      completion-in-region-mode-predicate)
-                  (buffer-substring
-                   (save-excursion
-                     (if completion-in-region-mode-predicate
-                         (point)
-                       (or (search-forward "/" nil t) (point-max))))
-                   (point-max))
-                ""))
+             (base-suffix (let ((suffix (buffer-substring (point) end)))
+                            (substring
+                             suffix
+                             (cdr (completion-boundaries string
+                                                         minibuffer-completion-table
+                                                         minibuffer-completion-predicate
+                                                         suffix)))))
              (all-md (completion--metadata (buffer-substring-no-properties
                                             start (point))
                                            base-size md

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-08 21:59             ` Dmitry Gutov
@ 2024-04-08 22:27               ` Dmitry Gutov
  2024-04-08 23:50               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 0 replies; 24+ messages in thread
From: Dmitry Gutov @ 2024-04-08 22:27 UTC (permalink / raw)
  To: Juri Linkov, Daniel Mendler; +Cc: 48356, Stefan Monnier, JD Smith

On 09/04/2024 00:59, Dmitry Gutov wrote:
> But what I don't quite see yet, is why wouldn't the caller be able to 
> compute the bounds cheaply enough? We could offer an accessor function.

Reading completion-pcm--find-all-completions, it seems like the case 
where this wouldn't work is "The prefix has no completions at all ...".

What scenarios does this correspond to? If they are limited to the cases 
where the point jumps to a different field (and then completion has to 
be repeated), then maybe my patch would still be fine.

Otherwise, perhaps one of the other approaches is the way to go.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-08 21:59             ` Dmitry Gutov
  2024-04-08 22:27               ` Dmitry Gutov
@ 2024-04-08 23:50               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-10  1:33                 ` Dmitry Gutov
  1 sibling, 1 reply; 24+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-08 23:50 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 48356, Daniel Mendler, JD Smith, Juri Linkov

> ...which translates to "/" because of the double slash -- the filesystem
>  root directory (*). But that's the same data which would be used by any
>  other proposed solution, too.

More or less, tho the "ideal" solution is to do that in the
completion-style code, which has a bit more knowledge about it.

>  So maybe it should be either be fixed in the
>  completion table (avoid adding trailing slash when the last boundary is
>  already followed by slash?), or the insertion code should do some
>  additional post-processing of the completion string.

I think you can fix it in the same ad-hoc way we use elsewhere: compare
the first char after the boundary with the last char of the completion
and drop one of the two if they're the same.

> +             (base-suffix (let ((suffix (buffer-substring (point) end)))
> +                            (substring
> +                             suffix
> +                             (cdr (completion-boundaries string
> +                                                         minibuffer-completion-table
> +                                                         minibuffer-completion-predicate
> +                                                         suffix)))))

I think you want to be careful to pass (buffer-substring start (point))
rather than `string` to `completion-boundaries`.

In theory this approach can "do the wrong thing" with some completion
styles, but AFAIK they haven't been written yet.  🙂


        Stefan






^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-08 23:50               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-10  1:33                 ` Dmitry Gutov
  2024-04-10  2:38                   ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 24+ messages in thread
From: Dmitry Gutov @ 2024-04-10  1:33 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 48356, Daniel Mendler, JD Smith, Juri Linkov

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

Hi Stefan,

On 09/04/2024 02:50, Stefan Monnier wrote:
>> ...which translates to "/" because of the double slash -- the filesystem
>>   root directory (*). But that's the same data which would be used by any
>>   other proposed solution, too.
> 
> More or less, tho the "ideal" solution is to do that in the
> completion-style code, which has a bit more knowledge about it.

Doing it in completion-style, though, would either require a relatively 
awkward change in most/all styles (e.g. the "new dynamic variable" 
route), or a more straightforward change in styles together with an 
incompatible change in completion-all-completions.

So on balance, would you say it's a good idea to a) use this approach in 
minibuffer-completion-help, b) create a named function for it, for other 
callers to take advantage of it as well?

IIUC Vertico (and other minibuffer completion UIs) might be interested.

>>   So maybe it should be either be fixed in the
>>   completion table (avoid adding trailing slash when the last boundary is
>>   already followed by slash?), or the insertion code should do some
>>   additional post-processing of the completion string.
> 
> I think you can fix it in the same ad-hoc way we use elsewhere: compare
> the first char after the boundary with the last char of the completion
> and drop one of the two if they're the same.

Looks like completion--merge-suffix is the helper to use.

>> +             (base-suffix (let ((suffix (buffer-substring (point) end)))
>> +                            (substring
>> +                             suffix
>> +                             (cdr (completion-boundaries string
>> +                                                         minibuffer-completion-table
>> +                                                         minibuffer-completion-predicate
>> +                                                         suffix)))))
> 
> I think you want to be careful to pass (buffer-substring start (point))
> rather than `string` to `completion-boundaries`.

Thanks, see the update attached.

> In theory this approach can "do the wrong thing" with some completion
> styles, but AFAIK they haven't been written yet.  🙂

So you figure that such theoretical style would return adjusted 
base-suffix in -all-completions method, not just in -try-completion?

[-- Attachment #2: base-suffix-v2.diff --]
[-- Type: text/x-patch, Size: 2456 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 41b20174be1..0cccc46448b 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2586,16 +2586,13 @@ minibuffer-completion-help
              (minibuffer-completion-base (substring string 0 base-size))
              (base-prefix (buffer-substring (minibuffer--completion-prompt-end)
                                             (+ start base-size)))
-             (base-suffix
-              (if (or (eq (alist-get 'category (cdr md)) 'file)
-                      completion-in-region-mode-predicate)
-                  (buffer-substring
-                   (save-excursion
-                     (if completion-in-region-mode-predicate
-                         (point)
-                       (or (search-forward "/" nil t) (point-max))))
-                   (point-max))
-                ""))
+             (base-suffix (let ((suffix (buffer-substring (point) end)))
+                            (substring
+                             suffix
+                             (cdr (completion-boundaries (buffer-substring start (point))
+                                                         minibuffer-completion-table
+                                                         minibuffer-completion-predicate
+                                                         suffix)))))
              (all-md (completion--metadata (buffer-substring-no-properties
                                             start (point))
                                            base-size md
@@ -2697,7 +2694,11 @@ minibuffer-completion-help
                                        (delete-minibuffer-contents)
                                        (insert start choice)
                                        ;; Keep point after completion before suffix
-                                       (save-excursion (insert end)))
+                                       (save-excursion (insert
+                                                        (completion--merge-suffix
+                                                         choice
+                                                         (1- (length choice))
+                                                         end))))
                                    (unless (or (zerop (length prefix))
                                                (equal prefix
                                                       (buffer-substring-no-properties

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-10  1:33                 ` Dmitry Gutov
@ 2024-04-10  2:38                   ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-11  1:00                     ` Dmitry Gutov
  0 siblings, 1 reply; 24+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-10  2:38 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 48356, Daniel Mendler, JD Smith, Juri Linkov

>>> ...which translates to "/" because of the double slash -- the filesystem
>>>   root directory (*). But that's the same data which would be used by any
>>>   other proposed solution, too.
>> More or less, tho the "ideal" solution is to do that in the
>> completion-style code, which has a bit more knowledge about it.
> Doing it in completion-style, though, would either require a relatively
> awkward change in most/all styles (e.g. the "new dynamic variable" route),
> or a more straightforward change in styles together with an incompatible
> change in completion-all-completions.

Yup, hence the quotes around "ideal".

> So on balance, would you say it's a good idea to a) use this approach in
> minibuffer-completion-help, b) create a named function for it, for other
> callers to take advantage of it as well?

Yes, while waiting for a new API it seems like a good stop gap.

> Looks like completion--merge-suffix is the helper to use.

Yup.

>> In theory this approach can "do the wrong thing" with some completion
>> styles, but AFAIK they haven't been written yet.  🙂
> So you figure that such theoretical style would return adjusted base-suffix
> in -all-completions method, not just in -try-completion?

A completion style could make use of (and list) things after the
boundary.  E.g. completing a file name like

    foo/ba<|>ar/baz

could decide to list all the files that match

    *f*o*o*/*b*a*a*r*/*b*a*z*

in which case the "end boundary" of the `completion-all-completions`
output should not be the `cdr` of the `completion-boundaries`.

I wouldn't worry about it, tho.


        Stefan






^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-10  2:38                   ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-11  1:00                     ` Dmitry Gutov
  2024-04-11  6:55                       ` Eli Zaretskii
  0 siblings, 1 reply; 24+ messages in thread
From: Dmitry Gutov @ 2024-04-11  1:00 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 48356, Daniel Mendler, JD Smith, Juri Linkov

On 10/04/2024 05:38, Stefan Monnier wrote:
>>>> ...which translates to "/" because of the double slash -- the filesystem
>>>>    root directory (*). But that's the same data which would be used by any
>>>>    other proposed solution, too.
>>> More or less, tho the "ideal" solution is to do that in the
>>> completion-style code, which has a bit more knowledge about it.
>> Doing it in completion-style, though, would either require a relatively
>> awkward change in most/all styles (e.g. the "new dynamic variable" route),
>> or a more straightforward change in styles together with an incompatible
>> change in completion-all-completions.
> 
> Yup, hence the quotes around "ideal".
> 
>> So on balance, would you say it's a good idea to a) use this approach in
>> minibuffer-completion-help, b) create a named function for it, for other
>> callers to take advantage of it as well?
> 
> Yes, while waiting for a new API it seems like a good stop gap.
> 
>> Looks like completion--merge-suffix is the helper to use.
> 
> Yup.
> 
>>> In theory this approach can "do the wrong thing" with some completion
>>> styles, but AFAIK they haven't been written yet.  🙂
>> So you figure that such theoretical style would return adjusted base-suffix
>> in -all-completions method, not just in -try-completion?
> 
> A completion style could make use of (and list) things after the
> boundary.  E.g. completing a file name like
> 
>      foo/ba<|>ar/baz
> 
> could decide to list all the files that match
> 
>      *f*o*o*/*b*a*a*r*/*b*a*z*
> 
> in which case the "end boundary" of the `completion-all-completions`
> output should not be the `cdr` of the `completion-boundaries`.
> 
> I wouldn't worry about it, tho.

All right, please see the new function completion-base-suffix added in 
0288bc6c949. Any docstring improvements (and others) are welcome.

I guess we should also mention it in NEWS...





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-11  1:00                     ` Dmitry Gutov
@ 2024-04-11  6:55                       ` Eli Zaretskii
  2024-04-11 10:36                         ` Dmitry Gutov
  2024-04-11 21:59                         ` Dmitry Gutov
  0 siblings, 2 replies; 24+ messages in thread
From: Eli Zaretskii @ 2024-04-11  6:55 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 48356, juri, monnier, jdtsmith, mail

> Cc: 48356@debbugs.gnu.org, Daniel Mendler <mail@daniel-mendler.de>,
>  JD Smith <jdtsmith@gmail.com>, Juri Linkov <juri@linkov.net>
> Date: Thu, 11 Apr 2024 04:00:35 +0300
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> All right, please see the new function completion-base-suffix added in 
> 0288bc6c949. Any docstring improvements (and others) are welcome.

I tried to do that.

Is there any reason why this function shouldn't be called
completion-boundary-suffix instead?

> I guess we should also mention it in NEWS...

Yes, please.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-11  6:55                       ` Eli Zaretskii
@ 2024-04-11 10:36                         ` Dmitry Gutov
  2024-04-11 21:59                         ` Dmitry Gutov
  1 sibling, 0 replies; 24+ messages in thread
From: Dmitry Gutov @ 2024-04-11 10:36 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 48356, juri, monnier, jdtsmith, mail

On 11/04/2024 09:55, Eli Zaretskii wrote:
>> Cc: 48356@debbugs.gnu.org, Daniel Mendler <mail@daniel-mendler.de>,
>>   JD Smith <jdtsmith@gmail.com>, Juri Linkov <juri@linkov.net>
>> Date: Thu, 11 Apr 2024 04:00:35 +0300
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>> All right, please see the new function completion-base-suffix added in
>> 0288bc6c949. Any docstring improvements (and others) are welcome.
> 
> I tried to do that.
> 
> Is there any reason why this function shouldn't be called
> completion-boundary-suffix instead?

For such a name, I would probably expect the return value to be

   (buffer-substring (point) (cdr (completion-boundaries ...)))

Opinions welcome.

>> I guess we should also mention it in NEWS...
> 
> Yes, please.






^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-11  6:55                       ` Eli Zaretskii
  2024-04-11 10:36                         ` Dmitry Gutov
@ 2024-04-11 21:59                         ` Dmitry Gutov
  2024-04-14 16:44                           ` Juri Linkov
  1 sibling, 1 reply; 24+ messages in thread
From: Dmitry Gutov @ 2024-04-11 21:59 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: mail, juri, 48356, monnier, jdtsmith, Visuwesh

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

On 11/04/2024 09:55, Eli Zaretskii wrote:
>> Cc: 48356@debbugs.gnu.org, Daniel Mendler <mail@daniel-mendler.de>,
>>   JD Smith <jdtsmith@gmail.com>, Juri Linkov <juri@linkov.net>
>> Date: Thu, 11 Apr 2024 04:00:35 +0300
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>> All right, please see the new function completion-base-suffix added in
>> 0288bc6c949. Any docstring improvements (and others) are welcome.
> 
> I tried to do that.
> 
> Is there any reason why this function shouldn't be called
> completion-boundary-suffix instead?
> 
>> I guess we should also mention it in NEWS...
> 
> Yes, please.

Sorry about the trouble, here is the next patch on top which essentially 
had to change the function's semantics to match the name above, except 
it needed just the length. Since that made it a very thin wrapper, I'm 
inlining the code back, no docstring or announcement needed.

What else this patch does:

* Removes the variables completion-use-base-affixes and 
completion-base-affixes, just using the completion-base-position 
variable, although with a marker for the field end.
* Changes 'completion--replace' to preserve the said marker.

The result is that the text outside of the current field boundaries is 
maintained for both minibuffer and in-buffer completion (in particular, 
the suffix).

As one downside, it brings back behavior described in 
https://debbugs.gnu.org/34517#14. That doesn't seem too critical to me, 
but opinions might vary.

So more feedback welcome.

Also Cc'ing Visuwesh who filed bug#49931 (related).

[-- Attachment #2: base-suffix-v3.diff --]
[-- Type: text/x-patch, Size: 10246 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ad6a0928cda..56827f509aa 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -112,20 +112,6 @@ completion-boundaries
     (cons (or (cadr boundaries) 0)
           (or (cddr boundaries) (length suffix)))))
 
-(defun completion-base-suffix (start end collection predicate)
-  "Return suffix of completion of buffer text between START and END.
-COLLECTION and PREDICATE are, respectively, the completion's
-completion table and predicate, as in `completion-boundaries' (which see).
-Value is a substring of buffer text between point and END.  It is
-the completion suffix that follows the completion boundary."
-  (let ((suffix (buffer-substring (point) end)))
-    (substring
-     suffix
-     (cdr (completion-boundaries (buffer-substring start (point))
-                                 collection
-                                 predicate
-                                 suffix)))))
-
 (defun completion-metadata (string table pred)
   "Return the metadata of elements to complete at the end of STRING.
 This metadata is an alist.  Currently understood keys are:
@@ -1377,7 +1363,7 @@ completion--replace
       (setq newtext (substring newtext 0 (- suffix-len))))
     (goto-char beg)
     (let ((length (- end beg)))         ;Read `end' before we insert the text.
-      (insert-and-inherit newtext)
+      (insert-before-markers-and-inherit newtext)
       (delete-region (point) (+ (point) length)))
     (forward-char suffix-len)))
 
@@ -2598,12 +2584,15 @@ minibuffer-completion-help
              (base-size (or (cdr last) 0))
              (prefix (unless (zerop base-size) (substring string 0 base-size)))
              (minibuffer-completion-base (substring string 0 base-size))
-             (base-prefix (buffer-substring (minibuffer--completion-prompt-end)
-                                            (+ start base-size)))
-             (base-suffix (concat (completion-base-suffix start end
-                                                          minibuffer-completion-table
-                                                          minibuffer-completion-predicate)
-                                  (buffer-substring end (point-max))))
+             (field-end
+              (save-excursion
+                (forward-char
+                 (cdr (completion-boundaries (buffer-substring start (point))
+                                             minibuffer-completion-table
+                                             minibuffer-completion-predicate
+                                             (buffer-substring (point) end))))
+                (point-marker)))
+             (field-char (and (< field-end end) (char-after field-end)))
              (all-md (completion--metadata (buffer-substring-no-properties
                                             start (point))
                                            base-size md
@@ -2687,38 +2676,25 @@ minibuffer-completion-help
 
                       (with-current-buffer standard-output
                         (setq-local completion-base-position
-                             (list (+ start base-size)
-                                   ;; FIXME: We should pay attention to completion
-                                   ;; boundaries here, but currently
-                                   ;; completion-all-completions does not give us the
-                                   ;; necessary information.
-                                   end))
-                        (setq-local completion-base-affixes
-                                    (list base-prefix base-suffix))
+                             (list (+ start base-size) field-end))
                         (setq-local completion-list-insert-choice-function
                              (let ((ctable minibuffer-completion-table)
                                    (cpred minibuffer-completion-predicate)
                                    (cprops completion-extra-properties))
                                (lambda (start end choice)
-                                 (if (and (stringp start) (stringp end))
-                                     (progn
-                                       (delete-minibuffer-contents)
-                                       (insert start choice)
-                                       ;; Keep point after completion before suffix
-                                       (save-excursion (insert
-                                                        (completion--merge-suffix
-                                                         choice
-                                                         (1- (length choice))
-                                                         end))))
-                                   (unless (or (zerop (length prefix))
-                                               (equal prefix
-                                                      (buffer-substring-no-properties
-                                                       (max (point-min)
-                                                            (- start (length prefix)))
-                                                       start)))
-                                     (message "*Completions* out of date"))
-                                   ;; FIXME: Use `md' to do quoting&terminator here.
-                                   (completion--replace start end choice))
+                                 (unless (or (zerop (length prefix))
+                                             (equal prefix
+                                                    (buffer-substring-no-properties
+                                                     (max (point-min)
+                                                          (- start (length prefix)))
+                                                     start)))
+                                   (message "*Completions* out of date"))
+                                 (when (and field-char
+                                            (= (aref choice (1- (length choice)))
+                                               field-char))
+                                   (setq end (1+ end)))
+                                 ;; FIXME: Use `md' to do quoting&terminator here.
+                                 (completion--replace start end choice)
                                  (let* ((minibuffer-completion-table ctable)
                                         (minibuffer-completion-predicate cpred)
                                         (completion-extra-properties cprops)
@@ -4877,8 +4853,7 @@ minibuffer-next-completion
           (next-line-completion (or n 1))
         (next-completion (or n 1)))
       (when auto-choose
-        (let ((completion-use-base-affixes t)
-              (completion-auto-deselect nil))
+        (let ((completion-auto-deselect nil))
           (choose-completion nil t t))))))
 
 (defun minibuffer-previous-completion (&optional n)
@@ -4916,8 +4891,7 @@ minibuffer-choose-completion
 minibuffer, but don't quit the completions window."
   (interactive "P")
   (with-minibuffer-completions-window
-    (let ((completion-use-base-affixes t))
-      (choose-completion nil no-exit no-quit))))
+    (choose-completion nil no-exit no-quit)))
 
 (defun minibuffer-choose-completion-or-exit (&optional no-exit no-quit)
   "Choose the completion from the minibuffer or exit the minibuffer.
diff --git a/lisp/simple.el b/lisp/simple.el
index e4629ce3db7..741cf59f344 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9856,16 +9856,6 @@ completion-base-position
 where the completion should be inserted and END (if non-nil) is the end
 of the text to replace.  If END is nil, point is used instead.")
 
-(defvar completion-base-affixes nil
-  "Base context of the text corresponding to the shown completions.
-This variable is used in the *Completions* buffer.
-Its value is a list of the form (PREFIX SUFFIX) where PREFIX is the text
-before the place where completion should be inserted, and SUFFIX is the text
-after the completion.")
-
-(defvar completion-use-base-affixes nil
-  "Non-nil means to restore original prefix and suffix in the minibuffer.")
-
 (defvar completion-list-insert-choice-function #'completion--replace
   "Function to use to insert the text chosen in *Completions*.
 Called with three arguments (BEG END TEXT), it should replace the text
@@ -10126,7 +10116,6 @@ choose-completion
   (with-current-buffer (window-buffer (posn-window (event-start event)))
     (let ((buffer completion-reference-buffer)
           (base-position completion-base-position)
-          (base-affixes completion-base-affixes)
           (insert-function completion-list-insert-choice-function)
           (completion-no-auto-exit (if no-exit t completion-no-auto-exit))
           (choice
@@ -10159,13 +10148,7 @@ choose-completion
       (with-current-buffer buffer
         (choose-completion-string
          choice buffer
-         ;; Don't allow affixes to replace the whole buffer when not
-         ;; in the minibuffer.  Thus check for `completion-in-region-mode'
-         ;; to ignore non-nil value of `completion-use-base-affixes' set by
-         ;; `minibuffer-choose-completion'.
-         (or (and (not completion-in-region-mode)
-                  completion-use-base-affixes base-affixes)
-             base-position
+         (or base-position
              ;; If all else fails, just guess.
              (list (choose-completion-guess-base-position choice)))
          insert-function)))))
@@ -10321,11 +10304,9 @@ completion-setup-function
                 (buffer-substring (minibuffer-prompt-end) (point)))))))
     (with-current-buffer standard-output
       (let ((base-position completion-base-position)
-            (base-affixes completion-base-affixes)
             (insert-fun completion-list-insert-choice-function))
         (completion-list-mode)
         (setq-local completion-base-position base-position)
-        (setq-local completion-base-affixes base-affixes)
         (setq-local completion-list-insert-choice-function insert-fun))
       (setq-local completion-reference-buffer mainbuf)
       (if base-dir (setq default-directory base-dir))

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-11 21:59                         ` Dmitry Gutov
@ 2024-04-14 16:44                           ` Juri Linkov
  2024-04-14 23:55                             ` Dmitry Gutov
  0 siblings, 1 reply; 24+ messages in thread
From: Juri Linkov @ 2024-04-14 16:44 UTC (permalink / raw)
  To: Dmitry Gutov
  Cc: Spencer Baugh, mail, 48356, monnier, jdtsmith, Visuwesh,
	Eli Zaretskii

> As one downside, it brings back behavior described in
> https://debbugs.gnu.org/34517#14. That doesn't seem too critical to me, but
> opinions might vary.

Sadly, this is quite an important case.  Recently Spencer implemented
a way to deselect a candidate in the visible completions list
(minibuffer-visible-completions=t) when the user starts typing
in the minibuffer.  But then the user could change the mind
and still select a candidate.  This would interfere with the
contents of the minibuffer.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-14 16:44                           ` Juri Linkov
@ 2024-04-14 23:55                             ` Dmitry Gutov
  2024-04-18 14:25                               ` Spencer Baugh
  0 siblings, 1 reply; 24+ messages in thread
From: Dmitry Gutov @ 2024-04-14 23:55 UTC (permalink / raw)
  To: Juri Linkov
  Cc: Spencer Baugh, mail, 48356, monnier, jdtsmith, Visuwesh,
	Eli Zaretskii

Hi Juri,

On 14/04/2024 19:44, Juri Linkov wrote:
>> As one downside, it brings back behavior described in
>> https://debbugs.gnu.org/34517#14. That doesn't seem too critical to me, but
>> opinions might vary.
> Sadly, this is quite an important case.  Recently Spencer implemented
> a way to deselect a candidate in the visible completions list
> (minibuffer-visible-completions=t) when the user starts typing
> in the minibuffer.

I think the (admittedly pretty cool) minibuffer-visible-completions 
feature is orthogonal: the scenarios I'm considering and trying to fix 
here also involve selecting a completion from *Completions* in some way 
(e.g. using M-up or M-down followed by M-RET, in default configuration). 
And this is currently working worse for in-buffer completion than for 
minibuffer completion WRT keeping the suffix around.

> But then the user could change the mind
> and still select a candidate.  This would interfere with the
> contents of the minibuffer.

Suppose they do. But the list of completions they're shown is for an 
outdated input. Does it really make more sense to erase the current 
input than to insert the completion where it was supposed to go?

The problem here, from my POV, is that we currently have a solution 
which matches the above goal, but which only makes sense for minibuffer 
(I think you wouldn't store the before/after buffer contents in separate 
string variables the same way). Whereas the API used is the same, so it 
would really make sense to minimize the differences in behavior between 
minibuffer and in-buffer completion.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-14 23:55                             ` Dmitry Gutov
@ 2024-04-18 14:25                               ` Spencer Baugh
  2024-04-20  0:12                                 ` Dmitry Gutov
  0 siblings, 1 reply; 24+ messages in thread
From: Spencer Baugh @ 2024-04-18 14:25 UTC (permalink / raw)
  To: Dmitry Gutov
  Cc: mail, Juri Linkov, 48356, monnier, jdtsmith, Visuwesh,
	Eli Zaretskii

Dmitry Gutov <dmitry@gutov.dev> writes:
> Hi Juri,
>
> On 14/04/2024 19:44, Juri Linkov wrote:
>>> As one downside, it brings back behavior described in
>>> https://debbugs.gnu.org/34517#14. That doesn't seem too critical to me, but
>>> opinions might vary.
>> Sadly, this is quite an important case.  Recently Spencer implemented
>> a way to deselect a candidate in the visible completions list
>> (minibuffer-visible-completions=t) when the user starts typing
>> in the minibuffer.
>
> I think the (admittedly pretty cool) minibuffer-visible-completions
> feature is orthogonal: the scenarios I'm considering and trying to fix
> here also involve selecting a completion from *Completions* in some
> way (e.g. using M-up or M-down followed by M-RET, in default
> configuration). And this is currently working worse for in-buffer
> completion than for minibuffer completion WRT keeping the suffix
> around.

Yes, e.g.:

1. emacs -Q
2. M-x shell
3. mkdir -p dir1 dir2 && touch dir1/foo dir2/foo
4. cat dir/foo TAB
5. *Completions* contains: 2 possible completions: dir1/ dir2/
6. Click "dir1/"
7. The buffer now contains "cat dir1/", the "foo" has been deleted

This is a bug, but moreover it's also inconsistent with minibuffer
completion, e.g.:

8. C-x C-f dir/foo TAB
9. *Completions* contains: 2 possible completions: dir1/ dir2/
10. Click "dir1/"
11. The minibuffer now contains "dir1/foo"

We should make them both behave the same way.

>> But then the user could change the mind
>> and still select a candidate.  This would interfere with the
>> contents of the minibuffer.
>
> Suppose they do. But the list of completions they're shown is for an
> outdated input. Does it really make more sense to erase the current
> input than to insert the completion where it was supposed to go?

Yeah, this patch changes the behavior in the case of an outdated
*Completions* buffer, and the new behavior is buggy, but I think the old
behavior was also buggy and the new behavior is better.

Example: In emacs -Q, if I type

C-x C-f ~/src/emacs/emacs-2 <TAB>

I get completions "emacs-28" and "emacs-29"

Suppose I then type /src (without hitting tab or updating the
*Completions* buffer) so that the minibuffer contains:

~/src/emacs/emacs-2/src

Then I click on one of the completions to choose it.

Before this patch the minibuffer will contain:

~/src/emacs/emacs-28/

and after this patch it will contain

~/src/emacs/emacs-28//src

Both of these are wrong IMO, but IMO the second one is closer to
correct, and it's more consistent with the normal case (when
*Completions* is up to date) and with in-buffer completion behavior, so
I think this patch is an improvement and could be installed.

Still, maybe we can get it to the point where clicking on an outdated
completion makes the minibuffer contain

~/src/emacs/emacs-28/src

instead?  Which I think is the correct behavior.

But I don't think we need to do that before landing this patch.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-18 14:25                               ` Spencer Baugh
@ 2024-04-20  0:12                                 ` Dmitry Gutov
  2024-05-04  2:23                                   ` Dmitry Gutov
  0 siblings, 1 reply; 24+ messages in thread
From: Dmitry Gutov @ 2024-04-20  0:12 UTC (permalink / raw)
  To: Spencer Baugh
  Cc: mail, Juri Linkov, 48356, monnier, jdtsmith, Visuwesh,
	Eli Zaretskii

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

On 18/04/2024 17:25, Spencer Baugh wrote:

>>> But then the user could change the mind
>>> and still select a candidate.  This would interfere with the
>>> contents of the minibuffer.
>>
>> Suppose they do. But the list of completions they're shown is for an
>> outdated input. Does it really make more sense to erase the current
>> input than to insert the completion where it was supposed to go?
> 
> Yeah, this patch changes the behavior in the case of an outdated
> *Completions* buffer, and the new behavior is buggy, but I think the old
> behavior was also buggy and the new behavior is better.
> 
> Example: In emacs -Q, if I type
> 
> C-x C-f ~/src/emacs/emacs-2 <TAB>
> 
> I get completions "emacs-28" and "emacs-29"
> 
> Suppose I then type /src (without hitting tab or updating the
> *Completions* buffer) so that the minibuffer contains:
> 
> ~/src/emacs/emacs-2/src
> 
> Then I click on one of the completions to choose it.
> 
> Before this patch the minibuffer will contain:
> 
> ~/src/emacs/emacs-28/
> 
> and after this patch it will contain
> 
> ~/src/emacs/emacs-28//src
> 
> Both of these are wrong IMO, but IMO the second one is closer to
> correct, and it's more consistent with the normal case (when
> *Completions* is up to date) and with in-buffer completion behavior, so
> I think this patch is an improvement and could be installed.
> 
> Still, maybe we can get it to the point where clicking on an outdated
> completion makes the minibuffer contain
> 
> ~/src/emacs/emacs-28/src
> 
> instead?  Which I think is the correct behavior.
> 
> But I don't think we need to do that before landing this patch.

That gave me an idea. With a bit more spaghetti, we can support both 
this scenario and the others mentioned previously that didn't have field 
boundaries in the "new" input:

Whenever POINT > END, we can re-query the completion boundaries again 
inside completion-list-insert-choice-function and adjust END. In theory, 
I guess we'd ideally also recompute the completion entity first (to pass 
the correct prefix and suffix), but the capf function is not in context 
anymore. As long as the field boundaries simply looks for chars in 
passed string, this should work fine.

The attached v4 adds a new step and also fixes an issue apparently 
introduced in 2021 with 0ce2f591ff9 when making 
minibuffer-completion-table and others buffer-local. The bindings for 
CTABLE and other 2 vars currently always set them to nil because the 
variables are looked up right after the current buffer is changed 
(with-current-buffer standard-output ...).

This can affect the completion--done call at the end, reverting it to 
the previous behavior. But nobody noticed the change, so...

[-- Attachment #2: base-suffix-v4.diff --]
[-- Type: text/x-patch, Size: 12043 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ad6a0928cda..61395577035 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -112,20 +112,6 @@ completion-boundaries
     (cons (or (cadr boundaries) 0)
           (or (cddr boundaries) (length suffix)))))
 
-(defun completion-base-suffix (start end collection predicate)
-  "Return suffix of completion of buffer text between START and END.
-COLLECTION and PREDICATE are, respectively, the completion's
-completion table and predicate, as in `completion-boundaries' (which see).
-Value is a substring of buffer text between point and END.  It is
-the completion suffix that follows the completion boundary."
-  (let ((suffix (buffer-substring (point) end)))
-    (substring
-     suffix
-     (cdr (completion-boundaries (buffer-substring start (point))
-                                 collection
-                                 predicate
-                                 suffix)))))
-
 (defun completion-metadata (string table pred)
   "Return the metadata of elements to complete at the end of STRING.
 This metadata is an alist.  Currently understood keys are:
@@ -1377,7 +1363,7 @@ completion--replace
       (setq newtext (substring newtext 0 (- suffix-len))))
     (goto-char beg)
     (let ((length (- end beg)))         ;Read `end' before we insert the text.
-      (insert-and-inherit newtext)
+      (insert-before-markers-and-inherit newtext)
       (delete-region (point) (+ (point) length)))
     (forward-char suffix-len)))
 
@@ -2598,17 +2584,23 @@ minibuffer-completion-help
              (base-size (or (cdr last) 0))
              (prefix (unless (zerop base-size) (substring string 0 base-size)))
              (minibuffer-completion-base (substring string 0 base-size))
-             (base-prefix (buffer-substring (minibuffer--completion-prompt-end)
-                                            (+ start base-size)))
-             (base-suffix (concat (completion-base-suffix start end
-                                                          minibuffer-completion-table
-                                                          minibuffer-completion-predicate)
-                                  (buffer-substring end (point-max))))
+             (ctable minibuffer-completion-table)
+             (cpred minibuffer-completion-predicate)
+             (cprops completion-extra-properties)
+             (field-end
+              (save-excursion
+                (forward-char
+                 (cdr (completion-boundaries (buffer-substring start (point))
+                                             ctable
+                                             cpred
+                                             (buffer-substring (point) end))))
+                (point-marker)))
+             (field-char (and (< field-end end) (char-after field-end)))
              (all-md (completion--metadata (buffer-substring-no-properties
                                             start (point))
                                            base-size md
-                                           minibuffer-completion-table
-                                           minibuffer-completion-predicate))
+                                           ctable
+                                           cpred))
              (ann-fun (completion-metadata-get all-md 'annotation-function))
              (aff-fun (completion-metadata-get all-md 'affixation-function))
              (sort-fun (completion-metadata-get all-md 'display-sort-function))
@@ -2687,38 +2679,31 @@ minibuffer-completion-help
 
                       (with-current-buffer standard-output
                         (setq-local completion-base-position
-                             (list (+ start base-size)
-                                   ;; FIXME: We should pay attention to completion
-                                   ;; boundaries here, but currently
-                                   ;; completion-all-completions does not give us the
-                                   ;; necessary information.
-                                   end))
-                        (setq-local completion-base-affixes
-                                    (list base-prefix base-suffix))
+                                    (list (+ start base-size) field-end))
                         (setq-local completion-list-insert-choice-function
-                             (let ((ctable minibuffer-completion-table)
-                                   (cpred minibuffer-completion-predicate)
-                                   (cprops completion-extra-properties))
                                (lambda (start end choice)
-                                 (if (and (stringp start) (stringp end))
-                                     (progn
-                                       (delete-minibuffer-contents)
-                                       (insert start choice)
-                                       ;; Keep point after completion before suffix
-                                       (save-excursion (insert
-                                                        (completion--merge-suffix
-                                                         choice
-                                                         (1- (length choice))
-                                                         end))))
-                                   (unless (or (zerop (length prefix))
-                                               (equal prefix
-                                                      (buffer-substring-no-properties
-                                                       (max (point-min)
-                                                            (- start (length prefix)))
-                                                       start)))
-                                     (message "*Completions* out of date"))
-                                   ;; FIXME: Use `md' to do quoting&terminator here.
-                                   (completion--replace start end choice))
+                                 (unless (or (zerop (length prefix))
+                                             (equal prefix
+                                                    (buffer-substring-no-properties
+                                                     (max (point-min)
+                                                          (- start (length prefix)))
+                                                     start)))
+                                   (message "*Completions* out of date"))
+                                 (when (> (point) end)
+                                   ;; Completion suffix has changed, have to adapt.
+                                   (setq end (+ end
+                                                (cdr (completion-boundaries
+                                                      (concat prefix choice) ctable cpred
+                                                      (buffer-substring end (point))))))
+                                   ;; Stopped before some field boundary.
+                                   (when (> (point) end)
+                                     (setq field-char (char-after end))))
+                                 (when (and field-char
+                                            (= (aref choice (1- (length choice)))
+                                               field-char))
+                                   (setq end (1+ end)))
+                                 ;; FIXME: Use `md' to do quoting&terminator here.
+                                 (completion--replace start end choice)
                                  (let* ((minibuffer-completion-table ctable)
                                         (minibuffer-completion-predicate cpred)
                                         (completion-extra-properties cprops)
@@ -2729,7 +2714,7 @@ minibuffer-completion-help
                                    ;; completion is not finished.
                                    (completion--done result
                                                      (if (eq (car bounds) (length result))
-                                                         'exact 'finished)))))))
+                                                         'exact 'finished))))))
 
                       (display-completion-list completions nil group-fun)))))
           nil)))
@@ -4877,8 +4862,7 @@ minibuffer-next-completion
           (next-line-completion (or n 1))
         (next-completion (or n 1)))
       (when auto-choose
-        (let ((completion-use-base-affixes t)
-              (completion-auto-deselect nil))
+        (let ((completion-auto-deselect nil))
           (choose-completion nil t t))))))
 
 (defun minibuffer-previous-completion (&optional n)
@@ -4916,8 +4900,7 @@ minibuffer-choose-completion
 minibuffer, but don't quit the completions window."
   (interactive "P")
   (with-minibuffer-completions-window
-    (let ((completion-use-base-affixes t))
-      (choose-completion nil no-exit no-quit))))
+    (choose-completion nil no-exit no-quit)))
 
 (defun minibuffer-choose-completion-or-exit (&optional no-exit no-quit)
   "Choose the completion from the minibuffer or exit the minibuffer.
diff --git a/lisp/simple.el b/lisp/simple.el
index e4629ce3db7..741cf59f344 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9856,16 +9856,6 @@ completion-base-position
 where the completion should be inserted and END (if non-nil) is the end
 of the text to replace.  If END is nil, point is used instead.")
 
-(defvar completion-base-affixes nil
-  "Base context of the text corresponding to the shown completions.
-This variable is used in the *Completions* buffer.
-Its value is a list of the form (PREFIX SUFFIX) where PREFIX is the text
-before the place where completion should be inserted, and SUFFIX is the text
-after the completion.")
-
-(defvar completion-use-base-affixes nil
-  "Non-nil means to restore original prefix and suffix in the minibuffer.")
-
 (defvar completion-list-insert-choice-function #'completion--replace
   "Function to use to insert the text chosen in *Completions*.
 Called with three arguments (BEG END TEXT), it should replace the text
@@ -10126,7 +10116,6 @@ choose-completion
   (with-current-buffer (window-buffer (posn-window (event-start event)))
     (let ((buffer completion-reference-buffer)
           (base-position completion-base-position)
-          (base-affixes completion-base-affixes)
           (insert-function completion-list-insert-choice-function)
           (completion-no-auto-exit (if no-exit t completion-no-auto-exit))
           (choice
@@ -10159,13 +10148,7 @@ choose-completion
       (with-current-buffer buffer
         (choose-completion-string
          choice buffer
-         ;; Don't allow affixes to replace the whole buffer when not
-         ;; in the minibuffer.  Thus check for `completion-in-region-mode'
-         ;; to ignore non-nil value of `completion-use-base-affixes' set by
-         ;; `minibuffer-choose-completion'.
-         (or (and (not completion-in-region-mode)
-                  completion-use-base-affixes base-affixes)
-             base-position
+         (or base-position
              ;; If all else fails, just guess.
              (list (choose-completion-guess-base-position choice)))
          insert-function)))))
@@ -10321,11 +10304,9 @@ completion-setup-function
                 (buffer-substring (minibuffer-prompt-end) (point)))))))
     (with-current-buffer standard-output
       (let ((base-position completion-base-position)
-            (base-affixes completion-base-affixes)
             (insert-fun completion-list-insert-choice-function))
         (completion-list-mode)
         (setq-local completion-base-position base-position)
-        (setq-local completion-base-affixes base-affixes)
         (setq-local completion-list-insert-choice-function insert-fun))
       (setq-local completion-reference-buffer mainbuf)
       (if base-dir (setq default-directory base-dir))

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-04-20  0:12                                 ` Dmitry Gutov
@ 2024-05-04  2:23                                   ` Dmitry Gutov
  2024-05-09  2:33                                     ` Dmitry Gutov
  0 siblings, 1 reply; 24+ messages in thread
From: Dmitry Gutov @ 2024-05-04  2:23 UTC (permalink / raw)
  To: Spencer Baugh
  Cc: mail, Juri Linkov, 48356, monnier, jdtsmith, Visuwesh,
	Eli Zaretskii

On 20/04/2024 03:12, Dmitry Gutov wrote:
> The attached v4 adds a new step and also fixes an issue apparently 
> introduced in 2021 with 0ce2f591ff9 when making 
> minibuffer-completion-table and others buffer-local.

Unless anybody has objections, I'd like to install this patch soon-ish.

I think it addresses all the problem scenarios that had been brought up 
so far.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary
  2024-05-04  2:23                                   ` Dmitry Gutov
@ 2024-05-09  2:33                                     ` Dmitry Gutov
  0 siblings, 0 replies; 24+ messages in thread
From: Dmitry Gutov @ 2024-05-09  2:33 UTC (permalink / raw)
  To: Spencer Baugh
  Cc: mail, Juri Linkov, 48356, monnier, jdtsmith, Visuwesh,
	Eli Zaretskii

On 04/05/2024 05:23, Dmitry Gutov wrote:
> On 20/04/2024 03:12, Dmitry Gutov wrote:
>> The attached v4 adds a new step and also fixes an issue apparently 
>> introduced in 2021 with 0ce2f591ff9 when making 
>> minibuffer-completion-table and others buffer-local.
> 
> Unless anybody has objections, I'd like to install this patch soon-ish.
> 
> I think it addresses all the problem scenarios that had been brought up 
> so far.

Now installed. Please report any new problems.





^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2024-05-09  2:33 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-11 17:23 bug#48356: 28.0.50; choose-completion discards the suffix after the completion boundary Daniel Mendler
2022-03-13 17:56 ` Juri Linkov
2022-03-13 20:35   ` bug#48356: [External] : " Drew Adams
2022-03-14  3:10 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-03-14 18:53   ` Juri Linkov
2022-03-14 20:55     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-03-15  2:14       ` Daniel Mendler
2022-03-15  7:53         ` Juri Linkov
2022-03-20 20:34           ` Juri Linkov
2024-04-08 21:59             ` Dmitry Gutov
2024-04-08 22:27               ` Dmitry Gutov
2024-04-08 23:50               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-10  1:33                 ` Dmitry Gutov
2024-04-10  2:38                   ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-11  1:00                     ` Dmitry Gutov
2024-04-11  6:55                       ` Eli Zaretskii
2024-04-11 10:36                         ` Dmitry Gutov
2024-04-11 21:59                         ` Dmitry Gutov
2024-04-14 16:44                           ` Juri Linkov
2024-04-14 23:55                             ` Dmitry Gutov
2024-04-18 14:25                               ` Spencer Baugh
2024-04-20  0:12                                 ` Dmitry Gutov
2024-05-04  2:23                                   ` Dmitry Gutov
2024-05-09  2:33                                     ` Dmitry Gutov

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).