bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[jidanni]

Please don't use the same colors for both "*" and "D" marked files, seen here:

(defface dired-marked
  '((t (:inherit font-lock-warning-face)))
  "Face used for marked files."
  :group 'dired-faces
  :version "22.1")
(defvar dired-marked-face 'dired-marked
  "Face name used for marked files.")

(defface dired-flagged
  '((t (:inherit font-lock-warning-face)))
  "Face used for flagged files."
  :group 'dired-faces
  :version "22.1")
(defvar dired-flagged-face 'dired-flagged
  "Face name used for flagged files.")

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Stefan Monnier]

> Please don't use the same colors for both "*" and "D" marked files,
> seen here:

> (defface dired-marked
>   '((t (:inherit font-lock-warning-face)))
>   "Face used for marked files."
>   :group 'dired-faces
>   :version "22.1")
> (defvar dired-marked-face 'dired-marked
>   "Face name used for marked files.")

> (defface dired-flagged
>   '((t (:inherit font-lock-warning-face)))
>   "Face used for flagged files."
>   :group 'dired-faces
>   :version "22.1")
> (defvar dired-flagged-face 'dired-flagged
>   "Face name used for flagged files.")

Agreed.  And while there, please fix the docstrings so they say more
clearly what's different about "flagged vs marked".


        Stefan

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Drew Adams]

> Agreed.  And while there, please fix the docstrings so they say more
> clearly what's different about "flagged vs marked".

Yes, thanks. 

Some Emacs doc and source-code comments refer to deletion flags as "marks"
(which they are), so it helps to always specifically say "flagged for deletion"
instead of just "flagged".

Then any ambiguity will arise only wrt the terms "marked" or (more likely)
"mark", and that needs to be taken care of contextually. For example, Dired lets
you use `* c' to change "marks", and in that context the `D' flag is considered
a mark.

Always associating "flag" with "deletion" will go a long way to making things
clear.

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Juri Linkov]

> Please don't use the same colors for both "*" and "D" marked files, seen here:

I use the key `D' to delete files marked with "*", so to me there is
no difference between "*" and "D" marked files.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Drew Adams]

> > Please don't use the same colors for both "*" and "D" 
> > marked files, seen here:
> 
> I use the key `D' to delete files marked with "*", so to me there is
> no difference between "*" and "D" marked files.

I too use it. And I use `x' to delete flagged files. So what? It's not about how
you or I delete files. It's about the different behavior between * and D.

There is definitely a difference, even for you, between * and D marks. Commands
such as B, C, and M have no effect on D marks (flags), but they do affect *
marks.

* and D have different behavior and they should definitely have different faces
with different default colors.

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Juri Linkov]

> There is definitely a difference, even for you, between * and D marks.
> Commands such as B, C, and M have no effect on D marks (flags), but they
> do affect * marks.

I meant that the difference between marking and flagging is artificial.
There are just different keys to do the same: `D' for marked "*" files,
and `x' for flagged "D" files.

> * and D have different behavior and they should definitely have
> different faces with different default colors.

I don't oppose different default faces.  I only had a doubt if users
would like that.  However, when decided to use different faces,
I suggest to use the following default faces:

* for `dired-flagged' leave the current face unchanged, inherited
  from `font-lock-warning-face' just like `compilation-error';

* for `dired-marked' use the same face definition as for
  `compilation-warning'.  This face is closer to the current
  `font-lock-warning-face'.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#6163: 24.0.50; be sure the color of the mark matches that of the filename

[jidanni]

j> (defface dired-marked
j>   '((t (:inherit font-lock-warning-face)))

By the way, for all the marks, be sure the color of the mark matches
that of the filename. E.g., the color of * should match the color of
.emacs here:
* -rw-r--r-- 1 jidanni jidanni 16401 May 10 17:49 .emacs
It currently doesn't, at least today for me with emacs -nw.

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Lars Magne Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Please don't use the same colors for both "*" and "D" marked files,
>> seen here:
>
>> (defface dired-marked
>>   '((t (:inherit font-lock-warning-face)))
>>   "Face used for marked files."
>>   :group 'dired-faces
>>   :version "22.1")
>> (defvar dired-marked-face 'dired-marked
>>   "Face name used for marked files.")
>
>> (defface dired-flagged
>>   '((t (:inherit font-lock-warning-face)))
>>   "Face used for flagged files."
>>   :group 'dired-faces
>>   :version "22.1")
>> (defvar dired-flagged-face 'dired-flagged
>>   "Face name used for flagged files.")
>
> Agreed.  And while there, please fix the docstrings so they say more
> clearly what's different about "flagged vs marked".

I've fixed the doc string, but what colours should be used, then?

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Drew Adams]

> I've fixed the doc string, but what colours should be used, then?

FWIW (not much), I use these colors:

'((((background dark))
   (:foreground "Blue" :background "#7575D4D41D1D")) ; ~ olive green
  (t (:foreground "Yellow" :background "Blueviolet"))
"*Face used for marks (except D) in dired buffers."


'((t (:foreground "Yellow" :background "Red")))
"*Face used for deletion flags (D) in dired buffers."


Whatever you decide on, I suggest something quite visible for `D'.
  

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

