bug#53636: 29.0.50; face-remapping broken on master

bug#53636: 29.0.50; face-remapping broken on master

[Tassilo Horn]

I'm expeciencing/debugging a problem with the doom-themes package but
was able to come up with a simple recipe without dependencies.

--8<---------------cut here---------------start------------->8---
(defun doom-themes-visual-bell-fn ()
  "Blink the mode-line red briefly. Set `ring-bell-function' to this to use it."
  (let ((doom-themes--bell-cookie (face-remap-add-relative 'mode-line 'error)))
    (force-mode-line-update)
    (run-with-timer 0.15 nil
                    (lambda (cookie buf)
                      (with-current-buffer buf
                        (face-remap-remove-relative cookie)
                        (force-mode-line-update)))
                    doom-themes--bell-cookie
                    (current-buffer))))

(setq ring-bell-function #'doom-themes-visual-bell-fn
      visible-bell t)
--8<---------------cut here---------------end--------------->8---

When I eval that with emacs -Q, version 27.2, C-g will blink the
mode-line shortly (that is, all mode-line text is red and boldish for a
fraction of a second) as I would expect.  Same for the current emacs-28
branch.

However, with the current master configured --with-modules and
--with-pgtk, I don't see any flashes.  Even more, although I don't get
flashes, sometimes during normal emacs operation the mode-line will turn
into the error face after an error has happened and not pop back to
normal (unless I switch buffers back and forth) as if the timer function
would not have run.

