bug#63825: 29.0.90; The header line should be hidden when empty

[Eshel Yaron via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>>
>> Indeed, my patch doesn't change `format-mode-line`.  I removed this part
>> because AFAICT it's wrong: it suggests that if `format-mode-line`
>> returns the empty string with some argument, then using that argument as
>> the value of `header-line-format` will result in no header line at all.
>> But that's not the case (and it wasn't before my patch), because
>> `(format-mode-line header-line-format)` returning an empty string means
>> that the header line is either absent or empty, not necessarily absent.
>
> This is not what the text wants to convey.  It wants to say that if a
> window has no header-line, i.e. header-line-format is nil,
> format-mode-line returns an empty string.  This is true, before and
> after your changes.
>

Okay, I'm removing this change.  FTR I still find the remark that I
proposed removing slightly misleading.  Another option would be changing
the current "@code{""} if it has no header line" to "@code{""} if it has
an empty or no header line" instead of removing it.

>> +  if (CONSP (fmt))
>> +    {
>> +      car = XCAR (fmt);
>> +      if (SYMBOLP (car))
>> +	{
>> +	  if (EQ (car, QCeval)
>> +	      && NILP (safe_eval_inhibit_quit (XCAR (XCDR (fmt)))))
>> +	      return true;

> The indentation of "return true;" seems incorrect here.

Fixed.  Thanks for spotting it.

> Are you using a non-default setup of C indentation levels?

I don't think so, this one just slipped by I guess.

>> @@ -5495,8 +5534,9 @@ window_wants_header_line (struct window *w)
>>        && !MINI_WINDOW_P (w)
>>        && !WINDOW_PSEUDO_P (w)
>>        && !EQ (window_header_line_format, Qnone)
>> -	  && (!NILP (window_header_line_format)
>> -	      || !NILP (BVAR (XBUFFER (WINDOW_BUFFER (w)), header_line_format)))
>> +	  && (!null_header_line_format (window_header_line_format)
>> +	      || !null_header_line_format (BVAR (XBUFFER (WINDOW_BUFFER (w)),
>> +						 header_line_format)))
>>        && (WINDOW_PIXEL_HEIGHT (w)
>>            > (window_wants_mode_line (w)
>>               ? 2 * WINDOW_FRAME_LINE_HEIGHT (w)
>
> One more issue (sorry I didn't notice it before): the :eval form can
> potentially delete the window's frame.  See this part of
> display_mode_element:
>
>           if (CONSP (XCDR (elt)))
>             {
>               Lisp_Object spec;
>               spec = safe__eval (true, XCAR (XCDR (elt)));
>               /* The :eval form could delete the frame stored in the
>                  iterator, which will cause a crash if we try to
>                  access faces and other fields (e.g., FRAME_KBOARD)
>                  on that frame.  This is a nonsensical thing to do,
>                  and signaling an error from redisplay might be
>                  dangerous, but we cannot continue with an invalid frame.  */
>               if (!FRAME_LIVE_P (it->f))
>                 signal_error (":eval deleted the frame being displayed", elt);
>               n += display_mode_element (it, depth, field_width - n,
>                                          precision - n, spec, props,
>                                          risky);
>             }
>
> So I think we need to add to null_header_line_format a test for the
> window's frame to be live, to be on the safe side.  For that to work,
> the function should accept an additional argument: a pointer to the
> frame of the window whose header-line we are considering.
>

I see, done.

>> @@ -27408,7 +27414,7 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
>>          if (CONSP (XCDR (elt)))
>>            {
>>              Lisp_Object spec;
>> -		spec = safe__eval (true, XCAR (XCDR (elt)));
>> +		spec = safe_eval_inhibit_quit (XCAR (XCDR (elt)));
>>              /* The :eval form could delete the frame stored in the
>>                 iterator, which will cause a crash if we try to
>>                 access faces and other fields (e.g., FRAME_KBOARD)
>
> I see no reason to make this replacement.  Calling a static function
> lets the compiler optimize more aggressively than calling an
> non-static one.

Alright.  Here's an updated patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v3-0001-Avoid-header-line-with-some-empty-non-nil-formats.patch --]
[-- Type: text/x-patch, Size: 5931 bytes --]

From 98d0722d1e6e5d9d5b877f7df7e8ccd624ca9bd5 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <eshelyaron@dazzs-mbp.home>
Date: Sun, 4 Jun 2023 19:41:20 +0300
Subject: [PATCH v3] Avoid header line with some empty non-nil formats

Allow the value of 'header-line-format' to indicate that no header
line should be displayed when it trivially yields 'nil', even if it is
not plain 'nil'.  Previously, any non-nil 'header-line-format'
resulted in a (possibly empty) header line.  This change adds some
flexibility by also taking a non-nil value of 'header-line-format' to
mean that no header line should be displayed if it's a list whose
'car' is a symbol and either that symbol is ':eval' and the second
list element evaluates to 'nil', or the symbol's value as a variable
is 'nil' or void.
(Bug#63825)

* src/xdisp.c (safe_eval_inhibit_quit): New function.
* src/lisp.h (safe_eval_inhibit_quit): Declare it.
* src/window.c (null_header_line_format): New function.
(window_wants_header_line): Use it.

* doc/lispref/modes.texi (Header Line): Update to reflect new
conditions for displaying a window's header line.

* etc/NEWS: Announce updated treatment of 'header-line-format'.
---
 doc/lispref/modes.texi |  9 ++++++++
 etc/NEWS               |  8 +++++++
 src/lisp.h             |  1 +
 src/window.c           | 50 ++++++++++++++++++++++++++++++++++++++++--
 src/xdisp.c            |  6 +++++
 5 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/doc/lispref/modes.texi b/doc/lispref/modes.texi
index c2698da6d99..d2a05fe29e4 100644
--- a/doc/lispref/modes.texi
+++ b/doc/lispref/modes.texi
@@ -2597,6 +2597,15 @@ Header Lines
 line.
 @end defvar
 
+Emacs displays the header line for a window unless
+@code{header-line-format} is either @code{nil}, or it's a list whose
+@sc{car} is a symbol, and either that symbol is @code{:eval} and the
+second list element evaluates to @code{nil} or the symbol's value as a
+variable is @code{nil} or void.  Note that there are other possible
+values @code{header-line-format} that result in an empty header line
+(for example, @code{""}), but all other values tell Emacs to display a
+header line, whether or not it is empty.
+
 If @code{display-line-numbers-mode} is turned on in a buffer
 (@pxref{Display Custom, display-line-numbers-mode,, emacs, The GNU
 Emacs Manual}), the buffer text is indented on display by the amount
diff --git a/etc/NEWS b/etc/NEWS
index 9529282f047..f09e8972bc6 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -451,6 +451,14 @@ hooks named after the feature name, like 'esh-mode-unload-hook'.
 +++
 ** 'copy-tree' now copies records when its optional 2nd argument is non-nil.
 
++++
+** Certain values of 'header-line-format' now inhibit empty header line.
+Emacs now avoids displaying a header line, instead of displaying an
+empty one, when 'header-line-format' is a list whose 'car' is a
+symbol, and either that symbol is ':eval' and the second element of
+the list evaluates to 'nil' or the symbol's value as a variable is
+'nil' or void.
+
 \f
 * Lisp Changes in Emacs 30.1
 
diff --git a/src/lisp.h b/src/lisp.h
index 2bfcd1a1983..2978de962d9 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4174,6 +4174,7 @@ verify (FLT_RADIX == 2 || FLT_RADIX == 16);
 extern void syms_of_xdisp (void);
 extern void init_xdisp (void);
 extern Lisp_Object safe_eval (Lisp_Object);
+extern Lisp_Object safe_eval_inhibit_quit (Lisp_Object);
 extern bool pos_visible_p (struct window *, ptrdiff_t, int *,
 			   int *, int *, int *, int *, int *);
 
diff --git a/src/window.c b/src/window.c
index f4e09f49eae..9429679061e 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5471,6 +5471,48 @@ window_wants_mode_line (struct window *w)
 }
 
 
+/**
+ * null_header_line_format:
+ *
+ * Return non-zero when header line format FMT indicates that the
+ * header line should not be displayed at all.
+ *
+ * This is when FMT is nil, or if FMT is a cons cell and either its
+ * car is a symbol whose value as a variable is nil or void, or its
+ * car is the symbol ':eval' and its cadr evaluates to nil.
+ */
+static bool
+null_header_line_format (Lisp_Object fmt, struct frame * f)
+{
+  Lisp_Object car;
+  Lisp_Object val;
+
+  if (NILP (fmt))
+    return true;
+
+  if (CONSP (fmt))
+    {
+      car = XCAR (fmt);
+      if (SYMBOLP (car))
+	{
+	  if (EQ (car, QCeval))
+	    {
+	      val = safe_eval_inhibit_quit (XCAR (XCDR (fmt)));
+	      if (!FRAME_LIVE_P (f))
+		signal_error (":eval deleted the frame being displayed", fmt);
+	      return NILP (val);
+	    }
+	  val = find_symbol_value (car);
+	  return (SYMBOLP (car)
+		  && (EQ (val, Qunbound)
+		      || NILP (val)));
+	}
+    }
+
+  return false;
+}
+
+
 /**
  * window_wants_header_line:
  *
@@ -5491,12 +5533,16 @@ window_wants_header_line (struct window *w)
   Lisp_Object window_header_line_format =
     window_parameter (w, Qheader_line_format);
 
+  struct frame * f = WINDOW_XFRAME(w);
+
   return (WINDOW_LEAF_P (w)
 	  && !MINI_WINDOW_P (w)
 	  && !WINDOW_PSEUDO_P (w)
 	  && !EQ (window_header_line_format, Qnone)
-	  && (!NILP (window_header_line_format)
-	      || !NILP (BVAR (XBUFFER (WINDOW_BUFFER (w)), header_line_format)))
+	  && (!null_header_line_format (window_header_line_format, f)
+	      || !null_header_line_format (BVAR (XBUFFER (WINDOW_BUFFER (w)),
+						 header_line_format),
+					   f))
 	  && (WINDOW_PIXEL_HEIGHT (w)
 	      > (window_wants_mode_line (w)
 		 ? 2 * WINDOW_FRAME_LINE_HEIGHT (w)
diff --git a/src/xdisp.c b/src/xdisp.c
index a6ec966ea3c..5e25857322f 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -3074,6 +3074,12 @@ safe__eval (bool inhibit_quit, Lisp_Object sexpr)
   return safe__call1 (inhibit_quit, Qeval, sexpr);
 }
 
+Lisp_Object
+safe_eval_inhibit_quit (Lisp_Object sexpr)
+{
+  return safe__eval (true, sexpr);
+}
+
 /* Call function FN with two arguments ARG1 and ARG2.
    Return the result, or nil if something went wrong.  */
 
-- 
2.39.2 (Apple Git-143)