>>> Please don't use the same colors for both "*" and "D" marked files,
>>> seen here:
>>
>>> (defface dired-marked
>>>   '((t (:inherit font-lock-warning-face)))

[...]

>>> (defface dired-flagged
>>>   '((t (:inherit font-lock-warning-face)))

[...]

> I've fixed the doc string, but what colours should be used, then?

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

>> I've fixed the doc string, but what colours should be used, then?
>
> FWIW (not much), I use these colors:
>
> '((((background dark))
>    (:foreground "Blue" :background "#7575D4D41D1D")) ; ~ olive green
>   (t (:foreground "Yellow" :background "Blueviolet"))

There's apparently a rampage going on where all faces are supposed to
inherit from the `font-lock-*' faces, so I think this would be frowned
upon.

Does anybody have a suggestion taken from the `font-lock-*' corpus?

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

bug#6163: 24.0.50; be sure the color of the mark matches that of the filename

[Lars Magne Ingebrigtsen]

jidanni@jidanni.org writes:

> j> (defface dired-marked
> j>   '((t (:inherit font-lock-warning-face)))
>
> By the way, for all the marks, be sure the color of the mark matches
> that of the filename. E.g., the color of * should match the color of
> .emacs here:
> * -rw-r--r-- 1 jidanni jidanni 16401 May 10 17:49 .emacs
> It currently doesn't, at least today for me with emacs -nw.

I don't think it's supposed to, so I'm closing this report.

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> There's apparently a rampage going on where all faces are supposed to
> inherit from the `font-lock-*' faces, so I think this would be frowned
> upon.
>
> Does anybody have a suggestion taken from the `font-lock-*' corpus?

I picket a font-lock face at random.

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Juri Linkov]

>> There's apparently a rampage going on where all faces are supposed to
>> inherit from the `font-lock-*' faces, so I think this would be frowned
>> upon.
>>
>> Does anybody have a suggestion taken from the `font-lock-*' corpus?
>
> I picket a font-lock face at random.

This is a dangerous change.  It increases the likelihood of deleting the
wrong files, because its default color is not distinctive and visible enough
to help preventing the wrong operation.

Moreover, depending on `font-lock-variable-name-face' with the hope
that users never customize `font-lock-*' faces is a wrong assumption.
After selecting a new color for variable names, the user later
will discover an unpleasant effect that it have on other
completely unrelated faces.

For instance, I customized `font-lock-variable-name-face' to "Blue1",
and now I have the same colors for Dired directories and files flagged
for deletion!

If it's absolutely necessary to distinguish between marked and flagged
files, then they should use colors closer to traditional, e.g.:

* for `dired-flagged' leave the old red face unchanged,
  just like `compilation-error';

* for `dired-marked' use the same face definition as for
  `compilation-warning'.  Its orange color is very close
   to `dired-flagged' but still distinctive.

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Drew Adams]

> >> Does anybody have a suggestion taken from the `font-lock-*' corpus?
> >
> > I picket a font-lock face at random.
> 
> This is a dangerous change.  It increases the likelihood of 
> deleting the wrong files, because its default color is not
> distinctive and visible enough to help preventing the wrong
> operation.

Yes.  Misguided.

> Moreover, depending on `font-lock-variable-name-face' with the hope
> that users never customize `font-lock-*' faces is a wrong assumption.
> After selecting a new color for variable names, the user later
> will discover an unpleasant effect that it have on other
> completely unrelated faces.

Yes.  Misguided.

> For instance, I customized `font-lock-variable-name-face' to "Blue1",
> and now I have the same colors for Dired directories and files flagged
> for deletion!

It is simply a bad idea to inherit from a font-lock face here.

And even in general, but that's another story.  But you came close to it above,
where you noted that "the user later will discover an unpleasant effect that it
have on other completely unrelated faces".

That "unpleasant effect" has nothing in particular to do with the case at hand,
but is a general problem with inheriting faces willy nilly.  But for the case at
hand, at least, it should be clear to all that this is a bad idea.

