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

[Dmitry Gutov <dgutov@yandex.ru>]

On 02.12.2019 18:21, Eli Zaretskii wrote:
>> 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.

Fair enough.

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

Whose idea was it?

I think we can all agree about not extending foreground and related 
attributes, but extending background is a long-standing behavior. So it 
would make sense to introduce non-extending backgrounds only in select 
faces and gradually. I think that's the general expectation in the Emacs 
community. But we don't have to do this, we can go another way.

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

I'm not talking about face merging (*).

I'm talking about merging the attributes from the two properties (old 
and new) when computing the value of the array.

TBH, this is difficult to discuss. "Merging" and "array computation" are 
very generic terms.

(*) We shouldn't be talking about face merging at all because whatever 
happens after an "array" has been "computed" is opaque to defface, 
custom-theme-set-faces and custom-set-faces. So we should only discuss 
how "array" is computed, and what affects its resulting value, and not 
whatever happens next.

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

You haven't pointed at any code to read. So if this email doesn't help 
reach clarity, could you give some pointers?

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

Like I said, the new property I suggested is a way to go around having 
to change custom-theme-set-faces in an incompatible way. We would create 
a second "namespace" for face attributes that wouldn't be overwritten.

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

Yes. Sorry for not making this clear. Since you outlined the "unnamed 
face value" situation, I've had to amend the idea a little (**). 
Further, since the :extend attribute would still be used, this wouldn't 
require too many changes on the C level.

The new symbol property could be used for different attributes, but it 
seems like :extend needs it the most.

(**) Although we could go back to my previous suggestion of only setting 
"extend" via symbol property. That would still require the new :extend 
attribute, but it could remain hidden from the user and the Lisp code.