diff mbox

Ensure rl_get_screen_size() returns the actual terminal dimensions

Message ID 1409534708-31981-1-git-send-email-patrick@parcs.ath.cx
State New, archived
Headers

Commit Message

Patrick Palka Sept. 1, 2014, 1:25 a.m. UTC 
  We should call rl_resize_terminal() before calling rl_get_screen_size()
to help ensure that rl_get_screen_size() will return the correct
terminal dimensions.  Doing so fixes a couple issues where TUI does not
correctly resize itself due to rl_get_screen_size() returning stale
terminal dimensions.

	* tui/tui-win.c (tui_resize_all): Call rl_resize_terminal before
	calling rl_get_screen_size.
---
 gdb/tui/tui-win.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Pedro Alves Sept. 4, 2014, 10:48 a.m. UTC | #1 
On 09/01/2014 02:25 AM, Patrick Palka wrote:
> We should call rl_resize_terminal() before calling rl_get_screen_size()
> to help ensure that rl_get_screen_size() will return the correct
> terminal dimensions.  Doing so fixes a couple issues where TUI does not
> correctly resize itself due to rl_get_screen_size() returning stale
> terminal dimensions.
> 
> 	* tui/tui-win.c (tui_resize_all): Call rl_resize_terminal before
> 	calling rl_get_screen_size.

OK.

Thanks,
Pedro Alves
Pedro Alves Sept. 4, 2014, 10:52 a.m. UTC | #2 
On 09/04/2014 11:48 AM, Pedro Alves wrote:
> On 09/01/2014 02:25 AM, Patrick Palka wrote:
>> We should call rl_resize_terminal() before calling rl_get_screen_size()
>> to help ensure that rl_get_screen_size() will return the correct
>> terminal dimensions.  Doing so fixes a couple issues where TUI does not
>> correctly resize itself due to rl_get_screen_size() returning stale
>> terminal dimensions.
>>
>> 	* tui/tui-win.c (tui_resize_all): Call rl_resize_terminal before
>> 	calling rl_get_screen_size.
> 
> OK.

Actually, I take that back, for a moment.

readline's rl_sigwinch_handler already call rl_resize_terminal, and then
chains into the TUI's handler.  So, why do we need this?

Thanks,
Pedro Alves
Patrick Palka Sept. 4, 2014, 11:58 a.m. UTC | #3 
On Thu, Sep 4, 2014 at 6:52 AM, Pedro Alves <palves@redhat.com> wrote:
> On 09/04/2014 11:48 AM, Pedro Alves wrote:
>> On 09/01/2014 02:25 AM, Patrick Palka wrote:
>>> We should call rl_resize_terminal() before calling rl_get_screen_size()
>>> to help ensure that rl_get_screen_size() will return the correct
>>> terminal dimensions.  Doing so fixes a couple issues where TUI does not
>>> correctly resize itself due to rl_get_screen_size() returning stale
>>> terminal dimensions.
>>>
>>>      * tui/tui-win.c (tui_resize_all): Call rl_resize_terminal before
>>>      calling rl_get_screen_size.
>>
>> OK.
>
> Actually, I take that back, for a moment.
>
> readline's rl_sigwinch_handler already call rl_resize_terminal, and then
> chains into the TUI's handler.  So, why do we need this?
>
> Thanks,
> Pedro Alves
>

Sometimes we overwrite readline's screen size by calling
tui_update_gdb_sizes() like done in tui_enable().  When that's done
rl_get_screen_size() will not return the real terminal dimensions, but
rather the dimensions that were set by tui_update_gdb_sizes().  So in
the following scenario the TUI will get resized to the wrong
dimensions:

0. Run GDB.
1. Enter TUI mode.
2. Exit TUI mode.
3. Resize the terminal.
4. Enter TUI mode again.
5. Press a key so that the TUI will resize itself.

