bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[trevor.m.murphy]

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

Tags: patch

Got a quick feature request.

Over the years I've put more and more information into the header line, and at some point I wanted to get that quick visual indicator of selected vs non-selected windows when I glanced at the header as I already got from the mode line.

This was surprisingly hard to achieve!  For a few years I used an ugly `buffer-list-update-hook` hack.  Then a few months ago I got adventurous and found that it actually wasn't as hard as I'd feared to hack these into the C source.

The new faces both inherit from the current `header-line` face, so there shouldn't be any visible impact if users don't customize the new faces.  For cases like mine it's easy to customize header-line-inactive to inherit from mode-line-inactive.

Thanks in advance for your attention and feedback,

Trevor


In GNU Emacs 29.4 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.43,
cairo version 1.18.0)
Repository revision: a92669f7ae7de3172cf883d15af9d7bb9bda7dd9
Repository branch: main
System Description: Arch Linux

Configured using:
 'configure --with-pgtk --with-native-compilation=aot --sysconfdir=/etc
 --prefix=/usr --libexecdir=/usr/lib --with-tree-sitter
 --localstatedir=/var --with-cairo --disable-build-details
 --with-harfbuzz --with-libsystemd --with-modules 'CFLAGS=-march=x86-64
 -mtune=generic -O2 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=3
 -Wformat -Werror=format-security -fstack-clash-protection
 -fcf-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -g
 -ffile-prefix-map=/home/trevor/pkgbuilds/abs/emacs/src=/usr/src/debug/emacs
 -flto=auto' 'LDFLAGS=-Wl,-O1 -Wl,--sort-common -Wl,--as-needed
 -Wl,-z,relro -Wl,-z,now -Wl,-z,pack-relative-relocs -flto=auto'
 'CXXFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions
 -Wp,-D_FORTIFY_SOURCE=3 -Wformat -Werror=format-security
 -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer
 -mno-omit-leaf-frame-pointer -Wp,-D_GLIBCXX_ASSERTIONS -g
 -ffile-prefix-map=/home/trevor/pkgbuilds/abs/emacs/src=/usr/src/debug/emacs
 -flto=auto''


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-new-header-line-active-and-header-line-inactive-.patch --]
[-- Type: text/patch, Size: 12768 bytes --]

From 1000a0bac791dbe31da627389db9755fddf51243 Mon Sep 17 00:00:00 2001
From: Trevor Murphy <trevor.m.murphy@gmail.com>
Date: Thu, 17 Oct 2024 15:51:14 -0700
Subject: [PATCH] Add new `header-line-active` and `header-line-inactive`
 faces.

This is all intended to parallel the mode-line-active and
mode-line-inactive distinction.

* src/dispextern.h (CURRENT_HEADER_LINE_ACTIVE_FACE_ID_3,
CURRENT_HEADER_LINE_ACTIVE_FACE_ID): new macros based on mode line
equivalents
(face_id): new face IDs
* src/xdisp.c (window_box_height, pos_visible_p, init_iterator,
window_text_pixel_size, display_mode_lines, display_mode_line,
format-mode-line): replace all uses of HEADER_LINE_FACE_ID with either
a new macro or the new face IDs
* src/xfaces.c (syms_of_xfaces): new lisp symbols
(lookup_basic_face, realize_basic_faces): map new face IDs to their lisp
symbols
* lisp/faces.el (header-line-active, header-line-inactive): new deffaces
---
 lisp/faces.el    | 24 ++++++++++++++++++++++++
 src/dispextern.h | 33 +++++++++++++++++++++++++++++++--
 src/xdisp.c      | 45 ++++++++++++++++++++++++++-------------------
 src/xfaces.c     |  8 ++++++--
 4 files changed, 87 insertions(+), 23 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 21c3e663c6e..25a93d0ed7a 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2821,6 +2821,30 @@ Use the face `mode-line-highlight' for features that can be selected."
   :version "28.1"
   :group 'basic-faces)
 
+(defface header-line-active
+  '((t :inherit header-line))
+  "Face for the selected header line.
+This inherits from the `header-line' face."
+  :version "29.5"
+  :group 'mode-line-faces
+  :group 'basic-faces)
+
+(defface header-line-inactive
+  '((default
+     :inherit header-line)
+    (((class color) (min-colors 88) (background light))
+     :weight light
+     :box (:line-width -1 :color "grey75" :style nil)
+     :foreground "grey20" :background "grey90")
+    (((class color) (min-colors 88) (background dark) )
+     :weight light
+     :box (:line-width -1 :color "grey40" :style nil)
+     :foreground "grey80" :background "grey30"))
+  "Basic header line face for non-selected windows."
+  :version "29.5"
+  :group 'mode-line-faces
+  :group 'basic-faces)
+
 (defface vertical-border
   '((((type tty)) :inherit mode-line-inactive))
   "Face used for vertical window dividers on ttys."
diff --git a/src/dispextern.h b/src/dispextern.h
index cc248a4472e..0d4c0a6c67e 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1561,6 +1561,34 @@ struct glyph_string
 	 : estimate_mode_line_height					\
 	 (XFRAME ((W)->frame), CURRENT_MODE_LINE_ACTIVE_FACE_ID (W)))))
 
+/* Return the desired face id for the header line of a window, depending
+   on whether the window is selected or not, or if the window is the
+   scrolling window for the currently active minibuffer window.
+
+   Due to the way display_mode_lines manipulates with the contents of
+   selected_window, this macro needs three arguments: SELW which is
+   compared against the current value of selected_window, MBW which is
+   compared against minibuf_window (if SELW doesn't match), and SCRW
+   which is compared against minibuf_selected_window (if MBW matches).  */
+
+#define CURRENT_HEADER_LINE_ACTIVE_FACE_ID_3(SELW, MBW, SCRW)    	\
+     ((!mode_line_in_non_selected_windows			\
+       || (SELW) == XWINDOW (selected_window)			\
+       || (minibuf_level > 0					\
+           && !NILP (minibuf_selected_window)			\
+           && (MBW) == XWINDOW (minibuf_window)			\
+           && (SCRW) == XWINDOW (minibuf_selected_window)))	\
+      ? HEADER_LINE_ACTIVE_FACE_ID				\
+      : HEADER_LINE_INACTIVE_FACE_ID)
+
+
+/* Return the desired face id for the header line of window W.  */
+
+#define CURRENT_HEADER_LINE_ACTIVE_FACE_ID(W)		\
+	 CURRENT_HEADER_LINE_ACTIVE_FACE_ID_3(W, \
+					    XWINDOW (selected_window), \
+					    W)
+
 /* Return the current height of the header line of window W.  If not known
    from W->header_line_height, look at W's current glyph matrix, or return
    an estimation based on the height of the font of the face `header-line'.  */
