Message ID | 54E27241.6010302@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 26219 invoked by alias); 16 Feb 2015 22:42:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 26209 invoked by uid 89); 16 Feb 2015 22:42:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 16 Feb 2015 22:42:13 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t1GMgB3I032324 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 16 Feb 2015 17:42:11 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t1GMg9NC015700; Mon, 16 Feb 2015 17:42:10 -0500 Message-ID: <54E27241.6010302@redhat.com> Date: Mon, 16 Feb 2015 22:42:09 +0000 From: Pedro Alves <palves@redhat.com> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Patrick Palka <patrick@parcs.ath.cx> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org> Subject: Re: [PATCH 2/3] TUI: Don't print KEY_RESIZE keys 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> <CA+C-WL-ohtv2BvCT2H5Nfc8furA1uVR7SSa1X9HFO5CaZZN03A@mail.gmail.com> <54AE848B.1050606@redhat.com> <CA+C-WL8tLm3xithf-tDm3AGAN0SnuN1u3SUDjJkzXhqSZ+2SsA@mail.gmail.com> In-Reply-To: <CA+C-WL8tLm3xithf-tDm3AGAN0SnuN1u3SUDjJkzXhqSZ+2SsA@mail.gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit |
Commit Message
Pedro Alves
Feb. 16, 2015, 10:42 p.m. UTC
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? 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? (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.) --- 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(-)
Comments
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 >
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: 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
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 >
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 >
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. */