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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 2 Dec 2019 02:05:10 +0200
> 
>     feel free to change its :extend attribute.
> 
> OK, done. But it feels pretty useless, considering any theme where this would 
> affect something would have to repeat the 'extend t' definition anyway.

That's true for themes, but what about face customization by users
using the likes of set-face-background?  For them, we should have the
attribute even when the default face definition is empty.

>         Seems like it's a consequence of the implementation strategy. There 
> were
>         a couple of others that had been proposed (splitting the attribute in
>         two, with different default values) or using a symbol property.
>
>     Splitting into two attributes won't help here (quite the contrary,
>     since we'd have to deal with 2 new attributes).
>
> That depends on the end goal. If we were aiming to keep the previous behavior 
> almost entirely, but avoid extending things like underline over newlines, 
> there are two default values for the two proposed attributes that would 
> satisfy almost everybody. And the people who would prefer not to extend the 
> region face background over newlines would apply that in their init scripts 
> (ditto for other faces). 

The context of this part was how themes customize faces, it was not
about users customizing the faces directly.  In the context of themes,
having two attributes would not have helped, because themes will have
to be changed anyway.

As for the other aspect of this: the idea was that almost all faces do
not need to be extended, something that no default will succeed to do
silently.

>     Basically, on the C level each Lisp face is represented by an array of
>     its attributes, where each array element holds the value of the
>     corresponding attribute.  Once this array is computed, we can (and do)
>     manipulate just the array, and for all practical purposes can forget
>     about the face's symbol.  Using a symbol property would then need to
>     keep the symbol around at all times, which is inconvenient and would
>     make the code ugly.
>
> I don't think so. Once the symbol is gone, whatever left is just the value. 
> So when this array is computed, the function doing that would merge the faces 
> attributes with whatever attributes are specified using the alternative 
> symbol property. 

The array is computed only once, whereas merging happens many times
and in different places.  So we cannot compute the value of an
attribute only once, because its value depends on what other faces are
being merged, on whether their :extend attribute is set. and on
whether the particular merging process cares about :extend.

> To be more exact, the current face attributes are also assigned to a symbol 
> property (face-defface-spec). We can add another property: face-default-spec, 
> which would contain attributes that should apply to the face unless 
> explicitly overridden in face-defface-spec. It could even be set by a new 
> defface keyword instead of plain 'put', but that's a minor concern IMO. 
> (Option two ends here).
> 
> This seems to be the easiest way to go around the long-established behavior 
> that custom-theme-set-faces overwrites the face attributes (instead of trying 
> to merge them). We could do in the other way, but it would require changes in 
> both custom-theme-set-faces and defface, as well as some other functions I 
> imagine, and either a whitelist of attributes that would always be retained 
> unless overridden. Or a wholesale change to retain all attributes by default. 
> I might like the last option personally, but it would be a major breaking 
> change. Still, all themes could be updated to account for it while keeping 
> compatibility with older Emacs with little trouble (which is harder to do 
> with the current :extend situation).

I don't think I understand your proposal in concrete terms, and you
didn't read the code to check your proposal against the actual
implementation, so this is a sure way to misunderstandings and talking
past each other.  I can only say that face-defface-spec and other
properties of face symbols are used by custom.el on the Lisp level,
whereas we were talking about what happens on the C level.  On the C
level, face merging creates an unnamed face with its attributes
computed by the merging process, and that process currently takes the
:extend attribute into account.  Any proposal to use face symbol's
properties instead will have to explain how those properties are
communicated to the merging process.

>     But even if we'd overcome this annoyance, how do you specify this
>     property for a face like below?
> 
>         '(:inherit foo :background "green" :underline "red")
> 
>     There's no symbol to put the property on.  Do we say that such
>     anonymous faces cannot support this attribute?  Unclean.
> 
> See above. Just add ':extend t' (or nil) to the end of the value.

So you propose to have both symbol property _and_ a face attribute?