bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages

[Dmitry Gutov <dgutov@yandex.ru>]

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

On 08.12.2019 17:50, Eli Zaretskii wrote:

> Experience teaches up that quite a few problems, especially in subtle
> areas, are discovered only after the release.  I guess it means that
> the group of people who use the pretest is not representative enough.

Sure. But on the other hand, the number of users in the period before 
the pretest in even smaller.

> OK, I've now reviewed all the callers of face-spec-recalc, and all of
> its callers' callers, and wrote a bunch of tests to make sure that we
> don't break anything (or at least anything important).

Thank you. That's pretty comprehensive. I would suggest to install those 
tests, but I wonder how they would interact with a long-running test 
session.

Running them in an interactive session was tricky as well because 
visiting any file, even in 'emacs -Q', automatically leads to 
diff-mode.el being loaded, and so (should-not (featurep 'diff-mode)) 
fails right away.

They also rely on the existing themes, the definitions of which will change.

> The tests in
> the patch below all pass for the current code on master, and include a
> couple of comments where the changes to implicitly inherit :extend by
> themes are supposed to change the expected result.  If after applying
> your patch all the tests still pass, both in -batch and in an
> interactive session, then I think we are good to go (after adding the
> necessary documentation and NEWS entry).

They do! If by "still pass" you mean the version of these tests where 
the expected values are replaced with the values from "should be" comments.

All right, how does the attached patch look?

In addition to it, I'd like to revert the part of 64687872f6 that 
changed the bundled themed (etc/themes/*). Is that okay?

[-- Attachment #2: inherit-face-extend-spec-4.diff --]
[-- Type: text/x-patch, Size: 4055 bytes --]

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index fa81b2e953..df6f07496e 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -2499,9 +2499,9 @@ Face Attributes
 @code{nil} to not use this face for the space between the end of the
 line and the edge of the window.  When Emacs merges several faces for
 displaying the empty space beyond end of line, only those faces with
-@code{:extend} non-@code{nil} will be merged.  By default, only
-@code{region} and @code{hl-line} faces have this attribute set to
-@code{t}.
+@code{:extend} non-@code{nil} will be merged.  This attribute is
+different from the others in that when a theme doesn't define it for a
+face, the value from the default spec is inherited.
 
 @end table
 
diff --git a/etc/NEWS b/etc/NEWS
index 28bcb720cd..3d0781c24c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -437,15 +437,16 @@ to 'completion-styles' or 'completion-category-overrides' to use it.
 The new face attribute ':extend' controls whether to use the face for
 displaying the empty space beyond end of line (EOL) till the edge of
 the window.  By default, this attribute is non-nil only for 'region',
-'secondary-selection', 'hl-line' and some faces of Diff and Ediff
-modes; any other face that crosses end of line will not affect the
-display of the empty space at EOL.  This is to make Emacs behave more
-like other GUI applications with respect to displaying faces that
-cross line boundaries.
-
-Themes that redefine faces should add a non-nil ':extend' attribute to
-the above-mentioned faces, to keep the behavior of the default face
-definitions.
+'secondary-selection', 'hl-line' and some faces of Diff, Ediff,
+LogView and SMerge modes; any other face that crosses end of line will
+not affect the display of the empty space at EOL.  This is to make
+Emacs behave more like other GUI applications with respect to
+displaying faces that cross line boundaries.
+
+This attribute behaves specially when theme definitions are applied:
+if the theme doesn't specify its value for a face, the value from the
+default spec is used.  Consequently, themes usually shouldn't touch
+this attribute at all.
 
 ** Connection-local variables
 
diff --git a/lisp/faces.el b/lisp/faces.el
index dc5bcca760..0f31628f5f 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1669,7 +1669,7 @@ face-spec-recalc
   ;; `theme-face' records.
   (let ((theme-faces (get face 'theme-face))
 	(no-match-found 0)
-	face-attrs theme-face-applied)
+        default-attrs face-attrs theme-face-applied)
     (if theme-faces
 	(dolist (elt (reverse theme-faces))
 	  (setq face-attrs (face-spec-choose (cadr elt) frame no-match-found))
@@ -1677,13 +1677,20 @@ face-spec-recalc
 	    (face-spec-set-2 face frame face-attrs)
 	    (setq theme-face-applied t))))
     ;; If there was a spec applicable to FRAME, that overrides the
-    ;; defface spec entirely (rather than inheriting from it).  If
-    ;; there was no spec applicable to FRAME, apply the defface spec
-    ;; as well as any applicable X resources.
+    ;; defface spec entirely rather than inheriting from it, with the
+    ;; exception of the :extend attribute (which is inherited).
+    ;;
+    ;; If there was no spec applicable to FRAME, apply the defface
+    ;; spec as well as any applicable X resources.
+    (setq default-attrs (face-spec-choose (face-default-spec face) frame))
     (unless theme-face-applied
-      (setq face-attrs (face-spec-choose (face-default-spec face) frame))
-      (face-spec-set-2 face frame face-attrs)
+      (face-spec-set-2 face frame default-attrs)
       (make-face-x-resource-internal face frame))
+    (when (and theme-face-applied
+               (eq 'unspecified (face-attribute face :extend frame t)))
+      (let ((tail (plist-member default-attrs :extend)))
+        (and tail (face-spec-set-2 face frame
+                                   (list :extend (cadr tail))))))
     (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
     (face-spec-set-2 face frame face-attrs)))