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

From: Andy Moreton <[email-masked]>
Date: Fri, 30 May 2025 19:23:38 +0100

On Wed 28 May 2025, Eli Zaretskii wrote:

>> From: Steven Allen <[email-masked]>
>> Cc: [email-masked], [email-masked], [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.
>
> Thanks.
>
> Unfortunately, what you say above seems to assume the level of
> detailed knowledge in themes, use-package, and their interactions with
> Customize that I don't have.  I think I know how faces work and how
> face merging works, but after reading your explanations and examples
> several times I'm sorry to say that I'm utterly confused by what you
> say and don't have any clear idea about the problem, its consequences,
> and your analysis of the original issue and its fix.
>
> So either someone explains this to me in terms I can understand and
> reason about, or we will need to wait for someone more knowledgeable
> in this stuff to pick up the gauntlet.
>
> Alternatively, if you convince John and Michael that the changes you
> think should be done are indeed the way to go, I will gladly delegate
> the decision to you three.
>
> Sorry I couldn't be of more help.

I'm not at all expert, but this effect is easily reproducible.
Using the mingw64 build on Windows:

a) build from master, and start with "runemacs.exe -Q"
b) in the scratch buffer, evaluate:
  (use-package faces
    :custom-face
    (default
     ((((class color)) :font "DejaVu Sans Mono-9" :background "gray85")))
    (variable-pitch
     ((((class color)) :font "Liberation Sans-10"))) )

c) Display a tooltip e.g. from moving the mouse over the major mode
   indicator on the modeline. The tooltip is shown using a monospaced
   font, even though the tooltip face inherits from variable-pitch.

d) "M-x describe-face RET tooltip RET" shows the tooltip face as
   expected, with the sample text in a variable pitch font.

e) Repeat all steps, but instead of (b), evaluate:
  (use-package faces
    :init
    (apply #'face-spec-set
           '(variable-pitch
             ((((class color)) :font "Liberation Sans-11"))))
    :custom-face
    (default
     ((((class color)) :font "DejaVu Sans Mono-9" :background "gray85")))
    )

   That produces the expected variable pitch font for the tooltip.

Thus this is not working properly, even without interaction with themes.

    AndyM