[2/3] TUI: Don't print KEY_RESIZE keys

  On 02/11/2015 12:25 AM, Patrick Palka wrote:
> On Thu, Jan 8, 2015 at 8:22 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 01/08/2015 12:32 PM, Patrick Palka wrote:
>>> On Thu, Jan 8, 2015 at 6:29 AM, Pedro Alves <palves@redhat.com> wrote:
>>>> On 01/08/2015 04:04 AM, Patrick Palka wrote:
>>>>> wgetch() sometimes returns KEY_RESIZE when ncurses detects that the
>>>>> terminal has been resized.
>>>>
>>>> I think curses SIGWINCH handler ends up _not_ installed, right?
>>>> We install our own, and so does readline.
>>>> So how did a resize manage to be detected/processed while inside
>>>> wgetch?
>>>
>>> I'm pretty sure that the SIGWINCH handlers does not get installed.
>>> However ncurses may detect a resize event when we exit TUI (exiting
>>> ncurses), then resize the terminal, then re-enter TUI (restarting
>>> ncurses).  From there a KEY_RESIZE key is added to its internal FIFO.
>>> And the next call to wgetch will return this KEY_RESIZE key.
>>
>> Doesn't that mean then that we're delaying the resize until it's
>> "too late"?  IOW, there's a moment where we show the contents with
>> the wrong sizes.  And trying that out, I do see that happen: if I
>> disable the TUI, resize the terminal, and then reenable the TUI,
>> like you say, then until you press _another_ key, the windows
>> have the wrong size.  We should have resized them already when we
>> re-entered the TUI.
> 
> I have just pushed the patch that fixes this issue.  The screen now
> gets opportunistically resized when switching from the CLI to the TUI.

...

> The pushed fix was essentially that, but it also sets tui_win_resized
> to FALSE so that a subsequent keypress does not change the layout.  Is
> this patch now OK?  (You have already approved patch #1 and #3 in this
> series.)

Sorry, I'm still not convinced.  :-/  ISTM that KEY_RESIZE just
hides latent issues/design mistakes.

If we have a SIGWINCH handler, and we detect the need to resize when
we're re-enabling the TUI, and explicitly resize all windows in that
case, why would ncurses still detect a resize and put a KEY_RESIZE in
the wgetch queue?

ISTM that we're just resizing too late still.  Indeed, the patch below,
which makes us resize earlier, fixes the issue for me too.  Are you
aware of any other use case that might cause KEY_RESIZE?

(Note: Whatever the order of calls in tui_enable, we'll potentially be
showing stale contents momentarily and then overwriting with correct ones.
We get that today too.  IMO, today's even worse, as it can show windows with
the wrong size for a moment, and that might be more visible flicker than showing
the wrong contents with the correct border sizes already.  ISTM the fix for
that would be to decouple "update windows' contents" from actually
wrefresh/display'ing windows.  That looks like much more work than worth
it though.  I can only see this if I step through the code within tui_enable.
In a normal not-being-debugged run, I can't notice any flicker.)

---
From: Pedro Alves <palves@redhat.com>
Subject: [PATCH] TUI: resize windows to new terminal size before displaying
 them

If the user:

   #1 - disables the TUI
   #2 - resizes the terminal
   #3 - and then re-enables the TUI

the next wgetch() returns KEY_RESIZE.  This indicates to the ncurses
client that that ncurses detected that the terminal has been resized.
We don't handle KEY_RESIZE anywhere, so it gets passed on to readline
which interprets it as a multibyte character, and then the end result
is that the first key press after enabling the TUI is misinterpreted.

We shouldn't really need to handle KEY_RESIZE (and not all ncurses
implementations have that).  We have our own SIGWINCH handler, and,
when we re-enable the TUI, we explicitly detect terminal resizes and
resize all windows.  The reason ncurses currently does detects a
resize is that something within tui_enable forces a refresh/display of
some window before we get to do the actual resizing.  Setting a break
on ncurses' 'resizeterm' function helps find the culprit(s):

 (top-gdb) bt
 #0  resizeterm (ToLines=28, ToCols=114) at ../../ncurses/base/resizeterm.c:462
 #1  0x0000003b42812f3f in _nc_update_screensize (sp=0x2674730) at ../../ncurses/tinfo/lib_setup.c:443
 #2  0x0000003b0821cbe0 in doupdate () at ../../ncurses/tty/tty_update.c:726
 #3  0x0000003b08215539 in wrefresh (win=0x2a7bc00) at ../../ncurses/base/lib_refresh.c:65
 #4  0x00000000005257cb in tui_refresh_win (win_info=0xd73d60 <_locator>) at /home/pedro/gdb/mygit/src/gdb/tui/tui-wingeneral.c:60
 #5  0x000000000052265b in tui_show_locator_content () at /home/pedro/gdb/mygit/src/gdb/tui/tui-stack.c:269
 #6  0x00000000005273a6 in tui_set_key_mode (mode=TUI_COMMAND_MODE) at /home/pedro/gdb/mygit/src/gdb/tui/tui.c:321
 #7  0x00000000005278c7 in tui_enable () at /home/pedro/gdb/mygit/src/gdb/tui/tui.c:494
 #8  0x0000000000527011 in tui_rl_switch_mode (notused1=1, notused2=1) at /home/pedro/gdb/mygit/src/gdb/tui/tui.c:108

