* bug#64725: 30.0.50; set-face-foreground shows background colors @ 2023-07-19 7:09 Helmut Eller 2023-07-19 12:44 ` Eli Zaretskii 0 siblings, 1 reply; 6+ messages in thread From: Helmut Eller @ 2023-07-19 7:09 UTC (permalink / raw) To: 64725 [-- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#64725: 30.0.50; set-face-foreground shows background colors 2023-07-19 7:09 bug#64725: 30.0.50; set-face-foreground shows background colors Helmut Eller @ 2023-07-19 12:44 ` Eli Zaretskii 2023-07-19 15:45 ` Helmut Eller 0 siblings, 1 reply; 6+ messages in thread From: Eli Zaretskii @ 2023-07-19 12:44 UTC (permalink / raw) To: Helmut Eller; +Cc: 64725 > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#64725: 30.0.50; set-face-foreground shows background colors 2023-07-19 12:44 ` Eli Zaretskii @ 2023-07-19 15:45 ` Helmut Eller 2023-07-19 16:26 ` Eli Zaretskii 0 siblings, 1 reply; 6+ messages in thread From: Helmut Eller @ 2023-07-19 15:45 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 64725 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#64725: 30.0.50; set-face-foreground shows background colors 2023-07-19 15:45 ` Helmut Eller @ 2023-07-19 16:26 ` Eli Zaretskii 2023-07-20 14:34 ` Helmut Eller 0 siblings, 1 reply; 6+ messages in thread From: Eli Zaretskii @ 2023-07-19 16:26 UTC (permalink / raw) To: Helmut Eller; +Cc: 64725 > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#64725: 30.0.50; set-face-foreground shows background colors 2023-07-19 16:26 ` Eli Zaretskii @ 2023-07-20 14:34 ` Helmut Eller 2023-08-03 7:58 ` Eli Zaretskii 0 siblings, 1 reply; 6+ messages in thread From: Helmut Eller @ 2023-07-20 14:34 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 64725 [-- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#64725: 30.0.50; set-face-foreground shows background colors 2023-07-20 14:34 ` Helmut Eller @ 2023-08-03 7:58 ` Eli Zaretskii 0 siblings, 0 replies; 6+ messages in thread From: Eli Zaretskii @ 2023-08-03 7:58 UTC (permalink / raw) To: Helmut Eller; +Cc: 64725-done > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-03 7:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-07-19 7:09 bug#64725: 30.0.50; set-face-foreground shows background colors Helmut Eller 2023-07-19 12:44 ` Eli Zaretskii 2023-07-19 15:45 ` Helmut Eller 2023-07-19 16:26 ` Eli Zaretskii 2023-07-20 14:34 ` Helmut Eller 2023-08-03 7:58 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).