[review] Add TUI border colors

Message ID gerrit.1573404038000.Id13e2af0af2a0bde61282752f2c379db3220c9fc@gnutoolchain-gerrit.osci.io
State Superseded
Headers show
Series
  • [review] Add TUI border colors
Related show

Commit Message

Sourceware to Gerrit sync (Code Review) Nov. 10, 2019, 4:40 p.m.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/606
......................................................................

Add TUI border colors

This adds the ability to change the color of the TUI borders, both
ordinary and active.  Unlike other styling options, this doesn't allow
setting the intensity, because that is already done by the TUI in a
different way.

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

	* NEWS: Document new settings.
	* tui/tui-wingeneral.c (box_win): Apply appropriate border style.
	* tui/tui-win.c (_initialize_tui_win): Add border style
	observers.
	* tui/tui-io.h (tui_apply_style): Declare.
	* tui/tui-io.c (tui_apply_style): Rename from apply_style.  No
	longer static.
	(apply_ansi_escape, tui_set_reverse_mode): Update.
	* cli/cli-style.h (class cli_style_option) <add_setshow_commands>:
	Add "skip_intensity" parameter.
	<changed>: New member.
	<do_set_value>: Declare.
	(tui_border_style, tui_active_border_style): Declare.
	* cli/cli-style.c (tui_border_style, tui_active_border_style): New
	globals.
	(cli_style_option): Initialize "changed".
	(cli_style_option::do_set_value): New function.
	(cli_style_option::add_setshow_commands): Add "skip_intensity"
	parameter.  Update.
	(STYLE_ADD_SETSHOW_COMMANDS): Add "SKIP" parameter.
	(_initialize_cli_style): Update.  Create TUI border style
	commands.

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

	* gdb.texinfo (TUI Configuration): Mention TUI border styles.
	(Output Styling): Document new settings.

Change-Id: Id13e2af0af2a0bde61282752f2c379db3220c9fc
---
M gdb/ChangeLog
M gdb/NEWS
M gdb/cli/cli-style.c
M gdb/cli/cli-style.h
M gdb/doc/ChangeLog
M gdb/doc/gdb.texinfo
M gdb/tui/tui-io.c
M gdb/tui/tui-io.h
M gdb/tui/tui-win.c
M gdb/tui/tui-wingeneral.c
10 files changed, 150 insertions(+), 27 deletions(-)




-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id13e2af0af2a0bde61282752f2c379db3220c9fc
Gerrit-Change-Number: 606
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newchange

Comments

Eli Zaretskii Nov. 14, 2019, 9:59 a.m. | #1
> Date: Sun, 10 Nov 2019 11:40:39 -0500

> From: "Tom Tromey (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>

> 

> +TUI active border display styling.\n\

> +Configure TUI active border colors\n\

> +The \"tui-active-border\" style is used when GDB displays the border of a\n\

> +TUI window that does have the focus."), true);


Instead of "that does have the focus", I'd say "that has the focus".
I think the former builds on the doc string for tui-border, and is
less clear when read separately.

> +@item tui-border

> +Control the styling of the TUI border.  Note that, unlike other

> +styling options, only the color of the border can be controlled via

> +@code{set style}.  This was done for compatibility reasons, as TUI

> +controls to set the border's intensity predated the addition of

> +general styling to @value{GDBN}.  @xref{TUI Configuration}.

> +

> +@item tui-active-border

> +Control the styling of the active TUI border; that is, the TUI window

> +that has the focus.  Note that, unlike other styling options, only the

> +color of the border can be controlled via @code{set style}.  This was

> +done for compatibility reasons, as TUI controls to set the active

> +border's intensity predated the addition of general styling to

> +@value{GDBN}.  @xref{TUI Configuration}.


I'd suggest to have the note only once, for both options.

The documentation parts are OK with these nits fixed.

Thanks.
Sourceware to Gerrit sync (Code Review) Nov. 27, 2019, 12:13 a.m. | #2
Andrew Burgess has posted comments on this change.

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


Patch Set 2:

I spotted some weird behaviour:

  - Start up a test program,
  - 'tui enable'
  - 'layout next'
  - 'layout next'

At this point you should be in SRC/ASM/CMD layout with the ASM window focused, and its border coloured on just three sides, left, right, and bottom.

  - Scroll the view up (up arrow key), and the top border suddenly becomes coloured.

