Re: valid forms of face property: documentation bug, font-lock-{prepend, append}-text-property bug

Re: valid forms of face property: documentation bug, font-lock-{prepend, append}-text-property bug

[Joe Wells]

I hope it is okay that I am adding emacs-devel in the CC header.  I
have some questions in this e-mail that others might be able to
address.  (This e-mail conversation began with a bug report sent to
bug-gnu-emacs.)

Richard Stallman <rms@gnu.org> writes:

>     BUG #1:  The documentation in (info "(elisp)Special Properties") is
>     misleading about the legal values of the face property.
>
> I think I decided to deprecate the other kinds of values
> which do work.  I will change the manual to say so.

I think it is not the right solution to deprecate all of the shapes
that are not documented in elisp.info.

First, I think this face property shape should be documented (and not
deprecated):

  (KEYWORD VALUE ...)

It should be documented as an abbreviation of ((KEYWORD VALUE ...)).
This face property shape is widely used, and quite convenient.  Here
are some places code in Emacs uses this shape:

  * term.el uses face property values like this:

      (:foreground "red3" :background unspecified)

  * ses.el uses this face property value:

      (:box (:line-width 2 :style released-button))

  * gdb-ui.el uses this face property value:

      (:inverse-video t)

  * There may be other places in Emacs that I did not spot with a
    quick find-grep.

  * The code in wid-edit.el uses whatever values its clients provide.
    Random user 3rd party code might pass the above shapes.

  * I have not searched through 3rd-party code but I suspect lots of
    uses of this shape will be found.

Second, I think these two shapes should be documented *and*
deprecated:

  (foreground-color . COLOR-NAME)
  (background-color . COLOR-NAME)

There is still some code in Emacs that uses this old shape.  For
example, the code in facemenu.el makes face properties with values
like this:

  (background-color . "linen")

Third, I think this shape should not be documented (or should be
explicitly warned against):

  (PRIMITIVE-FACE-VALUE1 ... PRIMITIVE-FACE-VALUEn . PRIMITIVE-FACE-VALUE)
    where PRIMITIVE-FACE-VALUE is one of FACENAME
                                      or (KEYWORD VALUE ...)
                                      or (foreground-color . COLOR-NAME)
                                      or (background-color . COLOR-NAME)

I also think this shape should not be supported.  Except for the
special cases of face property lists already described above (a
keyword list or a cons beginning with foreground-color or
background-color), a face property list should be required to be a
proper list (with tail of nil) of primitive face values (as described
above).  If a face property list is not one of the allowed shapes,
then ideally the entire list should have no effect, but certainly at
least the non-nil tail should be ignored.

By the way, these shapes are in fact currently used in Emacs!  The
code in startup.el uses face property values like the following in the
splash screen:

  (variable-pitch :foreground "red")
  (variable-pitch :weight bold)
  (variable-pitch :slant oblique)
  (variable-pitch :foreground "darkblue")
  (variable-pitch :height 0.5)

