bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages

[Dmitry Gutov <dgutov@yandex.ru>]

On 03.12.2019 18:21, Eli Zaretskii wrote:

>> I think we can all agree about not extending foreground and related
>> attributes, but extending background is a long-standing behavior. So it
>> would make sense to introduce non-extending backgrounds only in select
>> faces and gradually. I think that's the general expectation in the Emacs
>> community. But we don't have to do this, we can go another way.
> 
> I don't think we should restart this discussion, we already had it.

Fair enough.

>>> The array is computed only once, whereas merging happens many times
>>> and in different places.  So we cannot compute the value of an
>>> attribute only once, because its value depends on what other faces are
>>> being merged, on whether their :extend attribute is set. and on
>>> whether the particular merging process cares about :extend.
>>
>> I'm not talking about face merging (*).
> 
> This whole issue, and in fact the feature itself, is about face
> merging, and only about it.  Emacs displays faces by merging
> attributes from all of the possible sources of face information that
> are in effect at a given buffer position.

If the effect of the new symbol property is recorded in the array, 
whatever calculations happen thereafter using different arrays that 
represent faces (named or otherwise) should be orthogonal to my proposal.

>>> I don't think I understand your proposal in concrete terms, and you
>>> didn't read the code to check your proposal against the actual
>>> implementation, so this is a sure way to misunderstandings and talking
>>> past each other.
>>
>> You haven't pointed at any code to read. So if this email doesn't help
>> reach clarity, could you give some pointers?
> 
> I suggest to start with the large comment at the beginning of
> xfaces.c, and then proceed to read these functions:

Thank you.

>    get_lface_attributes

So apparently lface_from_face_name_no_resolve is the place where a face 
name turns into an plist-like array of attributes.

So we could, as a rough idea, look up the symbol property there and, if 
present, merge its contents with the return value.

I'm not quite sure of the purpose behind Vface_new_frame_defaults (vs. 
just using props on face symbols), but we could add a similar storage 
for "transient face spec". And in the frame structs too.

>    merge_face_vectors
>    merge_named_face
>    merge_face_ref
>    internal-make-lisp-face
>    internal-set-lisp-face-attribute

The last function might grow a new optional attribute: TRANSIENTP, if we 
want to be able to change such "transient" attributes at runtime.

To define "transient" here: these will be attributes that are not 
affected by custom-theme-set-faces unless explicitly mentioned in the 
corresponding specs. Which is what we had been discussing for :extend 
for quite a while.

>> The new symbol property could be used for different attributes, but it
>> seems like :extend needs it the most.
> 
> Sorry, still not clear.  Maybe providing examples of defining a face
> to be extended, and then repeating the above in more detail with
> references to the examples, would help in clearing the picture.

The new definition for diff-added would look like:

   (defface diff-added
     '((default
        :inherit diff-changed)
       (((class color) (min-colors 257) (background light))
        :background "#eeffee")
       (((class color) (min-colors 88) (background light))
        :background "#ddffdd")
       (((class color) (min-colors 88) (background dark))
        :background "#335533")
       (((class color))
        :foreground "green"))
     "`diff-mode' face used to highlight added lines.")

   (put 'diff-added 'face-transient-spec '((t :extend t)))

Or maybe like:

   (defface diff-added
     '((default
        :inherit diff-changed)
       (((class color) (min-colors 257) (background light))
        :background "#eeffee")
       (((class color) (min-colors 88) (background light))
        :background "#ddffdd")
       (((class color) (min-colors 88) (background dark))
        :background "#335533")
       (((class color))
        :foreground "green"))
     "`diff-mode' face used to highlight added lines."
     :transient '((t :extend t)))