This seems a little odd!  Now,

  - 'focus prev'

You should still be in SRC/ASM/CMD layout, now with the source window focused, and its border coloured on all four sides.

  - Scroll up (press the up arrow key), you should see the bottom border loose its colour.

This seems odd.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id13e2af0af2a0bde61282752f2c379db3220c9fc
Gerrit-Change-Number: 606
Gerrit-PatchSet: 2
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 00:13:43 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Tom Tromey Dec. 1, 2019, 6:59 p.m. | #3
>>>>> "Andrew" == Andrew Burgess (Code Review) <gerrit@gnutoolchain-gerrit.osci.io> writes:


Andrew> I spotted some weird behaviour:

[...]

Andrew> At this point you should be in SRC/ASM/CMD layout with the ASM
Andrew> window focused, and its border coloured on just three sides,
Andrew> left, right, and bottom.

Andrew>   - Scroll the view up (up arrow key), and the top border suddenly becomes coloured.

Andrew> This seems a little odd!

I agree, but I tried with an unmodified gdb and I can reproduce the bug
there.  So, because it isn't introduced by this series, I'm going to go
ahead and check in these patches.

This can probably be fixed.  Maybe we can arrange to always draw the
focus window's border last.  This is a little weird, though, since then
it will always overwrite any other window title that might ordinarily
appear there.

Maybe we could also get rid of the highlighting along the borders of the
terminal, which would free up a little space for content.  Not sure.

Tom

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9162bb7..7ea57bc 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,30 @@ 
 2019-11-10  Tom Tromey  <tom@tromey.com>
 
+	* NEWS: Document new settings.
+	* tui/tui-wingeneral.c (box_win): Apply appropriate border style.
+	* tui/tui-win.c (_initialize_tui_win): Add border style
+	observers.
+	* tui/tui-io.h (tui_apply_style): Declare.
+	* tui/tui-io.c (tui_apply_style): Rename from apply_style.  No
+	longer static.
+	(apply_ansi_escape, tui_set_reverse_mode): Update.
+	* cli/cli-style.h (class cli_style_option) <add_setshow_commands>:
+	Add "skip_intensity" parameter.
+	<changed>: New member.
+	<do_set_value>: Declare.
+	(tui_border_style, tui_active_border_style): Declare.
+	* cli/cli-style.c (tui_border_style, tui_active_border_style): New
+	globals.
+	(cli_style_option): Initialize "changed".
+	(cli_style_option::do_set_value): New function.
+	(cli_style_option::add_setshow_commands): Add "skip_intensity"
+	parameter.  Update.
+	(STYLE_ADD_SETSHOW_COMMANDS): Add "SKIP" parameter.
+	(_initialize_cli_style): Update.  Create TUI border style
+	commands.
+
+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"
diff --git a/gdb/NEWS b/gdb/NEWS
index ec86120..d41a98d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -146,6 +146,14 @@ 
 set style highlight intensity VALUE
   Control the styling of highlightings.
 
+set style tui-border foreground COLOR
+set style tui-border background COLOR
+  Control the styling of TUI borders.
+
+set style tui-active-border foreground COLOR
+set style tui-active-border background COLOR
+  Control the styling of the active TUI border.
+
 maint set test-settings KIND
 maint show test-settings KIND
   A set of commands used by the testsuite for exercising the settings
diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c
index 5535d67..5955d93 100644
--- a/gdb/cli/cli-style.c
+++ b/gdb/cli/cli-style.c
@@ -85,13 +85,23 @@ 
 
 /* See cli-style.h.  */
 
+cli_style_option tui_border_style ("tui-border", ui_file_style::CYAN);
+
+/* See cli-style.h.  */
+
+cli_style_option tui_active_border_style ("tui-active-border",
+					  ui_file_style::CYAN);
+
+/* See cli-style.h.  */
+
 cli_style_option metadata_style ("metadata", ui_file_style::DIM);
 
 /* See cli-style.h.  */
 
 cli_style_option::cli_style_option (const char *name,
 				    ui_file_style::basic_color fg)
-  : m_name (name),
+  : changed (name),
+    m_name (name),
     m_foreground (cli_colors[fg - ui_file_style::NONE]),
     m_background (cli_colors[0]),
     m_intensity (cli_intensities[ui_file_style::NORMAL])
