bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Protesilaos Stavrou]

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

Dear maintainers,

The vc-dir.el library hardcodes all of its faces to generic font-lock
ones.  This makes it impossible for users/themes to exert any control
over the presentation of those buffers.

In the attached patch, I do the following:

1. Define new faces.  Each has semantic value in that it applies to
   constructs implied by its name.

2. Cover the vc-git backend's implementation of extra vc-dir headers.
   This necessarily means that not all backends are brought to the same
   state after applying this patch.

3. Address a "FIXME" comment in vc-git.el concerning the use of a
   bespoke face for the stash header's value when that is empty.

4. Use new color combinations which conform with the WCAG AAA standard
   for color contrast against pure white/black (this standard pertains
   to legibility and is the highest of its kind).

With regard to point 2, I only use Git and thus cannot test the other
backends with the requisite degree of confidence.  Do you think I should
try regardless?  Or should we just support the Git backend and hope that
someone else will work on [some of] the other backends?

On point 4, please consider this a proposal: it is a highly opinionated
change.  If you feel we should in no way alienate existing users, I am
prepared to remove all colors and just :inherit from the faces that
applied before.

I attach a couple of screenshots showcasing the expected results.

Please let me know what you think.  I remain at your disposal for any
possible amendments to this patch, assuming you are willing to merge it.

All the best,
Protesilaos

-- 
Protesilaos Stavrou
protesilaos.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-vc-dir-faces-also-apply-them-to-vc-git.patch --]
[-- Type: text/x-patch, Size: 11404 bytes --]

From 9ee98edc88d90479ea6e67406776bd9676e9af32 Mon Sep 17 00:00:00 2001
Message-Id: <9ee98edc88d90479ea6e67406776bd9676e9af32.1612697511.git.info@protesilaos.com>
From: Protesilaos Stavrou <info@protesilaos.com>
Date: Sun, 7 Feb 2021 13:12:43 +0200
Subject: [PATCH] Add vc-dir faces; also apply them to vc-git

* etc/NEWS: Document the new faces.

* lisp/vc/vc-dir.el (vc-dir-header, vc-dir-header-value)
(vc-dir-directory, vc-dir-file, vc-dir-mark-indicator)
(vc-dir-status-warning, vc-dir-status-edited, vc-dir-status-up-to-date)
(vc-dir-ignored): Add new faces.

(vc-dir-headers, vc-default-dir-extra-headers)
(vc-default-dir-printer): Apply new faces.

* lisp/vc/vc-git.el (vc-git-permissions-as-string, vc-git-dir-printer)
(vc-git-dir-extra-headers): Apply new faces.
---
 etc/NEWS          |  6 +++
 lisp/vc/vc-dir.el | 94 +++++++++++++++++++++++++++++++++++++++++------
 lisp/vc/vc-git.el | 36 +++++++++---------
 3 files changed, 107 insertions(+), 29 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index fb77688470..475b29b8f5 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -587,6 +587,12 @@ their 'default-directory' under VC.
 This is used when expanding commit messages from 'vc-print-root-log'
 and similar commands.
 
+---
+*** New faces for 'vc-dir' buffers and their Git VC backend.
+Those are: 'vc-dir-header', 'vc-dir-header-value', 'vc-dir-directory',
+'vc-dir-file', 'vc-dir-mark-indicator', 'vc-dir-status-warning',
+'vc-dir-status-edited', 'vc-dir-status-up-to-date', 'vc-dir-ignored'.
+
 ---
 *** The responsible VC backend is now the most specific one.
 'vc-responsible-backend' loops over the backends in
diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 9d0808c043..67792fb6e7 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -54,6 +54,78 @@ vc-dir-mode-hook
   :type 'hook
   :group 'vc)
 
