Face initialization

Face initialization

[Chong Yidong]

After staring at face-set-after-frame-default for a while, I think it
can be simplified somewhat.  If we only set frame parameters based on
the parameter list explicitly passed to `make-frame', it shouldn't be
necessary to mess around with the default face in the beginning of the
function.  Also, the call to make-face-x-resource-internal seems to be
unnecessary.  I think this also fixes some of the problems with faces in
new frames that we've been seeing.

Could someone test this patch out and see if I've missed anything?


*** trunk/lisp/faces.el.~1.417.~	2008-07-06 01:05:46.000000000 -0400
--- trunk/lisp/faces.el	2008-07-06 01:46:31.000000000 -0400
***************
*** 1993,1999 ****
  	  (x-setup-function-keys frame)
  	  (x-handle-reverse-video frame parameters)
  	  (frame-set-background-mode frame)
! 	  (face-set-after-frame-default frame)
  	  ;; Make sure the tool-bar is ready to be enabled.  The
  	  ;; `tool-bar-lines' frame parameter will not take effect
  	  ;; without this call.
--- 1993,1999 ----
  	  (x-setup-function-keys frame)
  	  (x-handle-reverse-video frame parameters)
  	  (frame-set-background-mode frame)
! 	  (face-set-after-frame-default frame parameters)
  	  ;; Make sure the tool-bar is ready to be enabled.  The
  	  ;; `tool-bar-lines' frame parameter will not take effect
  	  ;; without this call.
***************
*** 2006,2031 ****
  	(delete-frame frame)))
      frame))
  
! (defun face-set-after-frame-default (frame)
    "Set frame-local faces of FRAME from face specs and resources.
  Initialize colors of certain faces from frame parameters."
-   (if (face-attribute 'default :font t)
-       (set-face-attribute 'default frame :font
- 			  (face-attribute 'default :font t))
-     (set-face-attribute 'default frame :family
- 			(face-attribute 'default :family t))
-     (set-face-attribute 'default frame :height
- 			(face-attribute 'default :height t))
-     (set-face-attribute 'default frame :slant
- 			(face-attribute 'default :slant t))
-     (set-face-attribute 'default frame :weight
- 			(face-attribute 'default :weight t))
-     (set-face-attribute 'default frame :width
- 			(face-attribute 'default :width t)))
    ;; Find attributes that should be initialized from frame parameters.
    (let ((face-params '((foreground-color default :foreground)
  		       (background-color default :background)
!                        (font-parameter default :font)
  		       (border-color border :background)
  		       (cursor-color cursor :background)
  		       (scroll-bar-foreground scroll-bar :foreground)
--- 2006,2018 ----
  	(delete-frame frame)))
      frame))
  
! (defun face-set-after-frame-default (frame parameters)
    "Set frame-local faces of FRAME from face specs and resources.
  Initialize colors of certain faces from frame parameters."
    ;; Find attributes that should be initialized from frame parameters.
    (let ((face-params '((foreground-color default :foreground)
  		       (background-color default :background)
!                        (font default :font)
  		       (border-color border :background)
  		       (cursor-color cursor :background)
  		       (scroll-bar-foreground scroll-bar :foreground)
***************
*** 2033,2071 ****
  		       (mouse-color mouse :background)))
  	apply-params)
      (dolist (param face-params)
!       (let* ((value (frame-parameter frame (nth 0 param)))
  	     (face (nth 1 param))
  	     (attr (nth 2 param))
! 	     (default-value (face-attribute face attr t)))
  	;; Compile a list of face attributes to set, but don't set
  	;; them yet.  The call to make-face-x-resource-internal,
  	;; below, can change frame parameters, and the final set of
  	;; frame parameters should be the ones acquired at this step.
! 	(if (eq default-value 'unspecified)
! 	    ;; The face spec does not specify a new-frame value for
! 	    ;; this attribute.  Check if the existing frame parameter
! 	    ;; specifies it.
! 	    (if value
! 		(push (list face frame attr value) apply-params))
! 	  ;; The face spec specifies a value for this attribute, to be
! 	  ;; applied to the face on all new frames.
! 	  (push (list face frame attr default-value) apply-params))))
!     ;; Initialize faces from face specs and X resources.  The
!     ;; condition-case prevents invalid specs from causing frame
      ;; creation to fail.
      (dolist (face (face-list))
-       ;; This loop used to exclude the `default' face for an unknown reason.
-       ;; It lead to odd behaviors where face-spec settings on the `default'
-       ;; face weren't obeyed for new frame.
        (condition-case ()
  	  (progn
  	    (face-spec-recalc face frame)
- 	    (if (memq (window-system frame) '(x w32 mac))
- 		(make-face-x-resource-internal face frame))
  	    (internal-merge-in-global-face face frame))
  	(error nil)))
!     ;; Apply the attributes specified by frame parameters.  This
!     ;; rewrites parameters changed by make-face-x-resource-internal
      (dolist (param apply-params)
        (apply 'set-face-attribute param))))
  
--- 2020,2046 ----
  		       (mouse-color mouse :background)))
  	apply-params)
      (dolist (param face-params)
!       (let* ((param-name (nth 0 param))
  	     (face (nth 1 param))
  	     (attr (nth 2 param))
! 	     (value (cdr (or (assq param-name parameters)
! 			     (assq param-name default-frame-alist)))))
  	;; Compile a list of face attributes to set, but don't set
  	;; them yet.  The call to make-face-x-resource-internal,
  	;; below, can change frame parameters, and the final set of
  	;; frame parameters should be the ones acquired at this step.
! 	(if value
! 	    (push (list face frame attr value) apply-params))))
!     ;; Initialize faces from face specs and face-new-frame-defaults.
!     ;; The condition-case prevents invalid specs from causing frame
      ;; creation to fail.
      (dolist (face (face-list))
        (condition-case ()
  	  (progn
  	    (face-spec-recalc face frame)
  	    (internal-merge-in-global-face face frame))
  	(error nil)))
!     ;; Apply the attributes specified by frame parameters.
      (dolist (param apply-params)
        (apply 'set-face-attribute param))))
  
***************
*** 2104,2110 ****
              (set-locale-environment nil frame)
              (tty-run-terminal-initialization frame))
  	  (frame-set-background-mode frame)
! 	  (face-set-after-frame-default frame)
  	  (setq success t))
        (unless success
  	(delete-frame frame)))
--- 2079,2085 ----
              (set-locale-environment nil frame)
              (tty-run-terminal-initialization frame))
  	  (frame-set-background-mode frame)
! 	  (face-set-after-frame-default frame parameters)
  	  (setq success t))
        (unless success
  	(delete-frame frame)))
***************
*** 2160,2166 ****
  (defun tty-set-up-initial-frame-faces ()
    (let ((frame (selected-frame)))
      (frame-set-background-mode frame)
!     (face-set-after-frame-default frame)))
  
  
  
--- 2135,2141 ----
  (defun tty-set-up-initial-frame-faces ()
    (let ((frame (selected-frame)))
      (frame-set-background-mode frame)
!     (face-set-after-frame-default frame nil)))
  
  
  

Re: Face initialization

[Stefan Monnier]

> After staring at face-set-after-frame-default for a while, I think it
> can be simplified somewhat.  If we only set frame parameters based on
> the parameter list explicitly passed to `make-frame', it shouldn't be
> necessary to mess around with the default face in the beginning of the
> function.

