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

[Dmitry Gutov <dgutov@yandex.ru>]

On 06.12.2019 18:18, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Fri, 6 Dec 2019 17:44:33 +0200
>>
>> 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.
> 
> 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).

Further, as you noted in https://debbugs.gnu.org/37774#404, :extend is 
somewhat special, in that we really want its values to be consistent, so 
the results look uniform. It's not the only attribute like that, but for 
others we have different way to ensure that, such as having a default 
face (where font face, height, etc, are usually customized, instead of 
doing that in all individual faces).

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

> The latter will not risk producing
> unintended consequences for callers of face-spec-recalc other than
> custom-set-faces.

I think the purpose of face-spec-recalc is clear enough. So I'd like to 
see at least one of those "unintended consequences".

>> -    ;; 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).
> 
> You slightly modified the syntax of the comment, probably a typo.

I inserted an empty line for clarity (IMHO), but I certainly do not 
insist on it. There would be other documentation changes required, too.

>> +    (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. If faces didn't specify 
it either, there's nothing to change, right?