GNU bug report logs - #77928
[PATCH] use-package :custom-face is meant to behave like custom-set-face

Previous Next

To reply to this bug, email your comments to 77928 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Michael Shields <shields <at> msrl.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] use-package :custom-face is meant to behave like
 custom-set-face
Date: Sat, 19 Apr 2025 13:41:01 -0700

[Message part 1 (text/plain, inline)]

The attached patch fixes a bug where migrating a face spec from custom.el
to use-package :custom-face results in a surprising behavior change: the
new spec is overlaid on the default value instead of replacing it. This
seems to have been an unintended consequence of
https://github.com/jwiegley/use-package/issues/934.

[Message part 2 (text/html, inline)]

[0001-Fix-use-package-custom-face-to-set-face-defface-spec.patch (application/octet-stream, attachment)]

Message #8 received at 77928 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Shields <shields <at> msrl.com>, John Wiegley <johnw <at> gnu.org>
Cc: 77928 <at> debbugs.gnu.org
Subject: Re: bug#77928: [PATCH] use-package :custom-face is meant to behave
 like custom-set-face
Date: Sun, 20 Apr 2025 09:10:47 +0300

> From: Michael Shields <shields <at> msrl.com>
> Date: Sat, 19 Apr 2025 13:41:01 -0700
> 
> The attached patch fixes a bug where migrating a face spec from custom.el to use-package :custom-face
> results in a surprising behavior change: the new spec is overlaid on the default value instead of replacing it.
> This seems to have been an unintended consequence of
> https://github.com/jwiegley/use-package/issues/934.

John, any comments?

