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.