bug#71419: 30.0.50; Completion does not always highlight the "common part" corresponding to suffix

bug#71419: 30.0.50; Completion does not always highlight the "common part" corresponding to suffix

[Dmitry Gutov]

X-Debbugs-Cc: monnier@IRO.UMontreal.CA

For example:

1. Type

   (fo|-function

where | denotes the position of point.

2. C-M-i

You get 5 completions (the exact number is not important), where "fo" is
highlighted in all of them, but "function is not.

3. Move point to after "-" (meaning, just one character forward), press
C-M-i again (twice).

Completion expands the text to "(fo-|-function" (the addition might be
unimportant) and "function" is highlighted with the face
`completions-common-part' now, in all completions.

This seems inconsistent.

Also, highlighting is information. If it was more uniform, we could use
it to e.g. address the FIXME in completion-all-completions. Though
that's up for discussion, given that highlighting is inherently less
reliable than other methods we use. But it would be a non-breaking
change, OT2H.

WDYT?

In GNU Emacs 30.0.50 (build 28, x86_64-pc-linux-gnu, X toolkit, cairo
  version 1.18.0, Xaw scroll bars) of 2024-06-07 built on potemkin
Repository revision: b571c6571c8bc4c34569650104aee273c19cbfd4
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12302000
System Description: Ubuntu 23.10

bug#71419: 30.0.50; Completion does not always highlight the "common part" corresponding to suffix

[Dmitry Gutov]

On 08/06/2024 01:36, Dmitry Gutov wrote:
> If it was more uniform, we could use
> it to e.g. address the FIXME in completion-all-completions.

To address bug#70968, among other things.

bug#71419: 30.0.50; Completion does not always highlight the "common part" corresponding to suffix

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

> For example:
>
> 1. Type (fo|-function
> 2. C-M-i
>
> You get 5 completions (the exact number is not important), where "fo" is
> highlighted in all of them, but "function is not.

Looks like a bug in the `basic` completion style: it filters things that
match the glob pattern `fo*-function` yet it only highlights the prefix.

> Also, highlighting is information. If it was more uniform, we could use
> it to e.g. address the FIXME in completion-all-completions.

I don't think it could be trusted to always provide the needed data (and
even if it does, it would be sufficiently clunky to use that I'm not
sure we'd want to rely on it).


        Stefan

bug#71419: 30.0.50; Completion does not always highlight the "common part" corresponding to suffix

[Dmitry Gutov]

On 09/06/2024 16:52, Stefan Monnier wrote:
>> For example:
>>
>> 1. Type (fo|-function
>> 2. C-M-i
>>
>> You get 5 completions (the exact number is not important), where "fo" is
>> highlighted in all of them, but "function is not.
> 
> Looks like a bug in the `basic` completion style: it filters things that
> match the glob pattern `fo*-function` yet it only highlights the prefix.

Yes, thanks. Looks like this one-liner fixes it:

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 @@ completion-basic-all-completions
                              '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)))

  ;;; Partial-completion-mode style completion.



>> Also, highlighting is information. If it was more uniform, we could use
>> it to e.g. address the FIXME in completion-all-completions.
> 
> I don't think it could be trusted to always provide the needed data (and
> even if it does, it would be sufficiently clunky to use that I'm not
> sure we'd want to rely on it).

You're probably right.

But it would have been nice to be able to use it in the "progressive 
improvement" kind of fashion: when the suffix and the other parts are 
highlighted correctly, we do the right thing; if the style missed the 
suffix highlighting, we do the other thing - and put the responsibility 
on the third party.

bug#71419: 30.0.50; Completion does not always highlight the "common part" corresponding to suffix

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

> 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 @@ completion-basic-all-completions
>                              '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)))
>
>  ;;; Partial-completion-mode style completion.

Thanks for tracking it down.  It matches my expectation.

> But it would have been nice to be able to use it in the "progressive
>  improvement" kind of fashion: when the suffix and the other parts are
>  highlighted correctly, we do the right thing; if the style missed the
>  suffix highlighting, we do the other thing - and put the responsibility on
> the third party.

🙂


        Stefan

bug#71419: 30.0.50; Completion does not always highlight the "common part" corresponding to suffix

[Dmitry Gutov]

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

On 10/06/2024 00:06, Stefan Monnier wrote:
>> 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 @@ completion-basic-all-completions
>>                               '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)))
>>
>>   ;;; Partial-completion-mode style completion.
> 
> Thanks for tracking it down.  It matches my expectation.

Thanks, now fixed on master.

>> But it would have been nice to be able to use it in the "progressive
>>   improvement" kind of fashion: when the suffix and the other parts are
>>   highlighted correctly, we do the right thing; if the style missed the
>>   suffix highlighting, we do the other thing - and put the responsibility on
>> the third party.
> 
> 🙂

It could work like in the attached. I agree that it's somewhat brittle, 
though.

If you're not in favor of installing this patch, what would be your 
preferred strategy for fixing bug#70968? We could try resurrecting the 
relevant part of Daniel's patch for completion-all-completions, but it 
means a fair amount of breakage. Or another dynamic variable similar to 
completion-lazy-hilit-fn...

FWIW my interest here is how to better implement the same step in 
company-mode, but the default UI is a good common ground.

[-- Attachment #2: completions--count-common-spans.diff --]
[-- Type: text/x-patch, Size: 1861 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 144cda8cfdc..d8952094196 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2551,6 +2551,25 @@ completions--after-change
       (with-selected-window window
         (completions--deselect)))))
 
+(defun completions--count-common-spans (str)
+  (let ((pos 0)
+        prev-pos
+        value
+        (sum 0))
+    (while pos
+      (when prev-pos
+        (cl-incf sum (- pos prev-pos)))
+      (if (and (setq value (get-text-property pos 'face str))
+               (if (listp value)
+                   (memq 'completions-common-part value)
+                 (eq 'completions-common-part value)))
+          (setq prev-pos pos)
+        (setq prev-pos nil))
+      (setq pos (next-single-property-change pos 'face str))
+      (when (and (not pos) prev-pos)
+        (cl-incf sum (- (length str) prev-pos))))
+    sum))
+
 (defun minibuffer-completion-help (&optional start end)
   "Display a list of possible completions of the current minibuffer contents."
   (interactive)
@@ -2706,6 +2725,10 @@ minibuffer-completion-help
                                  ;; but that clashed with another existing marker.
                                  (cl-decf (nth 1 base-position)
                                           (- end start (length choice)))
+                                 ;; Completion style that doesn't match suffix.
+                                 (unless (> (completions--count-common-spans choice)
+                                            (- (point) start))
+                                   (setq end (point)))
                                  ;; FIXME: Use `md' to do quoting&terminator here.
                                  (completion--replace start (min end (point-max)) choice)
                                  (let* ((minibuffer-completion-table ctable)