Subject: Re: bug#33567: Syntactic fontification of diff hunks

From: Dmitry Gutov <[email-masked]>
Date: Thu, 20 Dec 2018 00:50:15 +0200

On 19.12.2018 23:49, Juri Linkov wrote:
>> Anyway, to be clear, and other considerations aside, this works:
>> [...]
>>   (defface diff-added
>> @@ -315,9 +313,7 @@ diff-added
>>       (((class color) (min-colors 88) (background light))
>>        :background "#ddffdd")
>>       (((class color) (min-colors 88) (background dark))
>> -     :background "#335533")
>> -    (((class color))
>> -     :foreground "green"))
>> +     :background "#335533"))
>>     "`diff-mode' face used to highlight added lines.")
>> [...]
>>   (defface diff-indicator-added
>>     '((default :inherit diff-added)
>>       (((class color) (min-colors 88))
>> -     :foreground "#22aa22"))
>> +     :foreground "#22aa22")
>> +    (((class color))
>> +     :foreground "green"))
>>     "`diff-mode' face used to highlight indicator of added lines (+, >)."
>>     :version "22.1")
>>   (defvar diff-indicator-added-face 'diff-indicator-added)
> 
> This looks good.

Should I install it? Nobody has commented on my earlier stated concerns, 
but maybe we should just push it and see how it plays out.

> For the same reason we have the face font-lock-comment-delimiter-face
> separate from font-lock-comment-face to use colors only on the former,
> but not on the latter on tty with 8 colors to make easier to read comments.

Yeah, it's totally fine to use separate faces. And I was happy to see 
diff-indicator-* were already defined and in use.

My concerns were different, though:

1. Is it okay to use the black foreground inside diff hunks even when 
diff-font-lock-syntax is nil? It's an incompatible change.

2. Even if we change the default in diff-added and diff-removed, some 
themes might have their foregrounds customized, so those users won't 
notice the change. It will trickle down to the themes eventually, I 
think, but it's unclear how the theme authors will choose to deal with 
this change while keeping compatibility with previous Emacs releases.