bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Damien Cassou]

I have this in my init.el file:

(set-face-attribute 'finsit-javascript-html-tag-face nil :background nil)

As you can see, the FRAME argument (the 2nd) is nil. The documentation
says: "If FRAME is nil, set the attributes for all existing frames, as
well as the default for new frames.".

However, when creating a new frame the background of this face is
reset and I have to execute this line manually.

This might be related to
https://lists.gnu.org/archive/html/emacs-bug-tracker/2021-10/msg00694.html
but I'm not sure and I don't really understand the discussion.

Workaround:

  (defun my/fix-finsit-javascript-html-tag-face ()
    "Remove the background of `finsit-javascript-html-tag-face'."
    (set-face-attribute 'finsit-javascript-html-tag-face nil :background nil))

  (add-hook 'server-after-make-frame-hook #'my/fix-finsit-javascript-html-tag-face)



In GNU Emacs 28.0.91 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.30, cairo version 1.16.0)
Repository revision: emacs-28.0.91
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12014000
System Description: Fedora Linux 35 (Workstation Edition)

Configured using:
 'configure
 --prefix=/nix/store/pw4gz0xqxx8jai84ljsc8xk9km6zqsg9-emacs-gcc-28.0.91
 --disable-build-details --with-modules --with-x-toolkit=gtk3 --with-xft
 --with-cairo --with-native-compilation'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NATIVE_COMP NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $EMACSLOADPATH: /nix/store/hpbp1wn0jqc1amyxpw2bd2flwzh4fd2d-emacs-packages-deps/share/emacs/site-lisp:
  value of $EMACSNATIVELOADPATH: /nix/store/hpbp1wn0jqc1amyxpw2bd2flwzh4fd2d-emacs-packages-deps/share/emacs/native-lisp::
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: ELisp/l

Minor modes in effect:
  ws-butler-mode: t
  firestarter-mode: t
  shell-switcher-mode: t
  diff-hl-mode: t
  editorconfig-mode: t
  marginalia-mode: t
  vertico-indexed-mode: t
  vertico-mode: t
  savehist-mode: t
  envrc-global-mode: t
  envrc-mode: t
  minions-mode: t
  mpdel-mode: t
  paredit-mode: t
  aggressive-indent-mode: t
  nameless-mode: t
  display-line-numbers-mode: t
  bug-reference-prog-mode: t
  global-subword-mode: t
  subword-mode: t
  beginend-global-mode: t
  beginend-prog-mode: t
  global-paren-face-mode: t
  paren-face-mode: t
  electric-pair-mode: t
  which-key-mode: t
  drag-stuff-global-mode: t
  org-roam-db-autosync-mode: t
  goggles-mode: t
  global-git-commit-mode: t
  runner-run-in-background: t
  magit-auto-revert-mode: t
  shell-dirtrack-mode: t
  recentf-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  global-hl-line-mode: t
  global-auto-revert-mode: t
  winner-mode: t
  save-place-mode: t
  global-so-long-mode: t
  auto-compile-on-load-mode: t
  auto-compile-on-save-mode: t
  auto-compile-mode: t
  override-global-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/cassou/.emacs.d/lib/flycheck-elsa/Elsafile hides /home/cassou/.emacs.d/lib/elsa/Elsafile
/home/cassou/.emacs.d/lib/password-store/password-store hides /home/cassou/.nix-profile/share/emacs/site-lisp/password-store
/home/cassou/.nix-profile/share/emacs/site-lisp/site-start hides /nix/store/hpbp1wn0jqc1amyxpw2bd2flwzh4fd2d-emacs-packages-deps/share/emacs/site-lisp/site-start
/home/cassou/.nix-profile/share/emacs/site-lisp/site-start hides /nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/site-start
/home/cassou/.emacs.d/lib/marginalia/marginalia hides /nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/marginalia-20220217.21/marginalia
/home/cassou/.emacs.d/lib/marginalia/marginalia-autoloads hides /nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/marginalia-20220217.21/marginalia-autoloads
/home/cassou/.emacs.d/lib/tablist/tablist-filter hides /nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/tablist-20200427.2205/tablist-filter
/home/cassou/.emacs.d/lib/tablist/tablist-autoloads hides /nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/tablist-20200427.2205/tablist-autoloads
/home/cassou/.emacs.d/lib/tablist/tablist hides /nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/tablist-20200427.2205/tablist
/home/cassou/.nix-profile/share/emacs/site-lisp/site-start hides /nix/store/pw4gz0xqxx8jai84ljsc8xk9km6zqsg9-emacs-gcc-28.0.91/share/emacs/site-lisp/site-start
/nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/jsonrpc-1.0.15/jsonrpc hides /nix/store/pw4gz0xqxx8jai84ljsc8xk9km6zqsg9-emacs-gcc-28.0.91/share/emacs/28.0.91/lisp/jsonrpc
/home/cassou/.emacs.d/lib/transient/lisp/transient hides /nix/store/pw4gz0xqxx8jai84ljsc8xk9km6zqsg9-emacs-gcc-28.0.91/share/emacs/28.0.91/lisp/transient
/nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/flymake-1.2.2/flymake hides /nix/store/pw4gz0xqxx8jai84ljsc8xk9km6zqsg9-emacs-gcc-28.0.91/share/emacs/28.0.91/lisp/progmodes/flymake
/nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/xref-1.3.2/xref hides /nix/store/pw4gz0xqxx8jai84ljsc8xk9km6zqsg9-emacs-gcc-28.0.91/share/emacs/28.0.91/lisp/progmodes/xref
/nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/nadvice-0.3/nadvice hides /nix/store/pw4gz0xqxx8jai84ljsc8xk9km6zqsg9-emacs-gcc-28.0.91/share/emacs/28.0.91/lisp/emacs-lisp/nadvice
/home/cassou/.emacs.d/lib/hierarchy/hierarchy hides /nix/store/pw4gz0xqxx8jai84ljsc8xk9km6zqsg9-emacs-gcc-28.0.91/share/emacs/28.0.91/lisp/emacs-lisp/hierarchy
/nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/let-alist-1.0.6/let-alist hides /nix/store/pw4gz0xqxx8jai84ljsc8xk9km6zqsg9-emacs-gcc-28.0.91/share/emacs/28.0.91/lisp/emacs-lisp/let-alist
/nix/store/ld0vv2k4yjy7388jab1mn1p7f1bv3qiz-emacs-packages-deps/share/emacs/site-lisp/elpa/eldoc-1.11.0/eldoc hides /nix/store/pw4gz0xqxx8jai84ljsc8xk9km6zqsg9-emacs-gcc-28.0.91/share/emacs/28.0.91/lisp/emacs-lisp/eldoc

Features:
(shadow macrostep-c cmacexp macrostep emacsbug view ledger-import
descr-text ivy-hydra hydra lv wdired flycheck-hledger ledger-mode
ledger-check ledger-texi ledger-test ledger-sort ledger-report
ledger-reconcile ledger-occur ledger-fonts ledger-fontify ledger-state
ledger-complete ledger-schedule ledger-init ledger-xact ledger-post
ledger-exec ledger-navigate eshell esh-cmd esh-ext esh-opt esh-proc
esh-io esh-arg esh-module esh-groups esh-util ledger-context
ledger-commodities ledger-regex tabify timezone zoom-frm frame-cmds
frame-fns avoid helpful trace edebug info-look elisp-refs embrace
expand-region subword-mode-expansions text-mode-expansions
cc-mode-expansions the-org-mode-expansions python-el-fgallina-expansions
js2-mode-expansions js-mode-expansions html-mode-expansions
css-mode-expansions er-basic-expansions expand-region-core
expand-region-custom org-clock org-duration cal-iso mixed-pitch
ox-beamer org-protocol shr-color dabbrev markdown-mode edit-indirect
json-mode json-snatcher csharp-mode csharp-compilation cc-langs
magit-imenu duplicate-thing mhtml-mode css-mode smie eww shr kinsoku svg
mm-url gnus nnheader sgml-mode facemenu dom wgrep grep gnus-dired
help-fns cl-print debug backtrace magit-ediff ediff ediff-merg
ediff-mult ediff-wind ediff-diff ediff-help ediff-init ediff-util gnutls
gravatar dns url-cache git-rebase consult-xref misearch multi-isearch
network-stream ws-butler firestarter cursor-sensor editorconfig-core
editorconfig-core-handle editorconfig-fnmatch vterm compile term
disp-table ehelp vterm-module term/xterm xterm shell-switcher rswitcher
diff-hl log-view vc-dir ewoc vc-mtn vc-hg vc-bzr vc-src vc-sccs vc-svn
vc-cvs vc-rcs direnv consult-imenu embark-consult consult-vertico
consult finsit-prodigy prodigy f vertico-directory tramp-cmds sendmail
qp sort company-oddmuse company-keywords company-etags company-gtags
company-dabbrev-code company-files company-clang company-capf
company-cmake company-semantic company-template company-bbdb mail-extr
mm-archive mule-util notmuch notmuch-tree notmuch-jump notmuch-hello
notmuch-show notmuch-print notmuch-crypto notmuch-mua notmuch-message
notmuch-draft notmuch-maildir-fcc notmuch-address notmuch-company
notmuch-parser notmuch-wash coolj notmuch-query goto-addr icalendar
diary-lib diary-loaddefs notmuch-tag notmuch-lib notmuch-version
notmuch-compat mm-view mml-smime smime dig flyspell ispell editorconfig
embark ffap orderless epkg-marginalia epkg-melpa epkg-gelpa epkg-utils
epkg-list epkg-desc epkg closql marginalia vertico-indexed vertico
savehist envrc inheritenv nix-sandbox ob-verb verb finsit-javascript
js2-imenu-extras indium indium-list-sources indium-scratch
indium-interaction indium-chrome indium-nodejs indium-repl
indium-debugger indium-debugger-litable indium-debugger-locals
indium-breakpoint indium-inspector indium-render indium-faces cus-edit
cus-load indium-seq-fix indium-client indium-structs json-process-client
xref-js2 vc flycheck-package package-lint finder finder-inf flycheck
company-dabbrev company tern eslintd-fix xdg js2-refactor js2r-paredit
js2r-conveniences js2r-conditionals js2r-wrapping js2r-functions
yasnippet js2r-vars mc-hide-unmatched-lines-mode mc-mark-more thingatpt
mc-cycle-cursors multiple-cursors-core rect js2r-iife js2r-formatting
js2r-helpers s js2-mode etags fileloop xref project js cc-mode cc-fonts
cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs
url-queue minions ivy-mpdel ivy flx delsel ivy-faces ivy-overlay colir
mpdel mpdel-browser libmpdel-directory mpdel-playlist mpdel-tablist
mpdel-song mpdel-core navigel tablist tablist-filter
semantic/wisent/comp semantic/wisent semantic/wisent/wisent
semantic/util-modes semantic/util semantic semantic/tag semantic/lex
semantic/fw mode-local cedet libmpdel tq time-stamp alert log4e
notifications dbus xml finsit-basecamp epa-file libbcel libbcel-actions
libbcel-nav libbcel-structs libbcel-client libbcel-oauth request
libbcel-util finsit-magit finsit-bugref vc-git vc-dispatcher paredit
aggressive-indent nameless display-line-numbers magit-bookmark bookmark
pp finsit-core bug-reference cap-words superword subword beginend
auth-source-pass paren-face elec-pair unify-opening which-key drag-stuff
org-roam-dailies org-roam-migrate org-roam-mode org-roam-capture
org-roam-id org-roam-node org-roam-db org-roam-utils org-roam-compat
org-roam org-capture org-id ox-twbs ox-odt rng-loc rng-uri rng-parse
rng-match rng-dt rng-util rng-pttrn nxml-parse nxml-ns nxml-enc xmltok
nxml-util ox-latex ox-icalendar org-agenda org-refile ox-html table
ox-ascii ox-publish ox org-element avl-tree generator org org-macro
org-footnote org-pcomplete org-list org-faces org-entities noutline
outline org-version ob-python python tramp-sh tramp tramp-loaddefs
trampver tramp-integration files-x tramp-compat parse-time ls-lisp ob-R
ob-dot ob-emacs-lisp ob-shell ob ob-tangle org-src ob-ref ob-lob
ob-table ob-exp ob-comint ob-core ob-eval org-table oc-basic bibtex
iso8601 ol org-keys oc org-compat org-macs org-loaddefs find-func
cal-menu calendar cal-loaddefs emacsql-sqlite url-http url-auth url-gw
nsm advice emacsql emacsql-compiler goggles pulse color ace-link avy
moody let-alist magit-tbdiff magit-extras magit-submodule magit-obsolete
magit-popup magit-blame magit-stash magit-reflog magit-bisect magit-push
magit-pull magit-fetch magit-clone magit-remote magit-commit
magit-sequence magit-notes magit-worktree magit-tag magit-merge
magit-branch magit-reset magit-files magit-refs magit-status magit
package browse-url url url-proxy url-privacy url-expand url-methods
url-history url-cookie url-domsuf url-util mailcap url-handlers
url-parse url-vars magit-repos magit-apply magit-wip magit-log
which-func magit-diff smerge-mode diff-mode git-commit log-edit message
rmc puny dired-imenu imenu runner dired-aux dired-x dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs auth-source password-cache json map text-property-search
time-date mm-decode mm-bodies mm-encode mailabbrev mail-utils gmm-utils
mailheader pcvs-util add-log magit-core magit-autorevert magit-margin
magit-transient magit-process with-editor shell pcomplete comint server
ansi-color magit-mode transient format-spec magit-git magit-section
magit-utils crm eieio eieio-core eieio-loaddefs dash recentf tree-widget
wid-edit undo-tree diff queue lin face-remap hl-line info-colors
autorevert filenotify winner ring saveplace so-long jka-compr
modus-operandi-theme modus-themes no-littering auto-compile comp
comp-cstr warnings packed cl-macs use-package use-package-ensure
use-package-delight use-package-diminish use-package-bind-key bind-key
easy-mmode use-package-core rx edmacro kmacro cl-extra help-mode cl-seq
borg subr-x pcase info cl-loaddefs cl-lib autoload radix-tree lisp-mnt
mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr seq
byte-opt gv bytecomp byte-compile cconv iso-transl tooltip eldoc paren
electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice
button loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 2599129 406378)
 (symbols 48 102505 3)
 (strings 32 522205 64036)
 (string-bytes 1 27283234)
 (vectors 16 402178)
 (vector-slots 8 6373952 1321791)
 (floats 8 1265 2952)
 (intervals 56 216397 10807)
 (buffers 992 243))

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Lars Ingebrigtsen]

