[patch] Slightly better resize support in TUI mode

[patch] Slightly better resize support in TUI mode

[Balazs Kezes]

Hi,

Resizing doesn't work when in TUI mode (at least on my computer). I now
present a patch which adds a minimal support to it, although there are lots of
issues with it which won't be handled yet. This makes sure that TUI users can
make their terminal a line bigger or smaller without messing up the whole
screen.

First of all we need to make sure that readline works with correct screen
sizes. When readline is not echoing it doesn't update its internal variables
about the screen size. This is an issue because TUI uses curses functions to
echo a character back so readline is in noecho mode. Here's the patch for that:

Changelog

2010-07-19 Balazs Kezes (rlblaster@gmail.com)

	* terminal.c (rl_resize_terminal): Make sure terminal size is updated
	even when readline is not echoing.

Index: terminal.c
===================================================================
RCS file: /cvs/src/src/readline/terminal.c,v
retrieving revision 1.11
diff -c -p -r1.11 terminal.c
*** terminal.c	13 Nov 2006 09:33:30 -0000	1.11
--- terminal.c	19 Jul 2010 20:11:05 -0000
*************** rl_reset_screen_size ()
*** 347,355 ****
  void
  rl_resize_terminal ()
  {
    if (readline_echoing_p)
      {
-       _rl_get_screen_size (fileno (rl_instream), 1);
        if (CUSTOM_REDISPLAY_FUNC ())
  	rl_forced_update_display ();
        else
--- 347,355 ----
  void
  rl_resize_terminal ()
  {
+   _rl_get_screen_size (fileno (rl_instream), 1);
    if (readline_echoing_p)
      {
        if (CUSTOM_REDISPLAY_FUNC ())
  	rl_forced_update_display ();
        else

This is important to make sure readline works even after a resize in
TUI mode when switched back to normal mode.

The other issue is that in TUI's SIGWINCH handler the resize code is not
called at all. In the resize code you need to call resize_term which is
ncurses function in order to update its internal structures. This was placed
into a #ifdef HAVE_RESIZE_TERM but I have not found any references to that
macro so I removed the it. Also it seems to me that wresize is not called for
the command window so I added a call to it too.

Cheers,
Balazs

2010-07-19 Balazs Kezes (rlblaster@gmail.com)

	* tui/tui-win.c (tui_resize_all): Make sure resize_term is called
	and resize the command window after the dimensions are set.
	(tui_sigwinch_handler): Call resize in the SIGWINCH handler but only
	when in TUI mode.

Index: tui-win.c
===================================================================
RCS file: /cvs/src/src/gdb/tui/tui-win.c,v
retrieving revision 1.47
diff -c -p -r1.47 tui-win.c
*** tui-win.c	17 May 2010 22:21:43 -0000	1.47
--- tui-win.c	19 Jul 2010 20:13:08 -0000
*************** tui_resize_all (void)
*** 669,677 ****
        enum tui_win_type win_type;
        int new_height, split_diff, cmd_split_diff, num_wins_displayed = 2;

- #ifdef HAVE_RESIZE_TERM
        resize_term (screenheight, screenwidth);
! #endif
        /* Turn keypad off while we resize.  */
        if (win_with_focus != TUI_CMD_WIN)
  	keypad (TUI_CMD_WIN->generic.handle, FALSE);
--- 669,676 ----
        enum tui_win_type win_type;
        int new_height, split_diff, cmd_split_diff, num_wins_displayed = 2;

        resize_term (screenheight, screenwidth);
!
        /* Turn keypad off while we resize.  */
        if (win_with_focus != TUI_CMD_WIN)
  	keypad (TUI_CMD_WIN->generic.handle, FALSE);
*************** tui_resize_all (void)
*** 804,809 ****
--- 803,812 ----
  	 window.  */
        if (win_with_focus != TUI_CMD_WIN)
  	keypad (TUI_CMD_WIN->generic.handle, TRUE);
+
+       wresize(TUI_CMD_WIN->generic.handle,
+               TUI_CMD_WIN->generic.height,
+               TUI_CMD_WIN->generic.width);
      }
  }

*************** tui_sigwinch_handler (int signal)
*** 817,822 ****
--- 820,831 ----
    /* Say that a resize was done so that the readline can do it later
       when appropriate.  */
    tui_set_win_resized_to (TRUE);
+
+   if (tui_active)
+     {
+       tui_resize_all ();
+       tui_refresh_all_win ();
+     }
  }
  #endif

Re: [patch] Slightly better resize support in TUI mode

[Pedro Alves]

Thanks for taking the time to work on the TUI.  I personally
appreciate it, being a TUI user myself.

On Monday 19 July 2010 22:20:38, Balazs Kezes wrote:

> First of all we need to make sure that readline works with correct screen
> sizes. When readline is not echoing it doesn't update its internal variables
> about the screen size. This is an issue because TUI uses curses functions to
> echo a character back so readline is in noecho mode. Here's the patch for that:
> 
> Changelog
> 
> 2010-07-19 Balazs Kezes (rlblaster@gmail.com)
> 
> 	* terminal.c (rl_resize_terminal): Make sure terminal size is updated
> 	even when readline is not echoing.

We don't maintain readline ourselves (although we do have a few local
patches, we much prefer not carrying them).  Could you check
whether this code has changed in more recent readline's than the one
we have in our tree, and submit this to readline upstream if not?
(we were recently talking about updating readline right after 7.2 branches,
and the the branching already happened, though the update hasn't yet).  In
any case, we also support building gdb against the system readline,
which again suggests that it is much better to have the fix upstream.

> The other issue is that 

(...)

(it is usually better to have each separate problem, and especially,
each patch submitted as a separate email.)

> in TUI's SIGWINCH handler the resize code is not
> called at all. In the resize code you need to call resize_term which is
> ncurses function in order to update its internal structures. This was placed
> into a #ifdef HAVE_RESIZE_TERM but I have not found any references to that
> macro so I removed the it. Also it seems to me that wresize is not called for
> the command window so I added a call to it too.

It sounds like the intent was to add an autoconf check for resize_term (so
that HAVE_RESIZE_TERM would appear in build/gdb/config.h):
  <http://linux.die.net/man/3/resize_term>
mentions that this is an extension.

> 
> *************** tui_sigwinch_handler (int signal)
> *** 817,822 ****
> --- 820,831 ----
>     /* Say that a resize was done so that the readline can do it later
>        when appropriate.  */
>     tui_set_win_resized_to (TRUE);
> +
> +   if (tui_active)
> +     {
> +       tui_resize_all ();
> +       tui_refresh_all_win ();
> +     }
>   }
>   #endif
> 

