[review] Allow using less horizontal space in TUI source window

Message ID gerrit.1573404038000.I46ce9a68b12c9c79332d510f9c14b3c84b7efadd@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Nov. 10, 2019, 4:40 p.m. UTC
  Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/605
......................................................................

Allow using less horizontal space in TUI source window

The source window currently uses a field width of 6 for line numbers,
and it further aligns to the next tab stop.  This seemed a bit
wasteful of horizontal space to me, so I changed that in an earlier
patch.

However, that change wasn't universally popular.  This patch instead
adds the option to use less horizontal space in the TUI source window.

gdb/ChangeLog
2019-11-10  Tom Tromey  <tom@tromey.com>

	* tui/tui-winsource.h (tui_copy_source_line): Add "ndigits"
	parameter.
	* tui/tui-winsource.c (tui_copy_source_line): Add "ndigits"
	parameter.
	* tui/tui-win.h (compact_source): Declare.
	* tui/tui-win.c (compact_source): New global.
	(tui_set_compact_source, tui_show_compact_source): New functions.
	(_initialize_tui_win): Add "compact-source" setting.
	* tui/tui-source.c (tui_source_window::set_contents): Handle
	compact_source setting.
	* tui/tui-disasm.c (tui_disasm_window::set_contents): Update.
	* NEWS: Document new setting.

gdb/doc/ChangeLog
2019-11-10  Tom Tromey  <tom@tromey.com>

	* gdb.texinfo (TUI Configuration): Document new setting.

Change-Id: I46ce9a68b12c9c79332d510f9c14b3c84b7efadd
---
M gdb/ChangeLog
M gdb/NEWS
M gdb/doc/ChangeLog
M gdb/doc/gdb.texinfo
M gdb/tui/tui-disasm.c
M gdb/tui/tui-source.c
M gdb/tui/tui-win.c
M gdb/tui/tui-win.h
M gdb/tui/tui-winsource.c
M gdb/tui/tui-winsource.h
10 files changed, 102 insertions(+), 11 deletions(-)
  

Comments

Simon Marchi (Code Review) Nov. 11, 2019, 11:19 a.m. UTC | #1
Andrew Burgess has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/605
......................................................................


Patch Set 1:

(1 comment)