Interesting: could you explain why you think so?

> Also, the call to make-face-x-resource-internal seems to be
> unnecessary.

Why?


        Stefan

Re: Face initialization

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> After staring at face-set-after-frame-default for a while, I think it
>> can be simplified somewhat.  If we only set frame parameters based on
>> the parameter list explicitly passed to `make-frame', it shouldn't be
>> necessary to mess around with the default face in the beginning of the
>> function.
>
> Interesting: could you explain why you think so?

I think on reason for the stuff in the beginning was for the following
check:

! 	     (default-value (face-attribute face attr t)))
....
! 	(if (eq default-value 'unspecified)
! 	    ;; The face spec does not specify a new-frame value for
! 	    ;; this attribute.  Check if the existing frame parameter
! 	    ;; specifies it.
! 	    (if value
! 		(push (list face frame attr value) apply-params))
! 	  ;; The face spec specifies a value for this attribute, to be
! 	  ;; applied to the face on all new frames.
! 	  (push (list face frame attr default-value) apply-params))))

If we only set the frame parameters explicitly specified for that frame,
this is not required.  I think all the other information is already
present in the face spec.

>> Also, the call to make-face-x-resource-internal seems to be
>> unnecessary.
>
> Why?

The X resource settings are already stored in the `theme-face' property
of the default face, so face-spec-recalc applies it (now that it is
called for the default face).