That is, tui_enable calls tui_set_key_mode before we've resized all
windows, and that refreshes a window as side effect.

And if we're already debugging something (there's a frame), then we'll
instead show a window from within tui_show_frame_info:

 (top-gdb) bt
 #0  resizeterm (ToLines=28, ToCols=114) at ../../ncurses/base/resizeterm.c:462
 #1  0x0000003b42812f3f in _nc_update_screensize (sp=0x202e6c0) at ../../ncurses/tinfo/lib_setup.c:443
 #2  0x0000003b0821cbe0 in doupdate () at ../../ncurses/tty/tty_update.c:726
 #3  0x0000003b08215539 in wrefresh (win=0x2042890) at ../../ncurses/base/lib_refresh.c:65
 #4  0x00000000005257cb in tui_refresh_win (win_info=0xd73d60 <_locator>) at /home/pedro/gdb/mygit/src/gdb/tui/tui-wingeneral.c:60
 #5  0x000000000052265b in tui_show_locator_content () at /home/pedro/gdb/mygit/src/gdb/tui/tui-stack.c:269
 #6  0x0000000000522931 in tui_show_frame_info (fi=0x16b9cc0) at /home/pedro/gdb/mygit/src/gdb/tui/tui-stack.c:364
 #7  0x00000000005278ba in tui_enable () at /home/pedro/gdb/mygit/src/gdb/tui/tui.c:491
 #8  0x0000000000527011 in tui_rl_switch_mode (notused1=1, notused2=1) at /home/pedro/gdb/mygit/src/gdb/tui/tui.c:108

The fix is to resize windows earlier.

gdb/ChangeLog:
2015-02-16 Pedro Alves <palves@redhat.com>

	* tui/tui.c (tui_enable): Resize windows before anything
	might show a window.
---
 gdb/tui/tui.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
  

