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: 1839 bytes --]

On 05.12.2019 17:47, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Thu, 5 Dec 2019 03:44:22 +0200
>>
>> The new definition for diff-added would look like:
>>
>>     (defface diff-added
>>       '((default
>>          :inherit diff-changed)
>>         (((class color) (min-colors 257) (background light))
>>          :background "#eeffee")
>>         (((class color) (min-colors 88) (background light))
>>          :background "#ddffdd")
>>         (((class color) (min-colors 88) (background dark))
>>          :background "#335533")
>>         (((class color))
>>          :foreground "green"))
>>       "`diff-mode' face used to highlight added lines.")
>>
>>     (put 'diff-added 'face-transient-spec '((t :extend t)))
> 
> OK, and how will this work to countermand the problem with themes?
> 
> custom-theme-set-faces calls face-spec-set, which calls
> face-spec-recalc, which starts by resetting all face attributes to
> 'unspecified'.  And the last 2 functions are general-purpose, not
> specific to themes.

Well, the idea was to use a different structure to store the "transient" 
attributes. That could be an extra symbol property, or an additional 
structure for storing faces attributes, in addition to 
default-frame-alist. But looking at the code now, it seems fairly clunky 
and crossing abstraction levels.

It's great that you mentioned face-spec-recalc. It looks just like the 
place to change, since both defface and theme definitions and 
customizations go through it.

We can implement in there a new kind of "face spec" along the lines of 
my previous description, or simply special-case the :extend attribute, 
and take it from the default spec. The latter option is implemented in 
the attached patch, which seems to work in my limited testing.

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

diff --git a/lisp/faces.el b/lisp/faces.el
index dc5bcca760..b3659605a2 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1669,21 +1669,29 @@ face-spec-recalc
   ;; `theme-face' records.
   (let ((theme-faces (get face 'theme-face))
 	(no-match-found 0)
-	face-attrs theme-face-applied)
+        face-attrs theme-face-applied
+        default-attrs themed-extend-attr)
     (if theme-faces
 	(dolist (elt (reverse theme-faces))
 	  (setq face-attrs (face-spec-choose (cadr elt) frame no-match-found))
 	  (unless (eq face-attrs no-match-found)
+            (unless themed-extend-attr
+              (setq themed-extend-attr (plist-member face-attrs :extend)))
 	    (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 (not themed-extend-attr))
+      (let ((extend-p (plist-get default-attrs :extend)))
+        (and extend-p (face-spec-set-2 face frame '(:extend t)))))
     (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
     (face-spec-set-2 face frame face-attrs)))