Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Eshel Yaron]

Hi Dmitry,

Dmitry Gutov <dgutov@yandex.ru> writes:

> branch: master
> commit ff3f17ca3cdd9e82355942f577e7807acc76ddcd
> Author: Dmitry Gutov <dmitry@gutov.dev>
> Commit: Dmitry Gutov <dmitry@gutov.dev>
>
>     choose-completion: Retain the suffix after completion boundary
>
>     * lisp/minibuffer.el (completion-base-suffix):
>     Remove as not optimal after all (bug#48356).
>     (completion--replace): Use insert-before-markers-and-inherit.

[...]

> -      (insert-and-inherit newtext)
> +      (insert-before-markers-and-inherit newtext)

It seems that this change breaks the cycling behavior of
minibuffer-force-complete.  You can see the problem by saying:

1. emacs -Q
2. (setq completion-cycle-threshold t)
3. Open some buffers
4. C-x b TAB TAB TAB ...

The repeated TAB presses are expected to cycle through the completion
candidates (this is the behavior in Emacs 29 for example) but now each
TAB appends to the minibuffer input instead of replacing it.


Best,

Eshel

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Dmitry Gutov]

Hi Eshel,

On 10/05/2024 19:10, Eshel Yaron wrote:

>> branch: master
>> commit ff3f17ca3cdd9e82355942f577e7807acc76ddcd
>> Author: Dmitry Gutov <dmitry@gutov.dev>
>> Commit: Dmitry Gutov <dmitry@gutov.dev>
>>
>>      choose-completion: Retain the suffix after completion boundary
>>
>>      * lisp/minibuffer.el (completion-base-suffix):
>>      Remove as not optimal after all (bug#48356).
>>      (completion--replace): Use insert-before-markers-and-inherit.
> 
> [...]
> 
>> -      (insert-and-inherit newtext)
>> +      (insert-before-markers-and-inherit newtext)
> 
> It seems that this change breaks the cycling behavior of
> minibuffer-force-complete.  You can see the problem by saying:
> 
> 1. emacs -Q
> 2. (setq completion-cycle-threshold t)
> 3. Open some buffers
> 4. C-x b TAB TAB TAB ...
> 
> The repeated TAB presses are expected to cycle through the completion
> candidates (this is the behavior in Emacs 29 for example) but now each
> TAB appends to the minibuffer input instead of replacing it.

Thanks for reporting that. Indeed cycling was a special case because it 
tracks BEG with a marker.

This should be fixed in commit f0009c2f5ae, hopefully without regressing 
on other cases.

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Eshel Yaron]

Dmitry Gutov <dmitry@gutov.dev> writes:

> Hi Eshel,
>
> On 10/05/2024 19:10, Eshel Yaron wrote:
>
>>> branch: master
>>> commit ff3f17ca3cdd9e82355942f577e7807acc76ddcd
>>> Author: Dmitry Gutov <dmitry@gutov.dev>
>>> Commit: Dmitry Gutov <dmitry@gutov.dev>
>>>
>>>      choose-completion: Retain the suffix after completion boundary
>>>
>>>      * lisp/minibuffer.el (completion-base-suffix):
>>>      Remove as not optimal after all (bug#48356).
>>>      (completion--replace): Use insert-before-markers-and-inherit.
>> [...]
>>
>>> -      (insert-and-inherit newtext)
>>> +      (insert-before-markers-and-inherit newtext)
>> It seems that this change breaks the cycling behavior of
>> minibuffer-force-complete.  You can see the problem by saying:
>> 1. emacs -Q
>> 2. (setq completion-cycle-threshold t)
>> 3. Open some buffers
>> 4. C-x b TAB TAB TAB ...
>> The repeated TAB presses are expected to cycle through the
>> completion
>> candidates (this is the behavior in Emacs 29 for example) but now each
>> TAB appends to the minibuffer input instead of replacing it.
>
> Thanks for reporting that. Indeed cycling was a special case because
> it tracks BEG with a marker.
>
> This should be fixed in commit f0009c2f5ae, hopefully without
> regressing on other cases.

Great, thanks!
I can't seem to find this commit, though, is that on master?

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Dmitry Gutov]