Damien Cassou <damien@cassou.me> writes:

> I have this in my init.el file:
>
> (set-face-attribute 'finsit-javascript-html-tag-face nil :background nil)

It's perhaps easier to reproduce with a standard face:

(set-face-attribute 'region nil :background nil)

Eval this and mark some area and see that it marking a region no longer
has a background.  `C-x 5 2' and everything is back to normal in the new
frame.

However, this works:

(set-face-attribute 'region nil :background "red")

So it looks like setting something to nil is forgotten?  I.e., there's
no difference between nil and "use the default", I'm guessing, but I
haven't tried to debug further.  Anybody familiar with the code in this
area?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Eli Zaretskii]

> From: Damien Cassou <damien@cassou.me>
> Date: Fri, 25 Feb 2022 11:21:45 +0100
> 
> I have this in my init.el file:
> 
> (set-face-attribute 'finsit-javascript-html-tag-face nil :background nil)
> 
> As you can see, the FRAME argument (the 2nd) is nil. The documentation
> says: "If FRAME is nil, set the attributes for all existing frames, as
> well as the default for new frames.".
> 
> However, when creating a new frame the background of this face is
> reset and I have to execute this line manually.

Please show a complete recipe for reproducing the problem, starting
from "emacs -Q".  In particular, we need to see:

  . how that face was defined in the first place (defface or somesuch)
  . how you create a new frame

> Workaround:
> 
>   (defun my/fix-finsit-javascript-html-tag-face ()
>     "Remove the background of `finsit-javascript-html-tag-face'."
>     (set-face-attribute 'finsit-javascript-html-tag-face nil :background nil))
> 
>   (add-hook 'server-after-make-frame-hook #'my/fix-finsit-javascript-html-tag-face)

Since you set 'server-after-make-frame-hook', does this mean the
problem happens only with emacsclient frames?  If so, that is one more
reason for providing a complete recipe for us to investigate.

Thanks.

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Eli Zaretskii]

> Resent-From: Lars Ingebrigtsen <larsi@gnus.org>
> Original-Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
> Resent-CC: bug-gnu-emacs@gnu.org
> Resent-Sender: help-debbugs@gnu.org
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 25 Feb 2022 13:15:32 +0100
> Cc: 54156@debbugs.gnu.org
> 
> Damien Cassou <damien@cassou.me> writes:
> 
> > I have this in my init.el file:
> >
> > (set-face-attribute 'finsit-javascript-html-tag-face nil :background nil)
> 
> It's perhaps easier to reproduce with a standard face:
> 
> (set-face-attribute 'region nil :background nil)
> 
> Eval this and mark some area and see that it marking a region no longer
> has a background.  `C-x 5 2' and everything is back to normal in the new
> frame.
> 
> However, this works:
> 
> (set-face-attribute 'region nil :background "red")
> 
> So it looks like setting something to nil is forgotten?  I.e., there's
> no difference between nil and "use the default", I'm guessing, but I
> haven't tried to debug further.  Anybody familiar with the code in this
> area?

Does this issue happen only with the nil values of :background?  If
so, may I ask what do you and Damien think should be the effect of
setting the background color of a face to nil?

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Does this issue happen only with the nil values of :background?  If
> so, may I ask what do you and Damien think should be the effect of
> setting the background color of a face to nil?

That there is no background colour on the face -- that's the effect of
doing that (but only on existing frames).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: damien@cassou.me,  54156@debbugs.gnu.org
> Date: Fri, 25 Feb 2022 13:30:14 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Does this issue happen only with the nil values of :background?  If
> > so, may I ask what do you and Damien think should be the effect of
> > setting the background color of a face to nil?
> 
> That there is no background colour on the face -- that's the effect of
> doing that (but only on existing frames).

But that's not what nil does in this case.  It means the same as
'unspecified': that you have nothing to say about that particular
attribute.  So Emacs does nothing.

Think about it: any face attribute not explicitly mentioned in a
defface is set to 'unspecified' by the low-level code.  This is basic
in how faces are handled in Emacs; we cannot easily change that
without breaking gobs of code.

Moreover, attributes of the faces for future frames, the ones you get
by calling face-attribute with FRAME = t, are documented to be
'unspecified' by default.

The correct way to do what Damien wants (AFAIU) is this:

  (set-face-attribute 'region nil :background 'unspecified)
  (set-face-attribute 'region t :background 'unspecified)

That is, one must explicitly call set-face-attribute with FRAME = t
(as well as nil), and pass 'unspecified' (NOT nil!) as the value.
Maybe we should document that, although it is a obscure and unusual
thing to do.

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> But that's not what nil does in this case.  It means the same as
> 'unspecified': that you have nothing to say about that particular
> attribute.  So Emacs does nothing.

But like I said -- Emacs does do something, but only on existing frames.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: damien@cassou.me,  54156@debbugs.gnu.org
> Date: Fri, 25 Feb 2022 14:12:17 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But that's not what nil does in this case.  It means the same as
> > 'unspecified': that you have nothing to say about that particular
> > attribute.  So Emacs does nothing.
> 
> But like I said -- Emacs does do something, but only on existing frames.

Right; sorry for not being accurate.

But let me explain a bit more.

The default attributes for faces that are used when creating new
frames are stored in face--new-frame-defaults.  When a frame is
created, those attributes are merged with what the face's spec (from
defface) says.  Thus, having 'unspecified' in face--new-frame-defaults
for an attribute has no effect: the definition of the attribute in
defface will override it.

So we need a special trick to override defface with 'unspecified', and
that trick is this call:

  (set-face-attribute 'region t :background 'unspecified)

This is handled specially in internal-set-lisp-face-attribute to do
what Damien wants.

bug#54156: [External] : bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Drew Adams]

> The default attributes for faces that are used when creating new
> frames are stored in face--new-frame-defaults.  When a frame is
> created, those attributes are merged with what the face's spec (from
> defface) says.  Thus, having 'unspecified' in face--new-frame-defaults
> for an attribute has no effect: the definition of the attribute in
> defface will override it.
> 
> So we need a special trick to override defface with 'unspecified',
> and that trick is this call:
>   (set-face-attribute 'region t :background 'unspecified)
> This is handled specially in internal-set-lisp-face-attribute to
> do what Damien wants.

This is likely all documented.  However, it's a
bit tricky to absorb, and perhaps to explain.

Consider taking another look at the doc for this,
and see if you think you could improve it to make
this a bit clearer.

(Just a suggestion.  Pretty much any text can be
improved, and rereading is the way to begin.)

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> So we need a special trick to override defface with 'unspecified', and
> that trick is this call:
>
>   (set-face-attribute 'region t :background 'unspecified)
>
> This is handled specially in internal-set-lisp-face-attribute to do
> what Damien wants.

So perhaps set-face-attribute should do that automatically when handed a
nil value?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: damien@cassou.me,  54156@debbugs.gnu.org
> Date: Sat, 26 Feb 2022 16:04:28 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So we need a special trick to override defface with 'unspecified', and
> > that trick is this call:
> >
> >   (set-face-attribute 'region t :background 'unspecified)
> >
> > This is handled specially in internal-set-lisp-face-attribute to do
> > what Damien wants.
> 
> So perhaps set-face-attribute should do that automatically when handed a
> nil value?

The fact that we handle nil as 'unspecified' in the case of the color
is for compatibility with Emacs 20.  In Emacs 21 and later, a color
cannot be nil.  This is well-documented: a color must be a string or
'unspecified'.  I don't think we want to resurrect that old convention
of nil; it's just that Damien and others will have to get used to not
using nil for a color.

bug#54156: [External] : bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Drew Adams]

> > > So we need a special trick to override defface with 'unspecified',
> > > and that trick is this call:
> > >   (set-face-attribute 'region t :background 'unspecified)
> > > This is handled specially in internal-set-lisp-face-attribute
> > > to do what Damien wants.
> >
> > So perhaps set-face-attribute should do that automatically when
> > handed a nil value?
> 
> The fact that we handle nil as 'unspecified' in the case of the color
> is for compatibility with Emacs 20.  In Emacs 21 and later, a color
> cannot be nil.  This is well-documented: a color must be a string or
> 'unspecified'.  I don't think we want to resurrect that old convention
> of nil; it's just that Damien and others will have to get used to not
> using nil for a color.

What Eli says here makes sense to me.  We could also
perhaps explicitly (more visibly) recommend that you
always use an explicit `unspecified' rather than nil.
___

It does not make sense to me to change the longstanding
behavior, to make `set-face-attibute' do something
different (and backward-incompatible).  As Eli said:
"This is basic in how faces are handled in Emacs; we
cannot easily change that without breaking gobs of code."
___

I'm not sure I understand this from Eli, however:

  The correct way to do what Damien wants (AFAIU) is this:

   (set-face-attribute 'region nil :background 'unspecified)
   (set-face-attribute 'region t :background 'unspecified)

  That is, one must explicitly call set-face-attribute
  with FRAME = t (as well as nil), and pass 'unspecified'
               ^^^^^^^^^^^^^^^^^^
  (NOT nil!) as the value.  Maybe we should document that,
  although it is a obscure and unusual thing to do.

My impression is that it's enough to do this:

(set-face-attribute 'region nil :background 'unspecified)

I'm probably not testing/checking this correctly,
but it seems to me that both the selected frame
and new (future) frames are affected by using
nil for the frame (and `unspecified' for the
face attribute value).

In any case, yes, whatever the correct message
is for users, please do consider adding it to
the doc.  You might consider this an unusual
thing to do (why so?), but misunderstanding this
is a gotcha that we should help users avoid.

Eli, you say "This is well-documented: a color
must be a string or 'unspecified'."  Still, it
wouldn't hurt to add something to that effect in
the doc of `set-face-attribute' - and say what
the effect is of using nil (for the face color)
- it's tolerated (no error) but it doesn't have
the effect of `unspecified'.

bug#54156: [External] : bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "damien@cassou.me" <damien@cassou.me>,
>         "54156@debbugs.gnu.org"
> 	<54156@debbugs.gnu.org>
> Date: Sat, 26 Feb 2022 16:17:19 +0000
> 
> I'm not sure I understand this from Eli, however:
> 
>   The correct way to do what Damien wants (AFAIU) is this:
> 
>    (set-face-attribute 'region nil :background 'unspecified)
>    (set-face-attribute 'region t :background 'unspecified)
> 
>   That is, one must explicitly call set-face-attribute
>   with FRAME = t (as well as nil), and pass 'unspecified'
>                ^^^^^^^^^^^^^^^^^^
>   (NOT nil!) as the value.  Maybe we should document that,
>   although it is a obscure and unusual thing to do.
> 
> My impression is that it's enough to do this:
> 
> (set-face-attribute 'region nil :background 'unspecified)
> 
> I'm probably not testing/checking this correctly,
> but it seems to me that both the selected frame
> and new (future) frames are affected by using
> nil for the frame (and `unspecified' for the
> face attribute value).

I'm not sure this isn't the result of the particular implementation we
have, so I prefer to tell people to call with FRAME = t explicitly.
After all, this is a rare use case.

> Eli, you say "This is well-documented: a color
> must be a string or 'unspecified'."  Still, it
> wouldn't hurt to add something to that effect in
> the doc of `set-face-attribute' - and say what
> the effect is of using nil (for the face color)
> - it's tolerated (no error) but it doesn't have
> the effect of `unspecified'.

We generally don't advertise compatibility shims for obsolete
features, because we want people to stop using them.

bug#54156: [External] : bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Drew Adams]

> > I'm not sure I understand this from Eli, however:
> >
> >   The correct way to do what Damien wants (AFAIU) is this:
> >    (set-face-attribute 'region nil :background 'unspecified)
> >    (set-face-attribute 'region t :background 'unspecified)
> >   That is, one must explicitly call set-face-attribute
> >   with FRAME = t (as well as nil), and pass 'unspecified'
> >                ^^^^^^^^^^^^^^^^^^
> >   (NOT nil!) as the value.  Maybe we should document that,
> >   although it is a obscure and unusual thing to do.
> >
> > My impression is that it's enough to do this:
> >
> > (set-face-attribute 'region nil :background 'unspecified)
> >
> > I'm probably not testing/checking this correctly,
> > but it seems to me that both the selected frame
> > and new (future) frames are affected by using
> > nil for the frame (and `unspecified' for the
> > face attribute value).
> 
> I'm not sure this isn't the result of the particular implementation we
> have, so I prefer to tell people to call with FRAME = t explicitly.
> After all, this is a rare use case.

But is what I said correct, that just using nil
as the frame makes both the existing frames and
future ones use `unspecified' as the face value?

That's what I think I see, and that was what OP
was after, IIUC.

IOW, is it really necessary to use two calls to
`set-face-attribute', one with nil FRAME and one
with `t' FRAME?

And using _only_ `t' doesn't set the attribute
to `unspecified' for the existing frames, right?

> > Eli, you say "This is well-documented: a color
> > must be a string or 'unspecified'."  Still, it
> > wouldn't hurt to add something to that effect in
> > the doc of `set-face-attribute' - and say what
> > the effect is of using nil (for the face color)
> > - it's tolerated (no error) but it doesn't have
> > the effect of `unspecified'.
> 
> We generally don't advertise compatibility shims for obsolete
> features, because we want people to stop using them.

Is it declared to be obsolete?  We generally do
let users know when something they use is obsolete,
e.g. with a warning.  Do we do that for this case?

bug#54156: [External] : bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "larsi@gnus.org" <larsi@gnus.org>, "damien@cassou.me" <damien@cassou.me>,
>         "54156@debbugs.gnu.org" <54156@debbugs.gnu.org>
> Date: Sat, 26 Feb 2022 17:23:22 +0000
> 
> > I'm not sure this isn't the result of the particular implementation we
> > have, so I prefer to tell people to call with FRAME = t explicitly.
> > After all, this is a rare use case.
> 
> But is what I said correct, that just using nil
> as the frame makes both the existing frames and
> future ones use `unspecified' as the face value?

If you use 'unspecified', yes.

> And using _only_ `t' doesn't set the attribute
> to `unspecified' for the existing frames, right?

I didn't say to use only t.

> > We generally don't advertise compatibility shims for obsolete
> > features, because we want people to stop using them.
> 
> Is it declared to be obsolete?

It's considered unsupported.  We just silently support it for
compatibility with old versions.

bug#54156: [External] : bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Drew Adams]

> > But is what I said correct, that just using nil
> > as the frame makes both the existing frames and
> > future ones use `unspecified' as the face value?
> 
> If you use 'unspecified', yes.

Yes, what I said was that it seems using `unspecified'
for the face attribute value and nil for the frame is
all that's needed.

> > And using _only_ `t' doesn't set the attribute
> > to `unspecified' for the existing frames, right?
> 
> I didn't say to use only t.

I didn't say you did say to use only t.

You said to use both (separately).  My question was
why to bother _also_ using `t'.  IIUC, `t' alone
doesn't help with existing frames (I asked for
confirmation).

And nil alone helps with both existing and future,
AFAICT.  I was asking for confirmation that what I
reported is correct - there's no need to also use
`t'.

> > > We generally don't advertise compatibility shims for obsolete
> > > features, because we want people to stop using them.
> >
> > Is it declared to be obsolete? We generally do
> > let users know when something they use is obsolete,
> > e.g. with a warning.  Do we do that for this case?
> 
> It's considered unsupported.  We just silently support 
> it for compatibility with old versions.

We could still do that but also warn that it's obsolete.

It could help to show a warning that lets you know that
(based on your using nil here) you likely want to use
`unspecified' instead.  Isn't that what we usually do?

bug#54156: [External] : bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "larsi@gnus.org" <larsi@gnus.org>, "damien@cassou.me" <damien@cassou.me>,
>         "54156@debbugs.gnu.org" <54156@debbugs.gnu.org>
> Date: Sat, 26 Feb 2022 22:47:41 +0000
> 
> > I didn't say to use only t.
> 
> I didn't say you did say to use only t.
> 
> You said to use both (separately).  My question was
> why to bother _also_ using `t'.

Because it's future-proof.  And because resetting a color attribute to
'unspecified' so that it overrides the defface is an unusual thing to
do, and so it better stands out in the code, telling everyone that
this is the intent.

bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> The fact that we handle nil as 'unspecified' in the case of the color
> is for compatibility with Emacs 20.  In Emacs 21 and later, a color
> cannot be nil.  This is well-documented: a color must be a string or
> 'unspecified'.  I don't think we want to resurrect that old convention
> of nil; it's just that Damien and others will have to get used to not
> using nil for a color.

Makes sense to me.  Then I guess there's nothing more to do in this bug
report, and I'm closing it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54156: [External] : bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Drew Adams]

> > You said to use both (separately).
> > My question was why to bother _also_
> > using `t'.
> 
> Because it's future-proof.  And because resetting a color attribute to
> 'unspecified' so that it overrides the defface is an unusual thing to
> do, and so it better stands out in the code, telling everyone that
> this is the intent.

Thank you for finally stating your reason.  And
for finally confirming that what I said I thought
was the behavior is in fact the behavior.
___

FWIW, I disagree (but at least now it's clear
why you have that opinion).  I think telling
people to use both only confuses.  They'll wonder
why, the same as I did - unless you also indicate
your reason somehow/somewhere.
___

Beyond that, what about the question of making
backward-support behavior known?  Why only
"silently support" the use of nil this way?
Support, yes, sure.  But why silently?

We usually try to also provide a warning message
when you use constructs deemed deprecated/obsolete.
This pitfall is easy to fall into, IMO, regardless
of whether you think the reasonable use case is
rare.

You haven't yet given your reasons for resisting
making this gotcha better known to users - e.g.
by displaying a warning message.

When you don't give reasons it just leads to more
back & forth and misunderstanding.  Please try to
be clear and explicit about this.  Thank you.
___

I also don't agree that a user or user code would
only _rarely_ want to reset a face attribute to
`unspecified'.  We provide checkboxes in Customize
for exactly that, no?  Is this different from
unchecking a checkbox?

After you uncheck the Background checkbox for
`M-x customize-face region' this sexp returns
`unspecified':

 (face-attribute 'region :background t)

What am I missing, here?  If this has the same
effect as setting it to `unspecified' in Lisp:

 (set-face-attribute 'region nil :background 'unspecified)

then why consider it and "unusual" and "rare"?

bug#54156: [External] : bug#54156: 28.0.91; set-face-attribute with a nil FRAME doesn't change the default

[Drew Adams]

> > The fact that we handle nil as 'unspecified' in the case of the color
> > is for compatibility with Emacs 20.  In Emacs 21 and later, a color
> > cannot be nil.  This is well-documented: a color must be a string or
> > 'unspecified'.  I don't think we want to resurrect that old
> > convention of nil; it's just that Damien and others will have to get
> > used to not using nil for a color.
> 
> Makes sense to me.  Then I guess there's nothing more to do in this bug
> report, and I'm closing it.

Too bad.