| --- gdb/tui/tui-source.c
| +++ gdb/tui/tui-source.c
| @@ -76,12 +80,19 @@ tui_source_window::set_contents (struct gdbarch *arch,
|  	  cur_line = 0;
|  	  gdbarch = get_objfile_arch (SYMTAB_OBJFILE (s));
|  	  start_line_or_addr.loa = LOA_LINE;
|  	  cur_line_no = start_line_or_addr.u.line_no = line_no;
|  
| +	  int digits = 0;
| +	  if (compact_source)
| +	    {
| +	      double l = log10 (offsets->size ()) + 1;
| +	      digits = 1 + (int) l;

PS1, Line 89:

If I understand correctly one of the +1's here is for the white space
that is displayed on the left of the number when it is being printed
(the calculation here always gives number of digits + 1).  However it
feels a little weird to me to have part of the display logic here, and
part of the display logic in tui_copy_source_line.

Personally I'd rather have this calculation always return the correct
number of digits and have all display logic - what whitespace is where
inside tui_copy_source_line.

| +	    }
| +
|  	  const char *iter = srclines.c_str ();
|  	  content.resize (nlines);
|  	  while (cur_line < nlines)
|  	    {
|  	      struct tui_source_element *element
|  		= &content[cur_line];
|
  
Eli Zaretskii Nov. 14, 2019, 9:55 a.m. UTC | #2
> Date: Sun, 10 Nov 2019 11:40:39 -0500
> From: "Tom Tromey (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>
> 
> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/605
> ......................................................................
> 
> Allow using less horizontal space in TUI source window
> 
> The source window currently uses a field width of 6 for line numbers,
> and it further aligns to the next tab stop.  This seemed a bit
> wasteful of horizontal space to me, so I changed that in an earlier
> patch.
> 
> However, that change wasn't universally popular.  This patch instead
> adds the option to use less horizontal space in the TUI source window.
> 
> gdb/ChangeLog
> 2019-11-10  Tom Tromey  <tom@tromey.com>
> 
> 	* tui/tui-winsource.h (tui_copy_source_line): Add "ndigits"
> 	parameter.
> 	* tui/tui-winsource.c (tui_copy_source_line): Add "ndigits"
> 	parameter.
> 	* tui/tui-win.h (compact_source): Declare.
> 	* tui/tui-win.c (compact_source): New global.
> 	(tui_set_compact_source, tui_show_compact_source): New functions.
> 	(_initialize_tui_win): Add "compact-source" setting.
> 	* tui/tui-source.c (tui_source_window::set_contents): Handle
> 	compact_source setting.
> 	* tui/tui-disasm.c (tui_disasm_window::set_contents): Update.
> 	* NEWS: Document new setting.
> 
> gdb/doc/ChangeLog
> 2019-11-10  Tom Tromey  <tom@tromey.com>
> 
> 	* gdb.texinfo (TUI Configuration): Document new setting.

OK for the documentation parts.

Thanks.
  
Simon Marchi (Code Review) Nov. 22, 2019, 11:53 p.m. UTC | #3
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/605
......................................................................


Patch Set 1:

(1 comment)

| --- gdb/tui/tui-source.c
| +++ gdb/tui/tui-source.c
| @@ -76,12 +80,19 @@ tui_source_window::set_contents (struct gdbarch *arch,
|  	  cur_line = 0;
|  	  gdbarch = get_objfile_arch (SYMTAB_OBJFILE (s));
|  	  start_line_or_addr.loa = LOA_LINE;
|  	  cur_line_no = start_line_or_addr.u.line_no = line_no;
|  
| +	  int digits = 0;
| +	  if (compact_source)
| +	    {
| +	      double l = log10 (offsets->size ()) + 1;
| +	      digits = 1 + (int) l;

PS1, Line 89:

> If I understand correctly one of the +1's here is for the white space that is displayed on the left of the number when it is being printed (the calculation here always gives number of digits + 1).

Actually I just inserted a second +1 for no reason -- it was an error.
I've corrected this.

| +	    }
| +
|  	  const char *iter = srclines.c_str ();
|  	  content.resize (nlines);
|  	  while (cur_line < nlines)
|  	    {
|  	      struct tui_source_element *element
|  		= &content[cur_line];
|
  
Simon Marchi (Code Review) Nov. 28, 2019, 10:12 a.m. UTC | #4
Andrew Burgess has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/605
......................................................................


Patch Set 2: Code-Review+2

LGTM.
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f5c8a76..9162bb7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,18 @@ 
+2019-11-10  Tom Tromey  <tom@tromey.com>
+
+	* tui/tui-winsource.h (tui_copy_source_line): Add "ndigits"
+	parameter.
+	* tui/tui-winsource.c (tui_copy_source_line): Add "ndigits"
+	parameter.
+	* tui/tui-win.h (compact_source): Declare.
+	* tui/tui-win.c (compact_source): New global.
+	(tui_set_compact_source, tui_show_compact_source): New functions.
+	(_initialize_tui_win): Add "compact-source" setting.
+	* tui/tui-source.c (tui_source_window::set_contents): Handle
+	compact_source setting.
+	* tui/tui-disasm.c (tui_disasm_window::set_contents): Update.
+	* NEWS: Document new setting.
+
 2019-11-08  Tom Tromey  <tromey@adacore.com>
 
 	* top.c (read_command_file): Update.
diff --git a/gdb/NEWS b/gdb/NEWS
index dc63179..ec86120 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -159,6 +159,14 @@ 
   'frame', 'stepi'.  The python frame filtering also respect this setting.
   The 'backtrace' '-frame-info' option can override this global setting.
 
+set tui compact-source
+show tui compact-source
+
+  Enable the "compact" display mode for the TUI source window.  The
+  compact display uses only as much space as is needed for the line
+  numbers in the current file, and only a single space to separate the
+  line numbers from the source.
+
 info modules [-q] [REGEXP]
   Return a list of Fortran modules matching REGEXP, or all modules if
   no REGEXP is given.
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index ce89ee4..30407de 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@ 
+2019-11-10  Tom Tromey  <tom@tromey.com>
+
+	* gdb.texinfo (TUI Configuration): Document new setting.
+
 2019-10-31  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* gdb.texinfo (Symbols): Document new 'info module variables' and
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 70e4be1..c3e0538 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27941,6 +27941,14 @@ 
 Set the width of tab stops to be @var{nchars} characters.  This
 setting affects the display of TAB characters in the source and
 assembly windows.
+
+@item set tui compact-source @r{[}on@r{|}off@r{]}
+@kindex set tui compact-source
+Set whether the TUI source window is displayed in ``compact'' form.
+The default display uses more space for line numbers and starts the
+source text at the next tab stop; the compact display uses only as
+much space as is needed for the line numbers in the current file, and
+only a single space to separate the line numbers from the source.
 @end table
 
 @node Emacs
diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index 8d5512e..9819cb9 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -245,7 +245,7 @@ 
 	   + asm_lines[i].insn);
 
       const char *ptr = line.c_str ();
-      src->line = tui_copy_source_line (&ptr, -1, offset, line_width);
+      src->line = tui_copy_source_line (&ptr, -1, offset, line_width, 0);
 
       src->line_or_addr.loa = LOA_ADDRESS;
       src->line_or_addr.u.addr = asm_lines[i].addr;
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 915f9e3..1a1c590 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -20,6 +20,7 @@ 
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include <math.h>
 #include <ctype.h>
 #include "symtab.h"
 #include "frame.h"
@@ -33,6 +34,7 @@ 
 #include "tui/tui-data.h"
 #include "tui/tui-io.h"
 #include "tui/tui-stack.h"
+#include "tui/tui-win.h"
 #include "tui/tui-winsource.h"
 #include "tui/tui-source.h"
 #include "gdb_curses.h"
@@ -59,8 +61,10 @@ 
       nlines = (line_no + (height - 2)) - line_no;
 
       std::string srclines;
+      const std::vector<off_t> *offsets;
       if (!g_source_cache.get_source_lines (s, line_no, line_no + nlines,
-					    &srclines))
+					    &srclines)
+	  || !g_source_cache.get_line_charpos (s, &offsets))
 	ret = TUI_FAILURE;
       else
 	{
@@ -78,6 +82,13 @@ 
 	  start_line_or_addr.loa = LOA_LINE;
 	  cur_line_no = start_line_or_addr.u.line_no = line_no;
 
+	  int digits = 0;
+	  if (compact_source)
+	    {
+	      double l = log10 (offsets->size ()) + 1;
+	      digits = 1 + (int) l;
+	    }
+
 	  const char *iter = srclines.c_str ();
 	  content.resize (nlines);
 	  while (cur_line < nlines)
@@ -89,7 +100,7 @@ 
 	      if (*iter != '\0')
 		text = tui_copy_source_line (&iter, cur_line_no,
 					     horizontal_offset,
-					     line_width);
+					     line_width, digits);
 
 	      /* Set whether element is the execution point
 		 and whether there is a break point on it.  */
diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 6df5ea2..ddc93e4 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -883,6 +883,29 @@ 
 
 }
 
