Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Eshel Yaron]

Hi Dmitry,

Dmitry Gutov <dgutov@yandex.ru> writes:

> branch: master
> commit 6b7ff60a5e71c161a064e27509fe4fb95cf74ddd
> Author: Dmitry Gutov <dmitry@gutov.dev>
> Commit: Dmitry Gutov <dmitry@gutov.dev>
>
>     Highlight the suffix in *Completions* buffer in 'basic' style too
>     
>     * lisp/minibuffer.el (completion-basic-all-completions):
>     Make sure to highlight the suffix as well (bug#71419).
> ---
>  lisp/minibuffer.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index f62cb2566b2..144cda8cfdc 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3816,7 +3816,7 @@ Return the new suffix."
>                              'point
>                              (substring afterpoint 0 (cdr bounds)))))
>           (all (completion-pcm--all-completions prefix pattern table pred)))
> -    (completion-hilit-commonality all point (car bounds))))
> +    (completion-pcm--hilit-commonality pattern all)))

Note that unlike completion-hilit-commonality,
completion-pcm--hilit-commonality does not add the base size to the last
cdr of the result.  So this breaks e.g. file name completion:

1. emacs -Q in emacs.git checkout root directory
2. C-x C-f lisp/fo TAB
3. *Completions* pops up with some completion candidates
4. M-<down>

The minibuffer should now contain something like
~/checkouts/emacs/lisp/foldout.el but after this commit it instead
contains just foldout.el.

Something like the following should fix this issue:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 144cda8cfdc..0a0b17b3850 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3816,7 +3816,9 @@ completion-basic-all-completions
                             'point
                             (substring afterpoint 0 (cdr bounds)))))
          (all (completion-pcm--all-completions prefix pattern table pred)))
-    (completion-pcm--hilit-commonality pattern all)))
+    (when all
+      (nconc (completion-pcm--hilit-commonality pattern all)
+             (car bounds)))))
 
 ;;; Partial-completion-mode style completion.
 

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Dmitry Gutov]

Hi!

On 13/06/2024 11:20, Eshel Yaron wrote:
> Note that unlike completion-hilit-commonality,
> completion-pcm--hilit-commonality does not add the base size to the last
> cdr of the result.  So this breaks e.g. file name completion:
> 
> 1. emacs -Q in emacs.git checkout root directory
> 2. C-x C-f lisp/fo TAB
> 3.*Completions*  pops up with some completion candidates
> 4. M-<down>
> 
> The minibuffer should now contain something like
> ~/checkouts/emacs/lisp/foldout.el but after this commit it instead
> contains just foldout.el.

Thanks for your vigilance. ;)

> Something like the following should fix this issue:
> 
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index 144cda8cfdc..0a0b17b3850 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3816,7 +3816,9 @@ completion-basic-all-completions
>                               'point
>                               (substring afterpoint 0 (cdr bounds)))))
>            (all (completion-pcm--all-completions prefix pattern table pred)))
> -    (completion-pcm--hilit-commonality pattern all)))
> +    (when all
> +      (nconc (completion-pcm--hilit-commonality pattern all)
> +             (car bounds)))))
>   
>   ;;; Partial-completion-mode style completion.

Please install, if you have access.

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Eshel Yaron]

Dmitry Gutov <dmitry@gutov.dev> writes:

> Hi!
>
> On 13/06/2024 11:20, Eshel Yaron wrote:
>> Note that unlike completion-hilit-commonality,
>> completion-pcm--hilit-commonality does not add the base size to the last
>> cdr of the result.  So this breaks e.g. file name completion:
>> 1. emacs -Q in emacs.git checkout root directory
>> 2. C-x C-f lisp/fo TAB
>> 3.*Completions*  pops up with some completion candidates
>> 4. M-<down>
>> The minibuffer should now contain something like
>> ~/checkouts/emacs/lisp/foldout.el but after this commit it instead
>> contains just foldout.el.
>
> Thanks for your vigilance. ;)

No problem :)

