* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
@ 2011-10-10 22:49 Juanma Barranquero
2011-10-11 6:01 ` Eli Zaretskii
0 siblings, 1 reply; 16+ messages in thread
From: Juanma Barranquero @ 2011-10-10 22:49 UTC (permalink / raw)
To: 9722
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@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))))
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-10 22:49 bug#9722: list-colors-duplicates does not exclude enough colors on Windows Juanma Barranquero
@ 2011-10-11 6:01 ` Eli Zaretskii
2011-10-11 11:24 ` Juanma Barranquero
0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2011-10-11 6:01 UTC (permalink / raw)
To: Juanma Barranquero; +Cc: 9722
> From: Juanma Barranquero <lekktu@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?
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-11 6:01 ` Eli Zaretskii
@ 2011-10-11 11:24 ` Juanma Barranquero
2011-10-11 19:39 ` Eli Zaretskii
0 siblings, 1 reply; 16+ messages in thread
From: Juanma Barranquero @ 2011-10-11 11:24 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 9722
On Tue, Oct 11, 2011 at 08:01, Eli Zaretskii <eliz@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
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-11 11:24 ` Juanma Barranquero
@ 2011-10-11 19:39 ` Eli Zaretskii
2011-10-11 20:54 ` Juanma Barranquero
0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2011-10-11 19:39 UTC (permalink / raw)
To: Juanma Barranquero; +Cc: 9722
> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 11 Oct 2011 13:24:04 +0200
> Cc: 9722@debbugs.gnu.org
>
> On Tue, Oct 11, 2011 at 08:01, Eli Zaretskii <eliz@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
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-11 19:39 ` Eli Zaretskii
@ 2011-10-11 20:54 ` Juanma Barranquero
2011-10-17 12:00 ` Juanma Barranquero
2011-10-19 8:20 ` Juri Linkov
0 siblings, 2 replies; 16+ messages in thread
From: Juanma Barranquero @ 2011-10-11 20:54 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 9722
On Tue, Oct 11, 2011 at 21:39, Eli Zaretskii <eliz@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
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-11 20:54 ` Juanma Barranquero
@ 2011-10-17 12:00 ` Juanma Barranquero
2011-10-17 16:41 ` Eli Zaretskii
2011-10-19 8:20 ` Juri Linkov
1 sibling, 1 reply; 16+ messages in thread
From: Juanma Barranquero @ 2011-10-17 12:00 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 9722
On Tue, Oct 11, 2011 at 22:54, Juanma Barranquero <lekktu@gmail.com> wrote:
> Or am I missing something?
Ping?
Juanma
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-17 12:00 ` Juanma Barranquero
@ 2011-10-17 16:41 ` Eli Zaretskii
2011-10-17 16:55 ` Juanma Barranquero
0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2011-10-17 16:41 UTC (permalink / raw)
To: Juanma Barranquero; +Cc: 9722
> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Mon, 17 Oct 2011 14:00:54 +0200
> Cc: 9722@debbugs.gnu.org
>
> On Tue, Oct 11, 2011 at 22:54, Juanma Barranquero <lekktu@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?
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-17 16:41 ` Eli Zaretskii
@ 2011-10-17 16:55 ` Juanma Barranquero
2011-10-17 17:15 ` Eli Zaretskii
0 siblings, 1 reply; 16+ messages in thread
From: Juanma Barranquero @ 2011-10-17 16:55 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 9722
On Mon, Oct 17, 2011 at 18:41, Eli Zaretskii <eliz@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
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-17 16:55 ` Juanma Barranquero
@ 2011-10-17 17:15 ` Eli Zaretskii
2011-10-18 14:50 ` Juanma Barranquero
0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2011-10-17 17:15 UTC (permalink / raw)
To: Juanma Barranquero; +Cc: 9722
> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Mon, 17 Oct 2011 18:55:05 +0200
> Cc: 9722@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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-17 17:15 ` Eli Zaretskii
@ 2011-10-18 14:50 ` Juanma Barranquero
[not found] ` <83obxeuwa8.fsf@gnu.org>
0 siblings, 1 reply; 16+ messages in thread
From: Juanma Barranquero @ 2011-10-18 14:50 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 9722-done
On Mon, Oct 17, 2011 at 19:15, Eli Zaretskii <eliz@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
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
[not found] ` <83k482ut21.fsf@gnu.org>
@ 2011-10-18 19:25 ` Juanma Barranquero
2011-10-18 21:01 ` Eli Zaretskii
0 siblings, 1 reply; 16+ messages in thread
From: Juanma Barranquero @ 2011-10-18 19:25 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 9722
On Tue, Oct 18, 2011 at 19:07, Eli Zaretskii <eliz@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
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-18 19:25 ` Juanma Barranquero
@ 2011-10-18 21:01 ` Eli Zaretskii
2011-10-24 19:29 ` Juanma Barranquero
0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2011-10-18 21:01 UTC (permalink / raw)
To: Juanma Barranquero; +Cc: 9722
> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 18 Oct 2011 21:25:01 +0200
> Cc: 9722@debbugs.gnu.org
>
> On Tue, Oct 18, 2011 at 19:07, Eli Zaretskii <eliz@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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-11 20:54 ` Juanma Barranquero
2011-10-17 12:00 ` Juanma Barranquero
@ 2011-10-19 8:20 ` Juri Linkov
2011-10-19 8:36 ` Eli Zaretskii
2011-10-19 8:57 ` Juanma Barranquero
1 sibling, 2 replies; 16+ messages in thread
From: Juri Linkov @ 2011-10-19 8:20 UTC (permalink / raw)
To: Juanma Barranquero; +Cc: 9722
> 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).
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-19 8:20 ` Juri Linkov
@ 2011-10-19 8:36 ` Eli Zaretskii
2011-10-19 8:57 ` Juanma Barranquero
1 sibling, 0 replies; 16+ messages in thread
From: Eli Zaretskii @ 2011-10-19 8:36 UTC (permalink / raw)
To: Juri Linkov; +Cc: lekktu, 9722
> From: Juri Linkov <juri@jurta.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, 9722@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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-19 8:20 ` Juri Linkov
2011-10-19 8:36 ` Eli Zaretskii
@ 2011-10-19 8:57 ` Juanma Barranquero
1 sibling, 0 replies; 16+ messages in thread
From: Juanma Barranquero @ 2011-10-19 8:57 UTC (permalink / raw)
To: Juri Linkov; +Cc: 9722
On Wed, Oct 19, 2011 at 10:20, Juri Linkov <juri@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
^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#9722: list-colors-duplicates does not exclude enough colors on Windows
2011-10-18 21:01 ` Eli Zaretskii
@ 2011-10-24 19:29 ` Juanma Barranquero
0 siblings, 0 replies; 16+ messages in thread
From: Juanma Barranquero @ 2011-10-24 19:29 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 9722
On Tue, Oct 18, 2011 at 23:01, Eli Zaretskii <eliz@gnu.org> wrote:
> Perhaps add a comment in facemenu.el that mentions where the System*
> prefix comes from.
Done.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2011-10-24 19:29 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-10 22:49 bug#9722: list-colors-duplicates does not exclude enough colors on Windows Juanma Barranquero
2011-10-11 6:01 ` Eli Zaretskii
2011-10-11 11:24 ` Juanma Barranquero
2011-10-11 19:39 ` Eli Zaretskii
2011-10-11 20:54 ` Juanma Barranquero
2011-10-17 12:00 ` Juanma Barranquero
2011-10-17 16:41 ` Eli Zaretskii
2011-10-17 16:55 ` Juanma Barranquero
2011-10-17 17:15 ` Eli Zaretskii
2011-10-18 14:50 ` Juanma Barranquero
[not found] ` <83obxeuwa8.fsf@gnu.org>
[not found] ` <CAAeL0SRRkqY2r6QwASsb1JVdeSfTd9UDz3RY5QTx-PHME2iZsQ@mail.gmail.com>
[not found] ` <83k482ut21.fsf@gnu.org>
2011-10-18 19:25 ` Juanma Barranquero
2011-10-18 21:01 ` Eli Zaretskii
2011-10-24 19:29 ` Juanma Barranquero
2011-10-19 8:20 ` Juri Linkov
2011-10-19 8:36 ` Eli Zaretskii
2011-10-19 8:57 ` Juanma Barranquero
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).