emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Testing redisplay code in batch


From: Stefan Monnier
Subject: Re: Testing redisplay code in batch
Date: Thu, 24 Sep 2020 14:31:58 -0400
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)

> Thanks.  I have a couple of minor comments:

Based on your comments, I assume you consider that this feature is
acceptable for `master` once it's clean.

>> +      if (FRAME_INITIAL_P (f))
>> +        paused_p = false;       /* No actual display to update!  */
>
> I'd appreciate a comment here saying why setting paused_p to false is
> the consequence of "no display to update".

It's not.  The other branch sets `paused_p` so we need to set it
as well.  The comment explains why we don't do anything else *beside*
setting `paused_p`.
Not sure how best to explain that in the comment.  I changed it to:

        /* No actual display to update so the "update" is a nop and
           obviously isn't interrupted by pending input.  */
        paused_p = false;

>> -  /* 10 is arbitrary,
>> +  /* 80/40 is arbitrary,
> Why not 80x25?

80x25 it is, then ;-)

>> @@ -1136,6 +1136,7 @@ make_initial_frame (void)
>>      init_frame_faces (f);
>>  
>>    last_nonminibuf_frame = f;
>> +  echo_area_window = f->minibuffer_window;
>
> Why is this bit needed?

Ah, good question.
Without it I got an assertion failure in xdisp.c:18241

  if (MINI_WINDOW_P (w))
    {
      if (w == XWINDOW (echo_area_window)
          && !NILP (echo_area_buffer[0]))

because XWINDOW complains that `echo_area_window` is not a window.

Maybe the better fix is to change `init_xdisp` so it also sets
`echo_area_window` when `noninteractive`?

>> -      move_it_by_lines (&it, 0);
>> +      move_it_by_lines (&it, 0); /* bug#43519 */
>
> This is unrelated (and unneeded, IMO).

Indeed, it's just a temporary hack I added so I can quickly find that
spot in the code (via `C-x v =`) when I need to (un)comment it as I test
my patch ;-)

>> @@ -15414,8 +15414,8 @@ redisplay_internal (void)
>>    /* No redisplay if running in batch mode or frame is not yet fully
>>       initialized, or redisplay is explicitly turned off by setting
>>       Vinhibit_redisplay.  */
>> -  if (FRAME_INITIAL_P (SELECTED_FRAME ())
>> -      || !NILP (Vinhibit_redisplay))
>> +  if (/* FRAME_INITIAL_P (SELECTED_FRAME ())
>> +       * || */ !NILP (Vinhibit_redisplay))
>>      return;
>
> This should be done cleaner, and should also update the commentary.

Do you think it's otherwise acceptable as-is?  I was thinking of only
allowing redisplay in `FRAME_INITIAL_P` under the control of some
special config var.

> How about a small NEWS item about this?

I'll see to it,


        Stefan




reply via email to

[Prev in Thread] Current Thread [Next in Thread]