@@ -102,7 +112,8 @@ 
 
 cli_style_option::cli_style_option (const char *name,
 				    ui_file_style::intensity i)
-  : m_name (name),
+  : changed (name),
+    m_name (name),
     m_foreground (cli_colors[0]),
     m_background (cli_colors[0]),
     m_intensity (cli_intensities[i])
@@ -143,6 +154,16 @@ 
   return ui_file_style (fg, bg, intensity);
 }
 
+/* See cli-style.h.  */
+
+void
+cli_style_option::do_set_value (const char *ignore, int from_tty,
+				struct cmd_list_element *cmd)
+{
+  cli_style_option *cso = (cli_style_option *) get_cmd_context (cmd);
+  cso->changed.notify ();
+}
+
 /* Implements the cli_style_option::do_show_* functions.
    WHAT and VALUE are the property and value to show.
    The style for which WHAT is shown is retrieved from CMD context.  */
@@ -198,7 +219,8 @@ 
 							int from_tty),
 					struct cmd_list_element **show_list,
 					void (*do_show) (const char *args,
-							 int from_tty))
+							 int from_tty),
+					bool skip_intensity)
 {
   m_set_prefix = std::string ("set style ") + m_name + " ";
   m_show_prefix = std::string ("show style ") + m_name + " ";
@@ -213,7 +235,7 @@ 
 			_("Set the foreground color for this property."),
 			_("Show the foreground color for this property."),
 			nullptr,
-			nullptr,
+			do_set_value,
 			do_show_foreground,
 			&m_set_list, &m_show_list, (void *) this);
   add_setshow_enum_cmd ("background", theclass, cli_colors,
@@ -221,17 +243,18 @@ 
 			_("Set the background color for this property."),
 			_("Show the background color for this property."),
 			nullptr,
-			nullptr,
+			do_set_value,
 			do_show_background,
 			&m_set_list, &m_show_list, (void *) this);
-  add_setshow_enum_cmd ("intensity", theclass, cli_intensities,
-			&m_intensity,
-			_("Set the display intensity for this property."),
-			_("Show the display intensity for this property."),
-			nullptr,
-			nullptr,
-			do_show_intensity,
-			&m_set_list, &m_show_list, (void *) this);
+  if (!skip_intensity)
+    add_setshow_enum_cmd ("intensity", theclass, cli_intensities,
+			  &m_intensity,
+			  _("Set the display intensity for this property."),
+			  _("Show the display intensity for this property."),
+			  nullptr,
+			  do_set_value,
+			  do_show_intensity,
+			  &m_set_list, &m_show_list, (void *) this);
 }
 
 static cmd_list_element *style_set_list;
@@ -323,7 +346,7 @@ 
 			   ), set_style_enabled, show_style_sources,
 			   &style_set_list, &style_show_list);
 