Re: Face initialization

[Richard M Stallman]

    The X resource settings are already stored in the `theme-face' property
    of the default face, so face-spec-recalc applies it (now that it is
    called for the default face).

At present, Emacs implements frame-specific X resources.
Maybe that call is needed to handle them.

I proposed desupporting frame-specific X resources.
If we do that, maybe this call will cease to be neede.

Re: Face initialization

[Chong Yidong]

Richard M Stallman <rms@gnu.org> writes:

> At present, Emacs implements frame-specific X resources.
> Maybe that call is needed to handle them.
>
> I proposed desupporting frame-specific X resources.
> If we do that, maybe this call will cease to be neede.

I agree that we should desupport frame-specific X resources.  But in the
first place, is this feature documented?  I didn't see a mention of it
in the manual.

Re: Face initialization

[Stefan Monnier]

>>> After staring at face-set-after-frame-default for a while, I think it
>>> can be simplified somewhat.  If we only set frame parameters based on
>>> the parameter list explicitly passed to `make-frame', it shouldn't be
>>> necessary to mess around with the default face in the beginning of the
>>> function.
>> Interesting: could you explain why you think so?
> I think on reason for the stuff in the beginning was for the following
> check:

> ! 	     (default-value (face-attribute face attr t)))
> ....
> ! 	(if (eq default-value 'unspecified)
> ! 	    ;; The face spec does not specify a new-frame value for
> ! 	    ;; this attribute.  Check if the existing frame parameter
> ! 	    ;; specifies it.
> ! 	    (if value
> ! 		(push (list face frame attr value) apply-params))
> ! 	  ;; The face spec specifies a value for this attribute, to be
> ! 	  ;; applied to the face on all new frames.
> ! 	  (push (list face frame attr default-value) apply-params))))

> If we only set the frame parameters explicitly specified for that frame,
> this is not required.  I think all the other information is already
> present in the face spec.

Oh, I see I misunderstood what you were saying.  Yes, maybe you're right
on this one.

>>> Also, the call to make-face-x-resource-internal seems to be
>>> unnecessary.
>> Why?

> The X resource settings are already stored in the `theme-face' property
> of the default face, so face-spec-recalc applies it (now that it is
> called for the default face).

Oh, I didn't know about that.  Do you happen to know the place in the
source code where that `theme-face' is set?  [I'm trying to track down
a problem where X resources take too much precedence and maybe this
will help me figure it out]


        Stefan

Re: Face initialization

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The X resource settings are already stored in the `theme-face' property
>> of the default face, so face-spec-recalc applies it (now that it is
>> called for the default face).
>
> Oh, I didn't know about that.  Do you happen to know the place in the
> source code where that `theme-face' is set?  [I'm trying to track down
> a problem where X resources take too much precedence and maybe this
> will help me figure it out]

My mistake again :-P  The entry in theme-face isn't used for new frames.
Its purpose is to record the face settings (derived from X resources
etc) prior to what is set by customization, so that it can be restored
when the customization is removed.

If I'm not mistaken, the X resources for new frames are applied during
x-create-frame itself.  It calls x_default_parameter, which consults X
resource settings.  Playing around with the face code indicates that the
X resources are indeed already applied when the frame is created, before
face-set-after-frame-default is called.

So I think the following simplified procedure for
x-create-frame-with-faces should DTRT (with the exception of the font
parameter, which I'm not sure how to handle):

(defun face-set-after-frame-default (frame parameters)
  (dolist (face (face-list))
    (condition-case ()
	(progn
	  ;; Initialize faces from face spec and custom theme.
	  (face-spec-recalc face frame)
	  ;; Apply attributes specified by face-new-frame-defaults
	  (internal-merge-in-global-face face frame))
      ;; Don't let invalid specs prevent frame creation.
      (error nil)))
  ;; Apply attributes specified by frame parameters explicitly
  ;; passed to `make-frame' or in `default-frame-alist'.
  (let ((face-params '((foreground-color default :foreground)
  		       (background-color default :background)
  		       (border-color border :background)
  		       (cursor-color cursor :background)
  		       (scroll-bar-foreground scroll-bar :foreground)
  		       (scroll-bar-background scroll-bar :background)
  		       (mouse-color mouse :background))))
    (dolist (param face-params)
      (let* ((param-name (nth 0 param))
  	     (value (cdr (or (assq param-name parameters)
  			     (assq param-name default-frame-alist)))))
  	(if value
  	    (set-face-attribute (nth 1 param) frame
				(nth 2 param) value))))))