On Mon, Feb 16, 2015 at 5:42 PM, Pedro Alves <palves@redhat.com> wrote:
> On 02/11/2015 12:25 AM, Patrick Palka wrote:
>> On Thu, Jan 8, 2015 at 8:22 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 01/08/2015 12:32 PM, Patrick Palka wrote:
>>>> On Thu, Jan 8, 2015 at 6:29 AM, Pedro Alves <palves@redhat.com> wrote:
>>>>> On 01/08/2015 04:04 AM, Patrick Palka wrote:
>>>>>> wgetch() sometimes returns KEY_RESIZE when ncurses detects that the
>>>>>> terminal has been resized.
>>>>>
>>>>> I think curses SIGWINCH handler ends up _not_ installed, right?
>>>>> We install our own, and so does readline.
>>>>> So how did a resize manage to be detected/processed while inside
>>>>> wgetch?
>>>>
>>>> I'm pretty sure that the SIGWINCH handlers does not get installed.
>>>> However ncurses may detect a resize event when we exit TUI (exiting
>>>> ncurses), then resize the terminal, then re-enter TUI (restarting
>>>> ncurses).  From there a KEY_RESIZE key is added to its internal FIFO.
>>>> And the next call to wgetch will return this KEY_RESIZE key.
>>>
>>> Doesn't that mean then that we're delaying the resize until it's
>>> "too late"?  IOW, there's a moment where we show the contents with
>>> the wrong sizes.  And trying that out, I do see that happen: if I
>>> disable the TUI, resize the terminal, and then reenable the TUI,
>>> like you say, then until you press _another_ key, the windows
>>> have the wrong size.  We should have resized them already when we
>>> re-entered the TUI.
>>
>> I have just pushed the patch that fixes this issue.  The screen now
>> gets opportunistically resized when switching from the CLI to the TUI.
>
> ...
>
>> The pushed fix was essentially that, but it also sets tui_win_resized
>> to FALSE so that a subsequent keypress does not change the layout.  Is
>> this patch now OK?  (You have already approved patch #1 and #3 in this
>> series.)
>
> Sorry, I'm still not convinced.  :-/  ISTM that KEY_RESIZE just
> hides latent issues/design mistakes.
>
> If we have a SIGWINCH handler, and we detect the need to resize when
> we're re-enabling the TUI, and explicitly resize all windows in that
> case, why would ncurses still detect a resize and put a KEY_RESIZE in
> the wgetch queue?

Good point...  When leaving TUI mode, we disable ncurses via endwin().
When re-entering TUI mode, we are re-enabling ncurses through the
first call to refresh().  If we manage to sync ncurses' knowledge of
the terminal dimensions (via the call to resize_terminall() in
tui_resize_all() I believe) before re-enabling ncurses then a
KEY_RESIZE should not get placed into the queue.

>
> ISTM that we're just resizing too late still.  Indeed, the patch below,
> which makes us resize earlier, fixes the issue for me too.  Are you
> aware of any other use case that might cause KEY_RESIZE?

Nice, it fixes the issue for me too.  No more KEY_RESIZE keys.  And I
am not aware of another way to trigger KEY_RESIZE keys.

