bug#10062: 24.0.91; completions-first-difference

bug#10062: 24.0.91; completions-first-difference

[Leo]

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

1. Emacs -q
2. (setq completion-cycle-threshold 4)
3. In the *scratch* buffer type "(push" without the double quotes
4. Type M-Tab a few times

You should see something like in the screenshot:

[-- Attachment #2: emacs-completion-face.png --]
[-- Type: image/png, Size: 4639 bytes --]

[-- Attachment #3: Type: text/plain, Size: 195 bytes --]


BTW, I think the face property completions-first-difference should not
be rear-sticky. When that char is the last in the completion anything I
type after inherits it as seen in this screenshot:

[-- Attachment #4: emacs-slime-repl.png --]
[-- Type: image/png, Size: 5470 bytes --]

[-- Attachment #5: Type: text/plain, Size: 311 bytes --]


In GNU Emacs 24.0.91.1 (x86_64-unknown-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2011-11-09
Windowing system distributor `The X.Org Foundation', version 11.0.11004000
configured using `configure  'CFLAGS=-g' '--prefix=/usr/local/unix/emacs' '--with-wide-int' '--with-x-toolkit=lucid' '--without-gsettings''

bug#10062: 24.0.91; completions-first-difference

[Leo]

On 2011-11-16 17:45 +0800, Leo wrote:
> 1. Emacs -q
> 2. (setq completion-cycle-threshold 4)
> 3. In the *scratch* buffer type "(push" without the double quotes
> 4. Type M-Tab a few times

Any objection to the following patch:

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2011-10-17 16:30:02 +0000
+++ lisp/minibuffer.el	2011-11-17 16:22:45 +0000
@@ -1203,9 +1203,10 @@
                                'font-lock-face 'completions-common-part
                                str)
             (if (> (length str) com-str-len)
-                (put-text-property com-str-len (1+ com-str-len)
-                                   'font-lock-face 'completions-first-difference
-                                   str)))
+                (add-text-properties com-str-len (1+ com-str-len)
+				     '(font-lock-face completions-first-difference
+						      rear-nonsticky t)
+				     str)))
           elem)
         completions)
        base-size))))


Leo

bug#10062: 24.0.91; completions-first-difference

[Stefan Monnier]

>> 1. Emacs -q
>> 2. (setq completion-cycle-threshold 4)
>> 3. In the *scratch* buffer type "(push" without the double quotes
>> 4. Type M-Tab a few times

> Any objection to the following patch:

Yup: the bug is not in the rear-stickiness but in the mere presence of
this face.  I.e. the fix is to strip these faces when used for
completion (but of course keep them when used for *Completions* display).


        Stefan

bug#10062: 24.0.91; completions-first-difference

[Leo]

> Yup: the bug is not in the rear-stickiness but in the mere presence of
> this face.  I.e. the fix is to strip these faces when used for
> completion (but of course keep them when used for *Completions*
> display).

Sorry for the delay.

It is kind of nice to indicate the first different char when used with
cycle completion. WDYT?

Leo

bug#10062: 24.0.91; completions-first-difference

[Stefan Monnier]

>> Yup: the bug is not in the rear-stickiness but in the mere presence of
>> this face.  I.e. the fix is to strip these faces when used for
>> completion (but of course keep them when used for *Completions*
>> display).
> Sorry for the delay.
> It is kind of nice to indicate the first different char when used with
> cycle completion. WDYT?

I guess it could make sense to keep the face on the inserted text while
you're still cycling, but it should be removed afterwards.  And since
the transition between "cycling" and "not cycling" is not explicit,
you'd then need/want to remove the face from something like
a pre-command-hook.


        Stefan

bug#10062: 24.0.91; completions-first-difference

[Leo]

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

On 2011-11-18 09:47 +0800, Stefan Monnier wrote:
[snipped 7 lines]
> Yup: the bug is not in the rear-stickiness but in the mere presence of
> this face.  I.e. the fix is to strip these faces when used for
> completion (but of course keep them when used for *Completions* display).
>
>
>         Stefan

On 2011-12-01 23:57 +0800, Stefan Monnier wrote:
[snipped 8 lines]
> I guess it could make sense to keep the face on the inserted text while
> you're still cycling, but it should be removed afterwards.  And since
> the transition between "cycling" and "not cycling" is not explicit,
> you'd then need/want to remove the face from something like
> a pre-command-hook.
>
>
>         Stefan

Hello Stefan,

I have been using completion with completion-cycle-threshold set to 4
and I have been annoyed often enough that I think it is worthwhile to
fix this bug because it often leaves my buffer weirdly fontified. For
example in elisp, `defma' can complete to `defmacro' unfontified.


