bug#64725: 30.0.50; set-face-foreground shows background colors

bug#64725: 30.0.50; set-face-foreground shows background colors

[Helmut Eller]

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


When I try to set the foreground color of a face, e.g. with:

  M-x set-face-foreground RET font-lock-string-face RET

and then press TAB to see the available colors, then Emacs displays the
list of colors as background with text in the default foreground.

It would be more useful to see the text with the candidate color as
foreground on the default background.

Maybe you could consider this patch:


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

diff --git a/lisp/faces.el b/lisp/faces.el
index 44d64c743ba..6233afb0d4d 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1340,10 +1340,11 @@ read-face-attribute
 		       (format "%s" old-value))))
 	     (setq new-value
                    (if (memq attribute '(:foreground :background))
-                       (let ((color
-                              (read-color
-                               (format-prompt "%s for face `%s'"
-                                              default attribute-name face))))
+                       (let* ((prompt (format-prompt
+                                       "%s for face `%s'"
+                                       default attribute-name face))
+                              (fg (eq attribute ':foreground))
+                              (color (read-color prompt nil nil nil fg)))
                          (if (equal (string-trim color) "")
                              default
                            color))

[-- Attachment #3: Type: text/plain, Size: 792 bytes --]




In GNU Emacs 30.0.50 (build 4, x86_64-pc-linux-gnu, GTK+ Version
 3.24.37, cairo version 1.16.0) of 2023-07-19 built on caladan
Repository revision: 8b1c92da79f967172afc3214bc9ee58bd08ddc17
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101007
System Description: Debian GNU/Linux 12 (bookworm)

Configured using:
 'configure --with-xpm=ifavailable --with-jpeg=ifavailable
 --with-gif=ifavailable --with-tiff=ifavailable'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG LIBSELINUX
LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG SECCOMP SOUND
SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 GTK3
ZLIB

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

bug#64725: 30.0.50; set-face-foreground shows background colors

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Date: Wed, 19 Jul 2023 09:09:11 +0200
> 
> When I try to set the foreground color of a face, e.g. with:
> 
>   M-x set-face-foreground RET font-lock-string-face RET
> 
> and then press TAB to see the available colors, then Emacs displays the
> list of colors as background with text in the default foreground.
> 
> It would be more useful to see the text with the candidate color as
> foreground on the default background.

Why not use the actual foreground of the face being customized?  Then
the user could see whether the background color being selected will
have good contrast (or just look well) with the face's foreground.

bug#64725: 30.0.50; set-face-foreground shows background colors

[Helmut Eller]

On Wed, Jul 19 2023, Eli Zaretskii wrote:

>> It would be more useful to see the text with the candidate color as
>> foreground on the default background.
>
> Why not use the actual foreground of the face being customized?  Then
> the user could see whether the background color being selected will
> have good contrast (or just look well) with the face's foreground.

I don't understand what you mean with "actual foreground". 

The problem I have, is that some color combinations are hard to read,
like yellow on white.  Then I usually want to change the foreground
color and not the background.

Assume that we have white as default background, black as default
foreground, font-lock-string-face has foreground yellow and we want to
change the foreground from yellow to brown.

In this situation I would like to see how brown on white would look and
not so much black on brown (or white on brown).

Even better then displaying the candidate color on the default
background would be to show the candidate color on the current
background color of the face.  But I guess that's harder to implement.

Helmut

bug#64725: 30.0.50; set-face-foreground shows background colors

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: 64725@debbugs.gnu.org
> Date: Wed, 19 Jul 2023 17:45:17 +0200
> 
> On Wed, Jul 19 2023, Eli Zaretskii wrote:
> 
> >> It would be more useful to see the text with the candidate color as
> >> foreground on the default background.
> >
> > Why not use the actual foreground of the face being customized?  Then
> > the user could see whether the background color being selected will
> > have good contrast (or just look well) with the face's foreground.
> 
> I don't understand what you mean with "actual foreground". 
> [...]
> Even better then displaying the candidate color on the default
> background would be to show the candidate color on the current
> background color of the face.  But I guess that's harder to implement.

That's exactly what I meant: when you customize the foreground color
of a face, show the candidates as text in that color on the background
of the face's background color, and when you customize the background
of a face, show the candidates as background with the text in the
foreground color of the face.  IOW, show the face with its both colors
as it will look if this candidate is chosen.

bug#64725: 30.0.50; set-face-foreground shows background colors

[Helmut Eller]

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

On Wed, Jul 19 2023, Eli Zaretskii wrote:

> That's exactly what I meant: when you customize the foreground color
> of a face, show the candidates as text in that color on the background
> of the face's background color, and when you customize the background
> of a face, show the candidates as background with the text in the
> foreground color of the face.  IOW, show the face with its both colors
> as it will look if this candidate is chosen.

OK. Here is a patch that should do this:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-the-interactive-use-of-set-face-foreground.patch --]
[-- Type: text/x-diff, Size: 6567 bytes --]

From 69159b5ca0123692373a014ed19e01547c12449e Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Thu, 20 Jul 2023 16:27:34 +0200
Subject: [PATCH] Improve the interactive use of set-face-foreground

When displaying the completion candidates, show how the face would
look with the new foreground.

* lisp/faces.el (faces--string-with-color): New helper. Factored
out from defined-colors-with-face-attributes.
(defined-colors-with-face-attributes): Use it.
(read-color): Add optional argument FACE and pass
it to faces--string-with-color.
(read-face-attribute): Call read-color with more appropriate
foreground and face arguments.

* doc/lispref/minibuf.texi (High-Level Completion): Describe the
intention behind the arguments FOREGROUND and FACE of read-color.
---
 doc/lispref/minibuf.texi |  8 ++++-
 lisp/faces.el            | 64 +++++++++++++++++++++++++---------------
 2 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/doc/lispref/minibuf.texi b/doc/lispref/minibuf.texi
index a861b8e910b..12ea7b12386 100644
--- a/doc/lispref/minibuf.texi
+++ b/doc/lispref/minibuf.texi
@@ -1515,7 +1515,8 @@ High-Level Completion
 @code{commandp}.
 @end defun
 
-@deffn Command read-color &optional prompt convert allow-empty display
+@deffn Command read-color &optional prompt convert allow-empty @
+  display foreground face
 This function reads a string that is a color specification, either the
 color's name or an RGB hex value such as @code{#RRRGGGBBB}.  It
 prompts with @var{prompt} (default: @code{"Color (name or #RGB triplet):"})
@@ -1535,6 +1536,11 @@ High-Level Completion
 
 Interactively, or when @var{display} is non-@code{nil}, the return
 value is also displayed in the echo area.
+
+The optional arguments FOREGROUND and FACE control the appearence of
+the completion candidates.  The candidates are displayed like FACE but
+with different colors.  If FOREGROUND is non-@code{nil} the foreground
+varies, otherwise the background.
 @end deffn
 
   See also the functions @code{read-coding-system} and
diff --git a/lisp/faces.el b/lisp/faces.el
index 44d64c743ba..4f51a031156 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1340,10 +1340,11 @@ read-face-attribute
 		       (format "%s" old-value))))
 	     (setq new-value
                    (if (memq attribute '(:foreground :background))
-                       (let ((color
-                              (read-color
-                               (format-prompt "%s for face `%s'"
-                                              default attribute-name face))))
+                       (let* ((prompt (format-prompt
+                                       "%s for face `%s'"
+                                       default attribute-name face))
+                              (fg (eq attribute ':foreground))
+                              (color (read-color prompt nil nil nil fg face)))
                          (if (equal (string-trim color) "")
                              default
                            color))
@@ -1870,15 +1871,26 @@ defined-colors-with-face-attributes
 strings with text properties, that make the color names render
 with the color they represent as background color (if FOREGROUND
 is nil; otherwise use the foreground color)."
-  (mapcar
-   (lambda (color-name)
-     (let ((color (copy-sequence color-name)))
-       (propertize color 'face
-		   (if foreground
-		       (list :foreground color)
-		     (list :foreground (readable-foreground-color color-name)
-                           :background color)))))
-   (defined-colors frame)))
+  (mapcar (lambda (color-name)
+            (faces--string-with-color color-name color-name foreground))
+          (defined-colors frame)))
+
+(defun faces--string-with-color (string color &optional foreground face)
+  "Return a copy of STRING with face attributes for COLOR.
+Set the :background or :foreground attribute to COLOR, depending
+on the argument FOREGROUND.
+
+The optional FACE argument controls the values for other
+attributes."
+  (let* ((defaults (if face (list face) '()))
+         (colors (cond (foreground
+                        (list :foreground color))
+                       (face
+                        (list :background color))
+                       (t
+                        (list :foreground (readable-foreground-color color)
+                              :background color)))))
+    (propertize string 'face (cons colors defaults))))
 
 (defun readable-foreground-color (color)
   "Return a readable foreground color for background COLOR.
@@ -1987,7 +1999,7 @@ display-grayscale-p
     (> (tty-color-gray-shades display) 2)))
 
 (defun read-color (&optional prompt convert-to-RGB allow-empty-name msg
-			     foreground)
+			     foreground face)
   "Read a color name or RGB triplet.
 Completion is available for color names, but not for RGB triplets.
 
@@ -2016,17 +2028,23 @@ read-color
 Interactively, or with optional arg MSG non-nil, print the
 resulting color name in the echo area.
 
-Interactively, displays a list of colored completions.  If optional
-argument FOREGROUND is non-nil, shows them as foregrounds, otherwise
-as backgrounds."
+Interactively, displays a list of colored completions.  If
+optional argument FOREGROUND is non-nil, shows them as
+foregrounds, otherwise as backgrounds.  The optional argument
+FACE controls the default appearance."
   (interactive "i\np\ni\np")    ; Always convert to RGB interactively.
   (let* ((completion-ignore-case t)
-	 (colors (append '("foreground at point" "background at point")
-			 (if allow-empty-name '(""))
-                         (if (display-color-p)
-                             (defined-colors-with-face-attributes
-                               nil foreground)
-                           (defined-colors))))
+	 (color-alist
+          `(("foreground at point" . ,(foreground-color-at-point))
+            ("background at point" . ,(background-color-at-point))
+            ,@(if allow-empty-name '(("" . unspecified)))
+            ,@(mapcar (lambda (c) (cons c c)) (defined-colors))))
+         (colors (mapcar (lambda (pair)
+                           (let* ((name (car pair))
+                                  (color (cdr pair)))
+                             (faces--string-with-color name color
+                                                       foreground face)))
+                         color-alist))
 	 (color (completing-read
 		 (or prompt "Color (name or #RGB triplet): ")
 		 ;; Completing function for reading colors, accepting
-- 
2.39.2

bug#64725: 30.0.50; set-face-foreground shows background colors

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: 64725@debbugs.gnu.org
> Date: Thu, 20 Jul 2023 16:34:34 +0200
> 
> On Wed, Jul 19 2023, Eli Zaretskii wrote:
> 
> > That's exactly what I meant: when you customize the foreground color
> > of a face, show the candidates as text in that color on the background
> > of the face's background color, and when you customize the background
> > of a face, show the candidates as background with the text in the
> > foreground color of the face.  IOW, show the face with its both colors
> > as it will look if this candidate is chosen.
> 
> OK. Here is a patch that should do this:

Thanks, installed on the master branch, and closing the bug.