bug#8897: `completion--insert-strings' clobbers user-added text properties

bug#8897: `completion--insert-strings' clobbers user-added text properties

[Štěpán Němec]

Tags: patch

It is possible to bind `completion-annotate-function' to add custom
annotations, which is great. Unfortunately, the `face' and `mouse-face'
text properties added by such a function are then unconditionally
overwritten by `completion--insert-strings'.

In my particular case I define annotations as buttons (which display
even more detail about a completion value upon activation), so a visual
indication of clickability is very important for me.

I wonder if something like the patch below, which fixes the problem for
me, could be applied?

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 284cbdc..11534e6 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1003,9 +1003,12 @@ (defun completion--insert-strings (strings)
                                    'mouse-face 'highlight)
               (put-text-property (point) (progn (insert (car str)) (point))
                                  'mouse-face 'highlight)
-              (add-text-properties (point) (progn (insert (cadr str)) (point))
-                                   '(mouse-face nil
-						face completions-annotations)))
+              (let ((annotation (cadr str)))
+                (if (text-properties-at 1 annotation)
+                    (insert annotation)
+                  (add-text-properties (point) (progn (insert annotation) (point))
+                                       '(mouse-face nil
+                                                    face completions-annotations)))))
 	    (cond
 	     ((eq completions-format 'vertical)
 	      ;; Vertical format

bug#8897: `completion--insert-strings' clobbers user-added text properties

[Glenn Morris]

> I wonder if something like the patch below, which fixes the problem for
> me, could be applied?

We cannot apply any (more) patches from you because you don't want to
complete a copyright assignment. That's your prerogative of course, but
given that, it is best if you do not send patches when you report issues.

bug#8897: `completion--insert-strings' clobbers user-added text properties

[Stefan Monnier]

> In my particular case I define annotations as buttons (which display
> even more detail about a completion value upon activation), so a visual
> indication of clickability is very important for me.

Currently, the completion-list-mode uses the `mouse-face' text-property
to determine what is a completion item and what i something else (blank
space, annotation, you name it).  So as it stands, any mouse-face you'd
add to an annotation would confuse completion-list-mode into thinking it
is a completion item.

Maybe you could try to use the `category' text-property instead.


        Stefan

bug#8897: `completion--insert-strings' clobbers user-added text properties

[Štěpán Němec]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> In my particular case I define annotations as buttons (which display
>> even more detail about a completion value upon activation), so a visual
>> indication of clickability is very important for me.
>
> Currently, the completion-list-mode uses the `mouse-face' text-property
> to determine what is a completion item and what i something else (blank
> space, annotation, you name it).  So as it stands, any mouse-face you'd
> add to an annotation would confuse completion-list-mode into thinking it
> is a completion item.
>
> Maybe you could try to use the `category' text-property instead.

Well I do use the `category' property, but the button functions seem to
set up the button face automatically, which is then clobbered by
`completion--insert-strings' (which is the problem). Here's how I set up
an annotation, `n' being the number of a pull request as a string (i.e.,
the completion value itself):

#+begin_src elisp
(let ((req (assoc-default n minibuffer-completion-table)))
  (concat " "
          (propertize (plist-get req :title)
                      'fontified t
                      'button '(t)
                      'category 'default-button
                      'help-echo "RET or mouse-2 for details"
                      'pr-data req
                      'action (lambda (b) (magithub-pull-request-details
                                           (button-get b 'pr-data))))))
#+end_src

With my patch I haven't noticed any problems you mention -- clicking
or RET on the completion itself selects it, clicking or RET on the
annotation displays further details, the button face is preserved.

  Štěpán

bug#8897: `completion--insert-strings' clobbers user-added text properties

[Stefan Monnier]

>>> In my particular case I define annotations as buttons (which display
>>> even more detail about a completion value upon activation), so a visual
>>> indication of clickability is very important for me.
>> Currently, the completion-list-mode uses the `mouse-face' text-property
>> to determine what is a completion item and what i something else (blank
>> space, annotation, you name it).  So as it stands, any mouse-face you'd
>> add to an annotation would confuse completion-list-mode into thinking it
>> is a completion item.
>> Maybe you could try to use the `category' text-property instead.

> Well I do use the `category' property, but the button functions seem to
> set up the button face automatically, which is then clobbered by
> `completion--insert-strings' (which is the problem). Here's how I set up
> an annotation, `n' being the number of a pull request as a string (i.e.,
> the completion value itself):

> #+begin_src elisp
> (let ((req (assoc-default n minibuffer-completion-table)))
>   (concat " "
>           (propertize (plist-get req :title)
>                       'fontified t
>                       'button '(t)
>                       'category 'default-button
>                       'help-echo "RET or mouse-2 for details"
>                       'pr-data req
>                       'action (lambda (b) (magithub-pull-request-details
>                                            (button-get b 'pr-data))))))
> #+end_src

I must be missing something: where is a `face' or `mouse-face' property
added?  The above code should not be affected by your patch, AFAICT.
And I don't understand your comment about "but the button functions seem
to set up the button face automatically" since I don't see where you
call a button function before insertion.

> With my patch I haven't noticed any problems you mention -- clicking
> or RET on the completion itself selects it, clicking or RET on the
> annotation displays further details, the button face is preserved.