-#define STYLE_ADD_SETSHOW_COMMANDS(STYLE, PREFIX_DOC)	  \
+#define STYLE_ADD_SETSHOW_COMMANDS(STYLE, PREFIX_DOC, SKIP)		\
   STYLE.add_setshow_commands (no_class, PREFIX_DOC,		\
 			      &style_set_list,				\
 			      [] (const char *args, int from_tty)	\
@@ -341,46 +364,60 @@ 
 				  (STYLE.show_list (),			\
 				   from_tty,				\
 				   "");					\
-			      })
+			      }, SKIP)
 
   STYLE_ADD_SETSHOW_COMMANDS (file_name_style,
 			      _("\
 Filename display styling.\n\
-Configure filename colors and display intensity."));
+Configure filename colors and display intensity."), false);
 
   STYLE_ADD_SETSHOW_COMMANDS (function_name_style,
 			      _("\
 Function name display styling.\n\
-Configure function name colors and display intensity"));
+Configure function name colors and display intensity"), false);
 
   STYLE_ADD_SETSHOW_COMMANDS (variable_name_style,
 			      _("\
 Variable name display styling.\n\
-Configure variable name colors and display intensity"));
+Configure variable name colors and display intensity"), false);
 
   STYLE_ADD_SETSHOW_COMMANDS (address_style,
 			      _("\
 Address display styling.\n\
-Configure address colors and display intensity"));
+Configure address colors and display intensity"), false);
 
   STYLE_ADD_SETSHOW_COMMANDS (title_style,
 			      _("\
 Title display styling.\n\
 Configure title colors and display intensity\n\
 Some commands (such as \"apropos -v REGEXP\") use the title style to improve\n\
-readability."));
+readability."), false);
 
   STYLE_ADD_SETSHOW_COMMANDS (highlight_style,
 			      _("\
 Highlight display styling.\n\
 Configure highlight colors and display intensity\n\
 Some commands use the highlight style to draw the attention to a part\n\
-of their output."));
+of their output."), false);
 
   STYLE_ADD_SETSHOW_COMMANDS (metadata_style,
 			      _("\
 Metadata display styling.\n\
 Configure metadata colors and display intensity\n\
 The \"metadata\" style is used when GDB displays information about\n\
-your data, for example \"<unavailable>\""));
+your data, for example \"<unavailable>\""), false);
+
+  STYLE_ADD_SETSHOW_COMMANDS (tui_border_style,
+			      _("\
+TUI border display styling.\n\
+Configure TUI border colors\n\
+The \"tui-border\" style is used when GDB displays the border of a\n\
+TUI window that does not have the focus."), true);
+
+  STYLE_ADD_SETSHOW_COMMANDS (tui_active_border_style,
+			      _("\
+TUI active border display styling.\n\
+Configure TUI active border colors\n\
+The \"tui-active-border\" style is used when GDB displays the border of a\n\
+TUI window that does have the focus."), true);
 }
diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h
index 6716471..a14e81d 100644
--- a/gdb/cli/cli-style.h
+++ b/gdb/cli/cli-style.h
@@ -22,6 +22,7 @@ 
 
 #include "ui-file.h"
 #include "command.h"
+#include "gdbsupport/observable.h"
 
 /* A single CLI style option.  */
 class cli_style_option
@@ -50,7 +51,8 @@ 
 			     struct cmd_list_element **set_list,
 			     void (*do_set) (const char *args, int from_tty),
 			     struct cmd_list_element **show_list,
-			     void (*do_show) (const char *args, int from_tty));
+			     void (*do_show) (const char *args, int from_tty),
+			     bool skip_intensity);
 
   /* Return the 'set style NAME' command list, that can be used
      to build a lambda DO_SET to call add_setshow_commands.  */
@@ -59,6 +61,9 @@ 
   /* Same as SET_LIST but for the show command list.  */
   struct cmd_list_element *show_list () { return m_show_list; };
 
+  /* This style can be observed for any changes.  */
+  gdb::observers::observable<> changed;
+
 private:
 
   /* The style name.  */
@@ -79,6 +84,10 @@ 
   struct cmd_list_element *m_set_list = nullptr;
   struct cmd_list_element *m_show_list = nullptr;
 