Re: Face initialization

[Stefan Monnier]

> So I think the following simplified procedure for
> x-create-frame-with-faces should DTRT (with the exception of the font
> parameter, which I'm not sure how to handle):

What's the problem with the `font' parameter?


        Stefan

Re: Face initialization

[Richard M Stallman]

    > So I think the following simplified procedure for
    > x-create-frame-with-faces should DTRT (with the exception of the font
    > parameter, which I'm not sure how to handle):

    What's the problem with the `font' parameter?

I suggested eliminating the `font' parameter and instead
defining a function which sets the default face according to an
X font descriptor.

Re: Face initialization

[Stephen J. Turnbull]

Richard M Stallman writes:

 > I suggested eliminating the `font' parameter and instead
 > defining a function which sets the default face according to an
 > X font descriptor.

I suggest that the main advertised interface should use Xft/fontconfig
notation.  Even if the underlying internal engine is XLFD-based.  Xft
is trivial to translate correctly to XLFD form[1][2], more flexible,
easier to compose correctly, and quite possibly more familiar to most
users.

Of course there should be a helper function which translates XLFDs
(and even "loser" font names that are not XLFDs) into Xft form.

Footnotes: 
[1]  Admittedly semantics differ and this may matter, as some users of
XLFDs may prefer to specify size in pixels while Xft makes that difficult to
do.  Personally, I've had no trouble getting used to it but YMMV.

[2]  This may require some Emacs-specific fontconfig properties (which
will be ignored by the Xft font engine, of course), to emulate XLFD
semantics that Xft doesn't provide.  fontconfig is quite happy to do
this for you; the hardest problem is choosing an appropriate naming
convention for these properties.

Re: Face initialization

[Richard M Stallman]

    I suggest that the main advertised interface should use Xft/fontconfig
    notation.

I do not know this notation, but I won't argue against this.
Can one function accept both of these forms of font specification?

Re: Face initialization

[Jason Rumney]

Richard M Stallman wrote:
>     I suggest that the main advertised interface should use Xft/fontconfig
>     notation.
> 
> I do not know this notation, but I won't argue against this.
> Can one function accept both of these forms of font specification?

Currently the -fn command line argument accepts 3 forms of font
specification; GTK, Fontconfig and XLFD, so there is no reason why any
other place where fonts are specified as strings could not do the same.
GTK and Fontconfig formats are much easier for new users to deal with,
as most of the information is optional, and in their simplest forms,
they can just be the family name of a font, though for the default font
you will usually want to specify a size as well.


The following are equivalents in the different formats:

GTK:        Fixed 12
Fontconfig: Fixed-12
XLFD:       -*-Fixed-normal-r-normal-*-*-120-*-*-*-*-*-*

GTK:        Fixed Bold 12
Fontconfig: Fixed-12:bold
XLFD:       -*-Fixed-bold-r-normal-*-*-120-*-*-*-*-*-*

GTK:        unrepresentable (AFAIK)
Fontconfig: Fixed-12:bold:foundry=misc
XLFD:       -misc-Fixed-bold-r-normal-*-*-120-*-*-*-*-*-*


AFAIK GTK format only supports specifying family, point size, weight and
slant, while fontconfig supports those, plus arbitrary named properties,
which can include properties beyond the 14 fields in an XLFD descriptor.

Re: Face initialization

[Richard M Stallman]

    Currently the -fn command line argument accepts 3 forms of font
    specification; GTK, Fontconfig and XLFD, so there is no reason why any
    other place where fonts are specified as strings could not do the same.

I am glad it is feasible to handle all three.  My suggestion is to
make a function `set-face-font' to do that, and get rid of the `font'
frame parameter.

Re: Face initialization

[Richard M Stallman]

Frame-specific face X resources:

The manual fails to explain it clearly, but x-create-frame sets
x-resource-name to the frame name.  So if you create a frame named
`foo', initializing its default face looks for resources such as
`foo.default.attributeForeground', and so on.

The only case for which this is documented is that of the initial
frame with --name.

Re: Face initialization

[Chong Yidong]

Richard M Stallman <rms@gnu.org> writes:

> Frame-specific face X resources:
>
> The manual fails to explain it clearly, but x-create-frame sets
> x-resource-name to the frame name.  So if you create a frame named
> `foo', initializing its default face looks for resources such as
> `foo.default.attributeForeground', and so on.
>
> The only case for which this is documented is that of the initial
> frame with --name.

I see.

Okay, does anyone object to desupporting frame-specific X resources?

Re: Face initialization

[Jason Rumney]

Chong Yidong wrote:

> Okay, does anyone object to desupporting frame-specific X resources?

It seems your change for this has also desupported the use of
frame-independent X resources for faces.

The following should demonstrate this rather loudly (on previous builds):

emacs -Q -xrm Emacs.tool-bar.attributeBackground:red

Re: Face initialization

[Chong Yidong]

Jason Rumney <jasonr@gnu.org> writes:

> Chong Yidong wrote:
>
>> Okay, does anyone object to desupporting frame-specific X resources?
>
> It seems your change for this has also desupported the use of
> frame-independent X resources for faces.
>
> The following should demonstrate this rather loudly (on previous builds):
>
> emacs -Q -xrm Emacs.tool-bar.attributeBackground:red

I'll check in a fix ASAP.

Re: Face initialization

[Miles Bader]

Jason Rumney <jasonr@gnu.org> writes:
>> Okay, does anyone object to desupporting frame-specific X resources?
>
> It seems your change for this has also desupported the use of
> frame-independent X resources for faces.
>
> The following should demonstrate this rather loudly (on previous builds):
>
> emacs -Q -xrm Emacs.tool-bar.attributeBackground:red

Do we really care about those anyway?

-Miles

-- 
Is it true that nothing can be known?  If so how do we know this?  -Woody Allen

Re: Face initialization

[Jason Rumney]

Miles Bader wrote:
> Jason Rumney <jasonr@gnu.org> writes:
>>> Okay, does anyone object to desupporting frame-specific X resources?
>> It seems your change for this has also desupported the use of
>> frame-independent X resources for faces.
>>
>> The following should demonstrate this rather loudly (on previous builds):
>>
>> emacs -Q -xrm Emacs.tool-bar.attributeBackground:red
> 
> Do we really care about those anyway?

We care enough to devote a subsection in the manual to it, and I wasn't
the first to notice they were missing.

For the tool-bar and tooltips especially, it is a good way for platforms
and distributions to make Emacs fit with the rest of the system without
interfering with customize.

Re: Face initialization

[Stefan Monnier]

> For the tool-bar and tooltips especially, it is a good way for platforms
> and distributions to make Emacs fit with the rest of the system without
> interfering with customize.

You mean "without *using* customize", right?  Because it does
interfere, AFAIK.


        Stefan

Re: Face initialization

[Jason Rumney]

Stefan Monnier wrote:
>> For the tool-bar and tooltips especially, it is a good way for platforms
>> and distributions to make Emacs fit with the rest of the system without
>> interfering with customize.
> 
> You mean "without *using* customize", right?  Because it does
> interfere, AFAIK.

It depends on what you mean by interfere, I guess. If the user
customizes those faces, then they do not get "changed outside of
customize" or "set and saved" messages like they would if you used
set-face-* functions or customize-face functions respectively. Also you
don't have to worry about load order of files in site-lisp, X resources
are processed early enough in startup that they don't interfere with any
user customizations.

Re: Face initialization

[Richard M Stallman]

      Also, the call to make-face-x-resource-internal seems to be
    unnecessary.

SOMETHING needs to check for those X resources.
Is it done elsewhere?  Is this call redundant?

I don't entirely understand what is going on in this change,
but if you think that certain steps are unnecessary,
please double check.  It is easy to make mistakes in this code.

    -   (if (face-attribute 'default :font t)
    -       (set-face-attribute 'default frame :font
    - 			  (face-attribute 'default :font t))
    -     (set-face-attribute 'default frame :family
    - 			(face-attribute 'default :family t))
    -     (set-face-attribute 'default frame :height
    - 			(face-attribute 'default :height t))
    -     (set-face-attribute 'default frame :slant
    - 			(face-attribute 'default :slant t))
    -     (set-face-attribute 'default frame :weight
    - 			(face-attribute 'default :weight t))
    -     (set-face-attribute 'default frame :width
    - 			(face-attribute 'default :width t)))

The purpose of that code is to initialize `default'
for this frame from the frame-independent face attributes.

If this is done elsewhere, then maybe this code is not needed,
but how about adding comments that explain all the overall logic
for giving the faces the right settings?