* 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
[parent not found: <83obxeuwa8.fsf@gnu.org>]
[parent not found: <CAAeL0SRRkqY2r6QwASsb1JVdeSfTd9UDz3RY5QTx-PHME2iZsQ@mail.gmail.com>]
[parent not found: <83k482ut21.fsf@gnu.org>]
* 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-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
* 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
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 external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.