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: Mon, 16 Feb 2015 19:52:51 -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>
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
>