> If it's absolutely necessary to distinguish between marked and flagged
> files, then they should use colors closer to traditional, e.g.:
> 
> * for `dired-flagged' leave the old red face unchanged,
>   just like `compilation-error';
>
> * for `dired-marked' use the same face definition as for
>   `compilation-warning'.  Its orange color is very close
>    to `dired-flagged' but still distinctive.

That would be OK.  Same colors, but not via inheritance (else you get the same
problem you indicated above).

What's important is that:
a. Both faces be easily noticeable.
b. They be easily distinguished from each other.
c. The deletion flag be most noticeable.
   A "warning" color such as red is good for this,
   as it signals potential danger. 

FWIW, I use these:
Skyblue background for marked.
Red foreground for flagged.
Yellow-on-red for the `D' itself.
Yellow-on-blueviolet for the `*' itself.

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Juri Linkov]

> It is simply a bad idea to inherit from a font-lock face here.

Yes, basically the inheritance is a convenient tool to allow the user
to modify a set of faces by editing one parent face, but only if
these faces are semantically related.

Font-lock faces semantically correspond to the entities existing in
programming languages like comments, strings, keywords, function-names,
variable-names, constants.

So to create a reasonable face hierarchy, we should introduce faces with
a universal color code like traffic lights where the red color denotes
a dangerous activity or errors, the yellow/amber color denotes a caution,
and the green color denotes a successful activity.

In the patch below, I've removed the regression and restored traditional
pre-24 colors in compilation and dired, and also changed `dired-flagged'
to inherit from the `error' face, and `dired-marked' from `warning'.

There are more places that need these new faces to indicate
error/warning/success, e.g. battery.el and other packages
unrelated to font-lock that currently abuse font-lock faces.
They could be modified later.

=== modified file 'lisp/dired.el'
--- lisp/dired.el	2011-08-10 08:40:46 +0000
+++ lisp/dired.el	2011-08-17 19:31:50 +0000
@@ -342,7 +342,7 @@ (defvar dired-mark-face 'dired-mark
   "Face name used for dired marks.")
 
 (defface dired-marked
-  '((t (:inherit font-lock-warning-face)))
+  '((t (:inherit warning)))
   "Face used for marked files."
   :group 'dired-faces
   :version "22.1")
@@ -350,7 +350,7 @@ (defvar dired-marked-face 'dired-marked
   "Face name used for marked files.")
 
 (defface dired-flagged
-  '((t (:inherit font-lock-variable-name-face)))
+  '((t (:inherit error)))
   "Face used for files flagged for deletion."
   :group 'dired-faces
   :version "22.1")

