Subject: bug#10181: 24.0.92; [wishlist] split `diff-refine-change' in several faces

From: Stefan Monnier <[email-masked]>
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