@@ -1572,7 +1600,7 @@ struct glyph_string
       = (MATRIX_HEADER_LINE_HEIGHT ((W)->current_matrix)	\
 	 ? MATRIX_HEADER_LINE_HEIGHT ((W)->current_matrix)	\
 	 : estimate_mode_line_height				\
-	     (XFRAME ((W)->frame), HEADER_LINE_FACE_ID))))
+	 (XFRAME ((W)->frame), CURRENT_HEADER_LINE_ACTIVE_FACE_ID (W)))))
 
 /* Return the current height of the tab line of window W.  If not known
    from W->tab_line_height, look at W's current glyph matrix, or return
@@ -1893,7 +1921,8 @@ enum face_id
   MODE_LINE_INACTIVE_FACE_ID,
   TOOL_BAR_FACE_ID,
   FRINGE_FACE_ID,
-  HEADER_LINE_FACE_ID,
+  HEADER_LINE_ACTIVE_FACE_ID,
+  HEADER_LINE_INACTIVE_FACE_ID,
   SCROLL_BAR_FACE_ID,
   BORDER_FACE_ID,
   CURSOR_FACE_ID,
diff --git a/src/xdisp.c b/src/xdisp.c
index c74e81a3933..98bc157c789 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -1358,7 +1358,7 @@ window_box_height (struct window *w)
 	  if (hl_row && hl_row->mode_line_p)
 	    height -= hl_row->height;
 	  else
-	    height -= estimate_mode_line_height (f, HEADER_LINE_FACE_ID);
+	    height -= estimate_mode_line_height (f, CURRENT_HEADER_LINE_ACTIVE_FACE_ID (w));
 	}
     }
 
@@ -1753,7 +1753,7 @@ pos_visible_p (struct window *w, ptrdiff_t charpos, int *x, int *y,
 	= window_parameter (w, Qheader_line_format);
 
       w->header_line_height
-	= display_mode_line (w, HEADER_LINE_FACE_ID,
+	= display_mode_line (w, CURRENT_HEADER_LINE_ACTIVE_FACE_ID (w),
 			     NILP (window_header_line_format)
 			     ? BVAR (current_buffer, header_line_format)
 			     : window_header_line_format);
@@ -3197,13 +3197,14 @@ CHECK_WINDOW_END (struct window *w)
 
    BASE_FACE_ID is the id of a base face to use.  It must be one of
    DEFAULT_FACE_ID for normal text, MODE_LINE_ACTIVE_FACE_ID,
-   MODE_LINE_INACTIVE_FACE_ID, or HEADER_LINE_FACE_ID for displaying
-   mode lines, or TOOL_BAR_FACE_ID for displaying the tool-bar.
+   MODE_LINE_INACTIVE_FACE_ID, HEADER_LINE_ACTIVE_FACE_ID, or
+   HEADER_LINE_INACTIVE_FACE_ID for displaying mode lines, or
+   TOOL_BAR_FACE_ID for displaying the tool-bar.
 
    If ROW is null and BASE_FACE_ID is equal to MODE_LINE_ACTIVE_FACE_ID,
-   MODE_LINE_INACTIVE_FACE_ID, or HEADER_LINE_FACE_ID, the iterator
-   will be initialized to use the corresponding mode line glyph row of
-   the desired matrix of W.  */
+   MODE_LINE_INACTIVE_FACE_ID, HEADER_LINE_ACTIVE_FACE_ID, or
+   HEADER_LINE_INACTIVE_FACE_ID the iterator will be initialized to use
+   the corresponding mode line glyph row of the desired matrix of W.  */
 
 void
 init_iterator (struct it *it, struct window *w,
@@ -3251,7 +3252,8 @@ init_iterator (struct it *it, struct window *w,
 	row = MATRIX_MODE_LINE_ROW (w->desired_matrix);
       else if (base_face_id == TAB_LINE_FACE_ID)
 	row = MATRIX_TAB_LINE_ROW (w->desired_matrix);
-      else if (base_face_id == HEADER_LINE_FACE_ID)
+      else if (base_face_id == HEADER_LINE_ACTIVE_FACE_ID
+	       || base_face_id == HEADER_LINE_INACTIVE_FACE_ID)
 	{
 	  /* Header line row depends on whether tab line is enabled.  */
 	  w->desired_matrix->tab_line_p = window_wants_tab_line (w);
@@ -11854,7 +11856,7 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
       Lisp_Object window_header_line_format
 	= window_parameter (w, Qheader_line_format);
 
-      y = y + display_mode_line (w, HEADER_LINE_FACE_ID,
+      y = y + display_mode_line (w, CURRENT_HEADER_LINE_ACTIVE_FACE_ID (w),
 				 NILP (window_header_line_format)
 				 ? BVAR (current_buffer, header_line_format)
 				 : window_header_line_format);
@@ -27453,11 +27455,11 @@ display_mode_lines (struct window *w)
   line_number_displayed = false;
   w->column_number_displayed = -1;
 
+  struct window *sel_w = XWINDOW (old_selected_window);
   if (window_wants_mode_line (w))
     {
       Lisp_Object window_mode_line_format
 	= window_parameter (w, Qmode_line_format);
-      struct window *sel_w = XWINDOW (old_selected_window);
 
       /* Select mode line face based on the real selected window.  */
       display_mode_line (w,
@@ -27485,7 +27487,7 @@ display_mode_lines (struct window *w)
       Lisp_Object window_header_line_format
 	= window_parameter (w, Qheader_line_format);
 
-      display_mode_line (w, HEADER_LINE_FACE_ID,
+      display_mode_line (w, CURRENT_HEADER_LINE_ACTIVE_FACE_ID_3 (sel_w, sel_w, w),
 			 NILP (window_header_line_format)
 			 ? BVAR (current_buffer, header_line_format)
 			 : window_header_line_format);
@@ -27500,11 +27502,12 @@ display_mode_lines (struct window *w)
 }
 
 
-/* Display mode or header/tab line of window W.  FACE_ID specifies
-   which line to display; it is either MODE_LINE_ACTIVE_FACE_ID,
-   HEADER_LINE_FACE_ID or TAB_LINE_FACE_ID.  FORMAT is the
-   mode/header/tab line format to display.  Value is the pixel height
-   of the mode/header/tab line displayed.  */
+/* Display mode or header/tab line of window W.  FACE_ID specifies which
+   line to display; it is either MODE_LINE_ACTIVE_FACE_ID,
+   HEADER_LINE_ACTIVE_FACE_ID, HEADER_LINE_INACTIVE_FACE_ID, or
+   TAB_LINE_FACE_ID.  FORMAT is the mode/header/tab line format to
+   display.  Value is the pixel height of the mode/header/tab line
+   displayed.  */
 
 static int
 display_mode_line (struct window *w, enum face_id face_id, Lisp_Object format)
@@ -27525,7 +27528,8 @@ display_mode_line (struct window *w, enum face_id face_id, Lisp_Object format)
       it.glyph_row->tab_line_p = true;
       w->desired_matrix->tab_line_p = true;
     }
-  else if (face_id == HEADER_LINE_FACE_ID)
+  else if (face_id == HEADER_LINE_ACTIVE_FACE_ID
+	   || face_id == HEADER_LINE_INACTIVE_FACE_ID)
     w->desired_matrix->header_line_p = true;
 
   /* FIXME: This should be controlled by a user option.  But
@@ -27544,7 +27548,9 @@ display_mode_line (struct window *w, enum face_id face_id, Lisp_Object format)
   record_unwind_save_match_data ();
 
   if (NILP (Vmode_line_compact)
-      || face_id == HEADER_LINE_FACE_ID || face_id == TAB_LINE_FACE_ID)
+      || face_id == HEADER_LINE_ACTIVE_FACE_ID
+      || face_id == HEADER_LINE_INACTIVE_FACE_ID
+      || face_id == TAB_LINE_FACE_ID)
     {
       mode_line_target = MODE_LINE_DISPLAY;
       display_mode_element (&it, 0, 0, 0, format, Qnil, false);
@@ -28313,7 +28319,8 @@ are the selected window and the WINDOW's buffer).  */)
 		       ? MODE_LINE_ACTIVE_FACE_ID : MODE_LINE_INACTIVE_FACE_ID)
     : EQ (face, Qmode_line_active) ? MODE_LINE_ACTIVE_FACE_ID
     : EQ (face, Qmode_line_inactive) ? MODE_LINE_INACTIVE_FACE_ID
-    : EQ (face, Qheader_line) ? HEADER_LINE_FACE_ID
+    : EQ (face, Qheader_line_active) ? HEADER_LINE_ACTIVE_FACE_ID
+    : EQ (face, Qheader_line_inactive) ? HEADER_LINE_INACTIVE_FACE_ID
     : EQ (face, Qtab_line) ? TAB_LINE_FACE_ID
     : EQ (face, Qtab_bar) ? TAB_BAR_FACE_ID
     : EQ (face, Qtool_bar) ? TOOL_BAR_FACE_ID
diff --git a/src/xfaces.c b/src/xfaces.c
index e248279e9b7..f6264802fa4 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -5117,7 +5117,8 @@ lookup_basic_face (struct window *w, struct frame *f, int face_id)
     case DEFAULT_FACE_ID:		name = Qdefault;		break;
     case MODE_LINE_ACTIVE_FACE_ID:	name = Qmode_line_active;      	break;
     case MODE_LINE_INACTIVE_FACE_ID:	name = Qmode_line_inactive;	break;
-    case HEADER_LINE_FACE_ID:		name = Qheader_line;		break;
+    case HEADER_LINE_ACTIVE_FACE_ID:	name = Qheader_line_active;	break;
+    case HEADER_LINE_INACTIVE_FACE_ID:	name = Qheader_line_inactive;	break;
     case TAB_LINE_FACE_ID:		name = Qtab_line;		break;
     case TAB_BAR_FACE_ID:		name = Qtab_bar;		break;
     case TOOL_BAR_FACE_ID:		name = Qtool_bar;		break;
@@ -5867,7 +5868,8 @@ realize_basic_faces (struct frame *f)
       realize_named_face (f, Qmode_line_inactive, MODE_LINE_INACTIVE_FACE_ID);
       realize_named_face (f, Qtool_bar, TOOL_BAR_FACE_ID);
       realize_named_face (f, Qfringe, FRINGE_FACE_ID);
-      realize_named_face (f, Qheader_line, HEADER_LINE_FACE_ID);
+      realize_named_face (f, Qheader_line_active, HEADER_LINE_ACTIVE_FACE_ID);
+      realize_named_face (f, Qheader_line_inactive, HEADER_LINE_INACTIVE_FACE_ID);
       realize_named_face (f, Qscroll_bar, SCROLL_BAR_FACE_ID);
       realize_named_face (f, Qborder, BORDER_FACE_ID);
       realize_named_face (f, Qcursor, CURSOR_FACE_ID);
@@ -7438,6 +7440,8 @@ syms_of_xfaces (void)
   DEFSYM (Qfringe, "fringe");
   DEFSYM (Qtab_line, "tab-line");
   DEFSYM (Qheader_line, "header-line");
+  DEFSYM (Qheader_line_inactive, "header-line-inactive");
+  DEFSYM (Qheader_line_active, "header-line-active");
   DEFSYM (Qscroll_bar, "scroll-bar");
   DEFSYM (Qmenu, "menu");
   DEFSYM (Qcursor, "cursor");
-- 
2.46.2

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: trevor.m.murphy@gmail.com
> Date: Fri, 18 Oct 2024 05:56:32 -0700
> 
> Over the years I've put more and more information into the header line, and at some point I wanted to get that quick visual indicator of selected vs non-selected windows when I glanced at the header as I already got from the mode line.
> 
> This was surprisingly hard to achieve!  For a few years I used an ugly `buffer-list-update-hook` hack.  Then a few months ago I got adventurous and found that it actually wasn't as hard as I'd feared to hack these into the C source.
> 
> The new faces both inherit from the current `header-line` face, so there shouldn't be any visible impact if users don't customize the new faces.  For cases like mine it's easy to customize header-line-inactive to inherit from mode-line-inactive.
> 
> Thanks in advance for your attention and feedback,

Thanks.  This changeset needs the appropriate changes in the Emacs
user manual (which lists the standard faces in "Standard Faces"), and
also a NEWS entry that announces the change.

Also, did you verify that modes which use header-line and customize
the header-line face still work as before after this change, both when
the window is selected and non-selected?

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> Cc: 73862@debbugs.gnu.org
> Date: Sun, 27 Oct 2024 12:46:53 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: trevor.m.murphy@gmail.com
> > Date: Fri, 18 Oct 2024 05:56:32 -0700
> > 
> > Over the years I've put more and more information into the header line, and at some point I wanted to get that quick visual indicator of selected vs non-selected windows when I glanced at the header as I already got from the mode line.
> > 
> > This was surprisingly hard to achieve!  For a few years I used an ugly `buffer-list-update-hook` hack.  Then a few months ago I got adventurous and found that it actually wasn't as hard as I'd feared to hack these into the C source.
> > 
> > The new faces both inherit from the current `header-line` face, so there shouldn't be any visible impact if users don't customize the new faces.  For cases like mine it's easy to customize header-line-inactive to inherit from mode-line-inactive.
> > 
> > Thanks in advance for your attention and feedback,
> 
> Thanks.  This changeset needs the appropriate changes in the Emacs
> user manual (which lists the standard faces in "Standard Faces"), and
> also a NEWS entry that announces the change.
> 
> Also, did you verify that modes which use header-line and customize
> the header-line face still work as before after this change, both when
> the window is selected and non-selected?

Ping! Trevor, could you please answer the above question, and provide
an updated patch with documentation changes?  I'd like to install this
as soon as these issues are resolved.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Trevor Murphy]

[-- Attachment #1.1: Type: text/plain, Size: 2318 bytes --]

Hi!  Thanks for your patience, attached please find the updated patch.
I've added documentation and a NEWS entry modeled off of similar commits.

When I put together the original patch I included a non-trivial value for
the default header-line-inactive (copied from mode-line-inactive) as a
visual indicator for the new face.  Happy to take it out.  In this updated
patch, header-line-inactive just inherits from header-line.  I tested by
looking at selected and not-selected Info buffers with emacs -Q both
patched and unpatched, in both graphical and terminal.

On Sat, Nov 9, 2024 at 1:37 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > Cc: 73862@debbugs.gnu.org
> > Date: Sun, 27 Oct 2024 12:46:53 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> >
> > > From: trevor.m.murphy@gmail.com
> > > Date: Fri, 18 Oct 2024 05:56:32 -0700
> > >
> > > Over the years I've put more and more information into the header
> line, and at some point I wanted to get that quick visual indicator of
> selected vs non-selected windows when I glanced at the header as I already
> got from the mode line.
> > >
> > > This was surprisingly hard to achieve!  For a few years I used an ugly
> `buffer-list-update-hook` hack.  Then a few months ago I got adventurous
> and found that it actually wasn't as hard as I'd feared to hack these into
> the C source.
> > >
> > > The new faces both inherit from the current `header-line` face, so
> there shouldn't be any visible impact if users don't customize the new
> faces.  For cases like mine it's easy to customize header-line-inactive to
> inherit from mode-line-inactive.
> > >
> > > Thanks in advance for your attention and feedback,
> >
> > Thanks.  This changeset needs the appropriate changes in the Emacs
> > user manual (which lists the standard faces in "Standard Faces"), and
> > also a NEWS entry that announces the change.
> >
> > Also, did you verify that modes which use header-line and customize
> > the header-line face still work as before after this change, both when
> > the window is selected and non-selected?
>
> Ping! Trevor, could you please answer the above question, and provide
> an updated patch with documentation changes?  I'd like to install this
> as soon as these issues are resolved.
>


-- 
Trevor Murphy

[-- Attachment #1.2: Type: text/html, Size: 3151 bytes --]

[-- Attachment #2: 0001-Add-new-header-line-active-and-header-line-inactive-.patch --]
[-- Type: text/x-patch, Size: 14234 bytes --]

From fed179e66b521d30e9a7f393230ce10573302973 Mon Sep 17 00:00:00 2001
From: Trevor Murphy <trevor.m.murphy@gmail.com>
Date: Thu, 17 Oct 2024 15:51:14 -0700
Subject: [PATCH] Add new `header-line-active` and `header-line-inactive`
 faces.

This is all intended to parallel the mode-line-active and
mode-line-inactive distinction.

* doc/emacs/display.texi (Standard Faces): Document the new faces.

* lisp/faces.el (header-line-active, header-line-inactive): New faces.

* src/dispextern.h (CURRENT_HEADER_LINE_ACTIVE_FACE_ID_3,
CURRENT_HEADER_LINE_ACTIVE_FACE_ID): new macros based on mode line
equivalents
(face_id): new face IDs
* src/xdisp.c (window_box_height, pos_visible_p, init_iterator,
window_text_pixel_size, display_mode_lines, display_mode_line,
format-mode-line): replace all uses of HEADER_LINE_FACE_ID with either
a new macro or the new face IDs
* src/xfaces.c (syms_of_xfaces): new lisp symbols
(lookup_basic_face, realize_basic_faces): map new face IDs to their lisp
symbols
---
 doc/emacs/display.texi | 16 +++++++++++++++
 etc/NEWS               |  5 +++++
 lisp/faces.el          | 15 ++++++++++++++
 src/dispextern.h       | 33 +++++++++++++++++++++++++++++--
 src/xdisp.c            | 45 ++++++++++++++++++++++++------------------
 src/xfaces.c           |  8 ++++++--
 6 files changed, 99 insertions(+), 23 deletions(-)

diff --git a/doc/emacs/display.texi b/doc/emacs/display.texi
index 88520874c8e..22cd316f836 100644
--- a/doc/emacs/display.texi
+++ b/doc/emacs/display.texi
@@ -782,6 +782,22 @@ at the top of a window just as the mode line appears at the bottom.
 Most windows do not have a header line---only some special modes, such
 Info mode, create one.
 
+The @code{header-line-active} and @code{header-line-inactive} faces (which
+are the ones used on the header lines) inherit from this face.
+
+@item header-line-active
+@cindex faces for header lines
+Like @code{header-line}, but used for the header line of the currently
+selected window.  This face inherits from @code{header-line}, so changes
+in that face affect header lines in all windows.
+
+@item header-line-inactive
+@cindex @code{header-line-inactive} face
+Like @code{header-line}, but used for header lines of the windows other
+than the selected one (if those windows have a header line).  This face
+inherits from @code{header-line}, so changes in that face affect header
+lines in all windows.
+
 @item header-line-highlight
 @cindex @code{header-line-highlight} face
 Similar to @code{highlight} and @code{mode-line-highlight}, but used
diff --git a/etc/NEWS b/etc/NEWS
index a8ece5c3dc9..b0390e597d0 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -150,6 +150,11 @@ Killed buffers stored in a register using 'buffer-to-register' are
 automatically converted to a file-query value if the buffer was visiting
 a file.
 
+** New face 'header-line-active'.
+This inherits from the 'header-line' face, but is the face actually used
+on the header lines (along with 'header-line-inactive').
+
++++
 \f
 * Editing Changes in Emacs 31.1
 
diff --git a/lisp/faces.el b/lisp/faces.el
index 21c3e663c6e..dd85376de24 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2821,6 +2821,21 @@ Use the face `mode-line-highlight' for features that can be selected."
   :version "28.1"
   :group 'basic-faces)
 
+(defface header-line-active
+  '((t :inherit header-line))
+  "Face for the selected header line.
+This inherits from the `header-line' face."
+  :version "29.5"
+  :group 'mode-line-faces
+  :group 'basic-faces)
+
+(defface header-line-inactive
+  '((t :inherit header-line))
+  "Basic header line face for non-selected windows."
+  :version "29.5"
+  :group 'mode-line-faces
+  :group 'basic-faces)
+
 (defface vertical-border
   '((((type tty)) :inherit mode-line-inactive))
   "Face used for vertical window dividers on ttys."
diff --git a/src/dispextern.h b/src/dispextern.h
index cc248a4472e..0d4c0a6c67e 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1561,6 +1561,34 @@ struct glyph_string
 	 : estimate_mode_line_height					\
 	 (XFRAME ((W)->frame), CURRENT_MODE_LINE_ACTIVE_FACE_ID (W)))))
 
+/* Return the desired face id for the header line of a window, depending
+   on whether the window is selected or not, or if the window is the
+   scrolling window for the currently active minibuffer window.
+
+   Due to the way display_mode_lines manipulates with the contents of
+   selected_window, this macro needs three arguments: SELW which is
+   compared against the current value of selected_window, MBW which is
+   compared against minibuf_window (if SELW doesn't match), and SCRW
+   which is compared against minibuf_selected_window (if MBW matches).  */
+
+#define CURRENT_HEADER_LINE_ACTIVE_FACE_ID_3(SELW, MBW, SCRW)    	\
+     ((!mode_line_in_non_selected_windows			\
+       || (SELW) == XWINDOW (selected_window)			\
+       || (minibuf_level > 0					\
+           && !NILP (minibuf_selected_window)			\
+           && (MBW) == XWINDOW (minibuf_window)			\
+           && (SCRW) == XWINDOW (minibuf_selected_window)))	\
+      ? HEADER_LINE_ACTIVE_FACE_ID				\
+      : HEADER_LINE_INACTIVE_FACE_ID)
+
+
+/* Return the desired face id for the header line of window W.  */
+
+#define CURRENT_HEADER_LINE_ACTIVE_FACE_ID(W)		\
+	 CURRENT_HEADER_LINE_ACTIVE_FACE_ID_3(W, \
+					    XWINDOW (selected_window), \
+					    W)
+
 /* Return the current height of the header line of window W.  If not known
    from W->header_line_height, look at W's current glyph matrix, or return
    an estimation based on the height of the font of the face `header-line'.  */
@@ -1572,7 +1600,7 @@ struct glyph_string
       = (MATRIX_HEADER_LINE_HEIGHT ((W)->current_matrix)	\
 	 ? MATRIX_HEADER_LINE_HEIGHT ((W)->current_matrix)	\
 	 : estimate_mode_line_height				\
-	     (XFRAME ((W)->frame), HEADER_LINE_FACE_ID))))
+	 (XFRAME ((W)->frame), CURRENT_HEADER_LINE_ACTIVE_FACE_ID (W)))))
 
 /* Return the current height of the tab line of window W.  If not known
    from W->tab_line_height, look at W's current glyph matrix, or return
@@ -1893,7 +1921,8 @@ enum face_id
   MODE_LINE_INACTIVE_FACE_ID,
   TOOL_BAR_FACE_ID,
   FRINGE_FACE_ID,
-  HEADER_LINE_FACE_ID,
+  HEADER_LINE_ACTIVE_FACE_ID,
+  HEADER_LINE_INACTIVE_FACE_ID,
   SCROLL_BAR_FACE_ID,
   BORDER_FACE_ID,
   CURSOR_FACE_ID,
diff --git a/src/xdisp.c b/src/xdisp.c
index c74e81a3933..98bc157c789 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -1358,7 +1358,7 @@ window_box_height (struct window *w)
 	  if (hl_row && hl_row->mode_line_p)
 	    height -= hl_row->height;
 	  else
-	    height -= estimate_mode_line_height (f, HEADER_LINE_FACE_ID);
+	    height -= estimate_mode_line_height (f, CURRENT_HEADER_LINE_ACTIVE_FACE_ID (w));
 	}
     }
 
@@ -1753,7 +1753,7 @@ pos_visible_p (struct window *w, ptrdiff_t charpos, int *x, int *y,
 	= window_parameter (w, Qheader_line_format);
 
       w->header_line_height
-	= display_mode_line (w, HEADER_LINE_FACE_ID,
+	= display_mode_line (w, CURRENT_HEADER_LINE_ACTIVE_FACE_ID (w),
 			     NILP (window_header_line_format)
 			     ? BVAR (current_buffer, header_line_format)
 			     : window_header_line_format);
@@ -3197,13 +3197,14 @@ CHECK_WINDOW_END (struct window *w)
 
    BASE_FACE_ID is the id of a base face to use.  It must be one of
    DEFAULT_FACE_ID for normal text, MODE_LINE_ACTIVE_FACE_ID,
-   MODE_LINE_INACTIVE_FACE_ID, or HEADER_LINE_FACE_ID for displaying
-   mode lines, or TOOL_BAR_FACE_ID for displaying the tool-bar.
+   MODE_LINE_INACTIVE_FACE_ID, HEADER_LINE_ACTIVE_FACE_ID, or
+   HEADER_LINE_INACTIVE_FACE_ID for displaying mode lines, or
+   TOOL_BAR_FACE_ID for displaying the tool-bar.
 
    If ROW is null and BASE_FACE_ID is equal to MODE_LINE_ACTIVE_FACE_ID,
-   MODE_LINE_INACTIVE_FACE_ID, or HEADER_LINE_FACE_ID, the iterator
-   will be initialized to use the corresponding mode line glyph row of
-   the desired matrix of W.  */
+   MODE_LINE_INACTIVE_FACE_ID, HEADER_LINE_ACTIVE_FACE_ID, or
+   HEADER_LINE_INACTIVE_FACE_ID the iterator will be initialized to use
+   the corresponding mode line glyph row of the desired matrix of W.  */
 
 void
 init_iterator (struct it *it, struct window *w,
@@ -3251,7 +3252,8 @@ init_iterator (struct it *it, struct window *w,
 	row = MATRIX_MODE_LINE_ROW (w->desired_matrix);
       else if (base_face_id == TAB_LINE_FACE_ID)
 	row = MATRIX_TAB_LINE_ROW (w->desired_matrix);
-      else if (base_face_id == HEADER_LINE_FACE_ID)
+      else if (base_face_id == HEADER_LINE_ACTIVE_FACE_ID
+	       || base_face_id == HEADER_LINE_INACTIVE_FACE_ID)
 	{
 	  /* Header line row depends on whether tab line is enabled.  */
 	  w->desired_matrix->tab_line_p = window_wants_tab_line (w);
@@ -11854,7 +11856,7 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to,
       Lisp_Object window_header_line_format
 	= window_parameter (w, Qheader_line_format);
 
-      y = y + display_mode_line (w, HEADER_LINE_FACE_ID,
+      y = y + display_mode_line (w, CURRENT_HEADER_LINE_ACTIVE_FACE_ID (w),
 				 NILP (window_header_line_format)
 				 ? BVAR (current_buffer, header_line_format)
 				 : window_header_line_format);
@@ -27453,11 +27455,11 @@ display_mode_lines (struct window *w)
   line_number_displayed = false;
   w->column_number_displayed = -1;
 
+  struct window *sel_w = XWINDOW (old_selected_window);
   if (window_wants_mode_line (w))
     {
       Lisp_Object window_mode_line_format
 	= window_parameter (w, Qmode_line_format);
-      struct window *sel_w = XWINDOW (old_selected_window);
 
       /* Select mode line face based on the real selected window.  */
       display_mode_line (w,
@@ -27485,7 +27487,7 @@ display_mode_lines (struct window *w)
       Lisp_Object window_header_line_format
 	= window_parameter (w, Qheader_line_format);
 
-      display_mode_line (w, HEADER_LINE_FACE_ID,
+      display_mode_line (w, CURRENT_HEADER_LINE_ACTIVE_FACE_ID_3 (sel_w, sel_w, w),
 			 NILP (window_header_line_format)
 			 ? BVAR (current_buffer, header_line_format)
 			 : window_header_line_format);
@@ -27500,11 +27502,12 @@ display_mode_lines (struct window *w)
 }
 
 
-/* Display mode or header/tab line of window W.  FACE_ID specifies
-   which line to display; it is either MODE_LINE_ACTIVE_FACE_ID,
-   HEADER_LINE_FACE_ID or TAB_LINE_FACE_ID.  FORMAT is the
-   mode/header/tab line format to display.  Value is the pixel height
-   of the mode/header/tab line displayed.  */
+/* Display mode or header/tab line of window W.  FACE_ID specifies which
+   line to display; it is either MODE_LINE_ACTIVE_FACE_ID,
+   HEADER_LINE_ACTIVE_FACE_ID, HEADER_LINE_INACTIVE_FACE_ID, or
+   TAB_LINE_FACE_ID.  FORMAT is the mode/header/tab line format to
+   display.  Value is the pixel height of the mode/header/tab line
+   displayed.  */
 
 static int
 display_mode_line (struct window *w, enum face_id face_id, Lisp_Object format)
@@ -27525,7 +27528,8 @@ display_mode_line (struct window *w, enum face_id face_id, Lisp_Object format)
       it.glyph_row->tab_line_p = true;
       w->desired_matrix->tab_line_p = true;
     }
-  else if (face_id == HEADER_LINE_FACE_ID)
+  else if (face_id == HEADER_LINE_ACTIVE_FACE_ID
+	   || face_id == HEADER_LINE_INACTIVE_FACE_ID)
     w->desired_matrix->header_line_p = true;
 
   /* FIXME: This should be controlled by a user option.  But
@@ -27544,7 +27548,9 @@ display_mode_line (struct window *w, enum face_id face_id, Lisp_Object format)
   record_unwind_save_match_data ();
 
   if (NILP (Vmode_line_compact)
-      || face_id == HEADER_LINE_FACE_ID || face_id == TAB_LINE_FACE_ID)
+      || face_id == HEADER_LINE_ACTIVE_FACE_ID
+      || face_id == HEADER_LINE_INACTIVE_FACE_ID
+      || face_id == TAB_LINE_FACE_ID)
     {
       mode_line_target = MODE_LINE_DISPLAY;
       display_mode_element (&it, 0, 0, 0, format, Qnil, false);
@@ -28313,7 +28319,8 @@ are the selected window and the WINDOW's buffer).  */)
 		       ? MODE_LINE_ACTIVE_FACE_ID : MODE_LINE_INACTIVE_FACE_ID)
     : EQ (face, Qmode_line_active) ? MODE_LINE_ACTIVE_FACE_ID
     : EQ (face, Qmode_line_inactive) ? MODE_LINE_INACTIVE_FACE_ID
-    : EQ (face, Qheader_line) ? HEADER_LINE_FACE_ID
+    : EQ (face, Qheader_line_active) ? HEADER_LINE_ACTIVE_FACE_ID
+    : EQ (face, Qheader_line_inactive) ? HEADER_LINE_INACTIVE_FACE_ID
     : EQ (face, Qtab_line) ? TAB_LINE_FACE_ID
     : EQ (face, Qtab_bar) ? TAB_BAR_FACE_ID
     : EQ (face, Qtool_bar) ? TOOL_BAR_FACE_ID
diff --git a/src/xfaces.c b/src/xfaces.c
index e248279e9b7..f6264802fa4 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -5117,7 +5117,8 @@ lookup_basic_face (struct window *w, struct frame *f, int face_id)
     case DEFAULT_FACE_ID:		name = Qdefault;		break;
     case MODE_LINE_ACTIVE_FACE_ID:	name = Qmode_line_active;      	break;
     case MODE_LINE_INACTIVE_FACE_ID:	name = Qmode_line_inactive;	break;
-    case HEADER_LINE_FACE_ID:		name = Qheader_line;		break;
+    case HEADER_LINE_ACTIVE_FACE_ID:	name = Qheader_line_active;	break;
+    case HEADER_LINE_INACTIVE_FACE_ID:	name = Qheader_line_inactive;	break;
     case TAB_LINE_FACE_ID:		name = Qtab_line;		break;
     case TAB_BAR_FACE_ID:		name = Qtab_bar;		break;
     case TOOL_BAR_FACE_ID:		name = Qtool_bar;		break;
@@ -5867,7 +5868,8 @@ realize_basic_faces (struct frame *f)
       realize_named_face (f, Qmode_line_inactive, MODE_LINE_INACTIVE_FACE_ID);
       realize_named_face (f, Qtool_bar, TOOL_BAR_FACE_ID);
       realize_named_face (f, Qfringe, FRINGE_FACE_ID);
-      realize_named_face (f, Qheader_line, HEADER_LINE_FACE_ID);
+      realize_named_face (f, Qheader_line_active, HEADER_LINE_ACTIVE_FACE_ID);
+      realize_named_face (f, Qheader_line_inactive, HEADER_LINE_INACTIVE_FACE_ID);
       realize_named_face (f, Qscroll_bar, SCROLL_BAR_FACE_ID);
       realize_named_face (f, Qborder, BORDER_FACE_ID);
       realize_named_face (f, Qcursor, CURSOR_FACE_ID);
@@ -7438,6 +7440,8 @@ syms_of_xfaces (void)
   DEFSYM (Qfringe, "fringe");
   DEFSYM (Qtab_line, "tab-line");
   DEFSYM (Qheader_line, "header-line");
+  DEFSYM (Qheader_line_inactive, "header-line-inactive");
+  DEFSYM (Qheader_line_active, "header-line-active");
   DEFSYM (Qscroll_bar, "scroll-bar");
   DEFSYM (Qmenu, "menu");
   DEFSYM (Qcursor, "cursor");
-- 
2.47.0

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Trevor Murphy <trevor.m.murphy@gmail.com>
> Date: Sun, 10 Nov 2024 22:11:30 -0800
> Cc: 73862@debbugs.gnu.org
> 
> Hi!  Thanks for your patience, attached please find the updated patch.  I've added documentation and a
> NEWS entry modeled off of similar commits.  
> 
> When I put together the original patch I included a non-trivial value for the default header-line-inactive
> (copied from mode-line-inactive) as a visual indicator for the new face.  Happy to take it out.  In this updated
> patch, header-line-inactive just inherits from header-line.  I tested by looking at selected and not-selected
> Info buffers with emacs -Q both patched and unpatched, in both graphical and terminal.

Thanks, installed on the master branch, and closing the bug.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

I bisected and found that this patch caused a regression in header-line
formatting. I don't have the exact reproduction steps yet, but the gist is
that I use a header-line with multiple faces in one window, then open a new
popup window that has no header-line. The original window's header-line
will get its faces changed slightly, but it's hard to tell how.

Here's a video demonstrating it: https://share.cleanshot.com/CD9PtVv3

You can see that the padding on the left of the header line goes away and
the line numbers change to a variable pitch font. If I adjust my font size,
the header line redraws with the proper faces. This will happen randomly,
but I can force it with the font size change reliably.

I'll see if I can narrow reproduction steps, but it may be worth
considering a revert for now.

Thanks,


Aaron

[-- Attachment #2: Type: text/html, Size: 1228 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

I'm able to reproduce this with the mode-line as well, so it appears that
there may be a bug there too in the code that was copied to implement the
active/inactive faces in the header line. It's entirely possible that
there's a bug in nano-modeline, but it seems suspect that code was added to
consider windows and now this issue triggers when new windows are created
(and possibly when selection changes, I haven't eliminated whether or not
that's a factor yet).


Aaron


On Tue, Dec 03, 2024 at 9:06 PM, Aaron Jensen <aaronjensen@gmail.com> wrote:

> I bisected and found that this patch caused a regression in header-line
> formatting. I don't have the exact reproduction steps yet, but the gist is
> that I use a header-line with multiple faces in one window, then open a new
> popup window that has no header-line. The original window's header-line
> will get its faces changed slightly, but it's hard to tell how.
>
> Here's a video demonstrating it: https://share.cleanshot.com/CD9PtVv3
>
> You can see that the padding on the left of the header line goes away and
> the line numbers change to a variable pitch font. If I adjust my font size,
> the header line redraws with the proper faces. This will happen randomly,
> but I can force it with the font size change reliably.
>
> I'll see if I can narrow reproduction steps, but it may be worth
> considering a revert for now.
>
> Thanks,
>
>
> Aaron
>

[-- Attachment #2: Type: text/html, Size: 2714 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Tue, 3 Dec 2024 22:30:58 -0800

Aaron, it would have been more useful to CC Trevor, who is the author
of that changeset.  I've added him now.

> I'm able to reproduce this with the mode-line as well, so it appears that there may be a bug there too in the
> code that was copied to implement the active/inactive faces in the header line. It's entirely possible that
> there's a bug in nano-modeline, but it seems suspect that code was added to consider windows and now this
> issue triggers when new windows are created (and possibly when selection changes, I haven't eliminated
> whether or not that's a factor yet).

You were able to reproduce what?  I don't think you posted a recipe to
reproduce the problem.  Please do, if at all possible, preferably
starting from "emacs -Q".

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: Trevor Murphy <trevor.m.murphy@gmail.com>
> Date: Wed, 04 Dec 2024 10:47:18 +0100
> 
> >     Add new `header-line-active' and `header-line-inactive' faces
> >     
> >     This is all intended to parallel the 'mode-line-active' and
> >     'mode-line-inactive' distinction.
> [...]
> 
> This seems to introduce a regression, consider the following recipe:
> 
> 1. emacs -Q
> 2. In the scratch buffer, evaluate:
>    (setq header-line-format "foobar")
>    (face-remap-add-relative 'header-line 'highlight)

Aren't you supposed to remap the two new faces instead of
'header-line'?

> 3. Type C-x C-M-= or something similar to force updating the header
>    line.  The header line in the scratch buffer now shows "foobar" and
>    uses the highlight face, as expected
> 4. Type C-x 4 b new RET to switch to another buffer in another window
> 5. In the new buffer evaluate (setq header-line-format "foobar")
> 6. Observe that the header line in the new buffer is also using the
>    highlight face.  That's unexpected!
> 7. Type C-x C-M-= while the new buffer is current
> 8. Observe that the header lines in both windows no longer have the
>    highlight face.  That's unexpected!
> 
> Before, remapping the header-line face with face-remap-add-relative
> would only affect the current buffer, as expected.  Now it seems like
> the face remapping "leaks" between buffers/windows somehow...

Trevor, could you please look into this?

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Wed, Dec 04, 2024 at 5:49 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> Aaron, it would have been more useful to CC Trevor, who is the author of
> that changeset. I've added him now.
>
>
You're right, I realized that too late. Thank you for the reminder.

I'm able to reproduce this with the mode-line as well, so it appears that
> there may be a bug there too in the code that was copied to implement the
> active/inactive faces in the header line. It's entirely possible that
> there's a bug in nano-modeline, but it seems suspect that code was added to
> consider windows and now this issue triggers when new windows are created
> (and possibly when selection changes, I haven't eliminated whether or not
> that's a factor yet).
>
> You were able to reproduce what? I don't think you posted a recipe to
> reproduce the problem. Please do, if at all possible, preferably starting
> from "emacs -Q".
>

I didn't — I thought I mentioned that. I had intended to provide one as
soon as I had a chance to, but it turns out that Eshel encountered the same
issue and provided a recipe (thank you, Eshel). The only difference in my
case is that face-remap-set-base is used, rather than
face-remap-add-relative.

As far as I can tell, this same bug occurs in the mode-line as well as the
header-line. I.e., there was an existing bug in the mode-line and it was
replicated to the header-line after the two new faces were added.

Thanks,

Aaron

[-- Attachment #2: Type: text/html, Size: 2725 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Wed, 4 Dec 2024 19:06:26 -0800
> Cc: Trevor Murphy <trevor.m.murphy@gmail.com>, Eshel Yaron <me@eshelyaron.com>, 73862@debbugs.gnu.org
> 
>  You were able to reproduce what? I don't think you posted a recipe to reproduce the problem. Please
>  do, if at all possible, preferably starting from "emacs -Q".
> 
> I didn't — I thought I mentioned that. I had intended to provide one as soon as I had a chance to, but it turns
> out that Eshel encountered the same issue and provided a recipe (thank you, Eshel). The only difference in
> my case is that face-remap-set-base is used, rather than face-remap-add-relative.
> 
> As far as I can tell, this same bug occurs in the mode-line as well as the header-line. I.e., there was an
> existing bug in the mode-line and it was replicated to the header-line after the two new faces were added.

If what you see is the same as Eshel, I will ask you the same
question: shouldn't you apply face-remapping to the 2 new faces
instead of the 'header-line' face from which they both inherit?
What happens if you do define remapping for those two new faces?

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Aaron Jensen <aaronjensen@gmail.com>
>> Date: Wed, 4 Dec 2024 19:06:26 -0800
>> Cc: Trevor Murphy <trevor.m.murphy@gmail.com>, Eshel Yaron <me@eshelyaron.com>, 73862@debbugs.gnu.org
>> 
>>  You were able to reproduce what? I don't think you posted a recipe to reproduce the problem. Please
>>  do, if at all possible, preferably starting from "emacs -Q".
>> 
>> I didn't — I thought I mentioned that. I had intended to provide one as soon as I had a chance to, but it turns
>> out that Eshel encountered the same issue and provided a recipe (thank you, Eshel). The only difference in
>> my case is that face-remap-set-base is used, rather than face-remap-add-relative.
>> 
>> As far as I can tell, this same bug occurs in the mode-line as well as the header-line. I.e., there was an
>> existing bug in the mode-line and it was replicated to the header-line after the two new faces were added.
>
> If what you see is the same as Eshel, I will ask you the same
> question: shouldn't you apply face-remapping to the 2 new faces
> instead of the 'header-line' face from which they both inherit?
> What happens if you do define remapping for those two new faces?

At least to me, it's not clear what you mean by "should".  Existing code
remaps the header-line face with good results (prior to this change), so
if we now "should" remap something else instead to get the same results,
that means this is a breaking change.  Is that intended?  If so, OK, if
not, shouldn't it be fixed? :)


Thanks,

Eshel

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

>
> If what you see is the same as Eshel, I will ask you the same question:
> shouldn't you apply face-remapping to the 2 new faces instead of the
> 'header-line' face from which they both inherit? What happens if you do
> define remapping for those two new faces?
>

My specific problem does not occur if I remap the two new faces. Why
would I need to do that though? Both inherit from header-line, so if I
wanted to change both, I would naturally change the base face.

Furthermore, this problem only happens *once* per Emacs session. After
that, I cannot seem to reproduce it again until I restart Emacs. All of
this points to a bug, in my opinion unless header-line is considered
deprecated or somehow falls into a realm of not being able to remap for
some reason.

Thanks,

[-- Attachment #2: Type: text/html, Size: 1253 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Wed, Dec 04, 2024 at 10:53 PM, Aaron Jensen <aaronjensen@gmail.com>
wrote:

> Furthermore, this problem only happens *once* per Emacs session. After
> that, I cannot seem to reproduce it again until I restart Emacs. All of
> this points to a bug, in my opinion unless header-line is considered
> deprecated or somehow falls into a realm of not being able to remap for
> some reason.
>

This may be obvious to you all, and me going much further probably isn't
realistic, but I am curious if there's a problem in the resolving of
inherited remapped faces while changing windows. Specifically that it
appears to use the lack of remap in the newly created window to resolve the
remap in the original window. The remapping of the active/inactive faces
happens explicitly inside display_mode_line / init_iterator with an
explicit window reference. I don't know enough to know where the inherited
faces get remapped, but if the window used to remap them is incorrect
(because it's the current window, rather than the window the mode/header
line is being drawn in), I could see there being the issue we're observing.

Not sure if that helps, but that's as far as I was able to get in my
investigation so far.

Thanks,

[-- Attachment #2: Type: text/html, Size: 2158 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: Aaron Jensen <aaronjensen@gmail.com>,  trevor.m.murphy@gmail.com,
>   73862@debbugs.gnu.org
> Date: Thu, 05 Dec 2024 07:50:38 +0100
> 
> > If what you see is the same as Eshel, I will ask you the same
> > question: shouldn't you apply face-remapping to the 2 new faces
> > instead of the 'header-line' face from which they both inherit?
> > What happens if you do define remapping for those two new faces?
> 
> At least to me, it's not clear what you mean by "should".  Existing code
> remaps the header-line face with good results (prior to this change), so
> if we now "should" remap something else instead to get the same results,
> that means this is a breaking change.  Is that intended?  If so, OK, if
> not, shouldn't it be fixed? :)

Let's first understand the scope of the problem, shall we?  If
remapping the two new faces does what you want, then the only problem
is in backward-incompatible nature of this change, when face remapping
is considered.  If remapping the two new faces does NOT do what you
want, the problem is elsewhere.

More to the point you raise: when we introduced mode-line-active face,
the same happened with the 2 mode-line faces.  We should indeed decide
whether we need to support remapping the parent mode-line and
header-line faces, but at least for mode line we don't, since Emacs
29.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Thu, 5 Dec 2024 01:53:30 -0500
> Cc: trevor.m.murphy@gmail.com, me@eshelyaron.com, 73862@debbugs.gnu.org
> 
>  If what you see is the same as Eshel, I will ask you the same question: shouldn't you apply
>  face-remapping to the 2 new faces instead of the 'header-line' face from which they both inherit? What
>  happens if you do define remapping for those two new faces?
> 
> My specific problem does not occur if I remap the two new faces. Why would I need to do that though? Both
> inherit from header-line, so if I wanted to change both, I would naturally change the base face.

Technically, because mode-line and header-line are no longer
considered "basic faces", and because the display code uses these
faces directly in C.

> Furthermore, this problem only happens *once* per Emacs session. After that, I cannot seem to reproduce it
> again until I restart Emacs. All of this points to a bug, in my opinion unless header-line is considered
> deprecated or somehow falls into a realm of not being able to remap for some reason. 

What exactly happens "once per session"?  Can you show some Lisp to
reproduce this "once" occurrence?

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Wed, 4 Dec 2024 23:29:32 -0800
> Cc: trevor.m.murphy@gmail.com, me@eshelyaron.com, 73862@debbugs.gnu.org
> 
>  Furthermore, this problem only happens *once* per Emacs session. After that, I cannot seem to
>  reproduce it again until I restart Emacs. All of this points to a bug, in my opinion unless header-line is
>  considered deprecated or somehow falls into a realm of not being able to remap for some reason. 
> 
> This may be obvious to you all, and me going much further probably isn't realistic, but I am curious if there's
> a problem in the resolving of inherited remapped faces while changing windows. Specifically that it appears
> to use the lack of remap in the newly created window to resolve the remap in the original window. The
> remapping of the active/inactive faces happens explicitly inside display_mode_line / init_iterator with an
> explicit window reference. I don't know enough to know where the inherited faces get remapped, but if the
> window used to remap them is incorrect (because it's the current window, rather than the window the
> mode/header line is being drawn in), I could see there being the issue we're observing.
> 
> Not sure if that helps, but that's as far as I was able to get in my investigation so far.

Once again, please show some simple Lisp to reproduce the phenomena
you are observing.  It is hard to discuss these highly technical
issues on this abstract level.

On the very basic level of the display code, when a display iterator
is initialized, the window for whose display the iterator is used must
be given to init_iterator as its argument, and the buffer of that
window must be temporarily made to be the current buffer.  So this
cannot be the problem in this case.  If init_iterator would be passed
an incorrect window, we'd have much more grave display problems than
this minor issue.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Wed, Dec 04, 2024 at 11:51 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> Once again, please show some simple Lisp to reproduce the phenomena you
> are observing. It is hard to discuss these highly technical issues on this
> abstract level.
>

Happy to. Sorry, I didn't think it was necessary given Eshel's providing of
the recipe. It wasn't lisp though, so here's the lisp version from emacs -Q:


   1. (setq header-line-format "Some Header")
   2. (face-remap-set-base 'header-line 'highlight)
   3. (set-face-attribute 'default nil :height (+ (face-attribute 'default
   :height) 10))
   4. (switch-to-buffer-other-window "new")
   5. From within new buffer/window: (set-face-attribute 'default nil
   :height (+ (face-attribute 'default :height) 10))

You'll see after step 3 the header line switches to the highlight face.
After step 5, the header line in the original buffer switches away from the
highlight face, which is unexpected.

If you then switch back to the original buffer and change the font size
again, the highlight will display again. Switching back to the new buffer
and changing the font size causes it to disappear again, so I was mistaken
about it only happening once. It just so happens that a full redisplay only
triggers once for me in my normal usage when I have a window without the
header line override specified.

On the very basic level of the display code, when a display iterator is
> initialized, the window for whose display the iterator is used must be
> given to init_iterator as its argument, and the buffer of that window must
> be temporarily made to be the current buffer. So this cannot be the problem
> in this case. If init_iterator would be passed an incorrect window, we'd
> have much more grave display problems than this minor issue.
>

Understood, but that's not what I was attempting to describe. I was trying
to say that the window passed to init_interator IS the correct window. And
that that is used to resolve the remap for the inactive/active faces.
However, it does not appear that that same window is used to resolve
remapping of faces that are inherited by the inactive/active faces.

From what I can tell, this only applies to header-line-active and
header-line-inactive (and their mode line equivalents) and it does not
apply to other faces in the header line that may inherit from remapped
faces, so that's good.

Also, I understand what you're saying that remapping mode-line was decided
in Emacs 29 to not be supported and the conclusion may be the same for
header-line. If that's what it needs to be, that's fine. As Eshel said, it
will be a breaking change because people have relied on this behavior.

Thanks,

[-- Attachment #2: Type: text/html, Size: 4528 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Thu, 5 Dec 2024 08:02:06 -0800
> Cc: trevor.m.murphy@gmail.com, me@eshelyaron.com, 73862@debbugs.gnu.org
> 
> On Wed, Dec 04, 2024 at 11:51 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > Once again, please show some simple Lisp to reproduce the phenomena you
> > are observing. It is hard to discuss these highly technical issues on this
> > abstract level.
> >
> 
> Happy to. Sorry, I didn't think it was necessary given Eshel's providing of
> the recipe.

I asked a recipe from you because you seemed to be reporting something
different: the happening once" part.

>    1. (setq header-line-format "Some Header")
>    2. (face-remap-set-base 'header-line 'highlight)
>    3. (set-face-attribute 'default nil :height (+ (face-attribute 'default
>    :height) 10))
>    4. (switch-to-buffer-other-window "new")
>    5. From within new buffer/window: (set-face-attribute 'default nil
>    :height (+ (face-attribute 'default :height) 10))
> 
> You'll see after step 3 the header line switches to the highlight face.
> After step 5, the header line in the original buffer switches away from the
> highlight face, which is unexpected.

Why are you changing the attributes of the default face in step 3 and
step 5 when what you want is to change the header-line face?  And do
you understand why step 2 didn't change the way header line is
displayed, but step 3 did?

> If you then switch back to the original buffer and change the font size
> again, the highlight will display again.

How come the font of the default face affects the colors of the
header-line faces?  Doesn't that already look strange?

> Switching back to the new buffer and changing the font size causes
> it to disappear again, so I was mistaken about it only happening
> once.

OK, so that mystery doesn't exist.  Good.

> > On the very basic level of the display code, when a display iterator is
> > initialized, the window for whose display the iterator is used must be
> > given to init_iterator as its argument, and the buffer of that window must
> > be temporarily made to be the current buffer. So this cannot be the problem
> > in this case. If init_iterator would be passed an incorrect window, we'd
> > have much more grave display problems than this minor issue.
> >
> 
> Understood, but that's not what I was attempting to describe. I was trying
> to say that the window passed to init_interator IS the correct window. And
> that that is used to resolve the remap for the inactive/active faces.
> However, it does not appear that that same window is used to resolve
> remapping of faces that are inherited by the inactive/active faces.

Face remapping is local to buffers, not to windows.  So which window
is passed to init_interator is not relevant to face remapping; the
buffer of that window is.

> From what I can tell, this only applies to header-line-active and
> header-line-inactive (and their mode line equivalents) and it does not
> apply to other faces in the header line that may inherit from remapped
> faces, so that's good.
> 
> Also, I understand what you're saying that remapping mode-line was decided
> in Emacs 29 to not be supported and the conclusion may be the same for
> header-line. If that's what it needs to be, that's fine. As Eshel said, it
> will be a breaking change because people have relied on this behavior.

I think there's no way to support what you expect cleanly.  The reason
is simple: inheritance in so-called "basic faces" is not taken into
account when face-remapping is considered for them.  Face inheritance
is considered when faces are merged, but basic faces are not merged
when they are used by the display engine for the respective parts of
the Emacs display (i.e., the header-line faces for the header line, as
opposed to just using the header-line faces for buffer text).  That
changing the attributes of the default faces has the effect of
propagating the inheritance through face-remapping is an accident, the
side effect of the particular implementation of when we recompute all
the basic faces anew.  It could be considered a bug on its own.

We could perhaps add special-purpose code that would consider face
inheritance for mode-line and header-line faces when remapping them,
but that would get is in a different mess:

 . mode-line-inactive inherits from mode-line only by default, on
   color displays it doesn't; so if you remap mode-line,
   mode-line-inactive will only be affected on monochrome displays
 . header-line also inherits from mode-line, albeit only by default,
   so remapping mode-line would _sometimes_ affect header-line (and
   also header-line-active and header-line-inactive) and sometimes not
 . if the user customizes some of the mode-line or header-line faces
   to not inherit or to always inherit, remapping mode-line or
   header-line will produce different results for different faces

So basically it's a royal mess, all of it caused by the fact that
people somehow expect that remapping the header-line face will affect
header-line-active and header-line-inactive, and similarly for
mode-line.  That was never supported for basic faces, and I think it
makes little sense to support it in the future, because the results
will certainly surprise someone.

So I tend to close this bug as wontfix, and just mention in the
documentation (NEWS at least) that people who remap header-line or
mode-line need now to remap the new -active and -inactive faces.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Thu, Dec 05, 2024 at 12:42 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Thu, 5 Dec 2024 08:02:06 -0800
> Cc: trevor.m.murphy@gmail.com, me@eshelyaron.com, 73862@debbugs.gnu.org
>
> On Wed, Dec 04, 2024 at 11:51 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>
> Once again, please show some simple Lisp to reproduce the phenomena you
> are observing. It is hard to discuss these highly technical issues on this
> abstract level.
>
> Happy to. Sorry, I didn't think it was necessary given Eshel's providing
> of the recipe.
>
> I asked a recipe from you because you seemed to be reporting something
> different: the happening once" part.
>
> 1. (setq header-line-format "Some Header")
> 2. (face-remap-set-base 'header-line 'highlight)
> 3. (set-face-attribute 'default nil :height (+ (face-attribute 'default
> :height) 10))
> 4. (switch-to-buffer-other-window "new")
> 5. From within new buffer/window: (set-face-attribute 'default nil
> :height (+ (face-attribute 'default :height) 10))
>
> You'll see after step 3 the header line switches to the highlight face.
> After step 5, the header line in the original buffer switches away from the
> highlight face, which is unexpected.
>
> Why are you changing the attributes of the default face in step 3 and step
> 5 when what you want is to change the header-line face? And do you
> understand why step 2 didn't change the way header line is displayed, but
> step 3 did?
>

Only to force a redisplay in a way that actually causes the inheritance to
be considered. Note that when you reproduce this same thing with the
mode-line, force-mode-line-update doesn't work to cause the inheritance to
be considered. There's something "special" about changing the font or the
geometry of the frame or something that's causing the consideration of
inheritance.


> If you then switch back to the original buffer and change the font size
> again, the highlight will display again.
>
> How come the font of the default face affects the colors of the
> header-line faces? Doesn't that already look strange?
>

The strange thing is that when and how the inheritance is taken into
account is not the same as for other faces. So yes, it already looks
strange — I shouldn't need to force a redisplay. Affecting the font of the
default face is just one means by which I can trigger the redisplay in a
way that causes the issue. The specifics of it are a red herring. When I
first noticed the issue it was as a result of a vterm popup window being
shown. No default font modifications were made. I don't know what the
specific trigger was, however.


> Switching back to the new buffer and changing the font size causes it to
> disappear again, so I was mistaken about it only happening once.
>
> OK, so that mystery doesn't exist. Good.
>
> On the very basic level of the display code, when a display iterator is
> initialized, the window for whose display the iterator is used must be
> given to init_iterator as its argument, and the buffer of that window must
> be temporarily made to be the current buffer. So this cannot be the problem
> in this case. If init_iterator would be passed an incorrect window, we'd
> have much more grave display problems than this minor issue.
>
> Understood, but that's not what I was attempting to describe. I was trying
> to say that the window passed to init_interator IS the correct window. And
> that that is used to resolve the remap for the inactive/active faces.
> However, it does not appear that that same window is used to resolve
> remapping of faces that are inherited by the inactive/active faces.
>
> Face remapping is local to buffers, not to windows. So which window is
> passed to init_interator is not relevant to face remapping; the buffer of
> that window is.
>

My mistake. Even so, the buffer isn't being taken into account
consistently, but you speak to that below.


> From what I can tell, this only applies to header-line-active and
> header-line-inactive (and their mode line equivalents) and it does not
> apply to other faces in the header line that may inherit from remapped
> faces, so that's good.
>
> Also, I understand what you're saying that remapping mode-line was decided
> in Emacs 29 to not be supported and the conclusion may be the same for
> header-line. If that's what it needs to be, that's fine. As Eshel said, it
> will be a breaking change because people have relied on this behavior.
>
> I think there's no way to support what you expect cleanly. The reason is
> simple: inheritance in so-called "basic faces" is not taken into account
> when face-remapping is considered for them. Face inheritance is considered
> when faces are merged, but basic faces are not merged when they are used by
> the display engine for the respective parts of the Emacs display (i.e., the
> header-line faces for the header line, as opposed to just using the
> header-line faces for buffer text). That changing the attributes of the
> default faces has the effect of propagating the inheritance through
> face-remapping is an accident, the side effect of the particular
> implementation of when we recompute all the basic faces anew. It could be
> considered a bug on its own.
>
> We could perhaps add special-purpose code that would consider face
> inheritance for mode-line and header-line faces when remapping them, but
> that would get is in a different mess:
>
> . mode-line-inactive inherits from mode-line only by default, on color
> displays it doesn't; so if you remap mode-line, mode-line-inactive will
> only be affected on monochrome displays
> . header-line also inherits from mode-line, albeit only by default, so
> remapping mode-line would _sometimes_ affect header-line (and also
> header-line-active and header-line-inactive) and sometimes not
> . if the user customizes some of the mode-line or header-line faces to not
> inherit or to always inherit, remapping mode-line or header-line will
> produce different results for different faces
>
> So basically it's a royal mess, all of it caused by the fact that people
> somehow expect that remapping the header-line face will affect
> header-line-active and header-line-inactive, and similarly for mode-line.
> That was never supported for basic faces, and I think it makes little sense
> to support it in the future, because the results will certainly surprise
> someone.
>
> So I tend to close this bug as wontfix, and just mention in the
> documentation (NEWS at least) that people who remap header-line or
> mode-line need now to remap the new -active and -inactive faces.
>

Ok, I get what you're saying here, but I don't know that I agree with this:
"...people somehow expect that remapping the header-line face will affect
header-line-active and header-line-inactive." I'm not sure why it wouldn't
be expected. It's reasonable to expect that inherit and face remapping
would work the same way always. I didn't even know there was a notion of
"basic faces" and what that meant. If that's somehow a special designation
that confers different behavior, then we shouldn't be surprised if people
are confused by its differences. I do not have these differences memorized,
and ultimately that's what it sounds like we are being asked to do. Yes,
you can remap faces, yes, you can inherit faces, and yes those two work
together EXCEPT in this one case. Even with documentation, this will be
confusing because we cannot rely on people finding that documentation.

If basic faces do not fully support inheritance, we probably shouldn't
allow them to have inheritance. That would mean that header-line-inactive
would be defined explicitly, without inheritance, and if someone attempted
to add an inherit property, it would fail.

This would mean that the base faces, header-line and mode-line would be
eliminated. Is it too much to ask for someone to explicitly configure the
four faces (mode-line-active, mode-line-inactive, header-line-active, and
header-line-inactive) any time they want to apply a theme to them? It
sounds like that's the expectation for remapping, so why not extend that to
customizing them?

If there were no header-line or mode-line face then I would not have run
into this. I would have been required to update my theme accordingly and
the code that I use to remap header line faces, but that would have been
apparent and seems like a reasonable change to make in a major version, but
I can't recommend any of this because I don't know enough about the version
criteria, release process, internals, etc, so please consider this just a
perspective.

Thanks,

[-- Attachment #2: Type: text/html, Size: 12196 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Thu, 5 Dec 2024 13:14:32 -0800
> Cc: trevor.m.murphy@gmail.com, me@eshelyaron.com, 73862@debbugs.gnu.org
> 
> > Why are you changing the attributes of the default face in step 3 and step
> > 5 when what you want is to change the header-line face? And do you
> > understand why step 2 didn't change the way header line is displayed, but
> > step 3 did?
> 
> Only to force a redisplay in a way that actually causes the inheritance to
> be considered. Note that when you reproduce this same thing with the
> mode-line, force-mode-line-update doesn't work to cause the inheritance to
> be considered. There's something "special" about changing the font or the
> geometry of the frame or something that's causing the consideration of
> inheritance.

That's because changing the default face causes recomputation of all
the faces, starting from the basic faces.  It is not directly related
to redisplay (which is why force-mode-line-update doesn't have that
effect), although, of course, the recomputation is triggered by the
next redisplay cycle.

> > If you then switch back to the original buffer and change the font size
> > again, the highlight will display again.
> >
> > How come the font of the default face affects the colors of the
> > header-line faces? Doesn't that already look strange?
> 
> The strange thing is that when and how the inheritance is taken into
> account is not the same as for other faces. So yes, it already looks
> strange — I shouldn't need to force a redisplay. Affecting the font of the
> default face is just one means by which I can trigger the redisplay in a
> way that causes the issue. The specifics of it are a red herring. When I
> first noticed the issue it was as a result of a vterm popup window being
> shown. No default font modifications were made. I don't know what the
> specific trigger was, however.

See above: I hope the situation is more clear now.

> > So I tend to close this bug as wontfix, and just mention in the
> > documentation (NEWS at least) that people who remap header-line or
> > mode-line need now to remap the new -active and -inactive faces.
> 
> Ok, I get what you're saying here, but I don't know that I agree with this:
> "...people somehow expect that remapping the header-line face will affect
> header-line-active and header-line-inactive." I'm not sure why it wouldn't
> be expected.

It shouldn't be expected for basic faces.

> It's reasonable to expect that inherit and face remapping
> would work the same way always.

It doesn't, and never was.  The current implementation of faces has a
built-in assumption that basic faces don't inherit from any other
faces, except the (implicit) inheritance from the default face.  So
face-remapping of basic faces doesn't take inheritance into account,
and never did.

> I didn't even know there was a notion of "basic faces" and what that
> meant.

As a Lisp programmer, you aren't supposed to know that, or care.  The
problem here is that, by making some basic faces inherit from other
faces, we caused this abstraction to leak.

> If basic faces do not fully support inheritance, we probably shouldn't
> allow them to have inheritance.

Yes.  But that train has left the station, unfortunately, and quite a
long time ago.  I don't think there's a way back.  Note that
mode-line-inactive was originally introduced _in_addition_ to
mode-line (which was then used only for the "active" mode-line), so
people were expected to customize/remap that new face _in_addition_ to
mode-line.  It's when we introduced mode-line-active, which _always_
inherits from mode-line, that this problem was first introduced.  And
now the two header-line faces, which basically repeat the same
paradigm, added yet another group of problems.

> That would mean that header-line-inactive
> would be defined explicitly, without inheritance, and if someone attempted
> to add an inherit property, it would fail.

Or we could do what the mode-line case did originally: leave the
active face to be header-line and define header-line-inactive without
inheritance.  This will at least let existing remapping of header-line
work as it did before.

For the same reasons, I would prefer to go back on the
mode-line-active change, but I'm not sure this is practical at this
point.

> This would mean that the base faces, header-line and mode-line would be
> eliminated.

No need to go that far.  These faces are still useful, because they
allow to define the inheriting faces more compactly, and allow to
change the definitions easier.  Inheritance still works on the defface
level, even for basic faces.

> Is it too much to ask for someone to explicitly configure the
> four faces (mode-line-active, mode-line-inactive, header-line-active, and
> header-line-inactive) any time they want to apply a theme to them? It
> sounds like that's the expectation for remapping, so why not extend that to
> customizing them?

Remapping and customizing are two different customizations, though.
They don't have to work the same.

> If there were no header-line or mode-line face then I would not have run
> into this.

Yes, but removing a face from Emacs is a much more significant
backward incompatibility than the problems discussed in this bug.

I added Stefan to this discussion, in the hope that he could have some
suggestions or comments.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Fri, Dec 06, 2024 at 12:55 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> See above: I hope the situation is more clear now.
>

Yes, thank you. I still don't know all of the different things that trigger
computation of faces, but I probably don't need to (and likely shouldn't
need to).

So I tend to close this bug as wontfix, and just mention in the
> documentation (NEWS at least) that people who remap header-line or
> mode-line need now to remap the new -active and -inactive faces.
>
> Ok, I get what you're saying here, but I don't know that I agree with
> this:
> "...people somehow expect that remapping the header-line face will affect
> header-line-active and header-line-inactive." I'm not sure why it wouldn't
> be expected.
>
> It shouldn't be expected for basic faces.
>

From a purely human, cognitive load, and/or usability perspective, it
should be expected. As designers of software we have to be careful not to
let our detailed knowledge of things influence how we think other people
who do not have that detailed knowledge will or should think about things.
I was speaking purely from a "principle of least surprise" perspective. All
I mean is that if there are 10 widgets, and 9 widgets work a certain way
when you push a button on them, it is human and natural to expect the 10th
to work that same way. It doesn't matter that the builder of the widget
refers to it as a "basic widget", because the user of the widget has never
come across that terminology, nor do they know what special variation it
confers.

It does sound like below you agree that the situation caused is
undesirable, I'm trying to speak to why I believe it is. But yes,
perhaps the train has left the station though.

Or we could do what the mode-line case did originally: leave the active
> face to be header-line and define header-line-inactive without inheritance.
> This will at least let existing remapping of header-line work as it did
> before.
>
> For the same reasons, I would prefer to go back on the mode-line-active
> change, but I'm not sure this is practical at this point.
>

I'd personally be fine with this. It would be obvious when an inactive
header line looked different than my header line I configured and I would
have to look into it. I'm guessing there's no way to apply
header-line-active and header-line-inactive to the actual formatted header
line in the same way a user would with propertize so that it would behave
like a non-basic face? I have no idea what that would entail, but I'm
assuming there's a special reason for treating basic faces specially
(perhaps it has to do with how init_iterator is initialized with it)

This would mean that the base faces, header-line and mode-line would be
> eliminated.
>
> No need to go that far. These faces are still useful, because they allow
> to define the inheriting faces more compactly, and allow to change the
> definitions easier. Inheritance still works on the defface level, even for
> basic faces.
>

I think you're saying above you'd prefer nearly the same thing, but
header-line would survive, rather than header-line-active, yes? The point
is that inheritance only "kind of" works for basic faces (from a user's
perspective), so no matter how we get rid of the default use of
inheritance, I think it would be better for users.

Thanks,

[-- Attachment #2: Type: text/html, Size: 5459 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

Would it be possible, aside from the remapping of the particular face being
built (like header-line-active) to disable remapping when building the
basic faces? It occurred to me that one way to see the problem is that
there is inconsistency in how the remapping are applied (because they are
per-buffer). If it were consistent that the remap was not applied to any
face that the basic face inherited from, then the problem wouldn't appear
to be as bad as it is. It would come across as a limitation, rather than
something that seems like a defect.

[-- Attachment #2: Type: text/html, Size: 891 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> Cc: trevor.m.murphy@gmail.com, me@eshelyaron.com, 73862@debbugs.gnu.org
> Date: Thu, 05 Dec 2024 22:42:58 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> We could perhaps add special-purpose code that would consider face
> inheritance for mode-line and header-line faces when remapping them,
> but that would get is in a different mess:
> 
>  . mode-line-inactive inherits from mode-line only by default, on
>    color displays it doesn't; so if you remap mode-line,
>    mode-line-inactive will only be affected on monochrome displays
>  . header-line also inherits from mode-line, albeit only by default,
>    so remapping mode-line would _sometimes_ affect header-line (and
>    also header-line-active and header-line-inactive) and sometimes not
>  . if the user customizes some of the mode-line or header-line faces
>    to not inherit or to always inherit, remapping mode-line or
>    header-line will produce different results for different faces
> 
> So basically it's a royal mess, all of it caused by the fact that
> people somehow expect that remapping the header-line face will affect
> header-line-active and header-line-inactive, and similarly for
> mode-line.  That was never supported for basic faces, and I think it
> makes little sense to support it in the future, because the results
> will certainly surprise someone.
> 
> So I tend to close this bug as wontfix, and just mention in the
> documentation (NEWS at least) that people who remap header-line or
> mode-line need now to remap the new -active and -inactive faces.

If we do prefer to support remapping mode-line and header-line faces,
then I can suggest the semi-kludgey "fix" below.  Is this better than
what we have now?

Note that it immediately reveals the problems I mentioned above.  For
example, if you remap header-line, both header-line-active and
header-line-inactive immediately follow suit, but if you remap
mode-line, only mode-line-active follows, whereas mode-line-inactive
only follows on TTY frames (because all other frames support :box).
So if we install this, we should probably change the definition of
mode-line-inactive in some way?

Stefan, any suggestions or comments?

diff --git a/src/xfaces.c b/src/xfaces.c
index f626480..9704c05 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -5144,6 +5144,19 @@ lookup_basic_face (struct window *w, struct frame *f, int face_id)
      for the very common no-remapping case.  */
   mapping = assq_no_quit (name, Vface_remapping_alist);
   if (NILP (mapping))
+    {
+      /* Special treatment for mode-line and header-line faces, for
+	 backward compatibility: people might remap 'mode-line' and
+	 'header-line' faces and expect the *-active and *-inactive
+	 faces to change accordingly.  */
+      if (face_id == MODE_LINE_ACTIVE_FACE_ID
+	  || face_id == MODE_LINE_INACTIVE_FACE_ID)
+	mapping = assq_no_quit (Qmode_line, Vface_remapping_alist);
+      else if (face_id == HEADER_LINE_ACTIVE_FACE_ID
+	       || face_id == HEADER_LINE_INACTIVE_FACE_ID)
+	mapping = assq_no_quit (Qheader_line, Vface_remapping_alist);
+    }
+  if (NILP (mapping))
     return face_id;		/* Give up.  */
 
   /* If there is a remapping entry, lookup the face using NAME, which will
@@ -7447,6 +7460,7 @@ syms_of_xfaces (void)
   DEFSYM (Qcursor, "cursor");
   DEFSYM (Qborder, "border");
   DEFSYM (Qmouse, "mouse");
+  /* Qmode_line is defined in keyboard.c  */
   DEFSYM (Qmode_line_inactive, "mode-line-inactive");
   DEFSYM (Qmode_line_active, "mode-line-active");
   DEFSYM (Qvertical_border, "vertical-border");

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Fri, 6 Dec 2024 11:28:53 -0500
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, trevor.m.murphy@gmail.com, me@eshelyaron.com, 
> 	73862@debbugs.gnu.org
> 
> Would it be possible, aside from the remapping of the particular face being
> built (like header-line-active) to disable remapping when building the
> basic faces? It occurred to me that one way to see the problem is that
> there is inconsistency in how the remapping are applied (because they are
> per-buffer). If it were consistent that the remap was not applied to any
> face that the basic face inherited from, then the problem wouldn't appear
> to be as bad as it is. It would come across as a limitation, rather than
> something that seems like a defect.

If we are going to change the C code related to basic faces, I'd
prefer the semi-kludgey change I sent a few minutes ago.  It at least
brings the effect closer to naïve user expectations.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Sat, Dec 07, 2024 at 1:50 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> If we do prefer to support remapping mode-line and header-line faces, then
> I can suggest the semi-kludgey "fix" below. Is this better than what we
> have now?
>

Does this patch make it so that remaps are considered for header-line even
if header-line-active no longer inherits from header-line? If so, I'd
consider that even more surprising. The more I think about this, the less
inclined I'd be to play special case whack-a-mole with it and the more I'd
be inclined to either "live with it" or figure out a way to disable
remapping entirely in certain rendering contexts. Tab bar mode is another
one that comes to mind that probably shouldn't use remaps at all when
rendering. We could treat the computing of basic faces similarly… aside
from the initial remap of *-inactive/active, remapping would stop being
considered. If that's not feasible, then I don't see this proposed patch as
better. Users have already made do with the header-line transition in 29 it
sounds like.

Thanks,

[-- Attachment #2: Type: text/html, Size: 1914 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 7 Dec 2024 07:28:33 -0600
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, trevor.m.murphy@gmail.com, me@eshelyaron.com, 
> 	73862@debbugs.gnu.org
> 
> On Sat, Dec 07, 2024 at 1:50 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > If we do prefer to support remapping mode-line and header-line faces, then
> > I can suggest the semi-kludgey "fix" below. Is this better than what we
> > have now?
> 
> Does this patch make it so that remaps are considered for header-line even
> if header-line-active no longer inherits from header-line?

It shouldn't.  If you apply it and see something like that, it should
be considered a bug somewhere (but I would be very surprised if it did
happen).

All this change does it give the code the chance to account for
remapping of header-line-active if header-line was remapped.  But if
the latter doesn't inherit from the former, that chance will not
produce anything that depends on header-line's remapping.

> The more I think about this, the less
> inclined I'd be to play special case whack-a-mole with it and the more I'd
> be inclined to either "live with it" or figure out a way to disable
> remapping entirely in certain rendering contexts.

Disabling remapping entirely is not feasible, since too many places
already account for remapping.

> Tab bar mode is another one that comes to mind that probably
> shouldn't use remaps at all when rendering.

Why not?

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Sat, Dec 07, 2024 at 7:02 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> It shouldn't. If you apply it and see something like that, it should be
> considered a bug somewhere (but I would be very surprised if it did happen).
>

Ok, I tested it and you're right, I couldn't see this happen.

What I did see happen was if I did this:

(custom-set-faces '(header-line-active ((t (:inherit highlight)))))
(face-remap-set-base 'highlight 'default)

Then the remapping doesn't work. I'm not surprised at this point, but it's
still "surprising".  Given it's highly unlikely people would do something
like this, one could get away with a patch like this most likely.

All this change does it give the code the chance to account for remapping
> of header-line-active if header-line was remapped. But if the latter
> doesn't inherit from the former, that chance will not produce anything that
> depends on header-line's remapping.
>

Ok, thanks. I can't say I understand the code yet, I'd have to study it

Tab bar mode is another one that comes to mind that probably shouldn't use
> remaps at all when rendering.
>
> Why not?
>

Because it's rendered outside of a single buffer. I tried to use it for
https://github.com/aaronjensen/emacs-modern-tab-bar and it was a bad idea.
I added hooks to set the remaps in every buffer, but once I had a child
frame open or was away from the window, the remaps would not be in effect.
I ended up doing it with a custom theme, which worked fine and was a better
idea. So yes, technically, they "work" and if one anted to make the tab bar
look different while a single buffer was selected, they could, so I retract
my suggestion to make remapping not work there.

Thanks,

[-- Attachment #2: Type: text/html, Size: 3465 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 7 Dec 2024 12:13:20 -0500
> Cc: monnier@iro.umontreal.ca, trevor.m.murphy@gmail.com, me@eshelyaron.com, 
> 	73862@debbugs.gnu.org
> 
> On Sat, Dec 07, 2024 at 7:02 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > It shouldn't. If you apply it and see something like that, it should be
> > considered a bug somewhere (but I would be very surprised if it did happen).
> >
> 
> Ok, I tested it and you're right, I couldn't see this happen.
> 
> What I did see happen was if I did this:
> 
> (custom-set-faces '(header-line-active ((t (:inherit highlight)))))
> (face-remap-set-base 'highlight 'default)
> 
> Then the remapping doesn't work.

Repeat after me: "basic faces cannot follow remapping due to face
inheritance".

They are called "basic" because they aren't supposed to inherit from
anything, but be used to inherit _from_.

The patch I posted is supposed to make Emacs be more
backward-compatible, in that people who used to remap header-line will
see their remapping propagate to header-line-active etc., but only as
long as they inherit from header-line, which they do by default.
Making header-line inherit from highlight didn't work before, and
should not be expected to work now.

If we install the patch I posted, I wouldn't even document this
special handling of these faces, because its only purpose is to help
with backward compatibility.

> I'm not surprised at this point, but it's still "surprising".

It had never worked!  And was not supposed to work!

> Given it's highly unlikely people would do something
> like this, one could get away with a patch like this most likely.

If people were doing something like that, they would be complaining
long ago.

> > Tab bar mode is another one that comes to mind that probably shouldn't use
> > remaps at all when rendering.
> >
> > Why not?
> 
> Because it's rendered outside of a single buffer.

It doesn't have to be.  It's just a face.  Granted, when a face is
used without relation to a buffer, then using a buffer-local
customization of it (which is what face-remapping is) is not a good
idea.

As with many Emacs features, users shoot themselves in the foot by
(ab)using the features outside of their intended design space, and
then complain that things fall apart.  Emacs trusts the users that
they know what they are doing, although that trust is not always
justified, it seems...

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Sat, Dec 07, 2024 at 10:25 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> Repeat after me: "basic faces cannot follow remapping due to face
> inheritance".
>

Oh, I know that *now*. I also know what basic faces are *now*. I'm no
longer ignorant to this, and, for what it's worth, this came across as rude
given the circumstances. I'm not being obstinate here. I'm not expecting
Emacs to do something it never did. I'm only representing the beginner's
mind. The thing that we *should* be considering when designing tools.
Developers "knowing better" than users is what makes so much of the
software we use garbage. Emacs isn't garbage, but I'm trying to communicate
that this behavior is surprising because there is no explicit (that I know
of) differentiation between "basic" faces and non-"basic" faces.

You said this about me not knowing about basic faces: "As a Lisp
programmer, you aren't supposed to know that, or care."

And I'm saying that as someone who does not know what a basic face is, how
in the world can I be expected to know or intuit that this classification I
do not know exist means that inheriting and remapping doesn't work? That is
my *only* point.

They are called "basic" because they aren't supposed to inherit from
> anything, but be used to inherit _from_.
>

If this were *prevented* or a warning in some way, this would solve all of
the problems. If this is really what they are, and that were codified and
explicit, the guard rails would be in place to teach the beginner users. I
would prefer this to something that enables backwards compatibility, but
that ship may have sunk (sailed) already. A combination of the two things
could work as well (explicit backwards compatibility, for mode-line and
header-line faces, but a warning when "basic" faces inherit from anything
else).

The patch I posted is supposed to make Emacs be more backward-compatible,
> in that people who used to remap header-line will see their remapping
> propagate to header-line-active etc., but only as long as they inherit from
> header-line, which they do by default. Making header-line inherit from
> highlight didn't work before, and should not be expected to work now.
>
> If we install the patch I posted, I wouldn't even document this special
> handling of these faces, because its only purpose is to help with backward
> compatibility.
>
> I'm not surprised at this point, but it's still "surprising".
>
> It had never worked! And was not supposed to work!
>

I believe you are misunderstanding my use of the word "surprising". I hope
it's more clear now.

As with many Emacs features, users shoot themselves in the foot by
> (ab)using the features outside of their intended design space, and then
> complain that things fall apart. Emacs trusts the users that they know what
> they are doing, although that trust is not always justified, it seems...
>
>
Aye, even someone who has been using Emacs for nearly 10 years does not
know everything about customizing it and building packages for it and has
to learn through trial and error. It's part of what makes Emacs Emacs. I
learned something about face remapping and themes in building that package
and I'm glad to know it.

That said, as software designers, we are not supposed to be frustrated be
insulting when our users complain "that things fall apart", nor should we
think of it as trust not being justified. We can miss the target too. If
our designs are not intention revealing, or are "surprising", we should
recognize that as feedback and recognize we may have gotten something
wrong. Our users are doing us a favor by giving us feedback in that moment.
Belittling them doesn't get us anywhere good, in my experience. There will
always be users that we could never "dumb things down" enough for, and if
that's how I'm coming across right now, I apologize.

Aaron

[-- Attachment #2: Type: text/html, Size: 6457 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 7 Dec 2024 13:46:33 -0500
> Cc: monnier@iro.umontreal.ca, trevor.m.murphy@gmail.com, me@eshelyaron.com, 
> 	73862@debbugs.gnu.org
> 
> On Sat, Dec 07, 2024 at 10:25 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > Repeat after me: "basic faces cannot follow remapping due to face
> > inheritance".
> >
> 
> Oh, I know that *now*. I also know what basic faces are *now*. I'm no
> longer ignorant to this, and, for what it's worth, this came across as rude
> given the circumstances. I'm not being obstinate here. I'm not expecting
> Emacs to do something it never did. I'm only representing the beginner's
> mind. The thing that we *should* be considering when designing tools.
> Developers "knowing better" than users is what makes so much of the
> software we use garbage. Emacs isn't garbage, but I'm trying to communicate
> that this behavior is surprising because there is no explicit (that I know
> of) differentiation between "basic" faces and non-"basic" faces.
> 
> You said this about me not knowing about basic faces: "As a Lisp
> programmer, you aren't supposed to know that, or care."
> 
> And I'm saying that as someone who does not know what a basic face is, how
> in the world can I be expected to know or intuit that this classification I
> do not know exist means that inheriting and remapping doesn't work? That is
> my *only* point.
> 
> They are called "basic" because they aren't supposed to inherit from
> > anything, but be used to inherit _from_.
> >
> 
> If this were *prevented* or a warning in some way, this would solve all of
> the problems. If this is really what they are, and that were codified and
> explicit, the guard rails would be in place to teach the beginner users. I
> would prefer this to something that enables backwards compatibility, but
> that ship may have sunk (sailed) already. A combination of the two things
> could work as well (explicit backwards compatibility, for mode-line and
> header-line faces, but a warning when "basic" faces inherit from anything
> else).
> 
> The patch I posted is supposed to make Emacs be more backward-compatible,
> > in that people who used to remap header-line will see their remapping
> > propagate to header-line-active etc., but only as long as they inherit from
> > header-line, which they do by default. Making header-line inherit from
> > highlight didn't work before, and should not be expected to work now.
> >
> > If we install the patch I posted, I wouldn't even document this special
> > handling of these faces, because its only purpose is to help with backward
> > compatibility.
> >
> > I'm not surprised at this point, but it's still "surprising".
> >
> > It had never worked! And was not supposed to work!
> >
> 
> I believe you are misunderstanding my use of the word "surprising". I hope
> it's more clear now.
> 
> As with many Emacs features, users shoot themselves in the foot by
> > (ab)using the features outside of their intended design space, and then
> > complain that things fall apart. Emacs trusts the users that they know what
> > they are doing, although that trust is not always justified, it seems...
> >
> >
> Aye, even someone who has been using Emacs for nearly 10 years does not
> know everything about customizing it and building packages for it and has
> to learn through trial and error. It's part of what makes Emacs Emacs. I
> learned something about face remapping and themes in building that package
> and I'm glad to know it.
> 
> That said, as software designers, we are not supposed to be frustrated be
> insulting when our users complain "that things fall apart", nor should we
> think of it as trust not being justified. We can miss the target too. If
> our designs are not intention revealing, or are "surprising", we should
> recognize that as feedback and recognize we may have gotten something
> wrong. Our users are doing us a favor by giving us feedback in that moment.
> Belittling them doesn't get us anywhere good, in my experience. There will
> always be users that we could never "dumb things down" enough for, and if
> that's how I'm coming across right now, I apologize.

I'm sorry you interpret what I wrote as rude, or insulting, or
anything of that kind.  If anything, it was supposed to be mildly
humorous.  And when I say "you aren't supposed to", I don't mean you
personally.

So this is all a huge misunderstanding.  My only motivation was to
explain what you see and maybe make Emacs a tad better, that's all.

Sorry.

P.S. My personal conclusion from this, and from many past bug reports
that causes us to add explicit remapping in many places, is that
face-remapping was a clever hack that should not have been allowed to
happen without a thorough rewrite of all the basic code which supports
faces and their merging.  Hindsight is always 20/20.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Sat, Dec 07, 2024 at 10:59 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> I'm sorry you interpret what I wrote as rude, or insulting, or anything of
> that kind. If anything, it was supposed to be mildly humorous. And when I
> say "you aren't supposed to", I don't mean you personally.
>

Thank you, it's all good and I'm glad it was only a misinterpretation on my
part. For what it's worth, I do appreciate the *massive* amount of user
engagement choose to do. Thank you for that and for everything else you do
for Emacs.

P.S. My personal conclusion from this, and from many past bug reports that
> causes us to add explicit remapping in many places, is that face-remapping
> was a clever hack that should not have been allowed to happen without a
> thorough rewrite of all the basic code which supports faces and their
> merging. Hindsight is always 20/20.
>

That's a very interesting conclusion, and it makes sense.

Would it make sense to extend the face doc strings that should not use
inheritance to indicate that? That combined with the backwards
compatibility fix you suggested may very well be a good compromise for the
current circumstance.

Alternatively, if there were a path to deprecating header-line and
mode-line then the compatibility fix could be skipped.

Thanks,

Aaron

[-- Attachment #2: Type: text/html, Size: 2527 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 7 Dec 2024 14:06:35 -0500
> Cc: monnier@iro.umontreal.ca, trevor.m.murphy@gmail.com, me@eshelyaron.com, 
> 	73862@debbugs.gnu.org
> 
> Would it make sense to extend the face doc strings that should not use
> inheritance to indicate that?

I'm not sure.  Inheritance does work for the basic faces, it's just
that face-remapping doesn't get passed by inheritance.

> Alternatively, if there were a path to deprecating header-line and
> mode-line then the compatibility fix could be skipped.

Let's see what Stefan and others think about this.

We also haven't yet heard from Eshel.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Sat, Dec 07, 2024 at 11:19 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 7 Dec 2024 14:06:35 -0500
> Cc: monnier@iro.umontreal.ca, trevor.m.murphy@gmail.com, me@eshelyaron.com,
> 73862@debbugs.gnu.org
>
> Would it make sense to extend the face doc strings that should not use
> inheritance to indicate that?
>
> I'm not sure. Inheritance does work for the basic faces, it's just that
> face-remapping doesn't get passed by inheritance.
>

Yeah, I guess that'd be throwing the baby out with the bathwater. I retract.

Another possibility would be to issue a warning when attempting to remap
mode-line or header-line. The user would at least see that what they're
doing is fraught. I believe this would require reconciling the terminal vs
GUI difference for mode-line you mentioned earlier. This would still be
adding specialization in a generalized place, and arguably that's a worse
place to do it than where you added it in your patch, so feel free to
disregard that idea.

Thanks,

Aaron

[-- Attachment #2: Type: text/html, Size: 2573 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Aaron Jensen <aaronjensen@gmail.com>
>> Date: Sat, 7 Dec 2024 14:06:35 -0500
>> Cc: monnier@iro.umontreal.ca, trevor.m.murphy@gmail.com, me@eshelyaron.com, 
>> 	73862@debbugs.gnu.org
>> 
>> Would it make sense to extend the face doc strings that should not use
>> inheritance to indicate that?
>
> I'm not sure.  Inheritance does work for the basic faces, it's just
> that face-remapping doesn't get passed by inheritance.
>
>> Alternatively, if there were a path to deprecating header-line and
>> mode-line then the compatibility fix could be skipped.
>
> Let's see what Stefan and others think about this.
>
> We also haven't yet heard from Eshel.

I don't have any concrete suggestions at this point.  It's surely a
complicated situation.

The workaround of remapping both header-line-active and
header-line-inactive instead of header-line seems to work.  The way I
see it, the essence of the issue is that the "face" abstraction is
leaky, in the sense that some faces (these "basic faces") behave
differently from other faces in some cases that involve inheritance and
remapping.  I think this calls for either changing the way basic faces
are handled so they do behave like any other face, or clearly
documenting their subtleties.


Eshel

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: Aaron Jensen <aaronjensen@gmail.com>,  monnier@iro.umontreal.ca,
>   trevor.m.murphy@gmail.com,  73862@debbugs.gnu.org
> Date: Sun, 08 Dec 2024 15:11:07 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > We also haven't yet heard from Eshel.
> 
> I don't have any concrete suggestions at this point.  It's surely a
> complicated situation.

Did you try the patch I posted, and if you did, did it resolve your
original problems?  That's what I wanted to hear, since you were one
of the two persons who raised these issues.

> The workaround of remapping both header-line-active and
> header-line-inactive instead of header-line seems to work.  The way I
> see it, the essence of the issue is that the "face" abstraction is
> leaky, in the sense that some faces (these "basic faces") behave
> differently from other faces in some cases that involve inheritance and
> remapping.

AFAIK, only face-remapping behaves differently, and only when a basic
face inherits from another.  The inheritance itself work as with other
faces.

> I think this calls for either changing the way basic faces are
> handled so they do behave like any other face, or clearly
> documenting their subtleties.

Patches to rework the faces support so that these idiosyncrasies are
removed will be most welcome.  I just don't plan on doing that myself,
as I won't have time for that.

TIA

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: Aaron Jensen <aaronjensen@gmail.com>,  monnier@iro.umontreal.ca,
>>   trevor.m.murphy@gmail.com,  73862@debbugs.gnu.org
>> Date: Sun, 08 Dec 2024 15:11:07 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > We also haven't yet heard from Eshel.
>> 
>> I don't have any concrete suggestions at this point.  It's surely a
>> complicated situation.
>
> Did you try the patch I posted, and if you did, did it resolve your
> original problems?  

I did now, and it doesn't entirely solve the problem: with this patch,
after remapping header-line in one buffer, the remapping now works for
that buffer as expected, but it still also "spreads" to other buffers,
which is unexpected.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Aaron Jensen]

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

On Sun, Dec 08, 2024 at 8:29 AM, Eshel Yaron <me@eshelyaron.com> wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
> From: Eshel Yaron <me@eshelyaron.com>
> Cc: Aaron Jensen <aaronjensen@gmail.com>, monnier@iro.umontreal.ca,
> trevor.m.murphy@gmail.com, 73862@debbugs.gnu.org
> Date: Sun, 08 Dec 2024 15:11:07 +0100
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> We also haven't yet heard from Eshel.
>
> I don't have any concrete suggestions at this point. It's surely a
> complicated situation.
>
> Did you try the patch I posted, and if you did, did it resolve your
> original problems?
>
> I did now, and it doesn't entirely solve the problem: with this patch,
> after remapping header-line in one buffer, the remapping now works for that
> buffer as expected, but it still also "spreads" to other buffers, which is
> unexpected.
>

I see the leak now as well. When I tested it at first, I did not see it
perhaps because I did not switch back to the original window and trigger a
computation of the basic faces. That's what causes the leak.

Steps:

(setq header-line-format "Some header")
(face-remap-set-base 'header-line 'highlight)
(switch-to-buffer-other-window "new")
;; In new buffer/window:
(setq header-line-format "Some header")
(other-window)
;; In original buffer/window:
(set-face-attribute 'default nil :height (+ (face-attribute 'default
:height) 10))

Aaron

[-- Attachment #2: Type: text/html, Size: 3556 bytes --]

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: aaronjensen@gmail.com,  monnier@iro.umontreal.ca,
>   trevor.m.murphy@gmail.com,  73862@debbugs.gnu.org
> Date: Sun, 08 Dec 2024 17:29:04 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Eshel Yaron <me@eshelyaron.com>
> >> Cc: Aaron Jensen <aaronjensen@gmail.com>,  monnier@iro.umontreal.ca,
> >>   trevor.m.murphy@gmail.com,  73862@debbugs.gnu.org
> >> Date: Sun, 08 Dec 2024 15:11:07 +0100
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> 
> >> > We also haven't yet heard from Eshel.
> >> 
> >> I don't have any concrete suggestions at this point.  It's surely a
> >> complicated situation.
> >
> > Did you try the patch I posted, and if you did, did it resolve your
> > original problems?  
> 
> I did now, and it doesn't entirely solve the problem: with this patch,
> after remapping header-line in one buffer, the remapping now works for
> that buffer as expected, but it still also "spreads" to other buffers,
> which is unexpected.

What do you mean by "spreads to other buffers"?  When you remapped
header-line face before this change on master, did it behave
differently, and if so, how?

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: aaronjensen@gmail.com,  monnier@iro.umontreal.ca,
>>   trevor.m.murphy@gmail.com,  73862@debbugs.gnu.org
>> Date: Sun, 08 Dec 2024 17:29:04 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Did you try the patch I posted, and if you did, did it resolve your
>> > original problems?  
>> 
>> I did now, and it doesn't entirely solve the problem: with this patch,
>> after remapping header-line in one buffer, the remapping now works for
>> that buffer as expected, but it still also "spreads" to other buffers,
>> which is unexpected.
>
> What do you mean by "spreads to other buffers"?  When you remapped
> header-line face before this change on master, did it behave
> differently, and if so, how?

Yes, before the introduction of header-line-(in)active, remapping
header-line buffer-locally in buffer A with face-remap-add-relative
would only affect the appearance of the header line in windows that show
buffer A.  Now, with this change and your patch, doing so can also
affect header lines in other windows, that display other buffers.

Aaron shared a recipe to reproduce this behavior, copied here:

--8<---------------cut here---------------start------------->8---
(setq header-line-format "Some header")
(face-remap-set-base 'header-line 'highlight)
(switch-to-buffer-other-window "new")
;; In new buffer/window:
(setq header-line-format "Some header")
(other-window 1)
;; In original buffer/window:
(set-face-attribute 'default nil :height (+ (face-attribute 'default :height) 10))
--8<---------------cut here---------------end--------------->8---


Eshel

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: trevor.m.murphy@gmail.com,  monnier@iro.umontreal.ca,
>   aaronjensen@gmail.com,  73862@debbugs.gnu.org
> Date: Sun, 08 Dec 2024 21:56:33 +0100
> 
> Aaron shared a recipe to reproduce this behavior, copied here:
> 
> --8<---------------cut here---------------start------------->8---
> (setq header-line-format "Some header")
> (face-remap-set-base 'header-line 'highlight)
> (switch-to-buffer-other-window "new")
> ;; In new buffer/window:
> (setq header-line-format "Some header")
> (other-window 1)
> ;; In original buffer/window:
> (set-face-attribute 'default nil :height (+ (face-attribute 'default :height) 10))
> --8<---------------cut here---------------end--------------->8---

Are you also change the attributes of the default face to cause the
"spreading"?  If not, please show a recipe.  Because I was unable to
see this without something like the last line of the recipe, and also
that line _must_ be in the buffer where header-line is remapped,
otherwise the effect is not seen.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi, 

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: trevor.m.murphy@gmail.com,  monnier@iro.umontreal.ca,
>>   aaronjensen@gmail.com,  73862@debbugs.gnu.org
>> Date: Sun, 08 Dec 2024 21:56:33 +0100
>> 
>> Aaron shared a recipe to reproduce this behavior, copied here:
>> 
>> --8<---------------cut here---------------start------------->8---
>> (setq header-line-format "Some header")
>> (face-remap-set-base 'header-line 'highlight)
>> (switch-to-buffer-other-window "new")
>> ;; In new buffer/window:
>> (setq header-line-format "Some header")
>> (other-window 1)
>> ;; In original buffer/window:
>> (set-face-attribute 'default nil :height (+ (face-attribute 'default :height) 10))
>> --8<---------------cut here---------------end--------------->8---
>
> Are you also change the attributes of the default face to cause the
> "spreading"?  If not, please show a recipe.  Because I was unable to
> see this without something like the last line of the recipe, and also
> that line _must_ be in the buffer where header-line is remapped,
> otherwise the effect is not seen.

Yes, I used C-x C-M-= in the buffer in which I remapped header-line.

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eshel Yaron writes:

> Hi, 
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Eshel Yaron <me@eshelyaron.com>
>>> Cc: trevor.m.murphy@gmail.com,  monnier@iro.umontreal.ca,
>>>   aaronjensen@gmail.com,  73862@debbugs.gnu.org
>>> Date: Sun, 08 Dec 2024 21:56:33 +0100
>>> 
>>> Aaron shared a recipe to reproduce this behavior, copied here:
>>> 
>>> --8<---------------cut here---------------start------------->8---
>>> (setq header-line-format "Some header")
>>> (face-remap-set-base 'header-line 'highlight)
>>> (switch-to-buffer-other-window "new")
>>> ;; In new buffer/window:
>>> (setq header-line-format "Some header")
>>> (other-window 1)
>>> ;; In original buffer/window:
>>> (set-face-attribute 'default nil :height (+ (face-attribute 'default :height) 10))
>>> --8<---------------cut here---------------end--------------->8---
>>
>> Are you also change the attributes of the default face to cause the
>> "spreading"?  If not, please show a recipe.  Because I was unable to
>> see this without something like the last line of the recipe, and also
>> that line _must_ be in the buffer where header-line is remapped,
>> otherwise the effect is not seen.
>
> Yes, I used C-x C-M-= in the buffer in which I remapped header-line.

Note that this issue also affects Org: org-columns remaps header-line in
org-columns--display-here.  (CC'ing Ihor.)


Best,

Eshel

bug#73862: [PATCH] Add `header-line-active` and `header-line-inactive` faces.

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  trevor.m.murphy@gmail.com,
>   monnier@iro.umontreal.ca,  aaronjensen@gmail.com,  73862@debbugs.gnu.org,
>  Ihor Radchenko <yantar92@posteo.net>
> Date: Sat, 14 Dec 2024 10:45:45 +0100
> 
> Eshel Yaron writes:
> 
> >>> Aaron shared a recipe to reproduce this behavior, copied here:
> >>> 
> >>> --8<---------------cut here---------------start------------->8---
> >>> (setq header-line-format "Some header")
> >>> (face-remap-set-base 'header-line 'highlight)
> >>> (switch-to-buffer-other-window "new")
> >>> ;; In new buffer/window:
> >>> (setq header-line-format "Some header")
> >>> (other-window 1)
> >>> ;; In original buffer/window:
> >>> (set-face-attribute 'default nil :height (+ (face-attribute 'default :height) 10))
> >>> --8<---------------cut here---------------end--------------->8---
> >>
> >> Are you also change the attributes of the default face to cause the
> >> "spreading"?  If not, please show a recipe.  Because I was unable to
> >> see this without something like the last line of the recipe, and also
> >> that line _must_ be in the buffer where header-line is remapped,
> >> otherwise the effect is not seen.
> >
> > Yes, I used C-x C-M-= in the buffer in which I remapped header-line.
> 
> Note that this issue also affects Org: org-columns remaps header-line in
> org-columns--display-here.  (CC'ing Ihor.)

Please try the patch below.  If it gives good results, please run with
it for awhile, and tell if you see anything unusual or unexpected.

diff --git a/src/xfaces.c b/src/xfaces.c
index 5f60f38..9591a5c 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -5143,10 +5143,19 @@ lookup_basic_face (struct window *w, struct frame *f, int face_id)
      for the very common no-remapping case.  */
   mapping = assq_no_quit (name, Vface_remapping_alist);
   if (NILP (mapping))
-    return face_id;		/* Give up.  */
+    {
+      Lisp_Object face_attrs[LFACE_VECTOR_SIZE];
+
+      /* If the face inherits from another, we need to realize it,
+         because the parent face could be remapped.  */
+      if (!get_lface_attributes (w, f, name, face_attrs, false, 0)
+	  || NILP (face_attrs[LFACE_INHERIT_INDEX])
+	  || UNSPECIFIEDP (face_attrs[LFACE_INHERIT_INDEX]))
+	return face_id;		/* Give up.  */
+    }
 
-  /* If there is a remapping entry, lookup the face using NAME, which will
-     handle the remapping too.  */
+  /* If there is a remapping entry, or the face inherits from another,
+     lookup the face using NAME, which will handle the remapping too.  */
   remapped_face_id = lookup_named_face (w, f, name, false);
   if (remapped_face_id < 0)
     return face_id;		/* Give up. */
@@ -5863,6 +5872,11 @@ realize_basic_faces (struct frame *f)
 
   if (realize_default_face (f))
     {
+      /* Basic faces must be realized disregarding face-remapping-alist,
+         since otherwise face-remapping might affect the basiic faces in
+         the face cache.  */
+      specpdl_ref count = SPECPDL_INDEX ();
+      specbind (Qface_remapping_alist, Qnil);
       realize_named_face (f, Qmode_line_active, MODE_LINE_ACTIVE_FACE_ID);
       realize_named_face (f, Qmode_line_inactive, MODE_LINE_INACTIVE_FACE_ID);
       realize_named_face (f, Qtool_bar, TOOL_BAR_FACE_ID);
@@ -5884,6 +5898,7 @@ realize_basic_faces (struct frame *f)
       realize_named_face (f, Qchild_frame_border, CHILD_FRAME_BORDER_FACE_ID);
       realize_named_face (f, Qtab_bar, TAB_BAR_FACE_ID);
       realize_named_face (f, Qtab_line, TAB_LINE_FACE_ID);
+      unbind_to (count, Qnil);
 
       /* Reflect changes in the `menu' face in menu bars.  */
       if (FRAME_FACE_CACHE (f)->menu_face_changed_p)