I've also tried with master and --without-pgtk.  There it seems to be
broken, too, i.e., after evaling the code above, C-g or errors won't
flash the mode-line (but I can't say anything about the "suddenly the
error face sticks" issue).


In GNU Emacs 29.0.50 (build 41, x86_64-pc-linux-gnu, GTK+ Version 3.24.31, cairo version 1.17.4)
 of 2022-01-30 built on thinkpad-t440p
Repository revision: 988d3d79bac0343dd2b1b89d1b15470edbb5e6ac
Repository branch: master
System Description: Arch Linux

Configured using:
 'configure --with-modules --with-pgtk'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PGTK
PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP XIM
GTK3 ZLIB

Important settings:
  value of $LC_MONETARY: de_DE.utf8
  value of $LC_NUMERIC: de_DE.utf8
  value of $LC_TIME: de_DE.utf8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: ELisp/d

Minor modes in effect:
  symbol-overlay-mode: t
  display-fill-column-indicator-mode: t
  editorconfig-mode: t
  debbugs-browse-mode: t
  hl-todo-mode: t
  global-aggressive-indent-mode: t
  aggressive-indent-mode: t
  pdf-occur-global-minor-mode: t
  diredfl-global-mode: t
  which-key-mode: t
  highlight-parentheses-mode: t
  corfu-global-mode: t
  corfu-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  outline-minor-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  bug-reference-prog-mode: t
  vertico-mode: t
  marginalia-mode: t
  minibuffer-depth-indicate-mode: t
  electric-pair-mode: t
  recentf-mode: t
  pixel-scroll-precision-mode: t
  pixel-scroll-mode: t
  override-global-mode: t
  save-place-mode: t
  savehist-mode: t
  shell-dirtrack-mode: t
  puni-global-mode: t
  puni-mode: t
  tooltip-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:
~/Repos/el/mu/build/mu4e/mu4e hides ~/Repos/el/mu/mu4e/mu4e
~/Repos/el/mu/build/mu4e/mu4e-main hides ~/Repos/el/mu/mu4e/mu4e-main
~/Repos/el/mu/build/mu4e/mu4e-view hides ~/Repos/el/mu/mu4e/mu4e-view
~/Repos/el/mu/build/mu4e/mu4e-org hides ~/Repos/el/mu/mu4e/mu4e-org
~/Repos/el/mu/build/mu4e/mu4e-lists hides ~/Repos/el/mu/mu4e/mu4e-lists
~/Repos/el/mu/build/mu4e/mu4e-actions hides ~/Repos/el/mu/mu4e/mu4e-actions
~/Repos/el/mu/build/mu4e/mu4e-helpers hides ~/Repos/el/mu/mu4e/mu4e-helpers
~/Repos/el/mu/build/mu4e/mu4e-search hides ~/Repos/el/mu/mu4e/mu4e-search
~/Repos/el/mu/build/mu4e/mu4e-server hides ~/Repos/el/mu/mu4e/mu4e-server
~/Repos/el/mu/build/mu4e/mu4e-update hides ~/Repos/el/mu/mu4e/mu4e-update
~/Repos/el/mu/build/mu4e/mu4e-context hides ~/Repos/el/mu/mu4e/mu4e-context
~/Repos/el/mu/build/mu4e/mu4e-draft hides ~/Repos/el/mu/mu4e/mu4e-draft
~/Repos/el/mu/build/mu4e/mu4e-bookmarks hides ~/Repos/el/mu/mu4e/mu4e-bookmarks
~/Repos/el/mu/build/mu4e/mu4e-message hides ~/Repos/el/mu/mu4e/mu4e-message
~/Repos/el/mu/build/mu4e/mu4e-compose hides ~/Repos/el/mu/mu4e/mu4e-compose
~/Repos/el/mu/build/mu4e/mu4e-headers hides ~/Repos/el/mu/mu4e/mu4e-headers
~/Repos/el/mu/build/mu4e/mu4e-mark hides ~/Repos/el/mu/mu4e/mu4e-mark
~/Repos/el/mu/build/mu4e/mu4e-contacts hides ~/Repos/el/mu/mu4e/mu4e-contacts
~/Repos/el/mu/build/mu4e/mu4e-icalendar hides ~/Repos/el/mu/mu4e/mu4e-icalendar
~/Repos/el/mu/build/mu4e/mu4e-folders hides ~/Repos/el/mu/mu4e/mu4e-folders
~/Repos/el/mu/build/mu4e/mu4e-speedbar hides ~/Repos/el/mu/mu4e/mu4e-speedbar
~/Repos/el/mu/build/mu4e/mu4e-contrib hides ~/Repos/el/mu/mu4e/mu4e-contrib
~/Repos/el/mu/build/mu4e/mu4e-vars hides ~/Repos/el/mu/mu4e/mu4e-vars
/home/horn/.emacs.d/elpa/transient-20220129.1515/transient hides /home/horn/Repos/el/emacs/lisp/transient

Features:
(shadow emacsbug repeat network-stream url-cache go-translate
gts-engine-deepl gts-engine-google-rpc gts-engine-google gts-engine-bing
gts-implements gts-faces gts-core debug edebug backtrace cl-print
symbol-overlay shortdoc xref conf-mode help-fns radix-tree cursor-sensor
executable dabbrev cape sort gnus-cite mm-archive mail-extr textsec
uni-scripts idna-mapping ucs-normalize uni-confusable textsec-check
expand-region yaml-mode-expansions text-mode-expansions
the-org-mode-expansions web-mode-expansions er-basic-expansions
expand-region-core expand-region-custom misearch multi-isearch
adaptive-wrap org-element ol-eww eww xdg mm-url ol-rmail ol-mhe ol-irc
ol-info ol-gnus nnselect gnus-search eieio-opt speedbar ezimage dframe
ol-docview doc-view ol-bibtex ol-bbdb ol-w3m ol-doi org-link-doi
editorconfig-core editorconfig-core-handle editorconfig-fnmatch project
consult-vertico consult-icomplete consult puni pulse dired-aux
display-fill-column-indicator auto-package-update finder-inf generic
yaml-mode fish-mode cargo cargo-process rust-utils rust-mode
rust-rustfmt rust-playpen rust-compile rust-cargo web-mode disp-table
preview-latex auto-loads tex-site editorconfig elfeed-show elfeed-search
vc-mtn vc-hg vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs debbugs-browse
elfeed-csv elfeed elfeed-curl elfeed-log elfeed-db elfeed-lib avl-tree
generator url-queue xml-query socks elpher hl-todo aggressive-indent
rainbow-mode pdf-occur 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
pdf-isearch pdf-misc pdf-tools pdf-view magit-bookmark bookmark
jka-compr pdf-cache pdf-info tq pdf-util pdf-macs image-mode exif vc-git
vc-dir ewoc epa-file rdictcc diredfl dired-x mu4e-icalendar
gnus-icalendar org-capture org-refile icalendar diary-lib diary-loaddefs
mu4e mu4e-org mu4e-view org ob ob-tangle ob-ref ob-lob ob-table ob-exp
org-macro org-footnote org-src ob-comint org-pcomplete org-list
org-faces org-entities org-version ob-emacs-lisp ob-core ob-eval
org-table oc-basic bibtex ol org-keys oc org-compat org-macs
org-loaddefs find-func cal-menu calendar cal-loaddefs mu4e-main
mu4e-headers mu4e-lists mu4e-compose mu4e-draft mu4e-actions smtpmail
sendmail mu4e-search mu4e-bookmarks mu4e-mark mu4e-message flow-fill
mule-util hl-line mu4e-contacts mu4e-update mu4e-folders mu4e-server
mu4e-context mu4e-vars mu4e-helpers ido mu4e-meta ecomplete
auto-dictionary flyspell ispell tramp-smb which-key
highlight-parentheses restclient kind-icon svg-lib corfu yasnippet
forge-list forge-commands forge-semi forge-bitbucket buck forge-gogs
gogs forge-gitea gtea forge-gitlab glab forge-github ghub-graphql treepy
gsexp ghub let-alist gnutls forge-notify forge-revnote forge-pullreq
forge-issue forge-topic yaml forge-post markdown-mode color thingatpt
noutline outline forge-repo forge forge-core forge-db closql
emacsql-sqlite emacsql emacsql-compiler magit-submodule magit-obsolete
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 magit-repos
magit-apply magit-wip magit-log which-func imenu magit-diff smerge-mode
diff diff-mode git-commit log-edit pcvs-util add-log magit-core
magit-autorevert autorevert filenotify magit-margin magit-transient
magit-process with-editor server magit-mode magit-git magit-section
magit-utils crm dash visual-filename-abbrev rg vc vc-dispatcher
rg-info-hack advice rg-menu transient rg-ibuffer rg-result wgrep-rg
wgrep rg-history rg-header ibuf-ext ibuffer ibuffer-loaddefs grep
compile cus-edit pp cus-load debbugs soap-client url-http url-auth
url-gw nsm warnings rng-xsd rng-dt rng-util xsd-regexp bug-reference
vertico edmacro kmacro marginalia icomplete mb-depth
use-package-diminish ace-window avy alert log4e notifications gntp
elec-pair rx recentf tree-widget pixel-scroll use-package-bind-key
bind-key saveplace savehist smiley gnus-art mm-uu mml2015 mm-view
mml-smime smime dig gnus-sum shr pixel-fill kinsoku svg dom gnus-group
gnus-undo gnus-start gnus-dbus dbus xml gnus-cloud nnimap nnmail
mail-source utf7 netrc nnoo gnus-spec gnus-int gnus-range message
yank-media rmc puny dired dired-loaddefs rfc822 mml mml-sec epa derived
epg rfc6068 epg-config mm-decode mm-bodies mm-encode mail-parse rfc2231
rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils mailheader gnus-win gnus
wid-edit nnheader gnus-util text-property-search mm-util mail-prsvr
mail-utils range doom-themes-ext-org doom-themes-ext-visual-bell
face-remap doom-Iosvkem-theme doom-themes doom-themes-base diminish
cl-extra help-mode use-package-ensure use-package-core tramp
tramp-loaddefs trampver tramp-integration files-x tramp-compat shell
pcomplete comint ansi-color ring parse-time iso8601 time-date ls-lisp
format-spec easy-mmode info 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 auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json map url-vars seq gv subr-x
byte-opt bytecomp byte-compile cconv cl-loaddefs cl-lib iso-transl
tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/pgtk-win pgtk-win term/common-win
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 keymap hashtable-print-readable backquote threads
dbusbind inotify dynamic-setting system-font-setting font-render-setting
cairo move-toolbar gtk x-toolkit pgtk lcms2 multi-tty
make-network-process emacs)

Memory information:
((conses 16 852054 127409)
 (symbols 48 53826 5)
 (strings 32 254312 8041)
 (string-bytes 1 7102601)
 (vectors 16 130308)
 (vector-slots 8 2357843 222007)
 (floats 8 908 1032)
 (intervals 56 8258 801)
 (buffers 992 41))

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Tassilo Horn <tsdh@gnu.org> writes:

> I've also tried with master and --without-pgtk.  There it seems to be
> broken, too, i.e., after evaling the code above, C-g or errors won't
> flash the mode-line (but I can't say anything about the "suddenly the
> error face sticks" issue).

I can confirm that I'm also seeing the bug on master, but not on
emacs-28 (with just ./configure on Debian/bookworm).

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 30 Jan 2022 18:41:36 +0100
> Cc: 53636@debbugs.gnu.org
> 
> Tassilo Horn <tsdh@gnu.org> writes:
> 
> > I've also tried with master and --without-pgtk.  There it seems to be
> > broken, too, i.e., after evaling the code above, C-g or errors won't
> > flash the mode-line (but I can't say anything about the "suddenly the
> > error face sticks" issue).
> 
> I can confirm that I'm also seeing the bug on master, but not on
> emacs-28 (with just ./configure on Debian/bookworm).

If the problem is with face remapping, can we please have a reproducer
that is simpler to debug?  Like one that doesn't need any timers and
flashing text?

TIA

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> If the problem is with face remapping, can we please have a reproducer
> that is simpler to debug?  Like one that doesn't need any timers and
> flashing text?

I assumed that all that async stuff was part of the reproducer, but
indeed it's not.

Just evaling this is sufficient:

 (face-remap-add-relative 'mode-line 'error)

And this is because mode-line is (in Emacs 29) the parent of two faces
(but not used directly on the mode line), so you apparently have to do
this instead:

 (face-remap-add-relative 'mode-line-active 'error)

I'm not sure whether that's a bug or not?  (That remapping a parent face
has no effect.)

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Sun, 30 Jan 2022 19:31:54 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If the problem is with face remapping, can we please have a reproducer
> > that is simpler to debug?  Like one that doesn't need any timers and
> > flashing text?
> 
> I assumed that all that async stuff was part of the reproducer, but
> indeed it's not.
> 
> Just evaling this is sufficient:
> 
>  (face-remap-add-relative 'mode-line 'error)
> 
> And this is because mode-line is (in Emacs 29) the parent of two faces
> (but not used directly on the mode line), so you apparently have to do
> this instead:
> 
>  (face-remap-add-relative 'mode-line-active 'error)
> 
> I'm not sure whether that's a bug or not?  (That remapping a parent face
> has no effect.)

But it does have effect.  Try "C-x 5 b *scratch* RET" after evaluating
the first face-remap-add-relative, and you will see that it does have
effect.  So it's something more subtle...

And yes, I think the extra indirection we now have on master exposed a
problem we never saw before.

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> Date: Sun, 30 Jan 2022 21:19:17 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 53636@debbugs.gnu.org, tsdh@gnu.org
> 
> > I'm not sure whether that's a bug or not?  (That remapping a parent face
> > has no effect.)
> 
> But it does have effect.  Try "C-x 5 b *scratch* RET" after evaluating
> the first face-remap-add-relative, and you will see that it does have
> effect.  So it's something more subtle...
> 
> And yes, I think the extra indirection we now have on master exposed a
> problem we never saw before.

Actually, I think this is a new problem on master, introduced by
making the mode-line face be a parent of mode-line-active and
mode-line-inactive faces.  When that was done, lookup_basic_face was
changed to use mode-line-active instead of mode-line, so now the
mode-line face is not recomputed when face-remapping-alist is changed.
Instead, we recompute mode-line-active and mode-line-inactive, but
those are not in face-remapping-alist in this scenario, so they don't
change on the existing frame.

Do we still need the mode-line as a parent of these two faces, or can
we go back to what we had before the variable-pitch mode-line changes?

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Actually, I think this is a new problem on master, introduced by
> making the mode-line face be a parent of mode-line-active and
> mode-line-inactive faces.  When that was done, lookup_basic_face was
> changed to use mode-line-active instead of mode-line, so now the
> mode-line face is not recomputed when face-remapping-alist is changed.
> Instead, we recompute mode-line-active and mode-line-inactive, but
> those are not in face-remapping-alist in this scenario, so they don't
> change on the existing frame.

Ah, right.  So we should go back to also recomputing mode-line, I guess...

> Do we still need the mode-line as a parent of these two faces, or can
> we go back to what we had before the variable-pitch mode-line changes?

The plan is to reintroduce the variable-pitch stuff after we've ironed
out the width issues, so I'd rather keep the faces as is.  (And I think
it makes more sense conceptually, since `mode-line' is also used as the
parent of other faces, like `header-line'.)

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Sun, 30 Jan 2022 21:28:53 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Do we still need the mode-line as a parent of these two faces, or can
> > we go back to what we had before the variable-pitch mode-line changes?
> 
> The plan is to reintroduce the variable-pitch stuff after we've ironed
> out the width issues, so I'd rather keep the faces as is.  (And I think
> it makes more sense conceptually, since `mode-line' is also used as the
> parent of other faces, like `header-line'.)

OK, I will see what I can do.

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> Date: Mon, 31 Jan 2022 14:20:01 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 53636@debbugs.gnu.org, tsdh@gnu.org
> 
> > From: Lars Ingebrigtsen <larsi@gnus.org>
> > Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> > Date: Sun, 30 Jan 2022 21:28:53 +0100
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > Do we still need the mode-line as a parent of these two faces, or can
> > > we go back to what we had before the variable-pitch mode-line changes?
> > 
> > The plan is to reintroduce the variable-pitch stuff after we've ironed
> > out the width issues, so I'd rather keep the faces as is.  (And I think
> > it makes more sense conceptually, since `mode-line' is also used as the
> > parent of other faces, like `header-line'.)
> 
> OK, I will see what I can do.

Basically, the problem is that mode-line is a well-known face name,
and so it can be expected that user customizations and Lisp packages
rely on the fact that remapping or otherwise tweaking that face
directly affects the display of the mode line of the selected window.

So when we effectively renamed mode-line to mode-line-active, we broke
compatibility, since in some situations, such as the one described
here, what was previously done with the mode-line face must now be
done with mode-line-active face.

Is that analysis correct, or am I missing something?

If so, my suggestion is:

  . rename mode-line-active back to mode-line
  . introduce a new face, mode-line-base, say, from which mode-line
    will inherit, like mode-line-active now inherits from mode-line

Then all the tricks people play with the mode-line face will work
again, and we can warn not to rely on remapping mode-line-base to
automatically affect the mode lines of the currently selected frame.
Since this is a new face, we don't need to worry about code out there
which already remaps that face.  But making this new mode-line-base
face use variable-pitch, for example, will still produce the same
effect that we had with mode-line a few weeks before.

Would that be an acceptable solution?  (I considered alternatives, but
they are all uglier.  Basically, the "basic faces" aren't supposed to
inherit from other faces, for face remapping to work as expected.)

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> So when we effectively renamed mode-line to mode-line-active, we broke
> compatibility, since in some situations, such as the one described
> here, what was previously done with the mode-line face must now be
> done with mode-line-active face.

But this is a bug, I think?  You can see the same effect in Emacs 28 if
you do:

(face-remap-add-relative 'mode-line 'link-visited)

This won't have the expected effect on faces that inherit from mode-line
(line mode-line-inactive/header-line) in the current frame, but if you
open a new frame, it'll have an effect there.

> Is that analysis correct, or am I missing something?
>
> If so, my suggestion is:
>
>   . rename mode-line-active back to mode-line
>   . introduce a new face, mode-line-base, say, from which mode-line
>     will inherit, like mode-line-active now inherits from mode-line

The new mode-line-active face was supposed to fix the decoupling of
mode-line and header-line.  In Emacs 28, there's no way to change the
look of the (active) mode line without also changing the header-line
face.  Introducing a new face that mode-line inherits from doesn't solve
this issue.

> Would that be an acceptable solution?  (I considered alternatives, but
> they are all uglier.  Basically, the "basic faces" aren't supposed to
> inherit from other faces, for face remapping to work as expected.)

Making face remapping work reliably for these faces would be better, but
I haven't looked at the code.

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Tue, 01 Feb 2022 20:38:53 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So when we effectively renamed mode-line to mode-line-active, we broke
> > compatibility, since in some situations, such as the one described
> > here, what was previously done with the mode-line face must now be
> > done with mode-line-active face.
> 
> But this is a bug, I think?  You can see the same effect in Emacs 28 if
> you do:
> 
> (face-remap-add-relative 'mode-line 'link-visited)

What do you suggest instead? to treat some basic faces specially
because we happen to know that they inherit from other basic faces?
That's ugly and fragile.

If we want to solve this cleanly and radically, I'd prefer to break
the vicious circle by removing the inheritance from basic faces
altogether.  The basic faces are just that: they aren't supposed to
inherit from other basic faces.  That would also decouple header-line
and everything else as a side effect.

> Making face remapping work reliably for these faces would be better, but
> I haven't looked at the code.

Maybe you will see something I don't, but in general face remapping
isn't magical, we must explicitly account for it where it might
matter, or it will not work.  After all, all face remapping does is
add some members to a list, it doesn't actually change any faces.  So
we need to do stuff like this:

  if (! NILP (Vface_remapping_alist))
    remapped_base_face_id
      = lookup_basic_face (w, XFRAME (w->frame), base_face_id);

That's how face-remapping-alist takes its effect.  If you want this to
work for inherited faces, you must do this for the parent face,
recursively, before you do it for the face in which you are
interested.  You want to add the inheritance-chasing loop to basic
face look up?  I don't.

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> If we want to solve this cleanly and radically, I'd prefer to break
> the vicious circle by removing the inheritance from basic faces
> altogether.  The basic faces are just that: they aren't supposed to
> inherit from other basic faces.  That would also decouple header-line
> and everything else as a side effect.

OK, you want to change the header-line and mode-line-inactive faces so
that they no longer inherit from the mode-line face?

But they aren't the only basic faces that have this problem --
`tab-line' inherits from `variable-pitch', for instance.

> That's how face-remapping-alist takes its effect.  If you want this to
> work for inherited faces, you must do this for the parent face,
> recursively, before you do it for the face in which you are
> interested.  You want to add the inheritance-chasing loop to basic
> face look up?  I don't.

No, I don't.  I was thinking of altering `face-remap-add-relative' so
that it resolves the faces that inherit instead of leaving it to
redisplay.

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Wed, 02 Feb 2022 18:59:49 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If we want to solve this cleanly and radically, I'd prefer to break
> > the vicious circle by removing the inheritance from basic faces
> > altogether.  The basic faces are just that: they aren't supposed to
> > inherit from other basic faces.  That would also decouple header-line
> > and everything else as a side effect.
> 
> OK, you want to change the header-line and mode-line-inactive faces so
> that they no longer inherit from the mode-line face?

Yes, I think this is the best way forward.  Though it will be somewhat
backward-incompatible, if someone customizes mode-line and expects
header-line to follow suit.

> But they aren't the only basic faces that have this problem --
> `tab-line' inherits from `variable-pitch', for instance.

We should "un-inherit" all of the basic faces.

> > That's how face-remapping-alist takes its effect.  If you want this to
> > work for inherited faces, you must do this for the parent face,
> > recursively, before you do it for the face in which you are
> > interested.  You want to add the inheritance-chasing loop to basic
> > face look up?  I don't.
> 
> No, I don't.  I was thinking of altering `face-remap-add-relative' so
> that it resolves the faces that inherit instead of leaving it to
> redisplay.

I don't think this can work, because that would mean the original face
will be affected by remapping, won't it?  Face remapping is
buffer-local, whereas faces are defined for the entire frame.  So we
cannot affect the original face and keep the effect buffer-local.  And
if we produce a different face symbol and modify it instead, then
references to the original face symbol will not be redirected to the
remapped face.

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> OK, you want to change the header-line and mode-line-inactive faces so
>> that they no longer inherit from the mode-line face?
>
> Yes, I think this is the best way forward.  Though it will be somewhat
> backward-incompatible, if someone customizes mode-line and expects
> header-line to follow suit.

I think this would break a lot more than the current situation does.
Lots of people expect these faces to inherit the way they do, and have
set up stuff based on that.  Programmatic remapping of the `mode-line'
face, on the other hand, is a much smaller issue, and we can just say
"use `mode-line-active' instead".

>> No, I don't.  I was thinking of altering `face-remap-add-relative' so
>> that it resolves the faces that inherit instead of leaving it to
>> redisplay.
>
> I don't think this can work, because that would mean the original face
> will be affected by remapping, won't it?  Face remapping is
> buffer-local, whereas faces are defined for the entire frame.  So we
> cannot affect the original face and keep the effect buffer-local.  And
> if we produce a different face symbol and modify it instead, then
> references to the original face symbol will not be redirected to the
> remapped face.

Hm...   That's not quite what I'm seeing with the mode line faces.

In Emacs 28, with

emacs -Q
(face-remap-add-relative 'mode-line 'link-visited)
C-x 5 2


[-- Attachment #2: Type: image/png, Size: 16013 bytes --]

[-- Attachment #3: Type: text/plain, Size: 642 bytes --]


Note that in the other frame (displaying *Messages*), the
mode-line-inactive face has inherited the underline from line-visited.
(If I select that window, then the mode-line face has not, that is just
in the current buffer.)  So remapping a face that has inherited faces
leads to side effects in other places...

Anyway, what I was thinking of is a really simple solution: Have
`face-remap-add-relative' loop over all children and remap them, too.
(I haven't actually attempted to write something like that, though.  😀)

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

bug#53636: [External] : bug#53636: 29.0.50; face-remapping broken on master

[Drew Adams]

 
> >> OK, you want to change the header-line and mode-line-inactive faces so
> >> that they no longer inherit from the mode-line face?
> >
> > Yes, I think this is the best way forward.  Though it will be somewhat
> > backward-incompatible, if someone customizes mode-line and expects
> > header-line to follow suit.
> 
> I think this would break a lot more than the current situation does.
> Lots of people expect these faces to inherit the way they do, and have
> set up stuff based on that.

FWIW, I think folks should _not_ depend on any
such predefined inheritances.  Emacs shouldn't
encourage it and should instead warn against it.

(I'm not a fan of such predefined inheritance.)

> Programmatic remapping of the `mode-line'
> face, on the other hand, is a much smaller issue,

Why do you think it's a much smaller issue?

> we can just say "use `mode-line-active' instead".

You can "just say" anything you like.  This bug
report is the kind of thing you'll see, as a
starter.

FWIW, I'm not in favor of changing what used to
be face `mode-line' to face `mode-line-active'.
I don't think that should have been necessary.

Let's see why it came to this...

You said earlier:

 > The new mode-line-active face was supposed to
 > fix the decoupling of mode-line and header-line.

The answer for how to decouple those should have
been for header-line to _not_ inherit mode-line.
Never should have had that inheritance in the
first place, IMO.

Face inheritance is over(ab)used.  Shouldn't be
done by `emacs -Q` itself.

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Wed, 02 Feb 2022 20:48:42 +0100
> 
> >> OK, you want to change the header-line and mode-line-inactive faces so
> >> that they no longer inherit from the mode-line face?
> >
> > Yes, I think this is the best way forward.  Though it will be somewhat
> > backward-incompatible, if someone customizes mode-line and expects
> > header-line to follow suit.
> 
> I think this would break a lot more than the current situation does.

Yes, it will.  But if we want to break the link between mode-line and
header-line, I see no better way.

> Lots of people expect these faces to inherit the way they do, and have
> set up stuff based on that.  Programmatic remapping of the `mode-line'
> face, on the other hand, is a much smaller issue, and we can just say
> "use `mode-line-active' instead".

That'd be fine with meas well,if it's acceptable.

> Note that in the other frame (displaying *Messages*), the
> mode-line-inactive face has inherited the underline from line-visited.
> (If I select that window, then the mode-line face has not, that is just
> in the current buffer.)  So remapping a face that has inherited faces
> leads to side effects in other places...

When you remap a face, the faces that inherit from it are affected
only on new frames, because when we create a frame, we recompute the
set of the basic faces for it from scratch and thoroughly.  That's
exactly the other side of the issue which started this discussion: the
inheriting faces on the original frame aren't affected by the
remapping of the parent face, but those same faces on new frames are.

> Anyway, what I was thinking of is a really simple solution: Have
> `face-remap-add-relative' loop over all children and remap them, too.
> (I haven't actually attempted to write something like that, though.  😀)

Why not leave this to the (small number of) applications that want to
do this?

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> When you remap a face, the faces that inherit from it are affected
> only on new frames, because when we create a frame, we recompute the
> set of the basic faces for it from scratch and thoroughly.  That's
> exactly the other side of the issue which started this discussion: the
> inheriting faces on the original frame aren't affected by the
> remapping of the parent face, but those same faces on new frames are.

Right.  There's two issues here, though, and I think both of them should
be fixed (but I don't know how difficult it would be).

1) `face-map-add-relative' is supposed to be buffer-local.  But the
example shows that faces that inherit from remapped faces are affected
in other (new) frames.  (The remapped faces themselves are not
affected.)

2) Remapping these base faces behave differently from other faces, and
it'd be nice to fix that.

>> Anyway, what I was thinking of is a really simple solution: Have
>> `face-remap-add-relative' loop over all children and remap them, too.
>> (I haven't actually attempted to write something like that, though.  😀)
>
> Why not leave this to the (small number of) applications that want to
> do this?

My feeling is that all users of face remapping would want this.

Eli Zaretskii <eliz@gnu.org> writes:

>> Anyway, what I was thinking of is a really simple solution: Have
>> `face-remap-add-relative' loop over all children and remap them, too.
>
> That'd mean looping over all the known faces for each change in
> face-remapping-alist, wouldn't it?  Because we don't track which faces
> inherit from a given face, we can only tell the reverse: from which
> faces a given face inherits.

Yes, but the number of faces is pretty small, so doing this looping in
`face-map-add-relative' would not impose a significant penalty overall.

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Thu, 03 Feb 2022 20:24:03 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Anyway, what I was thinking of is a really simple solution: Have
> >> `face-remap-add-relative' loop over all children and remap them, too.
> >
> > That'd mean looping over all the known faces for each change in
> > face-remapping-alist, wouldn't it?  Because we don't track which faces
> > inherit from a given face, we can only tell the reverse: from which
> > faces a given face inherits.
> 
> Yes, but the number of faces is pretty small

No, it isn't.  We had examples with hundreds of faces, back when we
decided to store faces in a hash-table.

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> No, it isn't.  We had examples with hundreds of faces, back when we
> decided to store faces in a hash-table.

Sorry, I don't follow -- examples of what that had hundreds of faces?

I'm saying that finding the faces that inherit from a specific face is
not prohibitively expensive -- iterating over all the symbols and
finding that info would be pretty fast.  If it's not, then extending
defface to keep track of this information would be simple enough, too.

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Sat, 05 Feb 2022 07:20:57 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > No, it isn't.  We had examples with hundreds of faces, back when we
> > decided to store faces in a hash-table.
> 
> Sorry, I don't follow -- examples of what that had hundreds of faces?

People reported they have hundreds of faces defined in their Emacs
sessions.  See this message, for example:

  https://lists.gnu.org/archive/html/emacs-devel/2021-05/msg01031.html

It reports to have 600 to 800 faces defined (with 129 in "emacs -Q").

> I'm saying that finding the faces that inherit from a specific face is
> not prohibitively expensive -- iterating over all the symbols and
> finding that info would be pretty fast.

You'd need to loop through all those hundreds of faces for each change
in face-remapping-alist.  Imagine what this will do to "C-x C-=" and
its ilk.

> If it's not, then extending defface to keep track of this
> information would be simple enough, too.

Still a major complication, since inheritance is recursive.

All that just to have one face that mode-line-* and header-line
etc. can inherit from?  Doesn't this look like a tail wagging the dog?

bug#53636: [External] : bug#53636: 29.0.50; face-remapping broken on master

[Drew Adams]

> > > No, it isn't.  We had examples with hundreds of faces, back when we
> > > decided to store faces in a hash-table.
> >
> > Sorry, I don't follow -- examples of what that had hundreds of faces?
> 
> People reported they have hundreds of faces defined in their Emacs
> sessions.  See this message, for example:
> 
>   https://urldefense.com/v3/__https://lists.gnu.org/archive/html/emacs-
> devel/2021-
> 05/msg01031.html__;!!ACWV5N9M2RV99hQ!eDGeVB_SQ0DAF1rV2RiFjVAIlEJnGK9tdVR-
> 2Epb6b7dlLp0UTSPD7Lm6l0_-q5Y$
> 
> It reports to have 600 to 800 faces defined (with 129 in "emacs -Q").

Yes.  FWIW, I have 475.

> All that just to have one face that mode-line-* and header-line
> etc. can inherit from?  Doesn't this look like a tail wagging the dog?

Right.  IMO, no such inheritance should be predefined.

If any user ever _wants_ a particular face to inherit
from another face she can easily do so.  Emacs should
not be predefining any such couplings.

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Sorry, I don't follow -- examples of what that had hundreds of faces?
>
> People reported they have hundreds of faces defined in their Emacs
> sessions.  See this message, for example:
>
>   https://lists.gnu.org/archive/html/emacs-devel/2021-05/msg01031.html
>
> It reports to have 600 to 800 faces defined (with 129 in "emacs -Q").

But people aren't remapping all those faces, so the number of faces in
total doesn't seem to matter that much here.  (And looping over 800
faces to pick out dependencies is probably not very expensive.)

> You'd need to loop through all those hundreds of faces for each change
> in face-remapping-alist.  Imagine what this will do to "C-x C-=" and
> its ilk.

It would have no impact on those ilks, because the looping will be done
in `face-remap-add-relative' and nowhere else?

>> If it's not, then extending defface to keep track of this
>> information would be simple enough, too.
>
> Still a major complication, since inheritance is recursive.
>
> All that just to have one face that mode-line-* and header-line
> etc. can inherit from?  Doesn't this look like a tail wagging the dog?

Well, fixing bugs is nice anyway.  :-)

So here's a reproducer for the first problem: These two forms, both from
"emacs -Q", have different effects on the new frame:

(progn
  (face-remap-add-relative 'mode-line 'link-visited)
  (make-frame))

vs

(progn
  (face-remap-add-relative 'mode-line 'link-visited)
  (switch-to-buffer "*Messages*")
  (make-frame))

In the first case, the new frame will have mode-line remapped to
link-visited in all windows, while in the latter it won't.  So it looks
like a pretty simple bug -- `make-frame' (when computing the faces for
the new frame) is using the buffer-local value of
`face-remapping-alist' instead of the global one.

But after poking at the code for a couple minutes, I'm not sure where
the computation for faces is done for new faces.  Hm...  is it
`face-spec-recalc'?  Hm...  but that doesn't access
`face-remapping-alist'...   Is this done at a lower level?

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Sat, 05 Feb 2022 23:27:36 +0100
> 
> (progn
>   (face-remap-add-relative 'mode-line 'link-visited)
>   (make-frame))
> 
> vs
> 
> (progn
>   (face-remap-add-relative 'mode-line 'link-visited)
>   (switch-to-buffer "*Messages*")
>   (make-frame))
> 
> In the first case, the new frame will have mode-line remapped to
> link-visited in all windows, while in the latter it won't.  So it looks
> like a pretty simple bug -- `make-frame' (when computing the faces for
> the new frame) is using the buffer-local value of
> `face-remapping-alist' instead of the global one.

But which case is considered a bug?  In the first case, the mode line
of *scratch* is affected in the second frame; in the second case the
remapping doesn't show in that buffer on _any_ frame, although
face-remapping-alist is updated.

> But after poking at the code for a couple minutes, I'm not sure where
> the computation for faces is done for new faces.  Hm...  is it
> `face-spec-recalc'?  Hm...  but that doesn't access
> `face-remapping-alist'...   Is this done at a lower level?

What is "this" and "the computation" which you are asking about here?

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> But which case is considered a bug?  In the first case, the mode line
> of *scratch* is affected in the second frame; in the second case the
> remapping doesn't show in that buffer on _any_ frame, although
> face-remapping-alist is updated.

The bug I'm talking about here is that `face-remap-add-relative' is
supposed to be buffer-local, but calling it, and then creating a new
frame (with point in that buffer) will have the remapping take place in
all buffer in the new frame.

>> But after poking at the code for a couple minutes, I'm not sure where
>> the computation for faces is done for new faces.  Hm...  is it
>> `face-spec-recalc'?  Hm...  but that doesn't access
>> `face-remapping-alist'...   Is this done at a lower level?
>
> What is "this" and "the computation" which you are asking about here?

The computation of the faces for the new frame.

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Mon, 07 Feb 2022 00:16:08 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But which case is considered a bug?  In the first case, the mode line
> > of *scratch* is affected in the second frame; in the second case the
> > remapping doesn't show in that buffer on _any_ frame, although
> > face-remapping-alist is updated.
> 
> The bug I'm talking about here is that `face-remap-add-relative' is
> supposed to be buffer-local, but calling it, and then creating a new
> frame (with point in that buffer) will have the remapping take place in
> all buffer in the new frame.

And what happens when you call make-frame from another buffer is not a
bug?  Is face-remapping-alist supposed to affect the buffer in which
it is set on all frames or just on the single frame where
face-remap-add-relative was called?

> >> But after poking at the code for a couple minutes, I'm not sure where
> >> the computation for faces is done for new faces.  Hm...  is it
> >> `face-spec-recalc'?  Hm...  but that doesn't access
> >> `face-remapping-alist'...   Is this done at a lower level?
> >
> > What is "this" and "the computation" which you are asking about here?
> 
> The computation of the faces for the new frame.

You mean init_frame_faces, which calls realize_basic_faces?  Or do you
mean some other kind of "face computation"?

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> And what happens when you call make-frame from another buffer is not a
> bug?  Is face-remapping-alist supposed to affect the buffer in which
> it is set on all frames or just on the single frame where
> face-remap-add-relative was called?

I'm not sure.  But the current behaviour (affecting all buffers on the
new frame) has to be a bug.

