Re: BIKESHED: completion faces

[Dmitry Gutov <dgutov@yandex.ru>]

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

On 05.11.2019 13:10, João Távora wrote:

>> We agree, yes. Although I'm more in favor of changing the defaults,
> 
> So are we all, but that's a non-starter.  I'm sure everyone here thinks
> "their" settings should be the defaults.

The implication would be that trying to improve the defaults is useless 
or hopeless. I disagree.

> So what you are proposing with the "do-nothing" approach amounts to a
> lose-lose.

Have you read my other idea, then?

>> as soon as the kinks are worked out (e.g. flex is a bit slow when
>> prefix is 1-2 chars).
> 
> You have to think about this "slowness".  A part of it might be
> discovering if the 1-2 chars are in the potential match.  Another
> different part of it might be due to the fact that the set of matches is
> much greater when the pattern in short.
> 
> The former part can be improved in flex, the latter can't: it's
> intrinsic to the technique.

All can be improved, just with varying degrees of difficulty. But there 
are other techniques, like limiting the number of matches shown at a 
time. One just has to make sure not to cache the result improperly.

> But in matching systems like icomplete-mode
> it isn't a problem (in terms of responsiveness) because icomplete-mode
> has a while-no-input trick in it.  Perhaps so should company (presuming
> that's what you are using).

I'm trying it out with 'M-x completion-at-point'.

>> I disagree that it's a significant problem, though. Enabling 'flex' is
>> one line. Customizing the face is just another line.
> 
> Isn't true for custom.el users.  And it just doesn't make sense that to
> enable "good" flex matching you have to go touch two places.  We're
> discussing usability, after all.

As you can imagine, IMHO this part "making sense" is less important than 
the consistency in highlighting.

>>> This is better: I think this would lead you to my idea of a new
>>> `completion-relevant` face, which we would set to bold or something very
>>> prominent.  For the 'prefix' the relevant part is the "next character"
>>> and for the 'flex' style is whatever has matched.
>>> completions-first-difference could then be made an alias of that new
>>> face.
>>
>> Hmm, I don't like this changing of terms, sorry. This way, the same
>> highlighting would be applied to two fairly different things.
> 
> So is 'shadow' and 'bold' and many more.  It all has to do with how you
> design the semantics, something that is our prerrogative.

I wouldn't use 'shadow' or 'bold' for the new face either. Nothing that 
matches completions-common-part of completions-first-difference exactly.

> The current
> face semantics were designed for 'prefix', they just don't scale well
> for other pattern-matching strategies.
> 
> What I'm proposing is no different from say, mode-line-emphasis, which
> lisp/man.el and lisp/vc/vc-dispatcher.el use in "two wildly different
> things".

Here's an example. When the input is one char, how will you figure out 
what the highlighting in the *Completions* buffer means? Just by whether 
the character under the face is the same one? It's not reliable.

> Of course, another way out, at the expense of yet another face, is to
> add completions-flex-emphasis or something like that.

I've tried to implement my idea, but somehow the added highlighting gets 
eaten before the buffer is displayed. But the attached patch should 
illustrate it anyway.

[-- Attachment #2: completions-nontrivial-common-part.diff --]
[-- Type: text/x-patch, Size: 4034 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 43dd277a2e..e272364b49 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1649,10 +1649,8 @@ completion--insert-strings
 		  nil))))
             (setq first nil)
             (if (not (consp str))
-                (put-text-property (point) (progn (insert str) (point))
-                                   'mouse-face 'highlight)
-              (put-text-property (point) (progn (insert (car str)) (point))
-                                 'mouse-face 'highlight)
+                (completion--insert-string str)
+              (completion--insert-string (car str))
               (let ((beg (point))
                     (end (progn (insert (cadr str)) (point))))
                 (put-text-property beg end 'mouse-face nil)
@@ -1672,6 +1670,53 @@ completion--insert-strings
 			      ;; Round up to a whole number of columns.
 			      (* colwidth (ceiling length colwidth))))))))))))
 
+(defun completion--insert-string (s)
+  (require 'text-property-search)
+  (let* ((beg (point))
+         (end (progn (insert s)
+                     (point)))
+         (search-pred (lambda (expected value)
+                        (or (eq expected value)
+                            (and (listp value)
+                                 (memq expected value)))))
+         fdiff-match)
+    (put-text-property beg end 'mouse-face 'highlight)
+    (save-excursion
+      (save-restriction
+        (narrow-to-region beg end)
+        (goto-char beg)
+        (setq fdiff-match
+              (text-property-search-forward 'face 'completions-first-difference
+                                            search-pred))
+        (when fdiff-match
+          (let ((fdiff-pos (prop-match-beginning fdiff-match))
+                (prop-pos (point)))
+            (goto-char fdiff-pos)
+            (when (or
+                   ;; There is a place before fdiff-pos without common part.
+                   (catch 'found
+                     (while (setq prop-pos
+                                  (previous-single-property-change prop-pos 'face
+                                                                   nil beg))
+                       (when (= prop-pos beg)
+                         (throw 'found nil))
+                       (let ((value (get-text-property (1- prop-pos) 'face)))
+                         (if (or (eq value 'completions-common-part)
+                                 (and (listp value)
+                                      (memq 'completions-common-part value)))
+                             (setq prop-pos (1- prop-pos))
+                           (throw 'found t)))))
+                   ;; There is a place after fdiff-pos with common part.
+                   (text-property-search-forward 'face 'completions-common-part
+                                                 search-pred))
+              ;; FIXME: For some reason, this highlighting gets eaten
+              ;; before the buffer is displayed, somewhere.
+              ;; TODO: Highlight only the parts with -common-part.
+              (font-lock-prepend-text-property beg fdiff-pos
+                                               'font-lock-face
+                                               'completions-nontrivial-common-part)
+              )))))))
+
 (defvar completion-common-substring nil)
 (make-obsolete-variable 'completion-common-substring nil "23.1")
 
@@ -1691,6 +1736,10 @@ completions-common-part
   "Face for the parts of completions which matched the pattern.
 See also the face `completions-first-difference'.")
 
+(defface completions-nontrivial-common-part '((t (:background "white smoke")))
+  "Face for the parts of completions which matches the pattern nontrivially.
+Meaning that the match is a non-prefix one.")
+
 (defun completion-hilit-commonality (completions prefix-len &optional base-size)
   "Apply font-lock highlighting to a list of completions, COMPLETIONS.
 PREFIX-LEN is an integer.  BASE-SIZE is an integer or nil (meaning zero).