public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [pushed] Fix PR tui/21216: TUI line breaks regression
@ 2017-03-08  0:19 Pedro Alves
  2017-03-09 23:04 ` Jan Kratochvil
  0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2017-03-08  0:19 UTC (permalink / raw)
  To: gdb-patches

Commit d7e747318f4d04 ("Eliminate make_cleanup_ui_file_delete / make
ui_file a class hierarchy") regressed the TUI's command window.
Newlines miss doing a "carriage return", resulting in output like:

~~~~~~~~~~~~~~~~~~
(gdb) helpList of classes of commands:

                                      aliases -- Aliases of other commands
                                                                          breakpoints -- Making program stop at certain points
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Before the commit mentioned above, the default ui_file->to_write
implementation had a hack that would defer into the ui_file->to_fputs
method.  The TUI's ui_file did not implement the to_write method, so
all writes would end up going to the ncurses window via tui_file_fputs
-> tui_puts.

After the commit above, the hack is gone, but the TUI's ui_file still
does not implement the ui_file::write method.  Since tui_file inherits
from stdio_file, writing to a tui_file ends up doing fwrite on the
FILE stream the TUI is "associated" with, via stdio_file::write,
instead of writing to the ncurses window.

The fix is to have tui_file override the "write" method.

New test included.

gdb/ChangeLog:
2017-03-08  Pedro Alves  <palves@redhat.com>

	PR tui/21216
	* tui/tui-file.c (tui_file::write): New.
	* tui/tui-file.h (tui_file): Override "write".
	* tui/tui-io.c (do_tui_putc, update_start_line): New functions,
	factored out from ...
	(tui_puts): ... here.
	(tui_putc): Use them.
	(tui_write): New function.
	* tui/tui-io.h (tui_write): Declare.

gdb/testsuite/ChangeLog:
2017-03-08  Pedro Alves  <palves@redhat.com>

	PR tui/21216
	* gdb.tui/tui-nl-filtered-output.exp: New file.
---
 gdb/ChangeLog                                    |  12 +++
 gdb/testsuite/ChangeLog                          |   5 +
 gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp |  57 ++++++++++++
 gdb/tui/tui-file.c                               |  10 ++
 gdb/tui/tui-file.h                               |   3 +-
 gdb/tui/tui-io.c                                 | 114 +++++++++++++++--------
 gdb/tui/tui-io.h                                 |   4 +
 7 files changed, 165 insertions(+), 40 deletions(-)
 create mode 100644 gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3a156ad..432cdcc 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,15 @@
+2017-03-08  Pedro Alves  <palves@redhat.com>
+
+	PR tui/21216
+	* tui/tui-file.c (tui_file::write): New.
+	* tui/tui-file.h (tui_file): Override "write".
+	* tui/tui-io.c (do_tui_putc, update_start_line): New functions,
+	factored out from ...
+	(tui_puts): ... here.
+	(tui_putc): Use them.
+	(tui_write): New function.
+	* tui/tui-io.h (tui_write): Declare.
+
 2017-03-07  Sergio Durigan Junior  <sergiodj@redhat.com>
 
 	* Makefile.in (SFILES): Replace "environ.c" with
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index f986520..0654bef 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@
 2017-03-08  Pedro Alves  <palves@redhat.com>
 
+	PR tui/21216
+	* gdb.tui/tui-nl-filtered-output.exp: New file.
+
+2017-03-08  Pedro Alves  <palves@redhat.com>
+
 	* gdb.base/completion.exp: Move TUI completion tests to ...
 	* gdb.tui/completion.exp: ... this new file.
 
diff --git a/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp b/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp
new file mode 100644
index 0000000..d1f56f2
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp
@@ -0,0 +1,57 @@
+# Copyright 2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Regression test for PR tui/21216 - TUI line breaks regression.
+#
+# Tests that newlines in filtered output force a "carriage return" in
+# the TUI command window.  With a broken gdb, instead of:
+#
+#  (gdb) printf "hello\nworld\n"
+#  hello
+#  world
+#  (gdb)
+#
+# we'd get:
+#
+#  (gdb) printf "hello\nworld\n"hello
+#                                    world
+#
+#  (gdb)
+
+gdb_exit
+gdb_start
+
+if {[skip_tui_tests]} {
+    return
+}
+
+# Enable the TUI.
+
+set test "tui enable"
+gdb_test_multiple "tui enable" $test {
+    -re "$gdb_prompt $" {
+	pass $test
+    }
+}
+
+# Make sure filtering/pagination is enabled, but make the window high
+# enough that we don't paginate in practice.
+gdb_test_no_output "set pagination on"
+gdb_test_no_output "set height 2000"
+
+gdb_test \
+    {printf "hello\nworld\n"} \
+    "hello\r\nworld" \
+    "correct line breaks"
diff --git a/gdb/tui/tui-file.c b/gdb/tui/tui-file.c
index 2f895b7..80a31f8 100644
--- a/gdb/tui/tui-file.c
+++ b/gdb/tui/tui-file.c
@@ -44,6 +44,16 @@ tui_file::puts (const char *linebuffer)
 }
 
 void
+tui_file::write (const char *buf, long length_buf)
+{
+  tui_write (buf, length_buf);
+  /* gdb_stdout is buffered, and the caller must gdb_flush it at
+     appropriate times.  Other streams are not so buffered.  */
+  if (this != gdb_stdout)
+    tui_refresh_cmd_win ();
+}
+
+void
 tui_file::flush ()
 {
   /* gdb_stdout is buffered.  Other files are always flushed on
diff --git a/gdb/tui/tui-file.h b/gdb/tui/tui-file.h
index aceaab6..c426a83 100644
--- a/gdb/tui/tui-file.h
+++ b/gdb/tui/tui-file.h
@@ -28,8 +28,9 @@ class tui_file : public stdio_file
 public:
   explicit tui_file (FILE *stream);
 
-  void flush () override;
+  void write (const char *buf, long length_buf) override;
   void puts (const char *) override;
+  void flush () override;
 };
 
 #endif
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 433762b..ba1ee9a 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -137,58 +137,94 @@ static int tui_readline_pipe[2];
    This may be the main gdb prompt or a secondary prompt.  */
 static char *tui_rl_saved_prompt;
 
+/* Print a character in the curses command window.  The output is
+   buffered.  It is up to the caller to refresh the screen if
+   necessary.  */
+
+static void
+do_tui_putc (WINDOW *w, char c)
+{
+  static int tui_skip_line = -1;
+
+  /* Catch annotation and discard them.  We need two \032 and discard
+     until a \n is seen.  */
+  if (c == '\032')
+    {
+      tui_skip_line++;
+    }
+  else if (tui_skip_line != 1)
+    {
+      tui_skip_line = -1;
+      /* Expand TABs, since ncurses on MS-Windows doesn't.  */
+      if (c == '\t')
+	{
+	  int col;
+
+	  col = getcurx (w);
+	  do
+	    {
+	      waddch (w, ' ');
+	      col++;
+	    }
+	  while ((col % 8) != 0);
+	}
+      else
+	waddch (w, c);
+    }
+  else if (c == '\n')
+    tui_skip_line = -1;
+}
+
+/* Update the cached value of the command window's start line based on
+   the window's current Y coordinate.  */
+
+static void
+update_cmdwin_start_line ()
+{
+  TUI_CMD_WIN->detail.command_info.start_line
+    = getcury (TUI_CMD_WIN->generic.handle);
+}
+
+/* Print a character in the curses command window.  The output is
+   buffered.  It is up to the caller to refresh the screen if
+   necessary.  */
+
 static void
 tui_putc (char c)
 {
-  char buf[2];
+  WINDOW *w = TUI_CMD_WIN->generic.handle;
+
+  do_tui_putc (w, c);
+  update_cmdwin_start_line ();
+}
+
+/* Print LENGTH characters from the buffer pointed to by BUF to the
+   curses command window.  The output is buffered.  It is up to the
+   caller to refresh the screen if necessary.  */
+
+void
+tui_write (const char *buf, size_t length)
+{
+  WINDOW *w = TUI_CMD_WIN->generic.handle;
 
-  buf[0] = c;
-  buf[1] = 0;
-  tui_puts (buf);
+  for (size_t i = 0; i < length; i++)
+    do_tui_putc (w, buf[i]);
+  update_cmdwin_start_line ();
 }
 
-/* Print the string in the curses command window.
-   The output is buffered.  It is up to the caller to refresh the screen
-   if necessary.  */
+/* Print a string in the curses command window.  The output is
+   buffered.  It is up to the caller to refresh the screen if
+   necessary.  */
 
 void
 tui_puts (const char *string)
 {
-  static int tui_skip_line = -1;
+  WINDOW *w = TUI_CMD_WIN->generic.handle;
   char c;
-  WINDOW *w;
 
-  w = TUI_CMD_WIN->generic.handle;
   while ((c = *string++) != 0)
-    {
-      /* Catch annotation and discard them.  We need two \032 and
-         discard until a \n is seen.  */
-      if (c == '\032')
-        {
-          tui_skip_line++;
-        }
-      else if (tui_skip_line != 1)
-        {
-          tui_skip_line = -1;
-	  /* Expand TABs, since ncurses on MS-Windows doesn't.  */
-	  if (c == '\t')
-	    {
-	      int col;
-
-	      col = getcurx (w);
-	      do
-		{
-		  waddch (w, ' ');
-		  col++;
-		} while ((col % 8) != 0);
-	    }
-	  else
-	    waddch (w, c);
-        }
-      else if (c == '\n')
-        tui_skip_line = -1;
-    }
-  TUI_CMD_WIN->detail.command_info.start_line = getcury (w);
+    do_tui_putc (w, c);
+  update_cmdwin_start_line ();
 }
 
 /* Readline callback.
diff --git a/gdb/tui/tui-io.h b/gdb/tui/tui-io.h
index e1d5f86..3bd465f 100644
--- a/gdb/tui/tui-io.h
+++ b/gdb/tui/tui-io.h
@@ -28,6 +28,10 @@ class cli_ui_out;
 /* Print the string in the curses command window.  */
 extern void tui_puts (const char *);
 
+/* Print LENGTH characters from the buffer pointed to by BUF to the
+   curses command window.  */
+extern void tui_write (const char *buf, size_t length);
+
 /* Setup the IO for curses or non-curses mode.  */
 extern void tui_setup_io (int mode);
 
-- 
2.5.5

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pushed] Fix PR tui/21216: TUI line breaks regression
  2017-03-08  0:19 [pushed] Fix PR tui/21216: TUI line breaks regression Pedro Alves
@ 2017-03-09 23:04 ` Jan Kratochvil
  2017-03-10 12:59   ` Jan Kratochvil
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Kratochvil @ 2017-03-09 23:04 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

On Wed, 08 Mar 2017 01:19:12 +0100, Pedro Alves wrote:
> +# Make sure filtering/pagination is enabled, but make the window high
> +# enough that we don't paginate in practice.
> +gdb_test_no_output "set pagination on"
> +gdb_test_no_output "set height 2000"

I get:
	FAIL: gdb.tui/tui-nl-filtered-output.exp: set pagination on (timeout)
	FAIL: gdb.tui/tui-nl-filtered-output.exp: set height 2000 (timeout)
But I have no idea why.

Although only during run by my testsuite tool:
	http://git.jankratochvil.net/?p=nethome.git;f=bin/hammock
So it may be also because I run the testsuite somehow wrong.

Attaching xz-ed gdb.log.


Jan

[-- Attachment #2: tui-nl-filtered-output.out.xz --]
[-- Type: application/x-xz, Size: 1400 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pushed] Fix PR tui/21216: TUI line breaks regression
  2017-03-09 23:04 ` Jan Kratochvil