+/* See tui-win.h.  */
+
+bool compact_source = false;
+
+/* Callback for "set tui compact-source".  */
+
+static void
+tui_set_compact_source (const char *ignore, int from_tty,
+			struct cmd_list_element *c)
+{
+  if (TUI_SRC_WIN != nullptr)
+    TUI_SRC_WIN->refill ();
+}
+
+/* Callback for "show tui compact-source".  */
+
+static void
+tui_show_compact_source (struct ui_file *file, int from_tty,
+			 struct cmd_list_element *c, const char *value)
+{
+  printf_filtered (_("TUI source window compactness is %s.\n"), value);
+}
+
 /* Set the tab width of the specified window.  */
 static void
 tui_set_tab_width_command (const char *arg, int from_tty)
@@ -1434,4 +1457,14 @@ 
 This variable controls how many spaces are used to display a tab character."),
 			     tui_set_tab_width, tui_show_tab_width,
 			     &tui_setlist, &tui_showlist);
+
+  add_setshow_boolean_cmd ("compact-source", class_tui,
+			   &compact_source, _("\
+Set whether the TUI source window is compact."), _("\
+Show whether the TUI source window is compact."), _("\
+This variable controls whether the TUI source window is shown\n\
+in a compact form.  The compact form puts the source closer to\n\
+the line numbers and uses less horizontal space."),
+			   tui_set_compact_source, tui_show_compact_source,
+			   &tui_setlist, &tui_showlist);
 }
