bug#35695: 27.0.50; Faces diff-added and diff-removed look the same in a 256 color terminal

bug#35695: 27.0.50; Faces diff-added and diff-removed look the same in a 256 color terminal

[Anders Lindgren]

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

Hi!

When using Emacs in a terminal that supports 256 colors (such as the
built-in terminal on macOS), the faces diff-added and diff-removed look the
same.

They are defined to have a background color of #eeffee and #ffeeee,
respectively, in light background mode. When a diff is viewed in a
terminal, they are both rendered using light grey background, so it's
impossible to distinguish between them.

In Emac 26, they were defined using #ddffdd and #ffdddd, respectively,
which is rendered fine in the terminal (on both Emacs 26 and 27).

One way to solve this is to add alternative color definitions using the
"min-colors" requirement. Another is to go back to the old values.

     -- Anders Lindgren

In GNU Emacs 27.0.50 (build 1, x86_64-apple-darwin16.7.0, NS appkit-1504.83
Version 10.12.6 (Build 16G29))
 of 2019-05-01 built on mbp.lan
Repository revision: 7dafbe3ab91e838803a84ab388bca03ff985e312
Repository branch: master
System Description:  Mac OS X 10.12.6

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
delete-backward-char: Text is read-only
Quit
Mark saved where search started
Making completion list...

Configured using:
 'configure --with-gnutls=no'

Configured features:
NOTIFY KQUEUE ACL LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS THREADS PDUMPER
GMP

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

Major mode: Apropos

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
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv dired dired-loaddefs format-spec rfc822 mml
mml-sec password-cache epa derived epg epg-config gnus-util rmail
rmail-loaddefs text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils misearch
multi-isearch help-mode cl-loaddefs cl-lib apropos smerge-mode
vc-dispatcher vc-svn diff-mode easymenu easy-mmode term/xterm xterm
elec-pair 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 menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame 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 minibuffer 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 multi-tty make-network-process emacs)

Memory information:
((conses 16 56669 7188)
 (symbols 48 6625 1)
 (strings 32 17368 1666)
 (string-bytes 1 565596)
 (vectors 16 7835)
 (vector-slots 8 78808 10720)
 (floats 8 25 439)
 (intervals 56 860 4)
 (buffers 992 14))

[-- Attachment #2: Type: text/html, Size: 4684 bytes --]

bug#35695: 27.0.50; Faces diff-added and diff-removed look the same in a 256 color terminal

[Eli Zaretskii]

> From: Anders Lindgren <andlind@gmail.com>
> Date: Sun, 12 May 2019 07:03:41 +0200
> 
> In Emac 26, they were defined using #ddffdd and #ffdddd, respectively, which is rendered fine in the terminal
> (on both Emacs 26 and 27).
> 
> One way to solve this is to add alternative color definitions using the "min-colors" requirement. Another is to go
> back to the old values.

I think we should go to the back values when min-colors is 256 or
less.

bug#35695: 27.0.50; Faces diff-added and diff-removed look the same in a 256 color terminal

[Eli Zaretskii]

> Date: Sun, 12 May 2019 08:39:58 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 35695@debbugs.gnu.org
> 
> I think we should go to the back values when min-colors is 256 or
> less.

I meant:

  I think we should go back to the old values when min-colors is 256
  or less.

Sorry.

bug#35695: 27.0.50; Faces diff-added and diff-removed look the same in a 256 color terminal

[Anders Lindgren]

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

Hi!

On Sun, May 12, 2019 at 7:59 AM Eli Zaretskii <eliz@gnu.org> wrote:

>   I think we should go back to the old values when min-colors is 256
>   or less.
>

Sounds good to me.

I just gave the following a try. It seems to be working, but I'd like to
double check the details so that I haven't missed a face. (Dark mode isn't
affected, as far as I can see.)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el

index a26e9ee..079806d 100644--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el@@ -314,8 +314,10 @@ diff-hunk-header
(defface diff-removed
   '((default
      :inherit diff-changed)-    (((class color) (min-colors 88)
(background light))+    (((class color) (min-colors 257) (background
light))      :background "#ffeeee")+    (((class color) (min-colors
88) (background light))+     :background "#ffdddd")     (((class
color) (min-colors 88) (background dark))
      :background "#553333")
     (((class color))@@ -325,8 +327,10 @@ diff-removed (defface diff-added
   '((default
      :inherit diff-changed)-    (((class color) (min-colors 88)
(background light))+    (((class color) (min-colors 257) (background
light))      :background "#eeffee")+    (((class color) (min-colors
88) (background light))+     :background "#ddffdd")     (((class
color) (min-colors 88) (background dark))
      :background "#335533")
     (((class color))@@ -2040,8 +2044,10 @@ diff-refine-changed
(defface diff-refine-removed
   '((default
      :inherit diff-refine-changed)-    (((class color) (min-colors
88) (background light))+    (((class color) (min-colors 257)
(background light))      :background "#ffcccc")+    (((class color)
(min-colors 88) (background light))+     :background "#ffbbbb")
(((class color) (min-colors 88) (background dark))
      :background "#aa2222"))
   "Face used for removed characters shown by `diff-refine-hunk'."


    -- Anders Lindgren

[-- Attachment #2: Type: text/html, Size: 4908 bytes --]

bug#35695: 27.0.50; Faces diff-added and diff-removed look the same in a 256 color terminal

[Juri Linkov]

>>   I think we should go back to the old values when min-colors is 256
>>   or less.
>
> Sounds good to me.
>
> I just gave the following a try. It seems to be working, but I'd like to
> double check the details so that I haven't missed a face. (Dark mode isn't
> affected, as far as I can see.)

Thanks, diff-refine-added needs old values for 288 colors as well.

bug#35695: 27.0.50; Faces diff-added and diff-removed look the same in a 256 color terminal

[Anders Lindgren]

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

Hi!

On Sun, May 12, 2019 at 9:28 PM Juri Linkov <juri@linkov.net> wrote:

> Thanks, diff-refine-added needs old values for 288 colors as well.
>

I didn't include it since the output looked good with the new value.
However, for consistency, we should probably add one for it as well.

Should I include a comment describing why we need to treat 256 color
displays differently?

    -- Anders

[-- Attachment #2: Type: text/html, Size: 770 bytes --]

bug#35695: 27.0.50; Faces diff-added and diff-removed look the same in a 256 color terminal

[Juri Linkov]

>> Thanks, diff-refine-added needs old values for 288 colors as well.
>
> I didn't include it since the output looked good with the new value.
> However, for consistency, we should probably add one for it as well.
>
> Should I include a comment describing why we need to treat 256 color
> displays differently?

Yes, comments are preferable because we don't have deffaces
with more than 88 min-colors anywhere else.

bug#35695: 27.0.50; Faces diff-added and diff-removed look the same in a 256 color terminal

[Anders Lindgren]

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

On Tue, May 14, 2019 at 10:36 PM Juri Linkov <juri@linkov.net> wrote:

> > Should I include a comment describing why we need to treat 256 color
> > displays differently?
>
> Yes, comments are preferable because we don't have deffaces
> with more than 88 min-colors anywhere else.
>

Thanks for the feedback!

I've pushed a patch fixing this issue.

    -- Anders

[-- Attachment #2: Type: text/html, Size: 732 bytes --]