Change in face-spec-set

Change in face-spec-set

[Richard Stallman]

I wrote the following patch to fix one of the bugs in `face-spec-set'
which was introduced by my first change there.  Other changes have
been made subsequently, and I don't know whether this patch is still
needed, or whether any problems remain there.  Can someone tell me
the situation?

diff -c -c -r1.381 faces.el
*** faces.el	18 Oct 2007 19:02:22 -0000	1.381
--- faces.el	19 Oct 2007 17:25:59 -0000
***************
*** 1472,1477 ****
--- 1472,1483 ----
      ;; When we reset the face based on its spec, then it is unmodified
      ;; as far as Custom is concerned.
      (put (or (get face 'face-alias) face) 'face-modified nil)
+     ;; Clear all the new-frame defaults for this face.
+     ;; face-spec-reset-face won't do it right.
+     (let ((facevec (cdr (assq face face-new-frame-defaults))))
+       (dotimes (i (length facevec))
+ 	(unless (= i 0)
+ 	  (aset facevec i 'unspecified))))
      ;; Set each frame according to the rules implied by SPEC.
      (dolist (frame (frame-list))
        (face-spec-set face spec frame))))

Re: Change in face-spec-set

[Glenn Morris]

Richard Stallman wrote:

> I wrote the following patch to fix one of the bugs in `face-spec-set'
> which was introduced by my first change there.  Other changes have
> been made subsequently, and I don't know whether this patch is still
> needed, or whether any problems remain there.  Can someone tell me
> the situation?

The only change to face-spec-set is mine, to set the default for new
frames when FRAME is nil. If I apply your patch, this is once again
broken. By design it seems ("Clear all the new-frame defaults for this
face"). Why do you want to do this?

Re: Change in face-spec-set

[Glenn Morris]

Glenn Morris wrote:

> Richard Stallman wrote:
>
>> I wrote the following patch to fix one of the bugs in `face-spec-set'
>> which was introduced by my first change there.  Other changes have
>> been made subsequently, and I don't know whether this patch is still
>> needed, or whether any problems remain there.  Can someone tell me
>> the situation?
>
> The only change to face-spec-set is mine, to set the default for new
> frames when FRAME is nil. If I apply your patch, this is once again
> broken.

Changing your patch to look like this seems better, if you think there
is some problem in face-spec-reset-face. Is fixing face-spec-reset-face 
not an option?


*** faces.el	18 Oct 2007 19:02:22 -0000	1.381
--- faces.el	19 Oct 2007 21:11:31 -0000
***************
*** 1449,1455 ****
  See `defface' for information about SPEC.  If SPEC is nil, do nothing."
    (let ((attrs (face-spec-choose spec frame)))
      (when spec
!       (face-spec-reset-face face (or frame t)))
      (while attrs
        (let ((attribute (car attrs))
  	    (value (car (cdr attrs))))
--- 1449,1462 ----
  See `defface' for information about SPEC.  If SPEC is nil, do nothing."
    (let ((attrs (face-spec-choose spec frame)))
      (when spec
!       (if frame
!           (face-spec-reset-face face frame)
!         ;; Clear all the new-frame defaults for this face.
!         ;; face-spec-reset-face won't do it right.
!         (let ((facevec (cdr (assq face face-new-frame-defaults))))
!           (dotimes (i (length facevec))
!             (unless (= i 0)
!               (aset facevec i 'unspecified))))))
      (while attrs
        (let ((attribute (car attrs))
  	    (value (car (cdr attrs))))

Re: Change in face-spec-set

[Johan Bockgård]

Glenn Morris <rgm@gnu.org> writes:

> Changing your patch to look like this seems better, if you think there
> is some problem in face-spec-reset-face. Is fixing
> face-spec-reset-face not an option?

The effect of face-spec-reset-face on the new-frame default for a face
is not to "set if from zero". It produces an entry like this in
`face-new-frame-defaults'

    (FACE . [face :ignore-defface :ignore-defface ...])

But we need `unspecified', not :ignore-defface. (`unspecified' means "no
global definition", whereas :ignore-defface means "the global definition
is `unspecified'").

(http://lists.gnu.org/archive/html/emacs-pretest-bug/2005-10/msg00208.html)


> *** faces.el	18 Oct 2007 19:02:22 -0000	1.381
> --- faces.el	19 Oct 2007 21:11:31 -0000
> ***************
> *** 1449,1455 ****
>   See `defface' for information about SPEC.  If SPEC is nil, do nothing."
>     (let ((attrs (face-spec-choose spec frame)))
>       (when spec
> !       (face-spec-reset-face face (or frame t)))
>       (while attrs
>         (let ((attribute (car attrs))
>   	    (value (car (cdr attrs))))
> --- 1449,1462 ----
>   See `defface' for information about SPEC.  If SPEC is nil, do nothing."
>     (let ((attrs (face-spec-choose spec frame)))
>       (when spec
> !       (if frame
> !           (face-spec-reset-face face frame)
> !         ;; Clear all the new-frame defaults for this face.
> !         ;; face-spec-reset-face won't do it right.
> !         (let ((facevec (cdr (assq face face-new-frame-defaults))))
> !           (dotimes (i (length facevec))
> !             (unless (= i 0)
> !               (aset facevec i 'unspecified))))))
>       (while attrs
>         (let ((attribute (car attrs))
>   	    (value (car (cdr attrs))))

-- 
Johan Bockgård

Re: Change in face-spec-set

[Richard Stallman]

    The only change to face-spec-set is mine, to set the default for new
    frames when FRAME is nil. If I apply your patch, this is once again
    broken. By design it seems ("Clear all the new-frame defaults for this
    face"). Why do you want to do this?

Because face-spec-set is supposed to completely override the settings
of the face, setting it from zero.

With the current code, and without my patch, does that work?
If so, my patch is not necessary.

Re: Change in face-spec-set

[Richard Stallman]

[I sent this message a week ago but did not get a response.
Could we get the discussion moving again?]

    The only change to face-spec-set is mine, to set the default for new
    frames when FRAME is nil. If I apply your patch, this is once again
    broken. By design it seems ("Clear all the new-frame defaults for this
    face"). Why do you want to do this?

Because face-spec-set is supposed to completely override the settings
of the face, setting it from zero.

With the current code, and without my patch, does that work?
If so, my patch is not necessary.

Re: Change in face-spec-set

[Glenn Morris]

Richard Stallman wrote:

> Because face-spec-set is supposed to completely override the settings
> of the face, setting it from zero.
>
> With the current code, and without my patch, does that work?

Give me a test case starting from emacs -Q and I'll tell you.

If I try to guess what you might mean, then with the current code:

emacs -Q
(describe-face 'font-lock-warning-face)
  -> weight = bold

(face-spec-set 'font-lock-warning-face '((t :underline t)))
(make-frame)

On new frame:
(describe-face 'font-lock-warning-face)
  -> weight = unspecified

Is this the kind of thing you are talking about?

Re: Change in face-spec-set

[Johan Bockgård]

Glenn Morris <rgm@gnu.org> writes:

>> With the current code, and without my patch, does that work?
>
> Give me a test case starting from emacs -Q and I'll tell you.

With or without the patch, more changes are needed.

The 2007-09-17 change was supposed to fix this case

    $ emacs -Q -bg black

    (face-spec-set 'font-lock-string-face
                   '((((background light)) :foreground "blue")
                     (((background dark))  :foreground "green")))

    (make-frame '((background-color . "white")))

-- 
Johan Bockgård

Re: Change in face-spec-set

[Richard Stallman]

I tested your example, and it seems to work without that change.
So I won't install the change.

Thanks.