[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#37774: 27.0.50; new :extend attribute broke visuals of all themes an
From: |
Dmitry Gutov |
Subject: |
bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages |
Date: |
Mon, 2 Dec 2019 02:05:10 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 |
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.
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages,
Dmitry Gutov <=
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages, Eli Zaretskii, 2019/12/02
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages, Dmitry Gutov, 2019/12/02
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages, Eli Zaretskii, 2019/12/03
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages, Dmitry Gutov, 2019/12/04
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages, Eli Zaretskii, 2019/12/05
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages, Dmitry Gutov, 2019/12/06
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages, Eli Zaretskii, 2019/12/06
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages, Dmitry Gutov, 2019/12/06
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages, Eli Zaretskii, 2019/12/06
- bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages, Dmitry Gutov, 2019/12/06