This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/3] TUI: Don't print KEY_RESIZE keys


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(-)

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]