GNU bug report logs - #41544
26.3; Possible incorrect results from color-distance

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 41544 <at> debbugs.gnu.org
Subject: bug#41544: 26.3; Possible incorrect results from color-distance
Date: Sat, 06 Jun 2020 21:27:42 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Sat, 6 Jun 2020 18:54:41 +0200
> Cc: 41544 <at> debbugs.gnu.org
> 
> Since we are making little progress, let's leave color-name-to-rgb unchanged for the moment. We can both change our minds later. It's not strictly required for the introduction and use of color-dark-p; patch updated.

What practical problem is being solved here?  (Please don't say "this
was done in different ways", that's not a practical problem from my
POV.)

> * lisp/facemenu.el (list-colors-print): Use readable-foreground-color.

I don't mind installing this part, but have the rationale should be
spelled out in the log message.

> --- a/lisp/faces.el
> +++ b/lisp/faces.el
> @@ -1786,15 +1786,24 @@ defined-colors-with-face-attributes
>  
>  (defun readable-foreground-color (color)
>    "Return a readable foreground color for background COLOR."
> -  (let* ((rgb   (color-values color))
> -	 (max   (apply #'max rgb))
> -	 (black (car (color-values "black")))
> -	 (white (car (color-values "white"))))
> -    ;; Select black or white depending on which one is less similar to
> -    ;; the brightest component.
> -    (if (> (abs (- max black)) (abs (- max white)))
> -	"black"
> -      "white")))

What was wrong with the original code?  If it produced sub-optimal
results, please show an example of that.

> +(defun color-dark-p (rgb)
> +  "Whether RGB is more readable against white than black.
> +RGB is a 3-element list (R G B), each component in the range [0,1]."
> +  (let* ((sr (nth 0 rgb))
> +         (sg (nth 1 rgb))
> +         (sb (nth 2 rgb))
> +         ;; Use the power 2.2 as an approximation to sRGB gamma;
> +         ;; it should be good enough for the purpose of this function.
> +         (r (expt sr 2.2))
> +         (g (expt sg 2.2))
> +         (b (expt sb 2.2)))
> +    (unless (<= 0 (min r g b) (max r g b) 1)
> +      (error "RGB components %S not in [0,1]" rgb))
> +    ;; The cut-off value was determined experimentally; see bug#41544.
> +    (< (+ (* r 0.299) (* g 0.587) (* b 0.114))
> +       (eval-when-compile (expt 0.6 2.2)))))

This code's algorithm and rationale should be explained in the
comments before we can discuss whether it's an improvement and why.

> diff --git a/lisp/frame.el b/lisp/frame.el
> index 6c2f774709..253528da75 100644
> --- a/lisp/frame.el
> +++ b/lisp/frame.el
> @@ -1156,6 +1156,13 @@ frame-background-mode

This is a non-starter, sorry.  I'm not interested in changing what is
considered dark and light background of a frame.

> diff --git a/lisp/term/rxvt.el b/lisp/term/rxvt.el
> index 31e3d6ede4..5dc754c8e0 100644
> --- a/lisp/term/rxvt.el
> +++ b/lisp/term/rxvt.el
> @@ -206,13 +206,11 @@ rxvt-set-background-mode

Likewise here.

> diff --git a/lisp/term/w32console.el b/lisp/term/w32console.el
> index 36e9d896c7..0e9d7c8b5c 100644
> --- a/lisp/term/w32console.el
> +++ b/lisp/term/w32console.el
> @@ -86,9 +86,9 @@ terminal-init-w32console
>      (setq r (nth 2 descr)
>  	  g (nth 3 descr)
>  	  b (nth 4 descr))
> -    (if (< (+ r g b) (* .6 (+ 65535 65535 65535)))
> -	(setq bg-mode 'dark)
> -      (setq bg-mode 'light))
> +    (setq bg-mode (if (color-dark-p
> +                       (list (/ r 65535.0) (/ g 65535.0) (/ b 65535.0)))
> +                      'dark 'light))
>      (set-terminal-parameter nil 'background-mode bg-mode))

And here.

> diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
> index 1a727e3933..bf9bcae526 100644
> --- a/lisp/term/xterm.el
> +++ b/lisp/term/xterm.el
> @@ -1120,9 +1120,8 @@ xterm-register-default-colors
>      (clear-face-cache)))
>  
>  (defun xterm-maybe-set-dark-background-mode (redc greenc bluec)
> -  ;; Use the heuristic in `frame-set-background-mode' to decide if a
> -  ;; frame is dark.
> -  (when (< (+ redc greenc bluec) (* .6 (+ 65535 65535 65535)))
> +  (when (color-dark-p (mapcar (lambda (c) (/ c 65535.0))
> +                              (list redc greenc bluec)))
>      (set-terminal-parameter nil 'background-mode 'dark)

And here.

> -(defun css--contrasty-color (name)
> -  "Return a color that contrasts with NAME.
> -NAME is of any form accepted by `color-distance'.
> -The returned color will be usable by Emacs and will contrast
> -with NAME; in particular so that if NAME is used as a background
> -color, the returned color can be used as the foreground and still
> -be readable."
> -  ;; See bug#25525 for a discussion of this.
> -  (if (> (color-distance name "black") 292485)
> -      "black" "white"))
> -
>  (defcustom css-fontify-colors t
>    "Whether CSS colors should be fontified using the color as the background.
>  When non-`nil', a text representing CSS color will be fontified
> @@ -1199,7 +1188,8 @@ css--fontify-region
>  		    (add-text-properties
>  		     start (point)
>  		     (list 'face (list :background color
> -				       :foreground (css--contrasty-color color)
> +				       :foreground (readable-foreground-color
> +                                                    color)
>  				       :box '(:line-width -1))))))))))))
>      extended-region))

Here, once again I will ask what practical problem is being fixed.

Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.