Message ID | 1409534708-31981-1-git-send-email-patrick@parcs.ath.cx |
---|---|
State | New, archived |
Headers |
Received: (qmail 9249 invoked by alias); 1 Sep 2014 01:25:25 -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 9239 invoked by uid 89); 1 Sep 2014 01:25:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-qa0-f51.google.com Received: from mail-qa0-f51.google.com (HELO mail-qa0-f51.google.com) (209.85.216.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 01 Sep 2014 01:25:23 +0000 Received: by mail-qa0-f51.google.com with SMTP id j7so4312400qaq.38 for <gdb-patches@sourceware.org>; Sun, 31 Aug 2014 18:25:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=qpnloVlJC5d6uW1k7dIsGkgW7f/EggLXxuAESCXNOpo=; b=iU4IW1SeoAUhuiyOhIL9t+WQ8a2lisX2kI/ufnGUmRssec53NSxrrXZttnMleoVwLH 8MlIRdLnen7QIPCWDoR0XFMDUR+qLtjZULso255ZEg+S9izi6pmFfsipCtxTWY/wcW2Z T+k0z9bcappIbk73TrLbRWA7a1OFJ33F0eR4nw6zkTf6DQvd9tsGwozkOY8DDLt3qPst 8b5FLHwjtLrrJ+D7ZVNT3P/EwZHwX26/EDeDAOpqD8h/Gk5S9gtYe8ozLrriOq2eDbX/ vMWnvF/BTxAKm0/BiZAA+2ryFCLyI9CdAuplkUsongZAdK/JLjxrzHqzOv/qqus2ghn3 etMg== X-Gm-Message-State: ALoCoQmIHaMffP5vaOByZG8QtX6L0BC0YQHEoSGKgkBi9aKeAkeu5U+I88cnH/G7bNCFnwlIba2D X-Received: by 10.140.107.198 with SMTP id h64mr37727858qgf.42.1409534721600; Sun, 31 Aug 2014 18:25:21 -0700 (PDT) Received: from localhost.localdomain (ool-4353af5c.dyn.optonline.net. [67.83.175.92]) by mx.google.com with ESMTPSA id l46sm10011276qgd.27.2014.08.31.18.25.20 for <multiple recipients> (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sun, 31 Aug 2014 18:25:20 -0700 (PDT) From: Patrick Palka <patrick@parcs.ath.cx> To: gdb-patches@sourceware.org Cc: Patrick Palka <patrick@parcs.ath.cx> Subject: [PATCH] Ensure rl_get_screen_size() returns the actual terminal dimensions Date: Sun, 31 Aug 2014 21:25:08 -0400 Message-Id: <1409534708-31981-1-git-send-email-patrick@parcs.ath.cx> X-IsSubscribed: yes |
Commit Message
Patrick Palka
Sept. 1, 2014, 1:25 a.m. UTC
We should call rl_resize_terminal() before calling rl_get_screen_size() to help ensure that rl_get_screen_size() will return the correct terminal dimensions. Doing so fixes a couple issues where TUI does not correctly resize itself due to rl_get_screen_size() returning stale terminal dimensions. * tui/tui-win.c (tui_resize_all): Call rl_resize_terminal before calling rl_get_screen_size. --- gdb/tui/tui-win.c | 1 + 1 file changed, 1 insertion(+)
Comments
On 09/01/2014 02:25 AM, Patrick Palka wrote: > We should call rl_resize_terminal() before calling rl_get_screen_size() > to help ensure that rl_get_screen_size() will return the correct > terminal dimensions. Doing so fixes a couple issues where TUI does not > correctly resize itself due to rl_get_screen_size() returning stale > terminal dimensions. > > * tui/tui-win.c (tui_resize_all): Call rl_resize_terminal before > calling rl_get_screen_size. OK. Thanks, Pedro Alves
On 09/04/2014 11:48 AM, Pedro Alves wrote: > On 09/01/2014 02:25 AM, Patrick Palka wrote: >> We should call rl_resize_terminal() before calling rl_get_screen_size() >> to help ensure that rl_get_screen_size() will return the correct >> terminal dimensions. Doing so fixes a couple issues where TUI does not >> correctly resize itself due to rl_get_screen_size() returning stale >> terminal dimensions. >> >> * tui/tui-win.c (tui_resize_all): Call rl_resize_terminal before >> calling rl_get_screen_size. > > OK. Actually, I take that back, for a moment. readline's rl_sigwinch_handler already call rl_resize_terminal, and then chains into the TUI's handler. So, why do we need this? Thanks, Pedro Alves
On Thu, Sep 4, 2014 at 6:52 AM, Pedro Alves <palves@redhat.com> wrote: > On 09/04/2014 11:48 AM, Pedro Alves wrote: >> On 09/01/2014 02:25 AM, Patrick Palka wrote: >>> We should call rl_resize_terminal() before calling rl_get_screen_size() >>> to help ensure that rl_get_screen_size() will return the correct >>> terminal dimensions. Doing so fixes a couple issues where TUI does not >>> correctly resize itself due to rl_get_screen_size() returning stale >>> terminal dimensions. >>> >>> * tui/tui-win.c (tui_resize_all): Call rl_resize_terminal before >>> calling rl_get_screen_size. >> >> OK. > > Actually, I take that back, for a moment. > > readline's rl_sigwinch_handler already call rl_resize_terminal, and then > chains into the TUI's handler. So, why do we need this? > > Thanks, > Pedro Alves > Sometimes we overwrite readline's screen size by calling tui_update_gdb_sizes() like done in tui_enable(). When that's done rl_get_screen_size() will not return the real terminal dimensions, but rather the dimensions that were set by tui_update_gdb_sizes(). So in the following scenario the TUI will get resized to the wrong dimensions: 0. Run GDB. 1. Enter TUI mode. 2. Exit TUI mode. 3. Resize the terminal. 4. Enter TUI mode again. 5. Press a key so that the TUI will resize itself. Here we resize the terminal (causing RL's SIGWINCH handler to run, updating RL's idea of the terminal size). Then we enter TUI mode which calls tui_update_gdb_sizes() which resets RL's idea of the terminal size. Then we call rl_get_screen_size() in tui_getc() which returns not the terminal dimensions but the dimensions of the TUI's command window as set by tui_update_gdb_sizes(). So in the end the TUI gets resized to incorrect dimensions. Also, when we are blocking inside a secondary prompt, readline's SIGWINCH handler does not seem to trigger for some reason. So when we resize the window during a secondary prompt, our SIGWINCH handler gets triggered but readline's handler doesn't which means that the dimensions returned by rl_get_screen_size() will be wrong. I'm not really sure why this happens. Explicitly calling rl_resize_terminal() fixes both of these issues. Patrick
On 09/04/2014 12:58 PM, Patrick Palka wrote: >> readline's rl_sigwinch_handler already call rl_resize_terminal, and then >> chains into the TUI's handler. So, why do we need this? > Sometimes we overwrite readline's screen size by calling > tui_update_gdb_sizes() like done in tui_enable(). When that's done > rl_get_screen_size() will not return the real terminal dimensions, but > rather the dimensions that were set by tui_update_gdb_sizes(). So in > the following scenario the TUI will get resized to the wrong > dimensions: > > 0. Run GDB. > 1. Enter TUI mode. > 2. Exit TUI mode. > 3. Resize the terminal. > 4. Enter TUI mode again. > 5. Press a key so that the TUI will resize itself. > > Here we resize the terminal (causing RL's SIGWINCH handler to run, > updating RL's idea of the terminal size). Then we enter TUI mode > which calls tui_update_gdb_sizes() which resets RL's idea of the > terminal size. Then we call rl_get_screen_size() in tui_getc() which > returns not the terminal dimensions but the dimensions of the TUI's > command window as set by tui_update_gdb_sizes(). So in the end the > TUI gets resized to incorrect dimensions. Hmm, OK, but... I think the story is just unraveling though. :-) E.g., trying with your patch, I do: 0. Run GDB. 1. Enter TUI mode. 2. "show height" -> 15. Looks legit. 3. Exit TUI mode. 4. "show height" -> 45 5. Resize the terminal to something smaller than 45 lines. 6. "show height" -> still 45 (!?!?) 7. Enter TUI mode again. I have now about 3 visible lines in my cmd window. 8. "show height" -> 15 (!?!?) 9. Exit TUI mode. A. "show height" -> _14_ (looks like it's the legit size now.) B. Enter TUI mode again. I still still about 3 lines height. C. "show height" -> _3_ (finally ??) The screen size handling looks seriously confused. Hmm, if I instead enter/exit/enter in a row, then the height looks reasonable, like: 0. Run GDB. 1. "show height" -> 45 2. Resize the terminal to something smaller than 45 lines. 3. Enter TUI mode. 4. Exit TUI mode. 5. Enter TUI mode immediately again. 6. "show height" -> 3. Looks legit. 7. Exit TUI mode. 8. "show height" -> 14 (looks legit) I notice we don't really need the TUI to see misbehavior, though: 0. Run GDB. 1. "show height" -> 45 2. Resize the terminal to something smaller than 45 lines. 3. "show height" -> still 45 (!?!?) Sounds like GDB should have a SIGWINCH handler even if not running the TUI, that marks the height as invalid, so the next time GDB needs the height, it's refetched from readline. > > Also, when we are blocking inside a secondary prompt, readline's > SIGWINCH handler does not seem to trigger for some reason. So when we > resize the window during a secondary prompt, our SIGWINCH handler gets > triggered but readline's handler doesn't which means that the > dimensions returned by rl_get_screen_size() will be wrong. I'm not > really sure why this happens. > > Explicitly calling rl_resize_terminal() fixes both of these issues. So, it actually sounds like we _should_ be calling rl_resize_terminal to get the full "root" window's sizes, and then _always_ end up calling tui_update_gdb_sizes -> rl_set_screen_size ? That, and maybe set rl_catch_sigwinch=1 as it sounds like readline's signal handler would end up useless. Thanks, Pedro Alves
diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c index b117634..05539fe 100644 --- a/gdb/tui/tui-win.c +++ b/gdb/tui/tui-win.c @@ -655,6 +655,7 @@ tui_resize_all (void) int height_diff, width_diff; int screenheight, screenwidth; + rl_resize_terminal (); rl_get_screen_size (&screenheight, &screenwidth); width_diff = screenwidth - tui_term_width (); height_diff = screenheight - tui_term_height ();