Re: Eliminating a couple of independent face definitions

[Tim Cross <theophilusx@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6983 bytes --]

On Mon, Feb 7, 2011 at 11:59 AM, Drew Adams <drew.adams@oracle.com> 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

[-- Attachment #2: Type: text/html, Size: 7964 bytes --]