>> The computation of the faces for the new frame.
>
> You mean init_frame_faces, which calls realize_basic_faces?  Or do you
> mean some other kind of "face computation"?

I don't know?  I'm looking for the code that leads to the behaviour I
described.

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

bug#53636: 29.0.50; face-remapping broken on master

[martin rudalics]

 >> And what happens when you call make-frame from another buffer is not a
 >> bug?  Is face-remapping-alist supposed to affect the buffer in which
 >> it is set on all frames or just on the single frame where
 >> face-remap-add-relative was called?
 >
 > I'm not sure.  But the current behaviour (affecting all buffers on the
 > new frame) has to be a bug.

Face remapping has never been properly synchronized with windows and
frames.  What's needed is a simple hierarchy window > window's buffer >
window's frame where the first should be implemented with the help of a
'face-remapping' window parameter and the last with the help of a
'face-remapping' frame parameter.  Just like we (should) do for things
like the scroll bars or fringes.  But we need a consensus on this first.

martin

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> Date: Tue, 8 Feb 2022 09:58:31 +0100
> Cc: 53636@debbugs.gnu.org, tsdh@gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  >> And what happens when you call make-frame from another buffer is not a
>  >> bug?  Is face-remapping-alist supposed to affect the buffer in which
>  >> it is set on all frames or just on the single frame where
>  >> face-remap-add-relative was called?
>  >
>  > I'm not sure.  But the current behaviour (affecting all buffers on the
>  > new frame) has to be a bug.
> 
> Face remapping has never been properly synchronized with windows and
> frames.  What's needed is a simple hierarchy window > window's buffer >
> window's frame where the first should be implemented with the help of a
> 'face-remapping' window parameter and the last with the help of a
> 'face-remapping' frame parameter.  Just like we (should) do for things
> like the scroll bars or fringes.  But we need a consensus on this first.

