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

On 07.12.2019 9:53, Eli Zaretskii wrote:

>>> Well, we will have to document this exemption prominently, then.
>>
>> I suppose so. Though I wouldn't consider it too important to most users,
> 
> It's important to developers of Lisp programs that manipulate faces.

Yes, sure.

>> 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.
> 
> I meant to avoid such consequences by making sure those other callers
> can never trigger this new processing of :extend.

Eli, what you're asking for would be actively harmful.

To make an analogy, when we're changing a core Emacs behavior, we 
shouldn't try to make it work only on Tuesdays. Even if the original bug 
reporter observed the problem on a Tuesday.

Can we please focus on the more pressing question: whether the proposed 
patch actually works, and does that reliably, or are there 
scenarios/configurations where it would do something unexpected.

>> 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).
> 
> The use case I had in mind is this:
> 
>    . the theme doesn't specify :extend
>    . the default spec for a face specifies ':extend nil'
> 
> In this case, after applying the theme, the face should have
> ':extend nil', implicitly "inherited" from the default spec.  Do you
> agree?

Ok, I think I understand the distinction: it's for when a character has 
several faces specified for it.

Sure. It's easy to fix anyway.

>> Finally, the value 'unspecified' seems impossible to get from the
>> attributes list this way. plist-get will simply return nil.
> 
> Exactly.  And when a face does specify a nil value for :extend, then
> plist will return the list '(:extend nil), which is non-nil.

plist-member, you mean.

>> 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.
> 
> Once again, I think this part:
> 
>> +    (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)))))
>                                                       ^^^^^^^^^^^^
> isn't right, because it seems to say that when the default face says
> ':extend nil', the face after applying a theme will have ':extend t',
> which isn't TRT, IMO.

Updated patch attached.

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

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