bug#26969: 26.0.50; diff-changed invisible by default

bug#26969: 26.0.50; diff-changed invisible by default

[Stefan Monnier]

Package: Emacs
Version: 26.0.50

Nowadays the `diff-changed` face is defined to be "invisible" (i.e. the
text is displayed identically with or without that face).
IOW it can only be used as a parent of diff-added or diff-removed (or
some other such face).

This is a bad idea since in cases such as vdiff.el where some changes
are neither additions nor removals, it would make sense to highlight the
buffer as just "changed" and using that face is a natural choice.

I suggest the patch below for that.  It is designed such that it should
make no visible difference to users of diff-added and diff-removed since
these faces override the parts that are set by my patch in diff-changed.

Another option would be to introduce a new face (not sure how to call
it, maybe diff-unspecified-change, maybe?) which inherits from
diff-changed as well.


        Stefan


diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index aa8d77882e..1aa6c05662 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -290,9 +290,14 @@ diff-added
   :group 'diff-mode)
 
 (defface diff-changed
-  '((t nil))
+  '((((class color) (min-colors 88) (background light))
+     :background "grey90")
+    (((class color) (min-colors 88) (background dark))
+     :background "grey20")
+    (((class color))
+     :foreground "grey"))
   "`diff-mode' face used to highlight changed lines."
-  :version "25.1"
+  :version "26.1"
   :group 'diff-mode)
 
 (defface diff-indicator-removed

bug#26969: 26.0.50; diff-changed invisible by default

[Juri Linkov]

> Nowadays the `diff-changed` face is defined to be "invisible" (i.e. the
> text is displayed identically with or without that face).
> IOW it can only be used as a parent of diff-added or diff-removed (or
> some other such face).
>
> This is a bad idea since in cases such as vdiff.el where some changes
> are neither additions nor removals, it would make sense to highlight the
> buffer as just "changed" and using that face is a natural choice.
>
> I suggest the patch below for that.  It is designed such that it should
> make no visible difference to users of diff-added and diff-removed since
> these faces override the parts that are set by my patch in diff-changed.

This might break a fragile condition in ‘diff-use-changed-face’
that was created to support backward-compatibility of previously
customized faces.  But maybe it's time to simplify the logic
of diff face definitions.

> Another option would be to introduce a new face (not sure how to call
> it, maybe diff-unspecified-change, maybe?) which inherits from
> diff-changed as well.

Then authors of external packages have to wait for the next Emacs release
to start using it.

>  (defface diff-changed
> -  '((t nil))
> +  '((((class color) (min-colors 88) (background light))
> +     :background "grey90")
> +    (((class color) (min-colors 88) (background dark))
> +     :background "grey20")
> +    (((class color))
> +     :foreground "grey"))

I wonder why grey?  It used to be yellow.

bug#26969: 26.0.50; diff-changed invisible by default

[Stefan Monnier]

>> (defface diff-changed
>> -  '((t nil))
>> +  '((((class color) (min-colors 88) (background light))
>> +     :background "grey90")
>> +    (((class color) (min-colors 88) (background dark))
>> +     :background "grey20")
>> +    (((class color))
>> +     :foreground "grey"))

> I wonder why grey?  It used to be yellow.

Grey is beautiful ;-)


        Stefan "who doesn't really care about colors, so long as they
                lie on the white->black axis"

bug#26969: 26.0.50; diff-changed invisible by default

[Lars Ingebrigtsen]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Nowadays the `diff-changed` face is defined to be "invisible" (i.e. the
> text is displayed identically with or without that face).
> IOW it can only be used as a parent of diff-added or diff-removed (or
> some other such face).
>
> This is a bad idea since in cases such as vdiff.el where some changes
> are neither additions nor removals, it would make sense to highlight the
> buffer as just "changed" and using that face is a natural choice.
>
> I suggest the patch below for that.  It is designed such that it should
> make no visible difference to users of diff-added and diff-removed since
> these faces override the parts that are set by my patch in diff-changed.

That does sound a bit fragile...

> Another option would be to introduce a new face (not sure how to call
> it, maybe diff-unspecified-change, maybe?) which inherits from
> diff-changed as well.

I think that sounds more straightforward?

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

bug#26969: 26.0.50; diff-changed invisible by default

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> Another option would be to introduce a new face (not sure how to call
>> it, maybe diff-unspecified-change, maybe?) which inherits from
>> diff-changed as well.
>
> I think that sounds more straightforward?

So I've now done this in Emacs 28.

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