GNU bug report logs - #3677
23.0.95; facemenu-read-color should not require match

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 3677 in the body.
You can then email your comments to 3677 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Jay Berkenbilt <ejb <at> ql.org>
To: emacs-pretest-bug <at> gnu.org
Subject: 23.0.95; facemenu-read-color should not require match
Date: Thu, 25 Jun 2009 10:24:38 -0400

[I apologize if this is a duplicate.  M-x report-emacs-bug doesn't
appear to have called my sendmail-send-it function, so my original
report is likely to be rejected as spam from many recipients.]

  M-x set-cursor-color RET #9ef RET

  -> [no match]

The same thing happens with set-foreground-color and
set-background-color, all of which call facemenu-read-color, but not
with set-face-foreground and set-face-background, which do not.  All
above mentioned functions do completing reads on color names, which is
appropriate, but they should also accept #xxx, #xxxxxx, rgb:xx/xx/xx,
etc.  If you do M-: (set-cursor-color "#9ef"), it works, so this is
clearly a case of the wrong kind of completing read being done.

In facemenu-read-color, in the let statement, require-match is
initialized this way:

	 (require-match (not (eq window-system 'ns)))

I believe it should always have the value nil so that people are free
to enter colors in alternative ways.


In GNU Emacs 23.0.95.1 (i686-pc-linux-gnu, GTK+ Version 2.10.4)
 of 2009-06-23 on motoko.argon.local
Windowing system distributor `The X.Org Foundation', version 11.0.70101000
configured using `configure  '--prefix=/opt/tps/packages/linux.ix86.rhel5/emacs-23.0.95-1''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=none
  locale-coding-system: utf-8-unix
  default-enable-multibyte-characters: t

Major mode: Conf[Xdefaults]

Minor modes in effect:
  diff-auto-refine-mode: t
  which-function-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
C-n C-n C-n C-n C-b C-d 9 C-d e C-x C-s C-p C-p C-p 
C-p C-b C-b C-d 8 d C-d C-x C-s C-d e C-x C-s C-x o 
C-v C-x C-x o C-x o C-b C-b C-b C-d d C-n C-n C-n C-n 
<backspace> e C-x C-s <C-backspace> <C-backspace> <C-backspace> 
<C-backspace> C-x C-s C-x 0 M-< C-v C-v C-v C-v C-v 
C-v C-v C-v C-v M-v M-v C-SPC C-p C-p C-p C-p C-p C-p 
C-p C-p C-p C-p C-p C-p C-p C-p C-p C-g M-v M-v M-v 
M-v M-v C-x b <return> C-f C-f C-b C-d b C-n C-n C-n 
C-n <backspace> c C-x C-s <C-backspace> <C-backspace> 
<C-backspace> <C-backspace> C-x C-s C-x b <return> 
C-s s k y C-x b <return> C-x v u y e s <return> C-n 
C-n C-n C-n C-k C-z C-o M-b 0 <backspace> 9 e f C-k 
C-p C-p C-p C-p <M-backspace> 8 d e C-x C-s <C-M-S-delete> 
C-n C-n C-n C-n C-n M-b C-k b l a c k C-x C-s <C-M-S-delete> 
M-x C-g C-x b C-g C-x C-f ~ / X r <tab> c o <tab> <return> 
C-s c u r s o r C-z C-o C-s C-s C-s C-e C-l M-b C-k 
9 e f C-x b <return> C-x b <return> C-x C-s C-x b <return> 
C-x b <return> M-x s e t SPC c u <tab> <return> # 9 
9 e e f f <return> <M-backspace> <backspace> M-p C-k 
' <backspace> # 9 e f <return> C-g M-: ( s e t - c 
u r s o r - c o l o r SPC " # d <backspace> 9 0 e f 
<backspace> <backspace> <backspace> e f " ) <return> 
C-a C-e M-x r e p o r t SPC b <backspace> e m SPC b 
SPC <return>

Recent messages:
Saving file /home/jberkenb/.local/share/themes/qtheme/openbox-3/themerc...
Wrote /home/jberkenb/.local/share/themes/qtheme/openbox-3/themerc
Quit [2 times]
Note: file is write protected
Mark saved where search started
Checking out /home/jberkenb/Xresources/color...done
Mark saved where search started
Saving file /home/jberkenb/Xresources/color...
Wrote /home/jberkenb/Xresources/color
Quit
nil

Message #10 received at 3677-done <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Chong Yidong <cyd <at> stupidchicken.com>
To: Jay Berkenbilt <ejb <at> ql.org>
Cc: 3677-done <at> debbugs.gnu.org
Subject: Re: 23.0.95; facemenu-read-color should not require match
Date: Sun, 16 Aug 2009 01:27:35 -0400

>   M-x set-cursor-color RET #9ef RET
>
>   -> [no match]
>
> The same thing happens with set-foreground-color and
> set-background-color, all of which call facemenu-read-color, but not
> with set-face-foreground and set-face-background, which do not.  All
> above mentioned functions do completing reads on color names, which is
> appropriate, but they should also accept #xxx, #xxxxxx, rgb:xx/xx/xx,
> etc.  If you do M-: (set-cursor-color "#9ef"), it works, so this is
> clearly a case of the wrong kind of completing read being done.

Thanks for the bug report.  I've hacked up the completion function so
that it allows any defined colors, including RGB triplets.

Message #11 received at 3677-done <at> emacsbugs.donarmstrong.com (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Chong Yidong'" <cyd <at> stupidchicken.com>, "'Jay Berkenbilt'" <ejb <at> ql.org>
Cc: <3677-done <at> debbugs.gnu.org>
Subject: RE: 23.0.95; facemenu-read-color should not require match
Date: Sun, 16 Aug 2009 07:56:44 -0700

> >   M-x set-cursor-color RET #9ef RET
> >
> >   -> [no match]
> >
> > The same thing happens with set-foreground-color and
> > set-background-color, all of which call facemenu-read-color, but not
> > with set-face-foreground and set-face-background, which do not.  All
> > above mentioned functions do completing reads on color 
> names, which is
> > appropriate, but they should also accept #xxx, #xxxxxx, 
> rgb:xx/xx/xx,
> > etc.  If you do M-: (set-cursor-color "#9ef"), it works, so this is
> > clearly a case of the wrong kind of completing read being done.
> 
> Thanks for the bug report.  I've hacked up the completion function so
> that it allows any defined colors, including RGB triplets.

1. I don't understand your change. Why define a `completer' function here,
instead of just using `completing-read' directly as before? And you still pass
`t' as the REQUIRE-MATCH parameter, so it's not clear to me how this accepts
#RRRGGGBBB colors. I didn't try it - I assume it works if you say it does, but I
don't see why it would or why the completer function is needed here. 

2. Why don't you just use the function `read-color', defined in faces.el? That's
exactly what it's for - it handles all colors correctly, including both named
colors and #RRRGGGBBB (hex RGB string) colors.

,----
| read-color is an interactive compiled Lisp function in `faces.el'.
| 
| (read-color &optional PROMPT CONVERT-TO-RGB-P ALLOW-EMPTY-NAME-P
| MSG-P)
| 
| Read a color name or RGB hex value: #RRRRGGGGBBBB.
| Completion is available for color names, but not for RGB hex strings.
| If the user inputs an RGB hex string, it must have the form
| #XXXXXXXXXXXX or XXXXXXXXXXXX, where each X is a hex digit.  The
| number of Xs must be a multiple of 3, with the same number of Xs for
| each of red, green, and blue.  The order is red, green, blue.
| 
| In addition to standard color names and RGB hex values, the following
| are available as color candidates.  In each case, the corresponding
| color is used.
| 
|  * `foreground at point'   - foreground under the cursor
|  * `background at point'   - background under the cursor
| 
| Checks input to be sure it represents a valid color.  If not, raises
| an error (but see exception for empty input with non-nil
| ALLOW-EMPTY-NAME-P).
| 
| Optional arg PROMPT is the prompt; if nil, uses a default prompt.
| 
| Interactively, or with optional arg CONVERT-TO-RGB-P non-nil, converts
| an input color name to an RGB hex string.  Returns the RGB hex string.
| 
| Optional arg ALLOW-EMPTY-NAME-P controls what happens if the user
| enters an empty color name (that is, just hits `RET').  If non-nil,
| then returns an empty color name, "".  If nil, then raises an error.
| Programs must test for "" if ALLOW-EMPTY-NAME-P is non-nil.  They
| can then perform an appropriate action in case of empty input.
| 
| Interactively, or with optional arg MSG-P non-nil, echoes the color in
| a message.
`----

You can forget about #1 - I'm just curious. I'm probably missing something here.
But #2 seems pertinent, at least. Using `read-color' seems like the better fix.

Message #12 received at 3677-done <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Chong Yidong <cyd <at> stupidchicken.com>
To: "Drew Adams" <drew.adams <at> oracle.com>
Cc: "'Jay Berkenbilt'" <ejb <at> ql.org>, <3677-done <at> debbugs.gnu.org>
Subject: Re: 23.0.95; facemenu-read-color should not require match
Date: Sun, 16 Aug 2009 14:10:58 -0400

"Drew Adams" <drew.adams <at> oracle.com> writes:

> 1. I don't understand your change. Why define a `completer' function
> here, instead of just using `completing-read' directly as before? And
> you still pass `t' as the REQUIRE-MATCH parameter, so it's not clear
> to me how this accepts #RRRGGGBBB colors.

The completer function checks `color-defined-p' if the color passed by
the user is not a predefined color name.  That's how it accepts RGB
triplets.

> 2. Why don't you just use the function `read-color', defined in
> faces.el? That's exactly what it's for - it handles all colors
> correctly, including both named colors and #RRRGGGBBB (hex RGB string)
> colors.

That's true, we should probably merge facemenu-read-color and
read-color.

Message #13 received at 3677-done <at> emacsbugs.donarmstrong.com (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Chong Yidong'" <cyd <at> stupidchicken.com>
Cc: "'Jay Berkenbilt'" <ejb <at> ql.org>, <3677-done <at> debbugs.gnu.org>
Subject: RE: 23.0.95; facemenu-read-color should not require match
Date: Sun, 16 Aug 2009 11:48:07 -0700

> > 1. I don't understand your change. Why define a `completer'
> > functionhere, instead of just using `completing-read' directly as 
> > before? And you still pass `t' as the REQUIRE-MATCH parameter,
> > so it's not clear to me how this accepts #RRRGGGBBB colors.
> 
> The completer function checks `color-defined-p' if the color passed by
> the user is not a predefined color name.  That's how it accepts RGB
> triplets.

Ah, right; got it. Thx.

> > 2. Why don't you just use the function `read-color', defined in
> > faces.el? That's exactly what it's for - it handles all colors
> > correctly, including both named colors and #RRRGGGBBB (hex 
> > RGB string) colors.
> 
> That's true, we should probably merge facemenu-read-color and
> read-color.

Just use `read-color'. There is nothing additional that `facemenu-read-color'
offers, except that it recognizes `facemenu-color-alist'. `facemenu-color-alist'
is only an internal variable, and it is not defined (beyond nil) or used
anywhere in the Emacs code. Its only purpose was to provide a list of colors for
completion, AFAIK. And if all supported colors are now included as completions
anyway, then it no longer serves a purpose. 

I suspect perhaps it was provided originally as a parallel to
`facemenu-listed-faces', which is now a defcustom. In any case, it seems that it
is only a vestige (cruft).

BTW, using your definition of `facemenu-read-color', I see two curious things
(probably having to do with the general completion code, not
`facemenu-read-color'):

1. Hitting TAB without typing anything at the prompt shows "Complete, but not
unique". That is clearly incorrect. The empty string is not a valid color name.

2. Typing #333333333 TAB shows "Complete, but not unique". Hitting TAB again
then shows "Sole completion". That sounds contradictory.

I suspect (without investigating, so probably wrongly) that it thinks the
completion is not unique because it cannot know what the completer function
might also include as another valid completion.

If so, then it would presumably always say "Complete, but not unique" whenever
you use a function-valued COLLECTION arg. If that's the case, then the message
should perhaps be changed to not be so definitive.

Currently, it is saying that there are in fact other completions with the same
prefix, even though #333333333 is complete. (That's true; #333333333333 is valid
too, but Emacs support doesn't reach beyond 4 hex digits per component, even
internally, AFAIK.) Is it saying that because it knows that there are such other
completions, or is it saying that because it just doesn't know? If the latter
(which I suspect), then the message should be something more like "Complete, but
not necessarily unique".

If we use `read-color' instead, then the behavior is more natural and more
usual:

1. TAB with no typed input immediately shows the completion candidates (instead
of "Complete, but not unique").

2. TAB after typing #333333333 shows "No match", which is true - it matches no
color name (all of the candidates are color names). However, completion is lax,
so when you hit RET the #333333333 is accepted correctly. This is the normal
behavior for lax completion - it is what users are used to.

Previous Next

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