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

From: Eli Zaretskii <[email-masked]>
Date: Tue, 27 May 2025 14:23:08 +0300

> 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?

> 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?