Diff mode faces

Diff mode faces

[Juri Linkov]

The default diff face used to highlight changed lines on tty
(magenta/yellow bold italic) makes an indigestible fruit salad.
OTOH, the same face has no highlighting under X.  This makes
sense, because changed lines are the primary text in diff files
that doesn't need special highlighting.

I propose instead of highlighting whole lines on tty to use at least
the same approach as introduced recently for comments on tty to highlight
only comment delimiters in new face font-lock-comment-delimiter-face.
Similarly, only diff indicators (the first character of the line) could
be highlighted in diff buffers on tty.

The patch below adds three new faces for highlighting diff indicators.
On tty they inherit from font-lock-comment-delimiter-face, but on X
there is no highlighting which preserves the current default appearance.
These faces are useful not only on tty but even on window systems
where users might want to redefine them.

Also I noticed that unlike context hunk headers `--- 123 ---',
normal (i.e. neither context nor unified) hunk headers `---' are
not highlighted.  The same patch fixes that.

With -D option diff makes `#ifdef' format output with conditional
preprocessor directives.  Currently diff-mode.el uses hard-coded
font-lock-string-face to highlight them.  The patch adds a new face
to allow users to configure it.  By default, it inherits from
font-lock-preprocessor-face.

Index: lisp/diff-mode.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/diff-mode.el,v
retrieving revision 1.75
diff -u -r1.75 diff-mode.el
--- lisp/diff-mode.el	14 Jun 2005 14:34:40 -0000	1.75
+++ lisp/diff-mode.el	17 Jun 2005 11:21:23 -0000
@@ -240,16 +240,34 @@
 (defvar diff-added-face 'diff-added)
 
 (defface diff-changed
-  '((((type tty pc) (class color) (background light))
-     :foreground "magenta" :weight bold :slant italic)
-    (((type tty pc) (class color) (background dark))
-     :foreground "yellow" :weight bold :slant italic))
+  nil
   "`diff-mode' face used to highlight changed lines."
   :group 'diff-mode)
 ;; backward-compatibility alias
 (put 'diff-changed-face 'face-alias 'diff-changed)
 (defvar diff-changed-face 'diff-changed)
 
+(defface diff-indicator-removed
+  '((t :inherit diff-indicator-changed))
+  "`diff-mode' face used to highlight indicator of removed lines (-, <)."
+  :group 'diff-mode
+  :version "22.1")
+(defvar diff-indicator-removed-face 'diff-indicator-removed)
+
+(defface diff-indicator-added
+  '((t :inherit diff-indicator-changed))
+  "`diff-mode' face used to highlight indicator of added lines (+, >)."
+  :group 'diff-mode
+  :version "22.1")
+(defvar diff-indicator-added-face 'diff-indicator-added)
+
+(defface diff-indicator-changed
+  '((((type tty pc) (class color)) :inherit font-lock-comment-delimiter-face))
+  "`diff-mode' face used to highlight indicator of changed lines."
+  :group 'diff-mode
+  :version "22.1")
+(defvar diff-indicator-changed-face 'diff-indicator-changed)
+
 (defface diff-function
   '((t :inherit diff-context))
   "`diff-mode' face used to highlight function names produced by \"diff -p\"."
@@ -274,6 +292,13 @@
 (put 'diff-nonexistent-face 'face-alias 'diff-nonexistent)
 (defvar diff-nonexistent-face 'diff-nonexistent)
 
+(defface diff-preprocessor
+  '((t :inherit  font-lock-preprocessor-face))
+  "`diff-mode' face used to highlight preprocessor lines staring with #."
+  :group 'diff-mode
+  :version "22.1")
+(defvar diff-preprocessor-face 'diff-preprocessor)
+
 (defconst diff-yank-handler '(diff-yank-function))
 (defun diff-yank-function (text)
   ;; FIXME: the yank-handler is now called separately on each piece of text
@@ -306,15 +331,16 @@
      (1 diff-hunk-header-face)
      (2 diff-function-face))
     ("^\\*\\*\\* .+ \\*\\*\\*\\*". diff-hunk-header-face) ;context
+    ("^---$" . diff-hunk-header-face) ; normal
     ("^\\(---\\|\\+\\+\\+\\|\\*\\*\\*\\) \\(\\S-+\\)\\(.*[^*-]\\)?\n"
      (0 diff-header-face) (2 diff-file-header-face prepend))
     ("^[0-9,]+[acd][0-9,]+$" . diff-hunk-header-face)
-    ("^!.*\n" (0 diff-changed-face))
-    ("^[+>].*\n" (0 diff-added-face))
-    ("^[-<].*\n" (0 diff-removed-face))
+    ("^\\([-<]\\)\\(.*\n\\)" (1 diff-indicator-removed-face) (2 diff-removed-face))
+    ("^\\([+>]\\)\\(.*\n\\)" (1 diff-indicator-added-face) (2 diff-added-face))
+    ("^\\(!\\)\\(.*\n\\)" (1 diff-indicator-changed-face) (2 diff-changed-face))
     ("^Index: \\(.+\\).*\n" (0 diff-header-face) (1 diff-index-face prepend))
     ("^Only in .*\n" . diff-nonexistent-face)
-    ("^#.*" . font-lock-string-face)
+    ("^#.*" . diff-preprocessor-face)
     ("^[^-=+*!<>].*\n" (0 diff-context-face))))
 
 (defconst diff-font-lock-defaults

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Jason Rumney]

Juri Linkov wrote:

>I propose instead of highlighting whole lines on tty to use at least
>the same approach as introduced recently for comments on tty to highlight
>only comment delimiters in new face font-lock-comment-delimiter-face.
>Similarly, only diff indicators (the first character of the line) could
>be highlighted in diff buffers on tty.
>  
>
Why should tty be different than non-tty in either of these cases?

Re: Diff mode faces

[Stefan Monnier]

> With -D option diff makes `#ifdef' format output with conditional
> preprocessor directives.  Currently diff-mode.el uses hard-coded
> font-lock-string-face to highlight them.  The patch adds a new face
> to allow users to configure it.  By default, it inherits from
> font-lock-preprocessor-face.

The "^#.*" pattern is meant to match "comment lines".  These are part of an
extension of the diff format.  It has nothing to do with the -D option
which I do not consider as outputting a diff format and diff-mode does not
support this output format in any way.


        Stefan

Re: Diff mode faces

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Date: Fri, 17 Jun 2005 14:47:05 +0300
> 
> The default diff face used to highlight changed lines on tty
> (magenta/yellow bold italic) makes an indigestible fruit salad.

In your opinion, perhaps.  These faces are there since 5 years ago,
and I don't recall anyone complaining.

> OTOH, the same face has no highlighting under X.  This makes
> sense, because changed lines are the primary text in diff files
> that doesn't need special highlighting.

They need highlighting to stand out against the unchanged lines.
Since most tty's don't have too many colors, they cannot make use of
the technique similar to what we do on X, which is to dim the context
lines by using light-grey colors for them.

> I propose instead of highlighting whole lines on tty to use at least
> the same approach as introduced recently for comments on tty to highlight
> only comment delimiters in new face font-lock-comment-delimiter-face.
> Similarly, only diff indicators (the first character of the line) could
> be highlighted in diff buffers on tty.

I don't think this is a good idea to make such a change.  Certainly
not now, before a release.  Haven't we already seen enough trouble
from this comment-delimiter face innovation?

Anyway, the results look IMHO ugly.  Partial highlighting of lines
generally looks ugly, and this case is no exception.

(I could never understand the urge to modify the default colors.  If
you dislike the defaults, customize the faces and be done with them.)

Re: Diff mode faces

[Richard Stallman]

I don't find the current Diff mode setup hard to use on a tty.
That is perhaps because the font used for the changed lines
is not hard to read.

Re: Diff mode faces

[Juri Linkov]

> I don't find the current Diff mode setup hard to use on a tty.
> That is perhaps because the font used for the changed lines
> is not hard to read.

Then could you explain the reasons for removing comments highlighting
on tty with leaving only comment delimiters highlighted.

I see no big difference between comments displayed in red and diff lines
displayed in bold italic magenta (the latter looks even worse).

I don't insist on changing the default diff face on tty, but trying to
achieve consistency.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Juri Linkov]

> The "^#.*" pattern is meant to match "comment lines".  These are
> part of an extension of the diff format.  It has nothing to do with
> the -D option which I do not consider as outputting a diff format
> and diff-mode does not support this output format in any way.

Since it is meant to match comments then maybe we should leave the
face for "^#.*" hard-coded in font-lock keywords, but change it to
font-lock-comment-face.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Juri Linkov]

>> The default diff face used to highlight changed lines on tty
>> (magenta/yellow bold italic) makes an indigestible fruit salad.
>
> In your opinion, perhaps.  These faces are there since 5 years ago,
> and I don't recall anyone complaining.

Maybe people prefer to quietly customize faces in .emacs instead of
spending energy arguing? ;-)

>> OTOH, the same face has no highlighting under X.  This makes
>> sense, because changed lines are the primary text in diff files
>> that doesn't need special highlighting.
>
> They need highlighting to stand out against the unchanged lines.
> Since most tty's don't have too many colors, they cannot make use of
> the technique similar to what we do on X, which is to dim the
> context lines by using light-grey colors for them.

Yellow foreground works well for context lines.

>> I propose instead of highlighting whole lines on tty to use at least
>> the same approach as introduced recently for comments on tty to highlight
>> only comment delimiters in new face font-lock-comment-delimiter-face.
>> Similarly, only diff indicators (the first character of the line) could
>> be highlighted in diff buffers on tty.
>
> I don't think this is a good idea to make such a change.  Certainly
> not now, before a release.

It is face fixing time now.

> Haven't we already seen enough trouble from this comment-delimiter
> face innovation?

Comment-delimiter face was the global change, diff faces are localized
to diff mode.

> Anyway, the results look IMHO ugly.  Partial highlighting of lines
> generally looks ugly, and this case is no exception.

Do you object against partial highlighting of comments on tty with
comment-delimiter too?  I tried to be consistent with comments
highlighting on tty.

> (I could never understand the urge to modify the default colors.  If
> you dislike the defaults, customize the faces and be done with them.)

I have 40 faces customized in .emacs and never wanted to suggest them
to anyone.  What I want is to propose fixes for badly chosen faces
according to more or less objective criteria such as that unimportant
faces should not stand out too much, important faces should be easily
readable, fg/bg colors should contrast well, etc.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Randal L. Schwartz]

>>>>> "Juri" == Juri Linkov <juri@jurta.org> writes:

Juri> Yellow foreground works well for context lines.

Except when you normally read black on white.  Is there no way to
switch automatically to a proper set of darker colors when the main
text is black instead of white?

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: Diff mode faces

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> From: merlyn@stonehenge.com (Randal L. Schwartz)
> Date: 18 Jun 2005 08:27:12 -0700
> 
> Is there no way to switch automatically to a proper set of darker
> colors when the main text is black instead of white?

Yes, it's called background-mode.

Re: Diff mode faces

[Richard Stallman]

    Then could you explain the reasons for removing comments highlighting
    on tty with leaving only comment delimiters highlighted.

I explained them a few weeks ago.

    I see no big difference between comments displayed in red and diff lines
    displayed in bold italic magenta (the latter looks even worse).

I don't think the issues are similar.

    I don't insist on changing the default diff face on tty, but trying to
    achieve consistency.

Consistency doesn't apply for two issues that are quite different.

Re: Diff mode faces

[Juri Linkov]

> Juri> Yellow foreground works well for context lines.
>
> Except when you normally read black on white.

For quite a long time (most likely from the time when color translation
was implemented for tty) colors of context lines in diff mode on tty
were yellow for light backgrounds.  There are also other faces with
yellow foreground for light backgrounds, like font-lock-variable-name-face.
Nobody complained about them so far.  So maybe yellow is the acceptable
color for context lines.

> Is there no way to switch automatically to a proper set of darker
> colors when the main text is black instead of white?

By default on terminals Emacs background mode is `light', regardless of
actual terminal background color, so you don't need to change the
background mode when the main text is black on white.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Juri Linkov]

>> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
>> From: merlyn@stonehenge.com (Randal L. Schwartz)
>> Date: 18 Jun 2005 08:27:12 -0700
>> 
>> Is there no way to switch automatically to a proper set of darker
>> colors when the main text is black instead of white?
>
> Yes, it's called background-mode.

I see there is one problem with background-mode on tty.  Unlike X,
on tty Emacs doesn't adjust background-mode after changing the
background color with any of the known methods: `set-background-color'
command, or customizing the background color of the default face.

It seems the correct place to fix this is `modify-frame-parameters'.
The call to Qframe_update_face_colors is duplicated from the function
`update_face_from_frame_parameter' in the patch below.  With this change
`frame-set-background-mode' gets called, so it updates background-mode
after every change of the background color on tty.

Index: src/frame.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/frame.c,v
retrieving revision 1.316
diff -u -r1.316 frame.c
--- src/frame.c	10 Jun 2005 02:22:11 -0000	1.316
+++ src/frame.c	19 Jun 2005 11:42:05 -0000
@@ -2313,6 +2313,13 @@
 	  prop = parms[i];
 	  val = values[i];
 	  store_frame_param (f, prop, val);
+
+	  if (EQ (prop, Qbackground_color)) {
+	    /* Changing the background color might change the background
+	       mode, so that we have to load new defface specs.  Call
+	       frame-update-face-colors to do that.  */
+	    call1 (Qframe_update_face_colors, frame);
+	  }
 	}
     }

Index: src/dispextern.h
===================================================================
RCS file: /cvsroot/emacs/emacs/src/dispextern.h,v
retrieving revision 1.202
diff -u -r1.202 dispextern.h
--- src/dispextern.h	17 Jun 2005 14:02:03 -0000	1.202
+++ src/dispextern.h	19 Jun 2005 11:43:31 -0000
@@ -2793,6 +2793,7 @@
 int merge_faces P_ ((struct frame *, Lisp_Object, int, int));
 int compute_char_face P_ ((struct frame *, int, Lisp_Object));
 void free_all_realized_faces P_ ((Lisp_Object));
+extern Lisp_Object Qframe_update_face_colors;
 extern Lisp_Object Qforeground_color, Qbackground_color;
 extern char unspecified_fg[], unspecified_bg[];
 void free_realized_multibyte_face P_ ((struct frame *, int));

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Juri Linkov]

BTW, font-lock-comment-delimiter-face as well as font-lock-negation-char-face
are very new faces, so it's better to remove the -face suffix from them now
than to create aliases for backward compatibility later after the release.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Luc Teirlinck]

Juri Linkov wrote:

   > Juri> Yellow foreground works well for context lines.
   >
   > Except when you normally read black on white.

   For quite a long time (most likely from the time when color translation
   was implemented for tty) colors of context lines in diff mode on tty
   were yellow for light backgrounds.  There are also other faces with
   yellow foreground for light backgrounds, like font-lock-variable-name-face.
   Nobody complained about them so far.  So maybe yellow is the acceptable
   color for context lines.

I remember seeing people complain that they have trouble reading
yellow on white _many_ times on this list.  I know of many people who
have trouble reading red on black.  I have trouble with both and I
have even more problems reading cyan on white.  I do not complain, I
just always turn colors off on tty's.

When discussing colors one has to realize that it depends very much on
individual eyesight, as well as on the monitor setting and plenty of
other factors.  Not everybody with an abnormal color vision is aware
that they have abnormal color vision.

It is not easy (probably hopeless) to support people with abnormal
color vision while picking default colors (other than only using black
on white or vice versa), since variation among people with abnormal
color vision is so large.

Sincerely,

Luc.

Re: Diff mode faces

[Randal L. Schwartz]

>>>>> "Luc" == Luc Teirlinck <teirllm@dms.auburn.edu> writes:

Luc> I remember seeing people complain that they have trouble reading
Luc> yellow on white _many_ times on this list.

Yup.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: Diff mode faces

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Cc: merlyn@stonehenge.com, emacs-devel@gnu.org
> Date: Sun, 19 Jun 2005 16:09:47 +0300
> 
> I see there is one problem with background-mode on tty.  Unlike X,
> on tty Emacs doesn't adjust background-mode after changing the
> background color with any of the known methods: `set-background-color'
> command, or customizing the background color of the default face.
> 
> It seems the correct place to fix this is `modify-frame-parameters'.
> The call to Qframe_update_face_colors is duplicated from the function
> `update_face_from_frame_parameter' in the patch below.  With this change
> `frame-set-background-mode' gets called, so it updates background-mode
> after every change of the background color on tty.

I have a few issues with this patch.

First, it calls an obsolete function frame-update-face-colors (it's an
alias for backward compatibility; let's use the function it is aliased
to).

Second, I think doing this unconditionally might not be a good idea:
wouldn't it clash with what x_set_frame_parameters and
IT_set_frame_parameters do for their respective displays?

Third, please be sure to test this change with various ways one can
use to set colors on a tty, including these few:

  . emacs -fg FOO
  . emacs -fg FOO -bg BAR
  . emacs -bg BAR
  . repeat the above 3 with -rv, and convince yourself that the above
    4 tests produce expected results
  . modify the default color with set-background-color and verify that
    it is in effect for new frames created with "C-x 5 b" and the like
  . same as the last one above, but with set-face-background for the
    default face, both with and without the optional frame arg; verify
    that with an arg only the named frame is affected and without an
    arg all frames are affected, including the newly created ones

Finally, could you please elaborate on your analysis of this issue; in
particular, where is this handled on X?

Re: Diff mode faces

[Miles Bader]

On 6/19/05, Juri Linkov <juri@jurta.org> wrote:
> By default on terminals Emacs background mode is `light', regardless of
> actual terminal background color, so you don't need to change the
> background mode when the main text is black on white.

No -- by default the background mode is `dark' on terminals, _except_
xterm, which defaults to `light' (and it additionally has a hack to
discover the real background mode on rxvt).

-Miles
-- 
Do not taunt Happy Fun Ball.

Re: Diff mode faces

[Richard Stallman]

    BTW, font-lock-comment-delimiter-face as well as font-lock-negation-char-face
    are very new faces, so it's better to remove the -face suffix from them now
    than to create aliases for backward compatibility later after the release.

Maybe, but currently all the faces defined by font-lock have
names ending in -face.

Perhaps we should change them all now, since so many other faces
have been renamed.  If so, I think it would be good to add
parallel aliases even for the faces that are new, just so that
all the faces have them in the same way.

Re: Diff mode faces

[Juri Linkov]

> First, it calls an obsolete function frame-update-face-colors (it's an
> alias for backward compatibility; let's use the function it is aliased
> to).

Then it would be better to rename it in all C files to not create
Lisp symbols in C for both of them.

> Second, I think doing this unconditionally might not be a good idea:
> wouldn't it clash with what x_set_frame_parameters and
> IT_set_frame_parameters do for their respective displays?

It is called conditionally on non-window and non-dos systems
so it doesn't clash with x_set_frame_parameters or IT_set_frame_parameters.

> Third, please be sure to test this change with various ways one can
> use to set colors on a tty, including these few:
>
>   . emacs -fg FOO
>   . emacs -fg FOO -bg BAR
>   . emacs -bg BAR
>   . repeat the above 3 with -rv, and convince yourself that the above
>     4 tests produce expected results

I see there is a bug not caused by my patch:

  emacs -q -nw -rv

sets the background mode to light on xterm.  But since -rv switches
foreground and background, it should switch the background mode too
from light to dark on xterm.

>   . modify the default color with set-background-color and verify that
>     it is in effect for new frames created with "C-x 5 b" and the like

For frames with undefined backgrounds it reuses the background mode
specified with set-background-color.  Is it right?

>   . same as the last one above, but with set-face-background for the
>     default face, both with and without the optional frame arg; verify
>     that with an arg only the named frame is affected and without an
>     arg all frames are affected, including the newly created ones

This works.

> Finally, could you please elaborate on your analysis of this issue; in
> particular, where is this handled on X?

On X this is handled in `if (EQ (param, Qbackground_color))' condition
in update_face_from_frame_parameter(xfaces.c) which is reached from
Fmodify_frame_parameters thru the following function calls:

update_face_from_frame_parameter at xfaces.c:4490
x_set_background_color at xfns.c:911
x_set_frame_parameters at frame.c:2738
Fmodify_frame_parameters at frame.c:2280
Finternal_set_lisp_face_attribute at xfaces.c:4434

But on tty it doesn't go past the function Fmodify_frame_parameters
due to the condition `if (FRAME_WINDOW_P (f))'.  The explicit call
to `Qframe_update_face_colors' I added in Fmodify_frame_parameters
is on the else-branch of this condition.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Date: Mon, 20 Jun 2005 07:48:08 +0300
> Cc: emacs-devel@gnu.org
> 
> > First, it calls an obsolete function frame-update-face-colors (it's an
> > alias for backward compatibility; let's use the function it is aliased
> > to).
> 
> Then it would be better to rename it in all C files to not create
> Lisp symbols in C for both of them.

Yes.

> > Second, I think doing this unconditionally might not be a good idea:
> > wouldn't it clash with what x_set_frame_parameters and
> > IT_set_frame_parameters do for their respective displays?
> 
> It is called conditionally on non-window and non-dos systems
> so it doesn't clash with x_set_frame_parameters or IT_set_frame_parameters.

Sorry, you are right.

> I see there is a bug not caused by my patch:
> 
>   emacs -q -nw -rv
> 
> sets the background mode to light on xterm.  But since -rv switches
> foreground and background, it should switch the background mode too
> from light to dark on xterm.

I think this is a bug.

> >   . modify the default color with set-background-color and verify that
> >     it is in effect for new frames created with "C-x 5 b" and the like
> 
> For frames with undefined backgrounds it reuses the background mode
> specified with set-background-color.  Is it right?

Yes, set-background-color is global, its effect is not limited to the
frame where it was invoked.

> update_face_from_frame_parameter at xfaces.c:4490
> x_set_background_color at xfns.c:911
> x_set_frame_parameters at frame.c:2738
> Fmodify_frame_parameters at frame.c:2280
> Finternal_set_lisp_face_attribute at xfaces.c:4434
> 
> But on tty it doesn't go past the function Fmodify_frame_parameters
> due to the condition `if (FRAME_WINDOW_P (f))'.  The explicit call
> to `Qframe_update_face_colors' I added in Fmodify_frame_parameters
> is on the else-branch of this condition.

Thanks, I think this changed is okay.

Background mode (was: Diff mode faces)

[Juri Linkov]

>> I see there is a bug not caused by my patch:
>> 
>>   emacs -q -nw -rv
>> 
>> sets the background mode to light on xterm.  But since -rv switches
>> foreground and background, it should switch the background mode too
>> from light to dark on xterm.
>
> I think this is a bug.

There are also other ways to reproduce this bug, for example, running
Emacs without -rv option on xterm and evaluating (invert-face 'default).
Every time it evaluated, it switches the background mode to a
reversed value: with real black background it sets the light
background mode; with white background - to dark mode.  That's because
xterm presence is checked only once in startup.el (with setting the
mode to light), but any subsequent call of `frame-set-background-mode'
ignores the fact that Emacs runs on xterm.  On rxvt (invert-face 'default)
doesn't change the background mode at all because rxvt.el sets the
value of `frame-background-mode' permanently.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Background mode

[Juri Linkov]

>>> I see there is a bug not caused by my patch:
>>> 
>>>   emacs -q -nw -rv
>>> 
>>> sets the background mode to light on xterm.  But since -rv switches
>>> foreground and background, it should switch the background mode too
>>> from light to dark on xterm.
>>
>> I think this is a bug.
>
> There are also other ways to reproduce this bug, for example, running
> Emacs without -rv option on xterm and evaluating (invert-face 'default).
> Every time it evaluated, it switches the background mode to a
> reversed value: with real black background it sets the light
> background mode; with white background - to dark mode.  That's because
> xterm presence is checked only once in startup.el (with setting the
> mode to light), but any subsequent call of `frame-set-background-mode'
> ignores the fact that Emacs runs on xterm.  On rxvt (invert-face 'default)
> doesn't change the background mode at all because rxvt.el sets the
> value of `frame-background-mode' permanently.

Below is the patch that fixes this problem.  It adds a new internal
variable for the default background mode for terminals.

Index: lisp/faces.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/faces.el,v
retrieving revision 1.324
diff -u -r1.324 faces.el
--- lisp/faces.el	23 Jun 2005 21:24:58 -0000	1.324
+++ lisp/faces.el	27 Jun 2005 00:00:55 -0000
@@ -1572,6 +1572,12 @@
 		 (choice-item light)
 		 (choice-item :tag "default" nil)))
 
+(defvar default-frame-background-mode nil
+  "Internal variable for the default brightness of the background.
+The default `nil' means `dark'.  If Emacs runs in non-windowed mode
+from `xterm' or a similar terminal emulator, the value is `light'.
+On rxvt terminal, the value depends on the environment variable
+COLORFGBG.")
 
 (defun frame-set-background-mode (frame)
   "Set up display-dependent faces on FRAME.
@@ -1587,13 +1593,13 @@
 		 (intern (downcase bg-resource)))
 		((and (null window-system) (null bg-color))
 		 ;; No way to determine this automatically (?).
-		 'dark)
+		 (or default-frame-background-mode 'dark))
 		;; Unspecified frame background color can only happen
 		;; on tty's.
 		((member bg-color '(unspecified "unspecified-bg"))
-		 'dark)
+		 (or default-frame-background-mode 'dark))
 		((equal bg-color "unspecified-fg") ; inverted colors
-		 'light)
+		 (if (eq default-frame-background-mode 'light) 'dark 'light))
 		((>= (apply '+ (x-color-values bg-color frame))
 		    ;; Just looking at the screen, colors whose
 		    ;; values add up to .6 of the white total

Index: lisp/startup.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/startup.el,v
retrieving revision 1.359
diff -u -r1.359 startup.el
--- lisp/startup.el	17 Jun 2005 15:34:39 -0000	1.359
+++ lisp/startup.el	27 Jun 2005 00:00:22 -0000
@@ -444,24 +444,25 @@
 	      ;; frame-notice-user-settings didn't (such as on a tty).
 	      ;; frame-set-background-mode is idempotent, so it won't
 	      ;; cause any harm if it's already been done.
-	      (let ((frame-background-mode frame-background-mode)
-		    (frame (selected-frame))
+	      (let ((frame (selected-frame))
 		    term)
 		(when (and (null window-system)
 			   ;; Don't override a possibly customized value.
 			   (null frame-background-mode)
-			   ;; Don't override user specifications.
-			   (null (frame-parameter frame 'reverse))
+			   ;; Don't override a default set by terminal.
+			   (null default-frame-background-mode)
 			   (let ((bg (frame-parameter frame 'background-color)))
 			     (or (null bg)
-				 (member bg '(unspecified "unspecified-bg")))))
+				 (member bg '(unspecified "unspecified-bg"
+							  "unspecified-fg")))))
+
 		  (setq term (getenv "TERM"))
 		  ;; Some files in lisp/term do a better job with the
 		  ;; background mode, but we leave this here anyway, in
 		  ;; case they remove those files.
 		  (if (string-match "^\\(xterm\\|rxvt\\|dtterm\\|eterm\\)"
 				    term)
-		      (setq frame-background-mode 'light)))
+		      (setq default-frame-background-mode 'light)))
 		(frame-set-background-mode (selected-frame)))))
 
 	;; Now we know the user's default font, so add it to the menu.

Index: lisp/term/xterm.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/term/xterm.el,v
retrieving revision 1.16
diff -u -r1.16 xterm.el
--- lisp/term/xterm.el	12 May 2005 01:35:20 -0000	1.16
+++ lisp/term/xterm.el	27 Jun 2005 00:00:33 -0000
@@ -366,7 +366,7 @@
   "Set background mode as appropriate for the default rxvt colors."
   (let ((fgbg (getenv "COLORFGBG"))
 	bg rgb)
-    (setq frame-background-mode 'light)	; default
+    (setq default-frame-background-mode 'light)
     (when (and fgbg
 	       (string-match ".*;\\([0-9][0-9]?\\)\\'" fgbg))
       (setq bg (string-to-number (substring fgbg (match-beginning 1))))
@@ -379,7 +379,7 @@
 	     ;; The following line assumes that white is the 15th
 	     ;; color in xterm-standard-colors.
 	     (* (apply '+ (car (cddr (nth 15 xterm-standard-colors)))) 0.6))
-	  (setq frame-background-mode 'dark)))
+	  (setq default-frame-background-mode 'dark)))
     (frame-set-background-mode (selected-frame))))
 
 ;; Do it!

Index: lisp/term/rxvt.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/term/rxvt.el,v
retrieving revision 1.4
diff -u -r1.4 rxvt.el
--- lisp/term/rxvt.el	1 Sep 2003 15:45:36 -0000	1.4
+++ lisp/term/rxvt.el	27 Jun 2005 00:00:33 -0000
@@ -150,7 +150,7 @@
   "Set background mode as appropriate for the default rxvt colors."
   (let ((fgbg (getenv "COLORFGBG"))
 	bg rgb)
-    (setq frame-background-mode 'light)	; default
+    (setq default-frame-background-mode 'light)
     (when (and fgbg
 	       (string-match ".*;\\([0-9][0-9]?\\)\\'" fgbg))
       (setq bg (string-to-number (substring fgbg (match-beginning 1))))
@@ -163,7 +163,7 @@
 	     ;; The following line assumes that white is the 15th
 	     ;; color in rxvt-standard-colors.
 	     (* (apply '+ (car (cddr (nth 15 rxvt-standard-colors)))) 0.6))
-	  (setq frame-background-mode 'dark)))
+	  (setq default-frame-background-mode 'dark)))
     (frame-set-background-mode (selected-frame))))
 
 ;; Do it!

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Background mode

[Richard M. Stallman]

Thanks for working on this.

When sending patches here, would you please use diff -c, not diff -u?
I find I cannot understand a diff -u patch unless it is very simple.
When I try to read the lines and think about what they do,
I cannot keep track of which lines are old and which are new.

However, after some struggling, I was able to read this change.
It seems like a good idea, so please install it.  I think it should
be mentioned in NEWS, and maybe somewhere in a manual, but I am
not sure where.

Re: Background mode

[Juri Linkov]

> When sending patches here, would you please use diff -c, not diff -u?
> I find I cannot understand a diff -u patch unless it is very simple.

I think diff -u is better when the goal is to show just the
differences, but diff -c is better for changes in the middle of
non-trivial code.

> When I try to read the lines and think about what they do,
> I cannot keep track of which lines are old and which are new.

Maybe it will help to send patches as inline text/x-patch attachments?
Such attachments are highlighted by diff-mode in the article buffer
(at least in Gnus), so after customizing diff mode faces it would be
easier to see which lines are old or new.

> However, after some struggling, I was able to read this change.

OK, I will remember to send patches with diff -c.

> It seems like a good idea, so please install it.  I think it should
> be mentioned in NEWS, and maybe somewhere in a manual, but I am not
> sure where.

Since `default-frame-background-mode' is not a user option, but an
internal variable, then maybe it should be mentioned under the `Lisp
Changes in Emacs 22.1' section of etc/NEWS?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Juri Linkov]

>    For quite a long time (most likely from the time when color translation
>    was implemented for tty) colors of context lines in diff mode on tty
>    were yellow for light backgrounds.  There are also other faces with
>    yellow foreground for light backgrounds, like font-lock-variable-name-face.
>    Nobody complained about them so far.  So maybe yellow is the acceptable
>    color for context lines.
>
> I remember seeing people complain that they have trouble reading
> yellow on white _many_ times on this list.  I know of many people who
> have trouble reading red on black.  I have trouble with both and I
> have even more problems reading cyan on white.  I do not complain, I
> just always turn colors off on tty's.

Something has to be done about colors of diff context, because
currently it is white on black for dark backgrounds, and yellow
on white for light backgrounds.

Yellow on black would be good for dark backgrounds, but what to put as
a foreground color for light backgrounds.  Maybe green on white?
Green is a dark color, so on white it is well visible.

BTW, the formula for calculating the background mode produces different
results for different terminals:

On TTY it classifies colors as follows:

Dark:  black red green blue
Light: magenta yellow cyan white

But on xterm it is quite different:

Dark:  black red green blue magenta yellow cyan
Light: white

According to this classification, yellow is a dark color,
as it were suitable as a foreground color for light backgrounds.

OTOH, the formula I proposed in January produces the same
classification on TTY, xterm and X as:

Dark:  black red green blue magenta
Light: yellow cyan white

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Background mode

[Juri Linkov]

I discovered another bug.  On non-window terminals when the background
color of some face is specified, but its foreground is not (and vice versa),
then the inverted face uses the wrong color.  These situations are described
by the following excerpts from the face customization buffer:

1.
             [X] Foreground: blue       (sample)
             [X] Background: unspecified-fg (sample)

Currently this sets the face background to blue, and the foreground
to unspecified-bg, i.e. it inverts the unspecified colors (function
toggle_highlight), but puts the specified color (blue) on the wrong
face attribute.

2.
             [X] Foreground: unspecified-bg (sample)
             [X] Background: blue       (sample)

This sets the foreground to blue, and the background to unspecified-fg.

The patch takes care of these cases and uses the correct colors.

Index: src/term.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/term.c,v
retrieving revision 1.162
diff -c -r1.162 term.c
*** src/term.c	17 Jun 2005 14:08:58 -0000	1.162
--- src/term.c	27 Jun 2005 23:27:14 -0000
***************
*** 2005,2018 ****
  
        if (fg >= 0 && TS_set_foreground)
  	{
! 	  p = tparam (TS_set_foreground, NULL, 0, (int) fg);
  	  OUTPUT (p);
  	  xfree (p);
  	}
  
        if (bg >= 0 && TS_set_background)
  	{
! 	  p = tparam (TS_set_background, NULL, 0, (int) bg);
  	  OUTPUT (p);
  	  xfree (p);
  	}
--- 2005,2024 ----
  
        if (fg >= 0 && TS_set_foreground)
  	{
! 	  if (inverse_video || bg == FACE_TTY_DEFAULT_FG_COLOR)
! 	    p = tparam (TS_set_background, NULL, 0, (int) fg);
! 	  else
! 	    p = tparam (TS_set_foreground, NULL, 0, (int) fg);
  	  OUTPUT (p);
  	  xfree (p);
  	}
  
        if (bg >= 0 && TS_set_background)
  	{
! 	  if (inverse_video || fg == FACE_TTY_DEFAULT_BG_COLOR)
! 	    p = tparam (TS_set_foreground, NULL, 0, (int) bg);
! 	  else
! 	    p = tparam (TS_set_background, NULL, 0, (int) bg);
  	  OUTPUT (p);
  	  xfree (p);
  	}

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Miles Bader]

On 6/28/05, Juri Linkov <juri@jurta.org> wrote:
> Something has to be done about colors of diff context, because
> currently it is white on black for dark backgrounds, and yellow
> on white for light backgrounds.

The diff-mode colors on a dark-background terminal (gnome-terminal or
generic xterm) are:

   Changed lines:  yellow+bold on black
   Context:            white/grey on black
   Headers:          green/blue+bold on black

These colors work very well and should not be changed

On the other hand, I find the terminal _light-background_ colors
almost unreadable.  Certainly the "bold yellow" foreground used in
headers is absurd on a white background.  In general changed
foreground colors seem to work less well against a light background
(perhaps because most terminal colors are fairly bright and don't
contrast well against a white background) so perhaps it would be
better to use the default [black] foreground for the context/changed
text, and rely on making the changed lines bold.

-Miles
-- 
Do not taunt Happy Fun Ball.

Re: Diff mode faces

[Randal L. Schwartz]

>>>>> "Juri" == Juri Linkov <juri@jurta.org> writes:

Juri> Something has to be done about colors of diff context, because
Juri> currently it is white on black for dark backgrounds, and yellow
Juri> on white for light backgrounds.

Yes, my original complaint. :)

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: Background mode

[Richard M. Stallman]

    I think diff -u is better when the goal is to show just the
    differences, but diff -c is better for changes in the middle of
    non-trivial code.

Different people have different preferences, but I need diff -c.
diff -u is always hard for me.

    Since `default-frame-background-mode' is not a user option, but an
    internal variable, then maybe it should be mentioned under the `Lisp
    Changes in Emacs 22.1' section of etc/NEWS?

I didn't know it was an internal variable.  That being so,
this shouldn't be mentioned in NEWS.  The only user-level impact
of this is to fix a bug, and we don't mention bug fixes in NEWS.

Re: Background mode

[Stefan Monnier]

>     I think diff -u is better when the goal is to show just the
>     differences, but diff -c is better for changes in the middle of
>     non-trivial code.

> Different people have different preferences, but I need diff -c.
> diff -u is always hard for me.

Note that M-x load-libray RET diff-mode RET gives you the commands

     M-x diff-context->unified
and
     M-x diff-unified->context

so you can turn one format into the other.  I usually prefer the -u
format, but occasionally like to switch some hunks to -c.


        Stefan

Re: Background mode

[Miles Bader]

2005/6/29, Stefan Monnier <monnier@iro.umontreal.ca>:
>      M-x diff-context->unified
>      M-x diff-unified->context

These are great commands; I've often wished for a diff-minor-mode or
something so I could use the various diff-mode commands on a mail
message in Gnus or something without  copying it.  I don't know how
well that'd work though...

-Miles
-- 
Do not taunt Happy Fun Ball.

Re: Background mode

[Stefan Monnier]

>>>>> "Miles" == Miles Bader <snogglethorpe@gmail.com> writes:

> 2005/6/29, Stefan Monnier <monnier@iro.umontreal.ca>:
>> M-x diff-context->unified
>> M-x diff-unified->context

> These are great commands; I've often wished for a diff-minor-mode or
> something so I could use the various diff-mode commands on a mail
> message in Gnus or something without  copying it.  I don't know how
> well that'd work though...

If you google for old versions of diff-mode.el, you'll see that it also had
a minor mode.  IIRC I removed it because it didn't seem very useful.


        Stefan

Re: Diff mode faces

[Richard M. Stallman]

    Something has to be done about colors of diff context, because
    currently it is white on black for dark backgrounds, and yellow
    on white for light backgrounds.

I don't see any face in diff-mode.el that produces yellow-on-white.
So I don't understand what specific behavior you're talking about
here.

Re: Diff mode faces

[Richard M. Stallman]

    On TTY it classifies colors as follows:

    Dark:  black red green blue
    Light: magenta yellow cyan white

    But on xterm it is quite different:

    Dark:  black red green blue magenta yellow cyan
    Light: white

That anomaly could be worth fixing.

    According to this classification, yellow is a dark color,
    as it were suitable as a foreground color for light backgrounds.

I think that is a non-issue.  Emacs never chooses foreground colors
based on such a classification.  The only thing Eacs does based on
this classification is to set the background mode.  So this
classification has no relevance for anything else.

    OTOH, the formula I proposed in January produces the same
    classification on TTY, xterm and X as:

    Dark:  black red green blue magenta
    Light: yellow cyan white

Could you propose it again now?

Re: Background mode

[Richard M. Stallman]

    If you google for old versions of diff-mode.el, you'll see that it also had
    a minor mode.

Why suggest using a search engine to find old versions of a file in Emacs?
They are all on savannah.

Re: Diff mode faces

[Eli Zaretskii]

> From: "Richard M. Stallman" <rms@gnu.org>
> CC: teirllm@dms.auburn.edu, eliz@gnu.org, emacs-devel@gnu.org,
> 	merlyn@stonehenge.com
> Date: Thu, 30 Jun 2005 17:30:23 -0400
> 
>     On TTY it classifies colors as follows:
> 
>     Dark:  black red green blue
>     Light: magenta yellow cyan white
> 
>     But on xterm it is quite different:
> 
>     Dark:  black red green blue magenta yellow cyan
>     Light: white
> 
> That anomaly could be worth fixing.

I think that's not an anomaly, but a bug.

Juri, can you explain how did this happen?  If that's an 8-color
xterm, then the code in tty-color-values should not distinguish
between xterm and any other 8-color TTY.  If the xterm in question
supports more than 8 colors, please tell the details, including how
many colors it supports, how were the colors set up, and how did you
test the above classification.

>     OTOH, the formula I proposed in January produces the same
>     classification on TTY, xterm and X as:
> 
>     Dark:  black red green blue magenta
>     Light: yellow cyan white
> 
> Could you propose it again now?

A formula that classifies red and magenta the same on X and on a TTY
is probably not very good, since the TTY's "red" is much darker that
the X's "red".  See the WARNING in the comments near the beginning of
tty-colors.el, for more about this.

Re: Background mode

[Stefan Monnier]

>     If you google for old versions of diff-mode.el, you'll see that it
>     also had a minor mode.

> Why suggest using a search engine to find old versions of a file in Emacs?
> They are all on savannah.

IIRC the minor mode was only present in diff-mode versions prior to
inclusion in Emacs.  But a rapid check shows that I actually haven't removed
diff-minor-mode at all.  It's still there alright.


        Stefan

Re: Diff mode faces

[Juri Linkov]

>     According to this classification, yellow is a dark color,
>     as it were suitable as a foreground color for light backgrounds.
>
> I think that is a non-issue.  Emacs never chooses foreground colors
> based on such a classification.  The only thing Eacs does based on
> this classification is to set the background mode.  So this
> classification has no relevance for anything else.

Sorry, I mixed foreground and background.  That sentence should read
as follows:

    According to this classification, yellow is a dark color, as
    suitable background color for a light foreground color.

>     OTOH, the formula I proposed in January produces the same
>     classification on TTY, xterm and X as:
>
>     Dark:  black red green blue magenta
>     Light: yellow cyan white
>
> Could you propose it again now?

(let ((bg-color-values (x-color-values bg-color))
      (white-values (x-color-values "white")))
  (>= (+ (* (nth 0 bg-color-values) 0.30)
	 (* (nth 1 bg-color-values) 0.59)
	 (* (nth 2 bg-color-values) 0.11))
      (* (+ (* (nth 0 white-values) 0.30)
	    (* (nth 1 white-values) 0.59)
	    (* (nth 2 white-values) 0.11))
	 .6)))

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Juri Linkov]

>>     On TTY it classifies colors as follows:
>> 
>>     Dark:  black red green blue
>>     Light: magenta yellow cyan white
>> 
>>     But on xterm it is quite different:
>> 
>>     Dark:  black red green blue magenta yellow cyan
>>     Light: white
>> 
>> That anomaly could be worth fixing.
>
> I think that's not an anomaly, but a bug.
>
> Juri, can you explain how did this happen?  If that's an 8-color
> xterm, then the code in tty-color-values should not distinguish
> between xterm and any other 8-color TTY.  If the xterm in question
> supports more than 8 colors, please tell the details, including how
> many colors it supports, how were the colors set up, and how did you
> test the above classification.

It is 8-color xterm, and according to color mapping `xterm-standard-colors'
in term/xterm.el `yellow' has color values of `yellow3' (52685 52685 0),
`cyan' - values of `cyan3' (0 52685 52685), and so on.  Many colors on
xterm have values darker than the colors with the same names in the RGB
table.

But on TTY, color values are the same as on X, so `yellow' is (65535 65535 0),
`cyan' is (0 65535 65535).  (Even `yellow3' has the same color value as
`yellow' (65535 65535 0), but this probably doesn't matter on TTY).

> A formula that classifies red and magenta the same on X and on a TTY
> is probably not very good, since the TTY's "red" is much darker that
> the X's "red".  See the WARNING in the comments near the beginning of
> tty-colors.el, for more about this.

xterm's `red' (which actually has the values of `red3') is darker than
the X's `red', but both `red' and `red3' are classified as dark colors
on TTY, xterm and X, so there is no problem with it.

The situation is different for `yellow3' and `cyan3'.  `yellow3' is
classified as a dark color, but it really looks like a light color.
White on yellow3 is still not readable, since this is a
light-foreground/light-background combination.

My formula puts `yellow3' and `cyan3' into the light category.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Juri Linkov]

>> Something has to be done about colors of diff context, because
>> currently it is white on black for dark backgrounds, and yellow
>> on white for light backgrounds.
>
> The diff-mode colors on a dark-background terminal (gnome-terminal or
> generic xterm) are:
>
>    Changed lines:  yellow+bold on black
>    Context:            white/grey on black
>    Headers:          green/blue+bold on black
>
> These colors work very well and should not be changed

Bold text is unreadable on terminals with small fonts.

> On the other hand, I find the terminal _light-background_ colors
> almost unreadable.  Certainly the "bold yellow" foreground used in
> headers is absurd on a white background.  In general changed
> foreground colors seem to work less well against a light background
> (perhaps because most terminal colors are fairly bright and don't
> contrast well against a white background) so perhaps it would be
> better to use the default [black] foreground for the context/changed
> text, and rely on making the changed lines bold.

The changed lines is the base text which users mostly read in the
diff-mode, so it makes sense not to highlight it at all and to use the
default foreground.  Most other modes do the same and don't highlight
normal text.  As for the context lines, to distinguish them from the
changed lines, dark-background terminals could use yellow-on-black,
and light-background terminals - maybe, green.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Miles Bader]

2005/7/2, Juri Linkov <juri@jurta.org>:
> > The diff-mode colors on a dark-background terminal (gnome-terminal or
> > generic xterm) are:
...
> > These colors work very well and should not be changed
> 
> Bold text is unreadable on terminals with small fonts.

I have not found this to be the case.

> The changed lines is the base text which users mostly read in the
> diff-mode, so it makes sense not to highlight it at all and to use the
> default foreground.  Most other modes do the same and don't highlight
> normal text.  As for the context lines, to distinguish them from the
> changed lines, dark-background terminals could use yellow-on-black,
> and light-background terminals - maybe, green.

No.

Indeed that's just stupid.  Making the context lines green or whatever
would make them stand out over the changed lines, which is _precisely
the opposite_ of what we want to do.

On a high-color display (like X11, usually), we can dim the context
lines, and so achieve our goal of making the changed lines stand out
while keeping them in the normal default face; the latter point,
however, is _not_ the main goal!  On a low-color terminal, we often
simply don't have the flexibility to do those two things at the same
time; in such a case, we must discard the less important guideline,
and pick something which at least makes the important lines stand out.
  The current dark-background colors achieve this, and should not be
changed.

-Miles
-- 
Do not taunt Happy Fun Ball.

xterm colors (was: Diff mode faces)

[Gaëtan LEURENT]

Juri Linkov wrote on 02 Jul 2005 01:59:19 +0200:

> It is 8-color xterm, and according to color mapping `xterm-standard-colors'
> in term/xterm.el `yellow' has color values of `yellow3' (52685 52685 0),
> `cyan' - values of `cyan3' (0 52685 52685), and so on.  Many colors on
> xterm have values darker than the colors with the same names in the RGB
> table.

The colors used by xterm can be changed through X resources, so the
colors defined in xterm-standard-colors are maybe not the ones that are
actually used. I don't know if we should do anything too smart with
them.

Or do you expect users to modify xterm-standard-colors if they change
their xterm colors ?

If this point have already been under discussion, I'll happily accept a
reference ...

-- 
Gaëtan LEURENT

Re: xterm colors (was: Diff mode faces)

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>, rms@gnu.org, emacs-devel@gnu.org
> From: gaetan.leurent@ens.fr (=?iso-8859-1?Q?Ga=EBtan?= LEURENT)
> Date: Sun, 03 Jul 2005 20:56:08 +0200
> 
> Or do you expect users to modify xterm-standard-colors if they change
> their xterm colors ?

Yes, that's what they should do.  But if there's a simple way for
Emacs to retrieve the xterm colors and update xterm-standard-colors
automatically, I have no problems with adding such a feature.  We just
need to make sure we DTRT with programs that pretend to be xterm (like
rxvt, for example).

Re: xterm colors (was: Diff mode faces)

[Richard M. Stallman]

    The colors used by xterm can be changed through X resources, so the
    colors defined in xterm-standard-colors are maybe not the ones that are
    actually used. I don't know if we should do anything too smart with
    them.

    Or do you expect users to modify xterm-standard-colors if they change
    their xterm colors ?

Since there is no way for Emacs to respond automatically to such
customization of Xterm, let's not worry about it.  There are many ways
they can customize Emacs for this.

Re: Diff mode faces

[Richard M. Stallman]

Would you please make whatever change you think is good
for the diff-header face in the case of a light background
and few colors supported?

Re: Diff mode faces

[Juri Linkov]

> Would you please make whatever change you think is good
> for the diff-header face in the case of a light background
> and few colors supported?

Are there any objections to changing context fg/bg to the default fg/bg
and file header's yellow foreground to green on light background tty?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Miles Bader]

2005/7/7, Juri Linkov <juri@jurta.org>:
> Are there any objections to changing context fg/bg to the default fg/bg

Yes; that is a bad change.  I went into detail in a previous message;
did you not read it?

-Miles
-- 
Do not taunt Happy Fun Ball.

Re: Diff mode faces

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Cc: snogglethorpe@gmail.com, miles@gnu.org, eliz@gnu.org, merlyn@stonehenge.com, teirllm@dms.auburn.edu, emacs-devel@gnu.org
> Date: Wed, 06 Jul 2005 23:53:53 +0300
> 
> Are there any objections to changing context fg/bg to the default fg/bg
> and file header's yellow foreground to green on light background tty?

Please post the suggested diffs, I'd like to try that before I make up
my mind about this proposal.

Re: Diff mode faces

[Juri Linkov]

>> Are there any objections to changing context fg/bg to the default fg/bg
>
> Yes; that is a bad change.  I went into detail in a previous message;
> did you not read it?

Then I don't understand why changing yellow context foreground to the
default black foreground is a bad change if everyone complained that
yellow-on-white color combination is not readable.  Do you have
a better suggestion for the foreground color of context lines,
other than current yellow and proposed black?  [Note that the
color in question is for context lines, not for changed lines.]

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Juri Linkov]

>> Are there any objections to changing context fg/bg to the default fg/bg
>> and file header's yellow foreground to green on light background tty?
>
> Please post the suggested diffs, I'd like to try that before I make up
> my mind about this proposal.

Index: lisp/diff-mode.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/diff-mode.el,v
retrieving revision 1.78
diff -u -r1.78 diff-mode.el
--- lisp/diff-mode.el	4 Jul 2005 23:08:52 -0000	1.78
+++ lisp/diff-mode.el	9 Jul 2005 20:09:20 -0000
@@ -197,7 +197,7 @@
     (((class color) (min-colors 88) (background dark))
      :background "grey60" :weight bold)
     (((class color) (background light))
-     :foreground "yellow" :weight bold)
+     :foreground "green" :weight bold)
     (((class color) (background dark))
      :foreground "cyan" :weight bold)
     (t :weight bold))			; :height 1.3
@@ -259,7 +280,8 @@
 (defvar diff-function-face 'diff-function)
 
 (defface diff-context
-  '((t :inherit shadow))
+  '((((type tty pc) (class color)))
+    (t :inherit shadow))
   "`diff-mode' face used to highlight context and other side-information."
   :group 'diff-mode)
 ;; backward-compatibility alias

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 09 Jul 2005 23:56:19 +0300
> 
>  (defface diff-context
> -  '((t :inherit shadow))
> +  '((((type tty pc) (class color)))
> +    (t :inherit shadow))
>    "`diff-mode' face used to highlight context and other side-information."
>    :group 'diff-mode)
>  ;; backward-compatibility alias

This part affects non-TTY platforms, while leaving the TTY colors
intact.  Why?  I don't think there's any problem with Diff-mode colors
on displays that support many colors, are there?

I will try the suggested colors on a TTY later, when I have time.

Re: Diff mode faces

[Juri Linkov]

> This part affects non-TTY platforms, while leaving the TTY colors
> intact.  Why?  I don't think there's any problem with Diff-mode colors
> on displays that support many colors, are there?

Maybe the following patch is more correct:

Index: lisp/diff-mode.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/diff-mode.el,v
retrieving revision 1.78
diff -u -r1.78 diff-mode.el
--- lisp/diff-mode.el	4 Jul 2005 23:08:52 -0000	1.78
+++ lisp/diff-mode.el	11 Jul 2005 00:06:51 -0000
@@ -197,7 +197,7 @@
     (((class color) (min-colors 88) (background dark))
      :background "grey60" :weight bold)
     (((class color) (background light))
-     :foreground "yellow" :weight bold)
+     :foreground "green" :weight bold)
     (((class color) (background dark))
      :foreground "cyan" :weight bold)
     (t :weight bold))			; :height 1.3
@@ -259,7 +280,7 @@
 (defvar diff-function-face 'diff-function)
 
 (defface diff-context
-  '((t :inherit shadow))
+  '((((class color) (min-colors 88)) :inherit shadow))
   "`diff-mode' face used to highlight context and other side-information."
   :group 'diff-mode)
 ;; backward-compatibility alias

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Stefan Monnier]

>  (defface diff-context
> -  '((t :inherit shadow))
> +  '((((class color) (min-colors 88)) :inherit shadow))
>    "`diff-mode' face used to highlight context and other side-information."
>    :group 'diff-mode)

Maybe the shadow face should be fixed instead.


        Stefan

Re: Diff mode faces

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 11 Jul 2005 09:43:07 -0400
> 
> Maybe the shadow face should be fixed instead.

Why?  It does what it's supposed to do: makes the text barely visible.

Re: Diff mode faces

[Stefan Monnier]

>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Mon, 11 Jul 2005 09:43:07 -0400
>> 
>> Maybe the shadow face should be fixed instead.

> Why?  It does what it's supposed to do: makes the text barely visible.

Then why isn't it good for the `diff-context' face?

Or more specifically, why is it only good for `diff-context' on color
terminals with more than 88 colors?  It should work just fine on gray-level
terminals and if the result is completely unreadable on terminals with less
than 88 colors, thenwhy shouldn't that problem also be fixed for the
`shadow' face?


        Stefan

Re: Diff mode faces

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 11 Jul 2005 15:46:43 -0400
> Cc: juri@jurta.org, emacs-devel@gnu.org
> 
> >> Maybe the shadow face should be fixed instead.
> 
> > Why?  It does what it's supposed to do: makes the text barely visible.
> 
> Then why isn't it good for the `diff-context' face?

Well, I didn't suggest to change it in the first place, so I really am
not the man to ask.  I guess the idea is that diff context needs to be
readable after all.

> Or more specifically, why is it only good for `diff-context' on color
> terminals with more than 88 colors?

Probably because, with more than 88 colors, the shadow face is more
readable than the current color on 16- and 8-color tty's.

> It should work just fine on gray-level terminals

Yes, those should need a separate definition, if we change the current
one.

> and if the result is completely unreadable on terminals with less
> than 88 colors, thenwhy shouldn't that problem also be fixed for the
> `shadow' face?

It's not ``completely unreadable'', at least not in my opinion.

Re: Diff mode faces

[Juri Linkov]

>>  (defface diff-context
>> -  '((t :inherit shadow))
>> +  '((((class color) (min-colors 88)) :inherit shadow))
>>    "`diff-mode' face used to highlight context and other side-information."
>>    :group 'diff-mode)
>
> Maybe the shadow face should be fixed instead.

The shadow face should be fixed too (and perhaps not `instead').
Such a fix will affect faces which inherit from the shadow face
(like `tmm-inactive', `widget-inactive').

But as I understood from the discussion, `diff-mode' is exceptional:
on terminals where shades of grey are not available, it should
highlight changed lines, and not highlight context lines.

What makes `diff-mode' different from other modes with faces
inheriting from the shadow face, is that `diff-mode' has a special
face for changed lines (which currently is set to the color different
from the default foreground color on terminals), whereas other modes
have no special faces for highlighting the normal text (i.e. the text
other than highlighted with the shadow face).

Here is a new patch:

Index: lisp/faces.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/faces.el,v
retrieving revision 1.329
diff -u -r1.329 faces.el
--- lisp/faces.el	4 Jul 2005 23:08:54 -0000	1.329
+++ lisp/faces.el	12 Jul 2005 06:33:20 -0000
@@ -2149,8 +2148,10 @@
   :version "22.1")
 
 (defface shadow
-  '((((background dark))  :foreground "grey70")
-    (((background light)) :foreground "grey50"))
+  '((((min-colors 88) (background light)) :foreground "grey50")
+    (((min-colors 88) (background dark))  :foreground "grey70")
+    (((background light)) :foreground "green")
+    (((background dark)) :foreground "yellow"))
   "Basic face for shadowed text."
   :group 'basic-faces
   :version "22.1")

Index: lisp/diff-mode.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/diff-mode.el,v
retrieving revision 1.78
diff -u -r1.78 diff-mode.el
--- lisp/diff-mode.el	4 Jul 2005 23:08:52 -0000	1.78
+++ lisp/diff-mode.el	12 Jul 2005 06:35:03 -0000
@@ -259,7 +280,7 @@
 (defvar diff-function-face 'diff-function)
 
 (defface diff-context
-  '((t :inherit shadow))
+  '((((min-colors 88)) :inherit shadow))
   "`diff-mode' face used to highlight context and other side-information."
   :group 'diff-mode)
 ;; backward-compatibility alias

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff mode faces

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Cc: eliz@gnu.org, emacs-devel@gnu.org
> Date: Tue, 12 Jul 2005 09:51:05 +0300
> 
> Here is a new patch:

I tried that on a tty.  I'm mildly worried about the visibility of
green on (the default) white background: is it possible that it will
stand out too much?  If it's possible, then this change is not good,
since `shadow' should not stand out.

But in my case, it didn't stand out, so I have no objections for this
change.

> Index: lisp/faces.el
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/lisp/faces.el,v
> retrieving revision 1.329
> diff -u -r1.329 faces.el
> --- lisp/faces.el	4 Jul 2005 23:08:54 -0000	1.329
> +++ lisp/faces.el	12 Jul 2005 06:33:20 -0000
> @@ -2149,8 +2148,10 @@
>    :version "22.1")
>  
>  (defface shadow
> -  '((((background dark))  :foreground "grey70")
> -    (((background light)) :foreground "grey50"))
> +  '((((min-colors 88) (background light)) :foreground "grey50")
> +    (((min-colors 88) (background dark))  :foreground "grey70")
> +    (((background light)) :foreground "green")
> +    (((background dark)) :foreground "yellow"))
>    "Basic face for shadowed text."
>    :group 'basic-faces
>    :version "22.1")
> 
> Index: lisp/diff-mode.el
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/lisp/diff-mode.el,v
> retrieving revision 1.78
> diff -u -r1.78 diff-mode.el
> --- lisp/diff-mode.el	4 Jul 2005 23:08:52 -0000	1.78
> +++ lisp/diff-mode.el	12 Jul 2005 06:35:03 -0000
> @@ -259,7 +280,7 @@
>  (defvar diff-function-face 'diff-function)
>  
>  (defface diff-context
> -  '((t :inherit shadow))
> +  '((((min-colors 88)) :inherit shadow))
>    "`diff-mode' face used to highlight context and other side-information."
>    :group 'diff-mode)
>  ;; backward-compatibility alias