|
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 19:06:08 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 |
On 07.12.2019 9:53, Eli Zaretskii wrote:
Well, we will have to document this exemption prominently, then.I suppose so. Though I wouldn't consider it too important to most users,It's important to developers of Lisp programs that manipulate faces.
Yes, sure.
I'm "avoiding such consequences" by trying to make sure the change is in the correct function and that it makes sense within the context.I 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.
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.
This distinction is handled in the second hunk of the patch where themed-extend-attr is calculated. If this attribute is not themed, there is no difference between nil and 'unspecified' (in the default spec).The use case I had in mind is this: . the theme doesn't specify :extend . the default spec for a face specifies ':extend nil' In this case, after applying the theme, the face should have ':extend nil', implicitly "inherited" from the default spec. Do you agree?
Ok, I think I understand the distinction: it's for when a character has several faces specified for it.
Sure. It's easy to fix anyway.
Finally, the value 'unspecified' seems impossible to get from the attributes list this way. plist-get will simply return nil.Exactly. And when a face does specify a nil value for :extend, then plist will return the list '(:extend nil), which is non-nil.
plist-member, you mean.
That said, I think I've found one valid scenario where this patch will do wrong: if the themed spec includes an :inherit directive, and the inherited face specifies (:extend nil). The current patch would inevitably ignore it and override with the value from the default spec.Once again, I think this part:+ (when (and theme-face-applied + (eq 'unspecified (face-attribute face :extend frame t))) + (let ((extend-p (plist-get default-attrs :extend))) + (and extend-p (face-spec-set-2 face frame '(:extend t)))))^^^^^^^^^^^^ isn't right, because it seems to say that when the default face says ':extend nil', the face after applying a theme will have ':extend t', which isn't TRT, IMO.
Updated patch attached.
inherit-face-extend-spec-3.diff
Description: Text Data
[Prev in Thread] | Current Thread | [Next in Thread] |