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

[Dmitry Gutov]

On 07.12.2019 19:53, Eli Zaretskii wrote:
> > Cc: 37774@debbugs.gnu.org, juri@linkov.net
> > From: Dmitry Gutov <dgutov@yandex.ru>
> > Date: Sat, 7 Dec 2019 19:06:08 +0200
> >
> > > 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.
>
> Sorry, I don't see the analogy.
>
> We are not trying to change the core behavior, we are trying to change
> how themes define faces, in a way that makes the :extend attribute be
> implicitly inherited from the default spec of the face to the face
> after theme's customizations.

We're really trying to change how a face's attributes are calculated 
based on its default spec, as well as enabled themes. There are 
different callers to face-spec-recalc because different user features 
require that re-calculation.

> All I want is to make sure no other
> caller of face-spec-recalc, one that has nothing to do with themes
> defining faces, picks up the change, because that would be unintended.
> If you consider this incorrect or unjustified, please explain why.

Here's some examples:

enable-theme needs that recalculation because a different set of themes 
is now in effect, and face attributes need to be updated.

face-set-after-frame-default needs that because a frame's parameters can 
affect how faces look as well.

frame-set-background-mode needs that to update how face specs are 
interpreted given the changed background mode.

All of these affect how a face spec is evaluated, which affects how 
theme specs and user specs apply to the face. Which should be able to 
change which spec the value of :extend is taken from.

Or, to look at it from another direction: if we create a special 
different version of face-spec-recalc purely for custom-theme-set-faces, 
and face-set-after-frame-default wouldn't use it, whatever changed logic 
we implement wouldn't apply to new frames.

> > 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.
>
> We were considering only one scenario: that of a theme defining its
> own face specs.

Right. But this scenario has different configurations. Like a themed 
spec can inherit from some other face (and the first face's default has 
':extend t' as well). I was wondering what's going to happen if the user 
customizes that other face to have ':extend t' or ':extend nil'. But my 
testing shows it behaves as expected.

> face-spec-recalc is called in other scenarios, but I
> don't think we should consider them, because we don't want to change
> the behavior in any of those other scenarios.

I'm pretty sure they'll be fine. Or if not, it'll likely be a bug 
somewhere else.

> > +    (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))))))
>
> This is OK, but why say
>
>      (list :extend (cadr tail))
>
> instead of just
>
>      tail
>
> ?  Unless I'm missing something here, the value of 'tail' here should
> be (:extend VAL), where VAL is either t or nil.  Right?

I'm not sure :extend is always the last pair in the list.

ELISP> (plist-member '(:a 1 :b 2 :c 3) :b)
(:b 2 :c 3)

I could use map-elt, though. If it's allowed in faces.el.