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

[Balazs Kezes <rlblaster@gmail.com>]

> 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
>