Faces applies to new frames

Faces applies to new frames

[Chong Yidong]

I took a look at the problem that several people have reported regarding
default faces not being applied to new frames (e.g. 47 and 120 in the
bug tracker).

AFAICT, the problem is that the default face is controlled by the `font'
frame parameter, but this is not saved to default-frame-alist by
internal-set-lisp-face-attribute.  In comparison, the foreground-color
and background-color frame attributes are saved properly (see
xfaces.c:3415 and 3470).  It doesn't seem practical to fix
internal-set-lisp-face-attribute, but I think we can patch it up in
faces.el.

Could someone check whether this reasoning makes sense, and whether the
attached patch to set-face-attribute makes things work properly?

(BTW, this doesn't affect default faces set under customize under some
circumstances, because that changes the defface spec directly instead of
using set-face-attribute.)


*** trunk/lisp/faces.el.~1.415.~	2008-06-25 18:27:01.000000000 -0400
--- trunk/lisp/faces.el	2008-06-26 18:41:07.000000000 -0400
***************
*** 700,706 ****
  VALUE is the name of a face from which to inherit attributes, or a list
  of face names.  Attributes from inherited faces are merged into the face
  like an underlying face would be, with higher priority than underlying faces."
!   (let ((where (if (null frame) 0 frame)))
      (setq args (purecopy args))
      ;; If we set the new-frame defaults, this face is modified outside Custom.
      (if (memq where '(0 t))
--- 700,716 ----
  VALUE is the name of a face from which to inherit attributes, or a list
  of face names.  Attributes from inherited faces are merged into the face
  like an underlying face would be, with higher priority than underlying faces."
!   (let* ((where (if (null frame) 0 frame))
! 	 (save-modified-font
! 	  (and (eq where 0)
! 	       (eq face 'default)
! 	       (or (plist-member args :family)
! 		   (plist-member args :height)
! 		   (plist-member args :weight)
! 		   (plist-member args :slant)
! 		   (plist-member args :width)
! 		   (plist-member args :bold)
! 		   (plist-member args :italic)))))
      (setq args (purecopy args))
      ;; If we set the new-frame defaults, this face is modified outside Custom.
      (if (memq where '(0 t))
***************
*** 723,729 ****
  	  (internal-set-lisp-face-attribute face (car args)
  					    (purecopy (cadr args))
  					    where)))
!       (setq args (cdr (cdr args))))))
  
  
  (defun make-face-bold (face &optional frame noerror)
--- 733,744 ----
  	  (internal-set-lisp-face-attribute face (car args)
  					    (purecopy (cadr args))
  					    where)))
!       (setq args (cdr (cdr args))))
! 
!     (when save-modified-font
!       (setq default-frame-alist
!       	    (cons (cons 'font (frame-parameter (selected-frame) 'font))
!       		  (assq-delete-all 'font default-frame-alist))))))
  
  
  (defun make-face-bold (face &optional frame noerror)

Re: Faces applies to new frames

[Miles Bader]

Chong Yidong <cyd@stupidchicken.com> writes:
> I took a look at the problem that several people have reported regarding
> default faces not being applied to new frames (e.g. 47 and 120 in the
> bug tracker).
>
> AFAICT, the problem is that the default face is controlled by the `font'
> frame parameter, but this is not saved to default-frame-alist by
> internal-set-lisp-face-attribute.  In comparison, the foreground-color
> and background-color frame attributes are saved properly (see
> xfaces.c:3415 and 3470).  It doesn't seem practical to fix
> internal-set-lisp-face-attribute, but I think we can patch it up in
> faces.el.
>
> Could someone check whether this reasoning makes sense, and whether the
> attached patch to set-face-attribute makes things work properly?

Note that in my previous bug report about (apparently) this issue, the
"variable-pitch" face also failed to apply properly to new frames.  So
it's not just "default".

-Miles

-- 
Youth, n. The Period of Possibility, when Archimedes finds a fulcrum,
Cassandra has a following and seven cities compete for the honor of endowing a
living Homer.

Re: Faces applies to new frames

[Stefan Monnier]

> AFAICT, the problem is that the default face is controlled by the `font'
> frame parameter, but this is not saved to default-frame-alist by
> internal-set-lisp-face-attribute.  In comparison, the foreground-color

I'm not convinced this is the root of the problem.  For one, by default
there is no `font' parameter in default-frame-alist, so as long as we
leave it that way, it should not affect face settings.  The code in
face-set-after-frame-default should apply the defface-spec to every
new frame, so we shouldn't need any entry in default-frame-alist for it
to apply to new frames.  The face-set-after-frame-default used to skip
`default', so that was an important reason for the bug#47.  I've fixed
this part, but there are other weird things going on (I have some other
weird problems here where X resources are obeyed even after I remove
the calls to `make-face-x-resource-internal').

I think we need to write down every step of how faces are initialized
for each frame (then lament the fact faces are the same on all frames in
99.99% of the cases), then how frame-parameters are treated, and then
how they affect each other.


        Stefan

Re: Faces applies to new frames

[Chong Yidong]

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

>> AFAICT, the problem is that the default face is controlled by the `font'
>> frame parameter, but this is not saved to default-frame-alist by
>> internal-set-lisp-face-attribute.  In comparison, the foreground-color
>
> I'm not convinced this is the root of the problem.  For one, by default
> there is no `font' parameter in default-frame-alist, so as long as we
> leave it that way, it should not affect face settings.  The code in
> face-set-after-frame-default should apply the defface-spec to every
> new frame, so we shouldn't need any entry in default-frame-alist for it
> to apply to new frames.  The face-set-after-frame-default used to skip
> `default', so that was an important reason for the bug#47.  I've fixed
> this part, but there are other weird things going on (I have some other
> weird problems here where X resources are obeyed even after I remove
> the calls to `make-face-x-resource-internal').

Maybe there's more than one bug here.  When set-face-attribute is called
with a nil FRAME argument, that changes the face for all frames, and
this is done via frame parameters and not by changing the defface spec.
I don't think face-set-after-frame-default is even relevant to this
case.

From looking inside internal-set-lisp-face-attribute, it seems that the
face for future frames is changed by changing the frame parameters
`foreground', `background', etc.  The `font' parameter is changed for
each existing frame (xfaces.c:3411), but nothing is saved in
default-frame-alist.

Re: Faces applies to new frames

[Stefan Monnier]

>>> AFAICT, the problem is that the default face is controlled by the `font'
>>> frame parameter, but this is not saved to default-frame-alist by
>>> internal-set-lisp-face-attribute.  In comparison, the foreground-color
>> 
>> I'm not convinced this is the root of the problem.  For one, by default
>> there is no `font' parameter in default-frame-alist, so as long as we
>> leave it that way, it should not affect face settings.  The code in
>> face-set-after-frame-default should apply the defface-spec to every
>> new frame, so we shouldn't need any entry in default-frame-alist for it
>> to apply to new frames.  The face-set-after-frame-default used to skip
>> `default', so that was an important reason for the bug#47.  I've fixed
>> this part, but there are other weird things going on (I have some other
>> weird problems here where X resources are obeyed even after I remove
>> the calls to `make-face-x-resource-internal').

