Subject: Re: bug#77928: 31.0.50; ebcde0f90f6 (bug#77928) breaks :custom-face for themed faces

From: Steven Allen <[email-masked]>
Date: Tue, 27 May 2025 09:38:28 -0700

Eli Zaretskii <[email-masked]> writes:

>> From: Steven Allen <[email-masked]>
>> Cc: Michael Shields <[email-masked]>, [email-masked]
>> Date: Mon, 26 May 2025 18:26:24 -0700
>>
>> John Wiegley <[email-masked]> writes:
>>
>> >>>>>> Eli Zaretskii <[email-masked]> writes:
>> >
>> >>> From: Michael Shields <[email-masked]>
>> >>> Date: Mon, 19 May 2025 22:51:29 -0700
>> >>> Cc: [email-masked], [email-masked]
>> >>>
>> >>> I think I understand the issue, but not its solution. There are more or less three standard ways to change
>> >>> faces now: Customize, themes, and use-package :custom-face. But there is no clear way to resolve the
>> >>> ordering between them, or whether they replace or inherit at each level. The behavior I was surprised by,
>> >>> and that has been reported previously, is that there was no way to specify a replacement for face settings.
>> >>> But Steven is surprised by the fact that things now happen at a level below theme application, and the theme
>> >>> replaces instead of inherits the defface spec.
>> >
>> >> John, could you perhaps help us out here?  Bug#77928 was about
>> >> :custom-face, which is a use-package feature.  I don't really
>> >> understand how this feature is supposed to work, given that
>> >> use-package produces macros whose effect happens when a package is
>> >> loaded, and given that other methods of face definition are available.
>> >
>> > All that use-package is doing here is expanding the ‘:custom-face‘ declaration
>> > into this call:
>> >
>> >   (apply #'face-spec-set '(font-lock-keyword-face ((t :italic t))))
>> >
>> > Should it be calling something other than `face-spec-set' now?
>>
>> That's what it used to be before the change in question. Now it expands
>> to:
>>
>>      (apply #'face-spec-set '(font-lock-keyword-face ((t :italic t)) face-defface-spec))
>>
>> The issue here is that the themed face spec has a higher priority than
>> the "defface" spec.
>
> But AFAIU, face-defface-spec records the theme customizations of the
> face as well, no?  I'm afraid I don't quite understand what you are
> saying here, could you please explain and elaborate?

face-defface-spec is only used when declaring faces, not when
customizing/theming them. The theme-face property is used to store the
themed face, customized-face is used to store customizations through the
"custom" system, and face-override-spec is used otherwise.

You can look at a face's plist to get a better idea of how this works:

    (symbol-plist 'default)

Also look at the documentation for facep-spec-recalc: theme and/or user
customizations override the "defface" spec, causing the bug I'm seeing here.

>> IMO, this should be reverted to just:
>>
>>      (apply #'face-spec-set '(font-lock-keyword-face ((t :italic t))))
>>
>> (leaving the part of the patch that marks the face as "modified" intact).
>>
>> >From what I can tell, the patch in question was attempting to solve an
>> issue where overriding faces get "merged" with "lower" priority face
>> specs but, IMO, that isn't really a bug.
>
> Why don't you consider that a bug?

That's how face customization works. E.g.:

    (custom-set-faces '(font-lock-keyword-face ((t :italic t)) t))

Will make keywords italic but won't override the theme/defaults unless
requested. E.g.:

    (custom-set-faces
      '(font-lock-keyword-face ((t :italic t :foreground unspecified)) t))

Will "unset" the foreground (it'll inherit from the face's parent). And:

    (custom-set-faces
      '(font-lock-keyword-face ((t :italic t :foreground reset)) t))

Will "reset" the foreground to the default face's foreground.

This makes it possible to compose/modify faces without completely
overriding them. E.g., if you want a specific face to be bold/italic but
otherwise want the underlying theme to apply. This makes it possible to,
e.g., customize certain aspects of faces across themes.