On 12/05/2024 15:38, Eshel Yaron wrote:
> Dmitry Gutov<dmitry@gutov.dev>  writes:
> 
>> Hi Eshel,
>>
>> On 10/05/2024 19:10, Eshel Yaron wrote:
>>
>>>> branch: master
>>>> commit ff3f17ca3cdd9e82355942f577e7807acc76ddcd
>>>> Author: Dmitry Gutov<dmitry@gutov.dev>
>>>> Commit: Dmitry Gutov<dmitry@gutov.dev>
>>>>
>>>>       choose-completion: Retain the suffix after completion boundary
>>>>
>>>>       * lisp/minibuffer.el (completion-base-suffix):
>>>>       Remove as not optimal after all (bug#48356).
>>>>       (completion--replace): Use insert-before-markers-and-inherit.
>>> [...]
>>>
>>>> -      (insert-and-inherit newtext)
>>>> +      (insert-before-markers-and-inherit newtext)
>>> It seems that this change breaks the cycling behavior of
>>> minibuffer-force-complete.  You can see the problem by saying:
>>> 1. emacs -Q
>>> 2. (setq completion-cycle-threshold t)
>>> 3. Open some buffers
>>> 4. C-x b TAB TAB TAB ...
>>> The repeated TAB presses are expected to cycle through the
>>> completion
>>> candidates (this is the behavior in Emacs 29 for example) but now each
>>> TAB appends to the minibuffer input instead of replacing it.
>> Thanks for reporting that. Indeed cycling was a special case because
>> it tracks BEG with a marker.
>>
>> This should be fixed in commit f0009c2f5ae, hopefully without
>> regressing on other cases.
> Great, thanks!
> I can't seem to find this commit, though, is that on master?

My bad, that push didn't go through. Now on master.

The new commit hash is 2c759b9ce620.

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Eshel Yaron]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 12/05/2024 15:38, Eshel Yaron wrote:
>
>> I can't seem to find this commit, though, is that on master?
>
> My bad, that push didn't go through. Now on master.

Thanks, everything seems to work well now.

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Eshel Yaron]

Hi Dmitry,

Eshel Yaron <me@eshelyaron.com> writes:

> Dmitry Gutov <dmitry@gutov.dev> writes:
>
>> On 12/05/2024 15:38, Eshel Yaron wrote:
>>
>>> I can't seem to find this commit, though, is that on master?
>>
>> My bad, that push didn't go through. Now on master.
>
> Thanks, everything seems to work well now.

I'm afraid there might be another regression, here's what I see:

1. emacs -Q
2. C-x C-f
3. C-a C-k to clear the minibuffer
4. Insert the path an Emacs checkout, e.g. "~/checkouts/emacs/"
5. Further insert "l/mi"
6. Hit ? to pop the completions list, which includes
   "lisp/minibuffer.el" and "leim/MISC-DIC/", among others.
7. Lean on M-<down> to cycle between completion candidates.

After the last fix, I see M-<down> accumulate text after point in the
minibuffer instead of replacing the part it previously inserted.  Do you
see that as well?


Thanks,

Eshel

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Dmitry Gutov]

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

Hi Eshel,

On 17/05/2024 11:56, Eshel Yaron wrote:
> Hi Dmitry,
> 
> Eshel Yaron <me@eshelyaron.com> writes:
> 
>> Dmitry Gutov <dmitry@gutov.dev> writes:
>>
>>> On 12/05/2024 15:38, Eshel Yaron wrote:
>>>
>>>> I can't seem to find this commit, though, is that on master?
>>>
>>> My bad, that push didn't go through. Now on master.
>>
>> Thanks, everything seems to work well now.
> 
> I'm afraid there might be another regression, here's what I see:

Thanks for keeping an eye out.

> 1. emacs -Q
> 2. C-x C-f
> 3. C-a C-k to clear the minibuffer
> 4. Insert the path an Emacs checkout, e.g. "~/checkouts/emacs/"
> 5. Further insert "l/mi"
> 6. Hit ? to pop the completions list, which includes
>     "lisp/minibuffer.el" and "leim/MISC-DIC/", among others.

FWIW, I don't see "leim/MISC-DIC" among completions, even if I first 
evaluate (setq completion-ignore-case t). But the crux of the problem 
reproduces fine.

> 7. Lean on M-<down> to cycle between completion candidates.
> 
> After the last fix, I see M-<down> accumulate text after point in the
> minibuffer instead of replacing the part it previously inserted.  Do you
> see that as well?

Please take the attached patch for a spin.