+(defface vc-dir-header
+  '((((class color) (min-colors 88) (background light))
+     :foreground "#005a5f")
+    (((class color) (min-colors 88) (background dark))
+     :foreground "#6ae4b9")
+    (t :inherit font-lock-type-face))
+  "Face for headers in VC-dir buffers.")
+
+(defface vc-dir-header-value
+  '((((class color) (min-colors 88) (background light))
+     :foreground "#5317ac")
+    (((class color) (min-colors 88) (background dark))
+     :foreground "#b6a0ff")
+    (t :inherit font-lock-variable-name-face))
+  "Face for header values in VC-dir buffers.")
+
+(defface vc-dir-directory
+  '((((class color) (min-colors 88) (background light))
+     :foreground "#0031a9")
+    (((class color) (min-colors 88) (background dark))
+     :foreground "#2fafff")
+    (t :inherit font-lock-comment-delimiter-face))
+  "Face for directories in VC-dir buffers.")
+
+(defface vc-dir-file
+  '((((class color) (min-colors 88) (background light))
+     :foreground "#000000")
+    (((class color) (min-colors 88) (background dark))
+     :foreground "#ffffff")
+    (t :inherit font-lock-function-name-face))
+  "Face for files in VC-dir buffers.")
+
+(defface vc-dir-mark-indicator
+  '((((class color) (min-colors 88) (background light))
+     :foreground "#0000c0")
+    (((class color) (min-colors 88) (background dark))
+     :foreground "#00bcff")
+    (t :inherit font-lock-keyword-face))
+  "Face for mark indicators in VC-dir buffers.")
+
+(defface vc-dir-status-warning
+  '((((class color) (min-colors 88) (background light))
+     :foreground "#a60000")
+    (((class color) (min-colors 88) (background dark))
+     :foreground "#ff8059")
+    (t :inherit font-lock-warning-face))
+  "Face for warning status in VC-dir buffers.")
+
+(defface vc-dir-status-edited
+  '((((class color) (min-colors 88) (background light))
+     :foreground "#863927")
+    (((class color) (min-colors 88) (background dark))
+     :foreground "#f0ce43")
+    (t :inherit font-lock-variable-name-face))
+  "Face for edited status in VC-dir buffers.")
+
+(defface vc-dir-status-up-to-date
+  '((((class color) (min-colors 88) (background light))
+     :foreground "#315b00")
+    (((class color) (min-colors 88) (background dark))
+     :foreground "#70c900")
+    (t :inherit font-lock-builtin-face))
+  "Face for up-to-date status in VC-dir buffers.")
+
+(defface vc-dir-ignored
+  '((((class color) (min-colors 88) (background light))
+     :foreground "#56576d")
+    (((class color) (min-colors 88) (background dark))
+     :foreground "#93959b")
+    (t :inherit shadow))
+  "Face for ignored or empty values in VC-dir buffers.")
+
 ;; Used to store information for the files displayed in the directory buffer.
 ;; Each item displayed corresponds to one of these defstructs.
 (cl-defstruct (vc-dir-fileinfo
@@ -1126,11 +1198,11 @@ vc-dir-headers
 specific headers."
   (concat
    ;; First layout the common headers.
-   (propertize "VC backend : " 'face 'font-lock-type-face)
-   (propertize (format "%s\n" backend) 'face 'font-lock-variable-name-face)
-   (propertize "Working dir: " 'face 'font-lock-type-face)
+   (propertize "VC backend : " 'face 'vc-dir-header)
+   (propertize (format "%s\n" backend) 'face 'vc-dir-header-value)
+   (propertize "Working dir: " 'face 'vc-dir-header)
    (propertize (format "%s\n" (abbreviate-file-name dir))
-               'face 'font-lock-variable-name-face)
+               'face 'vc-dir-header-value)
    ;; Then the backend specific ones.
    (vc-call-backend backend 'dir-extra-headers dir)
    "\n"))
@@ -1386,9 +1458,9 @@ vc-default-dir-extra-headers
   ;; backend specific headers.
   ;; XXX: change this to return nil before the release.
   (concat
-   (propertize "Extra      : " 'face 'font-lock-type-face)
+   (propertize "Extra      : " 'face 'vc-dir-header)
    (propertize "Please add backend specific headers here.  It's easy!"
-	       'face 'font-lock-warning-face)))
+	       'face 'vc-dir-status-warning)))
 
 (defvar vc-dir-status-mouse-map
   (let ((map (make-sparse-keymap)))
@@ -1414,21 +1486,21 @@ vc-default-dir-printer
     (insert
      (propertize
       (format "%c" (if (vc-dir-fileinfo->marked fileentry) ?* ? ))
-      'face 'font-lock-type-face)
+      'face 'vc-dir-mark-indicator)
      "   "
      (propertize
       (format "%-20s" state)
-      'face (cond ((eq state 'up-to-date) 'font-lock-builtin-face)
-		  ((memq state '(missing conflict)) 'font-lock-warning-face)
+      'face (cond ((eq state 'up-to-date) 'vc-dir-status-up-to-date)
+		  ((memq state '(missing conflict)) 'vc-dir-status-warning)
 		  ((eq state 'edited) 'font-lock-constant-face)
-		  (t 'font-lock-variable-name-face))
+		  (t 'vc-dir-header-value))
       'mouse-face 'highlight
       'keymap vc-dir-status-mouse-map)
      " "
      (propertize
       (format "%s" filename)
       'face
-      (if isdir 'font-lock-comment-delimiter-face 'font-lock-function-name-face)
+      (if isdir 'vc-dir-directory 'vc-dir-file)
       'help-echo
       (if isdir
 	  "Directory\nVC operations can be applied to it\nmouse-3: Pop-up menu"
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index d00c2c2133..b4e6223769 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -462,7 +462,7 @@ vc-git-permissions-as-string
            (eq 0 (logand ?\111 (logxor old-perm new-perm))))
        "  "
      (if (eq 0 (logand ?\111 old-perm)) "+x" "-x"))
-  'face 'font-lock-type-face))
+  'face 'vc-dir-header))
 
 (defun vc-git-dir-printer (info)
   "Pretty-printer for the vc-dir-fileinfo structure."
@@ -474,20 +474,20 @@ vc-git-dir-printer
     (insert
      "  "
      (propertize (format "%c" (if (vc-dir-fileinfo->marked info) ?* ? ))
-                 'face 'font-lock-type-face)
+                 'face 'vc-dir-mark-indicator)
      "  "
      (propertize
       (format "%-12s" state)
-      'face (cond ((eq state 'up-to-date) 'font-lock-builtin-face)
-                  ((eq state '(missing conflict)) 'font-lock-warning-face)
-                  (t 'font-lock-variable-name-face))
+      'face (cond ((eq state 'up-to-date) 'vc-dir-status-up-to-date)
+                  ((eq state '(missing conflict)) 'vc-dir-status-warning)
+                  (t 'vc-dir-status-edited))
       'mouse-face 'highlight
       'keymap vc-dir-status-mouse-map)
      "  " (vc-git-permissions-as-string old-perm new-perm)
      "    "
      (propertize (vc-git-escape-file-name (vc-dir-fileinfo->name info))
-                 'face (if isdir 'font-lock-comment-delimiter-face
-                         'font-lock-function-name-face)
+                 'face (if isdir 'vc-dir-directory
+                         'vc-dir-file)
 		 'help-echo
 		 (if isdir
 		     "Directory\nVC operations can be applied to it\nmouse-3: Pop-up menu"
@@ -784,7 +784,7 @@ vc-git-dir-extra-headers
                   (mapconcat
                    (lambda (x)
                      (propertize x
-                                 'face 'font-lock-variable-name-face
+                                 'face 'vc-dir-header-value
                                  'mouse-face 'highlight
                                  'vc-git-hideable all-hideable
                                  'help-echo vc-git-stash-list-help
@@ -800,7 +800,7 @@ vc-git-dir-extra-headers
                   (mapconcat
                    (lambda (x)
                      (propertize x
-                                 'face 'font-lock-variable-name-face
+                                 'face 'vc-dir-header-value
                                  'mouse-face 'highlight
                                  'invisible t
                                  'vc-git-hideable t
@@ -812,31 +812,31 @@ vc-git-dir-extra-headers
                                'vc-git-hideable t))))))))
     ;; FIXME: maybe use a different face when nothing is stashed.
     (concat
-     (propertize "Branch     : " 'face 'font-lock-type-face)
+     (propertize "Branch     : " 'face 'vc-dir-header)
      (propertize branch
-		 'face 'font-lock-variable-name-face)
+		 'face 'vc-dir-header-value)
      (when remote-url
        (concat
 	"\n"
-	(propertize "Remote     : " 'face 'font-lock-type-face)
+	(propertize "Remote     : " 'face 'vc-dir-header)
 	(propertize remote-url
-		    'face 'font-lock-variable-name-face)))
+		    'face 'vc-dir-header-value)))
      ;; For now just a heading, key bindings can be added later for various bisect actions
      (when (file-exists-p (expand-file-name ".git/BISECT_START" (vc-git-root dir)))
-       (propertize  "\nBisect     : in progress" 'face 'font-lock-warning-face))
+       (propertize  "\nBisect     : in progress" 'face 'vc-dir-status-warning))
      (when (file-exists-p (expand-file-name ".git/rebase-apply" (vc-git-root dir)))
-       (propertize  "\nRebase     : in progress" 'face 'font-lock-warning-face))
+       (propertize  "\nRebase     : in progress" 'face 'vc-dir-status-warning))
      (if stash-list
          (concat
-          (propertize "\nStash      : " 'face 'font-lock-type-face)
+          (propertize "\nStash      : " 'face 'vc-dir-header)
           stash-button
           stash-string)
        (concat
-	(propertize "\nStash      : " 'face 'font-lock-type-face)
+	(propertize "\nStash      : " 'face 'vc-dir-header)
 	(propertize "Nothing stashed"
 		    'help-echo vc-git-stash-shared-help
                     'keymap vc-git-stash-shared-map
-		    'face 'font-lock-variable-name-face))))))
+		    'face 'vc-dir-ignored))))))
 
 (defun vc-git-branches ()
   "Return the existing branches, as a list of strings.
-- 
2.30.0


[-- Attachment #3: vc-dir-refashion-faces-dark.png --]
[-- Type: image/png, Size: 101670 bytes --]

[-- Attachment #4: vc-dir-refashion-faces-light.png --]
[-- Type: image/png, Size: 104395 bytes --]

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Eli Zaretskii]

> From: Protesilaos Stavrou <info@protesilaos.com>
> Date: Sun, 07 Feb 2021 13:42:09 +0200
> 
> In the attached patch, I do the following:
> 
> 1. Define new faces.  Each has semantic value in that it applies to
>    constructs implied by its name.

Thanks.  Would it be possible to use color names rather than #RRGGBB
values?  The latter makes it very hard to figure out the color that
will be used by the face.

> 4. Use new color combinations which conform with the WCAG AAA standard
>    for color contrast against pure white/black (this standard pertains
>    to legibility and is the highest of its kind).

Not sure what that means in practical terms: most Emacs users I've
watched working (myself included) use some background color other than
pure black or white.  Doesn't that change the contrast and the optimal
colors?

> With regard to point 2, I only use Git and thus cannot test the other
> backends with the requisite degree of confidence.  Do you think I should
> try regardless?  Or should we just support the Git backend and hope that
> someone else will work on [some of] the other backends?

If you can easily try other backends, it will be appreciated.  But it
is not mandatory, IMO.

> On point 4, please consider this a proposal: it is a highly opinionated
> change.  If you feel we should in no way alienate existing users, I am
> prepared to remove all colors and just :inherit from the faces that
> applied before.

Personally, I think inheriting from the existing faces will be less
drastic, so it's probably better.

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Protesilaos Stavrou]

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

On 2021-02-07, 17:15 +0200, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Protesilaos Stavrou <info@protesilaos.com>
>> Date: Sun, 07 Feb 2021 13:42:09 +0200
>> 
>> In the attached patch, I do the following:
>> 
>> 1. Define new faces.  Each has semantic value in that it applies to
>>    constructs implied by its name.
>
> Thanks.  Would it be possible to use color names rather than #RRGGBB
> values?  The latter makes it very hard to figure out the color that
> will be used by the face.

I will keep this in mind for the next time.  For this case I removed all
color specifications (please find the revised patch attached to this
message).

>> 4. Use new color combinations which conform with the WCAG AAA standard
>>    for color contrast against pure white/black (this standard pertains
>>    to legibility and is the highest of its kind).
>
> Not sure what that means in practical terms: most Emacs users I've
> watched working (myself included) use some background color other than
> pure black or white.  Doesn't that change the contrast and the optimal
> colors?

You are right: I should have clarified that I meant the default white
background and its inverse.  Other themes would indeed have to adapt
things to their needs.

>> With regard to point 2, I only use Git and thus cannot test the other
>> backends with the requisite degree of confidence.  Do you think I should
>> try regardless?  Or should we just support the Git backend and hope that
>> someone else will work on [some of] the other backends?
>
> If you can easily try other backends, it will be appreciated.  But it
> is not mandatory, IMO.

I will inspect their code and try to identify whatever looks the same as
vc-git.  Then I will prepare a separate patch.

>> On point 4, please consider this a proposal: it is a highly opinionated
>> change.  If you feel we should in no way alienate existing users, I am
>> prepared to remove all colors and just :inherit from the faces that
>> applied before.
>
> Personally, I think inheriting from the existing faces will be less
> drastic, so it's probably better.

Very well!  I am doing just that in the revised patch.  So there should
be no visual difference between this and the prior state, except for one
case: the empty Git stash header, which will ultimately inherit from
'shadow' (before there was a "FIXME" to disambiguate it from other
header values).

-- 
Protesilaos Stavrou
protesilaos.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-vc-dir-faces-also-apply-them-to-vc-git.patch --]
[-- Type: text/x-patch, Size: 9936 bytes --]