@ 2017-03-10 12:59   ` Jan Kratochvil
  2017-03-10 13:33     ` Pedro Alves
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Kratochvil @ 2017-03-10 12:59 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Fri, 10 Mar 2017 00:03:59 +0100, Jan Kratochvil wrote:
> On Wed, 08 Mar 2017 01:19:12 +0100, Pedro Alves wrote:
> > +# Make sure filtering/pagination is enabled, but make the window high
> > +# enough that we don't paginate in practice.
> > +gdb_test_no_output "set pagination on"
> > +gdb_test_no_output "set height 2000"
> 
> I get:
> 	FAIL: gdb.tui/tui-nl-filtered-output.exp: set pagination on (timeout)
> 	FAIL: gdb.tui/tui-nl-filtered-output.exp: set height 2000 (timeout)
> But I have no idea why.

FAIL reproducibility requires: --with-system-readline
F-25: not reproducible
	readline-6.3-8.fc24.x86_64
	ncurses-libs-6.0-6.20160709.fc25.x86_64
F-26: not tested
Rawhide: reproducible
	readline-7.0-5.fc26.x86_64
	ncurses-libs-6.0-8.20170212.fc26.x86_64


Jan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pushed] Fix PR tui/21216: TUI line breaks regression
  2017-03-10 12:59   ` Jan Kratochvil
@ 2017-03-10 13:33     ` Pedro Alves
  2017-03-10 14:04       ` Jan Kratochvil
  0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2017-03-10 13:33 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 3393 bytes --]

On 03/10/2017 12:59 PM, Jan Kratochvil wrote:
> On Fri, 10 Mar 2017 00:03:59 +0100, Jan Kratochvil wrote:
>> On Wed, 08 Mar 2017 01:19:12 +0100, Pedro Alves wrote:
>>> +# Make sure filtering/pagination is enabled, but make the window high
>>> +# enough that we don't paginate in practice.
>>> +gdb_test_no_output "set pagination on"
>>> +gdb_test_no_output "set height 2000"
>>
>> I get:
>> 	FAIL: gdb.tui/tui-nl-filtered-output.exp: set pagination on (timeout)
>> 	FAIL: gdb.tui/tui-nl-filtered-output.exp: set height 2000 (timeout)
>> But I have no idea why.
> 
> FAIL reproducibility requires: --with-system-readline
> F-25: not reproducible
> 	readline-6.3-8.fc24.x86_64
> 	ncurses-libs-6.0-6.20160709.fc25.x86_64
> F-26: not tested
> Rawhide: reproducible
> 	readline-7.0-5.fc26.x86_64
> 	ncurses-libs-6.0-8.20170212.fc26.x86_64

Curious, I've been poking at this, and had tested with both
F23 and F25, with and without --with-system-readline without
success.  Hadn't tested F24.

From your log, up until the test does "set height 2000", looks like
curses is issuing "cursor backward" (the 17D and 15D) commands:

 (gdb) set pagination on^[[17D^[[KFAIL: gdb.tui/tui-nl-filtered-output.exp: set pagination on (timeout)
 (gdb) set height 2000^[[15D^[[KFAIL: gdb.tui/tui-nl-filtered-output.exp: set height 2000 (timeout)

I don't get those, and I guess you don't either when testing outside
hammoc.

Looks like the terminal environment under hammock is different
somehow.  Could it be the TERM env variable that influences this? 

By adding 

 gdb_test "show environment" ".*"

just before tui enable, I see that for me, TERM is always
set to vt100, no matter what TERM is set to in the shell
outside dejagnu.

/me pokes some more.

Hmm, the screen height makes a difference.  That could be it.  If I
add a few more commands, then the TUI starts issuing escape sequences
once the command line reaches the bottom.  And if I run make check
in a smaller terminal window, the pristine test starts failing
for me too, due to unexpected escape sequences.

See test patch below, and attached resulting gdb.log.  Note that viewing
the log with "less" on the terminal directly "hides" some of the escape
sequences (they get interpreted directly).

From 8973caf1689f87632e479fa3d8101a1eab827d24 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 10 Mar 2017 12:37:27 +0000
Subject: [PATCH] env

---
 gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp b/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp
index d1f56f2..fd33e70 100644
--- a/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp
+++ b/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp
@@ -37,6 +37,8 @@ if {[skip_tui_tests]} {
     return
 }
 
+gdb_test "show environment" ".*"
+
 # Enable the TUI.
 
 set test "tui enable"
@@ -48,8 +50,13 @@ gdb_test_multiple "tui enable" $test {
 
 # Make sure filtering/pagination is enabled, but make the window high
 # enough that we don't paginate in practice.
+gdb_test "show height" ".*"
 gdb_test_no_output "set pagination on"
 gdb_test_no_output "set height 2000"
+gdb_test_no_output "set height 2000"
+gdb_test_no_output "set height 2000"
+gdb_test_no_output "set height 2000"
+gdb_test_no_output "set height 2000"
 
 gdb_test \
     {printf "hello\nworld\n"} \
-- 
2.5.5



[-- Attachment #2: tui-gdb.log.gz --]
[-- Type: application/gzip, Size: 4102 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pushed] Fix PR tui/21216: TUI line breaks regression
  2017-03-10 13:33     ` Pedro Alves