I don't think I understand this remark.  While what you describe might
be a useful addition, I don't see why we must have it, or else.
Surely, Emacs has gobs of features that depend only on the buffer, but
not on the window nor the frame where that buffer is displayed?  Why
cannot we have a reasonable behavior with face-remapping-alist being
specific to a buffer, no matter in what window we display it?

bug#53636: 29.0.50; face-remapping broken on master

[martin rudalics]

 >> Face remapping has never been properly synchronized with windows and
 >> frames.  What's needed is a simple hierarchy window > window's buffer >
 >> window's frame where the first should be implemented with the help of a
 >> 'face-remapping' window parameter and the last with the help of a
 >> 'face-remapping' frame parameter.  Just like we (should) do for things
 >> like the scroll bars or fringes.  But we need a consensus on this first.
 >
 > I don't think I understand this remark.  While what you describe might
 > be a useful addition, I don't see why we must have it, or else.
 > Surely, Emacs has gobs of features that depend only on the buffer, but
 > not on the window nor the frame where that buffer is displayed?  Why
 > cannot we have a reasonable behavior with face-remapping-alist being
 > specific to a buffer, no matter in what window we display it?

What makes you think that this is not part of my proposal?  What I mean
is to have a clear rule how, for example, a buffer local setting of
'face-remapping-alist' may affect the creation of a new frame and the
display of buffers in it.