> Maybe there's more than one bug here.  When set-face-attribute is called
> with a nil FRAME argument, that changes the face for all frames, and
> this is done via frame parameters and not by changing the defface spec.
> I don't think face-set-after-frame-default is even relevant to this
> case.

Indeed, there's also the distinction between face changes made via
customize and those made by direct calls to set-face-attribute.

The more I look at it, the mor it seems obvious that we cannot fix it by
looking at individual events, but we need to first figure out it
currently (fails to) work overall, and then design a way to make it work
in an overall consistent way.


        Stefan

Re: Faces applies to new frames

[Chong Yidong]

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

>> Maybe there's more than one bug here.  When set-face-attribute is called
>> with a nil FRAME argument, that changes the face for all frames, and
>> this is done via frame parameters and not by changing the defface spec.
>> I don't think face-set-after-frame-default is even relevant to this
>> case.
>
> Indeed, there's also the distinction between face changes made via
> customize and those made by direct calls to set-face-attribute.
>
> The more I look at it, the mor it seems obvious that we cannot fix it
> by looking at individual events, but we need to first figure out it
> currently (fails to) work overall, and then design a way to make it
> work in an overall consistent way.

This is how information is currently saved for future frames.  In
addition to the defface spec, which stores information in the Lisp
symbol of the face name, there are two relevant variables:
default-frame-alist, which stores frame parameters for future frames,
and face-new-frame-defaults, which stores the default face definitions.

Finternal_set_lisp_face_attribute (called by set-face-attribute) is
supposed to do two things: changes the face on existing frames, and (if
passed an integer FRAME argument) change the defaults for future frames.

Currently, the latter is accomplished for only certain attributes: the
(default) foreground and background color, the default font, and the
colors for the border, cursor, scroll-bar, and mouse.  These are saved
by modifying default-frame-alist.  It does NOT modify
face-new-frame-defaults.

When a new frame is created, the faces are initialized using
face-set-after-frame-default.  This does a few things:

 1. Set the default face based on face-new-frame-defaults, via
    face-attribute with a t FRAME argument.  (It only does this if
    inhibit-face-set-after-frame-default is nil, which may be bogus,
    since that variable is only bound when doing set-face-attribute,
    which does not create frames.)

 2. Walk through (face-list), applying defface specs to the rest of the
    faces for this frame.

 3. Walk through (face-list), applying X resource settings to the rest
    of the faces for this frame.

 4. Apply the frame parameter based face attributes describe above
    (foreground and background color, the default font, and the colors
    for the border, cursor, scroll-bar, and mouse).

As a result, set-face-attribute does not seem to change attributes on
future frames, other than the foreground and background color, contrary
to the description of internal-set-lisp-face-attribute.

How about this: change internal-set-lisp-face-attribute so that its only
role is to change the faces on existing frames, leaving the frame
parameters alone.  Then, handle the "saving for future frames" part in
the Lisp function set-face-attribute.  Maybe it should do this by
modifying face-new-frame-defaults and/or the defface spec.

Re: Faces applies to new frames

[Stefan Monnier]

That's helpful, thanks.  We need to put this info somewhere in the the
CVS, as near the source as possible, but since the corresponding source
code is scattered, we may as well put it elsewhere (and refer to it from
the various relevant code sections).

> Finternal_set_lisp_face_attribute (called by set-face-attribute) is
> supposed to do two things: changes the face on existing frames, and (if
> passed an integer FRAME argument) change the defaults for future frames.

Hopefully Custom never calls it with an integer frame argument.

> Currently, the latter is accomplished for only certain attributes: the
> (default) foreground and background color, the default font, and the
> colors for the border, cursor, scroll-bar, and mouse.  These are saved
> by modifying default-frame-alist.  It does NOT modify
> face-new-frame-defaults.

Why doesn't change face-new-frame-defaults?  Who does?

>  1. Set the default face based on face-new-frame-defaults, via
>     face-attribute with a t FRAME argument.  (It only does this if
>     inhibit-face-set-after-frame-default is nil, which may be bogus,
>     since that variable is only bound when doing set-face-attribute,
>     which does not create frames.)

Yes, my recent change to fix the inf-recursion should make this
variable useless.

>  2. Walk through (face-list), applying defface specs to the rest of the
>     faces for this frame.

>  3. Walk through (face-list), applying X resource settings to the rest
>     of the faces for this frame.

>  4. Apply the frame parameter based face attributes describe above
>     (foreground and background color, the default font, and the colors
>     for the border, cursor, scroll-bar, and mouse).

But this #4 seems wrong: if set-face-attribute stores global settings in
those variables, then it means that step #4 will apply those global
settings over the defface settings, which are per-terminal.

> As a result, set-face-attribute does not seem to change attributes on
> future frames, other than the foreground and background color, contrary
> to the description of internal-set-lisp-face-attribute.

Sounds like you're right ;-)

> How about this: change internal-set-lisp-face-attribute so that its only
> role is to change the faces on existing frames, leaving the frame
> parameters alone.  Then, handle the "saving for future frames" part in
> the Lisp function set-face-attribute.

Given that those settings are used/applied by Elisp code (in
face-set-after-frame-default), I guess it makes sense to move that
C code to Elisp code as you suggest: this will bring the producer and
consumer into the same file, which can only be beneficial).

> Maybe it should do this by
> modifying face-new-frame-defaults and/or the defface spec.

Updating the defface specs directly sounds attractive, but it might turn
out to be difficult to do it right.


        Stefan

Re: Faces applies to new frames

[Chong Yidong]

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

>> Currently, the latter is accomplished for only certain attributes: the
>> (default) foreground and background color, the default font, and the
>> colors for the border, cursor, scroll-bar, and mouse.  These are saved
>> by modifying default-frame-alist.  It does NOT modify
>> face-new-frame-defaults.
>
> Why doesn't change face-new-frame-defaults?  Who does?

After looking at this some more, I have to take that back.  When
Finternal_set_lisp_face_attribute is told to modify the defaults, it
acts on the face vector stored in face-new-frame-defaults.

I guess this needs to be further disentangled.

Re: Faces applies to new frames