It would be great if someone looked into writing integration tests for 
this somehow. This is brittle stuff, and the variety of scenarios is 
high enough.

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

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index fbd49b569a8..f62cb2566b2 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2596,6 +2596,7 @@ 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
@@ -2678,8 +2679,7 @@ minibuffer-completion-help
                                       completions))))
 
                       (with-current-buffer standard-output
-                        (setq-local completion-base-position
-                                    (list (+ start base-size) field-end))
+                        (setq-local completion-base-position base-position)
                         (setq-local completion-list-insert-choice-function
                                (lambda (start end choice)
                                  (unless (or (zerop (length prefix))
@@ -2702,9 +2702,12 @@ minibuffer-completion-help
                                             (= (aref choice (1- (length choice)))
                                                field-char))
                                    (setq end (1+ end)))
-                                 (cl-decf field-end (- end start (length choice)))
+                                 ;; Tried to use a marker to track buffer changes
+                                 ;; but that clashed with another existing marker.
+                                 (cl-decf (nth 1 base-position)
+                                          (- end start (length choice)))
                                  ;; FIXME: Use `md' to do quoting&terminator here.
-                                 (completion--replace start end choice)
+                                 (completion--replace start (min end (point-max)) choice)
                                  (let* ((minibuffer-completion-table ctable)
                                         (minibuffer-completion-predicate cpred)
                                         (completion-extra-properties cprops)

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Eshel Yaron]

Dmitry Gutov <dmitry@gutov.dev> writes:

> Hi Eshel,
>
> On 17/05/2024 11:56, Eshel Yaron wrote:
[...]
>> I'm afraid there might be another regression, here's what I see:
>
> Thanks for keeping an eye out.
>
>> 1. emacs -Q
>> 2. C-x C-f
>> 3. C-a C-k to clear the minibuffer
>> 4. Insert the path an Emacs checkout, e.g. "~/checkouts/emacs/"
>> 5. Further insert "l/mi"
>> 6. Hit ? to pop the completions list, which includes
>>     "lisp/minibuffer.el" and "leim/MISC-DIC/", among others.
>
> FWIW, I don't see "leim/MISC-DIC" among completions, even if I first
> evaluate (setq completion-ignore-case t).

Weird, perhaps you have read-file-name-completion-ignore-case set to nil
and that takes effect rather than plain completion-ignore-case?

> But the crux of the problem reproduces fine.

Great.

>> 7. Lean on M-<down> to cycle between completion candidates.
>> After the last fix, I see M-<down> accumulate text after point in
>> the
>> minibuffer instead of replacing the part it previously inserted.  Do you
>> see that as well?
>
> Please take the attached patch for a spin.

Nice, this seems to fix the regression at hand, and so far I'm not
seeing any other issues.

> It would be great if someone looked into writing integration tests for
> this somehow. This is brittle stuff, and the variety of scenarios is
> high enough.

I agree.  The default completion UI supports various interactions that
can break in subtle ways, and that's without even configuring stuff.
I'll try to code a couple of test cases when I have the time.


Best,

Eshel

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Dmitry Gutov]

On 18/05/2024 09:55, Eshel Yaron wrote:
> Dmitry Gutov <dmitry@gutov.dev> writes:
> 
>> Hi Eshel,
>>
>> On 17/05/2024 11:56, Eshel Yaron wrote:
> [...]
>>> I'm afraid there might be another regression, here's what I see:
>>
>> Thanks for keeping an eye out.
>>
>>> 1. emacs -Q
>>> 2. C-x C-f
>>> 3. C-a C-k to clear the minibuffer
>>> 4. Insert the path an Emacs checkout, e.g. "~/checkouts/emacs/"
>>> 5. Further insert "l/mi"
>>> 6. Hit ? to pop the completions list, which includes
>>>      "lisp/minibuffer.el" and "leim/MISC-DIC/", among others.
>>
>> FWIW, I don't see "leim/MISC-DIC" among completions, even if I first
>> evaluate (setq completion-ignore-case t).
> 
> Weird, perhaps you have read-file-name-completion-ignore-case set to nil
> and that takes effect rather than plain completion-ignore-case?

Indeed, it's nil. I started with 'emacs -Q' to avoid my settings' 
interference, and assumed your scenario used it as well. Anyway...

>> But the crux of the problem reproduces fine.
> 
> Great.
> 
>>> 7. Lean on M-<down> to cycle between completion candidates.
>>> After the last fix, I see M-<down> accumulate text after point in
>>> the
>>> minibuffer instead of replacing the part it previously inserted.  Do you
>>> see that as well?
>>
>> Please take the attached patch for a spin.
> 
> Nice, this seems to fix the regression at hand, and so far I'm not
> seeing any other issues.

Great! I've pushed it to master now.

>> It would be great if someone looked into writing integration tests for
>> this somehow. This is brittle stuff, and the variety of scenarios is
>> high enough.
> 
> I agree.  The default completion UI supports various interactions that
> can break in subtle ways, and that's without even configuring stuff.
> I'll try to code a couple of test cases when I have the time.

That would be very welcome, thanks.

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Eshel Yaron]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 18/05/2024 09:55, Eshel Yaron wrote:
>> Dmitry Gutov <dmitry@gutov.dev> writes:
>>
>>> Hi Eshel,
>>>
>>> On 17/05/2024 11:56, Eshel Yaron wrote:
>> [...]
>>>> I'm afraid there might be another regression, here's what I see:
>>>
>>> Thanks for keeping an eye out.
>>>
>>>> 1. emacs -Q
>>>> 2. C-x C-f
>>>> 3. C-a C-k to clear the minibuffer
>>>> 4. Insert the path an Emacs checkout, e.g. "~/checkouts/emacs/"
>>>> 5. Further insert "l/mi"
>>>> 6. Hit ? to pop the completions list, which includes
>>>>      "lisp/minibuffer.el" and "leim/MISC-DIC/", among others.
>>>
>>> FWIW, I don't see "leim/MISC-DIC" among completions, even if I first
>>> evaluate (setq completion-ignore-case t).
>> Weird, perhaps you have read-file-name-completion-ignore-case set to
>> nil
>> and that takes effect rather than plain completion-ignore-case?
>
> Indeed, it's nil. I started with 'emacs -Q' to avoid my settings'
> interference, and assumed your scenario used it as well. Anyway...

FWIW I started with emacs -Q as well, but little did I know, the default
value of read-file-name-completion-ignore-case depends on system-type :(

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Dmitry Gutov]

On 18/05/2024 16:51, Eshel Yaron wrote:
>>>>> 1. emacs -Q
>>>>> 2. C-x C-f
>>>>> 3. C-a C-k to clear the minibuffer
>>>>> 4. Insert the path an Emacs checkout, e.g. "~/checkouts/emacs/"
>>>>> 5. Further insert "l/mi"
>>>>> 6. Hit ? to pop the completions list, which includes
>>>>>       "lisp/minibuffer.el" and "leim/MISC-DIC/", among others.
>>>> FWIW, I don't see "leim/MISC-DIC" among completions, even if I first
>>>> evaluate (setq completion-ignore-case t).
>>> Weird, perhaps you have read-file-name-completion-ignore-case set to
>>> nil
>>> and that takes effect rather than plain completion-ignore-case?
>> Indeed, it's nil. I started with 'emacs -Q' to avoid my settings'
>> interference, and assumed your scenario used it as well. Anyway...
> FWIW I started with emacs -Q as well, but little did I know, the default
> value of read-file-name-completion-ignore-case depends on system-type 🙁

Ouch. Right. Even 'darwin' is in that list.

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Eshel Yaron]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 18/05/2024 09:55, Eshel Yaron wrote:
>
>> Dmitry Gutov <dmitry@gutov.dev> writes:
>>>
>>> It would be great if someone looked into writing integration tests for
>>> this somehow. This is brittle stuff, and the variety of scenarios is
>>> high enough.
>>
>> I agree.  The default completion UI supports various interactions
>> that
>> can break in subtle ways, and that's without even configuring stuff.
>> I'll try to code a couple of test cases when I have the time.
>
> That would be very welcome, thanks.

I've now added basic regression tests for the issues we discussed here
to minibuffer-tests.el (commit a9af70849d2).


Best,

Eshel

Re: master ff3f17ca3cd: choose-completion: Retain the suffix after completion boundary

[Dmitry Gutov]

On 29/05/2024 13:05, Eshel Yaron wrote:
>>> I agree.  The default completion UI supports various interactions
>>> that
>>> can break in subtle ways, and that's without even configuring stuff.
>>> I'll try to code a couple of test cases when I have the time.
>> That would be very welcome, thanks.
> I've now added basic regression tests for the issues we discussed here
> to minibuffer-tests.el (commit a9af70849d2).

Nice! Thank you.