>
> (Note: Whatever the order of calls in tui_enable, we'll potentially be
> showing stale contents momentarily and then overwriting with correct ones.
> We get that today too.  IMO, today's even worse, as it can show windows with
> the wrong size for a moment, and that might be more visible flicker than showing
> the wrong contents with the correct border sizes already.  ISTM the fix for
> that would be to decouple "update windows' contents" from actually
> wrefresh/display'ing windows.  That looks like much more work than worth
> it though.  I can only see this if I step through the code within tui_enable.
> In a normal not-being-debugged run, I can't notice any flicker.)

I have never noticed such flickering.  But it is does not surprise me
that TUI has the potential to flicker like hell, see
https://sourceware.org/bugzilla/show_bug.cgi?id=13378

The patch makes a lot of sense to me... I appreciate your taking such
an in-depth look at such a tedious issue!  Is it OK to commit patches
1 and 3 of this series once you commit the below patch?

(As a side note it boggles my mind that [vertically] resizing the
terminal while inside TUI is horribly broken, yet indirectly resizing
TUI by temporarily exiting TUI, resizing within the CLI then
re-entering TUI works just fine.  Both of these methods ought to be
running the same resizing logic!  Something tells me most of
tui_resize_all() is unnecessary and broken.)

>
> ---
> From: Pedro Alves <palves@redhat.com>
> Subject: [PATCH] TUI: resize windows to new terminal size before displaying
>  them
>
> If the user:
>
>    #1 - disables the TUI
>    #2 - resizes the terminal
>    #3 - and then re-enables the TUI
>
> the next wgetch() returns KEY_RESIZE.  This indicates to the ncurses
> client that that ncurses detected that the terminal has been resized.
> We don't handle KEY_RESIZE anywhere, so it gets passed on to readline
> which interprets it as a multibyte character, and then the end result
> is that the first key press after enabling the TUI is misinterpreted.
>
> We shouldn't really need to handle KEY_RESIZE (and not all ncurses
> implementations have that).  We have our own SIGWINCH handler, and,
> when we re-enable the TUI, we explicitly detect terminal resizes and
> resize all windows.  The reason ncurses currently does detects a
> resize is that something within tui_enable forces a refresh/display of
> some window before we get to do the actual resizing.  Setting a break
> on ncurses' 'resizeterm' function helps find the culprit(s):
>
>  (top-gdb) bt
>  #0  resizeterm (ToLines=28, ToCols=114) at ../../ncurses/base/resizeterm.c:462
>  #1  0x0000003b42812f3f in _nc_update_screensize (sp=0x2674730) at ../../ncurses/tinfo/lib_setup.c:443
>  #2  0x0000003b0821cbe0 in doupdate () at ../../ncurses/tty/tty_update.c:726
>  #3  0x0000003b08215539 in wrefresh (win=0x2a7bc00) at ../../ncurses/base/lib_refresh.c:65
>  #4  0x00000000005257cb in tui_refresh_win (win_info=0xd73d60 <_locator>) at /home/pedro/gdb/mygit/src/gdb/tui/tui-wingeneral.c:60
>  #5  0x000000000052265b in tui_show_locator_content () at /home/pedro/gdb/mygit/src/gdb/tui/tui-stack.c:269
>  #6  0x00000000005273a6 in tui_set_key_mode (mode=TUI_COMMAND_MODE) at /home/pedro/gdb/mygit/src/gdb/tui/tui.c:321
>  #7  0x00000000005278c7 in tui_enable () at /home/pedro/gdb/mygit/src/gdb/tui/tui.c:494
>  #8  0x0000000000527011 in tui_rl_switch_mode (notused1=1, notused2=1) at /home/pedro/gdb/mygit/src/gdb/tui/tui.c:108
>
> That is, tui_enable calls tui_set_key_mode before we've resized all
> windows, and that refreshes a window as side effect.
>
> And if we're already debugging something (there's a frame), then we'll
> instead show a window from within tui_show_frame_info:
>
>  (top-gdb) bt
>  #0  resizeterm (ToLines=28, ToCols=114) at ../../ncurses/base/resizeterm.c:462
>  #1  0x0000003b42812f3f in _nc_update_screensize (sp=0x202e6c0) at ../../ncurses/tinfo/lib_setup.c:443
>  #2  0x0000003b0821cbe0 in doupdate () at ../../ncurses/tty/tty_update.c:726
>  #3  0x0000003b08215539 in wrefresh (win=0x2042890) at ../../ncurses/base/lib_refresh.c:65
>  #4  0x00000000005257cb in tui_refresh_win (win_info=0xd73d60 <_locator>) at /home/pedro/gdb/mygit/src/gdb/tui/tui-wingeneral.c:60
>  #5  0x000000000052265b in tui_show_locator_content () at /home/pedro/gdb/mygit/src/gdb/tui/tui-stack.c:269
>  #6  0x0000000000522931 in tui_show_frame_info (fi=0x16b9cc0) at /home/pedro/gdb/mygit/src/gdb/tui/tui-stack.c:364
>  #7  0x00000000005278ba in tui_enable () at /home/pedro/gdb/mygit/src/gdb/tui/tui.c:491
>  #8  0x0000000000527011 in tui_rl_switch_mode (notused1=1, notused2=1) at /home/pedro/gdb/mygit/src/gdb/tui/tui.c:108
>
> The fix is to resize windows earlier.
>
> gdb/ChangeLog:
> 2015-02-16 Pedro Alves <palves@redhat.com>
>
>         * tui/tui.c (tui_enable): Resize windows before anything
>         might show a window.
> ---
>  gdb/tui/tui.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
> index 834e682..0397ee9 100644
> --- a/gdb/tui/tui.c
> +++ b/gdb/tui/tui.c
> @@ -487,18 +487,22 @@ tui_enable (void)
>    tui_setup_io (1);
>
>    tui_active = 1;
> -  if (deprecated_safe_get_selected_frame ())
> -     tui_show_frame_info (deprecated_safe_get_selected_frame ());
>
> -  /* Restore TUI keymap.  */
> -  tui_set_key_mode (tui_current_key_mode);
> -
> -  /* Resize and refresh the screen.  */
> +  /* Resize windows before anything might display/refresh a
> +     window.  */
>    if (tui_win_resized ())
>      {
>        tui_resize_all ();
>        tui_set_win_resized_to (FALSE);
>      }
> +
> +  if (deprecated_safe_get_selected_frame ())
> +    tui_show_frame_info (deprecated_safe_get_selected_frame ());
> +
> +  /* Restore TUI keymap.  */
> +  tui_set_key_mode (tui_current_key_mode);
> +
> +  /* Refresh the screen.  */
>    tui_refresh_all_win ();
>
>    /* Update gdb's knowledge of its terminal.  */
> --
> 1.9.3
>
  

