From: Patrick Palka <patrick@parcs.ath.cx>
To: Pedro Alves <palves@redhat.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [PATCH 2/3] TUI: Don't print KEY_RESIZE keys
Date: Tue, 17 Feb 2015 13:02:00 -0000 [thread overview]
Message-ID: <CA+C-WL9wtrD4dmw0LhCt1j4-6Z7juop81-Ff0GSg66x-+hEGqg@mail.gmail.com> (raw)
In-Reply-To: <54E316A8.9070705@redhat.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
>
next prev parent reply other threads:[~2015-02-17 13:02 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-08 4:05 [PATCH 1/3] Remove superfluous function key_is_command_char() Patrick Palka
2015-01-08 4:05 ` [PATCH 2/3] TUI: Don't print KEY_RESIZE keys Patrick Palka
2015-01-08 11:29 ` Pedro Alves
2015-01-08 12:32 ` Patrick Palka
2015-01-08 13:22 ` Pedro Alves
2015-01-08 13:38 ` Patrick Palka
2015-02-11 0:25 ` Patrick Palka
2015-02-16 22:42 ` Pedro Alves
2015-02-17 0:53 ` Patrick Palka
2015-02-17 10:23 ` Pedro Alves
2015-02-17 13:02 ` Patrick Palka [this message]
2015-02-17 13:24 ` Patrick Palka
2015-01-08 4:05 ` [PATCH 3/3] Don't flush the prompt when resizing the terminal within TUI Patrick Palka
2015-01-08 11:39 ` Pedro Alves
2015-01-08 11:40 ` [PATCH 1/3] Remove superfluous function key_is_command_char() Pedro Alves
2015-01-08 13:34 ` Eli Zaretskii
2015-01-08 13:48 ` Patrick Palka
2015-01-08 14:58 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CA+C-WL9wtrD4dmw0LhCt1j4-6Z7juop81-Ff0GSg66x-+hEGqg@mail.gmail.com \
--to=patrick@parcs.ath.cx \
--cc=gdb-patches@sourceware.org \
--cc=palves@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).