GNU bug report logs - #9722
list-colors-duplicates does not exclude enough colors on Windows

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 9722 in the body.
You can then email your comments to 9722 AT debbugs.gnu.org in the normal way.

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

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Bug-Gnu-Emacs <bug-gnu-emacs <at> gnu.org>
Subject: list-colors-duplicates does not exclude enough colors on Windows
Date: Tue, 11 Oct 2011 00:49:43 +0200

Package: emacs
Severity: minor


On Windows, `list-colors-duplicates' matches the color name against
the output of `w32-default-color-map', to avoid conflating colors
which are RGB-equal, but semantically different, like SystemMenuText
and SystemWindowText.

Unfortunately, that makes colors not in that list different even if
they are not, in particular all the grayNN/greyNN pairs.

The following patch discards that check, and uses instead the
heuristic that the only special colors on Windows are the ones
starting with "System". That has always been the case anyway, and it's
unlikely for the user to define a non-special System* color (and if he
does, this patch will cause no harm anyway, it will just not be
considered a duplicate of other colors).

    Juanma



2011-10-10  Juanma Barranquero  <lekktu <at> gmail.com>

	* facemenu.el (list-colors-duplicates): On Windows, detect more
	duplicates by assuming that only colors matching "^System" are
	special "system colors".


=== modified file 'lisp/facemenu.el'
--- lisp/facemenu.el	2011-09-11 01:55:09 +0000
+++ lisp/facemenu.el	2011-10-10 22:39:20 +0000
@@ -639,8 +639,8 @@
 	 (l list))
     (while (cdr l)
       (if (and (facemenu-color-equal (car (car l)) (car (car (cdr l))))
-	       (not (if (fboundp 'w32-default-color-map)
-			(not (assoc (car (car l)) (w32-default-color-map))))))
+	       (not (and (eq system-type 'windows-nt)
+			 (string-match-p "^System" (car (car l))))))
 	  (progn
 	    (setcdr (car l) (cons (car (car (cdr l))) (cdr (car l))))
 	    (setcdr l (cdr (cdr l))))

Message #8 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors on
	Windows
Date: Tue, 11 Oct 2011 02:01:33 -0400

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Tue, 11 Oct 2011 00:49:43 +0200
> 
> On Windows, `list-colors-duplicates' matches the color name against
> the output of `w32-default-color-map', to avoid conflating colors
> which are RGB-equal, but semantically different, like SystemMenuText
> and SystemWindowText.
> 
> Unfortunately, that makes colors not in that list different even if
> they are not, in particular all the grayNN/greyNN pairs.

The duplicates annoyed me as well; thanks for taking care of it.

However, there's something I'm missing here: why doesn't
list-colors-duplicates recognize grayNN and greyNN as duplicates?
We don't have them in the list that w32-default-color-map returns.

As for duplicates such as "Dark Slate Gray" and "Dark Slate Grey",
which are not filtered out because they _are_ in
w32-default-color-map, would something break if we modify the list
returned by that function to not include any duplicates in the first
place?

Message #11 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Tue, 11 Oct 2011 13:24:04 +0200

On Tue, Oct 11, 2011 at 08:01, Eli Zaretskii <eliz <at> gnu.org> wrote:

> However, there's something I'm missing here: why doesn't
> list-colors-duplicates recognize grayNN and greyNN as duplicates?
> We don't have them in the list that w32-default-color-map returns.

That's why. The System* colors aren't either. What the current code
does on Windows is, basically, to detect duplicates when this

  (and (facemenu-color-equal COLOR1 COLOR2)
       (assoc COLOR1 (w32-default-color-map)))

is true. I.e., COLOR1 is only checked if present in the default color
list (which, I'm sure you remember, is the one hardcoded in w32fns.c,
not the one from etc/rgb.txt).

> As for duplicates such as "Dark Slate Gray" and "Dark Slate Grey",
> which are not filtered out because they _are_ in
> w32-default-color-map

I'm not sure I understand. The dark*slate*gr?y colors are in the list,
and so they are detected as duplicates.

> would something break if we modify the list
> returned by that function to not include any duplicates in the first
> place?

I don't think any code would break, but you would need to add the
missing names at some other point, woudn't you?

    Juanma

Message #14 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Tue, 11 Oct 2011 21:39:29 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Tue, 11 Oct 2011 13:24:04 +0200
> Cc: 9722 <at> debbugs.gnu.org
> 
> On Tue, Oct 11, 2011 at 08:01, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> > However, there's something I'm missing here: why doesn't
> > list-colors-duplicates recognize grayNN and greyNN as duplicates?
> > We don't have them in the list that w32-default-color-map returns.
> 
> That's why. The System* colors aren't either. What the current code
> does on Windows is, basically, to detect duplicates when this
> 
>   (and (facemenu-color-equal COLOR1 COLOR2)
>        (assoc COLOR1 (w32-default-color-map)))
> 
> is true. I.e., COLOR1 is only checked if present in the default color
> list

What is the purpose of checking w32-default-color-map?  Is it solely
for detecting the System* colors?  That sounds an odd method of doing
so.  It is also fragile: it means any color not in
w32-default-color-map will pass the duplicate test.

> > As for duplicates such as "Dark Slate Gray" and "Dark Slate Grey",
> > which are not filtered out because they _are_ in
> > w32-default-color-map
> 
> I'm not sure I understand. The dark*slate*gr?y colors are in the list,
> and so they are detected as duplicates.

No, they aren't.  Don't you see the lines below?

  dark slate gray	dark slate gray,dark slate grey,DarkSlateGray,DarkSlateGrey  #2f4f4f
  dim gray		dim gray,dim grey,DimGray,DimGrey  #696969
  slate gray		slate gray,slate grey,SlateGray,SlateGrey  #708090
  light slate gray	light slate gray,light slate grey,LightSlateGray,LightSlateGrey  #778899
  light gray		light gray,light grey,LightGray,LightGrey  #d3d3d3

Message #17 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Tue, 11 Oct 2011 22:54:22 +0200

On Tue, Oct 11, 2011 at 21:39, Eli Zaretskii <eliz <at> gnu.org> wrote:

> What is the purpose of checking w32-default-color-map?  Is it solely
> for detecting the System* colors?

Well, I don't really know, but in that function that's the only thing
it is used for. revno:59445, which introduced list-colors-duplicates
and the use of w32-default-color-map, removed the following comment

    ;; Identify duplicate colors by the name rather than the color
    ;; value.  For example, on MS-Windows, logical colors are added to
    ;; the list that might have the same value but have different
    ;; names and meanings.  For example, `SystemMenuText' (the color
    ;; w32 uses for the text in menu entries) and `SystemWindowText'
    ;; (the default color w32 uses for the text in windows and
    ;; dialogs) may be the same display color and be adjacent in the
    ;; list.  Detecting duplicates by name insures that both of these
    ;; colors remain despite identical color values.

so it seems that was the only intention.

BTW, if we remove the call to w32-d-c-m from list-colors-duplicates,
its only use will be in w32fns.c:Fx_open_connection (to initialize
Vw32_color_map) and then it is no longer necessary to have it as a
lisp level function.

> That sounds an odd method of doing so.

Yes.

> It is also fragile: it means any color not in
> w32-default-color-map will pass the duplicate test.

Not sure what you mean with "pass the duplicate test". It means that
any color not in w32-default-color-map will never be considered
duplicate of another color.

> No, they aren't.  Don't you see the lines below?
>
>  dark slate gray       dark slate gray,dark slate grey,DarkSlateGray,DarkSlateGrey  #2f4f4f
>  dim gray              dim gray,dim grey,DimGray,DimGrey  #696969
>  slate gray            slate gray,slate grey,SlateGray,SlateGrey  #708090
>  light slate gray      light slate gray,light slate grey,LightSlateGray,LightSlateGrey  #778899
>  light gray            light gray,light grey,LightGray,LightGrey  #d3d3d3

Yes, I see them. That means that list-colors-duplicates is correctly
detecting them as duplicates:

ELISP> (list-colors-duplicates '("black" "dark slate gray" "dark slate
gray" "dark slate grey" "DarkSlateGray" "DarkSlateGrey"))
(("black")
 ("dark slate gray" "DarkSlateGrey" "DarkSlateGray" "dark slate grey"
"dark slate gray"))

Or am I missing something?

    Juanma

Message #20 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Mon, 17 Oct 2011 14:00:54 +0200

On Tue, Oct 11, 2011 at 22:54, Juanma Barranquero <lekktu <at> gmail.com> wrote:

> Or am I missing something?

Ping?

    Juanma

Message #23 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Mon, 17 Oct 2011 18:41:14 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Mon, 17 Oct 2011 14:00:54 +0200
> Cc: 9722 <at> debbugs.gnu.org
> 
> On Tue, Oct 11, 2011 at 22:54, Juanma Barranquero <lekktu <at> gmail.com> wrote:
> 
> > Or am I missing something?
> 
> Ping?

Sorry.

It doesn't sound TRT to me to use w32-default-color-map for the
purpose of retaining some special colors in the list.  I think it
would be cleaner to have a separate (much shorter) list of colors that
should not be removed even if their RGB values are identical to other
colors already present in the list produced by defined-colors.

If you agree, let's create such a list, put it on w32-fns.el, and use
it in facemenu.el.

OK?

Message #26 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Mon, 17 Oct 2011 18:55:05 +0200

On Mon, Oct 17, 2011 at 18:41, Eli Zaretskii <eliz <at> gnu.org> wrote:

> It doesn't sound TRT to me to use w32-default-color-map for the
> purpose of retaining some special colors in the list.

I doesn't seem TRT to me either. My proposed patch removed that
dependency. In fact, I believe that w32-default-color-map should be
declared obsolete, to be turned into an internal-only function in some
future release. There's no real point to have it exposed to elisp,
other than using it in list-colors-duplicates.

>  I think it
> would be cleaner to have a separate (much shorter) list of colors that
> should not be removed even if their RGB values are identical to other
> colors already present in the list produced by defined-colors.

We can add that list, but currently, only colors named System.* are in
that category. Checking for "^System" or populating that list are both
very ad hoc fixes for a very specific Windows problem.

> If you agree, let's create such a list, put it on w32-fns.el, and use
> it in facemenu.el.
>
> OK?

I'm not sure.

Pros:
- The new list could be expanded by the user (if exposed to elisp as a
list and not a function)
- It does not depend on the reserved colors all being called System.*

Contras:
- It does not remove the need for w32_color_map /
Fw32_default_color_map, because these have another use (default color
list in case etc/rgb.txt is not found).
- It adds a new function or variable and a bit of complexity, for what
is just filtering out a few colors.

    Juanma

Message #29 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Mon, 17 Oct 2011 19:15:22 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Mon, 17 Oct 2011 18:55:05 +0200
> Cc: 9722 <at> debbugs.gnu.org
> 
> I believe that w32-default-color-map should be declared obsolete, to
> be turned into an internal-only function in some future
> release. There's no real point to have it exposed to elisp, other
> than using it in list-colors-duplicates.

Perhaps so, but it's a separate issue.

> >  I think it
> > would be cleaner to have a separate (much shorter) list of colors that
> > should not be removed even if their RGB values are identical to other
> > colors already present in the list produced by defined-colors.
> 
> We can add that list, but currently, only colors named System.* are in
> that category. Checking for "^System" or populating that list are both
> very ad hoc fixes for a very specific Windows problem.

Yes, but checking for "^System.*" must be in facemenu.el, while the
list could be on a w32-only file.  Not a bug deal either way.

> Pros:
> - The new list could be expanded by the user (if exposed to elisp as a
> list and not a function)
> - It does not depend on the reserved colors all being called System.*
> 
> Contras:
> - It does not remove the need for w32_color_map /
> Fw32_default_color_map, because these have another use (default color
> list in case etc/rgb.txt is not found).
> - It adds a new function or variable and a bit of complexity, for what
> is just filtering out a few colors.

If you feel that the cons outweigh the pros, go ahead and commit your
patch.

Message #34 received at 9722-done <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9722-done <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Tue, 18 Oct 2011 16:50:08 +0200

On Mon, Oct 17, 2011 at 19:15, Eli Zaretskii <eliz <at> gnu.org> wrote:

> Perhaps so, but it's a separate issue.

Obviously, it does not affect this bug. Rather, it depends on this bug
to be fixed.

> Yes, but checking for "^System.*" must be in facemenu.el, while the
> list could be on a w32-only file.  Not a bug deal either way.

You still would need a way to access the list from facemenu.el,
conditional to some f?boundp or system-type check, so no net gain
there.

> If you feel that the cons outweigh the pros, go ahead and commit your
> patch.

Well, I'm not enthralled by the idea of hardcoding "^System", but I
also dislike adding a w32-specific variable that isn't likely to
change or be useful to the user.

    Juanma

Message #35 received at 9722-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 9722-done <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Tue, 18 Oct 2011 17:58:07 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Tue, 18 Oct 2011 16:50:08 +0200
> Cc: 9722-done <at> debbugs.gnu.org
> 
> > Yes, but checking for "^System.*" must be in facemenu.el, while the
> > list could be on a w32-only file.  Not a bug deal either way.
> 
> You still would need a way to access the list from facemenu.el,
> conditional to some f?boundp or system-type check, so no net gain
> there.
> 
> > If you feel that the cons outweigh the pros, go ahead and commit your
> > patch.
> 
> Well, I'm not enthralled by the idea of hardcoding "^System", but I
> also dislike adding a w32-specific variable that isn't likely to
> change or be useful to the user.

We are in agreement.

Message #36 received at 9722-done <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9722-done <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Tue, 18 Oct 2011 18:01:56 +0200

On Tue, Oct 18, 2011 at 17:58, Eli Zaretskii <eliz <at> gnu.org> wrote:

> We are in agreement.

BTW, I just realized that the "System" prefix is hardcoded in w32fns.c
(SYSTEM_COLOR_PREFIX, which is called from
add_system_logical_colors_to_map).

    Juanma

Message #37 received at 9722-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 9722-done <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Tue, 18 Oct 2011 19:07:50 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Tue, 18 Oct 2011 18:01:56 +0200
> Cc: 9722-done <at> debbugs.gnu.org
> 
> BTW, I just realized that the "System" prefix is hardcoded in w32fns.c
> (SYSTEM_COLOR_PREFIX, which is called from
> add_system_logical_colors_to_map).

Right.  I wonder why we prepend the "System" part at all, why not use
the color names as they are in the Registry?  But I guess it's too
late now to change that.

Anyway, does this fact change the conclusions in any way?

Message #40 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Tue, 18 Oct 2011 21:25:01 +0200

On Tue, Oct 18, 2011 at 19:07, Eli Zaretskii <eliz <at> gnu.org> wrote:

> Anyway, does this fact change the conclusions in any way?

I don't think so. If anything, it makes checking for ^System a bit
less ad hoc and a bit safer to use.

    Juanma

Message #43 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Tue, 18 Oct 2011 23:01:09 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Tue, 18 Oct 2011 21:25:01 +0200
> Cc: 9722 <at> debbugs.gnu.org
> 
> On Tue, Oct 18, 2011 at 19:07, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> > Anyway, does this fact change the conclusions in any way?
> 
> I don't think so. If anything, it makes checking for ^System a bit
> less ad hoc and a bit safer to use.

Perhaps add a comment in facemenu.el that mentions where the System*
prefix comes from.

Message #46 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Wed, 19 Oct 2011 11:20:14 +0300

> Well, I don't really know, but in that function that's the only thing
> it is used for. revno:59445, which introduced list-colors-duplicates
> and the use of w32-default-color-map

As you can see in revno:59445, `w32-default-color-map' used to be
a variable that contained just "System" Windows colors.  I don't know
the subsequent fate of that variable.

Hardcoding "^System" in facemenu.el is not bad as long as adding the
prefix "System" is hardcoded as well in w32fns.c.

Or we could add a new system-independent variable to hold system colors
(and add GTK colors to it too).

Message #49 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> jurta.org>
Cc: lekktu <at> gmail.com, 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Wed, 19 Oct 2011 10:36:03 +0200

> From: Juri Linkov <juri <at> jurta.org>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  9722 <at> debbugs.gnu.org
> Date: Wed, 19 Oct 2011 11:20:14 +0300
> 
> Or we could add a new system-independent variable to hold system colors
> (and add GTK colors to it too).

GTK is available on Windows as well, so someone someday could port
the GTK Emacs to Windows.  IOW, if we just add the GTK colors, they
should be visible on all platforms.

Message #52 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Juri Linkov <juri <at> jurta.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Wed, 19 Oct 2011 10:57:20 +0200

On Wed, Oct 19, 2011 at 10:20, Juri Linkov <juri <at> jurta.org> wrote:

> Hardcoding "^System" in facemenu.el is not bad as long as adding the
> prefix "System" is hardcoded as well in w32fns.c.

Yes, that's my thinking too.

    Juanma

Message #55 received at 9722 <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9722 <at> debbugs.gnu.org
Subject: Re: bug#9722: list-colors-duplicates does not exclude enough colors
	on Windows
Date: Mon, 24 Oct 2011 21:29:48 +0200

On Tue, Oct 18, 2011 at 23:01, Eli Zaretskii <eliz <at> gnu.org> wrote:

> Perhaps add a comment in facemenu.el that mentions where the System*
> prefix comes from.

Done.

Previous Next

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