I guess click&RET work OK because they're locally overridden, but if you
run M-x choose-completion while on the button or if you hit `left' or
`right' to skip from one completion to the next, you might see some
problem (although not with the code quoted above which should not
suffer from any of the problems discussed in this thread, AFAICT).


        Stefan

bug#8897: `completion--insert-strings' clobbers user-added text properties

[Štěpán Němec]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> #+begin_src elisp
>> (let ((req (assoc-default n minibuffer-completion-table)))
>>   (concat " "
>>           (propertize (plist-get req :title)
>>                       'fontified t
>>                       'button '(t)
>>                       'category 'default-button
>>                       'help-echo "RET or mouse-2 for details"
>>                       'pr-data req
>>                       'action (lambda (b) (magithub-pull-request-details
>>                                            (button-get b 'pr-data))))))
>> #+end_src
>
> I must be missing something: where is a `face' or `mouse-face' property
> added?  The above code should not be affected by your patch, AFAICT.
> And I don't understand your comment about "but the button functions seem
> to set up the button face automatically" since I don't see where you
> call a button function before insertion.

Sorry for my confusing description. What I meant is that there is
apparently some magic involved that makes a string manufactured with a
`propertize' form like the one above appear as if it had the button face
(underlined), even though it's not explicitely among its text properties
(maybe the `category' does it? I don't know).

In any case, without my patch the face is overridden to become the usual
annotation italic, so it _does_ suffer from the problem I describe.

>> With my patch I haven't noticed any problems you mention -- clicking
>> or RET on the completion itself selects it, clicking or RET on the
>> annotation displays further details, the button face is preserved.
>
> I guess click&RET work OK because they're locally overridden, but if you
> run M-x choose-completion while on the button or if you hit `left' or
> `right' to skip from one completion to the next, you might see some
> problem (although not with the code quoted above which should not
> suffer from any of the problems discussed in this thread, AFAICT).

Indeed. I don't use the cursor keys, but you're right in that they seem
to include the button-like annotations among their jump targets; as for
`choose-completion', that seems to be bound to RET in the *Completions*
buffer, but as I said and in line with your guess, pressing RET on an
annotation actually invokes the button action as desired, so this
problem doesn't seem to occur.

  Štěpán

bug#8897: `completion--insert-strings' clobbers user-added text properties

[Stefan Monnier]

>>> #+begin_src elisp
>>> (let ((req (assoc-default n minibuffer-completion-table)))
>>> (concat " "
>>> (propertize (plist-get req :title)
>>> 'fontified t
>>> 'button '(t)
>>> 'category 'default-button
>>> 'help-echo "RET or mouse-2 for details"
>>> 'pr-data req
>>> 'action (lambda (b) (magithub-pull-request-details
>>> (button-get b 'pr-data))))))
>>> #+end_src
>> 
>> I must be missing something: where is a `face' or `mouse-face' property
>> added?  The above code should not be affected by your patch, AFAICT.
>> And I don't understand your comment about "but the button functions seem
>> to set up the button face automatically" since I don't see where you
>> call a button function before insertion.

> Sorry for my confusing description. What I meant is that there is
> apparently some magic involved that makes a string manufactured with a
> `propertize' form like the one above appear as if it had the button face
> (underlined), even though it's not explicitely among its text properties
> (maybe the `category' does it? I don't know).

Yes, the `category' property is special and is "equivalent" to adding
all the properties on the symbol used for the category (i.e. all the
properties on the `default-button' symbol in your above example), which
most likely includes `face' and/or `mouse-face' properties.

> In any case, without my patch the face is overridden to become the usual
> annotation italic, so it _does_ suffer from the problem I describe.

Right, the `face' text-property takes precedence over the `face'
property provided by the `category' text property.  Hmm...

> Indeed. I don't use the cursor keys, but you're right in that they seem
> to include the button-like annotations among their jump targets; as for
> `choose-completion', that seems to be bound to RET in the *Completions*
> buffer, but as I said and in line with your guess, pressing RET on an
> annotation actually invokes the button action as desired, so this
> problem doesn't seem to occur.

Yes, when point is on the button, RET (and mouse clicks) does not call
choose-completion but the button command instead.  But if you have bound
choose-completion to some other keys that are not overridden by the
button, you'll see the problem (or if you do M-x choose-completion).


        Stefan

bug#8897: `completion--insert-strings' clobbers user-added text properties

[Lars Ingebrigtsen]

Štěpán Němec <stepnem@gmail.com> writes:

> It is possible to bind `completion-annotate-function' to add custom
> annotations, which is great. Unfortunately, the `face' and `mouse-face'
> text properties added by such a function are then unconditionally
> overwritten by `completion--insert-strings'.
>
> In my particular case I define annotations as buttons (which display
> even more detail about a completion value upon activation), so a visual
> indication of clickability is very important for me.
>
> I wonder if something like the patch below, which fixes the problem for
> me, could be applied?

In conjunction with bug#43218, completion no longer clobbers all text
properties (but it does clobber the face text property).  So it still
requires some wrangling by the caller if the text property was the one
they wanted to have preserved, but the caller can stash that in a
different text property.

This requires that `minibuffer-allow-text-properties' is non-nil.

So I think that basically takes care of the problem described here, and
I'm closing this bug report.  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no