martin

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> Date: Tue, 8 Feb 2022 19:24:19 +0100
> Cc: larsi@gnus.org, 53636@debbugs.gnu.org, tsdh@gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  >> Face remapping has never been properly synchronized with windows and
>  >> frames.  What's needed is a simple hierarchy window > window's buffer >
>  >> window's frame where the first should be implemented with the help of a
>  >> 'face-remapping' window parameter and the last with the help of a
>  >> 'face-remapping' frame parameter.  Just like we (should) do for things
>  >> like the scroll bars or fringes.  But we need a consensus on this first.
>  >
>  > I don't think I understand this remark.  While what you describe might
>  > be a useful addition, I don't see why we must have it, or else.
>  > Surely, Emacs has gobs of features that depend only on the buffer, but
>  > not on the window nor the frame where that buffer is displayed?  Why
>  > cannot we have a reasonable behavior with face-remapping-alist being
>  > specific to a buffer, no matter in what window we display it?
> 
> What makes you think that this is not part of my proposal?  What I mean
> is to have a clear rule how, for example, a buffer local setting of
> 'face-remapping-alist' may affect the creation of a new frame and the
> display of buffers in it.

OK, then we need first to agree on what is the reasonable expected
behavior in such cases.

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Tue, 08 Feb 2022 07:08:29 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > And what happens when you call make-frame from another buffer is not a
> > bug?  Is face-remapping-alist supposed to affect the buffer in which
> > it is set on all frames or just on the single frame where
> > face-remap-add-relative was called?
> 
> I'm not sure.  But the current behaviour (affecting all buffers on the
> new frame) has to be a bug.
> 
> >> The computation of the faces for the new frame.
> >
> > You mean init_frame_faces, which calls realize_basic_faces?  Or do you
> > mean some other kind of "face computation"?
> 
> I don't know?  I'm looking for the code that leads to the behaviour I
> described.