Here we resize the terminal (causing RL's SIGWINCH handler to run,
updating RL's idea of the terminal size).  Then we enter TUI mode
which calls tui_update_gdb_sizes() which resets RL's idea of the
terminal size.  Then we call rl_get_screen_size() in tui_getc() which
returns not the terminal dimensions but the dimensions of the TUI's
command window as set by tui_update_gdb_sizes().  So in the end the
TUI gets resized to incorrect dimensions.

Also, when we are blocking inside a secondary prompt, readline's
SIGWINCH handler does not seem to trigger for some reason. So when we
resize the window during a secondary prompt, our SIGWINCH handler gets
triggered but readline's handler doesn't which means that the
dimensions returned by rl_get_screen_size() will be wrong.  I'm not
really sure why this happens.

Explicitly calling rl_resize_terminal() fixes both of these issues.

Patrick
Pedro Alves Sept. 4, 2014, 5:51 p.m. UTC | #4 
On 09/04/2014 12:58 PM, Patrick Palka wrote:
>> readline's rl_sigwinch_handler already call rl_resize_terminal, and then
>> chains into the TUI's handler.  So, why do we need this?

> Sometimes we overwrite readline's screen size by calling
> tui_update_gdb_sizes() like done in tui_enable().  When that's done
> rl_get_screen_size() will not return the real terminal dimensions, but
> rather the dimensions that were set by tui_update_gdb_sizes().  So in
> the following scenario the TUI will get resized to the wrong
> dimensions:
> 
> 0. Run GDB.
> 1. Enter TUI mode.
> 2. Exit TUI mode.
> 3. Resize the terminal.
> 4. Enter TUI mode again.
> 5. Press a key so that the TUI will resize itself.
> 
> Here we resize the terminal (causing RL's SIGWINCH handler to run,
> updating RL's idea of the terminal size).  Then we enter TUI mode
> which calls tui_update_gdb_sizes() which resets RL's idea of the
> terminal size.  Then we call rl_get_screen_size() in tui_getc() which
> returns not the terminal dimensions but the dimensions of the TUI's
> command window as set by tui_update_gdb_sizes().  So in the end the
> TUI gets resized to incorrect dimensions.

Hmm, OK, but...  I think the story is just unraveling though.  :-)
E.g., trying with your patch, I do:

0. Run GDB.
1. Enter TUI mode.
2. "show height" -> 15.  Looks legit.
3. Exit TUI mode.
4. "show height" -> 45
5. Resize the terminal to something smaller than 45 lines.
6. "show height" -> still 45 (!?!?)
7. Enter TUI mode again.  I have now about 3 visible lines in my cmd window.
8. "show height" -> 15       (!?!?)
9. Exit TUI mode.
A. "show height" -> _14_     (looks like it's the legit size now.)
B. Enter TUI mode again.  I still still about 3 lines height.
C. "show height" -> _3_      (finally ??)

The screen size handling looks seriously confused.

Hmm, if I instead enter/exit/enter in a row, then the height
looks reasonable, like:

0. Run GDB.
1. "show height" -> 45
2. Resize the terminal to something smaller than 45 lines.
3. Enter TUI mode.
4. Exit TUI mode.
5. Enter TUI mode immediately again.
6. "show height" -> 3.  Looks legit.
7. Exit TUI mode.
8. "show height" -> 14     (looks legit)

I notice we don't really need the TUI to see misbehavior, though:

0. Run GDB.
1. "show height" -> 45
2. Resize the terminal to something smaller than 45 lines.
3. "show height" -> still 45 (!?!?)

Sounds like GDB should have a SIGWINCH handler even if not running
the TUI, that marks the height as invalid, so the next time GDB needs
the height, it's refetched from readline.

> 
> Also, when we are blocking inside a secondary prompt, readline's
> SIGWINCH handler does not seem to trigger for some reason. So when we
> resize the window during a secondary prompt, our SIGWINCH handler gets
> triggered but readline's handler doesn't which means that the
> dimensions returned by rl_get_screen_size() will be wrong.  I'm not
> really sure why this happens.
> 
> Explicitly calling rl_resize_terminal() fixes both of these issues.

So, it actually sounds like we _should_ be calling rl_resize_terminal
to get the full "root" window's sizes, and then _always_ end
up calling tui_update_gdb_sizes -> rl_set_screen_size ?

That, and maybe set rl_catch_sigwinch=1 as it sounds like
readline's signal handler would end up useless.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index b117634..05539fe 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -655,6 +655,7 @@  tui_resize_all (void)
   int height_diff, width_diff;
   int screenheight, screenwidth;
 
+  rl_resize_terminal ();
   rl_get_screen_size (&screenheight, &screenwidth);
   width_diff = screenwidth - tui_term_width ();
   height_diff = screenheight - tui_term_height ();