>> Something like the following should fix this issue:
>> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
>> index 144cda8cfdc..0a0b17b3850 100644
>> --- a/lisp/minibuffer.el
>> +++ b/lisp/minibuffer.el
>> @@ -3816,7 +3816,9 @@ completion-basic-all-completions
>>                               'point
>>                               (substring afterpoint 0 (cdr bounds)))))
>>            (all (completion-pcm--all-completions prefix pattern table pred)))
>> -    (completion-pcm--hilit-commonality pattern all)))
>> +    (when all
>> +      (nconc (completion-pcm--hilit-commonality pattern all)
>> +             (car bounds)))))
>>     ;;; Partial-completion-mode style completion.
>
> Please install, if you have access.

Done in commit 56537ab0228.


Eshel

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Juri Linkov]

> Something like the following should fix this issue:
> [...]
> @@ -3816,7 +3816,9 @@ completion-basic-all-completions
>                              'point
>                              (substring afterpoint 0 (cdr bounds)))))
>           (all (completion-pcm--all-completions prefix pattern table pred)))
> -    (completion-pcm--hilit-commonality pattern all)))
> +    (when all
> +      (nconc (completion-pcm--hilit-commonality pattern all)
> +             (car bounds)))))

This fixes file name completion, but at the cost of breaking icomplete-mode:

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
  >(19 nil)
  (if (> (length string) pos) (add-face-text-property pos (1+ pos) 'completions-first-difference nil string))
  (let* ((md (and regexp (string-match regexp string) (cdr (cdr (match-data t))))) ...)
  completion--hilit-from-re("it-window-to-buffer" "\\`f\\(.*?\\)" 1)
  completion-lazy-hilit("it-window-to-buffer")
  icomplete-completions("f" ...)
  icomplete-exhibit()
  completing-read-default("M-x " ...)
  read-extended-command-1("M-x " nil)
  read-extended-command()
  command-execute(execute-extended-command)

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Eshel Yaron]

Hi Juri,

Juri Linkov <juri@linkov.net> writes:

>> Something like the following should fix this issue:
>> [...]
>> @@ -3816,7 +3816,9 @@ completion-basic-all-completions
>>                              'point
>>                              (substring afterpoint 0 (cdr bounds)))))
>>           (all (completion-pcm--all-completions prefix pattern table pred)))
>> -    (completion-pcm--hilit-commonality pattern all)))
>> +    (when all
>> +      (nconc (completion-pcm--hilit-commonality pattern all)
>> +             (car bounds)))))
>
> This fixes file name completion, but at the cost of breaking icomplete-mode:
>
> Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
>   >(19 nil)
>   (if (> (length string) pos) (add-face-text-property pos (1+ pos) 'completions-first-difference nil string))
>   (let* ((md (and regexp (string-match regexp string) (cdr (cdr (match-data t))))) ...)
>   completion--hilit-from-re("it-window-to-buffer" "\\`f\\(.*?\\)" 1)
>   completion-lazy-hilit("it-window-to-buffer")
>   icomplete-completions("f" ...)
>   icomplete-exhibit()
>   completing-read-default("M-x " ...)
>   read-extended-command-1("M-x " nil)
>   read-extended-command()
>   command-execute(execute-extended-command)

Hmm that's quite curious...  I can reproduce it also without my change
in 56537ab0228, FWIW.  AFAICT the issue is that icomplete trims the
prefix from completion candidates before passing them to
completion-lazy-hilit, while completion--hilit-from-re expects the full
candidate (prefix included).  I'm not deeply familiar with icomplete,
but maybe calling completion-lazy-hilit before trimming the candidates
as in the diff below should do the trick?

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 44bbe9772e4..d03e315fdc8 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -1043,7 +1043,8 @@ icomplete-completions
                   (setq determ (concat open-bracket "" close-bracket)))
                 (while (and comps (not limit))
                   (setq comp
-                        (if prefix-len (substring (car comps) prefix-len) (car comps))
+                        (let ((cur (completion-lazy-hilit (car comps))))
+                          (if prefix-len (substring cur prefix-len) cur))
                         comps (cdr comps))
                   (setq prospects-len
                         (+ (string-width comp)
@@ -1052,8 +1053,7 @@ icomplete-completions
                   (if (< prospects-len prospects-max)
                       (push comp prospects)
                     (setq limit t)))
-                (setq prospects
-                      (nreverse (mapcar #'completion-lazy-hilit prospects)))
+                (setq prospects (nreverse prospects))
                 ;; Decorate first of the prospects.
                 (when prospects
                   (let ((first (copy-sequence (pop prospects))))

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Juri Linkov]

>> Lisp error: (wrong-type-argument number-or-marker-p nil)
>
> Hmm that's quite curious...  I can reproduce it also without my change
> in 56537ab0228, FWIW.  AFAICT the issue is that icomplete trims the
> prefix from completion candidates before passing them to
> completion-lazy-hilit, while completion--hilit-from-re expects the full
> candidate (prefix included).  I'm not deeply familiar with icomplete,
> but maybe calling completion-lazy-hilit before trimming the candidates
> as in the diff below should do the trick?

Thanks, I confirm with this change icomplete doesn't fail anymore.

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Eshel Yaron]

Juri Linkov <juri@linkov.net> writes:

>>> Lisp error: (wrong-type-argument number-or-marker-p nil)
>>
>> Hmm that's quite curious...  I can reproduce it also without my change
>> in 56537ab0228, FWIW.  AFAICT the issue is that icomplete trims the
>> prefix from completion candidates before passing them to
>> completion-lazy-hilit, while completion--hilit-from-re expects the full
>> candidate (prefix included).  I'm not deeply familiar with icomplete,
>> but maybe calling completion-lazy-hilit before trimming the candidates
>> as in the diff below should do the trick?
>
> Thanks, I confirm with this change icomplete doesn't fail anymore.

Thanks for confirming, pushed to master as commit 3b07d330591.


Eshel

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Dmitry Gutov]

On 14/06/2024 21:04, Eshel Yaron wrote:
> Juri Linkov<juri@linkov.net>  writes:
> 
>>>> Lisp error: (wrong-type-argument number-or-marker-p nil)
>>> Hmm that's quite curious...  I can reproduce it also without my change
>>> in 56537ab0228, FWIW.  AFAICT the issue is that icomplete trims the
>>> prefix from completion candidates before passing them to
>>> completion-lazy-hilit, while completion--hilit-from-re expects the full
>>> candidate (prefix included).  I'm not deeply familiar with icomplete,
>>> but maybe calling completion-lazy-hilit before trimming the candidates
>>> as in the diff below should do the trick?
>> Thanks, I confirm with this change icomplete doesn't fail anymore.
> Thanks for confirming, pushed to master as commit 3b07d330591.

Looking good, thank you.

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Juri Linkov]

>>> Lisp error: (wrong-type-argument number-or-marker-p nil)
>>
>> Hmm that's quite curious...  I can reproduce it also without my change
>> in 56537ab0228, FWIW.  AFAICT the issue is that icomplete trims the
>> prefix from completion candidates before passing them to
>> completion-lazy-hilit, while completion--hilit-from-re expects the full
>> candidate (prefix included).  I'm not deeply familiar with icomplete,
>> but maybe calling completion-lazy-hilit before trimming the candidates
>> as in the diff below should do the trick?
>
> Thanks, I confirm with this change icomplete doesn't fail anymore.

I don't know what commits caused this, but there is still a regression
in another case:

0. emacs -Q
1. M-x icomplete-mode RET
2. M-x rgrep RET foo RET b

Error in post-command-hook (icomplete-post-command-hook):
(wrong-type-argument number-or-marker-p nil)

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Eshel Yaron]

Hi Juri,

Juri Linkov <juri@linkov.net> writes:

> I don't know what commits caused this, but there is still a regression
> in another case:
>
> 0. emacs -Q
> 1. M-x icomplete-mode RET
> 2. M-x rgrep RET foo RET b
>
> Error in post-command-hook (icomplete-post-command-hook):
> (wrong-type-argument number-or-marker-p nil)

Ouch.  AFAICT the problem is in grep.el, and it's not specific to
icomplete-mode, e.g. I can reproduce it with:

  M-: (grep-read-files "foo") RET b ?

grep-read-files calls completing-read with a misbehaving completion
table, that returns some completion candidates unconditionally.  This
breaks the assumption of completion-pcm--all-completions that the
candidates it gets actually match the given pattern.

Does the diff below help?

(Note that this change has the downside that file name quoting/unquoting
is no longer applied.  Handling that with completion-table-merge might
be a bit more tricky.)

diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 459f00e6805..e8d1e692d0f 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -1181,9 +1181,7 @@ grep-read-files
          (files (completing-read
                  (format-prompt "Search for \"%s\" in files matching wildcard"
                                 default regexp)
-                 (completion-table-merge
-                  (lambda (_string _pred _action) defaults)
-                  #'read-file-name-internal)
+                 (completion-table-merge defaults #'completion-file-name-table)
 		 nil nil nil 'grep-files-history defaults)))
     (and files
 	 (or (cdr (assoc files grep-files-aliases))

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Juri Linkov]

>> 1. M-x icomplete-mode RET
>> 2. M-x rgrep RET foo RET b
>>
>> Error in post-command-hook (icomplete-post-command-hook):
>> (wrong-type-argument number-or-marker-p nil)
>
> Ouch.  AFAICT the problem is in grep.el, and it's not specific to
> icomplete-mode, e.g. I can reproduce it with:
>
>   M-: (grep-read-files "foo") RET b ?

Also typing 'el TAB' after RET gives:

Debugger entered--Lisp error: (error "Internal error: all doesn’t match \\`el")
  signal(error ("Internal error: all doesn’t match \\`el"))
  error("Internal error: %s doesn't match %s" "all" "\\`el")
  completion-pcm--merge-completions(("all" "el" "ch" "c" "cc" "cchh" "hh" "h" "l" "am" "m" "tex" "texi" "asm") ("el"))
  completion-pcm--merge-try(("el") ("all" "el" "ch" "c" "cc" "cchh" "hh" "h" "l" "am" "m" "tex" "texi" "asm") "" "")

> grep-read-files calls completing-read with a misbehaving completion
> table, that returns some completion candidates unconditionally.  This
> breaks the assumption of completion-pcm--all-completions that the
> candidates it gets actually match the given pattern.
>
> Does the diff below help?

Thanks, after short testing I see it doesn't lose the ability to complete
in both cases: a list of abbreviations and file names, while fixing
the problem above, so it looks like the right thing to do.

> (Note that this change has the downside that file name quoting/unquoting
> is no longer applied.  Handling that with completion-table-merge might
> be a bit more tricky.)

Not sure how serious this limitation is.

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Eshel Yaron]

Juri Linkov <juri@linkov.net> writes:

>>> 1. M-x icomplete-mode RET
>>> 2. M-x rgrep RET foo RET b
>>>
>>> Error in post-command-hook (icomplete-post-command-hook):
>>> (wrong-type-argument number-or-marker-p nil)
>>
>> Ouch.  AFAICT the problem is in grep.el, and it's not specific to
>> icomplete-mode, e.g. I can reproduce it with:
>>
>>   M-: (grep-read-files "foo") RET b ?
>
> Also typing 'el TAB' after RET gives:
>
> Debugger entered--Lisp error: (error "Internal error: all doesn’t match \\`el")
>   signal(error ("Internal error: all doesn’t match \\`el"))
>   error("Internal error: %s doesn't match %s" "all" "\\`el")
>   completion-pcm--merge-completions(("all" "el" "ch" "c" "cc" "cchh" "hh" "h" "l" "am" "m" "tex" "texi" "asm") ("el"))
>   completion-pcm--merge-try(("el") ("all" "el" "ch" "c" "cc" "cchh" "hh" "h" "l" "am" "m" "tex" "texi" "asm") "" "")
>
>> Does the diff below help?
>
> Thanks, after short testing I see it doesn't lose the ability to complete
> in both cases: a list of abbreviations and file names, while fixing
> the problem above, so it looks like the right thing to do.

Great.

>> (Note that this change has the downside that file name quoting/unquoting
>> is no longer applied.  Handling that with completion-table-merge might
>> be a bit more tricky.)
>
> Not sure how serious this limitation is.

Yes, it's very minor, I think.

This issue is present already in Emacs 29 with partial-completion at the
front of completion-styles, but in Emacs 30 it affects the default setup.
Should we fix this on master or on the release branch?

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Juri Linkov]

>>>> 1. M-x icomplete-mode RET
>>>> 2. M-x rgrep RET foo RET b
>>>>
>>>> Error in post-command-hook (icomplete-post-command-hook):
>>>> (wrong-type-argument number-or-marker-p nil)
>>>
>>> Ouch.  AFAICT the problem is in grep.el, and it's not specific to
>>> icomplete-mode, e.g. I can reproduce it with:
>>>
>>>   M-: (grep-read-files "foo") RET b ?
>>
>> Also typing 'el TAB' after RET gives:
>>
>> Debugger entered--Lisp error: (error "Internal error: all doesn’t match \\`el")
>>   signal(error ("Internal error: all doesn’t match \\`el"))
>>   error("Internal error: %s doesn't match %s" "all" "\\`el")
>>   completion-pcm--merge-completions(("all" "el" "ch" "c" "cc" "cchh" "hh" "h" "l" "am" "m" "tex" "texi" "asm") ("el"))
>>   completion-pcm--merge-try(("el") ("all" "el" "ch" "c" "cc" "cchh" "hh" "h" "l" "am" "m" "tex" "texi" "asm") "" "")
>>
>>> Does the diff below help?
>>
>> Thanks, after short testing I see it doesn't lose the ability to complete
>> in both cases: a list of abbreviations and file names, while fixing
>> the problem above, so it looks like the right thing to do.
>
> Great.
>
>>> (Note that this change has the downside that file name quoting/unquoting
>>> is no longer applied.  Handling that with completion-table-merge might
>>> be a bit more tricky.)
>>
>> Not sure how serious this limitation is.
>
> Yes, it's very minor, I think.
>
> This issue is present already in Emacs 29 with partial-completion at the
> front of completion-styles, but in Emacs 30 it affects the default setup.
> Should we fix this on master or on the release branch?

I don't know what Eli will decide, but this bug exists in the release branch.

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: emacs-devel@gnu.org,  Dmitry Gutov <dmitry@gutov.dev>
> Date: Mon, 01 Jul 2024 19:29:37 +0300
> 
> >>>> 1. M-x icomplete-mode RET
> >>>> 2. M-x rgrep RET foo RET b
> >>>>
> >>>> Error in post-command-hook (icomplete-post-command-hook):
> >>>> (wrong-type-argument number-or-marker-p nil)
> >>>
> >>> Ouch.  AFAICT the problem is in grep.el, and it's not specific to
> >>> icomplete-mode, e.g. I can reproduce it with:
> >>>
> >>>   M-: (grep-read-files "foo") RET b ?
> >>
> >> Also typing 'el TAB' after RET gives:
> >>
> >> Debugger entered--Lisp error: (error "Internal error: all doesn’t match \\`el")
> >>   signal(error ("Internal error: all doesn’t match \\`el"))
> >>   error("Internal error: %s doesn't match %s" "all" "\\`el")
> >>   completion-pcm--merge-completions(("all" "el" "ch" "c" "cc" "cchh" "hh" "h" "l" "am" "m" "tex" "texi" "asm") ("el"))
> >>   completion-pcm--merge-try(("el") ("all" "el" "ch" "c" "cc" "cchh" "hh" "h" "l" "am" "m" "tex" "texi" "asm") "" "")
> >>
> >>> Does the diff below help?
> >>
> >> Thanks, after short testing I see it doesn't lose the ability to complete
> >> in both cases: a list of abbreviations and file names, while fixing
> >> the problem above, so it looks like the right thing to do.
> >
> > Great.
> >
> >>> (Note that this change has the downside that file name quoting/unquoting
> >>> is no longer applied.  Handling that with completion-table-merge might
> >>> be a bit more tricky.)
> >>
> >> Not sure how serious this limitation is.
> >
> > Yes, it's very minor, I think.
> >
> > This issue is present already in Emacs 29 with partial-completion at the
> > front of completion-styles, but in Emacs 30 it affects the default setup.
> > Should we fix this on master or on the release branch?
> 
> I don't know what Eli will decide, but this bug exists in the release branch.

Please fix on the emacs-30 release branch, and thanks.

Re: master 6b7ff60a5e7: Highlight the suffix in *Completions* buffer in 'basic' style too

[Eshel Yaron]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Juri Linkov <juri@linkov.net>
[...]
>> > This issue is present already in Emacs 29 with partial-completion at the
>> > front of completion-styles, but in Emacs 30 it affects the default setup.
>> > Should we fix this on master or on the release branch?
>> 
>> I don't know what Eli will decide, but this bug exists in the release branch.
>
> Please fix on the emacs-30 release branch, and thanks.

Done in commit 850fc68481a.


Best,

Eshel