|
From: | Dmitry Gutov |
Subject: | bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages |
Date: | Fri, 6 Dec 2019 18:58:26 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 |
On 06.12.2019 18:18, Eli Zaretskii wrote:
Cc: 37774@debbugs.gnu.org, juri@linkov.net From: Dmitry Gutov <dgutov@yandex.ru> Date: Fri, 6 Dec 2019 17:44:33 +0200 It's great that you mentioned face-spec-recalc. It looks just like the place to change, since both defface and theme definitions and customizations go through it. We can implement in there a new kind of "face spec" along the lines of my previous description, or simply special-case the :extend attribute, and take it from the default spec. The latter option is implemented in the attached patch, which seems to work in my limited testing.Thanks, but is it clean enough to do such exemption for :extend?
I think so. Most importantly, it will save a lot of people the trouble of adapting to the current change, limiting the necessary efforts to just package authors (and Emacs core, of course).
Further, as you noted in https://debbugs.gnu.org/37774#404, :extend is somewhat special, in that we really want its values to be consistent, so the results look uniform. It's not the only attribute like that, but for others we have different way to ensure that, such as having a default face (where font face, height, etc, are usually customized, instead of doing that in all individual faces).
And if we want to do this, why do it in face-spec-recalc and not in custom-set-faces itself?
Not the place to do that. custom-theme-set-faces saves the specs defined by the theme (or user customization) to the face's symbol property, and then leaves it to face-spec-recalc to combine and apply all the specs related to the face.
The latter will not risk producing unintended consequences for callers of face-spec-recalc other than custom-set-faces.
I think the purpose of face-spec-recalc is clear enough. So I'd like to see at least one of those "unintended consequences".
- ;; defface spec entirely (rather than inheriting from it). If - ;; there was no spec applicable to FRAME, apply the defface spec - ;; as well as any applicable X resources. + ;; defface spec entirely rather than inheriting from it, with the + ;; exception of the :extend attribute (which is inherited).You slightly modified the syntax of the comment, probably a typo.
I inserted an empty line for clarity (IMHO), but I certainly do not insist on it. There would be other documentation changes required, too.
+ (when (and theme-face-applied (not themed-extend-attr)) + (let ((extend-p (plist-get default-attrs :extend))) + (and extend-p (face-spec-set-2 face frame '(:extend t)))))^^^^^^^^^^^^ I think this should be extend-p instead, because the face's default spec could legitimately say ":extend nil". Right?
But that's the default value of that attribute. If faces didn't specify it either, there's nothing to change, right?
[Prev in Thread] | Current Thread | [Next in Thread] |