diff --git a/gdb/tui/tui-win.h b/gdb/tui/tui-win.h
index e97eb67..a9e0e84 100644
--- a/gdb/tui/tui-win.h
+++ b/gdb/tui/tui-win.h
@@ -57,4 +57,7 @@ 
 /* Set a TUI variable.  */
 void tui_set_var_cmd (char *, int, struct cmd_list_element *);
 
+/* Whether compact source display should be used.  */
+extern bool compact_source;
+
 #endif /* TUI_TUI_WIN_H */
diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
index 3ca723c..81937c1 100644
--- a/gdb/tui/tui-winsource.c
+++ b/gdb/tui/tui-winsource.c
@@ -69,7 +69,7 @@ 
 
 std::string
 tui_copy_source_line (const char **ptr, int line_no, int first_col,
-		      int line_width)
+		      int line_width, int ndigits)
 {
   const char *lineptr = *ptr;
 
@@ -78,10 +78,15 @@ 
 
   if (line_no > 0)
     {
-      result = string_printf ("%-6d", line_no);
-      int len = result.size ();
-      len = len - ((len / tui_tab_width) * tui_tab_width);
-      result.append (len, ' ');
+      if (ndigits > 0)
+	result = string_printf ("%*d ", ndigits, line_no);
+      else
+	{
+	  result = string_printf ("%-6d", line_no);
+	  int len = result.size ();
+	  len = len - ((len / tui_tab_width) * tui_tab_width);
+	  result.append (len, ' ');
+	}
     }
 
   int column = 0;
diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h
index 7c3c626..8b96200 100644
--- a/gdb/tui/tui-winsource.h
+++ b/gdb/tui/tui-winsource.h
@@ -238,12 +238,16 @@ 
 /* Extract some source text from PTR.  LINE_NO is the line number.  If
    it is positive, it is printed at the start of the line.  FIRST_COL
    is the first column to extract, and LINE_WIDTH is the number of
-   characters to display.  Returns a string holding the desired text.
-   PTR is updated to point to the start of the next line.  */
+   characters to display.  NDIGITS is used to format the line number
+   (if it is positive).  If NDIGITS is greater than 0, then that many
+   digits are used; otherwise the line number is formatted with 6
+   digits and the text is aligned to the next tab stop.  Returns a
+   string holding the desired text.  PTR is updated to point to the
+   start of the next line.  */
 
 extern std::string tui_copy_source_line (const char **ptr,
 					 int line_no, int first_col,
-					 int line_width);
+					 int line_width, int ndigits);
 
 /* Constant definitions. */
 #define SCROLL_THRESHOLD 2	/* Threshold for lazy scroll.  */