On 02/17/2015 12:52 AM, Patrick Palka wrote:
> On Mon, Feb 16, 2015 at 5:42 PM, Pedro Alves <palves@redhat.com> wrote:

> Good point...  When leaving TUI mode, we disable ncurses via endwin().
> When re-entering TUI mode, we are re-enabling ncurses through the
> first call to refresh().  If we manage to sync ncurses' knowledge of
> the terminal dimensions (via the call to resize_terminall() in
> tui_resize_all() I believe) before re-enabling ncurses then a
> KEY_RESIZE should not get placed into the queue.

Exactly.

> 
>>
>> ISTM that we're just resizing too late still.  Indeed, the patch below,
>> which makes us resize earlier, fixes the issue for me too.  Are you
>> aware of any other use case that might cause KEY_RESIZE?
> 
> Nice, it fixes the issue for me too.  No more KEY_RESIZE keys.  And I
> am not aware of another way to trigger KEY_RESIZE keys.

Alright, great.

>> (Note: Whatever the order of calls in tui_enable, we'll potentially be
>> showing stale contents momentarily and then overwriting with correct ones.
>> We get that today too.  IMO, today's even worse, as it can show windows with
>> the wrong size for a moment, and that might be more visible flicker than showing
>> the wrong contents with the correct border sizes already.  ISTM the fix for
>> that would be to decouple "update windows' contents" from actually
>> wrefresh/display'ing windows.  That looks like much more work than worth
>> it though.  I can only see this if I step through the code within tui_enable.
>> In a normal not-being-debugged run, I can't notice any flicker.)
> 
> I have never noticed such flickering.  But it is does not surprise me
> that TUI has the potential to flicker like hell, see
> https://sourceware.org/bugzilla/show_bug.cgi?id=13378

Yeah...

> The patch makes a lot of sense to me... I appreciate your taking such
> an in-depth look at such a tedious issue!

:-)  These discussions have resulted in several fixes (from you), so
I think it's been well worth it, IMO.  Thank _you_ for poking at
the TUI.

> Is it OK to commit patches
> 1 and 3 of this series once you commit the below patch?

Yes please.  I've now pushed mine in.

> 
> (As a side note it boggles my mind that [vertically] resizing the
> terminal while inside TUI is horribly broken, yet indirectly resizing
> TUI by temporarily exiting TUI, resizing within the CLI then
> re-entering TUI works just fine.  Both of these methods ought to be
> running the same resizing logic!  Something tells me most of
> tui_resize_all() is unnecessary and broken.)

I think that that's missing is integrating SIGWINCH handling with the
event loop.  That is, nothing is waking up the event loop to react to
SIGWINCH and do the resize, so the resize ends up happening on
next IO (next key press).  Something along these lines:

In the TUI setup code where we install the SIGWINCH signal
handler create a new event loop source for the SIGWINCH signal:

  sigwinch_token =
    create_async_signal_handler (async_sigwinch_handler, NULL);

Have the real SIGWINCH handler mark that async event source:

