bug-gnu-emacs
[Top][All Lists]
Advanced

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

Attachment: inherit-face-extend-spec-3.diff
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]