[gmane.emacs.diffs] master 2621c29: Use colors in the VC mode lines

[gmane.emacs.diffs] master 2621c29: Use colors in the VC mode lines

[Mark Oteiza]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]


This is a gripe:

Changing the faces like this is annoying for a few reasons:

- The mode line already shows most of these states with a
  character. This is documented.

- Whether or not people know what the characters mean, the faces have no
  further meaning over that of the characters, and are otherwise
  undocumented.

- The choice of colors is somewhat arbitrary, some of them are the same,
  and there is no inheritance other than to the base-face.

- Anyone who wants the faces to just be the same has to configure every
  single vc face except the base face

- Anyone who wants the faces to be different is going to override these
  faces anyways

- Every theme author likely has to account for every VC face instead of
  one (or none) because of the previous two points

Side point: Óscar already added a news item for the faces in
emacs-25. Now there is another NEWS item in master (the change
subsequent to the subject one).


[-- Attachment #2: Type: message/rfc822, Size: 2917 bytes --]

From: Lars Ingebrigtsen <larsi@gnus.org>
To: emacs-diffs@gnu.org
Subject: master 2621c29: Use colors in the VC mode lines
Date: Tue, 01 Mar 2016 03:25:55 +0000 (3 days, 16 hours, 52 minutes ago)

branch: master
commit 2621c293d82c15c00d9e73a8db75d70da7d0a23b
Author: Lars Ingebrigtsen <larsi@gnus.org>
Commit: Lars Ingebrigtsen <larsi@gnus.org>

    Use colors in the VC mode lines
    
    * lisp/vc/vc-hooks.el: Make the mode line faces default to
    using colors to more clearly tell the user what the status is.
---
 lisp/vc/vc-hooks.el |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index 0c1718e..6488e53 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -54,44 +54,51 @@
   :group 'vc-faces)
 
 (defface vc-needs-update-state
-  '((default :inherit vc-state-base-face))
+  '((default :inherit vc-state-base-face)
+    (((class color)) :foreground "blue" :weight bold))
   "Face for VC modeline state when the file needs update."
   :version "25.1"
   :group 'vc-faces)
 
 (defface vc-locked-state
-  '((default :inherit vc-state-base-face))
+  '((default :inherit vc-state-base-face)
+    (((class color)) :foreground "red"))
   "Face for VC modeline state when the file locked."
   :version "25.1"
   :group 'vc-faces)
 
 (defface vc-locally-added-state
-  '((default :inherit vc-state-base-face))
+  '((default :inherit vc-state-base-face)
+    (((class color)) :foreground "ForestGreen"))
   "Face for VC modeline state when the file is locally added."
   :version "25.1"
   :group 'vc-faces)
 
 (defface vc-conflict-state
-  '((default :inherit vc-state-base-face))
+  '((default :inherit vc-state-base-face)
+    (((class color)) :foreground "red" :weight bold))
   "Face for VC modeline state when the file contains merge conflicts."
   :version "25.1"
   :group 'vc-faces)
 
 (defface vc-removed-state
-  '((default :inherit vc-state-base-face))
+  '((default :inherit vc-state-base-face)
+    (((class color)) :foreground "red"))
   "Face for VC modeline state when the file was removed from the VC system."
   :version "25.1"
   :group 'vc-faces)
 
 (defface vc-missing-state
-  '((default :inherit vc-state-base-face))
+  '((default :inherit vc-state-base-face)
+    (((class color)) :foreground "red"))
   "Face for VC modeline state when the file is missing from the file system."
   :version "25.1"
   :group 'vc-faces)
 
 (defface vc-edited-state
-  '((default :inherit vc-state-base-face))
-  "Face for VC modeline state when the file is up to date."
+  '((default :inherit vc-state-base-face)
+    (((class color)) :foreground "ForestGreen"))
+  "Face for VC modeline state when the file is edited."
   :version "25.1"
   :group 'vc-faces)
 

Re: [gmane.emacs.diffs] master 2621c29: Use colors in the VC mode lines

[John Wiegley]

>>>>> Mark Oteiza <mvoteiza@udel.edu> writes:

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Subject: master 2621c29: Use colors in the VC mode lines

>     Use colors in the VC mode lines

>     * lisp/vc/vc-hooks.el: Make the mode line faces default to
>     using colors to more clearly tell the user what the status is.

Lars, why was this change made without discussion? Can you please revert this
until we've decided that we actually want this?  I'm inclined to say no, and
to document that users can, if they wish, add more color to their Emacs by
customization the appropriate faces.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: [gmane.emacs.diffs] master 2621c29: Use colors in the VC mode lines

[Lars Magne Ingebrigtsen]

John Wiegley <jwiegley@gmail.com> writes:

> Lars, why was this change made without discussion? Can you please revert this
> until we've decided that we actually want this?  I'm inclined to say no, and
> to document that users can, if they wish, add more color to their Emacs by
> customization the appropriate faces.

We've had several bug reports about users not being able to distinguish
between vc states because the way they're marked is, er, "too subtle".
(I wasn't aware of the character changing until Stefan pointed it out to
me.)

We should provide defaults that make sense for the users.  That's why
font locking comes with colours pre-chosen by us.  If some users want
different colours, they can choose them, including choosing not to have
any colours at all.

Us choosing the colours on the VC mode lines is clearly analogous. 

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

Re: [gmane.emacs.diffs] master 2621c29: Use colors in the CV mode lines

[John Wiegley]

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

> We should provide defaults that make sense for the users. That's why font
> locking comes with colours pre-chosen by us. If some users want different
> colours, they can choose them, including choosing not to have any colours at
> all.

I think emacs-devel has sufficiently demonstrated that no one of us knows what
"makes sense for the users". When establishing defaults for a new feature,
we're pretty flexible, but not when it comes to changing a long established
default without discussion.

I have reverted the change until more of us are convinced otherwise.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: [gmane.emacs.diffs] master 2621c29: Use colors in the VC mode lines

[Juri Linkov]

> - The choice of colors is somewhat arbitrary, some of them are the same,
>   and there is no inheritance other than to the base-face.

Not arbitrary, AFAICS.  There is clearly recognized natural color code:
“good” states are indicated by green, and “bad” by red.  So this definitely
is an improvement.

> - Anyone who wants the faces to just be the same has to configure every
>   single vc face except the base face

Face groups should inherit from intermediate faces, such as:

- ‘vc-state-error-face’ (red) for faces ‘vc-missing-state’,
  ‘vc-conflict-state’, ‘vc-removed-state’, ‘vc-locked-state’;

- ‘vc-state-warning-face’ (orange) for ‘vc-needs-update-state’;

- ‘vc-state-info-face’ (green) for ‘vc-locally-added-state’,
  ‘vc-edited-state’.

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Subject: master 2621c29: Use colors in the VC mode lines
> Newsgroups: gmane.emacs.diffs
> To: emacs-diffs@gnu.org
> Date: Tue, 01 Mar 2016 03:25:55 +0000 (3 days, 16 hours, 52 minutes ago)
> Mail-Followup-To: emacs-devel@gnu.org, Lars Ingebrigtsen <larsi@gnus.org>
>
> branch: master
> commit 2621c293d82c15c00d9e73a8db75d70da7d0a23b
> Author: Lars Ingebrigtsen <larsi@gnus.org>
> Commit: Lars Ingebrigtsen <larsi@gnus.org>
>
>     Use colors in the VC mode lines
>
>     * lisp/vc/vc-hooks.el: Make the mode line faces default to
>     using colors to more clearly tell the user what the status is.
> ---
>  lisp/vc/vc-hooks.el |   23 +++++++++++++++--------
>  1 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
> index 0c1718e..6488e53 100644
> --- a/lisp/vc/vc-hooks.el
> +++ b/lisp/vc/vc-hooks.el
> @@ -54,44 +54,51 @@
>    :group 'vc-faces)
>
>  (defface vc-needs-update-state
> -  '((default :inherit vc-state-base-face))
> +  '((default :inherit vc-state-base-face)
> +    (((class color)) :foreground "blue" :weight bold))
>    "Face for VC modeline state when the file needs update."
>    :version "25.1"
>    :group 'vc-faces)
>
>  (defface vc-locked-state
> -  '((default :inherit vc-state-base-face))
> +  '((default :inherit vc-state-base-face)
> +    (((class color)) :foreground "red"))
>    "Face for VC modeline state when the file locked."
>    :version "25.1"
>    :group 'vc-faces)
>
>  (defface vc-locally-added-state
> -  '((default :inherit vc-state-base-face))
> +  '((default :inherit vc-state-base-face)
> +    (((class color)) :foreground "ForestGreen"))
>    "Face for VC modeline state when the file is locally added."
>    :version "25.1"
>    :group 'vc-faces)
>
>  (defface vc-conflict-state
> -  '((default :inherit vc-state-base-face))
> +  '((default :inherit vc-state-base-face)
> +    (((class color)) :foreground "red" :weight bold))
>    "Face for VC modeline state when the file contains merge conflicts."
>    :version "25.1"
>    :group 'vc-faces)
>
>  (defface vc-removed-state
> -  '((default :inherit vc-state-base-face))
> +  '((default :inherit vc-state-base-face)
> +    (((class color)) :foreground "red"))
>    "Face for VC modeline state when the file was removed from the VC system."
>    :version "25.1"
>    :group 'vc-faces)
>
>  (defface vc-missing-state
> -  '((default :inherit vc-state-base-face))
> +  '((default :inherit vc-state-base-face)
> +    (((class color)) :foreground "red"))
>    "Face for VC modeline state when the file is missing from the file system."
>    :version "25.1"
>    :group 'vc-faces)
>
>  (defface vc-edited-state
> -  '((default :inherit vc-state-base-face))
> -  "Face for VC modeline state when the file is up to date."
> +  '((default :inherit vc-state-base-face)
> +    (((class color)) :foreground "ForestGreen"))
> +  "Face for VC modeline state when the file is edited."
>    :version "25.1"
>    :group 'vc-faces)
>
>
> ----------

Re: [gmane.emacs.diffs] master 2621c29: Use colors in the CV mode lines

[Juri Linkov]

>> We should provide defaults that make sense for the users. That's why font
>> locking comes with colours pre-chosen by us. If some users want different
>> colours, they can choose them, including choosing not to have any colours at
>> all.
>
> I think emacs-devel has sufficiently demonstrated that no one of us knows what
> "makes sense for the users". When establishing defaults for a new feature,
> we're pretty flexible, but not when it comes to changing a long established
> default without discussion.

It would be sad not to fix the existing problem of vc-mode failing
to notify the user about potentially detrimental states.

We are already using similar color-coding technique in the mode-lines of
compilation, grep, etc. with such faces as compilation-mode-line-fail,
compilation-mode-line-run, compilation-mode-line-exit.

It would be natural to extend the same faces to VC mode-lines as well.

Would you agree to install a patch after addressing the Mark's concern
about face inheritance?

Re: [gmane.emacs.diffs] master 2621c29: Use colors in the VC mode lines

[Stefan Monnier]

> We should provide defaults that make sense for the users.  That's why
> font locking comes with colours pre-chosen by us.  If some users want
> different colours, they can choose them, including choosing not to have
> any colours at all.

> Us choosing the colours on the VC mode lines is clearly analogous. 

FWIW I don't like these colors in my mode-line, but I recognize this as
a personal preference and I agree that the current "magic separator
char" indicator is much too subtle, so most users simply don't know that
this info is available.

IOW I'm in favor now of adding the color, tho we should try and make it
easy for users to "turn it off" (i.e. try and make sure the user
doesn't have to customize umpteen faces for that, tho customizing two
or three faces is probably OK).

The best would be if we could have all those faces inherit from a base
face that specifies the "amount of color", while the other faces only
specify the "kind of color".  So it can be "turned off" just be
customizing the "amount of color" in the base face.


        Stefan