bug#33799: 27.0.50; set-foreground-color completion shows background colors

bug#33799: 27.0.50; set-foreground-color completion shows background colors

[Juri Linkov]

0. emacs -Q

1. M-x set-foreground-color RET TAB

shows a completion list of colors with different backgrounds, not
foregrounds as it would be natural to expect, so it is difficult to
decide what foreground color to select by looking at background colors.

bug#33799: 27.0.50; set-foreground-color completion shows background colors

[Glenn Morris]

Juri Linkov wrote:

> 0. emacs -Q
>
> 1. M-x set-foreground-color RET TAB
>
> shows a completion list of colors with different backgrounds, not
> foregrounds as it would be natural to expect

Very lightly tested:


--- i/lisp/faces.el
+++ w/lisp/faces.el
@@ -1838,18 +1838,21 @@ defined-colors
     (mapcar 'car (tty-color-alist frame))))
 (defalias 'x-defined-colors 'defined-colors)
 
-(defun defined-colors-with-face-attributes (&optional frame)
+(defun defined-colors-with-face-attributes (&optional frame foreground)
   "Return a list of colors supported for a particular frame.
 See `defined-colors' for arguments and return value. In contrast
 to `define-colors' the elements of the returned list are color
 strings with text properties, that make the color names render
-with the color they represent as background color."
+with the color they represent as background (or foreground if
+optional argument FOREGROUND is no-nil) color. "
   (mapcar
    (lambda (color-name)
-     (let ((foreground (readable-foreground-color color-name))
-	   (color      (copy-sequence color-name)))
-       (propertize color 'face (list :foreground foreground
-				     :background color))))
+     (let ((readable (readable-foreground-color color-name))
+	   (color (copy-sequence color-name)))
+       (propertize color 'face
+		   (if foreground
+		       (list :foreground color)
+		     (list :foreground readable :background color)))))
    (defined-colors frame)))
 
 (defun readable-foreground-color (color)
@@ -1935,7 +1938,8 @@ display-grayscale-p
      (t
       (> (tty-color-gray-shades display) 2)))))
 
-(defun read-color (&optional prompt convert-to-RGB allow-empty-name msg)
+(defun read-color (&optional prompt convert-to-RGB allow-empty-name msg
+			     foreground)
   "Read a color name or RGB triplet.
 Completion is available for color names, but not for RGB triplets.
 
@@ -1962,14 +1966,19 @@ read-color
 to enter an empty color name (the empty string).
 
 Interactively, or with optional arg MSG non-nil, print the
-resulting color name in the echo area."
+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."
   (interactive "i\np\ni\np")    ; Always convert to RGB interactively.
   (let* ((completion-ignore-case t)
 	 (colors (or facemenu-color-alist
 		     (append '("foreground at point" "background at point")
 			     (if allow-empty-name '(""))
                              (if (display-color-p)
-                                 (defined-colors-with-face-attributes)
+                                 (defined-colors-with-face-attributes
+				   nil foreground)
                                (defined-colors)))))
 	 (color (completing-read
 		 (or prompt "Color (name or #RGB triplet): ")
diff --git i/lisp/frame.el w/lisp/frame.el
index 56b8c54..f056b6d 100644
--- i/lisp/frame.el
+++ w/lisp/frame.el
@@ -1335,7 +1335,7 @@ set-foreground-color
   "Set the foreground color of the selected frame to COLOR-NAME.
 When called interactively, prompt for the name of the color to use.
 To get the frame's current foreground color, use `frame-parameters'."
-  (interactive (list (read-color "Foreground color: ")))
+  (interactive (list (read-color "Foreground color: " nil nil nil t)))
   (modify-frame-parameters (selected-frame)
 			   (list (cons 'foreground-color color-name)))
   (or window-system

bug#33799: 27.0.50; set-foreground-color completion shows background colors

[Juri Linkov]

>> 1. M-x set-foreground-color RET TAB
>>
>> shows a completion list of colors with different backgrounds, not
>> foregrounds as it would be natural to expect
>
> Very lightly tested:
>
>    (mapcar
>     (lambda (color-name)
> -     (let ((foreground (readable-foreground-color color-name))
> -	   (color      (copy-sequence color-name)))
> -       (propertize color 'face (list :foreground foreground
> -				     :background color))))
> +     (let ((readable (readable-foreground-color color-name))
> +	   (color (copy-sequence color-name)))
> +       (propertize color 'face
> +		   (if foreground
> +		       (list :foreground color)
> +		     (list :foreground readable :background color)))))

I don't know if readable background is needed for set-foreground-color
like readable foreground is used for set-background-color.  I think
your change to not use readable background is more correct because
then the user will see how the selected color will fit with the background.
This helps to select a suitable combination of foreground/background
colors.  The same way maybe readable foreground should be removed
from the completions of set-background-color.  This would allow to see
how the current foreground color will look on different backgrounds
in the completion list.

bug#33799: 27.0.50; set-foreground-color completion shows background colors

[Glenn Morris]

Juri Linkov wrote:

> I don't know if readable background is needed for set-foreground-color
> like readable foreground is used for set-background-color.  I think
> your change to not use readable background is more correct because
> then the user will see how the selected color will fit with the background.
> This helps to select a suitable combination of foreground/background
> colors.  The same way maybe readable foreground should be removed
> from the completions of set-background-color.  This would allow to see
> how the current foreground color will look on different backgrounds
> in the completion list.

I think defined-colors-with-face-attributes probably needs yet another
argument, to control whether the non-varying component (be it foreground
or background) is a fixed value or a changing "readable" one.

bug#33799: 27.0.50; set-foreground-color completion shows background colors

[Juri Linkov]

>> I don't know if readable background is needed for set-foreground-color
>> like readable foreground is used for set-background-color.  I think
>> your change to not use readable background is more correct because
>> then the user will see how the selected color will fit with the background.
>> This helps to select a suitable combination of foreground/background
>> colors.  The same way maybe readable foreground should be removed
>> from the completions of set-background-color.  This would allow to see
>> how the current foreground color will look on different backgrounds
>> in the completion list.
>
> I think defined-colors-with-face-attributes probably needs yet another
> argument, to control whether the non-varying component (be it foreground
> or background) is a fixed value or a changing "readable" one.

Yes, another argument (or maybe rename the new argument `foreground',
but I have no idea for a good name and its values) is needed specially
for set-foreground-color and set-background-color that should leave the
default background/foreground color instead of replacing it with readable.

bug#33799: 27.0.50; set-foreground-color completion shows background colors

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

>> shows a completion list of colors with different backgrounds, not
>> foregrounds as it would be natural to expect
>
> Very lightly tested:
>
> --- i/lisp/faces.el
> +++ w/lisp/faces.el

I respun the patch for the current trunk and tested it, and I think it
looks really good, so I pushed it to Emacs 28 (with some minor changes).

There was then some discussion about whether
defined-colors-with-face-attributes should have a parameter that says
whether to do readable colours for the "other" colour, and that may be a
good idea, but I don't quite see how that'd be exposed to the user, so I
think we can leave that as is (at least for now).  (And see whether
anybody wants any changes here.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#33799: [External] : bug#33799: 27.0.50; set-foreground-color completion shows background colors

[Drew Adams]

See also bug #15427.