static void
handle_sigwinch (int sig)
{
  mark_async_signal_handler (sigwinch_token);
}

And then when that event source's callback is called
by the event loop, resize the TUI:

static void
async_sigwinch_handler (gdb_client_data arg)
{
  /* Trigger TUI resize here.  */
}

Would you like to try that?

Thanks,
Pedro Alves
  

On Tue, Feb 17, 2015 at 5:23 AM, Pedro Alves <palves@redhat.com> wrote:
> On 02/17/2015 12:52 AM, Patrick Palka wrote:
>> On Mon, Feb 16, 2015 at 5:42 PM, Pedro Alves <palves@redhat.com> wrote:
>
>> Good point...  When leaving TUI mode, we disable ncurses via endwin().
>> When re-entering TUI mode, we are re-enabling ncurses through the
>> first call to refresh().  If we manage to sync ncurses' knowledge of
>> the terminal dimensions (via the call to resize_terminall() in
>> tui_resize_all() I believe) before re-enabling ncurses then a
>> KEY_RESIZE should not get placed into the queue.
>
> Exactly.
>
>>
>>>
>>> ISTM that we're just resizing too late still.  Indeed, the patch below,
>>> which makes us resize earlier, fixes the issue for me too.  Are you
>>> aware of any other use case that might cause KEY_RESIZE?
>>
>> Nice, it fixes the issue for me too.  No more KEY_RESIZE keys.  And I
>> am not aware of another way to trigger KEY_RESIZE keys.
>
> Alright, great.
>
>>> (Note: Whatever the order of calls in tui_enable, we'll potentially be
>>> showing stale contents momentarily and then overwriting with correct ones.
>>> We get that today too.  IMO, today's even worse, as it can show windows with
>>> the wrong size for a moment, and that might be more visible flicker than showing
>>> the wrong contents with the correct border sizes already.  ISTM the fix for
>>> that would be to decouple "update windows' contents" from actually
>>> wrefresh/display'ing windows.  That looks like much more work than worth
>>> it though.  I can only see this if I step through the code within tui_enable.
>>> In a normal not-being-debugged run, I can't notice any flicker.)
>>
>> I have never noticed such flickering.  But it is does not surprise me
>> that TUI has the potential to flicker like hell, see
>> https://sourceware.org/bugzilla/show_bug.cgi?id=13378
>
> Yeah...
>
>> The patch makes a lot of sense to me... I appreciate your taking such
>> an in-depth look at such a tedious issue!
>
> :-)  These discussions have resulted in several fixes (from you), so
> I think it's been well worth it, IMO.  Thank _you_ for poking at
> the TUI.
>
>> Is it OK to commit patches
>> 1 and 3 of this series once you commit the below patch?
>
> Yes please.  I've now pushed mine in.
>
>>
>> (As a side note it boggles my mind that [vertically] resizing the
>> terminal while inside TUI is horribly broken, yet indirectly resizing
>> TUI by temporarily exiting TUI, resizing within the CLI then
>> re-entering TUI works just fine.  Both of these methods ought to be
>> running the same resizing logic!  Something tells me most of
>> tui_resize_all() is unnecessary and broken.)
>
> I think that that's missing is integrating SIGWINCH handling with the
> event loop.  That is, nothing is waking up the event loop to react to
> SIGWINCH and do the resize, so the resize ends up happening on
> next IO (next key press).  Something along these lines:

That would be nice.

>
> In the TUI setup code where we install the SIGWINCH signal
> handler create a new event loop source for the SIGWINCH signal:
>
>   sigwinch_token =
>     create_async_signal_handler (async_sigwinch_handler, NULL);
>
> Have the real SIGWINCH handler mark that async event source:
>
> static void
> handle_sigwinch (int sig)
> {
>   mark_async_signal_handler (sigwinch_token);
> }
>
> And then when that event source's callback is called
> by the event loop, resize the TUI:
>
> static void
> async_sigwinch_handler (gdb_client_data arg)
> {
>   /* Trigger TUI resize here.  */
> }
>
> Would you like to try that?

