Re: [PATCH v2] Fix first time you type UP or DOWN in TUI's command window (Re: [RFC 8.3 0/3] Some style fixes)

[Pedro Alves <palves@redhat.com>]

On 03/15/2019 03:37 PM, Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 15 Mar 2019 14:15:23 +0000
>>
>> Bingo.
>>
>> So I think this is the right fix:
>>
>> >From 781ed3b6d82a4fb54f7bfe59185f0e6e9efd6b59 Mon Sep 17 00:00:00 2001
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 15 Mar 2019 13:05:26 +0000
>> Subject: [PATCH] Fix first time you type UP or DOWN in TUI's command window
>>
>> The first time you type UP or DOWN arrow in the command window, GDB
>> should scroll the source window, but instead it displays the line
>> number and the file name in the command window(?).
>>
>> What happens there is that the first time we call
>> tui_ui_out::do_field_int, it doesn't initialize m_line, because
>> m_start_of_line is -1, as set by the constructor; and then the
>> following call to tui_ui_out::do_field_string falls back to
>> cli_ui_out::do_field_string because m_line is zero.
>>
>> The problem is caused by a typo in the C++ification of tui_ui_out,
>> commit 112e8700a6f, where m_line and m_start_of_line's initial values
>> were swapped from what they used to be:
> 
> Thanks, this sound right to me.  

I've merged the patch to master and the 8.3 branch.

> Still, I'd welcome some comments in
> the header which explain the semantics of non-positive values of these
> members, and for m_start_of_line, also its role in general.  Fixing
> this bug could have been much easier if that information was
> available to begin with.
For sure.  This code predates me by a long shot.  I wouldn't be
surprised if it was already in the original TUI dump from HP.

Anyway, I've stared at this for a while, and I _think_ this captures
the idea.  WDYT?

From 8e37e7076f6ddca767db35b66284268935bcc186 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 18 Mar 2019 19:10:42 +0000
Subject: [PATCH] Add comments describing tui_ui_out and its fields, cleanup a
 bit

This commit add comments describing tui_ui_out and its fields, and
cleans up the code a little bit.

Also switch to using in-class initialization so that the initial
values can be seen alongside the comments.

I see no reason for initializing m_line as -1 instead of 0, since all
the checks in the .c file are of the form "> 0".  AFAICS there's no
practical difference between -1 and 0.  So it seems simpler to
initialize it as 0.

There's a bit of redundancy in tui_ui_out::do_field_string, which is
fixed by this commit.

gdb/ChangeLog:
2019-03-18  Pedro Alves  <palves@redhat.com>

	* tui/tui-out.c (tui_ui_out::do_field_string): Simplify.
	(tui_ui_out::do_text): Add comments.  Reset M_LINE to 0 instead of
	to -1.  Fix TABs vs spaces.
	(tui_ui_out::tui_ui_out): Don't initialize fields here.
	* tui/tui-out.h (tui_ui_out) Add intro comments.
	<m_line, m_start_of_line>: In-class initialize, and add describing
	comment.
---
 gdb/tui/tui-out.c | 27 +++++++++++++--------------
 gdb/tui/tui-out.h | 21 +++++++++++++++++++--
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/gdb/tui/tui-out.c b/gdb/tui/tui-out.c
index dd37736c4a..64f77077c8 100644
--- a/gdb/tui/tui-out.c
+++ b/gdb/tui/tui-out.c
@@ -57,17 +57,13 @@ tui_ui_out::do_field_string (int fldno, int width, ui_align align,
   if (suppress_output ())
     return;
 
+  m_start_of_line++;
+
   if (fldname && m_line > 0 && strcmp (fldname, "fullname") == 0)
     {
-      m_start_of_line++;
-      if (m_line > 0)
-        {
-          tui_show_source (string, m_line);
-        }
+      tui_show_source (string, m_line);
       return;
     }
-  
-  m_start_of_line++;
 
   cli_ui_out::do_field_string (fldno, width, align, fldname, string, style);
 }
@@ -94,11 +90,16 @@ tui_ui_out::do_text (const char *string)
   m_start_of_line++;
   if (m_line > 0)
     {
+      /* Printing a source line, so suppress regular output -- the
+	 line was shown on the TUI's source window by tui_show_source
+	 above instead.  */
       if (strchr (string, '\n') != 0)
-        {
-          m_line = -1;
-          m_start_of_line = 0;
-        }
+	{
+	  /* If we've reached the end of the line, so go back to
+	     letting text output go to the console.  */
+	  m_line = 0;
+	  m_start_of_line = 0;
+	}
       return;
     }
   if (strchr (string, '\n'))
@@ -108,9 +109,7 @@ tui_ui_out::do_text (const char *string)
 }
 
 tui_ui_out::tui_ui_out (ui_file *stream)
-: cli_ui_out (stream, 0),
-  m_line (-1),
-  m_start_of_line (0)
+  : cli_ui_out (stream, 0)
 {
 }
 
diff --git a/gdb/tui/tui-out.h b/gdb/tui/tui-out.h
index 10311c9255..b0d8b8d898 100644
--- a/gdb/tui/tui-out.h
+++ b/gdb/tui/tui-out.h
@@ -20,6 +20,10 @@
 
 #include "cli-out.h"
 
+/* An ui_out class for the TUI.  This is just like the CLI's ui_out,
+   except that it overrides output methods to detect when a source
+   line is being printed and show the source in the TUI's source
+   window instead of printing the line in the console window.  */
 class tui_ui_out : public cli_ui_out
 {
 public:
@@ -39,8 +43,21 @@ protected:
 
 private:
 
-  int m_line;
-  int m_start_of_line;
+  /* These fields are used to make print_source_lines show the source
+     in the TUI's source window instead of in the console.
+     M_START_OF_LINE is incremented whenever something is output to
+     the ui_out.  If an integer field named "line" is printed on the
+     ui_out, and nothing else has been printed yet (both
+     M_START_OF_LINE and M_LINE are still 0), we assume
+     print_source_lines is starting to print a source line, and thus
+     record the line number in M_LINE.  Afterwards, when we see a
+     string field named "fullname" being output, we take the fullname
+     and the recorded line and show the source line in the TUI's
+     source window.  tui_ui_out::do_text() suppresses text output
+     until it sees an endline being printed, at which point these
+     variables are reset back to 0.  */
+  int m_line = 0;
+  int m_start_of_line = 0;
 };
 
 extern tui_ui_out *tui_out_new (struct ui_file *stream);
-- 
2.14.4