Fix scrolling right in the TUI (Re: [RFC 8.3 0/3] Some style fixes)
Commit Message
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(-)
Comments
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
>>>>> "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
> 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.
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
@@ -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);
}