@ 2017-03-10 14:04       ` Jan Kratochvil
  2017-03-10 17:20         ` Pedro Alves
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Kratochvil @ 2017-03-10 14:04 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On Fri, 10 Mar 2017 14:33:39 +0100, Pedro Alves wrote:
> See test patch below, and attached resulting gdb.log.  Note that viewing
> the log with "less" on the terminal directly "hides" some of the escape
> sequences (they get interpreted directly).

 Running gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp ...
+PASS: gdb.tui/tui-nl-filtered-output.exp: show environment
 PASS: gdb.tui/tui-nl-filtered-output.exp: tui enable
+PASS: gdb.tui/tui-nl-filtered-output.exp: show height
 PASS: gdb.tui/tui-nl-filtered-output.exp: set pagination on
 PASS: gdb.tui/tui-nl-filtered-output.exp: set height 2000
+PASS: gdb.tui/tui-nl-filtered-output.exp: set height 2000
+PASS: gdb.tui/tui-nl-filtered-output.exp: set height 2000
+PASS: gdb.tui/tui-nl-filtered-output.exp: set height 2000
+FAIL: gdb.tui/tui-nl-filtered-output.exp: set height 2000 (timeout)
 PASS: gdb.tui/tui-nl-filtered-output.exp: correct line breaks