[-- Attachment #2: emacs-comp-bug.png --]
[-- Type: image/png, Size: 7596 bytes --]

[-- Attachment #3: emacs-comp-correct.png --]
[-- Type: image/png, Size: 7782 bytes --]

[-- Attachment #4: Type: text/plain, Size: 133 bytes --]


I propose the attached patch following your advice i.e. strip faces when
used for completion. Could you take a look at it? Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: mb.diff --]
[-- Type: text/x-diff, Size: 4011 bytes --]

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2012-01-05 09:46:05 +0000
+++ lisp/minibuffer.el	2012-01-16 09:05:45 +0000
@@ -1180,6 +1180,9 @@
 of the differing parts is, by contrast, slightly highlighted."
   :group 'completion)
 
+(defvar completion-hilit-commonality-p nil
+  "Internal variable. Bound to t in `minibuffer-completion-help'.")
+
 (defun completion-hilit-commonality (completions prefix-len base-size)
   (when completions
     (let ((com-str-len (- prefix-len (or base-size 0))))
@@ -1195,17 +1198,18 @@
                      (car (setq elem (cons (copy-sequence (car elem))
                                            (cdr elem))))
                    (setq elem (copy-sequence elem)))))
-            (put-text-property 0
-			       ;; If completion-boundaries returns incorrect
-			       ;; values, all-completions may return strings
-			       ;; that don't contain the prefix.
-			       (min com-str-len (length str))
-                               'font-lock-face 'completions-common-part
-                               str)
-            (if (> (length str) com-str-len)
-                (put-text-property com-str-len (1+ com-str-len)
-                                   'font-lock-face 'completions-first-difference
-                                   str)))
+            (when completion-hilit-commonality-p
+              (put-text-property 0
+                                 ;; If completion-boundaries returns incorrect
+                                 ;; values, all-completions may return strings
+                                 ;; that don't contain the prefix.
+                                 (min com-str-len (length str))
+                                 'font-lock-face 'completions-common-part
+                                 str)
+              (if (> (length str) com-str-len)
+                  (put-text-property com-str-len (1+ com-str-len)
+                                     'font-lock-face 'completions-first-difference
+                                     str))))
           elem)
         completions)
        base-size))))
@@ -1314,12 +1318,13 @@
          (end (field-end))
          (string (field-string))
          (md (completion--field-metadata start))
-         (completions (completion-all-completions
-                       string
-                       minibuffer-completion-table
-                       minibuffer-completion-predicate
-                       (- (point) (field-beginning))
-                       md)))
+         (completions (let ((completion-hilit-commonality-p t))
+                        (completion-all-completions
+                         string
+                         minibuffer-completion-table
+                         minibuffer-completion-predicate
+                         (- (point) (field-beginning))
+                         md))))
     (message nil)
     (if (or (null completions)
             (and (not (consp (cdr completions)))
@@ -2411,14 +2416,15 @@
          (setq str (copy-sequence str))
          (unless (string-match re str)
            (error "Internal error: %s does not match %s" re str))
-         (let ((pos (or (match-beginning 1) (match-end 0))))
-           (put-text-property 0 pos
-                              'font-lock-face 'completions-common-part
-                              str)
-           (if (> (length str) pos)
-               (put-text-property pos (1+ pos)
-				  'font-lock-face 'completions-first-difference
-				  str)))
+         (when completion-hilit-commonality-p
+           (let ((pos (or (match-beginning 1) (match-end 0))))
+             (put-text-property 0 pos
+                                'font-lock-face 'completions-common-part
+                                str)
+             (if (> (length str) pos)
+                 (put-text-property pos (1+ pos)
+                                    'font-lock-face 'completions-first-difference
+                                    str))))
 	 str)
        completions))))
 


[-- Attachment #6: Type: text/plain, Size: 148 bytes --]


Another simpler fix is to replace 'font-lock-face with 'face which would
then allow the major-mode to override the faces added by completion.

Leo

bug#10062: 24.0.91; completions-first-difference

[Stefan Monnier]

> I propose the attached patch following your advice i.e. strip faces when
> used for completion. Could you take a look at it? Thanks.

Rather than prevent faces from being added, I installed a patch which
strips them before insertion.


        Stefan


--- lisp/minibuffer.el	2012-01-05 09:46:05 +0000
+++ lisp/minibuffer.el	2012-01-16 15:41:07 +0000
@@ -571,6 +571,10 @@
 (defun completion--replace (beg end newtext)
   "Replace the buffer text between BEG and END with NEWTEXT.
 Moves point to the end of the new text."
+  ;; The properties on `newtext' include things like
+  ;; completions-first-difference, which we don't want to include
+  ;; upon insertion.
+  (set-text-properties 0 (length newtext) nil newtext)
   ;; Maybe this should be in subr.el.
   ;; You'd think this is trivial to do, but details matter if you want
   ;; to keep markers "at the right place" and be robust in the face of

bug#10062: 24.0.91; completions-first-difference

[Leo]

On 2012-01-16 23:52 +0800, Stefan Monnier wrote:
> Rather than prevent faces from being added, I installed a patch which
> strips them before insertion.
>
>
>         Stefan
>
>
> --- lisp/minibuffer.el	2012-01-05 09:46:05 +0000
> +++ lisp/minibuffer.el	2012-01-16 15:41:07 +0000
> @@ -571,6 +571,10 @@
>  (defun completion--replace (beg end newtext)
>    "Replace the buffer text between BEG and END with NEWTEXT.
>  Moves point to the end of the new text."
> +  ;; The properties on `newtext' include things like
> +  ;; completions-first-difference, which we don't want to include
> +  ;; upon insertion.
> +  (set-text-properties 0 (length newtext) nil newtext)
>    ;; Maybe this should be in subr.el.
>    ;; You'd think this is trivial to do, but details matter if you want
>    ;; to keep markers "at the right place" and be robust in the face of

Thank you. This is a better fix. I think this bug can be closed.

Leo

bug#10062: 24.0.91; completions-first-difference

[Stefan Monnier]

> Thank you. This is a better fix. I think this bug can be closed.

Thanks, done,


        Stefan "not convinced it's a better fix, but he prefers it anyway"