This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/3] TUI: Don't print KEY_RESIZE keys
- From: Patrick Palka <patrick at parcs dot ath dot cx>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Tue, 17 Feb 2015 08:01:43 -0500
- Subject: Re: [PATCH 2/3] TUI: Don't print KEY_RESIZE keys
- Authentication-results: sourceware.org; auth=none
- References: <1420689885-31156-1-git-send-email-patrick at parcs dot ath dot cx> <1420689885-31156-2-git-send-email-patrick at parcs dot ath dot cx> <54AE6A19 dot 9070901 at redhat dot com> <CA+C-WL-ohtv2BvCT2H5Nfc8furA1uVR7SSa1X9HFO5CaZZN03A at mail dot gmail dot com> <54AE848B dot 1050606 at redhat dot com> <CA+C-WL8tLm3xithf-tDm3AGAN0SnuN1u3SUDjJkzXhqSZ+2SsA at mail dot gmail dot com> <54E27241 dot 6010302 at redhat dot com> <CA+C-WL_RkmQtgTvZzg5NXW7F+_dd1fymXThCqKs8OR_eTcU=yg at mail dot gmail dot com> <54E316A8 dot 9070705 at redhat dot com>
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
>