> From 748e620fe2d286a853f4030bba16c99470387a1b Mon Sep 17 00:00:00 2001
> From: Michael Shields <shields <at> msrl.com>
> Date: Sat, 19 Apr 2025 12:58:26 -0700
> Subject: [PATCH] Fix use-package :custom-face to set face-defface-spec
> 
> By default, `face-set-spec' sets the override face spec, so the supplied
> face attributes are combined with the default, rather than replacing
> them.  This was a behavior change that was an apparently unintended
> consequence of commit 6b344a9.
> 
> Also set the `face-modified' property, which causes Customize to flag
> the face as changed outside Customize.
> 
> * doc/misc/use-package.texi (Faces):
> * lisp/use-package/use-package-core.el (use-package-handler/:custom-face):
> (use-package):
> * test/lisp/use-package/use-package-tests.el (use-package-test/:custom-face-1):
> (use-package-test/:custom-face-2):
> (use-package-test/:custom-face-3):
> (use-package-test/:custom-face-4):
> ---
>  doc/misc/use-package.texi                  |  5 +++
>  lisp/use-package/use-package-core.el       |  8 +++--
>  test/lisp/use-package/use-package-tests.el | 40 ++++++++++++++++++----
>  3 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/doc/misc/use-package.texi b/doc/misc/use-package.texi
> index c14e7b77d23..cdae8d6e662 100644
> --- a/doc/misc/use-package.texi
> +++ b/doc/misc/use-package.texi
> @@ -1457,6 +1457,11 @@ Faces
>  @end group
>  @end lisp
>  
> +Similarly to @code{:custom} (@pxref{User options}), this allows
> +configuring customizable faces outside of Customize (@pxref{Saving
> +Customizations,,, emacs, GNU Emacs Manual}).  Using both systems to
> +configure the same face can lead to confusing results.
> +
>  @node Hiding minor modes
>  @section Hiding minor modes with diminish and delight
>  @cindex hiding minor modes
> diff --git a/lisp/use-package/use-package-core.el b/lisp/use-package/use-package-core.el
> index c04053c22ac..4b63d985604 100644
> --- a/lisp/use-package/use-package-core.el
> +++ b/lisp/use-package/use-package-core.el
> @@ -1584,7 +1584,11 @@ use-package-normalize/:custom-face
>  (defun use-package-handler/:custom-face (name _keyword args rest state)
>    "Generate use-package custom-face keyword code."
>    (use-package-concat
> -   (mapcar #'(lambda (def) `(apply #'face-spec-set (backquote ,def))) args)
> +   (mapcar #'(lambda (def)
> +               `(progn
> +                  (apply #'face-spec-set (append (backquote ,def) '(face-defface-spec)))
> +                  (put ',(car def) 'face-modified t)))
> +           args)
>     (use-package-process-keywords name rest state)))
>  
>  ;;;; :init
> @@ -1848,7 +1852,7 @@ use-package
>  :custom          Call `Custom-set' or `set-default' with each variable
>                   definition without modifying the Emacs `custom-file'.
>                   (compare with `custom-set-variables').
> -:custom-face     Call `custom-set-faces' with each face definition.
> +:custom-face     Call `face-spec-set' with each face definition.
>  :ensure          Loads the package using package.el if necessary.
>  :pin             Pin the package to an archive.
>  :vc              Install the package directly from a version control system
> diff --git a/test/lisp/use-package/use-package-tests.el b/test/lisp/use-package/use-package-tests.el
> index 8554b37d5b8..b221c5de5c1 100644
> --- a/test/lisp/use-package/use-package-tests.el
> +++ b/test/lisp/use-package/use-package-tests.el
> @@ -1153,7 +1153,12 @@ use-package-test/:custom-face-1
>    (match-expansion
>     (use-package foo :custom-face (foo ((t (:background "#e4edfc")))))
>     `(progn
> -      (apply #'face-spec-set (backquote (foo ((t (:background "#e4edfc"))))))
> +      (progn
> +        (apply #'face-spec-set
> +               (append (backquote (foo ((t (:background "#e4edfc")))))
> +                       '(face-defface-spec))
> +               )
> +        (put 'foo 'face-modified t))
>        (require 'foo nil nil))))
>  
>  (ert-deftest use-package-test/:custom-face-2 ()
> @@ -1163,19 +1168,42 @@ use-package-test/:custom-face-2
>       (example-1-face ((t (:foreground "LightPink"))))
>       (example-2-face ((t (:foreground "LightGreen")))))
>     `(progn
> -      (apply #'face-spec-set
> -             (backquote (example-1-face ((t (:foreground "LightPink"))))))
> -      (apply #'face-spec-set
> -             (backquote (example-2-face ((t (:foreground "LightGreen"))))))
> +      (progn
> +        (apply #'face-spec-set
> +               (append (backquote (example-1-face ((t (:foreground "LightPink")))))
> +                       '(face-defface-spec)))
> +        (put 'example-1-face 'face-modified t))
> +      (progn
> +        (apply #'face-spec-set
> +               (append (backquote (example-2-face ((t (:foreground "LightGreen")))))
> +                       '(face-defface-spec)))
> +        (put 'example-2-face 'face-modified t))
>        (require 'example nil nil))))
>  
>  (ert-deftest use-package-test/:custom-face-3 ()
>    (match-expansion
>     (use-package foo :custom-face (foo ((t (:background "#e4edfc"))) face-defspec-spec))
>     `(progn
> -      (apply #'face-spec-set (backquote (foo ((t (:background "#e4edfc"))) face-defspec-spec)))
> +      (progn
> +        (apply #'face-spec-set
> +               (append (backquote (foo ((t (:background "#e4edfc"))) face-defspec-spec))
> +                       '(face-defface-spec)))
> +        (put 'foo 'face-modified t))
>        (require 'foo nil nil))))
>  
> +(ert-deftest use-package-test/:custom-face-4 ()
> +  (defface use-package-test/base-face '((t (:background "green"))) "")
> +  (defface use-package-test/face '((t (:inherit use-package-test/base-face))) "")
> +  (use-package emacs
> +    :custom-face
> +    (use-package-test/face ((t (:foreground "blue")))))
> +  (should (equal (face-foreground 'use-package-test/face nil t)
> +                 "blue"))
> +  (should (equal (face-background 'use-package-test/face nil t)
> +                 nil))
> +  (should (equal (get 'use-package-test/face 'face-modified)
> +                 t)))
> +
>  (ert-deftest use-package-test/:init-1 ()
>    (match-expansion
>     (use-package foo :init (init))
> -- 
> 2.49.0
> 

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.