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

[Dmitry Gutov]

On 26.11.2019 19:43, Eli Zaretskii wrote:

> > It's not for context diffs, it's for context around the changes in
> > unified diffs as well. Notice the gray background on the screenshot.
>
> Ah, okay.  Still, 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. And this would work without the change I just made anyway.

Too bad we don't have a way to affect all themes at once here.

> > > We need to modify all the themes we provide to specify :extend for
> > > faces where we do that by default.  It seems there's no way around
> > > that, since the semantics of custom-theme-set-faces is clearly to
> > > reset all face attributes to 'unspecified' before applying the face
> > > spec, so keeping some attributes from the default face spec is out of
> > > the question, unfortunately.  It's clear that the faces stuff was not
> > > designed to accommodate addition of attributes easily.
> >
> > 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).

Sounds like a win-win, avoiding the necessity to update all themes, 
first- and third-party ones. Also it seems like it's close to the way we 
usually introduce far-reaching changes in Emacs. But it's one option.

> As for the symbol
> property suggestion, see below.

See below as well (option two, in my opinion).

> > You said the latter would complicate the face merging code (which makes
> > sense) and "is extremely unclean". I have to take you at your word here.
>
> You don't have to take my word for it, the code is there to read and
> make up your own mind.

Forgive my ignorance, but xfaces.c is 7K lines long. It will be easier 
to speak high-level, but please correct whatever misconceptions I may bring.

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

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

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