From 569437550fb0f04e2a271889467d25b773f57dca Mon Sep 17 00:00:00 2001
Message-Id: <569437550fb0f04e2a271889467d25b773f57dca.1612714070.git.info@protesilaos.com>
From: Protesilaos Stavrou <info@protesilaos.com>
Date: Sun, 7 Feb 2021 13:12:43 +0200
Subject: [PATCH] Add vc-dir faces; also apply them to vc-git

* etc/NEWS: Document the new faces.

* lisp/vc/vc-dir.el (vc-dir-header, vc-dir-header-value)
(vc-dir-directory, vc-dir-file, vc-dir-mark-indicator)
(vc-dir-status-warning, vc-dir-status-edited, vc-dir-status-up-to-date)
(vc-dir-ignored): Add new faces.

(vc-dir-headers, vc-default-dir-extra-headers)
(vc-default-dir-printer): Apply new faces.

* lisp/vc/vc-git.el (vc-git-permissions-as-string, vc-git-dir-printer)
(vc-git-dir-extra-headers): Apply new faces.
---
 etc/NEWS          |  6 ++++++
 lisp/vc/vc-dir.el | 49 ++++++++++++++++++++++++++++++++++++-----------
 lisp/vc/vc-git.el | 37 +++++++++++++++++------------------
 3 files changed, 62 insertions(+), 30 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index fb77688470..475b29b8f5 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -587,6 +587,12 @@ their 'default-directory' under VC.
 This is used when expanding commit messages from 'vc-print-root-log'
 and similar commands.
 
+---
+*** New faces for 'vc-dir' buffers and their Git VC backend.
+Those are: 'vc-dir-header', 'vc-dir-header-value', 'vc-dir-directory',
+'vc-dir-file', 'vc-dir-mark-indicator', 'vc-dir-status-warning',
+'vc-dir-status-edited', 'vc-dir-status-up-to-date', 'vc-dir-ignored'.
+
 ---
 *** The responsible VC backend is now the most specific one.
 'vc-responsible-backend' loops over the backends in
diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 9d0808c043..4f2de72afe 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -54,6 +54,33 @@ vc-dir-mode-hook
   :type 'hook
   :group 'vc)
 
+(defface vc-dir-header '((t :inherit font-lock-type-face))
+  "Face for headers in VC-dir buffers.")
+
+(defface vc-dir-header-value '((t :inherit font-lock-variable-name-face))
+  "Face for header values in VC-dir buffers.")
+
+(defface vc-dir-directory '((t :inherit font-lock-comment-delimiter-face))
+  "Face for directories in VC-dir buffers.")
+
+(defface vc-dir-file '((t :inherit font-lock-function-name-face))
+  "Face for files in VC-dir buffers.")
+
+(defface vc-dir-mark-indicator '((t :inherit font-lock-type-face))
+  "Face for mark indicators in VC-dir buffers.")
+
+(defface vc-dir-status-warning '((t :inherit font-lock-warning-face))
+  "Face for warning status in VC-dir buffers.")
+
+(defface vc-dir-status-edited '((t :inherit font-lock-variable-name-face))
+  "Face for edited status in VC-dir buffers.")
+
+(defface vc-dir-status-up-to-date '((t :inherit font-lock-builtin-face))
+  "Face for up-to-date status in VC-dir buffers.")
+
+(defface vc-dir-ignored '((t :inherit shadow))
+  "Face for ignored or empty values in VC-dir buffers.")
+
 ;; Used to store information for the files displayed in the directory buffer.
 ;; Each item displayed corresponds to one of these defstructs.
 (cl-defstruct (vc-dir-fileinfo
@@ -1126,11 +1153,11 @@ vc-dir-headers
 specific headers."
   (concat
    ;; First layout the common headers.
-   (propertize "VC backend : " 'face 'font-lock-type-face)
-   (propertize (format "%s\n" backend) 'face 'font-lock-variable-name-face)
-   (propertize "Working dir: " 'face 'font-lock-type-face)
+   (propertize "VC backend : " 'face 'vc-dir-header)
+   (propertize (format "%s\n" backend) 'face 'vc-dir-header-value)
+   (propertize "Working dir: " 'face 'vc-dir-header)
    (propertize (format "%s\n" (abbreviate-file-name dir))
-               'face 'font-lock-variable-name-face)
+               'face 'vc-dir-header-value)
    ;; Then the backend specific ones.
    (vc-call-backend backend 'dir-extra-headers dir)
    "\n"))
@@ -1386,9 +1413,9 @@ vc-default-dir-extra-headers
   ;; backend specific headers.
   ;; XXX: change this to return nil before the release.
   (concat
-   (propertize "Extra      : " 'face 'font-lock-type-face)
+   (propertize "Extra      : " 'face 'vc-dir-header)
    (propertize "Please add backend specific headers here.  It's easy!"
-	       'face 'font-lock-warning-face)))
+	       'face 'vc-dir-status-warning)))
 
 (defvar vc-dir-status-mouse-map
   (let ((map (make-sparse-keymap)))
@@ -1414,21 +1441,21 @@ vc-default-dir-printer
     (insert
      (propertize
       (format "%c" (if (vc-dir-fileinfo->marked fileentry) ?* ? ))
-      'face 'font-lock-type-face)
+      'face 'vc-dir-mark-indicator)
      "   "
      (propertize
       (format "%-20s" state)
-      'face (cond ((eq state 'up-to-date) 'font-lock-builtin-face)
-		  ((memq state '(missing conflict)) 'font-lock-warning-face)
+      'face (cond ((eq state 'up-to-date) 'vc-dir-status-up-to-date)
+		  ((memq state '(missing conflict)) 'vc-dir-status-warning)
 		  ((eq state 'edited) 'font-lock-constant-face)
-		  (t 'font-lock-variable-name-face))
+		  (t 'vc-dir-header-value))
       'mouse-face 'highlight
       'keymap vc-dir-status-mouse-map)
      " "
      (propertize
       (format "%s" filename)
       'face
-      (if isdir 'font-lock-comment-delimiter-face 'font-lock-function-name-face)
+      (if isdir 'vc-dir-directory 'vc-dir-file)
       'help-echo
       (if isdir
 	  "Directory\nVC operations can be applied to it\nmouse-3: Pop-up menu"
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index d00c2c2133..e7306386fe 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -462,7 +462,7 @@ vc-git-permissions-as-string
            (eq 0 (logand ?\111 (logxor old-perm new-perm))))
        "  "
      (if (eq 0 (logand ?\111 old-perm)) "+x" "-x"))
-  'face 'font-lock-type-face))
+  'face 'vc-dir-header))
 
 (defun vc-git-dir-printer (info)
   "Pretty-printer for the vc-dir-fileinfo structure."
@@ -474,20 +474,20 @@ vc-git-dir-printer
     (insert
      "  "
      (propertize (format "%c" (if (vc-dir-fileinfo->marked info) ?* ? ))
-                 'face 'font-lock-type-face)
+                 'face 'vc-dir-mark-indicator)
      "  "
      (propertize
       (format "%-12s" state)
-      'face (cond ((eq state 'up-to-date) 'font-lock-builtin-face)
-                  ((eq state '(missing conflict)) 'font-lock-warning-face)
-                  (t 'font-lock-variable-name-face))
+      'face (cond ((eq state 'up-to-date) 'vc-dir-status-up-to-date)
+                  ((eq state '(missing conflict)) 'vc-dir-status-warning)
+                  (t 'vc-dir-status-edited))
       'mouse-face 'highlight
       'keymap vc-dir-status-mouse-map)
      "  " (vc-git-permissions-as-string old-perm new-perm)
      "    "
      (propertize (vc-git-escape-file-name (vc-dir-fileinfo->name info))
-                 'face (if isdir 'font-lock-comment-delimiter-face
-                         'font-lock-function-name-face)
+                 'face (if isdir 'vc-dir-directory
+                         'vc-dir-file)
 		 'help-echo
 		 (if isdir
 		     "Directory\nVC operations can be applied to it\nmouse-3: Pop-up menu"
@@ -784,7 +784,7 @@ vc-git-dir-extra-headers
                   (mapconcat
                    (lambda (x)
                      (propertize x
-                                 'face 'font-lock-variable-name-face
+                                 'face 'vc-dir-header-value
                                  'mouse-face 'highlight
                                  'vc-git-hideable all-hideable
                                  'help-echo vc-git-stash-list-help
@@ -800,7 +800,7 @@ vc-git-dir-extra-headers
                   (mapconcat
                    (lambda (x)
                      (propertize x
-                                 'face 'font-lock-variable-name-face
+                                 'face 'vc-dir-header-value
                                  'mouse-face 'highlight
                                  'invisible t
                                  'vc-git-hideable t
@@ -810,33 +810,32 @@ vc-git-dir-extra-headers
                    (propertize "\n"
                                'invisible t
                                'vc-git-hideable t))))))))
-    ;; FIXME: maybe use a different face when nothing is stashed.
     (concat
-     (propertize "Branch     : " 'face 'font-lock-type-face)
+     (propertize "Branch     : " 'face 'vc-dir-header)
      (propertize branch
-		 'face 'font-lock-variable-name-face)
+		 'face 'vc-dir-header-value)
      (when remote-url
        (concat
 	"\n"
-	(propertize "Remote     : " 'face 'font-lock-type-face)
+	(propertize "Remote     : " 'face 'vc-dir-header)
 	(propertize remote-url
-		    'face 'font-lock-variable-name-face)))
+		    'face 'vc-dir-header-value)))
      ;; For now just a heading, key bindings can be added later for various bisect actions
      (when (file-exists-p (expand-file-name ".git/BISECT_START" (vc-git-root dir)))
-       (propertize  "\nBisect     : in progress" 'face 'font-lock-warning-face))
+       (propertize  "\nBisect     : in progress" 'face 'vc-dir-status-warning))
      (when (file-exists-p (expand-file-name ".git/rebase-apply" (vc-git-root dir)))
-       (propertize  "\nRebase     : in progress" 'face 'font-lock-warning-face))
+       (propertize  "\nRebase     : in progress" 'face 'vc-dir-status-warning))
      (if stash-list
          (concat
-          (propertize "\nStash      : " 'face 'font-lock-type-face)
+          (propertize "\nStash      : " 'face 'vc-dir-header)
           stash-button
           stash-string)
        (concat
-	(propertize "\nStash      : " 'face 'font-lock-type-face)
+	(propertize "\nStash      : " 'face 'vc-dir-header)
 	(propertize "Nothing stashed"
 		    'help-echo vc-git-stash-shared-help
                     'keymap vc-git-stash-shared-map
-		    'face 'font-lock-variable-name-face))))))
+		    'face 'vc-dir-ignored))))))
 
 (defun vc-git-branches ()
   "Return the existing branches, as a list of strings.
-- 
2.30.0

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Lars Ingebrigtsen]

Protesilaos Stavrou <info@protesilaos.com> writes:

> Very well!  I am doing just that in the revised patch.  So there should
> be no visual difference between this and the prior state, except for one
> case: the empty Git stash header, which will ultimately inherit from
> 'shadow' (before there was a "FIXME" to disambiguate it from other
> header values).

Looks good to me; pushed to Emacs 28 now.

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

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Dmitry Gutov]

On 07.02.2021 18:15, Protesilaos Stavrou wrote:
> On 2021-02-07, 17:15 +0200, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>>> From: Protesilaos Stavrou <info@protesilaos.com>
>>> Date: Sun, 07 Feb 2021 13:42:09 +0200
>>>
>>> In the attached patch, I do the following:
>>>
>>> 1. Define new faces.  Each has semantic value in that it applies to
>>>     constructs implied by its name.
>>
>> Thanks.  Would it be possible to use color names rather than #RRGGBB
>> values?  The latter makes it very hard to figure out the color that
>> will be used by the face.
> 
> I will keep this in mind for the next time.  For this case I removed all
> color specifications (please find the revised patch attached to this
> message).

Thanks, this is better.

I'm not opposed to changing colors, but this probably should be done 
systematically across many faces in the default theme, rather than in 
one specific UI element. Shouldn't it?

>>> 4. Use new color combinations which conform with the WCAG AAA standard
>>>     for color contrast against pure white/black (this standard pertains
>>>     to legibility and is the highest of its kind).
>>
>> Not sure what that means in practical terms: most Emacs users I've
>> watched working (myself included) use some background color other than
>> pure black or white.  Doesn't that change the contrast and the optimal
>> colors?
> 
> You are right: I should have clarified that I meant the default white
> background and its inverse.  Other themes would indeed have to adapt
> things to their needs.

True.

>>> With regard to point 2, I only use Git and thus cannot test the other
>>> backends with the requisite degree of confidence.  Do you think I should
>>> try regardless?  Or should we just support the Git backend and hope that
>>> someone else will work on [some of] the other backends?
>>
>> If you can easily try other backends, it will be appreciated.  But it
>> is not mandatory, IMO.
> 
> I will inspect their code and try to identify whatever looks the same as
> vc-git.  Then I will prepare a separate patch.

FWIW, Git is the only backend that has a complex dir-printer method.

The rest look pretty much like vc-hg-dir-printer, but 
'font-lock-comment-face' in there should be changed to some new face too.

>> Personally, I think inheriting from the existing faces will be less
>> drastic, so it's probably better.
> 
> Very well!  I am doing just that in the revised patch.  So there should
> be no visual difference between this and the prior state, except for one
> case: the empty Git stash header, which will ultimately inherit from
> 'shadow' (before there was a "FIXME" to disambiguate it from other
> header values).

Some questions:

- vc-dir-ignored face doesn't seem to be used the 'ignored' entries in 
the list. Wasn't that its main point?

- vc-git-dir-printer defaults entries to the 'vc-dir-status-edited' 
face, whereas vc-default-dir-printer defaults to vc-dir-header-value' 
(statuses that are not 'up-to-date', 'missing', 'conflict' or 'edited'). 
Which is the intended behavior? Which one do we want?

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Protesilaos Stavrou]

On 2021-02-08, 17:54 +0200, Dmitry Gutov <dgutov@yandex.ru> wrote:

> Thanks, this is better.
>
> I'm not opposed to changing colors, but this probably should be done
> systematically across many faces in the default theme, rather than in
> one specific UI element. Shouldn't it?

Yes, that would be better.

>>>> With regard to point 2, I only use Git and thus cannot test the other
>>>> backends with the requisite degree of confidence.  Do you think I should
>>>> try regardless?  Or should we just support the Git backend and hope that
>>>> someone else will work on [some of] the other backends?
>>>
>>> If you can easily try other backends, it will be appreciated.  But it
>>> is not mandatory, IMO.
>> I will inspect their code and try to identify whatever looks the same
>> as
>> vc-git.  Then I will prepare a separate patch.
>
> FWIW, Git is the only backend that has a complex dir-printer method.
>
> The rest look pretty much like vc-hg-dir-printer, but
> 'font-lock-comment-face' in there should be changed to some new face
> too.

Thanks!  I still have not had the time to check those, though I plan to
do so.  It would be nice to ensure consistency between all backends.

>>> Personally, I think inheriting from the existing faces will be less
>>> drastic, so it's probably better.
>> Very well!  I am doing just that in the revised patch.  So there
>> should
>> be no visual difference between this and the prior state, except for one
>> case: the empty Git stash header, which will ultimately inherit from
>> 'shadow' (before there was a "FIXME" to disambiguate it from other
>> header values).
>
> Some questions:
>
> - vc-dir-ignored face doesn't seem to be used the 'ignored' entries in
>   the list. Wasn't that its main point?

Can you please specify which are those?

I only applied the 'vc-dir-ignored' face to the empty Git stash and only
did so because there was a "FIXME" for it.  Otherwise, yes, the new face
should be used wherever it makes sense.

> - vc-git-dir-printer defaults entries to the 'vc-dir-status-edited'
>   face, whereas vc-default-dir-printer defaults to vc-dir-header-value'
>   (statuses that are not 'up-to-date', 'missing', 'conflict' or
>   'edited'). Which is the intended behavior? Which one do we want?
>

There is an omission from my part that I will now prepare a patch for
with regard to the "edited" check of 'vc-default-dir-printer'.  It needs
to specify 'vc-dir-status-edited' instead of 'font-lock-constant-face'.

As for its default value, I was not sure what those other states were,
so I just used 'vc-dir-header-value', thinking that this is a neutral
value.

Should the default look like "edited" as well?  Or does it have some
other meaning?  If the latter, then maybe we must have extra face?

-- 
Protesilaos Stavrou
protesilaos.com

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Juri Linkov]

>> Very well!  I am doing just that in the revised patch.  So there should
>> be no visual difference between this and the prior state, except for one
>> case: the empty Git stash header, which will ultimately inherit from
>> 'shadow' (before there was a "FIXME" to disambiguate it from other
>> header values).
>
> Looks good to me; pushed to Emacs 28 now.

I don't know if anyone else has such problem, but now highlighting
the empty Git stash header with a different color distinguishes it
from other header lines, and thus attracts more attention.
So now the most noticeable thing in the vc-dir is the Git stash header
(that I almost never use).

Maybe better to display the empty Git stash header using the default
colors, and then highlight it differently only when it's non-empty?

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Dmitry Gutov]

On 08.02.2021 20:17, Juri Linkov wrote:
>>> Very well!  I am doing just that in the revised patch.  So there should
>>> be no visual difference between this and the prior state, except for one
>>> case: the empty Git stash header, which will ultimately inherit from
>>> 'shadow' (before there was a "FIXME" to disambiguate it from other
>>> header values).
>>
>> Looks good to me; pushed to Emacs 28 now.
> 
> I don't know if anyone else has such problem, but now highlighting
> the empty Git stash header with a different color distinguishes it
> from other header lines, and thus attracts more attention.
> So now the most noticeable thing in the vc-dir is the Git stash header
> (that I almost never use).

It looks okay-ish for me, but that must depend on a particular theme.

> Maybe better to display the empty Git stash header using the default
> colors, and then highlight it differently only when it's non-empty?

Not with vc-dir-ignored, though (it is based on the 'shadow' face).

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Dmitry Gutov]

On 08.02.2021 18:35, Protesilaos Stavrou wrote:
> On 2021-02-08, 17:54 +0200, Dmitry Gutov <dgutov@yandex.ru> wrote:
>> Some questions:
>>
>> - vc-dir-ignored face doesn't seem to be used the 'ignored' entries in
>>    the list. Wasn't that its main point?
> 
> Can you please specify which are those?
> 
> I only applied the 'vc-dir-ignored' face to the empty Git stash and only
> did so because there was a "FIXME" for it.  Otherwise, yes, the new face
> should be used wherever it makes sense.

The 'ignored' files in the vc-dir tree.

To see one, edit some file that has a matching entry in .gitignore (such 
as ChangeLog in a Emacs repo checkout). You should see it in VC-Dir 
buffer now, with status 'ignored'.

>> - vc-git-dir-printer defaults entries to the 'vc-dir-status-edited'
>>    face, whereas vc-default-dir-printer defaults to vc-dir-header-value'
>>    (statuses that are not 'up-to-date', 'missing', 'conflict' or
>>    'edited'). Which is the intended behavior? Which one do we want?
>>
> 
> There is an omission from my part that I will now prepare a patch for
> with regard to the "edited" check of 'vc-default-dir-printer'.  It needs
> to specify 'vc-dir-status-edited' instead of 'font-lock-constant-face'.

Thank you.

> As for its default value, I was not sure what those other states were,
> so I just used 'vc-dir-header-value', thinking that this is a neutral
> value.

All possible states are listed in the docstring for 'vc-state'.

About half of them (almost) are pretty rare, though.

> Should the default look like "edited" as well?  Or does it have some
> other meaning?  If the latter, then maybe we must have extra face?

I don't have a strong opinion on this right now. But we should be 
consistent between the 'default' version and the backend-specific 
versions of the method.

Having a face per status might be too much both for the user and the 
theme authors, though (who will have to pick appropriate colors).

So I would keep the number of faces at 4: up-to-date, warning, ignored 
and edited.

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Protesilaos Stavrou]

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

On 2021-02-09, 01:33 +0200, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 08.02.2021 18:35, Protesilaos Stavrou wrote:
>> On 2021-02-08, 17:54 +0200, Dmitry Gutov <dgutov@yandex.ru> wrote:
>>> Some questions:
>>>
>>> - vc-dir-ignored face doesn't seem to be used the 'ignored' entries in
>>>    the list. Wasn't that its main point?
>> Can you please specify which are those?
>> I only applied the 'vc-dir-ignored' face to the empty Git stash and
>> only
>> did so because there was a "FIXME" for it.  Otherwise, yes, the new face
>> should be used wherever it makes sense.
>
> The 'ignored' files in the vc-dir tree.
>
> To see one, edit some file that has a matching entry in .gitignore (such
> as ChangeLog in a Emacs repo checkout). You should see it in VC-Dir
> buffer now, with status 'ignored'.

Please see the attached patch (unless you want me to open a new bug
report).  This should now account for the ignored state.  It also edits
a face that I had missed earlier, as was discussed herein.

>> As for its default value, I was not sure what those other states were,
>> so I just used 'vc-dir-header-value', thinking that this is a neutral
>> value.
>
> All possible states are listed in the docstring for 'vc-state'.
>
> About half of them (almost) are pretty rare, though.
>
>> Should the default look like "edited" as well?  Or does it have some
>> other meaning?  If the latter, then maybe we must have extra face?
>
> I don't have a strong opinion on this right now. But we should be
> consistent between the 'default' version and the backend-specific
> versions of the method.
>
> Having a face per status might be too much both for the user and the
> theme authors, though (who will have to pick appropriate colors).
>
> So I would keep the number of faces at 4: up-to-date, warning, ignored
> and edited.

I also think that 4 faces should suffice.  Having checked the doc string
of 'vc-state' this is how I feel they should be organised.

| status           | face ("?" means suggestion) |
|------------------+-----------------------------|
| up-to-date       | vc-dir-status-up-to-date    |
| edited           | vc-dir-status-edited        |
| USER             | vc-dir-status-warning?      |
| needs-update     | vc-dir-status-warning?      |
| unlocked-changes | vc-dir-status-warning?      |
| added            | vc-dir-status-edited        |
| removed          | vc-dir-status-edited        |
| conflict         | vc-dir-status-warning       |
| missing          | vc-dir-status-warning       |
| ignored          | vc-dir-ignored              |
| unregistered     | vc-dir-status-edited        |

With regard to 'vc-dir-ignored', do you think we should rename it to
'vc-dir-status-ignored' for the sake of consistency?  I wrote it that
way to denote that it is more generic than those that apply to files,
but I am okay either way.

-- 
Protesilaos Stavrou
protesilaos.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Refine-use-of-new-vc-dir-faces.patch --]
[-- Type: text/x-patch, Size: 1885 bytes --]

From 887b3333d09a93446dd7bd478e4e877df2884bd7 Mon Sep 17 00:00:00 2001
Message-Id: <887b3333d09a93446dd7bd478e4e877df2884bd7.1612846159.git.info@protesilaos.com>
From: Protesilaos Stavrou <info@protesilaos.com>
Date: Tue, 9 Feb 2021 06:49:05 +0200
Subject: [PATCH] Refine use of new vc-dir faces

* lisp/vc/vc-dir.el (vc-default-dir-printer): Apply check for the
"ignored" status and make 'vc-dir-status-edited' the default face.

* lisp/vc/vc-git.el (vc-git-dir-printer): Use the same conditions for
the Git backend.

This follows from the discussion in bug#46358.
---
 lisp/vc/vc-dir.el | 4 ++--
 lisp/vc/vc-git.el | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 14c81578b7..6aebd1b2e0 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -1456,8 +1456,8 @@ vc-default-dir-printer
       (format "%-20s" state)
       'face (cond ((eq state 'up-to-date) 'vc-dir-status-up-to-date)
 		  ((memq state '(missing conflict)) 'vc-dir-status-warning)
-		  ((eq state 'edited) 'font-lock-constant-face)
-		  (t 'vc-dir-header-value))
+		  ((eq state 'ignored) 'vc-dir-ignored)
+		  (t 'vc-dir-status-edited))
       'mouse-face 'highlight
       'keymap vc-dir-status-mouse-map)
      " "
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index e7306386fe..18c5432ad9 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -479,7 +479,8 @@ vc-git-dir-printer
      (propertize
       (format "%-12s" state)
       'face (cond ((eq state 'up-to-date) 'vc-dir-status-up-to-date)
-                  ((eq state '(missing conflict)) 'vc-dir-status-warning)
+                  ((memq state '(missing conflict)) 'vc-dir-status-warning)
+                  ((eq state 'ignored) 'vc-dir-ignored)
                   (t 'vc-dir-status-edited))
       'mouse-face 'highlight
       'keymap vc-dir-status-mouse-map)
-- 
2.30.0

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Protesilaos Stavrou]

On 2021-02-09, 01:24 +0200, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 08.02.2021 20:17, Juri Linkov wrote:
>>>> Very well!  I am doing just that in the revised patch.  So there should
>>>> be no visual difference between this and the prior state, except for one
>>>> case: the empty Git stash header, which will ultimately inherit from
>>>> 'shadow' (before there was a "FIXME" to disambiguate it from other
>>>> header values).
>>>
>>> Looks good to me; pushed to Emacs 28 now.
>> I don't know if anyone else has such problem, but now highlighting
>> the empty Git stash header with a different color distinguishes it
>> from other header lines, and thus attracts more attention.
>> So now the most noticeable thing in the vc-dir is the Git stash header
>> (that I almost never use).
>
> It looks okay-ish for me, but that must depend on a particular theme.

For the default theme, the headers are green while their values are
orange.  The value of an empty stash is gray right now, while that of a
non-empty one is orange, just like the other headers' values.

>> Maybe better to display the empty Git stash header using the default
>> colors, and then highlight it differently only when it's non-empty?
>
> Not with vc-dir-ignored, though (it is based on the 'shadow' face).

Before this change, the empty stash looked the same as all other
headers' values.  This meant that there was no distinction between empty
and non-empty stashes, something that was noted as a "FIXME" in the
source code.

I think there is value in distinguishing between those two states,
though I am fine with some other choice of fallback color/face.
Currently the empty stash uses 'vc-dir-ignored' which inherits from
'shadow'.  Whereas others use 'vc-dir-header-value' which inherit from
'font-lock-variable-name-face'.

If you feel this is a problem 'vc-dir-ignored' could also inherit from
'font-lock-variable-name-face'.

-- 
Protesilaos Stavrou
protesilaos.com

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Juri Linkov]

>>> Maybe better to display the empty Git stash header using the default
>>> colors, and then highlight it differently only when it's non-empty?
>>
>> Not with vc-dir-ignored, though (it is based on the 'shadow' face).
>
> Before this change, the empty stash looked the same as all other
> headers' values.

It could continue looking the same as all other headers' values.

> This meant that there was no distinction between empty and non-empty
> stashes, something that was noted as a "FIXME" in the source code.

Indeed, it could be better to have a distinction between empty and
non-empty stashes.  The FIXME proposed to use a different face
when nothing is stashed.

But it seems better to use a different face when something is stashed.
It's important to attract user attention to the fact that there are
stashes laying around.

> I think there is value in distinguishing between those two states,
> though I am fine with some other choice of fallback color/face.
> Currently the empty stash uses 'vc-dir-ignored' which inherits from
> 'shadow'.  Whereas others use 'vc-dir-header-value' which inherit from
> 'font-lock-variable-name-face'.

Could some of new vc-dir faces that you added recently
be used for non-empty stashes?

> If you feel this is a problem 'vc-dir-ignored' could also inherit from
> 'font-lock-variable-name-face'.

It's good that 'vc-dir-ignored' inherits from the 'shadow' face
when it's used on files with the "ignored" status.  Then it
could be renamed to 'vc-dir-status-ignored' as you already proposed.

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Dmitry Gutov]

On 09.02.2021 07:01, Protesilaos Stavrou wrote:
> I also think that 4 faces should suffice.  Having checked the doc string
> of 'vc-state' this is how I feel they should be organised.
> 
> | status           | face ("?" means suggestion) |
> |------------------+-----------------------------|
> | up-to-date       | vc-dir-status-up-to-date    |
> | edited           | vc-dir-status-edited        |
> | USER             | vc-dir-status-warning?      |
> | needs-update     | vc-dir-status-warning?      |
> | unlocked-changes | vc-dir-status-warning?      |
> | added            | vc-dir-status-edited        |
> | removed          | vc-dir-status-edited        |
> | conflict         | vc-dir-status-warning       |
> | missing          | vc-dir-status-warning       |
> | ignored          | vc-dir-ignored              |
> | unregistered     | vc-dir-status-edited        |

Looks good.

> With regard to 'vc-dir-ignored', do you think we should rename it to
> 'vc-dir-status-ignored' for the sake of consistency?

Yes, probably. Let's see how your discussion with Juri ends up.

But if the only one use of this face is related to stashes, perhaps 
introduce a stash-specific face instead.

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Protesilaos Stavrou]

On 2021-02-09, 11:19 +0200, Juri Linkov <juri@linkov.net> wrote:

>>>> Maybe better to display the empty Git stash header using the default
>>>> colors, and then highlight it differently only when it's non-empty?
>>>
>>> Not with vc-dir-ignored, though (it is based on the 'shadow' face).
>>
>> Before this change, the empty stash looked the same as all other
>> headers' values.
>
> It could continue looking the same as all other headers' values.

Thinking again about this, I now agree with this view.  An empty stash
is not an "inactive" status but rather a valid header value.

>> This meant that there was no distinction between empty and non-empty
>> stashes, something that was noted as a "FIXME" in the source code.
>
> Indeed, it could be better to have a distinction between empty and
> non-empty stashes.  The FIXME proposed to use a different face
> when nothing is stashed.
>
> But it seems better to use a different face when something is stashed.
> It's important to attract user attention to the fact that there are
> stashes laying around.

I actually feel that this level of distinction already exists.  When the
stash is non-empty, there is a button/link which by default is blue, so
it contrasts well with the orange header values.  This button can be
used to toggle the visibility of the stash list.  Stash entries are the
same color as all other header values, yet their presence is already
quite obvious in context.

>> I think there is value in distinguishing between those two states,
>> though I am fine with some other choice of fallback color/face.
>> Currently the empty stash uses 'vc-dir-ignored' which inherits from
>> 'shadow'.  Whereas others use 'vc-dir-header-value' which inherit from
>> 'font-lock-variable-name-face'.
>
> Could some of new vc-dir faces that you added recently
> be used for non-empty stashes?

In principle yes, though I now believe that we do not need to introduce
such a qualification.

>> If you feel this is a problem 'vc-dir-ignored' could also inherit from
>> 'font-lock-variable-name-face'.
>
> It's good that 'vc-dir-ignored' inherits from the 'shadow' face
> when it's used on files with the "ignored" status.  Then it
> could be renamed to 'vc-dir-status-ignored' as you already proposed.

Yes, it should be renamed.

I will now use this information, as well as what Dmitry shared in the
other comment to prepare a new patch that covers everything.  You can
all test it before it gets applied.  I will share it in this thread
either later in the day or tomorrow.

Thank you!

-- 
Protesilaos Stavrou
protesilaos.com

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Protesilaos Stavrou]

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

On 2021-02-09, 18:30 +0200, Protesilaos Stavrou <info@protesilaos.com> wrote:

> I will now use this information, as well as what Dmitry shared in the
> other comment to prepare a new patch that covers everything.  You can
> all test it before it gets applied.  I will share it in this thread
> either later in the day or tomorrow.
>
> Thank you!

Hello again!

I have prepared a new patch.

1. Incorporate the feedback received thus far:

   + Do not apply special treatment to the Git stash header.  It now
     looks the same as all other headers.

   + Rename 'vc-dir-ignored' to 'vc-dir-status-ignored'.

   + Apply 'vc-dir-status-ignored' to gitignored files.

2. Implement the new faces in all backends, while ensuring that every
   value documented for 'vc-state' is taken into account.

   + Please double check that the 'rev-and-lock' state is correct: it is
     the only one I could infer from the context of what the USER of
     'vc-state' is.

If you think there is something else that remains to be done, please let
me know.

Thank you for your time!

--
Protesilaos Stavrou
protesilaos.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Refine-use-of-vc-dir-faces-apply-to-all-backends.patch --]
[-- Type: text/x-patch, Size: 9372 bytes --]

From 052eec0c4d838e6d98ae165b0664cf428463fbdb Mon Sep 17 00:00:00 2001
Message-Id: <052eec0c4d838e6d98ae165b0664cf428463fbdb.1612891772.git.info@protesilaos.com>
From: Protesilaos Stavrou <info@protesilaos.com>
Date: Tue, 9 Feb 2021 06:49:05 +0200
Subject: [PATCH] Refine use of vc-dir faces; apply to all backends

* lisp/vc/vc-dir.el (vc-default-dir-printer): Add check for the
"ignored" status and make 'vc-dir-status-edited' the default face.
Also extend condition for more states that qualify as "warnings".

(vc-dir-ignored, vc-dir-status-ignored): Rename face for consistency.

* lisp/vc/vc-git.el (vc-git-dir-printer): Use the
'vc-dir-status-edited' as the default for the Git backend.  And
reference the renamed face.  Also stop treating the empty stash
differently from other header values.

* lisp/vc/vc-bzr.el (vc-bzr-dir-extra-headers): Implement new faces.
* lisp/vc/vc-cvs.el (vc-cvs-dir-extra-headers): Same.
* lisp/vc/vc-hg.el (vc-hg-dir-extra-headers): Same.
* lisp/vc/vc-svn.el (vc-svn-dir-extra-headers): Same.

This follows from the discussion in bug#46358.
---
 lisp/vc/vc-bzr.el | 24 ++++++++++++------------
 lisp/vc/vc-cvs.el | 18 +++++++++---------
 lisp/vc/vc-dir.el | 12 +++++++-----
 lisp/vc/vc-git.el |  5 +++--
 lisp/vc/vc-hg.el  |  4 ++--
 lisp/vc/vc-svn.el |  4 ++--
 6 files changed, 35 insertions(+), 32 deletions(-)

diff --git a/lisp/vc/vc-bzr.el b/lisp/vc/vc-bzr.el
index c495afb6ec..d1385ea778 100644
--- a/lisp/vc/vc-bzr.el
+++ b/lisp/vc/vc-bzr.el
@@ -1076,49 +1076,49 @@ vc-bzr-dir-extra-headers
 	  (when (string-match ".+checkout of branch: \\(.+\\)$" str)
 	    (match-string 1 str)))))
     (concat
-     (propertize "Parent branch      : " 'face 'font-lock-type-face)
+     (propertize "Parent branch      : " 'face 'vc-dir-header)
      (propertize
       (if (string-match "parent branch: \\(.+\\)$" str)
  	  (match-string 1 str)
  	"None")
-       'face 'font-lock-variable-name-face)
+       'face 'vc-dir-header-value)
      "\n"
       (when light-checkout
 	(concat
-	 (propertize "Light checkout root: " 'face 'font-lock-type-face)
-	 (propertize light-checkout 'face 'font-lock-variable-name-face)
+	 (propertize "Light checkout root: " 'face 'vc-dir-header)
+	 (propertize light-checkout 'face 'vc-dir-header-value)
 	 "\n"))
       (when light-checkout-branch
 	(concat
-	 (propertize "Checkout of branch : " 'face 'font-lock-type-face)
-	 (propertize light-checkout-branch 'face 'font-lock-variable-name-face)
+	 (propertize "Checkout of branch : " 'face 'vc-dir-header)
+	 (propertize light-checkout-branch 'face 'vc-dir-header-value)
 	 "\n"))
       (when pending-merge
 	(concat
-	 (propertize "Warning            : " 'face 'font-lock-warning-face
+	 (propertize "Warning            : " 'face 'vc-dir-status-warning
 		     'help-echo pending-merge-help-echo)
 	 (propertize "Pending merges, commit recommended before any other action"
 		     'help-echo pending-merge-help-echo
-		     'face 'font-lock-warning-face)
+		     'face 'vc-dir-status-warning)
 	 "\n"))
       (if shelve
 	  (concat
-	   (propertize "Shelves            :\n" 'face 'font-lock-type-face
+	   (propertize "Shelves            :\n" 'face 'vc-dir-header
 		       'help-echo shelve-help-echo)
 	   (mapconcat
 	    (lambda (x)
 	      (propertize x
-			  'face 'font-lock-variable-name-face
+			  'face 'vc-dir-header-value
 			  'mouse-face 'highlight
 			  'help-echo "mouse-3: Show shelve menu\nA: Apply and keep shelf\nP: Apply and remove shelf (pop)\nS: Snapshot to a shelf\nC-k: Delete shelf"
 			  'keymap vc-bzr-shelve-map))
 	    shelve "\n"))
 	(concat
-	 (propertize "Shelves            : " 'face 'font-lock-type-face
+	 (propertize "Shelves            : " 'face 'vc-dir-header
 		     'help-echo shelve-help-echo)
 	 (propertize "No shelved changes"
 		     'help-echo shelve-help-echo
-		     'face 'font-lock-variable-name-face))))))
+		     'face 'vc-dir-header-value))))))
 
 ;; Follows vc-bzr-command, which uses vc-do-command from vc-dispatcher.
 (declare-function vc-resynch-buffer "vc-dispatcher"
diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index a595cc9778..0adb5328bc 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -1047,29 +1047,29 @@ vc-cvs-dir-extra-headers
 	   (file-error nil))))
     (concat
      (cond (repo
-	    (concat (propertize "Repository : " 'face 'font-lock-type-face)
-                    (propertize repo 'face 'font-lock-variable-name-face)))
+	    (concat (propertize "Repository : " 'face 'vc-dir-header)
+                    (propertize repo 'face 'vc-dir-header-value)))
 	   (t ""))
      (cond (module
-	    (concat (propertize "Module     : " 'face 'font-lock-type-face)
-                    (propertize module 'face 'font-lock-variable-name-face)))
+	    (concat (propertize "Module     : " 'face 'vc-dir-header)
+                    (propertize module 'face 'vc-dir-header-value)))
 	   (t ""))
      (if (file-readable-p "CVS/Tag")
 	 (let ((tag (vc-cvs-file-to-string "CVS/Tag")))
 	   (cond
 	    ((string-match "\\`T" tag)
-	     (concat (propertize "Tag        : " 'face 'font-lock-type-face)
+	     (concat (propertize "Tag        : " 'face 'vc-dir-header)
 		     (propertize (substring tag 1)
-				 'face 'font-lock-variable-name-face)))
+				 'face 'vc-dir-header-value)))
 	    ((string-match "\\`D" tag)
-	     (concat (propertize "Date       : " 'face 'font-lock-type-face)
+	     (concat (propertize "Date       : " 'face 'vc-dir-header)
 		     (propertize (substring tag 1)
-				 'face 'font-lock-variable-name-face)))
+				 'face 'vc-dir-header-value)))
 	    (t ""))))
 
      ;; In CVS, branch is a per-file property, not a per-directory property.
      ;; We can't really do this here without making dangerous assumptions.
-     ;;(propertize "Branch:     " 'face 'font-lock-type-face)
+     ;;(propertize "Branch:     " 'face 'vc-dir-header)
      ;;(propertize "ADD CODE TO PRINT THE BRANCH NAME\n"
      ;;	 'face 'font-lock-warning-face)
      )))
diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 14c81578b7..d86b89bf80 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -86,7 +86,7 @@ vc-dir-status-up-to-date
   "Face for up-to-date status in VC-dir buffers."
   :group 'vc)
 
-(defface vc-dir-ignored '((t :inherit shadow))
+(defface vc-dir-status-ignored '((t :inherit shadow))
   "Face for ignored or empty values in VC-dir buffers."
   :group 'vc)
 
@@ -1454,10 +1454,12 @@ vc-default-dir-printer
      "   "
      (propertize
       (format "%-20s" state)
-      'face (cond ((eq state 'up-to-date) 'vc-dir-status-up-to-date)
-		  ((memq state '(missing conflict)) 'vc-dir-status-warning)
-		  ((eq state 'edited) 'font-lock-constant-face)
-		  (t 'vc-dir-header-value))
+      'face (cond
+             ((eq state 'up-to-date) 'vc-dir-status-up-to-date)
+             ((memq state '(missing conflict needs-update unlocked-changes rev-and-lock))
+              'vc-dir-status-warning)
+             ((eq state 'ignored) 'vc-dir-status-ignored)
+             (t 'vc-dir-status-edited))
       'mouse-face 'highlight
       'keymap vc-dir-status-mouse-map)
      " "
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index e7306386fe..25ae26d746 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -479,7 +479,8 @@ vc-git-dir-printer
      (propertize
       (format "%-12s" state)
       'face (cond ((eq state 'up-to-date) 'vc-dir-status-up-to-date)
-                  ((eq state '(missing conflict)) 'vc-dir-status-warning)
+                  ((memq state '(missing conflict)) 'vc-dir-status-warning)
+                  ((eq state 'ignored) 'vc-dir-status-ignored)
                   (t 'vc-dir-status-edited))
       'mouse-face 'highlight
       'keymap vc-dir-status-mouse-map)
@@ -835,7 +836,7 @@ vc-git-dir-extra-headers
 	(propertize "Nothing stashed"
 		    'help-echo vc-git-stash-shared-help
                     'keymap vc-git-stash-shared-map
-		    'face 'vc-dir-ignored))))))
+		    'face 'vc-dir-header-value))))))
 
 (defun vc-git-branches ()
   "Return the existing branches, as a list of strings.
diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index 1d163a64ab..adb0fce875 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -1403,8 +1403,8 @@ vc-hg-dir-extra-headers
                              (cons (capitalize (match-string 1)) (match-string 2))
                            (cons "" (buffer-substring (point) (line-end-position))))))
               (concat
-               (propertize (format "%-11s: " (car entry)) 'face 'font-lock-type-face)
-               (propertize (cdr entry) 'face 'font-lock-variable-name-face)))
+               (propertize (format "%-11s: " (car entry)) 'face 'vc-dir-header)
+               (propertize (cdr entry) 'face 'vc-dir-header-value)))
             result)
            (forward-line))
          (nreverse result))
diff --git a/lisp/vc/vc-svn.el b/lisp/vc/vc-svn.el
index da5471107d..22becc91cd 100644
--- a/lisp/vc/vc-svn.el
+++ b/lisp/vc/vc-svn.el
@@ -239,8 +239,8 @@ vc-svn-dir-extra-headers
     (concat
      (cond (repo
 	    (concat
-	     (propertize "Repository : " 'face 'font-lock-type-face)
-	     (propertize repo 'face 'font-lock-variable-name-face)))
+	     (propertize "Repository : " 'face 'vc-dir-header)
+	     (propertize repo 'face 'vc-dir-header-value)))
 	   (t "")))))
 
 (defun vc-svn-working-revision (file)
-- 
2.30.0

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Dmitry Gutov]

Hello!

On 09.02.2021 19:46, Protesilaos Stavrou wrote:

> I have prepared a new patch.
> 
> 1. Incorporate the feedback received thus far:
> 
>     + Do not apply special treatment to the Git stash header.  It now
>       looks the same as all other headers.
> 
>     + Rename 'vc-dir-ignored' to 'vc-dir-status-ignored'.
> 
>     + Apply 'vc-dir-status-ignored' to gitignored files.
> 
> 2. Implement the new faces in all backends, while ensuring that every
>     value documented for 'vc-state' is taken into account.

Thanks!

Looking good, I've applied and pushed this patch. Except for one thing:

>     + Please double check that the 'rev-and-lock' state is correct: it is
>       the only one I could infer from the context of what the USER of
>       'vc-state' is.

USER denotes string values (which will denote being locked by that user).

I don't think the symbol `rev-and-lock' can ever be the value returned 
by 'vc-state', though I'll have to admit not really understanding what 
vc-rcs-consult-headers does.

So I removed that bit. Someone should feel free to correct me.

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Protesilaos Stavrou]

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

On 2021-02-10, 03:48 +0200, Dmitry Gutov <dgutov@yandex.ru> wrote:

> Hello!
>
> On 09.02.2021 19:46, Protesilaos Stavrou wrote:
>
>> I have prepared a new patch.
>> 1. Incorporate the feedback received thus far:
>>     + Do not apply special treatment to the Git stash header.  It now
>>       looks the same as all other headers.
>>     + Rename 'vc-dir-ignored' to 'vc-dir-status-ignored'.
>>     + Apply 'vc-dir-status-ignored' to gitignored files.
>> 2. Implement the new faces in all backends, while ensuring that every
>>     value documented for 'vc-state' is taken into account.
>
> Thanks!
>
> Looking good, I've applied and pushed this patch. Except for one thing:

I just realised that I forgot to update the NEWS entry.  Patch
attached.  Sorry about that!

-- 
Protesilaos Stavrou
protesilaos.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Update-NEWS-entry-for-vc-dir-faces.patch --]
[-- Type: text/x-patch, Size: 1233 bytes --]

From 5314791cc3a85bf5786487f4740d30f20e941e01 Mon Sep 17 00:00:00 2001
Message-Id: <5314791cc3a85bf5786487f4740d30f20e941e01.1612929824.git.info@protesilaos.com>
From: Protesilaos Stavrou <info@protesilaos.com>
Date: Wed, 10 Feb 2021 06:03:33 +0200
Subject: [PATCH] Update NEWS entry for vc-dir faces

* NEWS: Remove reference to specific backend, as it now applies to all
of them.  Update name of 'vc-dir-status-ignored'.

This follows from the discussion in bug#46358.
---
 etc/NEWS | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index bd209de18e..3cbf2a0fe7 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -602,10 +602,11 @@ This is used when expanding commit messages from 'vc-print-root-log'
 and similar commands.
 
 ---
-*** New faces for 'vc-dir' buffers and their Git VC backend.
+*** New faces for 'vc-dir' buffers.
 Those are: 'vc-dir-header', 'vc-dir-header-value', 'vc-dir-directory',
 'vc-dir-file', 'vc-dir-mark-indicator', 'vc-dir-status-warning',
-'vc-dir-status-edited', 'vc-dir-status-up-to-date', 'vc-dir-ignored'.
+'vc-dir-status-edited', 'vc-dir-status-up-to-date',
+'vc-dir-status-ignored'.
 
 ---
 *** The responsible VC backend is now the most specific one.
-- 
2.30.0

bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

[Dmitry Gutov]

On 10.02.2021 06:06, Protesilaos Stavrou wrote:
> I just realised that I forgot to update the NEWS entry.  Patch
> attached.  Sorry about that!

Applied, thank you.