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

On 06.12.2019 19:31, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Fri, 6 Dec 2019 18:58:26 +0200
>>
>>> Thanks, but is it clean enough to do such exemption for :extend?
>>
>> I think so. Most importantly, it will save a lot of people the trouble
>> of adapting to the current change, limiting the necessary efforts to
>> just package authors (and Emacs core, of course).
> 
> Well, we will have to document this exemption prominently, then.

I suppose so. Though I wouldn't consider it too important to most users, 
considering that thanks to this change in semantics, most people won't 
have to do a thing when upgrading to the new version of Emacs.

>>> And if we want to do this, why do it in face-spec-recalc and not in
>>> custom-set-faces itself?
>>
>> Not the place to do that. custom-theme-set-faces saves the specs defined
>> by the theme (or user customization) to the face's symbol property, and
>> then leaves it to face-spec-recalc to combine and apply all the specs
>> related to the face.
> 
> Is the patch likely to change any behavior except that of
> custom-theme-set-faces? 

Only the behavior of other its callers (direct and indirect). :-)

Such as enable-theme, face-set-after-frame-default, 
frame-set-background-mode and face-spec-set. I'm pretty sure all of 
these should behave consistently WRT this change.

>> I think the purpose of face-spec-recalc is clear enough. So I'd like to
>> see at least one of those "unintended consequences".
> 
> Let's try to avoid such consequences from the get-go, okay?

I'm "avoiding such consequences" by trying to make sure the change is in 
the correct function and that it makes sense within the context.

>>>> +    (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)))))
>>>                                                        ^^^^^^^^^^^^
>>> I think this should be extend-p instead, because the face's default
>>> spec could legitimately say ":extend nil".  Right?
>>
>> But that's the default value of that attribute.
> 
> No, the default is 'unspecified', which is different from nil, when
> merging with a face that specifies :extend, and when inheriting.  A
> theme that says ':extend nil' should override the default spec, unlike
> 'unspecified'.

This distinction is handled in the second hunk of the patch where 
themed-extend-attr is calculated. If this attribute is not themed, there 
is no difference between nil and 'unspecified' (in the default spec).

Finally, the value 'unspecified' seems impossible to get from the 
attributes list this way. plist-get will simply return nil.

That said, I think I've found one valid scenario where this patch will 
do wrong: if the themed spec includes an :inherit directive, and the 
inherited face specifies (:extend nil). The current patch would 
inevitably ignore it and override with the value from the default spec.

So here's a second try (attached).

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

diff --git a/lisp/faces.el b/lisp/faces.el
index dc5bcca760..64cb76b4fa 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,19 @@ 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 ((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)))