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

[Eli Zaretskii <eliz@gnu.org>]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: sbaugh@janestreet.com,  63825@debbugs.gnu.org
> Date: Sat, 03 Jun 2023 10:28:20 +0300
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >>  For example, @code{(format-mode-line header-line-format)} returns the
> >> -text that would appear in the selected window's header line (@code{""}
> >> -if it has no header line).  @code{(format-mode-line header-line-format
> >> -'header-line)} returns the same text, with each character
> >> -carrying the face that it will have in the header line itself, and also
> >> -redraws the header line.
> >> +text that would appear in the selected window's header line.
> >> +@code{(format-mode-line header-line-format 'header-line)} returns the
> >> +same text, with each character carrying the face that it will have in
> >> +the header line itself, and also redraws the header line.
> >
> > I'm not sure why you removed the part about an empty string.  There's
> > no change in format-mode-line to justify that, AFAICT, and neither
> > should there be.
> 
> 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.

> >> +  if (CONSP (fmt))
> >> +    {
> >> +      car = XCAR (fmt);
> >> +      if (SYMBOLP (car))
> >> +	{
> >> +	  if (EQ (car, QCeval)
> >> +	      && NILP (Feval (XCAR (XCDR (fmt)), Qnil)))
> >> +	      return true;
> >
> > This should use safe__eval (or something similar), not Feval, because
> > it is called as part of redisplay, where we cannot allow any errors to
> > throw to top-level.
> 
> Got it, here's an updated patch:

Thanks.

> +  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.  Are you using
a non-default setup of C indentation levels?

> @@ -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.

> @@ -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.

Thanks.