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

[Patrick Palka <patrick@parcs.ath.cx>]

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
>