Fix scrolling right in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

Message ID 5bb1d8b6-1ee9-3c2b-7a03-224657e2287f@redhat.com
State New
Headers show
Series
  • Fix scrolling right in the TUI (Re: [RFC 8.3 0/3] Some style fixes)
Related show

Commit Message

Pedro Alves March 14, 2019, 8:58 p.m.
On 03/13/2019 03:44 PM, Eli Zaretskii wrote:
>> Date: Tue, 12 Mar 2019 17:08:33 +0000 (UTC)

>> From: "Hannes Domani via gdb-patches" <gdb-patches@sourceware.org>

>>

>> In copy_source_line() it checks if (column < first_col), and because of the ++column directly

>> before, it basically starts with 1 instead of 0.

>>

>> Attached is a patch that fixes most of 2) and 3), but I ignored the handling of escaped

>> characters, because I just don't have them in any of my sources.

> I can confirm that this patch fixes the problems with horizontal

> scrolling, with or without TAB characters in the sources.  I think we

> should push it.


I agree.  Hannes, I've written a git commit log entry, as well
as a ChangeLog entry for you.  Please double-check whether
I didn't make some mistake.

I've made one change in the patch, here:

On 03/12/2019 05:08 PM, Hannes Domani via gdb-patches wrote:
> +      if (column <= first_col || column > first_col + line_width)

> +	{

> +	  if (c == '\t')

> +	    {

> +	      int j, max_tab_len = tui_tab_width;

> +

> +	      --column;

> +	      for (j = column % max_tab_len;

> +		   j < max_tab_len && column < first_col + line_width;

> +		   column++, j++)

> +		if (column >= first_col)

> +		  result.push_back (' ');

> +	    }

> +	  continue;

> +	}


instead of duplicating that code, I'd put it in a lambda
and use it in both places.

Tromey, WDYT?  Would you prefer the version without the lambda?

From 3b88b2cebb2d8697fbbcf957e587440ee2a4968c Mon Sep 17 00:00:00 2001
From: Hannes Domani <ssbssa@yahoo.de>

Date: Thu, 14 Mar 2019 20:46:27 +0000
Subject: [PATCH] Fix scrolling right in the TUI

This commit fixes two issues in scrolling right in the TUI:

#1 - Scrolling right with the arrow keys, the first keypress doesn't
do anything.  The problem is that copy_source_line() checks if
(column < first_col), and because of the ++column directly before, it
basically starts with 1 instead of 0.

#2 - Scrolling right handles TABS and escaped characters as single
characters, which just looks weird.  The problem is that there's a
spot that misses handling TABS.

gdb/ChangeLog:
yyyy-mm-dd  Hannes Domani  <ssbssa@yahoo.de>

	* tui/tui-source.c (copy_source_line): Fix handling of 'column'.
	Handle tabs.
---
 gdb/tui/tui-source.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

-- 
2.14.4

Comments

Simon Marchi via Gdb-patches March 15, 2019, 12:34 p.m. | #1
Am Donnerstag, 14. März 2019, 21:58:40 MEZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben: 

> I agree.  Hannes, I've written a git commit log entry, as well

> as a ChangeLog entry for you.  Please double-check whether

> I didn't make some mistake.

>

> I've made one change in the patch, here:

>

> On 03/12/2019 05:08 PM, Hannes Domani via gdb-patches wrote:

> > +      if (column <= first_col || column > first_col + line_width)

> > +    {

> > +      if (c == '\t')

> > +        {

> > +          int j, max_tab_len = tui_tab_width;

> > +

> > +          --column;

> > +          for (j = column % max_tab_len;

> > +          j < max_tab_len && column < first_col + line_width;

> > +          column++, j++)

> > +        if (column >= first_col)

> > +          result.push_back (' ');

> > +        }

> > +      continue;

> > +    }

>

> instead of duplicating that code, I'd put it in a lambda

> and use it in both places.


Since that duplicate code actually did bother me, I prefer your changes.
And I just tested it, still works fine.


Regards
Hannes Domani
Tom Tromey March 15, 2019, 9:51 p.m. | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> Tromey, WDYT?  Would you prefer the version without the lambda?

I think it's fine with them.  Thank you.

Pedro> gdb/ChangeLog:
Pedro> yyyy-mm-dd  Hannes Domani  <ssbssa@yahoo.de>

Pedro> 	* tui/tui-source.c (copy_source_line): Fix handling of 'column'.
Pedro> 	Handle tabs.

Tom
Eli Zaretskii March 17, 2019, 4:05 p.m. | #3
> Cc: gdb-patches@sourceware.org

> From: Pedro Alves <palves@redhat.com>

> Date: Thu, 14 Mar 2019 20:58:36 +0000

> 

> On 03/12/2019 05:08 PM, Hannes Domani via gdb-patches wrote:

> > +      if (column <= first_col || column > first_col + line_width)

> > +	{

> > +	  if (c == '\t')

> > +	    {

> > +	      int j, max_tab_len = tui_tab_width;

> > +

> > +	      --column;

> > +	      for (j = column % max_tab_len;

> > +		   j < max_tab_len && column < first_col + line_width;

> > +		   column++, j++)

> > +		if (column >= first_col)

> > +		  result.push_back (' ');

> > +	    }

> > +	  continue;

> > +	}

> 

> instead of duplicating that code, I'd put it in a lambda

> and use it in both places.

> 

> Tromey, WDYT?  Would you prefer the version without the lambda?


Works for me, with the initialization fixed to be 'false'.

Thanks.
Pedro Alves March 18, 2019, 2:41 p.m. | #4
On 03/15/2019 09:51 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

> Pedro> Tromey, WDYT?  Would you prefer the version without the lambda?

> 

> I think it's fine with them.  Thank you.


Merged to master and branch.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 7cc3c00069..1fa0e986e0 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -71,10 +71,27 @@  copy_source_line (const char **ptr, int line_no, int first_col,
 
       ++lineptr;
       ++column;
+
+      auto process_tab = [&] ()
+	{
+	  int max_tab_len = tui_tab_width;
+
+	  --column;
+	  for (int j = column % max_tab_len;
+	       j < max_tab_len && column < first_col + line_width;
+	       column++, j++)
+	    if (column >= first_col)
+	      result.push_back (' ');
+	};
+
       /* We have to process all the text in order to pick up all the
 	 escapes.  */
-      if (column < first_col || column > first_col + line_width)
-	continue;
+      if (column <= first_col || column > first_col + line_width)
+	{
+	  if (c == '\t')
+	    process_tab ();
+	  continue;
+	}
 
       if (c == '\n' || c == '\r' || c == '\0')
 	{
@@ -91,14 +108,7 @@  copy_source_line (const char **ptr, int line_no, int first_col,
 	  result.push_back ('?');
 	}
       else if (c == '\t')
-	{
-	  int j, max_tab_len = tui_tab_width;
-
-	  for (j = column - ((column / max_tab_len) * max_tab_len);
-	       j < max_tab_len && column < first_col + line_width;
-	       column++, j++)
-	    result.push_back (' ');
-	}
+	process_tab ();
       else
 	result.push_back (c);
     }