[BUG] Dark/bright face detection problem

[BUG] Dark/bright face detection problem

[Johannes Weiner]

[-- Attachment #1.1: Type: text/plain, Size: 1361 bytes --]

Hi,

I have the following scenario:

$ cat broken.el
(set-background-color "gray20")
(set-foreground-color "white")
(set-default-font "lucidasans-10")

$ cat working.el
(set-background-color "gray20")
(set-foreground-color "white")
(set-default-font "lucidasans-10")

1. emacs -Q -l working.el
The background is dark, the face is bright.  Everything ok.

2. emacs -Q -l broken.el
The face stays dark. For example in the minibuffer, when I do `find-file', the
font is dark blue and I can hardly read it.

When I call `set-default-font' or evaluate (set-default-font ...) in the
scratch buffer, the face is updated to be bright as in the first scenario.


I compared `set-default-font' and `set-background-color' in frame.el and could
not find anything that could cause this behaviour, so I figured the bug has to
be in `modify-frame-parameters'.

So, `Fmodify_frame_parameters' calls `frame-set-background-mode' when
!HAVE_WINDOW_SYSTEM, otherwise it just calls `x_set_frame_parameter', which
does NOT call `frame-set-background-mode'.  Is that right?!

Shouldn't `x_set_frame_parameter' also do something to update the faces
afterwards?  I could not find something that loos like it in the code.

HTH! And hit me, if you need more debugging information.

	Hannes

-- 
3BD8 AF56 11AF 205C 9DB6  B10F 76F1 0634 71A4 DCA0

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [BUG] Dark/bright face detection problem

[Johannes Weiner]

[-- Attachment #1.1: Type: text/plain, Size: 686 bytes --]

Whoops,

On Fri, Sep 28, 2007 at 04:38:33PM +0200, Johannes Weiner wrote:
> $ cat broken.el
> (set-background-color "gray20")
> (set-foreground-color "white")
> (set-default-font "lucidasans-10")

This is wrong.  It must be:

(set-default-font ...)
(set-background-color ...)
(set-foreground-color ...)

The important part is, that (set-default-font ...) is called _before_ the
color settings.

Because in the working scenario, `set-default-font' triggers the face
dark/bright color updating to be done.  So the face stays dark when
`set-default-font' is not called after `set-background-color'.

	Hannes

-- 
3BD8 AF56 11AF 205C 9DB6  B10F 76F1 0634 71A4 DCA0

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [BUG] Dark/bright face detection problem

[Richard Stallman]

frame-set-background-mode should be called by setting the background color,
and it ought to recompute all faces that are affected by the background mode.

Can you please investigate what happens inside frame-set-background-mode
so as to figure out the cause of this bug?

Re: [BUG] Dark/bright face detection problem

[Johannes Weiner]

[-- Attachment #1.1: Type: text/plain, Size: 1260 bytes --]

Hi,

On Sat, Sep 29, 2007 at 12:10:55PM -0400, Richard Stallman wrote:
> frame-set-background-mode should be called by setting the background color,
> and it ought to recompute all faces that are affected by the background mode.
> 
> Can you please investigate what happens inside frame-set-background-mode
> so as to figure out the cause of this bug?

I think I have found the problem, please correct me if I am wrong:

Starting at lisp/faces.el, around the comment in line 1674.  It says that all
faces that should be NOT updated are collected (negative list, presuming that
most faces should be updated and just to collect the ones that do not).

Now, I can not tell why the following happens since I am not into it very
deeply, but almost all faces, excluding about 4 here, are pushed to this
locally-modified list, which won't get updated through `face-spec-set' in line
1695.

The logic seems to be reversed; all faces that should get updated are left
untouched and vice versa.  `locally-modified' contains exactly the opposite
set of items it should contain.

I will dig deeper into it, but for perhaps this already helps someone who
knows the code better.

	Hannes

-- 
3BD8 AF56 11AF 205C 9DB6  B10F 76F1 0634 71A4 DCA0

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [BUG] Dark/bright face detection problem

[Johannes Weiner]

[-- Attachment #1.1: Type: text/plain, Size: 1183 bytes --]

Hi again,

On Sat, Sep 29, 2007 at 11:20:19PM +0200, Johannes Weiner wrote:
> I think I have found the problem, please correct me if I am wrong:
> 
> Starting at lisp/faces.el, around the comment in line 1674.  It says that all
> faces that should be NOT updated are collected (negative list, presuming that
> most faces should be updated and just to collect the ones that do not).
> 
> Now, I can not tell why the following happens since I am not into it very
> deeply, but almost all faces, excluding about 4 here, are pushed to this
> locally-modified list, which won't get updated through `face-spec-set' in line
> 1695.
> 
> The logic seems to be reversed; all faces that should get updated are left
> untouched and vice versa.  `locally-modified' contains exactly the opposite
> set of items it should contain.

I was lagging.  If `face-spec-match-p' is t, then the face is already okay and
should not be updated and therefor be pushed to the list.

(when (not (face-spec-match-p ...)) ...) is therefor wrong. It should be
(when (face-spec-match-p ...) ...).

Did I miss something?

	Hannes

-- 
3BD8 AF56 11AF 205C 9DB6  B10F 76F1 0634 71A4 DCA0

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Problems with setting face attributes for specific frames

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> CC: emacs-devel@gnu.org
> Date: Sun, 30 Sep 2007 08:54:56 -0400
> 
> Not many packages call `set-face-attribute'.  There are just
> two in Emacs that use it with a frame: ediff-wind.el
> and mac-win.el.  pc-win.el calls it via `set-face-foreground'
> and `set-face-background'.  The obsolete hilit19 package 
> calls `set-face-font', but we could delete that.
> 
> Kifer, what is ediff-wind.el trying to do, and could it be done in
> another way?
> 
> Eli, what is pc-win.el trying to do, and could it be done in
> another way?  Perhaps by changing the deffaces for those faces?

Do I understand correctly that the problem is with pc-win.el's use of
the frame argument to `set-face-fore/background'?  If so, I think we
can easily delete that argument: the only frame that exists when
msdos-face-setup runs is `terminal-frame' so nothing bad will happen.

For the record, what those calls do is set up the standard colors for
some popular faces, notably the colors of the drop-down menus.

Re: Problems with setting face attributes for specific frames

[Richard Stallman]

    Do I understand correctly that the problem is with pc-win.el's use of
    the frame argument to `set-face-fore/background'?  If so, I think we
    can easily delete that argument: the only frame that exists when
    msdos-face-setup runs is `terminal-frame' so nothing bad will happen.

Please do that; it would be good to at least get rid of the FRAME argument.

RE: Problems with setting face attributes for specific frames

[Drew Adams]

> In this message I propose a change in how faces work, in response
> to the analysis of this bug.
>
> The logic in `frame-set-background-mode' is correct for the case
> of customizing `frame-background-mode'.  It distinguishes the
> faces that match their defface specs from those for which attributes
> were overridden (perhaps by calling `set-face-attribute').
> It recalculates the former in the proper new way from the defface,
> and it leaves the latter alone.
>
> However, the logic is wrong for this case, where
> `frame-set-background-mode' is called from `set-background-color'.  If
> my analysis is correct, by the time `frame-set-background-mode' is
> called, the new background color has already been installed in the
> frame, but most faces still use the old one, so they do not match
> their specs any more.
>
> I think the code that handles faces needs to be cleaned up to keep
> user-specified single-frame face attributes separately from the
> actual face attributes deduced from the combination of all sources.
> That is the only way to deduce new actual face attributes correctly
> after changes in some of the parameters that the deduction is done from.
>
> Or perhaps we should get rid of using `set-face-attribute' to specify
> face attributes on a frame.
>
> Not many packages call `set-face-attribute'.  There are just
> two in Emacs that use it with a frame: ediff-wind.el
> and mac-win.el.  pc-win.el calls it via `set-face-foreground'
> and `set-face-background'.  The obsolete hilit19 package
> calls `set-face-font', but we could delete that.

I don't understand all of what you're proposing or what the problem is that
you're fixing. I think you're proposing removing the FRAME arg from
`set-face-attribute' (perhaps among other changes). Is that correct?

I do use `set-face-attribute' in some of my code, but always with a nil
FRAME arg, so that would be OK by me. I assume that we would still have
`modify-face', in any case, right?

Re: Problems with setting face attributes for specific frames

[Richard Stallman]

    I don't understand all of what you're proposing or what the problem is that
    you're fixing. I think you're proposing removing the FRAME arg from
    `set-face-attribute' (perhaps among other changes). Is that correct?

Yes.

    I assume that we would still have
    `modify-face', in any case, right?

With THIS change, we would still have `modify-face' but it would
not take a FRAME argument.

I also made a separate proposal to abolish `set-face-attribute' entirely,
along with all the other functions that operate at the same level.
That would mean abolishing `modify-face'.

RE: Problems with setting face attributes for specific frames

[Drew Adams]

>     I don't understand all of what you're proposing or what the
>     problem is that you're fixing. I think you're proposing removing
>     the FRAME arg from `set-face-attribute' (perhaps among other
>     changes). Is that correct?
>
> Yes.

OK, but I still don't understand the problem that you're fixing. Could you
perhaps summarize it?

>     I assume that we would still have
>     `modify-face', in any case, right?
>
> With THIS change, we would still have `modify-face' but it would
> not take a FRAME argument.
>
> I also made a separate proposal to abolish `set-face-attribute' entirely,
> along with all the other functions that operate at the same level.
> That would mean abolishing `modify-face'.

I use both `set-face-attribute' and `modify-face' in my code. I do not use
the FRAME parameter with either, but I do use these functions to change
faces globally.

I do not understand (1) the problem you are solving or (2) how, after your
changes, we will modify faces, that is, how we will do what we do now with,
say, `modify-face'. Could you please explain that a bit?

I have no problem with doing things in a different way, but I would have a
problem with not being able to change face attributes in some (selective)
way.

Re: Problems with setting face attributes for specific frames

[Richard Stallman]

    >     I don't understand all of what you're proposing or what the
    >     problem is that you're fixing. I think you're proposing removing
    >     the FRAME arg from `set-face-attribute' (perhaps among other
    >     changes). Is that correct?
    >
    > Yes.

    OK, but I still don't understand the problem that you're fixing. Could you
    perhaps summarize it?

If we get rid of using `set-face-attribute' for specific frames, the
resulting simplification would make it possible to fix the handling of
changes in background mode due to setting the background color.

RE: Problems with setting face attributes for specific frames

[Drew Adams]

>     >     I don't understand all of what you're proposing or what the
>     >     problem is that you're fixing. I think you're proposing removing
>     >     the FRAME arg from `set-face-attribute' (perhaps among other
>     >     changes). Is that correct?
>     >
>     > Yes.
>
>     OK, but I still don't understand the problem that you're
>     fixing. Could you perhaps summarize it?
>
> If we get rid of using `set-face-attribute' for specific frames, the
> resulting simplification would make it possible to fix the handling of
> changes in background mode due to setting the background color.

AFAICT, I do not currently change a face attribute for a single frame, but
it sounds like a reasonable thing to want to do. Is getting rid of that
possibility really necessary to be able to handle "changes in background
mode due to setting the background color"?

I still don't understand what that problem is, BTW. In your description of
it in this thread, you say that it has to do with `set-background-color'
calling `frame-set-background'. Why does doing that affect the frame
background mode? And what does changing the frame background (with or
without the background mode) have to do with changing an attribute of an
individual face for that frame?

Conceptually, at least, those would seem to be separate things. Perhaps the
problem is that they are not separate in the implementation? You say that
you can alternatively solve this problem by keeping "user-specified
single-frame face attributes separate from the actual face attributes
deduced..." If the former are interfering with the frame background mode,
then that sounds like a better approach than simply getting rid of them.

To me, they sound like a useful feature, perhaps even more useful than the
frame-background-mode, which is anyway problematic wrt what constitutes
"light" and "dark" etc.

In any case, as I say, I don't use that feature today. The more important
problem you would create for me (by your other proposal) would be to get rid
of the ability to change a face attribute globally (for all frames at once).
What is the reason for that proposal? AFAICT, you gave no reason; you just
said you are thinking of making that change.

Re: Problems with setting face attributes for specific frames

[Richard Stallman]

    AFAICT, I do not currently change a face attribute for a single frame, but
    it sounds like a reasonable thing to want to do. Is getting rid of that
    possibility really necessary to be able to handle "changes in background
    mode due to setting the background color"?

Yes I think so.

    I still don't understand what that problem is, BTW.

To explain this would be a lot more work, and would not affect
this discussion.

    In any case, as I say, I don't use that feature today. The more important
    problem you would create for me (by your other proposal) would be to get rid
    of the ability to change a face attribute globally (for all frames at once).
    What is the reason for that proposal? AFAICT, you gave no reason; you just
    said you are thinking of making that change.

The motive is to make a big simplification in the face code.
Right now it is quite hard to maintain that code, because there are
many old and new interfaces, and it is often hard to figure out
the right way to reconcile them.

Re: Problems with setting face attributes for specific frames

[Stephen J. Turnbull]

Richard Stallman writes:

 > I also made a separate proposal to abolish `set-face-attribute' entirely,
 > along with all the other functions that operate at the same level.
 > That would mean abolishing `modify-face'.

Presumably also `copy-face', since since aliases would surely be
preferred rather than actual immutable copies of faces if you need
multiple names for backward compatibility or whatever.

Re: Problems with setting face attributes for specific frames

[Richard Stallman]

    Presumably also `copy-face', since since aliases would surely be
    preferred rather than actual immutable copies of faces if you need
    multiple names for backward compatibility or whatever.

Maybe you are right, but I am not sure.  Maybe it would still be
meaningful to copy a face by copying its spec.

Re: [BUG] Dark/bright face detection problem

[Dan Nicolaescu]

Johannes Weiner <hannes@saeurebad.de> writes:

  > Whoops,
  > 
  > On Fri, Sep 28, 2007 at 04:38:33PM +0200, Johannes Weiner wrote:
  > > $ cat broken.el
  > > (set-background-color "gray20")
  > > (set-foreground-color "white")
  > > (set-default-font "lucidasans-10")
  > 
  > This is wrong.  It must be:
  > 
  > (set-default-font ...)
  > (set-background-color ...)
  > (set-foreground-color ...)
  > 
  > The important part is, that (set-default-font ...) is called _before_ the
  > color settings.
  > 
  > Because in the working scenario, `set-default-font' triggers the face
  > dark/bright color updating to be done.  So the face stays dark when
  > `set-default-font' is not called after `set-background-color'.

This is just speculation, but it seems that the issue that you are
seeing is the same as the one discussed in the long running thread
"23.0.50; face-problems with multy-tty"

Re: [BUG] Dark/bright face detection problem

[Eli Zaretskii]

> Date: Fri, 28 Sep 2007 16:38:33 +0200
> From: Johannes Weiner <hannes@saeurebad.de>
> 
> HTH! And hit me, if you need more debugging information.

You didn't say in what version of Emacs do you see this.

Re: [BUG] Dark/bright face detection problem

[Johannes Weiner]

[-- Attachment #1.1: Type: text/plain, Size: 382 bytes --]

Hi,

On Sat, Sep 29, 2007 at 05:52:24PM +0200, Eli Zaretskii wrote:
> You didn't say in what version of Emacs do you see this.

Sorry.  Here it is GNU Emacs 23.0.50.1 (i686-pc-linux-gnu, GTK+ Version
2.10.13) of 2007-09-2.
But it is also buggy on a CVS HEAD build from yesterday (September 27th,
2007).

	Hannes

-- 
3BD8 AF56 11AF 205C 9DB6  B10F 76F1 0634 71A4 DCA0

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel