GNU bug report logs - #10181
24.0.92; [wishlist] split `diff-refine-change' in several faces

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 10181 in the body.
You can then email your comments to 10181 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dani Moncayo <dmoncayo <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.0.92; [wishlist] split `diff-refine-change' in several faces
Date: Thu, 1 Dec 2011 16:56:00 +0100

Severity: wishlist

Please, split the `diff-refine-change' face in several faces: one for
each of the possible "surrounding" face:
* for `diff-changed', define `diff-changed-refined'.
* for `diff-removed', define `diff-removed-refined'.
* for `diff-added', define `diff-added-refined'.

Why? --> Because users (like me) may want to define such "refined"
faces based on the "not-refined" ones.  Currently this is not
possible, so I cannot obtain a satisfactory aspect of the refined
hunks.

TIA.


In GNU Emacs 24.0.92.1 (i386-mingw-nt5.1.2600)
 of 2011-11-30 on DANI-PC
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.6) --no-opt --cflags
-fno-omit-frame-pointer'

-- 
Dani Moncayo

Message #8 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Dani Moncayo <dmoncayo <at> gmail.com>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Fri, 02 Dec 2011 12:47:45 +0200

> Please, split the `diff-refine-change' face in several faces: one for
> each of the possible "surrounding" face:
> * for `diff-changed', define `diff-changed-refined'.
> * for `diff-removed', define `diff-removed-refined'.
> * for `diff-added', define `diff-added-refined'.
>
> Why? --> Because users (like me) may want to define such "refined"
> faces based on the "not-refined" ones.  Currently this is not
> possible, so I cannot obtain a satisfactory aspect of the refined
> hunks.

Good idea.  Then it would be possible to create a nice color scheme by
customizing `diff-removed-refined' to dark red and `diff-added-refined'
to dark green, when `diff-removed' is customized to light red and
`diff-added' to light green.

Message #11 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Thu, 17 May 2012 03:33:23 +0300

> Please, split the `diff-refine-change' face in several faces: one for
> each of the possible "surrounding" face:
> * for `diff-changed', define `diff-changed-refined'.
> * for `diff-removed', define `diff-removed-refined'.
> * for `diff-added', define `diff-added-refined'.
>
> Why? --> Because users (like me) may want to define such "refined"
> faces based on the "not-refined" ones.  Currently this is not
> possible, so I cannot obtain a satisfactory aspect of the refined hunks.

This could be implemented exactly the same way as currently other
diff faces are implemented where `diff-removed' and `diff-added'
both inherit from `diff-changed' by default.  Anyone wishing to use
different faces to highlight removed and added lines can customize
`diff-removed' and `diff-added' to not inherit from `diff-changed'.

`diff-indicator-changed', `diff-indicator-removed' and
`diff-indicator-added' are defined with inheritance as well.

So the following patch does the same by defining new faces
`diff-refine-removed' and `diff-refine-added' inheriting from
`diff-refine-change'.  And `smerge-refined-removed' with
`smerge-refined-added' inheriting from `smerge-refined-change'
by default:

