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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 8 Dec 2019 12:39:06 +0200
> 
> > If we don't have a safe solution, we will have to live with that risk,
> > unfortunately.
> 
> We haven't even started the pretest yet. If there are bugs in this patch 
> (unlikely, but always possible), we have time for people to see and 
> report them.

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.

> >>> But I don't
> >>> consider myself an expert on these matters, so if you say we cannot
> >>> differentiate between general face definition and what themes do, so
> >>> be it.)
> >>
> >> What's a "general face definition"?
> > 
> > Everything except theme definition of faces.
> 
> Please give an example.

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).  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).

Thanks.

diff --git a/test/lisp/faces-tests.el b/test/lisp/faces-tests.el
index f00c93cedc..7cba4b26eb 100644
--- a/test/lisp/faces-tests.el
+++ b/test/lisp/faces-tests.el
@@ -36,6 +36,26 @@
   ""
   :group 'faces--test)
 
+(defface faces--test-extend
+  '((t :extend t :background "blue"))
+  ""
+  :group 'faces--test)
+
+(defface faces--test-no-extend
+  '((t :extend nil :background "blue"))
+  ""
+  :group 'faces--test)
+
+(defface faces--test-inherit-extend
+  '((t :inherit (faces--test-extend faces--test2) :background "blue"))
+  ""
+  :group 'faces--test)
+
+(defface faces--test-inherit-no-extend
+  '((t :inherit (faces--test2 faces--test-no-extend) :background "blue"))
+  ""
+  :group 'faces--test)
+
 (ert-deftest faces--test-color-at-point ()
   (with-temp-buffer
     (insert (propertize "STRING" 'face '(faces--test2 faces--test1)))
@@ -69,5 +89,133 @@
   ;; face IDs to faces.
   (should (> (face-id 'faces--test1) (face-id 'tooltip))))
 
+(ert-deftest faces--test-extend ()
+  (should (equal (face-attribute 'faces--test-extend :extend) t))
+  (should (equal (face-attribute 'faces--test-no-extend :extend) nil))
+  (should (equal (face-attribute 'faces--test1 :extend) 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  )
+
+(ert-deftest faces--test-extend-with-themes ()
+  ;; Make sure the diff-mode faces are not defined.
+  (should-not (featurep 'diff-mode))
+  (defface diff-changed-face
+    '((t :extend t :weight bold))
+    "")
+  (defface diff-added
+    '((t :background "grey"))
+    "")
+  (defface diff-file-header-face
+    '((t :extend nil :foreground "cyan"))
+    "")
+  (should (equal (face-attribute 'diff-changed-face :extend) t))
+  (should (equal (face-attribute 'diff-added :extend) 'unspecified))
+  (should (equal (face-attribute 'diff-file-header-face :extend) nil))
+  (load-theme 'manoj-dark t t)
+  (load-theme 'tsdh-light t t)
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  (should (equal (face-attribute 'diff-changed-face :extend) t))
+  (should (equal (face-attribute 'diff-added :extend) 'unspecified))
+  (should (equal (face-attribute 'diff-file-header-face :extend) nil))
+  (enable-theme 'manoj-dark)
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  (should (equal (face-attribute 'diff-changed-face :extend) 'unspecified)) ; 
should be t
+  (should (equal (face-attribute 'diff-added :extend) t))
+  (should (equal (face-attribute 'diff-file-header-face :extend) 
'unspecified)) ; should be nil
+  (defface faces--test-face3
+    '((t :inherit diff-added :weight bold))
+    "")
+  (should (equal (face-attribute 'faces--test-face3 :extend nil t) t))
+  (disable-theme 'manoj-dark)
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  (should (equal (face-attribute 'diff-changed-face :extend) t))
+  (should (equal (face-attribute 'diff-added :extend) 'unspecified))
+  (should (equal (face-attribute 'diff-file-header-face :extend) nil))
+  (should (equal (face-attribute 'faces--test-face3 :extend nil t) 
'unspecified))
+  (defface diff-indicator-changed
+    '((t (:weight bold :extend t)))
+    "")
+  (enable-theme 'tsdh-light)
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  (should (equal (face-attribute 'diff-changed-face :extend) t))
+  (should (equal (face-attribute 'diff-added :extend) t))
+  (should (equal (face-attribute 'diff-file-header-face :extend) nil))
+  (should (equal (face-attribute 'diff-indicator-changed :extend) 
'unspecified)) ; should be t
+  (should (equal (face-attribute 'faces--test-face3 :extend nil t) t))
+  (frame-set-background-mode (selected-frame) 'dark)
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  (should (equal (face-attribute 'diff-changed-face :extend) t))
+  (should (equal (face-attribute 'diff-added :extend) t))
+  (should (equal (face-attribute 'diff-file-header-face :extend) nil))
+  (should (equal (face-attribute 'diff-indicator-changed :extend) 
'unspecified)) ; should be t
+  (should (equal (face-attribute 'faces--test-face3 :extend nil t) t))
+  (or noninteractive
+      (let ((fr (make-frame)))
+        (should (equal (face-attribute 'faces--test-inherit-extend :extend fr)
+                       'unspecified))
+        (should (equal (face-attribute 'faces--test-inherit-extend :extend fr 
t)
+                       t))
+        (should (equal (face-attribute 'faces--test-inherit-no-extend
+                                       :extend fr)
+                       'unspecified))
+        (should (equal (face-attribute 'faces--test-inherit-no-extend
+                                       :extend fr t)
+                       nil))
+        (should (equal (face-attribute 'diff-changed-face :extend fr) t))
+        (should (equal (face-attribute 'diff-added :extend fr) t))
+        (should (equal (face-attribute 'diff-file-header-face :extend fr) nil))
+        (should (equal (face-attribute 'diff-indicator-changed :extend fr)
+                       'unspecified)) ; should be t
+        (should (equal (face-attribute 'faces--test-face3 :extend nil t) t))
+        ))
+  (disable-theme 'tsdh-light)
+  (should (equal (face-attribute 'diff-indicator-changed :extend) t))
+  (should (equal (face-attribute 'faces--test-face3 :extend nil t) 
'unspecified))
+  (or noninteractive
+      (let ((fr (make-frame)))
+        (should (equal (face-attribute 'diff-changed-face :extend fr) t))
+        (should (equal (face-attribute 'diff-added :extend fr) 'unspecified))
+        (should (equal (face-attribute 'diff-file-header-face :extend fr) nil))
+        (should (equal (face-attribute 'diff-indicator-changed :extend fr) t))
+        (should (equal (face-attribute 'faces--test-face3 :extend nil t) 
'unspecified))
+        ))
+  )
+
 (provide 'faces-tests)
 ;;; faces-tests.el ends here