NOTE: I'm briefly changing topic to something I noticed while
investigating this.  Supposedly, facep returns non-nil for internal
face objects, which are special vectors.  Do these internal face
objects work as face property values?  Also, it appears that (facep
'bold) in fact returns one of these internal face objects, but (facep
(facep 'bold)) returns nil, so facep seems to violate its own
documentation.  Is this what is happening?  Or is the vector returned
by (facep 'bold) not one of the internal face objects?  Can anyone
answer these questions?

(I noticed this because there is code in cus-edit.el that can set a
face property value that is anything that passes facep.)

>     BUG #2:  The font-lock-prepend-text-property and
>     font-lock-append-text-property seem to be designed specifically for
>     use with the face property (despite their documentation not mentioning
>     this aim).  They seem to have been written under the assumption that
>     the documentation in the manual for the face property was correct,
>
> They assume the value has one of the currently recommended forms.
>
> I wrote a patch to make those functions canonicalize deprecated forms
> of the face property.  Does it work?

Your changes look good.  They seem to address the concern I raised.  I
haven't tested them.  Thanks for making these changes!

By the way, there are two bad implementations of the same concept in
other places in Emacs.  They should both be replaced by calls to
font-lock-prepend-text-property or should be changed to use
font-lock-prepend-text-property.  First, rcirc-add-face function in
rcirc.el is a bad implementation of the idea of
font-lock-prepend-text-property which appears not to properly handle
the case where the value of the face property is a symbol.  Second,
erc-button-add-face in erc-button.el is yet another bad implementation
of the same idea that does not handle the case where the face property
is a list but is not a list of faces.

> However, I'd like to ask: where did you encounter those deprecated
> forms?  Perhaps we should fix those programs to generate recommended
> forms of the value.

See comments above.  Plus, there will be lots of 3rd-party code.

> *** font-lock.el	12 Aug 2007 13:52:30 -0400	1.317.2.3
> --- font-lock.el	28 Dec 2007 14:01:23 -0500	
> ***************
> *** 1295,1300 ****
> --- 1295,1306 ----
>       (while (/= start end)
>         (setq next (next-single-property-change start prop object end)
>   	    prev (get-text-property start prop object))
> +       ;; Canonicalize old forms of face property.
> +       (and (memq prop '(face font-lock-face))
> + 	   (listp prev)
> + 	   (or (keywordp (car prev))
> + 	       (memq (car prev) '(foreground-color background-color)))
> + 	   (setq prev (list prev)))
>         (put-text-property start next prop
>   			 (append val (if (listp prev) prev (list prev)))
>   			 object)
> ***************
> *** 1309,1314 ****
> --- 1315,1326 ----
>       (while (/= start end)
>         (setq next (next-single-property-change start prop object end)
>   	    prev (get-text-property start prop object))
> +       ;; Canonicalize old forms of face property.
> +       (and (memq prop '(face font-lock-face))
> + 	   (listp prev)
> + 	   (or (keywordp (car prev))
> + 	       (memq (car prev) '(foreground-color background-color)))
> + 	   (setq prev (list prev)))
>         (put-text-property start next prop
>   			 (append (if (listp prev) prev (list prev)) val)
>   			 object)

-- 
Joe

Re: valid forms of face property: documentation bug, font-lock-{prepend, append}-text-property bug

[Richard Stallman]

Your proposal seems pretty good to me.  I've written text to document
it; what do you think?  Meanwhile, I fixed the code in facemenu and startup
that generated other formats.


In the simplest case, the value is a face name.  It can also be a list;
then each element can be any of these possibilities;

@itemize @bullet
@item
A face name (a symbol or string).

@item
A property list of face attributes.  This has the
form (@var{keyword} @var{value} @dots{}), where each @var{keyword} is a
face attribute name and @var{value} is a meaningful value for that
attribute.  With this feature, you do not need to create a face each
time you want to specify a particular attribute for certain text.
@xref{Face Attributes}.

@item
A cons cell with the form @code{(foreground-color . @var{color-name})}
or @code{(background-color . @var{color-name})}.  These are older,
deprecated equivalents for @code{(:foreground @var{color-name})} and
@code{(:background @var{color-name})}.  Please convert code that uses
them.
@end itemize

It works to use the latter two forms directly as the value
of the @code{face} property.

Re: valid forms of face property: documentation bug, font-lock-{prepend, append}-text-property bug

[Richard Stallman]

    NOTE: I'm briefly changing topic to something I noticed while
    investigating this.  Supposedly, facep returns non-nil for internal
    face objects, which are special vectors.  Do these internal face
    objects work as face property values?

I am not sure the question is important.

					   Also, it appears that (facep
    'bold) in fact returns one of these internal face objects, but (facep
    (facep 'bold)) returns nil, so facep seems to violate its own
    documentation.  Is this what is happening?  Or is the vector returned
    by (facep 'bold) not one of the internal face objects?  Can anyone
    answer these questions?

It sounds like a documentation bug to me.  I will fix that.

    By the way, there are two bad implementations of the same concept in
    other places in Emacs.  They should both be replaced by calls to
    font-lock-prepend-text-property or should be changed to use
    font-lock-prepend-text-property.  First, rcirc-add-face function in
    rcirc.el is a bad implementation of the idea of
    font-lock-prepend-text-property which appears not to properly handle
    the case where the value of the face property is a symbol.  Second,
    erc-button-add-face in erc-button.el is yet another bad implementation
    of the same idea that does not handle the case where the face property
    is a list but is not a list of faces.

Could you write a fix for that?