+  /* Callback to notify the observable.  */
+  static void do_set_value (const char *ignore, int from_tty,
+			    struct cmd_list_element *cmd);
+
   /* Callback to show the foreground.  */
   static void do_show_foreground (struct ui_file *file, int from_tty,
 				  struct cmd_list_element *cmd,
@@ -114,6 +123,12 @@ 
 /* The metadata style.  */
 extern cli_style_option metadata_style;
 
+/* The border style of a TUI window that does not have the focus.  */
+extern cli_style_option tui_border_style;
+
+/* The border style of a TUI window that does have the focus.  */
+extern cli_style_option tui_active_border_style;
+
 /* True if source styling is enabled.  */
 extern bool source_styling;
 
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 30407de..d983e76 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,5 +1,10 @@ 
 2019-11-10  Tom Tromey  <tom@tromey.com>
 
+	* gdb.texinfo (TUI Configuration): Mention TUI border styles.
+	(Output Styling): Document new settings.
+
+2019-11-10  Tom Tromey  <tom@tromey.com>
+
 	* gdb.texinfo (TUI Configuration): Document new setting.
 
 2019-10-31  Andrew Burgess  <andrew.burgess@embecosm.com>
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index c3e0538..c09a523 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -25442,6 +25442,21 @@ 
 the command @command{apropos -v REGEXP} uses the highlight style to
 mark the documentation parts matching @var{regexp}.
 
+@item tui-border
+Control the styling of the TUI border.  Note that, unlike other
+styling options, only the color of the border can be controlled via
+@code{set style}.  This was done for compatibility reasons, as TUI
+controls to set the border's intensity predated the addition of
+general styling to @value{GDBN}.  @xref{TUI Configuration}.
+
+@item tui-active-border
+Control the styling of the active TUI border; that is, the TUI window
+that has the focus.  Note that, unlike other styling options, only the
+color of the border can be controlled via @code{set style}.  This was
+done for compatibility reasons, as TUI controls to set the active
+border's intensity predated the addition of general styling to
+@value{GDBN}.  @xref{TUI Configuration}.
+
 @end table
 
 @node Numbers
@@ -27951,6 +27966,9 @@ 
 only a single space to separate the line numbers from the source.
 @end table
 
+Note that the colors of the TUI borders can be controlled using the
+appropriate @code{set style} commands.  @xref{Output Styling}.
+
 @node Emacs
 @chapter Using @value{GDBN} under @sc{gnu} Emacs
 
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 964f2e3..2eef288 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -306,8 +306,8 @@ 
 
 /* Apply STYLE to W.  */
 
-static void
-apply_style (WINDOW *w, ui_file_style style)
+void
+tui_apply_style (WINDOW *w, ui_file_style style)
 {
   /* Reset.  */
   wattron (w, A_NORMAL);
@@ -413,7 +413,7 @@ 
       style.set_reverse (true);
     }
 
-  apply_style (w, style);
+  tui_apply_style (w, style);
   return n_read;
 }
 
@@ -438,7 +438,7 @@ 
       style.set_fg (reverse_save_fg);
     }
 
-  apply_style (w, style);
+  tui_apply_style (w, style);
 }
 
 /* Print LENGTH characters from the buffer pointed to by BUF to the
diff --git a/gdb/tui/tui-io.h b/gdb/tui/tui-io.h
index ec23787..01bfe45 100644
--- a/gdb/tui/tui-io.h
+++ b/gdb/tui/tui-io.h
@@ -51,6 +51,9 @@ 
 /* Enter/leave reverse video mode.  */
 extern void tui_set_reverse_mode (WINDOW *w, bool reverse);
 
+/* Apply STYLE to the window.  */
+extern void tui_apply_style (WINDOW *w, ui_file_style style);
+
 extern struct ui_out *tui_out;
 extern cli_ui_out *tui_old_uiout;
 
diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index ddc93e4..684936d 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -30,6 +30,7 @@ 
 #include "breakpoint.h"
 #include "frame.h"
 #include "cli/cli-cmds.h"
+#include "cli/cli-style.h"
 #include "top.h"
 #include "source.h"
 #include "event-loop.h"
@@ -1467,4 +1468,7 @@ 
 the line numbers and uses less horizontal space."),
 			   tui_set_compact_source, tui_show_compact_source,
 			   &tui_setlist, &tui_showlist);
+
+  tui_border_style.changed.attach (tui_rehighlight_all);
+  tui_active_border_style.changed.attach (tui_rehighlight_all);
 }
diff --git a/gdb/tui/tui-wingeneral.c b/gdb/tui/tui-wingeneral.c
index b6dd3f9..e139368 100644
--- a/gdb/tui/tui-wingeneral.c
+++ b/gdb/tui/tui-wingeneral.c
@@ -22,9 +22,11 @@ 
 #include "defs.h"
 #include "tui/tui.h"
 #include "tui/tui-data.h"
+#include "tui/tui-io.h"
 #include "tui/tui-wingeneral.h"
 #include "tui/tui-win.h"
 #include "tui/tui-stack.h"
+#include "cli/cli-style.h"
 
 #include "gdb_curses.h"
 
@@ -51,6 +53,11 @@ 
   else
     attrs = tui_border_attrs;
 
+  /* tui_apply_style resets the style entirely, so be sure to call it
+     before applying ATTRS.  */
+  tui_apply_style (win, (highlight_flag
+			 ? tui_active_border_style.style ()
+			 : tui_border_style.style ()));
   wattron (win, attrs);
 #ifdef HAVE_WBORDER
   wborder (win, tui_border_vline, tui_border_vline,
@@ -77,6 +84,7 @@ 
 	}
     }
   wattroff (win, attrs);
+  tui_apply_style (win, ui_file_style ());
 }