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

[Pedro Alves <palves at redhat dot com>]

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