[Robert J. Chassell]

    ... there is no `font' parameter in default-frame-alist ... 

That does not matter so long as the initialization file is called late.

The call

    emacs -q -l ~/.emacs-simple

fails because emacs/lisp/startup.el fails to process arguments on the
command line when they provided.

My ~/.emacs-simple invokes

    (setq initial-frame-alist
     '((font . "-Misc-Fixed-Medium-R-Normal--20-200-75-75-C-100-ISO8859-1")))

where
    xlsfonts -fn -Misc-Fixed-Medium-R-Normal--20-200-75-75-C-100-ISO8859-1
tells us
    -misc-fixed-medium-r-normal--20-200-75-75-c-100-iso8859-1
    -misc-fixed-medium-r-normal--20-200-75-75-c-100-iso8859-1
    -misc-fixed-medium-r-normal--20-200-75-75-c-100-iso8859-1
    -misc-fixed-medium-r-normal--20-200-75-75-c-100-iso8859-1
    -misc-fixed-medium-r-normal--20-200-75-75-c-100-iso8859-1
    -misc-fixed-medium-r-normal--20-200-75-75-c-100-iso8859-1

Today's GNU Emacs CVS, Fri, 2008 Jun 27  09:49 UTC
GNU Emacs 23.0.60.13 (i686-pc-linux-gnu, GTK+ Version 2.12.9)

(Incidently, lisp/startup.el says `process Emacs shell arguments' but
the only reference that I found in the Emacs manual for `Emacs shell'
was for an Emacs shell buffer.  However, the commentary for
lisp/startup.el says
  ;; This file parses the command line and gets Emacs running.
That sentence implies a non-Emacs shell that provides a command line.)

-- 
    Robert J. Chassell                          GnuPG Key ID: 004B4AC8
    bob@rattlesnake.com                         bob@gnu.org
    http://www.rattlesnake.com                  http://www.teak.cc

Re: Faces applies to new frames

[Robert J. Chassell]

        ... there is no `font' parameter in default-frame-alist ... 

    That does not matter so long as the initialization file is called late.

That may not be the case.  Both Stefan Monnier and Chong Yidong may be
right: we may need to write down every step and there may be more than
one bug.

When I call /usr/local/src/emacs/src/emacs -q -l ~/.emacs-simple on this
morning's CVS and my ~/.emacs-simple file has in it a 15 pixel
misc-fixed font:

    (setq initial-frame-alist
     '((font . "-misc-fixed-medium-r-normal--15-*-*-*-c-90-iso8859-1")))

    (setq default-frame-alist
     '((font . "-misc-fixed-medium-r-normal--15-*-*-*-c-90-iso8859-1")))

I see the fonts for two frames, but when I call the same command on a
~/.emacs-simple file that contains a 20 pixel misc-fixed font:

    (setq initial-frame-alist
     '((font . "-Misc-Fixed-Medium-R-Normal--20-200-75-75-C-100-ISO8859-1")))

    (setq default-frame-alist
     '((font . "-Misc-Fixed-Medium-R-Normal--20-200-75-75-C-100-ISO8859-1")))

the instance of Emacs reports

    frame-notice-user-settings: Font `-Misc-Fixed-Medium-R-Normal--20-200-75-75-C-100-ISO8859-1' is not defined

and I cannot produce the second frame because of an error with
x-create-frame-with-faces even though

    xlsfonts -fn -Misc-Fixed-Medium-R-Normal--20-200-75-75-C-100-ISO8859-1
tells us
    -misc-fixed-medium-r-normal--20-200-75-75-c-100-iso8859-1

The latter is both with a ~/.Xdefaults saying

    Emacs.FontBackend:	x,ftx

and with it saying nothing.

-- 
    Robert J. Chassell                          GnuPG Key ID: 004B4AC8
    bob@rattlesnake.com                         bob@gnu.org
    http://www.rattlesnake.com                  http://www.teak.cc

Re: Faces applies to new frames

[Chong Yidong]

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

> by default there is no `font' parameter in default-frame-alist, so as
> long as we leave it that way, it should not affect face settings.  The
> code in face-set-after-frame-default should apply the defface-spec to
> every new frame, so we shouldn't need any entry in default-frame-alist
> for it to apply to new frames.

There are several problems here, so let's take it one at a time.  First,
let's ignore Customize and try to make set-face-attribute work properly.

In this case, default faces aren't properly set for future frames.  The
problem lies in the interaction between face attributes and the `font'
frame parameter.  For historical reasons, we allow the attributes of the
default face to be changed by this frame parameter.  But that means that
when we create a new frame, the initial value of this frame parameter
needs to be set based on the value of the `default' face.  Otherwise, it
will override whatever attributes the `default' face has that were set
by set-face-attribute.

The attached patch does this.  It also removes the `font-parameter'
frame parameter, which is used to override the default face (a similar
patch will be needed for w32fns.c).  I think this is bogus.


*** trunk/lisp/faces.el.~1.416.~	2008-06-28 11:45:58.000000000 -0400
--- trunk/lisp/faces.el	2008-06-28 11:48:47.000000000 -0400
***************
*** 2017,2029 ****
    ;; 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)
  		       (scroll-bar-background scroll-bar :background)
  		       (mouse-color mouse :background)))
! 	apply-params)
      (dolist (param face-params)
        (let* ((value (frame-parameter frame (nth 0 param)))
  	     (face (nth 1 param))
--- 2017,2028 ----
    ;; Find attributes that should be initialized from frame parameters.
    (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)))
! 	apply-params default-font)
      (dolist (param face-params)
        (let* ((value (frame-parameter frame (nth 0 param)))
  	     (face (nth 1 param))
***************
*** 2056,2061 ****
--- 2055,2065 ----
  		(make-face-x-resource-internal face frame))
  	    (internal-merge-in-global-face face frame))
  	(error nil)))
+     ;; The face specs may specify a different default font.  Save this
+     ;; in the `font' frame parameter.
+     (setq default-font (face-font 'default))
+     (when default-font
+       (set-frame-parameter frame 'font default-font))
      ;; Apply the attributes specified by frame parameters.  This
      ;; rewrites parameters changed by make-face-x-resource-internal
      (dolist (param apply-params)
*** trunk/src/xfns.c.~1.720.~	2008-06-27 18:18:48.000000000 -0400
--- trunk/src/xfns.c	2008-06-28 11:50:55.000000000 -0400
***************
*** 204,210 ****
  Lisp_Object Qsuppress_icon;
  Lisp_Object Qundefined_color;
  Lisp_Object Qcompound_text, Qcancel_timer;
- static Lisp_Object Qfont_param;
  
  /* In dispnew.c */
  
--- 204,209 ----
***************
*** 3098,3109 ****
        if (NILP (font))
  	error ("No suitable font was found");
      }
-   else if (!NILP (font_param))
-     {
-       /* Remember the explicit font parameter, so we can re-apply it after
- 	 we've applied the `default' face settings.  */
-       x_set_frame_parameters (f, Fcons (Fcons (Qfont_param, font_param), Qnil));
-     }
    x_default_parameter (f, parms, Qfont, font, "font", "Font", RES_TYPE_STRING);
  }
  
--- 3097,3102 ----
***************
*** 5871,5878 ****
    staticpro (&Qcompound_text);
    Qcancel_timer = intern ("cancel-timer");
    staticpro (&Qcancel_timer);
-   Qfont_param = intern ("font-parameter");
-   staticpro (&Qfont_param);
    /* This is the end of symbol initialization.  */
  
    /* Text property `display' should be nonsticky by default.  */
--- 5864,5869 ----

Re: Faces applies to new frames

[Stefan Monnier]

