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

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

[Joe Wells]

BUG #1:  The documentation in (info "(elisp)Special Properties") is
misleading about the legal values of the face property.  The
documentation says this:

  `face'
     You can use the property `face' to control the font and color of
     text.  *Note Faces::, for more information.

     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;

        * A face name (a symbol or string).

        * A property list of face attributes.  This has the form
          (KEYWORD VALUE ...), where each KEYWORD is a face attribute
          name and 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.
          *Note Face Attributes::.

        * A cons cell with the form `(foreground-color . COLOR-NAME)' or
          `(background-color . COLOR-NAME)'.  These elements specify
          just the foreground color or just the background color.
          *Note Color Names::, for the supported forms of COLOR-NAME.

          A cons cell of `(foreground-color . COLOR-NAME)' is
          equivalent to specifying `(:foreground COLOR-NAME)'; likewise
          for the background.

This documentation is misleading.  It implies that

(1) if the property value is not a list, then it must be a face name
    (a symbol), and
(2) if one wants to use the property list (KEYWORD VALUE ...) form,
    then one must wrap it inside an extra list.

In fact, this impression is wrong.  All of these have the same visual
effect when inserted in a buffer with font-lock off:

  #("x" 0 1 (face (:background "green")))
  #("x" 0 1 (face ((:background "green"))))
  #("x" 0 1 (face (background-color . "green")))
  #("x" 0 1 (face ((background-color . "green"))))

But it gets worse!  In fact, the following forms are equivalent:

  #("x" 0 1 (face (highlight bold (:foreground "red"))))
  #("x" 0 1 (face (highlight bold :foreground "red")))
  #("x" 0 1 (face (highlight bold foreground-color . "red")))

Similarly, these two forms are equivalent:

  #("x" 0 1 (face (highlight bold)))
  #("x" 0 1 (face (highlight . bold)))

In contrast, these two forms are not equivalent

  #("x" 0 1 (face ((:foreground "red") highlight bold)))
  #("x" 0 1 (face (:foreground "red" highlight bold)))

Based on my investigations, I think the documentation should say
something like this:

     The value of the face property should be of the form FACE-VALUE,
     where FACE-VALUE is either of the form PRIMITIVE-FACE-VALUE or
     (PRIMITIVE-FACE-VALUE . FACE-VALUE).  PRIMITIVE-FACE-VALUE is
     one of these forms:

        * nil.

        * A face name (a symbol or string).

        * A property list of face attributes.  ...

        * A cons cell with the form `(foreground-color . COLOR-NAME)' or
          `(background-color . COLOR-NAME)'.  ...

But please don't just use my proposal without verifying it against the
actual code which interprets the face property!

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,
because they can produce useless results in the presence of the
(KEYWORD VALUE ...) form of the face property.  Furthermore, they can
crash in the presence of the (foreground-color . COLOR) form of the
face property.

For example, evaluating

  (let ((s (propertize "#" 'face '(:foreground "red"))))
    (font-lock-prepend-text-property 0 (length s) 'face 'highlight s)
    (insert s))

produces sensible visual results (with red foreground and
darkseagreen2 background), but evaluating

  (let ((s (propertize "#" 'face 'highlight)))
    (font-lock-prepend-text-property 0 (length s) 'face '(:foreground "red") s)
    (insert s)))

fails to use the specified face “highlight”.

Even worse, evaluating this expression raises an error:

  (let ((s (propertize "#" 'face '(foreground-color . "red"))))
    (font-lock-append-text-property 0 (length s) 'face 'highlight s)
    (insert s))

If font-lock is the only user of these two functions, this issue may
not matter, but it makes it difficult to safely use these functions in
other parts of Emacs.  And this means there is no easy way to safely
add a face to a piece of text that already has one or more face
properties.  Someone who wants to do this would have to write their
own versions of font-lock-{prepend,append}-text-property that do the
right thing for all valid values of the face property.

I hope this bug report is useful.

Joe

======================================================================
In GNU Emacs 22.1.1 (i686-pc-linux-gnu, GTK+ Version 2.8.20)
 of 2007-06-27 on artemis
Windowing system distributor `The X.Org Foundation', version 11.0.70000000
configured using `configure  '--prefix=/home/jbw/local2' '--enable-debug' '--disable-nls' '--with-x-toolkit=gtk' 'CFLAGS=-O0 -g3 -ggdb''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: en_US.UTF-8
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: jbw
  value of $LANG: nil
  locale-coding-system: utf-8
  default-enable-multibyte-characters: t

Minor modes in effect:
  shell-dirtrack-mode: t
  TeX-source-specials-mode: t
  outline-minor-mode: t
  desktop-save-mode: t
  url-handler-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  unify-8859-on-encoding-mode: t
  utf-translate-cjk-mode: t
  auto-compression-mode: t
  temp-buffer-resize-mode: t
  size-indication-mode: t
  line-number-mode: t
  transient-mark-mode: t

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?