We need to agree on what "this behavior" is.  Or at least I need to
understand what you mean by that, in order to be able to provide
information that could help you.

So: in the scenario you've shown, do we want *scratch* to have its
mode-line remapped on the new frame, or don't we?  IOW, I agree that
the result ideally shouldn't depend on the buffer where make-frame is
called, but I need to know what is the desired behavior in order to
find code which produces the undesired results.

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> So: in the scenario you've shown, do we want *scratch* to have its
> mode-line remapped on the new frame, or don't we?  IOW, I agree that
> the result ideally shouldn't depend on the buffer where make-frame is
> called, but I need to know what is the desired behavior in order to
> find code which produces the undesired results.

Yes, I don't think that the results in the new frame should depend on
the current buffer when `make-frame' is called.

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Wed, 09 Feb 2022 09:01:27 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So: in the scenario you've shown, do we want *scratch* to have its
> > mode-line remapped on the new frame, or don't we?  IOW, I agree that
> > the result ideally shouldn't depend on the buffer where make-frame is
> > called, but I need to know what is the desired behavior in order to
> > find code which produces the undesired results.
> 
> Yes, I don't think that the results in the new frame should depend on
> the current buffer when `make-frame' is called.

OK, I think I know where this happens.  Let me dig a bit.

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> Date: Wed, 09 Feb 2022 15:57:14 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 53636@debbugs.gnu.org, tsdh@gnu.org
> 
> > From: Lars Ingebrigtsen <larsi@gnus.org>
> > Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> > Date: Wed, 09 Feb 2022 09:01:27 +0100
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > So: in the scenario you've shown, do we want *scratch* to have its
> > > mode-line remapped on the new frame, or don't we?  IOW, I agree that
> > > the result ideally shouldn't depend on the buffer where make-frame is
> > > called, but I need to know what is the desired behavior in order to
> > > find code which produces the undesired results.
> > 
> > Yes, I don't think that the results in the new frame should depend on
> > the current buffer when `make-frame' is called.
> 
> OK, I think I know where this happens.  Let me dig a bit.

After digging into this, I'm sorry to say that this is unworkable: if
a basic face inherits from some other face, and that other face is
remapped via a buffer-local value of face-remapping-alist, the result
will be unpredictable.  For a simple demonstration of the
unpredictable nature of the result, try this simple variation of your
recipe, which just adds the last step:

  . emacs -Q
  . M-: (progn (face-remap-add-relative 'mode-line 'link-visited) (make-frame)) RET
  . C-x C-f src/xdisp.c RET

Now all the buffers on the new frame have the mode line rendered with
the default attributes, whereas all the buffers on the old frame
suddenly get the mode line rendered with the link-visited face.  IOW,
just visiting a file on the new frame causes the mode line to be
displayed differently than it was before you visit that file.

This is due to how we handle the basic faces.  There's "face
realization" and "face look up".  The former means we recompute all
the face's attributes and load the necessary GUI resources (colors,
fonts, etc.) from scratch; the latter means we just look up in the
frame's face cache the attributes that were already computed.  Face
realization is obviously much more expensive than look up, so we only
realize the basic faces when the frame's face cache is empty.  A new
frame has its cache empty, but there are other situations when the
face cache could become empty: for example, when attributes of a named
face change.  So fundamentally, whether using a basic face will go
through "face realization" or just "face look up" is unpredictable and
can happen at any moment.

Here is the crucial point: face inheritance is only considered in
"face realization", not in "face look up".  (That is why we empty the
face cache when a named face is modified in the first place.)  So if a
basic face inherits from another face, and that other face is
remapped, the effect of the remapping will make the result of "face
realization" different from "face look up", although no face has
changed the attributes.  And since it is unpredictable when Emacs will
use "face realization" and when it will settle with "face look up",
you get unpredictable results.  Moreover, Emacs expects a remapped
face to have a different face ID internally (the ID is just the index
of the remapped face in the frame's face cache).  But "face
realization" when a parent face is remapped doesn't yield a new face
ID, it just influences the attributes recorded under the original face
ID.  The bottom line is that if "face realization" is called when the
current buffer happens to be one with non-nil face-remapping-alist,
the default face ID will use the attributes of the remapped face, but
Emacs will not know that the face was effectively remapped.  And that
is the root cause of what you see in your recipes.

Contrast this with a perfectly correct and expected behavior with
these slightly modified recipes:

  (progn
    (face-remap-add-relative 'mode-line-active 'link-visited)
    (make-frame))

  (progn
    (face-remap-add-relative 'mode-line-active 'link-visited)
    (switch-to-buffer "*Messages*")
    (make-frame))

Now the only mode line that is affected is the one of the *scratch*
buffer (when it is the current buffer), no matter how many other files
you visit after creating the new frame, and no matter which frame
displays what buffer.

So I can just reiterate what I said up-thread: basic faces cannot
inherit from other faces, if we want to support remapping of those
parent faces.  Our internal infrastructure for handling the basic
faces simply doesn't support this.

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> So I can just reiterate what I said up-thread: basic faces cannot
> inherit from other faces, if we want to support remapping of those
> parent faces.  Our internal infrastructure for handling the basic
> faces simply doesn't support this.

Thanks for the analysis here -- it indeed seems like a much more complex
problem than I'd assumed.

I guess we should just document that face remapping doesn't work
reliably when remapping parent faces to the basic faces?

And for this bug report, the answer is "you have to use mode-line-active
instead of mode-line".

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

bug#53636: 29.0.50; face-remapping broken on master

[Tassilo Horn]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> And for this bug report, the answer is "you have to use
> mode-line-active instead of mode-line".

If that's the case, I'd submit a PR to the doom-themes package where
I've encountered the problem, i.e., use `mode-line-active' instead of
`mode-line' if `(facep 'mode-line-active)'.

Bye,
Tassilo

bug#53636: 29.0.50; face-remapping broken on master

[Tassilo Horn]

Tassilo Horn <tsdh@gnu.org> writes:

>> And for this bug report, the answer is "you have to use
>> mode-line-active instead of mode-line".
>
> If that's the case, I'd submit a PR to the doom-themes package where
> I've encountered the problem, i.e., use `mode-line-active' instead of
> `mode-line' if `(facep 'mode-line-active)'.

Done in https://github.com/doomemacs/themes/pull/705 which has already
been merged.

Bye,
Tassilo

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Sun, 13 Feb 2022 09:20:24 +0100
> 
> I guess we should just document that face remapping doesn't work
> reliably when remapping parent faces to the basic faces?

Yes, I think so.

> And for this bug report, the answer is "you have to use mode-line-active
> instead of mode-line".

Right.

bug#53636: 29.0.50; face-remapping broken on master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I guess we should just document that face remapping doesn't work
>> reliably when remapping parent faces to the basic faces?
>
> Yes, I think so.
>
>> And for this bug report, the answer is "you have to use mode-line-active
>> instead of mode-line".
>
> Right.

Now done, and I added a section in the "incompatible lisp changes" to
the NEWS file.

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

bug#53636: 29.0.50; face-remapping broken on master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 53636@debbugs.gnu.org,  tsdh@gnu.org
> Date: Wed, 02 Feb 2022 20:48:42 +0100
> 
> Anyway, what I was thinking of is a really simple solution: Have
> `face-remap-add-relative' loop over all children and remap them, too.

That'd mean looping over all the known faces for each change in
face-remapping-alist, wouldn't it?  Because we don't track which faces
inherit from a given face, we can only tell the reverse: from which
faces a given face inherits.