Re: [PATCH 2/3] Update our idea of our terminal's dimensions even outside of TUI

[Patrick Palka <patrick at parcs dot ath dot cx>]

On Mon, Apr 27, 2015 at 12:35 PM, Pedro Alves <palves@redhat.com> wrote:
> On 04/24/2015 01:53 AM, Patrick Palka wrote:
>> When in the CLI, GDB's "width" and "height" variables are not kept in sync
>> when the underlying terminal gets resized.
>>
>> This patch fixes this issue by making sure sure to update GDB's "width"
>> and "height" variables in the !tui_active case of our SIGWINCH handler.
>>
>> gdb/ChangeLog:
>>
>>       * tui/tui-win.c (tui_sigwinch_handler): Remove now-stale comment.
>>       (tui_sigwinch_handler): Still update our idea of
>>       the terminal's width and height even when TUI is not active.
>
> (A next step for a rainy day would be to more the signal handler to
> core code, and make this work even when the TUI is not compiled in.)

Good idea... I'll do this.

>
>> @@ -850,15 +844,26 @@ tui_sigwinch_handler (int signal)
>>  static void
>>  tui_async_resize_screen (gdb_client_data arg)
>>  {
>> -  if (!tui_active)
>> -    return;
>> -
>>    rl_resize_terminal ();
>> -  tui_resize_all ();
>> -  tui_refresh_all_win ();
>> -  tui_update_gdb_sizes ();
>> -  tui_set_win_resized_to (FALSE);
>> -  tui_redisplay_readline ();
>> +
>> +  if (!tui_active)
>> +    {
>> +      int screen_height, screen_width;
>> +
>> +      rl_get_screen_size (&screen_height, &screen_width);
>> +      set_screen_width_and_height (screen_width, screen_height);
>> +
>> +      /* win_resized will be untoggled and the windows resized in the next call
>> +      to tui_enable().  */
>
> Hmm, can we please avoid "untoggle"?  I'm not a native speaker, but it
> game me pause, as assuming toggle is x^=1, then untoggle is x^=1 too,
> thus toggle==untoggle.  :-)  I'd rather use "unset".
>
> OK with that change.
>
> FWIW, the comment gives a bit of pause.  I'd suggest something like this
> instead:
>
>      /* win_resized is left set so that the next call to tui_enable
>         resizes windows.  */

Good point, it's better to explicitly mention that win_resized is left set.

>
>> +    }
>> +  else
>> +    {
>> +      tui_resize_all ();
>> +      tui_refresh_all_win ();
>> +      tui_update_gdb_sizes ();
>> +      tui_set_win_resized_to (FALSE);
>> +      tui_redisplay_readline ();
>
> A preexisting problem (thus not be fixed by this patch), but
> I note that this seems racy.  If another SIGWINCH comes in after
> between tui_resize_all and tui_set_win_resized_to, we'll clear
> tui_set_win_resized_to and lose the need to resize in tui_enable:
>
>   /* Resize windows before anything might display/refresh a
>      window.  */
>   if (tui_win_resized ())
>     {
>       tui_resize_all ();
>       tui_set_win_resized_to (FALSE);
>     }
>
> That's why the mainline code that handles a signal should always clear
> the signal handler's control variable before actually reacting to
> it, like:
>
>       tui_set_win_resized_to (FALSE);
>       tui_resize_all ();
>       tui_refresh_all_win ();
>       tui_update_gdb_sizes ();
>       tui_redisplay_readline ();

Dang, I totally missed that.  I'll post a patch for this too.

Just an FYI, you have commented on patches 2/3 and 3/3 but not 1/3.

>
> Thanks,
> Pedro Alves
>