> The attached patch does this.  It also removes the `font-parameter'
> frame parameter, which is used to override the default face (a similar
> patch will be needed for w32fns.c).  I think this is bogus.

I'm not sure I understand what your patch does (as mentioned already,
I think the current setup's interactions are too numerous and I don't
know them all, so I can't predict the effect of any particular change).

> +     ;; The face specs may specify a different default font.  Save this
> +     ;; in the `font' frame parameter.
> +     (setq default-font (face-font 'default))
> +     (when default-font
> +       (set-frame-parameter frame 'font default-font))

But doesn't that mean that (make-frame '((font . "foo-bar"))) will
disregard the explicit request for "foo-bar"?


        Stefan

Re: Faces applies to new frames

[Chong Yidong]

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

>> The attached patch does this.  It also removes the `font-parameter'
>> frame parameter, which is used to override the default face (a similar
>> patch will be needed for w32fns.c).  I think this is bogus.
>
> I'm not sure I understand what your patch does (as mentioned already,
> I think the current setup's interactions are too numerous and I don't
> know them all, so I can't predict the effect of any particular change).

When set-face-attribute changes the default for new frames, what it does
is to change the face vector for the `default' face entry in
face-new-frame-defaults.  When you create a new frame, the frame
parameter for that frame is whatever was specified using X resources or
the command line.  If you've changed face-new-frame-defaults in the
meantime, you get a mismatch between the two fonts.  What then happens
is that in the last step of face-new-frame-defaults, the frame parameter
inadvertently gets applied to the default face, wiping out the effects
of face-new-frame-defaults.

>> +     ;; The face specs may specify a different default font.  Save this
>> +     ;; in the `font' frame parameter.
>> +     (setq default-font (face-font 'default))
>> +     (when default-font
>> +       (set-frame-parameter frame 'font default-font))
>
> But doesn't that mean that (make-frame '((font . "foo-bar"))) will
> disregard the explicit request for "foo-bar"?

True.  I think that in this case, x-create-frame-with-faces should take
note that it is passed a `font' frame parameter, and tell
face-set-after-frame-default to stick to that instead of imposing
whatever the `default' face says.

Re: Faces applies to new frames

[Stefan Monnier]

>>> +     ;; The face specs may specify a different default font.  Save this
>>> +     ;; in the `font' frame parameter.
>>> +     (setq default-font (face-font 'default))
>>> +     (when default-font
>>> +       (set-frame-parameter frame 'font default-font))
>> 
>> But doesn't that mean that (make-frame '((font . "foo-bar"))) will
>> disregard the explicit request for "foo-bar"?

> True.  I think that in this case, x-create-frame-with-faces should take
> note that it is passed a `font' frame parameter, and tell
> face-set-after-frame-default to stick to that instead of imposing
> whatever the `default' face says.

And that's exactly the reason why I introduced `font-parameter'.
Basically, I think that frame parameters should generally take
precedence since they may really be set on a frame-by-frame basis.
We can try and distinguish parameters coming from default-frame-alist
from those coming from elsewhere (pop-up-frame-alist,
minibuffer-frame-alist, ...), but it seems a simpler solution is to not
try and distinguish between the provenance and simply blindly give
priority to frame parameters.

This implies that set-face-attribute should probably store global
settings elsewhere than on default-frame-alist.  It sounds like
face-new-frame-defaults would be a good place for that instead (th0o
this is only based on its name)

Here's my current understanding of the various places where we store
face info:

- face-new-frame-defaults, should be global and seems to only ever
  contain "unspecified".  I.e. pretty much unused.
- defface.  Can go from global to terminal-specific.
  Usually terminal-specific.
- Xresources.  Can go from terminal-specific to frame-specific.
  Usually terminal-specific.
- default-frame-alist and other frame parameters.  Can go from global to
  frame-specific.

Are there any more?
I've sorted them based on their locality, so if we follow the principle
that more-local should take precedence over more global settings, they
should be taken in this order.  And indeed, I believe it's the order in
which they're applied right now.

Based on the above, we then have to figure out how set-face-attribute
(when applied globally) should affect the above settings, depending on
the effect we're looking for.

Here's a suggestion:
1 - rule out frame-specific Xresources settings
2 - make face-new-frame-defaults terminal-local, so it's initialized at
    terminal-creation time by applying defface and Xresources settings.
3 - rule that "global" set-face-attribute settings only apply to future
    frames on the same terminals (i.e. they're not truly global any
    more), so it can work by modifying the terminal's
    face-new-frame-defaults and it will hence take precedence over
    customize (i.e. defface) and Xresources settings.

This will also have the advantage that the only face-initialization we
need to do at frame-creation time is to apply the few face-relevant
settings from the frame parameters.  I.e. It will speed up frame creation
significantly when you have many faces.


        Stefan

Re: Faces applies to new frames

[Miles Bader]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> And that's exactly the reason why I introduced `font-parameter'.
> Basically, I think that frame parameters should generally take
> precedence since they may really be set on a frame-by-frame basis.

Eh?  face attributes can be set on a frame-by-frame basis too...

-Miles

-- 
Non-combatant, n. A dead Quaker.

Re: Faces applies to new frames

[Stefan Monnier]

>> And that's exactly the reason why I introduced `font-parameter'.
>> Basically, I think that frame parameters should generally take
>> precedence since they may really be set on a frame-by-frame basis.

> Eh?  face attributes can be set on a frame-by-frame basis too...

Sorry, I tend to leave this out indeed, but AFAIK this is never
a problem: we're mostly talking here about settings saved somewhere so
they're applied at frame creation.  `set-face-attribute' applied to
a single frame happen afterwards and don't need to save anything
anywhere, so they "just work" (tho they may be incorrectly overridden
later on by a defface spec if Custom is asked to reapply a particular
defface spec,. typically becaquse that spec was changed).


        Stefan

Re: Faces applies to new frames

[Miles Bader]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> And that's exactly the reason why I introduced `font-parameter'.
>>> Basically, I think that frame parameters should generally take
>>> precedence since they may really be set on a frame-by-frame basis.
>
>> Eh?  face attributes can be set on a frame-by-frame basis too...
>
> Sorry, I tend to leave this out indeed, but AFAIK this is never
> a problem: we're mostly talking here about settings saved somewhere so
> they're applied at frame creation.  `set-face-attribute' applied to
> a single frame happen afterwards and don't need to save anything
> anywhere, so they "just work" (tho they may be incorrectly overridden
> later on by a defface spec if Custom is asked to reapply a particular
> defface spec,. typically becaquse that spec was changed).

The thing is, we've got like 10 different ways of specifying stuff,
_all_ of which seem to be consulted at frame-creation time, and which
seem in many cases pretty redundant.

Maybe the facespec stuff is necessary (mainly becuase facespecs can
encode frame-type specific conditional settings), but face-attributes
and frame-paramter seem to cover roughly the same ground (with
frame-attributes being generally nicer and more functiona AFAICS).  Are
frame-parameter settings ever actually necessary, other than for
historical compatibility with emacs 19/20?

-Miles

-- 
Opposition, n. In politics the party that prevents the Goverment from running
amok by hamstringing it.

Re: Faces applies to new frames

[Chong Yidong]

Miles Bader <miles@gnu.org> writes:

> Maybe the facespec stuff is necessary (mainly becuase facespecs can
> encode frame-type specific conditional settings), but face-attributes
> and frame-paramter seem to cover roughly the same ground (with
> frame-attributes being generally nicer and more functiona AFAICS).  Are
> frame-parameter settings ever actually necessary, other than for
> historical compatibility with emacs 19/20?

They are mainly a pain in the butt, IMO.

Re: Faces applies to new frames

[David Kastrup]

Chong Yidong <cyd@stupidchicken.com> writes:

> Miles Bader <miles@gnu.org> writes:
>
>> Maybe the facespec stuff is necessary (mainly becuase facespecs can
>> encode frame-type specific conditional settings), but face-attributes
>> and frame-paramter seem to cover roughly the same ground (with
>> frame-attributes being generally nicer and more functiona AFAICS).  Are
>> frame-parameter settings ever actually necessary, other than for
>> historical compatibility with emacs 19/20?
>
> They are mainly a pain in the butt, IMO.

If somebody can think of a way of streamlining the mechanism to a sane
single interface, the time to do that would be now, before the feature
freeze.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Faces applies to new frames

[Stefan Monnier]

> Maybe the facespec stuff is necessary (mainly becuase facespecs can
> encode frame-type specific conditional settings), but face-attributes
> and frame-paramter seem to cover roughly the same ground (with
> frame-attributes being generally nicer and more functiona AFAICS).  Are
  ^^^^^
  face

> frame-parameter settings ever actually necessary, other than for
> historical compatibility with emacs 19/20?

AFAIK, the main situation where setting face attributes via
frame-parameters is useful/used is for things like
minibuffer-frame-alist (where I use it to give a special font and
background to my minibuffer-only frame(s)), and special-display-regexps.


-- Stefan

Re: Faces applies to new frames

[Richard M Stallman]

I think we need to change the way faces are specified,
and use something that is (1) simpler and (2) predictable.
I've included a proposal below.

    The trouble is that the `font' frame parameter interferes with the
    normal semantics of Lisp faces.

The `font' frame parameter and face attribute were set up so that
people accustomed to using X could specify fonts using the usual
X font specification syntax.  But they make things more complex.

I suggest eliminating the `font' parameter frame and the `font' face
attribute, and replacing them with a function to set up a face
based on a font name.  That function will serve the purpose, and it
won't cause any complications.

    AFAIK, the main situation where setting face attributes via
    frame-parameters is useful/used is for things like
    minibuffer-frame-alist (where I use it to give a special font and
    background to my minibuffer-only frame(s)), and special-display-regexps.

Would buffer-based interfaces be good for these jobs?
They might be better in every way.

Perhaps we should eliminate the per-frame face attributes, and have
only per-buffer ways to alter faces from the global specifications.

One benefit of that is that it would avoid the uncertainty about
precedence, which we have now when both users and defaults can specify
face attributes at various levels.  It would be clear and natural that
any per-buffer face specification overrides the global ones,
regardless of who set either one of them.

In this design, we would have defface, and the custom settings
(perhaps themed), and the program-specified face specs that you can
set with `face-spec-set', plus per-buffer overrides of various kinds.
That's all -- there would be no other way of controlling face
appearance.

This would mean changing `set-face-attribute' to reject a nontrivial
FRAME argument, and to work by calling `face-spec-set'.

The deprecated and apparently unused way of calling `face-spec-set'
that involves specifying a FRAME would be desupported.

It would be necessary to design a new implementation for the existing
feature that you can write X resources to control face appearance.
The feature of frame-name-specific X resources could be eliminated.

xfaces.c should be totally rewritten.  The only cause of face
variation between frames would be in the different kinds of terminals.
So it would make sense to realize each face only once per terminal.

Re: Faces applies to new frames

[Chong Yidong]

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

> Perhaps we should eliminate the per-frame face attributes, and have
> only per-buffer ways to alter faces from the global specifications.
>
> In this design, we would have defface, and the custom settings
> (perhaps themed), and the program-specified face specs that you can
> set with `face-spec-set', plus per-buffer overrides of various kinds.
> That's all -- there would be no other way of controlling face
> appearance.

I think this is a good idea.  Now that we have per-buffer
face-remapping, per-frame faces probably won't be missed.

I don't think we should make this change without at least warning users
at least one Emacs version in advance, though.  For Emacs 23.1, maybe we
could add a note to NEWS and the manuals saying that the various frame
attributes that affect the default face will become obsolete in a future
version.  Maybe set-frame-parameter could also issue a warning.

> xfaces.c should be totally rewritten.  The only cause of face
> variation between frames would be in the different kinds of terminals.
> So it would make sense to realize each face only once per terminal.

Actually, I don't think the required changes to xfaces.c would be
extensive.  Basically, this involves removing the face_alist member of
`struct frame' and assigning it to the terminal instead.

Re: Faces applies to new frames

[Richard M Stallman]

    I don't think we should make this change without at least warning users
    at least one Emacs version in advance, though.  For Emacs 23.1, maybe we
    could add a note to NEWS and the manuals saying that the various frame
    attributes that affect the default face will become obsolete in a future
    version.  Maybe set-frame-parameter could also issue a warning.

Alternatively, you could ask the users who actually needs these
features and for what, and how ready they are to adapt to the proposed
change.  Based on the answers you could see if there is a need for
that transition period.

Re: Faces applies to new frames

[Stefan Monnier]

> I think we need to change the way faces are specified,
> and use something that is (1) simpler and (2) predictable.

No argument.

>     AFAIK, the main situation where setting face attributes via
>     frame-parameters is useful/used is for things like
>     minibuffer-frame-alist (where I use it to give a special font and
>     background to my minibuffer-only frame(s)), and special-display-regexps.

> Would buffer-based interfaces be good for these jobs?
> They might be better in every way.

Could be, but currently we don't have any easy way to do it within
the buffer.  Especially not for minibuffer-only frames which may show
various buffers, all of them internal.  But yes, it's quite feasible.

> Perhaps we should eliminate the per-frame face attributes, and have
> only per-buffer ways to alter faces from the global specifications.

I'd tend to agree, although of course, having had per-frame faces for so
long has encouraged people to find uses for per-frame faces, some of
which may require a bit of work to port to per-buffer faces.

E.g. I use a dedicated frame for my MPC.el (a front-end to the MPD
daemon, see www.musicpd.org), but that frames contains many different
windows showing different buffer.  I currently set the frame to use
a proportional font (helvetica-like) because it uses a bit less space
and I find it somewhat nicer to look at, and none of those buffers
suffer from the usual alignment/filling problems we encounter with
proportional fonts in Emacs.  Making the setting per-buffer is possible,
but requires changing the face individually in each buffer, which is
less satisfactory.  Not a strong argument in favor of per-frame faces,
just a data point.

> One benefit of that is that it would avoid the uncertainty about
> precedence, which we have now when both users and defaults can specify
> face attributes at various levels.  It would be clear and natural that
> any per-buffer face specification overrides the global ones,
> regardless of who set either one of them.

The mere fact of changing from per-frame to per-buffer won't solve the
underlying precedence problem.  But if we get rid of settings coming
from frame parameters and Xresources, then yes, things become simpler
(both for per-buffer and for per-frame faces).


        Stefan

Re: Faces applies to new frames

[Richard M Stallman]

    Could be, but currently we don't have any easy way to do it within
    the buffer.  Especially not for minibuffer-only frames which may show
    various buffers, all of them internal.  But yes, it's quite feasible.

The minibuffer window only displays minibuffers.  It would be easy to
set up a face replacement for all the minibuffers.

    The mere fact of changing from per-frame to per-buffer won't solve the
    underlying precedence problem.

It will, in an indirect way.

WIth per-frame face attributes and global face attributes,
there is a tension about which should get precedence.
Suppose the user sets a global face attribute: should that
set the per-frame attribute of all frames that exist?
That is what it does.  But what if a program explicitly set
the per-frame attribute of one frame?  That will be overridden,
and maybe that's wrong.

Now suppose that there are global face attributes and per-buffer
overrides.  There is no such problem any more.  You set the global
face attributes, and that doesn't set anything else that is user-visible.
Any buffer-specific overrides remain in existence and continue to override.
It is totally clear what these things should do.

Really, this is the consequence of replacing a per-X value
with a per-X override.  If we replace per-frame face attributes
with per-frame overrides, we would get this same benefit.
But that change would be messy in other ways.

      But if we get rid of settings coming
    from frame parameters and Xresources,

A clarification.  I don't propose to entirely eliminate the use of X
resources to specify faces.  I just propose to eliminate doing so
in a frame-specific way.

      I currently set the frame to use
    a proportional font (helvetica-like) because it uses a bit less space
    and I find it somewhat nicer to look at, and none of those buffers
    suffer from the usual alignment/filling problems we encounter with
    proportional fonts in Emacs.  Making the setting per-buffer is possible,
    but requires changing the face individually in each buffer, which is
    less satisfactory.

Do these buffers use a particular mode, whose mode hook could set up
the per-buffer override?

Re: Faces applies to new frames

[Chong Yidong]

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

> Basically, I think that frame parameters should generally take
> precedence since they may really be set on a frame-by-frame basis.

The trouble is that the `font' frame parameter interferes with the
normal semantics of Lisp faces.  It should always be defined for each
frame, so if it always takes precedence, there would be no good way to
make changes to the default face apply to new frames, because any change
would be overriden by `font'.  And, I don't think it's easy to calculate
what `font' ought to be ahead of time.  That's why I suggested setting
`font' in face-set-after-frame-default, after processing the defface
spec.

By the way, a more limited change than what I suggested earlier seems to
DTRT: with this, calling make-frame with a `font' frame parameter seems
to DTRT, as do other tests of set-face-attribute I tried out.

*** trunk/lisp/faces.el.~1.416.~	2008-06-28 22:14:35.000000000 -0400
--- trunk/lisp/faces.el	2008-06-29 00:13:28.000000000 -0400
***************
*** 2056,2061 ****
--- 2056,2065 ----
  		(make-face-x-resource-internal face frame))
  	    (internal-merge-in-global-face face frame))
  	(error nil)))
+     ;; The face specs may specify a different default font.  Save this
+     ;; in the `font' frame parameter.
+     (when (face-font 'default)
+       (set-frame-parameter frame 'font (face-font 'default)))
      ;; Apply the attributes specified by frame parameters.  This
      ;; rewrites parameters changed by make-face-x-resource-internal
      (dolist (param apply-params)

> This implies that set-face-attribute should probably store global
> settings elsewhere than on default-frame-alist.  It sounds like
> face-new-frame-defaults would be a good place for that instead (th0o
> this is only based on its name)

That's currently the case.

> Here's my current understanding of the various places where we store
> face info:
>
> - face-new-frame-defaults, should be global and seems to only ever
>   contain "unspecified".  I.e. pretty much unused.

face-new-frame-defaults is where set-face-attribute stores changes that
are supposed to be applied to faces on furture frames.  So if you do

  (set-face-attribute 'default nil :slant 'italic)

Then you'll see that the `default' entry in face-new-frame-defaults
contains `italic' in its slant attribute.

> 1 - rule out frame-specific Xresources settings
> 2 - make face-new-frame-defaults terminal-local, so it's initialized at
>     terminal-creation time by applying defface and Xresources settings.
> 3 - rule that "global" set-face-attribute settings only apply to future
>     frames on the same terminals (i.e. they're not truly global any
>     more), so it can work by modifying the terminal's
>     face-new-frame-defaults and it will hence take precedence over
>     customize (i.e. defface) and Xresources settings.

I don't see how this makes sense.  As long as each frame is allowed to
have its own face settings, the current dance in face-new-frame-defaults
will be necessary each time we create a new frace.

Re: Faces applies to new frames

[Stefan Monnier]

> The trouble is that the `font' frame parameter interferes with the
> normal semantics of Lisp faces.  It should always be defined for each
> frame,

That's why I introduced font-parameter: it stores the setting specified
by the user (can be nil) rather than the actual font used for the frame
(never nil, always fully-specified, AFAIK).

> so if it always takes precedence, there would be no good way to
> make changes to the default face apply to new frames, because any change
> would be overriden by `font'.  And, I don't think it's easy to calculate
> what `font' ought to be ahead of time.  That's why I suggested setting
> `font' in face-set-after-frame-default, after processing the defface
> spec.

> By the way, a more limited change than what I suggested earlier seems to
> DTRT: with this, calling make-frame with a `font' frame parameter seems
> to DTRT, as do other tests of set-face-attribute I tried out.

> *** trunk/lisp/faces.el.~1.416.~	2008-06-28 22:14:35.000000000 -0400
> --- trunk/lisp/faces.el	2008-06-29 00:13:28.000000000 -0400
> ***************
> *** 2056,2061 ****
> --- 2056,2065 ----
>   		(make-face-x-resource-internal face frame))
>   	    (internal-merge-in-global-face face frame))
>   	(error nil)))
> +     ;; The face specs may specify a different default font.  Save this
> +     ;; in the `font' frame parameter.
> +     (when (face-font 'default)
> +       (set-frame-parameter frame 'font (face-font 'default)))

That can't be right.  What you're doing here is copying the
selected-frame's default font to `frame'.  Why should we rely on the
selected frame?

>> This implies that set-face-attribute should probably store global
>> settings elsewhere than on default-frame-alist.  It sounds like
>> face-new-frame-defaults would be a good place for that instead (th0o
>> this is only based on its name)
> That's currently the case.

Oh, yes, I see it now.  Good.

>> Here's my current understanding of the various places where we store
>> face info:
>> 
>> - face-new-frame-defaults, should be global and seems to only ever
>> contain "unspecified".  I.e. pretty much unused.

> face-new-frame-defaults is where set-face-attribute stores changes that
> are supposed to be applied to faces on furture frames.  So if you do

>   (set-face-attribute 'default nil :slant 'italic)

> Then you'll see that the `default' entry in face-new-frame-defaults
> contains `italic' in its slant attribute.

Yes, I now see it, and found the code that does it.

>> 1 - rule out frame-specific Xresources settings
>> 2 - make face-new-frame-defaults terminal-local, so it's initialized at
>> terminal-creation time by applying defface and Xresources settings.
>> 3 - rule that "global" set-face-attribute settings only apply to future
>> frames on the same terminals (i.e. they're not truly global any
>> more), so it can work by modifying the terminal's
>> face-new-frame-defaults and it will hence take precedence over
>> customize (i.e. defface) and Xresources settings.

> I don't see how this makes sense.  As long as each frame is allowed to
> have its own face settings, the current dance in face-new-frame-defaults
> will be necessary each time we create a new frace.

Why?  Here's the dance, as I understand it:

  (unless inhibit-face-set-after-frame-default
    (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))))

No idea what this is about, really.  It appears to read some of the
settings for the `default' face from face-new-frame-defaults and then
applies them to the `default' face on the current frame.

This seems both useless and dubious: face-new-frame-defaults is
presumably obeyed elsewhere already, and why should we only play with
the `font' part of the `default' face?

  ;; 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)
		       (scroll-bar-background scroll-bar :background)
		       (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))))