Thanks,
Jan

[-- Attachment #2: tui-nl-filtered-output2.out.xz --]
[-- Type: application/x-xz, Size: 2940 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pushed] Fix PR tui/21216: TUI line breaks regression
  2017-03-10 14:04       ` Jan Kratochvil
@ 2017-03-10 17:20         ` Pedro Alves
  2017-03-10 17:27           ` Jan Kratochvil
  0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2017-03-10 17:20 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: gdb-patches

Fixing this properly is turning out to be much trickier than
I expected.

The main issue is that the TUI/ncurses updates/refreshes to
the command window are done by drawing only what changed,
using cursor movement.

For example, with a small enough screen, we see:

 (gdb) PASS: gdb.tui/tui-nl-filtered-output.exp: set pagination on
 set height 2000

 (gdb) PASS: gdb.tui/tui-nl-filtered-output.exp: set height 2000
 printf "hello\nworld\n"^[[2A^[[23Dprintf "hello\nworld\n"

 hello                

 world^[[K

 (gdb) FAIL: gdb.tui/tui-nl-filtered-output.exp: correct line breaks

"2A" is move up two lines, "23D" is move left 23 lines, then
print "printf "hello\nworld\n"", then "hello\nworld"
then "K" to clear the line, and finally print the prompt.

I tried to work around it by forcing empty commands in between
commands, to force the screen to clear, but the "smart" refresh makes
the output of that even harder to match.

What you were seeing:

 (gdb) set pagination on^[[17D^[[K


 (gdb) set height 2000^[[15D^[[K


... that's moving the cursor to the start of the line, and then
clearing it, making room for the next command.  I.e., both commands
appeared on the same vertical coordinate on the screen, presumably
because the command window only fitted one line or two.

So in general, we can't just strip escape sequences.  

Maybe we could pull it off by implementing a virtual terminal
that is aware of the escape sequences, understands the cursor
movement, line clearing, etc., something like the
un_ansi_vt procedure here:

 [handling of ANSI terminals using Expect]
 http://wiki.tcl.tk/9673

We'd send the test command to gdb, and feed gdb's output to that
procedure in a loop, which builds an array of lines, and then check
if the rendered command "screen" (an array of lines) had the
command result we wanted.

We'd need to match the virtual terminal's "bounding" with
the command window's size.  Ideally, we'd be able to force
the exact size of command window we want, instead of inheriting
that from the terminal dejagnu is running on.

Or maybe do something different.  Maybe dump the screen using scr_dump
and compare to a dump previously saved.  I'd need to investigate
further.

So while I think it'd be neat to do any of this, it's a lot more work
and investigation than this new test is worth it.  So for now, I'm
going to simply remove it.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pushed] Fix PR tui/21216: TUI line breaks regression
  2017-03-10 17:20         ` Pedro Alves
@ 2017-03-10 17:27           ` Jan Kratochvil
  2017-03-10 18:17             ` Pedro Alves
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Kratochvil @ 2017-03-10 17:27 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Fri, 10 Mar 2017 18:20:18 +0100, Pedro Alves wrote:
> Or maybe do something different.

Yes, I do know all these problems.  This is why I was going to separate TUI
output to a different project (with its own non-TCL testsuite) along with
separating MI, and replacing it all by some sane RPC variant later.
	https://git.jankratochvil.net/?p=gdbmicli.git;a=summary

But you ditched that and you were right, there are other debuggers which
already do it the right way.


Jan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pushed] Fix PR tui/21216: TUI line breaks regression
  2017-03-10 17:27           ` Jan Kratochvil
@ 2017-03-10 18:17             ` Pedro Alves
  2017-03-10 18:46               ` Jan Kratochvil
  0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2017-03-10 18:17 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: gdb-patches

On 03/10/2017 05:27 PM, Jan Kratochvil wrote:
> On Fri, 10 Mar 2017 18:20:18 +0100, Pedro Alves wrote:
>> Or maybe do something different.
> 
> Yes, I do know all these problems.  This is why I was going to separate TUI
> output to a different project (with its own non-TCL testsuite) along with
> separating MI, and replacing it all by some sane RPC variant later.
> 	https://git.jankratochvil.net/?p=gdbmicli.git;a=summary
> 

How you love pulling things out of context.  

Bringing up the separation-into-processes issues again suggests you're
thinking of testing at the core <-> TUI interface level.
We could have that too, by writing a unit test.  I thought of writing
one that exercised the tui_file class.  I may still write one.
Thing is, this case calls for a black-box test, to make sure that
final terminal output is how we intended.  That's why I spent the effort
to write the test, and more effort to try to preserve it.  How TUI
communicates with the core of the debugger is completely irrelevant
here.  Whatever testing you think would be appropriate for testing
TUI/ncurses _output_ in your project, we can consider for GDB too.  If
you have ideas for that, please share them.

> But you ditched that 

I didn't "ditch it".  I explained to you, with detail, why in my
opinion that wouldn't be a design we'd want.

> and you were right, there are other debuggers which
> already do it the right way.

I don't think I ever said such a thing.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pushed] Fix PR tui/21216: TUI line breaks regression
  2017-03-10 18:17             ` Pedro Alves