Didn't know that adding another event handler is so easy.  I'll try doing that.

>
> Thanks,
> Pedro Alves
>
  

On Tue, Feb 17, 2015 at 5:23 AM, Pedro Alves <palves@redhat.com> wrote:
> On 02/17/2015 12:52 AM, Patrick Palka wrote:
>> On Mon, Feb 16, 2015 at 5:42 PM, Pedro Alves <palves@redhat.com> wrote:
>
>> Good point...  When leaving TUI mode, we disable ncurses via endwin().
>> When re-entering TUI mode, we are re-enabling ncurses through the
>> first call to refresh().  If we manage to sync ncurses' knowledge of
>> the terminal dimensions (via the call to resize_terminall() in
>> tui_resize_all() I believe) before re-enabling ncurses then a
>> KEY_RESIZE should not get placed into the queue.
>
> Exactly.
>
>>
>>>
>>> ISTM that we're just resizing too late still.  Indeed, the patch below,
>>> which makes us resize earlier, fixes the issue for me too.  Are you
>>> aware of any other use case that might cause KEY_RESIZE?
>>
>> Nice, it fixes the issue for me too.  No more KEY_RESIZE keys.  And I
>> am not aware of another way to trigger KEY_RESIZE keys.
>
> Alright, great.
>
>>> (Note: Whatever the order of calls in tui_enable, we'll potentially be
>>> showing stale contents momentarily and then overwriting with correct ones.
>>> We get that today too.  IMO, today's even worse, as it can show windows with
>>> the wrong size for a moment, and that might be more visible flicker than showing
>>> the wrong contents with the correct border sizes already.  ISTM the fix for
>>> that would be to decouple "update windows' contents" from actually
>>> wrefresh/display'ing windows.  That looks like much more work than worth
>>> it though.  I can only see this if I step through the code within tui_enable.
>>> In a normal not-being-debugged run, I can't notice any flicker.)
>>
>> I have never noticed such flickering.  But it is does not surprise me
>> that TUI has the potential to flicker like hell, see
>> https://sourceware.org/bugzilla/show_bug.cgi?id=13378
>
> Yeah...
>
>> The patch makes a lot of sense to me... I appreciate your taking such
>> an in-depth look at such a tedious issue!
>
> :-)  These discussions have resulted in several fixes (from you), so
> I think it's been well worth it, IMO.  Thank _you_ for poking at
> the TUI.
>
>> Is it OK to commit patches
>> 1 and 3 of this series once you commit the below patch?
>
> Yes please.  I've now pushed mine in.

Actually, I will not commit patch #3 because it no longer applies
cleanly after the CLI/TUI tab completion consolidation and
incorporating sigwinch handling will allow for the complete
eradication of tui_handle_resize_during_io().

>
>>
>> (As a side note it boggles my mind that [vertically] resizing the
>> terminal while inside TUI is horribly broken, yet indirectly resizing
>> TUI by temporarily exiting TUI, resizing within the CLI then
>> re-entering TUI works just fine.  Both of these methods ought to be
>> running the same resizing logic!  Something tells me most of
>> tui_resize_all() is unnecessary and broken.)
>
> I think that that's missing is integrating SIGWINCH handling with the
> event loop.  That is, nothing is waking up the event loop to react to
> SIGWINCH and do the resize, so the resize ends up happening on
> next IO (next key press).  Something along these lines:
>
> In the TUI setup code where we install the SIGWINCH signal
> handler create a new event loop source for the SIGWINCH signal:
>
>   sigwinch_token =
>     create_async_signal_handler (async_sigwinch_handler, NULL);
>
> Have the real SIGWINCH handler mark that async event source:
>
> static void
> handle_sigwinch (int sig)
> {
>   mark_async_signal_handler (sigwinch_token);
> }
>
> And then when that event source's callback is called
> by the event loop, resize the TUI:
>
> static void
> async_sigwinch_handler (gdb_client_data arg)
> {
>   /* Trigger TUI resize here.  */
> }
>
> Would you like to try that?
>
> Thanks,
> Pedro Alves
>