GNU bug report logs - #63825
29.0.90; The header line should be hidden when empty

Previous Next

Full log

View this message in rfc822 format

From: Eshel Yaron <me <at> eshelyaron.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: sbaugh <at> janestreet.com, 63825 <at> debbugs.gnu.org
Subject: bug#63825: 29.0.90; The header line should be hidden when empty
Date: Sun, 04 Jun 2023 19:45:23 +0300

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Eshel Yaron <me <at> 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:

[v3-0001-Avoid-header-line-with-some-empty-non-nil-formats.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.