=== modified file 'lisp/progmodes/compile.el'
--- lisp/progmodes/compile.el	2011-07-25 08:23:29 +0000
+++ lisp/progmodes/compile.el	2011-08-17 19:32:45 +0000
@@ -647,19 +647,19 @@ (defvar compilation-environment nil
 (defvar compile-history nil)
 
 (defface compilation-error
-  '((t :inherit font-lock-warning-face))
+  '((t :inherit error))
   "Face used to highlight compiler errors."
   :group 'compilation
   :version "22.1")
 
 (defface compilation-warning
-  '((t :inherit font-lock-variable-name-face))
+  '((t :inherit warning))
   "Face used to highlight compiler warnings."
   :group 'compilation
   :version "22.1")
 
 (defface compilation-info
-  '((t :inherit font-lock-type-face))
+  '((t :inherit success))
   "Face used to highlight compiler information."
   :group 'compilation
   :version "22.1")

=== modified file 'lisp/faces.el'
--- lisp/faces.el	2011-08-10 18:17:21 +0000
+++ lisp/faces.el	2011-08-17 19:31:45 +0000
@@ -2427,6 +2427,39 @@ (defface glyphless-char
 It is used for characters of no fonts too."
   :version "24.1"
   :group 'basic-faces)
+
+(defface error
+  '((((class color) (min-colors 88) (background light)) (:foreground "Red1" :weight bold))
+    (((class color) (min-colors 88) (background dark)) (:foreground "Pink" :weight bold))
+    (((class color) (min-colors 16) (background light)) (:foreground "Red1" :weight bold))
+    (((class color) (min-colors 16) (background dark)) (:foreground "Pink" :weight bold))
+    (((class color) (min-colors 8)) (:foreground "red"))
+    (t (:inverse-video t :weight bold)))
+  "Face used to highlight errors."
+  :version "24.1"
+  :group 'basic-faces)
+
+(defface warning
+  '((((class color) (min-colors 16)) (:foreground "Orange" :weight bold))
+    (((class color)) (:foreground "cyan" :weight bold))
+    (t (:weight bold)))
+  "Face used to highlight warnings."
+  :version "24.1"
+  :group 'basic-faces)
+
+(defface success
+  '((((class color) (min-colors 16) (background light))
+     (:foreground "Green3" :weight bold))
+    (((class color) (min-colors 88) (background dark))
+     (:foreground "Green1" :weight bold))
+    (((class color) (min-colors 16) (background dark))
+     (:foreground "Green" :weight bold))
+    (((class color)) (:foreground "green" :weight bold))
+    (t (:weight bold)))
+  "Face used to indicate success."
+  :version "24.1"
+  :group 'basic-faces)
+
 \f
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;;; Manipulating font names.

=== modified file 'lisp/font-lock.el'
--- lisp/font-lock.el	2011-07-14 13:50:25 +0000
+++ lisp/font-lock.el	2011-08-17 19:32:27 +0000
@@ -1959,12 +1959,7 @@ (defface font-lock-constant-face
   :group 'font-lock-faces)
 
 (defface font-lock-warning-face
-  '((((class color) (min-colors 88) (background light)) (:foreground "Red1" :weight bold))
-    (((class color) (min-colors 88) (background dark)) (:foreground "Pink" :weight bold))
-    (((class color) (min-colors 16) (background light)) (:foreground "Red1" :weight bold))
-    (((class color) (min-colors 16) (background dark)) (:foreground "Pink" :weight bold))
-    (((class color) (min-colors 8)) (:foreground "red"))
-    (t (:inverse-video t :weight bold)))
+  '((t :inherit error))
   "Font Lock mode face used to highlight warnings."
   :group 'font-lock-faces)
 

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[jidanni]

And pink, for files one needs their parents permissions to read, or be over 18 ☻

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Chong Yidong]

Juri Linkov <juri@jurta.org> writes:

> In the patch below, I've removed the regression and restored
> traditional pre-24 colors in compilation and dired, and also changed
> `dired-flagged' to inherit from the `error' face, and `dired-marked'
> from `warning'.
>
> There are more places that need these new faces to indicate
> error/warning/success, e.g. battery.el and other packages unrelated to
> font-lock that currently abuse font-lock faces.  They could be
> modified later.

Looks fine to me, but test the 16-color versions of those faces in an
xterm first, to see if those color values give the right results.

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Juri Linkov]

> Looks fine to me, but test the 16-color versions of those faces in an
> xterm first, to see if those color values give the right results.

Done.

I tested the 16-color versions in an xterm, and they look fine
on light and dark backgrounds.

I'm also going to install changes in packages related to dired
that use faces for marked/flagged for deletion:

=== modified file 'lisp/ibuffer.el'
--- lisp/ibuffer.el	2011-08-15 10:21:47 +0000
+++ lisp/ibuffer.el	2011-08-18 11:20:35 +0000
@@ -356,12 +356,12 @@ (defcustom ibuffer-load-hook nil
   :type 'hook
   :group 'ibuffer)
 
-(defcustom ibuffer-marked-face 'font-lock-warning-face
+(defcustom ibuffer-marked-face 'warning
   "Face used for displaying marked buffers."
   :type 'face
   :group 'ibuffer)
 
-(defcustom ibuffer-deletion-face 'font-lock-type-face
+(defcustom ibuffer-deletion-face 'error
   "Face used for displaying buffers marked for deletion."
   :type 'face
   :group 'ibuffer)

=== modified file 'lisp/proced.el'
--- lisp/proced.el	2011-07-20 09:53:51 +0000
+++ lisp/proced.el	2011-08-18 11:29:33 +0000
@@ -395,7 +395,7 @@ (defface proced-mark
   :group 'proced-faces)
 
 (defface proced-marked
-  '((t (:inherit font-lock-warning-face)))
+  '((t (:inherit error)))
   "Face used for marked processes."
   :group 'proced-faces)
 
=== modified file 'lisp/battery.el'
--- lisp/battery.el	2011-06-27 08:04:55 +0000
+++ lisp/battery.el	2011-08-18 11:12:33 +0000
@@ -199,7 +199,7 @@ (defun battery-update ()
 		      'face
 		      (and (<= (car (read-from-string (cdr (assq ?p data))))
 				   battery-load-critical)
-			   'font-lock-warning-face)
+			   'error)
 		      'help-echo "Battery status information")))
   (force-mode-line-update))
 \f

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Juri Linkov]

> I'm also going to install changes in packages related to dired
> that use faces for marked/flagged for deletion:

Installed.

I also think it makes sense to create a new face `line-number'
and inherit from it in `compilation-line-number'.  Another place
that could use this face is Occur to highlight line numbers
in the *Occur* buffer.

bug#6117: 24.0.50; dired-marked-face same as dired-flagged-face

[Drew Adams]

> I also think it makes sense to create a new face `line-number'
> and inherit from it in `compilation-line-number'.  Another place
> that could use this face is Occur to highlight line numbers
> in the *Occur* buffer.

FWIW, grep and compilation output also refer to line numbers.  These are not the
numbers of the lines in those buffers, but they are line numbers.