Extract face info from the frame parameters.  It's not applied yet,
because we want those frame parameter settings to take precedence.

    ;; 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)

Apply the defface specs.

	    (if (memq (window-system frame) '(x w32 mac))
		(make-face-x-resource-internal face frame))

Apply the Xresources specs.

	    (internal-merge-in-global-face face frame))

Apply the face-new-frame-defaults.  Note that although global, they take
precedence over defface and Xresources.

	(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))))

Finally, apply the face info extracted from the frame-parameters.

So, ignoring the first part, we do:

- apply defface
- apply Xresources
- apply face-new-frame-defaults
- apply frame-parameters

The first 3 will always return the exact same result for every frame on
the same terminal (at least, if we ignore frame-specific Xresource
settings).

So I don't see why we can't just precompute this result at
terminal-creation time and store it directly in face-new-frame-defaults
(after making this variable terminal-local).


        Stefan

Re: Faces applies to new frames

[Chong Yidong]

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

>> +     ;; The face specs may specify a different default font.  Save this
>> +     ;; in the `font' frame parameter.
>> +     (when (face-font 'default)
>> +       (set-frame-parameter frame 'font (face-font 'default)))
>
> That can't be right.  What you're doing here is copying the
> selected-frame's default font to `frame'.  Why should we rely on the
> selected frame?