=== modified file 'lisp/vc/diff-mode.el'
--- lisp/vc/diff-mode.el	2012-05-01 02:48:41 +0000
+++ lisp/vc/diff-mode.el	2012-05-17 00:27:59 +0000
@@ -1866,6 +1873,18 @@ (defface diff-refine-change
   "Face used for char-based changes shown by `diff-refine-hunk'."
   :group 'diff-mode)
 
+(defface diff-refine-removed
+  '((t :inherit diff-refine-change))
+  "Face used for removed characters shown by `diff-refine-hunk'."
+  :group 'diff-mode
+  :version "24.2")
+
+(defface diff-refine-added
+  '((t :inherit diff-refine-change))
+  "Face used for added characters shown by `diff-refine-hunk'."
+  :group 'diff-mode
+  :version "24.2")
+
 (defun diff-refine-preproc ()
   (while (re-search-forward "^[+>]" nil t)
     ;; Remove spurious changes due to the fact that one side of the hunk is
@@ -1879,7 +1898,7 @@ (defun diff-refine-preproc ()
   )
 
 (declare-function smerge-refine-subst "smerge-mode"
-                  (beg1 end1 beg2 end2 props &optional preproc))
+                  (beg1 end1 beg2 end2 props &optional preproc props2))
 
 (defun diff-refine-hunk ()
   "Highlight changes of hunk at point at a finer granularity."
@@ -1890,7 +1909,8 @@ (defun diff-refine-hunk ()
     (let* ((start (point))
            (style (diff-hunk-style))    ;Skips the hunk header as well.
            (beg (point))
-           (props '((diff-mode . fine) (face diff-refine-change)))
+           (props  '((diff-mode . fine) (face diff-refine-removed)))
+           (props2 '((diff-mode . fine) (face diff-refine-added)))
            ;; Be careful to go back to `start' so diff-end-of-hunk gets
            ;; to read the hunk header's line info.
            (end (progn (goto-char start) (diff-end-of-hunk) (point))))
@@ -1904,7 +1924,7 @@ (defun diff-refine-hunk ()
                                    end t)
            (smerge-refine-subst (match-beginning 0) (match-end 1)
                                 (match-end 1) (match-end 0)
-                                props 'diff-refine-preproc)))
+                                props 'diff-refine-preproc props2)))
         (context
          (let* ((middle (save-excursion (re-search-forward "^---")))
                 (other middle))
@@ -1916,14 +1936,14 @@ (defun diff-refine-hunk ()
                                     (setq other (match-end 0))
                                     (match-beginning 0))
                                   other
-                                  props 'diff-refine-preproc))))
+                                  props 'diff-refine-preproc props2))))
         (t ;; Normal diffs.
          (let ((beg1 (1+ (point))))
            (when (re-search-forward "^---.*\n" end t)
              ;; It's a combined add&remove, so there's something to do.
              (smerge-refine-subst beg1 (match-beginning 0)
                                   (match-end 0) end
-                                  props 'diff-refine-preproc))))))))
+                                  props 'diff-refine-preproc props2))))))))
 
 (defun diff-undo (&optional arg)
   "Perform `undo', ignoring the buffer's read-only status."

=== modified file 'lisp/vc/smerge-mode.el'
--- lisp/vc/smerge-mode.el	2012-05-04 23:16:47 +0000
+++ lisp/vc/smerge-mode.el	2012-05-17 00:28:03 +0000
@@ -128,6 +128,18 @@ (defface smerge-refined-change
   "Face used for char-based changes shown by `smerge-refine'."
   :group 'smerge)
 
+(defface smerge-refined-removed
+  '((t :inherit smerge-refined-change))
+  "Face used for removed characters shown by `smerge-refine'."
+  :group 'smerge
+  :version "24.2")
+
+(defface smerge-refined-added
+  '((t :inherit smerge-refined-change))
+  "Face used for added characters shown by `smerge-refine'."
+  :group 'smerge
+  :version "24.2")
+
 (easy-mmode-defmap smerge-basic-map
   `(("n" . smerge-next)
     ("p" . smerge-prev)
@@ -980,9 +992,11 @@ (defun smerge-refine-highlight-change (b
           (dolist (x props) (overlay-put ol (car x) (cdr x)))
           ol)))))
 
-(defun smerge-refine-subst (beg1 end1 beg2 end2 props &optional preproc)
+(defun smerge-refine-subst (beg1 end1 beg2 end2 props &optional preproc props2)
   "Show fine differences in the two regions BEG1..END1 and BEG2..END2.
-PROPS is an alist of properties to put (via overlays) on the changes.
+PROPS is an alist of properties to put (via overlays) on the changes,
+or only on removed characters when PROPS2 is non-nil.
+PROPS2 is an alist of properties to put on added characters.
 If non-nil, PREPROC is called with no argument in a buffer that contains
 a copy of a region, just before preparing it to for `diff'.  It can be
 used to replace chars to try and eliminate some spurious differences."
@@ -1029,7 +1043,7 @@ (defun smerge-refine-subst (beg1 end1 be
                         (smerge-refine-highlight-change buf beg1 m1 m2 props)))
                 (when (memq op '(?a ?c))
                   (setq last2
-                        (smerge-refine-highlight-change buf beg2 m4 m5 props))))
+                        (smerge-refine-highlight-change buf beg2 m4 m5 (or props2 props)))))
               (forward-line 1)                            ;Skip hunk header.
               (and (re-search-forward "^[0-9]" nil 'move) ;Skip hunk body.
                    (goto-char (match-beginning 0))))
@@ -1091,7 +1105,10 @@ (defun smerge-refine (&optional part)
     (smerge-refine-subst (match-beginning n1) (match-end n1)
                          (match-beginning n2)  (match-end n2)
                          '((smerge . refine)
-                           (face . smerge-refined-change)))))
+                           (face . smerge-refined-removed))
+			 nil
+			 '((smerge . refine)
+                           (face . smerge-refined-added)))))
 
 (defun smerge-diff (n1 n2)
   (smerge-match-conflict)

Message #14 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Wed, 16 May 2012 21:37:33 -0400

>> each of the possible "surrounding" face:
>> * for `diff-changed', define `diff-changed-refined'.
>> * for `diff-removed', define `diff-removed-refined'.
>> * for `diff-added', define `diff-added-refined'.
>> 
>> Why? --> Because users (like me) may want to define such "refined"
>> faces based on the "not-refined" ones.  Currently this is not
>> possible, so I cannot obtain a satisfactory aspect of the refined hunks.

> This could be implemented exactly the same way as currently other
> diff faces are implemented where `diff-removed' and `diff-added'
> both inherit from `diff-changed' by default.  Anyone wishing to use

I think this only covers part of what is requested, because it doesn't
distinguish "changed" (meaning that the modification replaces something
with something else) from "added"/"removed".


        Stefan

Message #17 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Fri, 18 May 2012 03:32:45 +0300

>> This could be implemented exactly the same way as currently other
>> diff faces are implemented where `diff-removed' and `diff-added'
>> both inherit from `diff-changed' by default.  Anyone wishing to use
>
> I think this only covers part of what is requested, because it doesn't
> distinguish "changed" (meaning that the modification replaces something
> with something else) from "added"/"removed".

A modification can be represented as deletion of old lines with subsequent
insertion of new lines.

This is what the unified diff format cleverly does with just
`+' and `-' indicators.

Unfortunately, the context diff format uses an additional indicator `!'
that is ambiguous whether it indicates deletions or insertions.

I see no other way to highlight deletions and insertions of the context
diff format in distinct faces than to search for the --- line that
separates them.  This is possible thanks to conditionalized font-locking.

To not slow down font-locking of context diffs by default
it could check if `diff-added' and `diff-removed' inherit
from `diff-changed', and not search for the --- line in this case
when deletions and insertions will be highlighted in the same face.
If this is too ugly then we could add a new defcustom to define
user preference.

But at least this patch keeps the same highlighting of changes
after executing `diff-unified->context' that adds the `!' indicator
when the faces `diff-added' and `diff-removed' are customized
not to inherit from `diff-changed':

=== modified file 'lisp/vc/diff-mode.el'
--- lisp/vc/diff-mode.el	2012-05-01 02:48:41 +0000
+++ lisp/vc/diff-mode.el	2012-05-18 00:32:18 +0000
@@ -393,7 +393,21 @@ (defvar diff-font-lock-keywords
     ("^\\([+>]\\)\\(.*\n\\)"
      (1 diff-indicator-added-face) (2 diff-added-face))
     ("^\\(!\\)\\(.*\n\\)"
-     (1 diff-indicator-changed-face) (2 diff-changed-face))
+     (1 diff-indicator-changed-face)
+     (2
+      ;; When the default face definitions are not customized and
+      ;; `diff-added' and `diff-removed' inherit from `diff-changed'
+      ;; then just use `diff-changed-face'.
+      ;; Otherwise, search for `diff-context-mid-hunk-header-re' and
+      ;; if the line of context diff is above, use `diff-removed-face';
+      ;; if below, use `diff-added-face'.
+      (if (and (eq (face-attribute 'diff-added   :inherit) 'diff-changed)
+               (eq (face-attribute 'diff-removed :inherit) 'diff-changed))
+         diff-changed-face
+       (let ((limit (save-excursion (diff-beginning-of-hunk))))
+         (if (save-excursion (re-search-backward diff-context-mid-hunk-header-re limit t))
+             diff-added-face
+           diff-removed-face)))))
     ("^\\(?:Index\\|revno\\): \\(.+\\).*\n"
      (0 diff-header-face) (1 diff-index-face prepend))
     ("^Only in .*\n" . diff-nonexistent-face)

Message #20 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Fri, 18 May 2012 14:12:05 -0400

Thank Juri.  The patch looks like a good idea.  Checking the :inherit is
not sufficient, because I'd typically modify the added-face without
removing the inheritance.
Better to add a customization variable.

This said, the OP's request is about the diff-refine-hook case, not
about the whole-line highlighting.


        Stefan

Message #23 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Sat, 19 May 2012 02:07:14 +0300

> Thank Juri.  The patch looks like a good idea.  Checking the :inherit is
> not sufficient, because I'd typically modify the added-face without
> removing the inheritance.
> Better to add a customization variable.
>
> This said, the OP's request is about the diff-refine-hook case, not
> about the whole-line highlighting.

The reason why I picked this report is because I found that I can't
anymore use diff-mode without these improvements.  I suppose everyone
dealing with the UI of modern version control systems including our very own
http://bzr.savannah.gnu.org/lh/emacs/trunk/changes
that uses red to highlight diff deletions and green to highlight
diff insertions might feel uncomfortable with the current default
Emacs colored diff.

AFAICS, some users might prefer an older color scheme with three colors
(removed - red, changed - yellow, added - green) used in the UI of older
version control systems like ViewVC for CVS and Subversion, but some users
might prefer a color scheme used in newer VCS like Loggerhead for Bazaar
with just two colors (removed - red, added - green).

The diff-refine-hook case depends on this customization, i.e. when the user
prefers the removed-changed-added scheme, then the face `diff-refine-change'
should be used.  When the user prefers the removed-added scheme, then
only `diff-refine-removed' and `diff-refine-added' should be used.

One solution would be to define `diff-changed-face' (currently defvar)
with defcustom.  Then users will be able to customize it to nil to use
just two faces `diff-removed' and `diff-added'.

Message #26 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Sat, 19 May 2012 14:24:21 -0400

> One solution would be to define `diff-changed-face' (currently defvar)
> with defcustom.  Then users will be able to customize it to nil to use
> just two faces `diff-removed' and `diff-added'.

I think

  (defvar diff-use-changed (not (or (face-equal diff-changed diff-added)
                                    (face-equal diff-changed diff-removed))))

would be good enough (together with the rest of your patch, which
decides whether to use added-vs-changed or removed-vs-changed.


        Stefan

Message #29 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Sun, 20 May 2012 02:55:58 +0300

>> One solution would be to define `diff-changed-face' (currently defvar)
>> with defcustom.  Then users will be able to customize it to nil to use
>> just two faces `diff-removed' and `diff-added'.
>
> I think
>
>   (defvar diff-use-changed (not (or (face-equal diff-changed diff-added)
>                                     (face-equal diff-changed diff-removed))))
>
> would be good enough (together with the rest of your patch, which
> decides whether to use added-vs-changed or removed-vs-changed.

Another variant to try is to check the customization state of these faces.
This gives more flexibility.  What I'm trying to achieve is to prepare
a color scheme used on most modern version control systems that would be
easy for users to enable in diff mode.  At the same time I'd rather be careful
not to ignite a flamewar about the default highlighting.

So a new version of the patch below keeps the current default highlighting
that highlights all changes with one face `diff-changed' for users who
not customized `diff-removed' and `diff-added' to non-default values.

Users who customized `diff-removed', `diff-added' as well as
`diff-changed' to non-default values will see the same highlighting
as before this patch.

Users who want to use the modern color scheme with red for removed and
green for added lines could customize the new variable `diff-color-scheme'
to the `removed-added' value.

If this heuristic for the default value of `diff-color-scheme'
is acceptable then I'll adapt the diff-refine-hook case to these
three color schemes.

=== modified file 'lisp/vc/diff-mode.el'
--- lisp/vc/diff-mode.el	2012-05-01 02:48:41 +0000
+++ lisp/vc/diff-mode.el	2012-05-19 23:54:38 +0000
@@ -277,14 +275,20 @@ (define-obsolete-face-alias 'diff-hunk-h
 (defvar diff-hunk-header-face 'diff-hunk-header)
 
 (defface diff-removed
-  '((t :inherit diff-changed))
+  '((((class color) (min-colors 88))
+     :background "#ffdddd")
+    (((class color))
+     :foreground "red"))
   "`diff-mode' face used to highlight removed lines."
   :group 'diff-mode)
 (define-obsolete-face-alias 'diff-removed-face 'diff-removed "22.1")
 (defvar diff-removed-face 'diff-removed)
 
 (defface diff-added
-  '((t :inherit diff-changed))
+  '((((class color) (min-colors 88))
+     :background "#ddffdd")
+    (((class color))
+     :foreground "green"))
   "`diff-mode' face used to highlight added lines."
   :group 'diff-mode)
 (define-obsolete-face-alias 'diff-added-face 'diff-added "22.1")
@@ -369,6 +373,27 @@ (defun diff-yank-function (text)
 	  (while (re-search-backward re start t)
 	    (replace-match "" t t)))))))
 
+(defvar diff-color-scheme (cond
+			   ((not (or (get diff-removed-face 'customized-face)
+				     (get diff-removed-face 'saved-face)
+				     (get diff-added-face 'customized-face)
+				     (get diff-added-face 'saved-face)))
+			    'changed)
+			   ((or (face-equal diff-changed-face diff-added-face)
+				(face-equal diff-changed-face diff-removed-face))
+			    'removed-added)
+			   ((or (get diff-changed-face 'customized-face)
+				(get diff-changed-face 'saved-face))
+			    'changed-removed-added)
+			   (t
+			    'removed-added))
+  "Color scheme used to highlight changes.
+When the default definitions of faces `diff-removed' and `diff-added',
+are not customized then highlight all changes with the same
+face `diff-changed-face'.  This is the default scheme.
+When all three faces were customized then use them all.
+Otherwise, use just `diff-removed' and `diff-added'.")
+
 (defconst diff-hunk-header-re-unified
   "^@@ -\\([0-9]+\\)\\(?:,\\([0-9]+\\)\\)? \\+\\([0-9]+\\)\\(?:,\\([0-9]+\\)\\)? @@")
 (defconst diff-context-mid-hunk-header-re
@@ -389,11 +414,25 @@ (defvar diff-font-lock-keywords
      (0 diff-header-face)
      (2 (if (not (match-end 3)) diff-file-header-face) prepend))
     ("^\\([-<]\\)\\(.*\n\\)"
-     (1 diff-indicator-removed-face) (2 diff-removed-face))
+     (1 (if (eq diff-color-scheme 'changed) diff-indicator-changed-face diff-indicator-removed-face))
+     (2 (if (eq diff-color-scheme 'changed) diff-changed-face diff-removed-face)))
     ("^\\([+>]\\)\\(.*\n\\)"
-     (1 diff-indicator-added-face) (2 diff-added-face))
+     (1 (if (eq diff-color-scheme 'changed) diff-indicator-changed-face diff-indicator-added-face))
+     (2 (if (eq diff-color-scheme 'changed) diff-changed-face diff-added-face)))
     ("^\\(!\\)\\(.*\n\\)"
-     (1 diff-indicator-changed-face) (2 diff-changed-face))
+     (1 (if (memq diff-color-scheme '(changed changed-removed-added))
+	    diff-indicator-changed-face
+	  diff-indicator-changed-face))
+     (2
+      (if (memq diff-color-scheme '(changed changed-removed-added))
+	  diff-changed-face
+	;; Otherwise, search for `diff-context-mid-hunk-header-re' and
+	;; if the line of context diff is above, use `diff-removed-face';
+	;; if below, use `diff-added-face'.
+	(let ((limit (save-excursion (diff-beginning-of-hunk))))
+	  (if (save-excursion (re-search-backward diff-context-mid-hunk-header-re limit t))
+	      diff-added-face
+	    diff-removed-face)))))
     ("^\\(?:Index\\|revno\\): \\(.+\\).*\n"
      (0 diff-header-face) (1 diff-index-face prepend))
     ("^Only in .*\n" . diff-nonexistent-face)
@@ -1866,6 +1905,22 @@ (defface diff-refine-change
   "Face used for char-based changes shown by `diff-refine-hunk'."
   :group 'diff-mode)
 
+(defface diff-refine-removed
+  '((((class color) (min-colors 88))
+     :background "#ffaaaa")
+    (t :inherit diff-removed :inverse-video t))
+  "Face used for removed characters shown by `diff-refine-hunk'."
+  :group 'diff-mode
+  :version "24.2")
+
+(defface diff-refine-added
+  '((((class color) (min-colors 88))
+     :background "#aaffaa")
+    (t :inherit diff-added :inverse-video t))
+  "Face used for added characters shown by `diff-refine-hunk'."
+  :group 'diff-mode
+  :version "24.2")
+
 (defun diff-refine-preproc ()
   (while (re-search-forward "^[+>]" nil t)
     ;; Remove spurious changes due to the fact that one side of the hunk is

Message #32 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Sun, 20 May 2012 11:06:38 -0400

>  (defface diff-removed
> -  '((t :inherit diff-changed))
> +  '((((class color) (min-colors 88))
> +     :background "#ffdddd")
> +    (((class color))
> +     :foreground "red"))

Please keep the inheritance, even if the default overrides all fields.

> +			   ((not (or (get diff-removed-face 'customized-face)
> +				     (get diff-removed-face 'saved-face)
> +				     (get diff-added-face 'customized-face)
> +				     (get diff-added-face 'saved-face)))

Making decisions based on whether something is customized or not sounds
fundamentally wrong.
Maybe you meant something more like:

	   ((and (face-equal diff-changed-face diff-added-face)
		 (face-equal diff-changed-face diff-removed-face))

Tho, I don't see what's the point of this `changed' color-scheme: you
get the same visual result if you use the `removed-added' color scheme
and pretty much the same resource usage.  The only point of the
distinction between removed-added and changed-removed-added is to avoid
the additional resource usage when it would have no visual effect.

> +			   ((or (get diff-changed-face 'customized-face)
> +				(get diff-changed-face 'saved-face))
> +			    'changed-removed-added)

This is very wrong

> +(defface diff-refine-removed
> +  '((((class color) (min-colors 88))
> +     :background "#ffaaaa")
> +    (t :inherit diff-removed :inverse-video t))
> +  "Face used for removed characters shown by `diff-refine-hunk'."
> +  :group 'diff-mode
> +  :version "24.2")
> +
> +(defface diff-refine-added
> +  '((((class color) (min-colors 88))
> +     :background "#aaffaa")
> +    (t :inherit diff-added :inverse-video t))
> +  "Face used for added characters shown by `diff-refine-hunk'."
> +  :group 'diff-mode
> +  :version "24.2")

This is just an accidental left-over, right?


        Stefan

Message #35 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Mon, 21 May 2012 03:28:17 +0300

>>  (defface diff-removed
>> -  '((t :inherit diff-changed))
>> +  '((((class color) (min-colors 88))
>> +     :background "#ffdddd")
>> +    (((class color))
>> +     :foreground "red"))
>
> Please keep the inheritance, even if the default overrides all fields.

Point taken, in the new version of the patch it keeps the inheritance.

> Tho, I don't see what's the point of this `changed' color-scheme: you
> get the same visual result if you use the `removed-added' color scheme
> and pretty much the same resource usage.  The only point of the
> distinction between removed-added and changed-removed-added is to avoid
> the additional resource usage when it would have no visual effect.

Agreed, and the patch adjusted accordingly.

>> +(defface diff-refine-added
>> +  '((((class color) (min-colors 88))
>> +     :background "#aaffaa")
>> +    (t :inherit diff-added :inverse-video t))
>> +  "Face used for added characters shown by `diff-refine-hunk'."
>> +  :group 'diff-mode
>> +  :version "24.2")
>
> This is just an accidental left-over, right?

This is a left-over from the second part of the patch that deals with
the diff-refine-hook case.  The whole patch is:

=== modified file 'lisp/vc/diff-mode.el'
--- lisp/vc/diff-mode.el	2012-05-01 02:48:41 +0000
+++ lisp/vc/diff-mode.el	2012-05-21 00:27:43 +0000
@@ -277,14 +275,32 @@ (define-obsolete-face-alias 'diff-hunk-h
 (defvar diff-hunk-header-face 'diff-hunk-header)
 
 (defface diff-removed
-  '((t :inherit diff-changed))
+  '((((class color) (min-colors 88))
+     :inherit diff-changed
+     :background "#ffdddd")
+    (((class color))
+     :inherit diff-changed
+     :foreground "red"
+     :weight normal
+     :slant normal)
+    (t
+     :inherit diff-changed))
   "`diff-mode' face used to highlight removed lines."
   :group 'diff-mode)
 (define-obsolete-face-alias 'diff-removed-face 'diff-removed "22.1")
 (defvar diff-removed-face 'diff-removed)
 
 (defface diff-added
-  '((t :inherit diff-changed))
+  '((((class color) (min-colors 88))
+     :inherit diff-changed
+     :background "#ddffdd")
+    (((class color))
+     :inherit diff-changed
+     :foreground "green"
+     :weight normal
+     :slant normal)
+    (t
+     :inherit diff-changed))
   "`diff-mode' face used to highlight added lines."
   :group 'diff-mode)
 (define-obsolete-face-alias 'diff-added-face 'diff-added "22.1")
@@ -393,7 +409,18 @@ (defvar diff-font-lock-keywords
     ("^\\([+>]\\)\\(.*\n\\)"
      (1 diff-indicator-added-face) (2 diff-added-face))
     ("^\\(!\\)\\(.*\n\\)"
-     (1 diff-indicator-changed-face) (2 diff-changed-face))
+     (1 diff-indicator-changed-face)
+     (2
+      (if (not (or (face-equal diff-changed-face diff-added-face)
+		   (face-equal diff-changed-face diff-removed-face)))
+	  diff-changed-face
+	;; Otherwise, search for `diff-context-mid-hunk-header-re' and
+	;; if the line of context diff is above, use `diff-removed-face';
+	;; if below, use `diff-added-face'.
+	(let ((limit (save-excursion (diff-beginning-of-hunk))))
+	  (if (save-excursion (re-search-backward diff-context-mid-hunk-header-re limit t))
+	      diff-added-face
+	    diff-removed-face)))))
     ("^\\(?:Index\\|revno\\): \\(.+\\).*\n"
      (0 diff-header-face) (1 diff-index-face prepend))
     ("^Only in .*\n" . diff-nonexistent-face)
@@ -1866,6 +1893,32 @@ (defface diff-refine-change
   "Face used for char-based changes shown by `diff-refine-hunk'."
   :group 'diff-mode)
 
+(defface diff-refine-removed
+  '((((class color) (min-colors 88))
+     :inherit diff-refine-change
+     :background "#ffaaaa")
+    (((class color))
+     :inherit diff-refine-change
+     :background "red")
+    (t
+     :inherit diff-refine-change))
+  "Face used for removed characters shown by `diff-refine-hunk'."
+  :group 'diff-mode
+  :version "24.2")
+
+(defface diff-refine-added
+  '((((class color) (min-colors 88))
+     :inherit diff-refine-change
+     :background "#aaffaa")
+    (((class color))
+     :inherit diff-refine-change
+     :background "green")
+    (t
+     :inherit diff-refine-change))
+  "Face used for added characters shown by `diff-refine-hunk'."
+  :group 'diff-mode
+  :version "24.2")
+
 (defun diff-refine-preproc ()
   (while (re-search-forward "^[+>]" nil t)
     ;; Remove spurious changes due to the fact that one side of the hunk is
@@ -1879,7 +1932,7 @@ (defun diff-refine-preproc ()
   )
 
 (declare-function smerge-refine-subst "smerge-mode"
-                  (beg1 end1 beg2 end2 props &optional preproc))
+                  (beg1 end1 beg2 end2 props &optional preproc props2))
 
 (defun diff-refine-hunk ()
   "Highlight changes of hunk at point at a finer granularity."
@@ -1890,7 +1943,11 @@ (defun diff-refine-hunk ()
     (let* ((start (point))
            (style (diff-hunk-style))    ;Skips the hunk header as well.
            (beg (point))
-           (props '((diff-mode . fine) (face diff-refine-change)))
+           (props-c '((diff-mode . fine) (face diff-refine-change)))
+           (props-r '((diff-mode . fine) (face diff-refine-removed)))
+           (props-a '((diff-mode . fine) (face diff-refine-added)))
+           (diff-use-changed (not (or (face-equal diff-changed-face diff-added-face)
+				      (face-equal diff-changed-face diff-removed-face))))
            ;; Be careful to go back to `start' so diff-end-of-hunk gets
            ;; to read the hunk header's line info.
            (end (progn (goto-char start) (diff-end-of-hunk) (point))))
@@ -1904,7 +1961,7 @@ (defun diff-refine-hunk ()
                                    end t)
            (smerge-refine-subst (match-beginning 0) (match-end 1)
                                 (match-end 1) (match-end 0)
-                                props 'diff-refine-preproc)))
+                                props-r 'diff-refine-preproc props-a)))
         (context
          (let* ((middle (save-excursion (re-search-forward "^---")))
                 (other middle))
@@ -1916,14 +1973,16 @@ (defun diff-refine-hunk ()
                                     (setq other (match-end 0))
                                     (match-beginning 0))
                                   other
-                                  props 'diff-refine-preproc))))
+                                  (if diff-use-changed props-c props-r)
+				  'diff-refine-preproc
+				  (if diff-use-changed props-c props-a)))))
         (t ;; Normal diffs.
          (let ((beg1 (1+ (point))))
            (when (re-search-forward "^---.*\n" end t)
              ;; It's a combined add&remove, so there's something to do.
              (smerge-refine-subst beg1 (match-beginning 0)
                                   (match-end 0) end
-                                  props 'diff-refine-preproc))))))))
+                                  props-r 'diff-refine-preproc props-a))))))))
 
 (defun diff-undo (&optional arg)
   "Perform `undo', ignoring the buffer's read-only status."

=== modified file 'lisp/vc/smerge-mode.el'
--- lisp/vc/smerge-mode.el	2012-05-04 23:16:47 +0000
+++ lisp/vc/smerge-mode.el	2012-05-21 00:27:50 +0000
@@ -128,6 +128,32 @@ (defface smerge-refined-change
   "Face used for char-based changes shown by `smerge-refine'."
   :group 'smerge)
 
+(defface smerge-refined-removed
+  '((((class color) (min-colors 88))
+     :inherit smerge-refined-change
+     :background "#ffaaaa")
+    (((class color))
+     :inherit smerge-refined-change
+     :background "red")
+    (t
+     :inherit smerge-refined-change))
+  "Face used for removed characters shown by `smerge-refine'."
+  :group 'smerge
+  :version "24.2")
+
+(defface smerge-refined-added
+  '((((class color) (min-colors 88))
+     :inherit smerge-refined-change
+     :background "#aaffaa")
+    (((class color))
+     :inherit smerge-refined-change
+     :background "green")
+    (t
+     :inherit smerge-refined-change))
+  "Face used for added characters shown by `smerge-refine'."
+  :group 'smerge
+  :version "24.2")
+
 (easy-mmode-defmap smerge-basic-map
   `(("n" . smerge-next)
     ("p" . smerge-prev)
@@ -980,9 +1006,11 @@ (defun smerge-refine-highlight-change (b
           (dolist (x props) (overlay-put ol (car x) (cdr x)))
           ol)))))
 
-(defun smerge-refine-subst (beg1 end1 beg2 end2 props &optional preproc)
+(defun smerge-refine-subst (beg1 end1 beg2 end2 props &optional preproc props2)
   "Show fine differences in the two regions BEG1..END1 and BEG2..END2.
-PROPS is an alist of properties to put (via overlays) on the changes.
+PROPS is an alist of properties to put (via overlays) on the changes,
+or only on removed characters when PROPS2 is non-nil.
+PROPS2 is an alist of properties to put on added characters.
 If non-nil, PREPROC is called with no argument in a buffer that contains
 a copy of a region, just before preparing it to for `diff'.  It can be
 used to replace chars to try and eliminate some spurious differences."
@@ -1029,7 +1057,7 @@ (defun smerge-refine-subst (beg1 end1 be
                         (smerge-refine-highlight-change buf beg1 m1 m2 props)))
                 (when (memq op '(?a ?c))
                   (setq last2
-                        (smerge-refine-highlight-change buf beg2 m4 m5 props))))
+                        (smerge-refine-highlight-change buf beg2 m4 m5 (or props2 props)))))
               (forward-line 1)                            ;Skip hunk header.
               (and (re-search-forward "^[0-9]" nil 'move) ;Skip hunk body.
                    (goto-char (match-beginning 0))))
@@ -1081,7 +1109,10 @@ (defun smerge-refine (&optional part)
                    ((eq (match-end 3) (match-beginning 3)) 3)
                    (t 2)))
   (let ((n1 (if (eq part 1) 2 1))
-        (n2 (if (eq part 3) 2 3)))
+        (n2 (if (eq part 3) 2 3))
+	(smerge-use-changed
+	 (not (or (face-equal 'smerge-refined-change 'smerge-refined-added)
+		  (face-equal 'smerge-refined-change 'smerge-refined-removed)))))
     (smerge-ensure-match n1)
     (smerge-ensure-match n2)
     (with-silent-modifications
@@ -1090,8 +1121,13 @@ (defun smerge-refine (&optional part)
                          (cons (buffer-chars-modified-tick) part)))
     (smerge-refine-subst (match-beginning n1) (match-end n1)
                          (match-beginning n2)  (match-end n2)
-                         '((smerge . refine)
-                           (face . smerge-refined-change)))))
+                         (if smerge-use-changed
+			     '((smerge . refine) (face . smerge-refined-change))
+			   '((smerge . refine) (face . smerge-refined-removed)))
+			 nil
+			 (if smerge-use-changed
+			     '((smerge . refine) (face . smerge-refined-change))
+			   '((smerge . refine) (face . smerge-refined-added))))))
 
 (defun smerge-diff (n1 n2)
   (smerge-match-conflict)

Message #38 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Sun, 20 May 2012 21:45:54 -0400

>  (defface diff-removed
> -  '((t :inherit diff-changed))
> +  '((((class color) (min-colors 88))
> +     :inherit diff-changed
> +     :background "#ffdddd")
> +    (((class color))
> +     :inherit diff-changed
> +     :foreground "red"
> +     :weight normal
> +     :slant normal)
> +    (t
> +     :inherit diff-changed))

Please move the :inherit to a `default' clause instead of copying it
into each one of the clauses.

> @@ -393,7 +409,18 @@ (defvar diff-font-lock-keywords
>      ("^\\([+>]\\)\\(.*\n\\)"
>       (1 diff-indicator-added-face) (2 diff-added-face))
>      ("^\\(!\\)\\(.*\n\\)"
> -     (1 diff-indicator-changed-face) (2 diff-changed-face))
> +     (1 diff-indicator-changed-face)
> +     (2
> +      (if (not (or (face-equal diff-changed-face diff-added-face)
> +		   (face-equal diff-changed-face diff-removed-face)))

Please introduce a defvar for it, so we don't re-evaluate the face
comparison for each and every line.

> +PROPS is an alist of properties to put (via overlays) on the changes,
> +or only on removed characters when PROPS2 is non-nil.
> +PROPS2 is an alist of properties to put on added characters.

This doesn't say that PROPS will be put on the "removed" chars.

And while I'm OK with not implementing the changed-added-remove scheme
for refinement yet, I think that if we change the API of
smerge-refine-subst for the added-removed case, we should make sure the
API won't need to be changed yet again if/when we add the
changed-added-removed scheme.


        Stefan

Message #41 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Wed, 23 May 2012 03:36:18 +0300

> Please move the :inherit to a `default' clause instead of copying it
> into each one of the clauses.

Fixed in the patch below.

>> +      (if (not (or (face-equal diff-changed-face diff-added-face)
>> +		   (face-equal diff-changed-face diff-removed-face)))
>
> Please introduce a defvar for it, so we don't re-evaluate the face
> comparison for each and every line.

This defvar needs to be re-evaluated when the user customized faces.
Since faces are used by font-lock, a good place to re-evaluate the value
of this defvar before font-lock starts is in the function `diff-mode'.

> And while I'm OK with not implementing the changed-added-remove scheme
> for refinement yet, I think that if we change the API of
> smerge-refine-subst for the added-removed case, we should make sure the
> API won't need to be changed yet again if/when we add the
> changed-added-removed scheme.

The following patch implements the API for all possible cases:

=== modified file 'lisp/vc/diff-mode.el'
--- lisp/vc/diff-mode.el	2012-05-01 02:48:41 +0000
+++ lisp/vc/diff-mode.el	2012-05-23 00:31:05 +0000
@@ -277,14 +275,28 @@ (define-obsolete-face-alias 'diff-hunk-h
 (defvar diff-hunk-header-face 'diff-hunk-header)
 
 (defface diff-removed
-  '((t :inherit diff-changed))
+  '((default
+     :inherit diff-changed)
+    (((class color) (min-colors 88))
+     :background "#ffdddd")
+    (((class color))
+     :foreground "red"
+     :weight normal
+     :slant normal))
   "`diff-mode' face used to highlight removed lines."
   :group 'diff-mode)
 (define-obsolete-face-alias 'diff-removed-face 'diff-removed "22.1")
 (defvar diff-removed-face 'diff-removed)
 
 (defface diff-added
-  '((t :inherit diff-changed))
+  '((default
+     :inherit diff-changed)
+    (((class color) (min-colors 88))
+     :background "#ddffdd")
+    (((class color))
+     :foreground "green"
+     :weight normal
+     :slant normal))
   "`diff-mode' face used to highlight added lines."
   :group 'diff-mode)
 (define-obsolete-face-alias 'diff-added-face 'diff-added "22.1")
@@ -374,6 +386,8 @@ (defconst diff-hunk-header-re-unified
 (defconst diff-context-mid-hunk-header-re
   "--- \\([0-9]+\\)\\(?:,\\([0-9]+\\)\\)? ----$")
 
+(defvar diff-use-changed-face nil)
+
 (defvar diff-font-lock-keywords
   `((,(concat "\\(" diff-hunk-header-re-unified "\\)\\(.*\\)$")
      (1 diff-hunk-header-face) (6 diff-function-face))
@@ -393,7 +407,17 @@ (defvar diff-font-lock-keywords
     ("^\\([+>]\\)\\(.*\n\\)"
      (1 diff-indicator-added-face) (2 diff-added-face))
     ("^\\(!\\)\\(.*\n\\)"
-     (1 diff-indicator-changed-face) (2 diff-changed-face))
+     (1 diff-indicator-changed-face)
+     (2
+      (if diff-use-changed-face
+	  diff-changed-face
+	;; Otherwise, search for `diff-context-mid-hunk-header-re' and
+	;; if the line of context diff is above, use `diff-removed-face';
+	;; if below, use `diff-added-face'.
+	(let ((limit (save-excursion (diff-beginning-of-hunk))))
+	  (if (save-excursion (re-search-backward diff-context-mid-hunk-header-re limit t))
+	      diff-added-face
+	    diff-removed-face)))))
     ("^\\(?:Index\\|revno\\): \\(.+\\).*\n"
      (0 diff-header-face) (1 diff-index-face prepend))
     ("^Only in .*\n" . diff-nonexistent-face)
@@ -1281,6 +1305,11 @@ (define-derived-mode diff-mode fundament
    \\{diff-mode-map}"
 
   (set (make-local-variable 'font-lock-defaults) diff-font-lock-defaults)
+  (set (make-local-variable 'diff-use-changed-face)
+       (and (face-differs-from-default-p diff-changed-face)
+	    (not (face-equal diff-changed-face diff-added-face))
+	    (not (face-equal diff-changed-face diff-removed-face))))
+
   (set (make-local-variable 'outline-regexp) diff-outline-regexp)
   (set (make-local-variable 'imenu-generic-expression)
        diff-imenu-generic-expression)
@@ -1866,6 +1895,28 @@ (defface diff-refine-change
   "Face used for char-based changes shown by `diff-refine-hunk'."
   :group 'diff-mode)
 
+(defface diff-refine-removed
+  '((default
+     :inherit diff-refine-change)
+    (((class color) (min-colors 88))
+     :background "#ffaaaa")
+    (((class color))
+     :background "red"))
+  "Face used for removed characters shown by `diff-refine-hunk'."
+  :group 'diff-mode
+  :version "24.2")
+
+(defface diff-refine-added
+  '((default
+     :inherit diff-refine-change)
+    (((class color) (min-colors 88))
+     :background "#aaffaa")
+    (((class color))
+     :background "green"))
+  "Face used for added characters shown by `diff-refine-hunk'."
+  :group 'diff-mode
+  :version "24.2")
+
 (defun diff-refine-preproc ()
   (while (re-search-forward "^[+>]" nil t)
     ;; Remove spurious changes due to the fact that one side of the hunk is
@@ -1879,7 +1930,7 @@ (defun diff-refine-preproc ()
   )
 
 (declare-function smerge-refine-subst "smerge-mode"
-                  (beg1 end1 beg2 end2 props &optional preproc))
+                  (beg1 end1 beg2 end2 props-c &optional preproc props-r props-a))
 
 (defun diff-refine-hunk ()
   "Highlight changes of hunk at point at a finer granularity."
@@ -1890,7 +1941,9 @@ (defun diff-refine-hunk ()
     (let* ((start (point))
            (style (diff-hunk-style))    ;Skips the hunk header as well.
            (beg (point))
-           (props '((diff-mode . fine) (face diff-refine-change)))
+           (props-c '((diff-mode . fine) (face diff-refine-change)))
+           (props-r '((diff-mode . fine) (face diff-refine-removed)))
+           (props-a '((diff-mode . fine) (face diff-refine-added)))
            ;; Be careful to go back to `start' so diff-end-of-hunk gets
            ;; to read the hunk header's line info.
            (end (progn (goto-char start) (diff-end-of-hunk) (point))))
@@ -1904,7 +1957,7 @@ (defun diff-refine-hunk ()
                                    end t)
            (smerge-refine-subst (match-beginning 0) (match-end 1)
                                 (match-end 1) (match-end 0)
-                                props 'diff-refine-preproc)))
+                                nil 'diff-refine-preproc props-r props-a)))
         (context
          (let* ((middle (save-excursion (re-search-forward "^---")))
                 (other middle))
@@ -1916,14 +1969,17 @@ (defun diff-refine-hunk ()
                                     (setq other (match-end 0))
                                     (match-beginning 0))
                                   other
-                                  props 'diff-refine-preproc))))
+                                  (if diff-use-changed-face props-c)
+                                  'diff-refine-preproc
+                                  (unless diff-use-changed-face props-r)
+                                  (unless diff-use-changed-face props-a)))))
         (t ;; Normal diffs.
          (let ((beg1 (1+ (point))))
            (when (re-search-forward "^---.*\n" end t)
              ;; It's a combined add&remove, so there's something to do.
              (smerge-refine-subst beg1 (match-beginning 0)
                                   (match-end 0) end
-                                  props 'diff-refine-preproc))))))))
+                                  nil 'diff-refine-preproc props-r props-a))))))))
 
 (defun diff-undo (&optional arg)
   "Perform `undo', ignoring the buffer's read-only status."

=== modified file 'lisp/vc/smerge-mode.el'
--- lisp/vc/smerge-mode.el	2012-05-04 23:16:47 +0000
+++ lisp/vc/smerge-mode.el	2012-05-23 00:34:09 +0000
@@ -128,6 +128,28 @@ (defface smerge-refined-change
   "Face used for char-based changes shown by `smerge-refine'."
   :group 'smerge)
 
+(defface smerge-refined-removed
+  '((default
+     :inherit smerge-refined-change)
+    (((class color) (min-colors 88))
+     :background "#ffaaaa")
+    (((class color))
+     :background "red"))
+  "Face used for removed characters shown by `smerge-refine'."
+  :group 'smerge
+  :version "24.2")
+
+(defface smerge-refined-added
+  '((default
+     :inherit smerge-refined-change)
+    (((class color) (min-colors 88))
+     :background "#aaffaa")
+    (((class color))
+     :background "green"))
+  "Face used for added characters shown by `smerge-refine'."
+  :group 'smerge
+  :version "24.2")
+
 (easy-mmode-defmap smerge-basic-map
   `(("n" . smerge-next)
     ("p" . smerge-prev)
@@ -980,9 +1002,17 @@ (defun smerge-refine-highlight-change (b
           (dolist (x props) (overlay-put ol (car x) (cdr x)))
           ol)))))
 
-(defun smerge-refine-subst (beg1 end1 beg2 end2 props &optional preproc)
+(defun smerge-refine-subst (beg1 end1 beg2 end2 props-c &optional preproc props-r props-a)
   "Show fine differences in the two regions BEG1..END1 and BEG2..END2.
-PROPS is an alist of properties to put (via overlays) on the changes.
+PROPS-C is an alist of properties to put (via overlays) on the changes.
+PROPS-R is an alist of properties to put on removed characters.
+PROPS-A is an alist of properties to put on added characters.
+If PROPS-R and PROPS-A are nil, put PROPS-C on all changes.
+If PROPS-C is nil, but PROPS-R and PROPS-A are non-nil,
+put PROPS-A on added characters, PROPS-R on removed characters.
+If PROPS-C, PROPS-R and PROPS-A are non-nil, put PROPS-C on changes characters,
+PROPS-A on added characters, PROPS-R on removed characters.
+
 If non-nil, PREPROC is called with no argument in a buffer that contains
 a copy of a region, just before preparing it to for `diff'.  It can be
 used to replace chars to try and eliminate some spurious differences."
@@ -1026,10 +1056,18 @@ (defun smerge-refine-subst (beg1 end1 be
                     (m5 (match-string 5)))
                 (when (memq op '(?d ?c))
                   (setq last1
-                        (smerge-refine-highlight-change buf beg1 m1 m2 props)))
+                        (smerge-refine-highlight-change
+			 buf beg1 m1 m2
+			 ;; Try to use props-c only for changed chars,
+			 ;; fallback to props-r for changed/removed chars,
+			 ;; but if props-r is nil then fallback to props-c.
+			 (or (and (eq op '?c) props-c) props-r props-c))))
                 (when (memq op '(?a ?c))
                   (setq last2
-                        (smerge-refine-highlight-change buf beg2 m4 m5 props))))
+                        (smerge-refine-highlight-change
+			 buf beg2 m4 m5
+			 ;; Same logic as for removed chars above.
+			 (or (and (eq op '?c) props-c) props-a props-c)))))
               (forward-line 1)                            ;Skip hunk header.
               (and (re-search-forward "^[0-9]" nil 'move) ;Skip hunk body.
                    (goto-char (match-beginning 0))))
@@ -1081,7 +1119,11 @@ (defun smerge-refine (&optional part)
                    ((eq (match-end 3) (match-beginning 3)) 3)
                    (t 2)))
   (let ((n1 (if (eq part 1) 2 1))
-        (n2 (if (eq part 3) 2 3)))
+        (n2 (if (eq part 3) 2 3))
+	(smerge-use-changed-face
+	 (and (face-differs-from-default-p 'smerge-refined-change)
+	      (not (face-equal 'smerge-refined-change 'smerge-refined-added))
+	      (not (face-equal 'smerge-refined-change 'smerge-refined-removed)))))
     (smerge-ensure-match n1)
     (smerge-ensure-match n2)
     (with-silent-modifications
@@ -1090,8 +1132,13 @@ (defun smerge-refine (&optional part)
                          (cons (buffer-chars-modified-tick) part)))
     (smerge-refine-subst (match-beginning n1) (match-end n1)
                          (match-beginning n2)  (match-end n2)
-                         '((smerge . refine)
-                           (face . smerge-refined-change)))))
+                         (if smerge-use-changed-face
+			     '((smerge . refine) (face . smerge-refined-change)))
+			 nil
+			 (unless smerge-use-changed-face
+			   '((smerge . refine) (face . smerge-refined-removed)))
+			 (unless smerge-use-changed-face
+			   '((smerge . refine) (face . smerge-refined-added))))))
 
 (defun smerge-diff (n1 n2)
   (smerge-match-conflict)

Message #44 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Wed, 23 May 2012 09:53:09 -0400

>>> +      (if (not (or (face-equal diff-changed-face diff-added-face)
>>> +		   (face-equal diff-changed-face diff-removed-face)))
>> Please introduce a defvar for it, so we don't re-evaluate the face
>> comparison for each and every line.
> This defvar needs to be re-evaluated when the user customized faces.

Which rarely happens after loading diff-mode.el, so I wouldn't worry
about it.  That also lets the user set the variable explicitly if she
wants to (after all, the faces can be different in one frame but
equal in another, so the above test is not 100% foolproof anyway).

The rest of the patch looks OK,


        Stefan

Message #47 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Fri, 25 May 2012 03:57:49 +0300

>>> Please introduce a defvar for it, so we don't re-evaluate the face
>>> comparison for each and every line.
>> This defvar needs to be re-evaluated when the user customized faces.
>
> Which rarely happens after loading diff-mode.el, so I wouldn't worry
> about it.  That also lets the user set the variable explicitly if she
> wants to (after all, the faces can be different in one frame but
> equal in another, so the above test is not 100% foolproof anyway).

So I added this condition to the defvar.

> The rest of the patch looks OK,

Installed, with face adjustments in diff-mode, smerge and ediff.

I tested all face changes in different environments: high-color X11 with
light/dark background and 8-color xterm with light/dark background.

During testing I noticed that `C-M-x' doesn't work anymore on `defface'.
It doesn't re-evaluate the face definition because `eval-sexp-add-defvars'
in `eval-defun-2' produces an expression that can't be macroexpanded.
For instance:

(setq form (eval-sexp-add-defvars (read (current-buffer))))
=>
(progn (defvar add-log-buffer-file-name-function)
       (defface diff-removed (quote ((((class color)) :background "red"))) "..."))

(macroexpand form)
=>
(progn (defvar add-log-buffer-file-name-function)
       (defface diff-removed (quote ((((class color)) :background "red"))) "..."))

The problem is that `eval-sexp-add-defvars' adds `progn' that prevents
defface macro expansion.  `macroexpand' can't expand `defface' to
`custom-declare-face' (this is expected in `eval-defun-1').

Without `progn', `macroexpand' works correctly, e.g.:

(macroexpand '(defface diff-removed (quote ((((class color)) :background "red"))) "..."))
=>
(custom-declare-face (quote diff-removed) (quote ((((class color)) :background "red"))) "...")

Message #50 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Fri, 25 May 2012 09:21:02 -0400

> During testing I noticed that `C-M-x' doesn't work anymore on `defface'.
> It doesn't re-evaluate the face definition because `eval-sexp-add-defvars'
> in `eval-defun-2' produces an expression that can't be macroexpanded.
> For instance:

> (setq form (eval-sexp-add-defvars (read (current-buffer))))
> =>
> (progn (defvar add-log-buffer-file-name-function)
>        (defface diff-removed (quote ((((class color)) :background "red"))) "..."))

> (macroexpand form)
> =>
> (progn (defvar add-log-buffer-file-name-function)
>        (defface diff-removed (quote ((((class color)) :background "red"))) "..."))

> The problem is that `eval-sexp-add-defvars' adds `progn' that prevents
> defface macro expansion.  `macroexpand' can't expand `defface' to
> `custom-declare-face' (this is expected in `eval-defun-1').

> Without `progn', `macroexpand' works correctly, e.g.:

> (macroexpand '(defface diff-removed (quote ((((class color)) :background "red"))) "..."))
> =>
> (custom-declare-face (quote diff-removed) (quote ((((class color)) :background "red"))) "...")

Good point.  We should change eval-defun-1 to look inside `progn'.
Or alternatively, we should use eval-sexp-add-defvars after the
macroexpand+specialcaseing.


        Stefan

Message #53 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Fri, 25 May 2012 23:35:58 +0300

> Good point.  We should change eval-defun-1 to look inside `progn'.
> Or alternatively, we should use eval-sexp-add-defvars after the
> macroexpand+specialcaseing.

`edebug-eval-defun' uses `eval-sexp-add-defvars' after the specialcaseing,
so `eval-defun-1' could do the same:

=== modified file 'lisp/emacs-lisp/lisp-mode.el'
--- lisp/emacs-lisp/lisp-mode.el	2012-05-24 23:24:47 +0000
+++ lisp/emacs-lisp/lisp-mode.el	2012-05-25 20:28:17 +0000
@@ -830,10 +830,10 @@ (defun eval-defun-2 ()
 	   (end-of-defun)
 	   (beginning-of-defun)
 	   (setq beg (point))
-	   (setq form (eval-sexp-add-defvars (read (current-buffer))))
+	   (setq form (read (current-buffer)))
 	   (setq end (point)))
 	 ;; Alter the form if necessary.
-	 (setq form (eval-defun-1 (macroexpand form)))
+	 (setq form (eval-sexp-add-defvars (eval-defun-1 (macroexpand form))))
 	 (list beg end standard-output
 	       `(lambda (ignore)
 		 ;; Skipping to the end of the specified region

Message #56 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Sat, 26 May 2012 09:13:31 -0400

>> Good point.  We should change eval-defun-1 to look inside `progn'.
>> Or alternatively, we should use eval-sexp-add-defvars after the
>> macroexpand+specialcaseing.

> `edebug-eval-defun' uses `eval-sexp-add-defvars' after the specialcaseing,
> so `eval-defun-1' could do the same:

The patch looks OK, please install,


        Stefan

Message #61 received at 10181-done <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 10181-done <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Sun, 27 May 2012 12:46:31 +0300

>> `edebug-eval-defun' uses `eval-sexp-add-defvars' after the specialcaseing,
>> so `eval-defun-1' could do the same:
>
> The patch looks OK, please install,

Installed and closed.

Message #68 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: 24.0.92;
	[wishlist] split `diff-refine-change' in several faces
Date: Sun, 30 Sep 2012 19:38:03 +0300

>> The patch looks OK, please install,
>
> Installed and closed.

I changed the background color of `diff-refine-removed' and
`smerge-refined-removed' from "#ffaaaa" to "#ffbbbb" because
I learned that due to wavelength differences of red and green
colors they are not symmetrical in perception as can be seen
in the small relative shift on this diagram:

http://en.wikipedia.org/wiki/File:CIE_1931_XYZ_Color_Matching_Functions.svg

It is amusing to see how this problem is correctly addressed on
bitbucket.org, but github.com fails to recognize this problem
and still uses "#ffaaaa" instead of "#ffbbbb".

To help resolving this kind of problem I also added a new option
to `list-colors-sort' to sort colors by relative luminance
in the CIE XYZ color space.

Message #75 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Dani Moncayo <dmoncayo <at> gmail.com>
To: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: split `diff-refine-change' in several faces
Date: Sat, 14 Jun 2014 13:58:00 +0200

In this bug report, I requested to replace the face
'diff-refine-change' with three different faces, related to the faces
'diff-added', 'diff-removed' and 'diff-changed'.

That was implemented time ago (thanks Juri!), but I've just realized a
little inconsistency in the name of one of the new faces.  The new
faces were:
* 'diff-refine-added', related to 'diff-added'.
* 'diff-refine-removed', related to 'diff-removed'.
* 'diff-refine-change', related to 'diff-changed'.

So, I think that 'diff-refine-change' should be named
'diff-refine-changed' instead, for consistency.

-- 
Dani Moncayo

Message #78 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Dani Moncayo <dmoncayo <at> gmail.com>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: split `diff-refine-change' in several faces
Date: Sat, 14 Jun 2014 15:54:17 +0300

> In this bug report, I requested to replace the face
> 'diff-refine-change' with three different faces, related to the faces
> 'diff-added', 'diff-removed' and 'diff-changed'.
>
> That was implemented time ago (thanks Juri!), but I've just realized a
> little inconsistency in the name of one of the new faces.  The new
> faces were:
> * 'diff-refine-added', related to 'diff-added'.
> * 'diff-refine-removed', related to 'diff-removed'.
> * 'diff-refine-change', related to 'diff-changed'.
>
> So, I think that 'diff-refine-change' should be named
> 'diff-refine-changed' instead, for consistency.

There are more faces that use the suffix `-changed', namely
`diff-indicator-changed' that confirms your request to rename
`diff-refine-change' to `diff-refine-changed'.  OTOH, there are
more faces that use the suffix `-change', namely `smerge-refined-change'
that could be renamed to `smerge-refined-changed' for consistency with
the basic face `diff-changed', and other suffixes `-added' and `-removed'.
Since they were introduced long ago, I guess this change is for the trunk:

=== modified file 'lisp/vc/diff-mode.el'
--- lisp/vc/diff-mode.el	2014-01-01 07:43:34 +0000
+++ lisp/vc/diff-mode.el	2014-06-14 12:50:02 +0000
@@ -1916,7 +1915,7 @@ (defun diff-ignore-whitespace-hunk ()
 
 ;;; Fine change highlighting.
 
-(defface diff-refine-change
+(defface diff-refine-changed
   '((((class color) (min-colors 88) (background light))
      :background "#ffff55")
     (((class color) (min-colors 88) (background dark))
@@ -1924,6 +1923,7 @@ (defface diff-refine-change
     (t :inverse-video t))
   "Face used for char-based changes shown by `diff-refine-hunk'."
   :group 'diff-mode)
+(define-obsolete-face-alias 'diff-refine-change 'diff-refine-changed "24.5")
 
 (defface diff-refine-removed
   '((default

=== modified file 'lisp/vc/smerge-mode.el'
--- lisp/vc/smerge-mode.el	2014-01-01 07:43:34 +0000
+++ lisp/vc/smerge-mode.el	2014-06-14 12:50:05 +0000
@@ -116,9 +116,10 @@ (defface smerge-markers
 (define-obsolete-face-alias 'smerge-markers-face 'smerge-markers "22.1")
 (defvar smerge-markers-face 'smerge-markers)
 
-(defface smerge-refined-change
+(defface smerge-refined-changed
   '((t nil))
   "Face used for char-based changes shown by `smerge-refine'.")
+(define-obsolete-face-alias 'smerge-refined-change 'smerge-refined-changed "24.5")
 
 (defface smerge-refined-removed
   '((default

Message #81 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Dani Moncayo <dmoncayo <at> gmail.com>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: split `diff-refine-change' in several faces
Date: Sat, 14 Jun 2014 16:12:12 +0300

> Since they were introduced long ago, I guess this change is for the trunk:

BTW, I don't remember what was the reason of defining a special
terminal version of the face `diff-changed' inconsistently with the
default min-colors 88 version, but it seems this is not necessary
anymore because in both tty and graphical versions the differences
are highlighted with distinct faces anyway.  Then its definition
will be the same '((t nil)) as already used by `smerge-refined-changed'.

Also this means that we don't need to use `shadow' on context lines anymore,
because the differing lines are always highlighted by default.

=== modified file 'lisp/vc/diff-mode.el'
--- lisp/vc/diff-mode.el	2014-01-01 07:43:34 +0000
+++ lisp/vc/diff-mode.el	2014-06-14 13:05:29 +0000
@@ -302,13 +302,7 @@ (define-obsolete-face-alias 'diff-added-
 (defvar diff-added-face 'diff-added)
 
 (defface diff-changed
-  ;; We normally apply a `shadow'-based face on the `diff-context'
-  ;; face, and keep `diff-changed' the default.
-  '((((class color grayscale) (min-colors 88)))
-    ;; If the terminal lacks sufficient colors for shadowing,
-    ;; highlight changed lines explicitly.
-    (((class color))
-     :foreground "yellow"))
+  '((t nil))
   "`diff-mode' face used to highlight changed lines."
   :group 'diff-mode)
 (define-obsolete-face-alias 'diff-changed-face 'diff-changed "22.1")
@@ -343,7 +337,7 @@ (define-obsolete-face-alias 'diff-functi
 (defvar diff-function-face 'diff-function)
 
 (defface diff-context
-  '((((class color grayscale) (min-colors 88)) :inherit shadow))
+  '((t nil))
   "`diff-mode' face used to highlight context and other side-information."
   :group 'diff-mode)
 (define-obsolete-face-alias 'diff-context-face 'diff-context "22.1")

Message #84 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Dani Moncayo <dmoncayo <at> gmail.com>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: split `diff-refine-change' in several faces
Date: Mon, 16 Jun 2014 09:55:31 +0300

> Also this means that we don't need to use `shadow' on context lines anymore,
> because the differing lines are always highlighted by default.

Actually better to use the same color for context lines as used
on most other VCSes - #333333 for light backgrounds (its symmetric
color for dark backgrounds would be #dddddd):

=== modified file 'lisp/vc/diff-mode.el'
--- lisp/vc/diff-mode.el	2014-01-01 07:43:34 +0000
+++ lisp/vc/diff-mode.el	2014-06-16 06:54:30 +0000
@@ -343,7 +337,10 @@ (define-obsolete-face-alias 'diff-functi
 (defvar diff-function-face 'diff-function)
 
 (defface diff-context
-  '((((class color grayscale) (min-colors 88)) :inherit shadow))
+  '((((class color grayscale) (min-colors 88) (background light))
+     :foreground "#333333")
+    (((class color grayscale) (min-colors 88) (background dark))
+     :foreground "#dddddd"))
   "`diff-mode' face used to highlight context and other side-information."
   :group 'diff-mode)
 (define-obsolete-face-alias 'diff-context-face 'diff-context "22.1")


BTW, Ediff keeps font-lock highlighting while displaying the current hunk
that is good, but removes font-lock highlighting on non-current differences.
It would be much nicer to keep font-lock highlighting on these lines as well.
This patch does this by removing :foreground "White" and :foreground "Black"
from face definitions for min-colors 88, and keeps their :background colors:

=== modified file 'lisp/vc/ediff-init.el'
--- lisp/vc/ediff-init.el	2014-05-03 02:27:46 +0000
+++ lisp/vc/ediff-init.el	2014-06-16 06:54:34 +0000
@@ -949,7 +949,9 @@ (and (featurep 'xemacs)
 
 (defface ediff-current-diff-Ancestor
   (if (featurep 'emacs)
-      '((((class color) (min-colors 16))
+      '((((class color) (min-colors 88))
+	 (:background "VioletRed"))
+	(((class color) (min-colors 16))
 	 (:foreground "Black" :background "VioletRed"))
 	(((class color))
 	 (:foreground "black" :background "magenta3"))
@@ -1057,7 +1059,9 @@ (ediff-hide-face ediff-fine-diff-face-C)
 
 (defface ediff-fine-diff-Ancestor
   (if (featurep 'emacs)
-      '((((class color) (min-colors 16))
+      '((((class color) (min-colors 88))
+	 (:background "Green"))
+	(((class color) (min-colors 16))
 	 (:foreground "Black" :background "Green"))
 	(((class color))
 	 (:foreground "red3" :background "green"))
@@ -1091,6 +1095,8 @@ (defface ediff-even-diff-A
   (if (featurep 'emacs)
       `((((type pc))
 	 (:foreground "green3" :background "light grey"))
+	(((class color) (min-colors 88))
+	 (:background "light grey"))
 	(((class color) (min-colors 16))
 	 (:foreground "Black" :background "light grey"))
 	(((class color))
@@ -1115,7 +1121,9 @@ (ediff-hide-face ediff-even-diff-face-A)
 
 (defface ediff-even-diff-B
   (if (featurep 'emacs)
-      `((((class color) (min-colors 16))
+      `((((class color) (min-colors 88))
+	 (:background "Grey"))
+	(((class color) (min-colors 16))
 	 (:foreground "White" :background "Grey"))
 	(((class color))
 	 (:foreground "blue3" :background "Grey" :weight bold))
@@ -1138,6 +1146,8 @@ (defface ediff-even-diff-C
   (if (featurep 'emacs)
       `((((type pc))
 	 (:foreground "yellow3" :background "light grey"))
+	(((class color) (min-colors 88))
+	 (:background "light grey"))
 	(((class color) (min-colors 16))
 	 (:foreground "Black" :background "light grey"))
 	(((class color))
@@ -1164,6 +1174,8 @@ (defface ediff-even-diff-Ancestor
   (if (featurep 'emacs)
       `((((type pc))
 	 (:foreground "cyan3" :background "light grey"))
+	(((class color) (min-colors 88))
+	 (:background "Grey"))
 	(((class color) (min-colors 16))
 	 (:foreground "White" :background "Grey"))
 	(((class color))
@@ -1197,6 +1209,8 @@ (defface ediff-odd-diff-A
   (if (featurep 'emacs)
       '((((type pc))
 	 (:foreground "green3" :background "gray40"))
+	(((class color) (min-colors 88))
+	 (:background "Grey"))
 	(((class color) (min-colors 16))
 	 (:foreground "White" :background "Grey"))
 	(((class color))
@@ -1222,6 +1236,8 @@ (defface ediff-odd-diff-B
   (if (featurep 'emacs)
       '((((type pc))
 	 (:foreground "White" :background "gray40"))
+	(((class color) (min-colors 88))
+	 (:background "light grey"))
 	(((class color) (min-colors 16))
 	 (:foreground "Black" :background "light grey"))
 	(((class color))
@@ -1246,6 +1262,8 @@ (defface ediff-odd-diff-C
   (if (featurep 'emacs)
       '((((type pc))
 	 (:foreground "yellow3" :background "gray40"))
+	(((class color) (min-colors 88))
+	 (:background "Grey"))
 	(((class color) (min-colors 16))
 	 (:foreground "White" :background "Grey"))
 	(((class color))
@@ -1268,7 +1286,9 @@ (ediff-hide-face ediff-odd-diff-face-C)
 
 (defface ediff-odd-diff-Ancestor
   (if (featurep 'emacs)
-      '((((class color) (min-colors 16))
+      '((((class color) (min-colors 88))
+	 (:background "gray40"))
+	(((class color) (min-colors 16))
 	 (:foreground "cyan3" :background "gray40"))
 	(((class color))
 	 (:foreground "green3" :background "black" :weight bold))

Message #87 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Dani Moncayo <dmoncayo <at> gmail.com>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: split `diff-refine-change' in several faces
Date: Wed, 18 Jun 2014 11:57:14 +0300

There is a related problem: currently vc-annotate uses dark background
even when the default background is light.  Would be nicer to have
another option similar to the existing `vc-annotate-background', that
will define the default foreground (instead of the default background) and
to put colors from color-map on the background instead of the foreground:

=== modified file 'lisp/vc/vc-annotate.el'
--- lisp/vc/vc-annotate.el	2014-02-10 01:34:22 +0000
+++ lisp/vc/vc-annotate.el	2014-06-18 08:55:39 +0000
@@ -109,6 +109,14 @@ (defcustom vc-annotate-background "black
   :type '(choice (const :tag "Default background" nil) (color))
   :group 'vc)
 
+(defcustom vc-annotate-foreground "black"
+  "Foreground color for \\[vc-annotate].
+Default color is used if nil.
+It it applied only when `vc-annotate-background' is nil."
+  :type '(choice (const :tag "Default foreground" nil) (color))
+  :version "24.4"
+  :group 'vc)
+
 (defcustom vc-annotate-menu-elements '(2 0.5 0.1 0.01)
   "Menu elements for the mode-specific menu of VC-Annotate mode.
 List of factors, used to expand/compress the time scale.  See `vc-annotate'."
@@ -347,7 +355,9 @@ (defun vc-annotate (file rev &optional d
 `vc-annotate-menu-elements' customizes the menu elements of the
 mode-specific menu.  `vc-annotate-color-map' and
 `vc-annotate-very-old-color' define the mapping of time to colors.
-`vc-annotate-background' specifies the background color."
+`vc-annotate-background' specifies the background color.
+`vc-annotate-foreground' specifies the foreground color
+when `vc-annotate-background' is nil."
   (interactive
    (save-current-buffer
      (vc-ensure-vc-buffer)
@@ -666,10 +676,15 @@ (defun vc-annotate-lines (limit)
                ;; Make the face if not done.
                (face (or (intern-soft face-name)
                          (let ((tmp-face (make-face (intern face-name))))
-                           (set-face-foreground tmp-face (cdr color))
-                           (when vc-annotate-background
-			     (set-face-background tmp-face
-						  vc-annotate-background))
+                           (cond
+                            (vc-annotate-background
+                             (set-face-foreground tmp-face (cdr color))
+                             (set-face-background tmp-face vc-annotate-background))
+                            (vc-annotate-foreground
+                             (set-face-foreground tmp-face vc-annotate-foreground)
+                             (set-face-background tmp-face (cdr color)))
+                            (t
+                             (set-face-foreground tmp-face (cdr color))))
                            tmp-face))))	; Return the face
           (put-text-property start end 'face face)))))
   ;; Pretend to font-lock there were no matches.

Message #90 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Dani Moncayo <dmoncayo <at> gmail.com>
To: Juri Linkov <juri <at> jurta.org>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: split `diff-refine-change' in several faces
Date: Wed, 18 Jun 2014 11:14:38 +0200

Hi Juri,

I just wanted to say that I don't use vc-annotate (yet).   So I cannot
comment on the patches you are proposing.

Also, I see that you've already committed the change I requested
(rename 'diff-refine-change' to 'diff-refine-changed').  So thank you
for that.

-- 
Dani Moncayo

Message #93 received at 10181 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Dani Moncayo <dmoncayo <at> gmail.com>
Cc: 10181 <at> debbugs.gnu.org
Subject: Re: bug#10181: split `diff-refine-change' in several faces
Date: Thu, 19 Jun 2014 09:54:51 +0300

> I just wanted to say that I don't use vc-annotate (yet).   So I cannot
> comment on the patches you are proposing.
>
> Also, I see that you've already committed the change I requested
> (rename 'diff-refine-change' to 'diff-refine-changed').  So thank you
> for that.

For new features I created separate requests.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.