@ 2017-03-10 18:46               ` Jan Kratochvil
  2017-03-10 18:55                 ` Pedro Alves
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Kratochvil @ 2017-03-10 18:46 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Fri, 10 Mar 2017 19:17:20 +0100, Pedro Alves wrote:
> How you love pulling things out of context.  

Because everything is interconnected.


> Whatever testing you think would be appropriate for testing TUI/ncurses
> _output_ in your project, we can consider for GDB too.  If you have ideas
> for that, please share them.

I do not think a front end UI really needs any testsuite as it is a trivial
code (in a proper language and with proper API - not MI; MI is not API and not
proper).  That is not the case of a front end intertwingled into GDB core.

Besides that maybe one could fix a TUI frontend for the testsuite purposes.
I have no idea how much usable ncurses is with "dumb":
$ TERM=dumb gdb -tui
Cannot enable the TUI: terminal doesn't support cursor addressing [TERM=dumb]


> > and you were right, there are other debuggers which
> > already do it the right way.
> 
> I don't think I ever said such a thing.

You said something different although IMO with the same practical result.
This text from an internal list but I do not find that too secret:

On Fri, 26 Jun 2015 18:44:11 +0200, Pedro Alves wrote:
# Basically you're saying we should rewrite the whole CLI from scratch.
# And it basically seems like writing a _different_ debugger from
# scratch to me.


