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

From: Steven Allen <[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. 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.