Typo.  I meant to use the font attribute of the default face in the new
frame:

*** trunk/lisp/faces.el.~1.416.~	2008-06-28 22:14:35.000000000 -0400
--- trunk/lisp/faces.el	2008-06-29 11:11:35.000000000 -0400
***************
*** 2056,2061 ****
--- 2056,2065 ----
  		(make-face-x-resource-internal face frame))
  	    (internal-merge-in-global-face face frame))
  	(error nil)))
+     ;; The face specs may specify a different default font.  Save this
+     ;; in the `font' frame parameter.
+     (when (face-font 'default frame)
+       (set-frame-parameter frame 'font (face-font 'default frame)))
      ;; Apply the attributes specified by frame parameters.  This
      ;; rewrites parameters changed by make-face-x-resource-internal
      (dolist (param apply-params)

The point is that the default face in the new frame specifies a font
(based on its family, slant, and weight attributes) that may differ from
the `font' frame parameter.  In that case, it's the former we should
use.  The call to face-font returns a font object consistent with the
attributes of the default face.  Any font-parameter supplied by the user
further overrides this.  Make sense?

> It appears to read some of the settings for the `default' face from
> face-new-frame-defaults and then applies them to the `default' face on
> the current frame.  This seems both useless and dubious:
> face-new-frame-defaults is presumably obeyed elsewhere already, and
> why should we only play with the `font' part of the `default' face?