It's best to avoid doing non async signal safe work in the signal
handler.  Instead, set a global flag, and have the main code
handle the resize.  This is what the tui_set_win_resized_to
call shown above is doing -- setting the flag.  But, it sounds
like in this scenario, nobody's reacting to the flag.  That's
what we should fix.

-- 
Pedro Alves

Re: [patch] Slightly better resize support in TUI mode

[Balazs Kezes]

> We don't maintain readline ourselves (although we do have a few local
> patches, we much prefer not carrying them).  Could you check
> whether this code has changed in more recent readline's than the one
> we have in our tree, and submit this to readline upstream if not?
> (we were recently talking about updating readline right after 7.2 branches,
> and the the branching already happened, though the update hasn't yet).  In
> any case, we also support building gdb against the system readline,
> which again suggests that it is much better to have the fix upstream.

Done (I've sent a mail to the readline guys).

> It's best to avoid doing non async signal safe work in the signal
> handler.  Instead, set a global flag, and have the main code
> handle the resize.  This is what the tui_set_win_resized_to
> call shown above is doing -- setting the flag.  But, it sounds
> like in this scenario, nobody's reacting to the flag.  That's
> what we should fix.

You are right. Actually there is a function which reacts to that flag
and it is called tui_handle_resize_during_io (I've missed this somehow).
But we need to add a call to tui_resize_all there.

I think in tui_resize_all we shouldn't be using rl_get_screen_size for
getting the screen size. Later in that function we call tui_update_gdb_size
which also tells readline to update its screen size to command window's size.
When I called tui_resize_all in it was just after readline's sigwinch
handler which determined the correct sizes for the terminal so I could use
that data to calculate the new sizes for the windows. But when I do this
delayed handling it doesn't work well because, I think, we can't rely on
readline for correct terminal size.

In short I think the line
  rl_get_screen_size (&screenheight, &screenwidth);
should be replaced to something else.
I'll look into this tomorrow evening whether this is the source of the problem.

--
Balazs

On 7/20/10, Pedro Alves <pedro@codesourcery.com> wrote:
> Thanks for taking the time to work on the TUI.  I personally
> appreciate it, being a TUI user myself.
>
> On Monday 19 July 2010 22:20:38, Balazs Kezes wrote:
>
>> First of all we need to make sure that readline works with correct screen
>> sizes. When readline is not echoing it doesn't update its internal
>> variables
>> about the screen size. This is an issue because TUI uses curses functions
>> to
>> echo a character back so readline is in noecho mode. Here's the patch for
>> that:
>>
>> Changelog
>>
>> 2010-07-19 Balazs Kezes (rlblaster@gmail.com)
>>
>> 	* terminal.c (rl_resize_terminal): Make sure terminal size is updated
>> 	even when readline is not echoing.
>
> We don't maintain readline ourselves (although we do have a few local
> patches, we much prefer not carrying them).  Could you check
> whether this code has changed in more recent readline's than the one
> we have in our tree, and submit this to readline upstream if not?
> (we were recently talking about updating readline right after 7.2 branches,
> and the the branching already happened, though the update hasn't yet).  In
> any case, we also support building gdb against the system readline,
> which again suggests that it is much better to have the fix upstream.
>
>> The other issue is that
>
> (...)
>
> (it is usually better to have each separate problem, and especially,
> each patch submitted as a separate email.)
>
>> in TUI's SIGWINCH handler the resize code is not
>> called at all. In the resize code you need to call resize_term which is
>> ncurses function in order to update its internal structures. This was
>> placed
>> into a #ifdef HAVE_RESIZE_TERM but I have not found any references to that
>> macro so I removed the it. Also it seems to me that wresize is not called
>> for
>> the command window so I added a call to it too.
>
> It sounds like the intent was to add an autoconf check for resize_term (so
> that HAVE_RESIZE_TERM would appear in build/gdb/config.h):
>   <http://linux.die.net/man/3/resize_term>
> mentions that this is an extension.
>
>>
>> *************** tui_sigwinch_handler (int signal)
>> *** 817,822 ****
>> --- 820,831 ----
>>     /* Say that a resize was done so that the readline can do it later
>>        when appropriate.  */
>>     tui_set_win_resized_to (TRUE);
>> +
>> +   if (tui_active)
>> +     {
>> +       tui_resize_all ();
>> +       tui_refresh_all_win ();
>> +     }
>>   }
>>   #endif
>>
>
> It's best to avoid doing non async signal safe work in the signal
> handler.  Instead, set a global flag, and have the main code
> handle the resize.  This is what the tui_set_win_resized_to
> call shown above is doing -- setting the flag.  But, it sounds
> like in this scenario, nobody's reacting to the flag.  That's
> what we should fix.
>
> --
> Pedro Alves
>