bug#911: 23.0.60; custom-set-faces causes "New Frame" to fail

bug#911: 23.0.60; custom-set-faces causes "New Frame" to fail

[Chong Yidong]

> > Why not in frame-set-background-mode itself?  Then, when it's set,
> > subsequent calls to that function could be no-ops.
>
> We have to reset it eventually.  And we have to cater for the situation
> that a user might want to open two frames in one and the same command.
> So I suppose the only reliable place is the command loop.  And we must
> make sure that the background determined by the user is considered.

I'm not sure what you mean.  To be precise, here's a patch implementing
the approach I'm suggesting (indentated for readability).  Would it
work?

(I can't reproduce the original bug myself, so maybe Ian can test it.)


*** trunk/lisp/faces.el.~1.425.~	2008-09-24 12:51:07.000000000 -0400
--- trunk/lisp/faces.el	2008-09-28 17:10:55.000000000 -0400
***************
*** 1839,1848 ****
--- 1839,1851 ----
  (declare-function x-get-resource "frame.c"
  		  (attribute class &optional component subclass))
  
+ (defvar inhibit-frame-set-background-mode nil)
+ 
  (defun frame-set-background-mode (frame)
    "Set up display-dependent faces on FRAME.
  Display-dependent faces are those which have different definitions
  according to the `background-mode' and `display-type' frame parameters."
+   (unless inhibit-frame-set-background-mode
    (let* ((bg-resource
  	  (and (window-system frame)
  	       (x-get-resource "backgroundMode" "BackgroundMode")))
***************
*** 1893,1899 ****
  	  (frame-parameter frame 'display-type)))
  
      (unless (and (eq bg-mode old-bg-mode) (eq display-type old-display-type))
!       (let ((locally-modified-faces nil))
  	;; Before modifying the frame parameters, we collect a list of
  	;; faces that don't match what their face-spec says they should
  	;; look like; we then avoid changing these faces below.
--- 1896,1905 ----
  	  (frame-parameter frame 'display-type)))
  
      (unless (and (eq bg-mode old-bg-mode) (eq display-type old-display-type))
!       (let ((locally-modified-faces nil)
! 	    ;; Prevent face-spec-recalc from calling this function
! 	    ;; again, resulting in a loop.
! 	    (inhibit-frame-set-background-mode t))
  	;; Before modifying the frame parameters, we collect a list of
  	;; faces that don't match what their face-spec says they should
  	;; look like; we then avoid changing these faces below.
***************
*** 1915,1920 ****
--- 1921,1927 ----
  	(dolist (face (face-list))
  	  (unless (memq face locally-modified-faces)
  	    (face-spec-recalc face frame)))))))
+ )
  
  \f
  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

bug#911: 23.0.60; custom-set-faces causes "New Frame" to fail

[martin rudalics]

 > How about defining a variable `frame-set-background-mode-bg-mode'?  If
 > it's nil, frame-set-background-mode uses the existing calculation to
 > determine whether it's light or dark; if it's non-nil, it uses that
 > existing value.  This should prevent the oscillation.

But who would be responsible for setting and resetting that variable?
Hardly `frame-set-background-mode' itself.  Would we make this a frame
parameter so the command loop can reset it and allow to change it once
and only once for a command?  Otherwise, we might risk reentering the
loop with the next call of `frame-set-background-mode'.

martin

BTW, it seems you already closed bug#911 ...

bug#911: 23.0.60; custom-set-faces causes "New Frame" to fail

[Chong Yidong]

martin rudalics <rudalics@gmx.at> writes:

>> How about defining a variable `frame-set-background-mode-bg-mode'?  If
>> it's nil, frame-set-background-mode uses the existing calculation to
>> determine whether it's light or dark; if it's non-nil, it uses that
>> existing value.  This should prevent the oscillation.
>
> But who would be responsible for setting and resetting that variable?
> Hardly `frame-set-background-mode' itself.  Would we make this a frame
> parameter so the command loop can reset it and allow to change it once
> and only once for a command?  Otherwise, we might risk reentering the
> loop with the next call of `frame-set-background-mode'.

Why not in frame-set-background-mode itself?  Then, when it's set,
subsequent calls to that function could be no-ops.

> BTW, it seems you already closed bug#911 ...

Sorry, I must have mistyped when trying to close a different bug.
Reopened.

bug#911: 23.0.60; custom-set-faces causes "New Frame" to fail

[martin rudalics]

 > Why not in frame-set-background-mode itself?  Then, when it's set,
 > subsequent calls to that function could be no-ops.

We have to reset it eventually.  And we have to cater for the situation
that a user might want to open two frames in one and the same command.
So I suppose the only reliable place is the command loop.  And we must
make sure that the background determined by the user is considered.

martin