|
From: | Dmitry Gutov |
Subject: | bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages |
Date: | Sat, 7 Dec 2019 20:55:31 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 |
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 +0200I 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.
[Prev in Thread] | Current Thread | [Next in Thread] |