bug#46396: 27.1.90; Ediff's non-focused diff section, background too light in --reverse

bug#46396: 27.1.90; Ediff's non-focused diff section, background too light in --reverse

[bug-gnu-emacs_at_gnu.org]

I created a file a.txt with foo\n\nbar\n\nbaz\n and a file b.txt with foo0\n\nbar\n\nbaz1\n.

I open these files with "emacs -Q --reverse a.txt b.txt", and run M-x ediff-buffers RET for the two corresponding buffers.

If I press n or p to traverse the diff sections, the diff section in focus is readable, but the unfocused sections have a white background, in other words, the contrast seems too low. The unfocused sections are fine without the --reverse option.

Aside: Similar to --reverse, I have been using (invert-face 'default) for a simple dark theme in emacs. I can get a similar result in terminal emacs with "(setq frame-background-mode 'dark) (mapc 'frame-set-background-mode (frame-list))". These approaches to a simple dark theme came up in discussions on #emacs.

I encountered the issue above, as I was using magit, with (invert-face 'default), and pressed `e` on a commit, wanting to view the diff in the side-by-side view.

Thank you,
-Brady

Data from M-x report-emacs-bug RET below:

In GNU Emacs 27.1.90 (build 1, x86_64-apple-darwin19.6.0, NS appkit-1894.60 Version 10.15.7 (Build 19H2))
of 2021-01-15 built on clay.local
Repository revision: 488204cdc64b6a130042ecc64d59c4538287b81d
Repository branch: emacs-27
Windowing system distributor 'Apple', version 10.3.1894
System Description:  Mac OS X 10.15.7

Recent messages:
Computing differences between a.txt and b.txt ...
Buffer A: Processing difference region 0 of 2
Buffer B: Processing difference region 0 of 2
Processing difference regions ... done
Refining difference region 1 ...
Refining difference region 2 ...
Quit this Ediff session? (y or n) y
Making completion list... [5 times]
completing-read-default: Command attempted to use minibuffer while in minibuffer
Making completion list...

Configured using:
'configure --with-rsvg'

Configured features:
RSVG GLIB NOTIFY KQUEUE ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS
MODULES THREADS JSON PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Text

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils ediff
ediff-merg ediff-mult ediff-wind ediff-diff ediff-help ediff-init
cl-loaddefs cl-lib ediff-util tooltip eldoc electric uniquify ediff-hook
vc-hooks lisp-float-type mwheel term/ns-win ns-win ucs-normalize
mule-util term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads kqueue cocoa ns lcms2 multi-tty make-network-process
emacs)

Memory information:
((conses 16 51551 8621)
(symbols 48 6696 1)
(strings 32 17852 1647)
(string-bytes 1 613638)
(vectors 16 10753)
(vector-slots 8 134394 11440)
(floats 8 34 173)
(intervals 56 229 0)
(buffers 1000 17))

bug#46396: 27.1.90; Ediff's non-focused diff section, background too light in --reverse

[Matt Armstrong]

bug-gnu-emacs_at_gnu.org@tangential.info writes:

> I created a file a.txt with foo\n\nbar\n\nbaz\n and a file b.txt with foo0\n\nbar\n\nbaz1\n.
>
> I open these files with "emacs -Q --reverse a.txt b.txt", and run M-x
> ediff-buffers RET for the two corresponding buffers.
>
> If I press n or p to traverse the diff sections, the diff section in focus
> is readable, but the unfocused sections have a white background, in other
> words, the contrast seems too low. The unfocused sections are fine without
> the --reverse option.
>
> Aside: Similar to --reverse, I have been using (invert-face 'default) for a
> simple dark theme in emacs. I can get a similar result in terminal emacs
> with "(setq frame-background-mode 'dark) (mapc 'frame-set-background-mode
> (frame-list))". These approaches to a simple dark theme came up in
> discussions on #emacs.
>
> I encountered the issue above, as I was using magit, with (invert-face
> 'default), and pressed `e` on a commit, wanting to view the diff in the
> side-by-side view.
>
> Thank you,
> -Brady
>
> Data from M-x report-emacs-bug RET below:
>
> In GNU Emacs 27.1.90 (build 1, x86_64-apple-darwin19.6.0, NS appkit-1894.60 Version 10.15.7 (Build 19H2))
> of 2021-01-15 built on clay.local
> Repository revision: 488204cdc64b6a130042ecc64d59c4538287b81d
> Repository branch: emacs-27
> Windowing system distributor 'Apple', version 10.3.1894
> System Description:  Mac OS X 10.15.7

I can confirm that the colors look bad on my system as well. The problem
does not seem confined to use of --reverse-video. They look bad if I set
the theme to a dark theme through "M-x customize-theme". For example,
the "deeper-blue" and "tango-dark" themes tickle the same problem. I
suspect any theme that sets the foreground color to a light color will
exhibit this.

I notice that one of the bad looking diff faces is `ediff-even-diff-A',
which, on any system with >88 colors, the face fails to specify a
foreground color. Most ediff-* faces are like this.

I wonder if faces should always set foreground/background colors in
pairs?

Anyway, when I expressly set the foreground colors for these faces to
black, they look fine in dark themes.

bug#46396: 27.1.90; Ediff's non-focused diff section, background too light in --reverse

[Matt Armstrong]

Matt Armstrong <matt@rfc20.org> writes:

> bug-gnu-emacs_at_gnu.org@tangential.info writes:
>
>> I created a file a.txt with foo\n\nbar\n\nbaz\n and a file b.txt with foo0\n\nbar\n\nbaz1\n.
>>
>> I open these files with "emacs -Q --reverse a.txt b.txt", and run M-x
>> ediff-buffers RET for the two corresponding buffers.
>>
>> If I press n or p to traverse the diff sections, the diff section in focus
>> is readable, but the unfocused sections have a white background, in other
>> words, the contrast seems too low. The unfocused sections are fine without
>> the --reverse option.

[...]

> I can confirm that the colors look bad on my system as well. The problem
> does not seem confined to use of --reverse-video. They look bad if I set
> the theme to a dark theme through "M-x customize-theme". For example,
> the "deeper-blue" and "tango-dark" themes tickle the same problem. I
> suspect any theme that sets the foreground color to a light color will
> exhibit this.
>
> I notice that one of the bad looking diff faces is `ediff-even-diff-A',
> which, on any system with >88 colors, the face fails to specify a
> foreground color. Most ediff-* faces are like this.
>
> I wonder if faces should always set foreground/background colors in
> pairs?
>
> Anyway, when I expressly set the foreground colors for these faces to
> black, they look fine in dark themes.

On the theory that not setting both foreground and background in a face
is the problem here, I went to see how we ended up in the situation.

The origin of the faces without a foreground setting is commit
382ceb2cdbedc06b06d9fb424d83f531339a3311 by Juri Linkov <juri@jurta.org>
while working on https://debbugs.gnu.org/cgi/bugreport.cgi?bug=10181.

I've cc'd him here. Juri, can you comment?

I'm especially interested to hear your thoughts on why the change made
things nicer (what you said in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=10181#84). It seems like a
user's theme can place the foreground color anywhere between white and
black, so specifying *only* the background color will have some risk
landing on a bad combination.

Or, perhaps the ediff faces should vary the background based on
(background light) and (background dark)?

bug#46396: 27.1.90; Ediff's non-focused diff section, background too light in --reverse

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

> The origin of the faces without a foreground setting is commit
> 382ceb2cdbedc06b06d9fb424d83f531339a3311 by Juri Linkov <juri@jurta.org>
> while working on https://debbugs.gnu.org/cgi/bugreport.cgi?bug=10181.
>
> I've cc'd him here. Juri, can you comment?

Thanks for pointing me at this issue since I had not realized
this is related to something affected by my previous changes.

> I'm especially interested to hear your thoughts on why the change made
> things nicer (what you said in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=10181#84).

The core point of my 2014-year message was that with the specified foreground,
Ediff removes font-lock highlighting on non-current differences.
The need for keeping the font-lock foreground colors is evident
when looking at these font-lock colors in buffers where the effect
of font-lock is noticeable, such as changes in the source code files.

But I completely agree that the current contrast is too low on a dark theme.

Fortunately, now we have a feature that will help to fix this problem:
the face attribute :distant-foreground specifies the alternative
foreground color that is used only when the background color is too
close to the foreground color, thus making the text readable in this case.

> Or, perhaps the ediff faces should vary the background based on
> (background light) and (background dark)?

The background colors could be adjusted as well for dark backgrounds.

After changing background colors to darker colors and adding
:distant-foreground I tried different default foreground colors with

  M-x set-foreground-color RET dark grey RET

and it seems everything looks readable.

Do you see more problems with the following patch?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ediff-dark.patch --]
[-- Type: text/x-diff, Size: 4339 bytes --]

diff --git a/lisp/vc/ediff-init.el b/lisp/vc/ediff-init.el
index 6e658163b9..3f33e6aae2 100644
--- a/lisp/vc/ediff-init.el
+++ b/lisp/vc/ediff-init.el
@@ -980,8 +980,10 @@ stipple-pixmap
 (defface ediff-even-diff-A
   `((((type pc))
      (:foreground "green3" :background "light grey" :extend t))
-    (((class color) (min-colors 88))
-     (:background "light grey" :extend t))
+    (((class color) (min-colors 88) (background light))
+     (:distant-foreground "Black" :background "light grey" :extend t))
+    (((class color) (min-colors 88) (background dark))
+     (:distant-foreground "White" :background "dark grey" :extend t))
     (((class color) (min-colors 16))
      (:foreground "Black" :background "light grey" :extend t))
     (((class color))
@@ -999,8 +1001,10 @@ ediff-even-diff-face-A
 this variable represents.")
 
 (defface ediff-even-diff-B
-  `((((class color) (min-colors 88))
-     (:background "Grey" :extend t))
+  `((((class color) (min-colors 88) (background light))
+     (:distant-foreground "Black" :background "Grey" :extend t))
+    (((class color) (min-colors 88) (background dark))
+     (:distant-foreground "White" :background "dim grey" :extend t))
     (((class color) (min-colors 16))
      (:foreground "White" :background "Grey" :extend t))
     (((class color))
@@ -1019,8 +1023,10 @@ ediff-even-diff-face-B
 (defface ediff-even-diff-C
   `((((type pc))
      (:foreground "yellow3" :background "light grey" :extend t))
-    (((class color) (min-colors 88))
-     (:background "light grey" :extend t))
+    (((class color) (min-colors 88) (background light))
+     (:distant-foreground "Black" :background "light grey" :extend t))
+    (((class color) (min-colors 88) (background dark))
+     (:distant-foreground "White" :background "dark grey" :extend t))
     (((class color) (min-colors 16))
      (:foreground "Black" :background "light grey" :extend t))
     (((class color))
@@ -1040,8 +1046,10 @@ ediff-even-diff-face-C
 (defface ediff-even-diff-Ancestor
   `((((type pc))
      (:foreground "cyan3" :background "light grey" :extend t))
-    (((class color) (min-colors 88))
-     (:background "Grey" :extend t))
+    (((class color) (min-colors 88) (background light))
+     (:distant-foreground "Black" :background "Grey" :extend t))
+    (((class color) (min-colors 88) (background dark))
+     (:distant-foreground "White" :background "dim grey" :extend t))
     (((class color) (min-colors 16))
      (:foreground "White" :background "Grey" :extend t))
     (((class color))
@@ -1068,8 +1076,10 @@ ediff-even-diff-face-alist
 (defface ediff-odd-diff-A
   '((((type pc))
      (:foreground "green3" :background "gray40" :extend t))
-    (((class color) (min-colors 88))
-     (:background "Grey" :extend t))
+    (((class color) (min-colors 88) (background light))
+     (:distant-foreground "Black" :background "Grey" :extend t))
+    (((class color) (min-colors 88) (background dark))
+     (:distant-foreground "White" :background "dim grey" :extend t))
     (((class color) (min-colors 16))
      (:foreground "White" :background "Grey" :extend t))
     (((class color))
@@ -1088,8 +1098,10 @@ ediff-odd-diff-face-A
 (defface ediff-odd-diff-B
   '((((type pc))
      (:foreground "White" :background "gray40" :extend t))
-    (((class color) (min-colors 88))
-     (:background "light grey" :extend t))
+    (((class color) (min-colors 88) (background light))
+     (:distant-foreground "Black" :background "light grey" :extend t))
+    (((class color) (min-colors 88) (background dark))
+     (:distant-foreground "White" :background "dark grey" :extend t))
     (((class color) (min-colors 16))
      (:foreground "Black" :background "light grey" :extend t))
     (((class color))
@@ -1108,8 +1120,10 @@ ediff-odd-diff-face-B
 (defface ediff-odd-diff-C
   '((((type pc))
      (:foreground "yellow3" :background "gray40" :extend t))
-    (((class color) (min-colors 88))
-     (:background "Grey" :extend t))
+    (((class color) (min-colors 88) (background light))
+     (:distant-foreground "Black" :background "Grey" :extend t))
+    (((class color) (min-colors 88) (background dark))
+     (:distant-foreground "White" :background "dim grey" :extend t))
     (((class color) (min-colors 16))
      (:foreground "White" :background "Grey" :extend t))
     (((class color))

bug#46396: 27.1.90; Ediff's non-focused diff section, background too light in --reverse

[Matt Armstrong]

Juri Linkov <juri@jurta.org> writes:


[...]

> After changing background colors to darker colors and adding
> :distant-foreground I tried different default foreground colors with
>
>   M-x set-foreground-color RET dark grey RET
>
> and it seems everything looks readable.
>
> Do you see more problems with the following patch?

[...]

Juri, this is a definite improvement, thank you. Committing it as it is
seems reasonable since it makes unreadable text readable again.

I applied this patch and played around with applying various light and
dark themes.

Some comments:

In the "tango-dark" theme:

 a) the non-current diff backgrounds are quite close to the theme's dark
    background color, so they are not very noticeable.
 b) the current diff (such as `ediff-current-diff-B`) is also quite close
    to the theme's background color.

In the "tango-light" theme:

 c) `ediff-current-diff-A' is quite dark (a darker blue/purple), and the
    foreground color is hard to read over it. This is a little odd,
    because `ediff-current-diff-B' is a fairly bright yellow background.

I saw similar issues in the various other standard themes (loading up
the mater branch with src/emacs -Q).

The issues I see with ediff-current-* faces are arguably a tangent to
this bug. Perhaps those faces could use :distant-foreground as well, for
similar reasons?

As for the background colors possibly being quite similar to the theme's
background, I'm not sure what to do about that.

bug#46396: 27.1.90; Ediff's non-focused diff section, background too light in --reverse

[Juri Linkov]

tags 46396 fixed
close 46396 28.0.50
thanks

>> Do you see more problems with the following patch?
>
> Juri, this is a definite improvement, thank you. Committing it as it is
> seems reasonable since it makes unreadable text readable again.

Thanks for confirming, now committed to master for Emacs 28.

> I applied this patch and played around with applying various light and
> dark themes.
>
> Some comments:
>
> In the "tango-dark" theme:
>
>  a) the non-current diff backgrounds are quite close to the theme's dark
>     background color, so they are not very noticeable.
>  b) the current diff (such as `ediff-current-diff-B`) is also quite close
>     to the theme's background color.
>
> In the "tango-light" theme:
>
>  c) `ediff-current-diff-A' is quite dark (a darker blue/purple), and the
>     foreground color is hard to read over it. This is a little odd,
>     because `ediff-current-diff-B' is a fairly bright yellow background.
>
> I saw similar issues in the various other standard themes (loading up
> the mater branch with src/emacs -Q).
>
> The issues I see with ediff-current-* faces are arguably a tangent to
> this bug. Perhaps those faces could use :distant-foreground as well, for
> similar reasons?

I'm not sure if it's possible to find such default colors that would
work well for all standard themes.  But there is no need to use
:distant-foreground in themes, because the authors of themes can find
the right combination of the default/ediff colors and adjust their themes
accordingly.

> As for the background colors possibly being quite similar to the theme's
> background, I'm not sure what to do about that.

Implementing something like :distant-background is less important than
:distant-foreground, because :distant-foreground is important to
make the text readable, while :distant-background would only help
making the background color differ from the default background color.

bug#46396: 27.1.90; Ediff's non-focused diff section, background too light in --reverse

[Matt Armstrong]

Juri Linkov <juri@jurta.org> writes:

[...]

>> The issues I see with ediff-current-* faces are arguably a tangent to
>> this bug. Perhaps those faces could use :distant-foreground as well, for
>> similar reasons?
>
> I'm not sure if it's possible to find such default colors that would
> work well for all standard themes.  But there is no need to use
> :distant-foreground in themes, because the authors of themes can find
> the right combination of the default/ediff colors and adjust their themes
> accordingly.
>
>> As for the background colors possibly being quite similar to the theme's
>> background, I'm not sure what to do about that.
>
> Implementing something like :distant-background is less important than
> :distant-foreground, because :distant-foreground is important to
> make the text readable, while :distant-background would only help
> making the background color differ from the default background color.

Thanks again for the fix.

Yes, I have long wondered if the experience of making and using themes
could be better. Today it seems like the best course of action is to
customize all known faces in your theme, at least if you'd like it to
look great in all cases. Otherwise the appearance is a bit of a gamble.