Not sure.  It's possible that some of the operations in
face-new-frame-defaults need some information in the default face,
e.g. to figure out column widths, even if that information is later
revised.

> - apply defface
> - apply Xresources
> - apply face-new-frame-defaults
> - apply frame-parameters
>
> The first 3 will always return the exact same result for every frame on
> the same terminal (at least, if we ignore frame-specific Xresource
> settings).
>
> So I don't see why we can't just precompute this result at
> terminal-creation time and store it directly in face-new-frame-defaults
> (after making this variable terminal-local).

Now I see your point.

The problem with this proposal is that if Lisp code changes the defface
spec, we would need to update this value by recomputing X resources,
face-new-frame-defaults, etc etc.  Since we allow Lisp code to access
defface specs as Lisp lists instead of using accessor functions, I don't
see how to handle this.

Re: Faces applies to new frames

[Stefan Monnier]

> Typo.  I meant to use the font attribute of the default face in the new
> frame:

> *** trunk/lisp/faces.el.~1.416.~	2008-06-28 22:14:35.000000000 -0400
> --- trunk/lisp/faces.el	2008-06-29 11:11:35.000000000 -0400
> ***************
> *** 2056,2061 ****
> --- 2056,2065 ----
>   		(make-face-x-resource-internal face frame))
>   	    (internal-merge-in-global-face face frame))
>   	(error nil)))
> +     ;; The face specs may specify a different default font.  Save this
> +     ;; in the `font' frame parameter.
> +     (when (face-font 'default frame)
> +       (set-frame-parameter frame 'font (face-font 'default frame)))
>       ;; Apply the attributes specified by frame parameters.  This
>       ;; rewrites parameters changed by make-face-x-resource-internal
>       (dolist (param apply-params)

> The point is that the default face in the new frame specifies a font
> (based on its family, slant, and weight attributes) that may differ from
> the `font' frame parameter.  In that case, it's the former we should
> use.  The call to face-font returns a font object consistent with the
> attributes of the default face.  Any font-parameter supplied by the user
> further overrides this.  Make sense?

Yes, it makes a bit more sense, except that now I wonder why this
code would be needed at all?  Isn't the `font' frame-parameter
automatically always kept up-to-date with the font of the
`default' face?  If not, I think it should, and hence it should be done
elsewhere so that it's also true when we later change the default face
via `set-face-attributes' or something like that.

>> It appears to read some of the settings for the `default' face from
>> face-new-frame-defaults and then applies them to the `default' face on
>> the current frame.  This seems both useless and dubious:
>> face-new-frame-defaults is presumably obeyed elsewhere already, and
>> why should we only play with the `font' part of the `default' face?

> Not sure.  It's possible that some of the operations in
> face-new-frame-defaults need some information in the default face,
> e.g. to figure out column widths, even if that information is later
> revised.

I do not follow.  AFAIK, there are no operations in
face-new-frame-defaults, and I have no idea what column width come
into play.  Can you give a concrete example, maybe?

>> - apply defface
>> - apply Xresources
>> - apply face-new-frame-defaults
>> - apply frame-parameters
>> 
>> The first 3 will always return the exact same result for every frame on
>> the same terminal (at least, if we ignore frame-specific Xresource
>> settings).
>> 
>> So I don't see why we can't just precompute this result at
>> terminal-creation time and store it directly in face-new-frame-defaults
>> (after making this variable terminal-local).

