Re: Eliminating a couple of independent face definitions

[Tim Cross]

On Mon, Feb 7, 2011 at 11:59 AM, Drew Adams <address@hidden> wrote:

Tim pointed out the problem of faces that are not fully defined, in particular,
for light and dark backgrounds (and for tty).  He pointed to the need for the
Emacs source code to define all faces fully so that they work, out-of-the-box,
regardless of default background etc.

And he pointed to the fact that, given such a basis, inheritance can carry the
ball forward so that newly created faces are more likely, themselves, to be
fully defined - without programmers needing to be face experts or jump through
hoops.

I agreed with Tim that it is good to fully define faces.  Not doing so makes
users customize more than they should need to.  But I also explained why
inheritance can be overkill as a solution to this problem.  Inheritance should
be used when the inheriting face is related to the inherited face - similar
use/meaning/purpose.  It should (generally) be avoided when there is no such
relation.

Inheritance does a good job of enforcing full face definition (given solid
starting points).  Can we get the same benefit some other way - a way that is
not error prone by relying on programmers to copy full definitions by hand etc.?

Sure - just use a copy instead of a pointer.  What's bad about inheriting an
unrelated fully-defined face is that customizing that face also changes the
inheriting face.  But that only happens because the new face always points to
its ancestor for its attribute values.

Why not introduce a new `defface' keyword `:copy-default'?  It would define the
new face with the same default attribute values as another face.  A copy of
those values (actually, of the attributes spec) would be made at `defface' time.
The two faces would remain independent instead of being joined at the hip.  Only
the _default_ attribute values would be used; the current values of the
reference face would have no effect at any time.

If face `barred-foo' is unrelated to font-locking and strings, then instead of
this (which is identical to the `defface' for `font-lock-doc-face):

(defface barred-foo '((t :inherit font-lock-string-face))
 "Face to use for foos that are barred."
 :group 'foobar)

You would use this:

(defface barred-foo '((t :copy-default font-lock-string-face))
 "Face to use for foos that are barred."
 :group 'foobar)

The new face `barred-foo' would have no relation to `font-lock-string-face'.  A
user could customize the latter without that change affecting the former.

The result of the `defface' would be identical to this complex definition (taken
from the definition of `font-lock-string-face'):

(defface barred-foo
 '((((class grayscale) (background light))
    (:foreground "DimGray" :slant italic))
   (((class grayscale) (background dark))
    (:foreground "LightGray" :slant italic))
   (((class color) (min-colors 88)
    (background light)) (:foreground "VioletRed4"))
   (((class color) (min-colors 88)
    (background dark))  (:foreground "LightSalmon"))
   (((class color) (min-colors 16)
    (background light)) (:foreground "RosyBrown"))
   (((class color) (min-colors 16)
    (background dark))  (:foreground "LightSalmon"))
   (((class color) (min-colors 8))
    (:foreground "green"))
   (t (:slant italic)))
 "Face to use for foos that are barred."
 :group 'foobar)

Simple for even lazy programmers to use.  Not so error-prone for eager
programmers who might otherwise try to make such a copy by hand.  Guaranteed to
be as reasonable for all backgrounds and tty as is the tried-and-true
`font-lock-string-face'.

Anywhere you might use `:inherit' in a face definition you could use
`:copy-default'.  The same `defface' could use both `:inherit' and
`:copy-default', to inherit some attribute values from one face and copy others
(defaults) from another.

We would encourage programmers to use `:inherit' when the new face (its
use/meaning/purpose) is related to the referenced face - that is, when they want
a change in the latter to be reflected in the former.  (`font-lock-doc-face'
inherits from `font-lock-string-face' because they are related.)

We would encourage them to use `:copy-default' when the referenced face is
unrelated and all they want to do is reuse its default-attributes spec.
(`barred-foo' is about foos, not about font-locking or strings.)

I expect that the latter case is more common than the former, but I could be
wrong.

New faces are often created together, as a group in some library, and these are
often related in terms of use/purpose, so inheritance among them can make sense.
But it is less likely that there is an existing face outside of that context
whose use/meaning/purpose is related.  IOW, I see inheritance as most useful
within a library or among related libraries.

It's worth quoting Tim again here.  Having `:copy-default' in addition to
`:inherit' would improve the solution, I think.

T> I would not argue that inheritance is an ideal solution to
T> this problem,  However, I do think it can be part of the
T> solution.  Perhaps something along the lines of
T>
T>  * Establish guidelines on how to use inheritance i.e. how to
T>    select which face to inherit from
T>  * Define a good (not too large) set of base faces.  Existing
T>    font-lock faces may be sufficient, maybe not.  Would need
T>    review.
T>  * Require all face definitions in core emacs packages to
T>    either fully define a face (i.e. definition for dark/light,
T>    tty, X mac ms etc) OR inherit from a base face (assuming
T>    all base faces are fully defined)
T>  * Add a section to the manual encouraging developers to
T>    either provide a fully defined face or inherit from a base
T>    face, but don't just define a single (usually) light
T>    background face
T>
T> The key here is that all faces in core emacs packages would
T> end up with a fully defined face, either explicitly or via
T> inherit.

Ignoring implementation issues, your suggestion appears to meet the main objective i.e. making it easier for programmers to do the right thing and have fully defined faces without having to jump through all the hoops. I would agree that it is a better solution than using inheritance with semantically unrelated parents as it does appear to provide a solution that does not compromise the strict form of preferred inheritance and a way to improve the quality of default face values.

I would suggest that in addition to such enhancements to defface, we need to provide guidelines in the manual on how to use inheritance and copy and would go further and argue that for semantically related faces, inheritance *should* be used to help enhance consistency and allow users to customize a semantic 'class' in one go.

Tim