Jan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pushed] Fix PR tui/21216: TUI line breaks regression
  2017-03-10 18:46               ` Jan Kratochvil
@ 2017-03-10 18:55                 ` Pedro Alves
  0 siblings, 0 replies; 10+ messages in thread
From: Pedro Alves @ 2017-03-10 18:55 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: gdb-patches

On 03/10/2017 06:46 PM, Jan Kratochvil wrote:

>>> and you were right, there are other debuggers which
>>> already do it the right way.
>>
>> I don't think I ever said such a thing.
> 
> You said something different although IMO with the same practical result.

That's an incorrect interpretation.

> This text from an internal list but I do not find that too secret:
> 
> On Fri, 26 Jun 2015 18:44:11 +0200, Pedro Alves wrote:
> # Basically you're saying we should rewrite the whole CLI from scratch.
> # And it basically seems like writing a _different_ debugger from
> # scratch to me.

That I did say, but that "different" meant that it'd be a different 
debugger in the user's perspective and in the use cases that it
could support.  E.g., how with separate components in separate processes
you can't have an single integrated Python API.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-03-10 18:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-08  0:19 [pushed] Fix PR tui/21216: TUI line breaks regression Pedro Alves
2017-03-09 23:04 ` Jan Kratochvil
2017-03-10 12:59   ` Jan Kratochvil
2017-03-10 13:33     ` Pedro Alves
2017-03-10 14:04       ` Jan Kratochvil
2017-03-10 17:20         ` Pedro Alves
2017-03-10 17:27           ` Jan Kratochvil
2017-03-10 18:17             ` Pedro Alves
2017-03-10 18:46               ` Jan Kratochvil
2017-03-10 18:55                 ` Pedro Alves

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).