emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Joe Wells
Subject: Re: valid forms of face property: documentation bug, font-lock-{prepend, append}-text-property bug
Date: Sat, 29 Dec 2007 06:52:35 +0000
User-agent: Gnus/5.11 (Gnus v5.11) Emacs/22.1 (gnu/linux)

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 <address@hidden> 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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]