From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23523 invoked by alias); 17 Feb 2015 00:53:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 23449 invoked by uid 89); 17 Feb 2015 00:53:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-pd0-f173.google.com Received: from mail-pd0-f173.google.com (HELO mail-pd0-f173.google.com) (209.85.192.173) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 17 Feb 2015 00:53:13 +0000 Received: by pdjy10 with SMTP id y10so39789007pdj.13 for ; Mon, 16 Feb 2015 16:53:11 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=7hYn6lT1hF9aJnXcjflGJerpmSyOrQnnm6aTy4OiiQY=; b=XrMJavO75tgiz0d42WS4A/Zl307i6fqsCVz0dLKeZrT5gOwIOAbqKm7OnSJC6WBn14 NFWOvhXOwkJQUQ3ubykw8mvcJbMHplzjT1l5JWdEfGYGSos7tDI0yWY4tZEeMoXHJfOu Dbvyt443Kqw0sF502nxpDf2EbqTHLWpZHdNXOqbZKvNnU0kyDC7Y+ClXuu/o5vllghj6 CVqMW6hMFUa5mlLflXgGH0GoBGi4Y3RPASNuUOhd5OTJk5/d4pKAsojxYODAbqofdiPV zzbtoAGzFU6BvFCeUtptTt5XjHYclvcJM4RRV4Uet5TKEVcQldVdYIR1iZE8dRMZdNhV XnIg== X-Gm-Message-State: ALoCoQmCpXwiy/OjaXo323mLoam4A/mJS26GF5wKSrlcOWqA2utmxv0A5ZUAHbeKqCEYL+E0pXaN X-Received: by 10.66.119.103 with SMTP id kt7mr45055094pab.157.1424134391309; Mon, 16 Feb 2015 16:53:11 -0800 (PST) MIME-Version: 1.0 Received: by 10.70.127.195 with HTTP; Mon, 16 Feb 2015 16:52:51 -0800 (PST) In-Reply-To: <54E27241.6010302@redhat.com> References: <1420689885-31156-1-git-send-email-patrick@parcs.ath.cx> <1420689885-31156-2-git-send-email-patrick@parcs.ath.cx> <54AE6A19.9070901@redhat.com> <54AE848B.1050606@redhat.com> <54E27241.6010302@redhat.com> From: Patrick Palka Date: Tue, 17 Feb 2015 00:53:00 -0000 Message-ID: Subject: Re: [PATCH 2/3] TUI: Don't print KEY_RESIZE keys To: Pedro Alves Cc: "gdb-patches@sourceware.org" Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2015-02/txt/msg00373.txt.bz2 On Mon, Feb 16, 2015 at 5:42 PM, Pedro Alves wrote: > On 02/11/2015 12:25 AM, Patrick Palka wrote: >> On Thu, Jan 8, 2015 at 8:22 AM, Pedro Alves wrote: >>> On 01/08/2015 12:32 PM, Patrick Palka wrote: >>>> On Thu, Jan 8, 2015 at 6:29 AM, Pedro Alves 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 > 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 > > * 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 >