> Now I see your point.

> The problem with this proposal is that if Lisp code changes the defface
> spec, we would need to update this value by recomputing X resources,
> face-new-frame-defaults, etc etc.  Since we allow Lisp code to access
> defface specs as Lisp lists instead of using accessor functions, I don't
> see how to handle this.

I don't understand how that's relevant: the same problem exists
already today, doesn't it?


        Stefan

Re: Faces applies to new frames

[Chong Yidong]

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

> Yes, it makes a bit more sense, except that now I wonder why this
> code would be needed at all?  Isn't the `font' frame-parameter
> automatically always kept up-to-date with the font of the
> `default' face?  If not, I think it should, and hence it should be done
> elsewhere so that it's also true when we later change the default face
> via `set-face-attributes' or something like that.

This is because internal-merge-in-global-face (which is called in the
middle of face-set-after-frame-default) merges in the face vector from
face-new-frame-defaults without updating the `font' frame parameter.

Every other place that changes the font uses set-face-attribute, which
updates the `font' parameter correctly.

I suppose we could change internal-merge-in-global-face to update the
frame parameters itself, instead of adding a separate line in faces.el
to do it.

Re: Faces applies to new frames

[Stefan Monnier]

>> Yes, it makes a bit more sense, except that now I wonder why this
>> code would be needed at all?  Isn't the `font' frame-parameter
>> automatically always kept up-to-date with the font of the
>> `default' face?  If not, I think it should, and hence it should be done
>> elsewhere so that it's also true when we later change the default face
>> via `set-face-attributes' or something like that.

> This is because internal-merge-in-global-face (which is called in the
> middle of face-set-after-frame-default) merges in the face vector from
> face-new-frame-defaults without updating the `font' frame parameter.

> Every other place that changes the font uses set-face-attribute, which
> updates the `font' parameter correctly.

> I suppose we could change internal-merge-in-global-face to update the
> frame parameters itself, instead of adding a separate line in faces.el
> to do it.

Yes, that would make a lot more sense, wouldn't it?
[ Tho, I'd have expected that updating the `font' frame-parameter was
  done at a lower-level, in a single place. ]


        Stefan

Re: Faces applies to new frames

[Chong Yidong]

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

>> I suppose we could change internal-merge-in-global-face to update the
>> frame parameters itself, instead of adding a separate line in faces.el
>> to do it.
>
> Yes, that would make a lot more sense, wouldn't it?

Only a little: internal-merge-in-global-face is only used in
face-set-after-frame-default, and it's more concise to update the frame
in Lisp in face-set-after-frame-defaultis than changing
internal-merge-in-global-face.  But OK.

> [ Tho, I'd have expected that updating the `font' frame-parameter was
>   done at a lower-level, in a single place. ]

Well, face vectors are Lisp-accessible data structures, and there's no
practical generic way to catch changes to these data structures.

Re: Faces applies to new frames

[Stefan Monnier]

> Only a little: internal-merge-in-global-face is only used in
> face-set-after-frame-default, and it's more concise to update the frame
> in Lisp in face-set-after-frame-defaultis than changing
> internal-merge-in-global-face.  But OK.

I'd expect that for most users (at least that's the case for me), 99% of
the faces are not changed by the call to internal-merge-in-global-face,
so it seems fair to let internal-merge-in-global-face do the update, so
it can optimize it away when no change were performed.

>> [ Tho, I'd have expected that updating the `font' frame-parameter was
>> done at a lower-level, in a single place. ]

> Well, face vectors are Lisp-accessible data structures, and there's no
> practical generic way to catch changes to these data structures.

That doesn't answer my point, which is that this problem you describe has
been with us for ever, and so it's irrelevant to the question, AFAICT.
Or are you saying that my proposal would make it somehow worse?


        Stefan

Re: Faces applies to new frames

[Chong Yidong]

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

>>> [ Tho, I'd have expected that updating the `font' frame-parameter was
>>> done at a lower-level, in a single place. ]
>
>> Well, face vectors are Lisp-accessible data structures, and there's no
>> practical generic way to catch changes to these data structures.
>
> That doesn't answer my point, which is that this problem you describe has
> been with us for ever, and so it's irrelevant to the question, AFAICT.
> Or are you saying that my proposal would make it somehow worse?

I wasn't referring to your proposal when I wrote the above, but I still
don't fully understand what your proposal entails.

From what I can tell, you want something like an alist that collects the
face settings to apply to new faces, based on defface specs, X
resources, and face-new-frame-defaults.  It seems that the main goal of
this is to avoid recomputing faces each time we create a frame.  But if
the defface specs are user-accessible Lisp structures, there's no way
for us to know that they haven't been changed since the last time the
alist was recomputed.  So we would have to recompute each time a new
frame is created, anyway.  No?

Re: Faces applies to new frames

[Stefan Monnier]

> From what I can tell, you want something like an alist that collects the
> face settings to apply to new faces, based on defface specs, X
> resources, and face-new-frame-defaults.  It seems that the main goal of
> this is to avoid recomputing faces each time we create a frame.  But if
> the defface specs are user-accessible Lisp structures, there's no way
> for us to know that they haven't been changed since the last time the
> alist was recomputed.  So we would have to recompute each time a new
> frame is created, anyway.  No?

I don't think so: presumably the code that changed the defface specs
will want to apply this change to existing frames, so wherever that is
done, we can update face-new-frame-defaults correspondingly.


        Stefan

Re: Faces applies to new frames

[Chong Yidong]

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

> I don't think so: presumably the code that changed the defface specs
> will want to apply this change to existing frames, so wherever that is
> done, we can update face-new-frame-defaults correspondingly.

Okay, now I understand: you want to treat the defface spec as an
internal variable, so that if external Lisp code changes it, it bears
responsibility for updating face-new-frame-defaults?

Re: Faces applies to new frames

[Richard M Stallman]

We could eliminate the `font' frame parameter now, in the way I suggested,
even if we do not eliminate frame-specific face attributes now.

Re: Faces applies to new frames

[Chong Yidong]

Regarding your recent change to face-set-after-frame-default: in the
loop that initializes faces from face specs and X resources, I believe
the reason this excluded the `default' face is that the `default' face's
attributes are supposed to be applied right at the beginning of
face-set-after-frame-default:

  (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))
     ....

Thus, the function is simply trying to avoid doing the work twice.

The question is, why does this reasoning break down, leading to the bugs
that you supposedly fixed with your change?

Re: Faces applies to new frames

[Stefan Monnier]

> Regarding your recent change to face-set-after-frame-default: in the
> loop that initializes faces from face specs and X resources, I believe
> the reason this excluded the `default' face is that the `default' face's
> attributes are supposed to be applied right at the beginning of
> face-set-after-frame-default:

>   (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))
>      ....

I must say I don't know what the above is trying to do.

> Thus, the function is simply trying to avoid doing the work twice.

Not quite: the second part applies defface settings, whereas the
first part applies "global face-attributes" (don't know why they
weren't set already).


        Stefan