[2/3] Update our idea of our terminal's dimensions even outside of TUI
Commit Message
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.
---
gdb/tui/tui-win.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
Comments
> 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.
Neat. Thanks for this nice little enhancement!
I've only glanced at the patch, as I'm not a specialist of this area
and Pedro has been pretty active there and would do a much better job
than I would. But if Pedro can't make it, I'm sure I could set some
time aside to review it a little more carefully.
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.)
> @@ -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. */
> + }
> + 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 ();
Thanks,
Pedro Alves
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
>
On 04/28/2015 02:08 AM, Patrick Palka wrote:
> Just an FYI, you have commented on patches 2/3 and 3/3 but not 1/3.
Whoops, I wrote the email, but forgot to press send yesterday; still
had the window open... Done now.
Thanks,
Pedro Alves
@@ -836,12 +836,6 @@ static struct async_signal_handler *tui_sigwinch_token;
static void
tui_sigwinch_handler (int signal)
{
- /* Set win_resized to TRUE and asynchronously invoke our resize callback. If
- the callback is invoked while TUI is active then it ought to successfully
- resize the screen, resetting win_resized to FALSE. Of course, if the
- callback is invoked while TUI is inactive then it will do nothing; in that
- case, win_resized will remain TRUE until we get a chance to synchronously
- resize the screen from tui_enable(). */
mark_async_signal_handler (tui_sigwinch_token);
tui_set_win_resized_to (TRUE);
}
@@ -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(). */
+ }
+ else
+ {
+ tui_resize_all ();
+ tui_refresh_all_win ();
+ tui_update_gdb_sizes ();
+ tui_set_win_resized_to (FALSE);
+ tui_redisplay_readline ();
+ }
}
#endif