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

[Eli Zaretskii]

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