Re: [RFC][PATCH] Call tui_before_prompt in do_scroll_vertical

[Tom Tromey <tom@tromey.com>]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

>> First I tried it with tui_update_source_windows_with_line, but this didn't
>> reset from_source_symtab (which was set deep in print_source_lines),
>> which resulted in some weird behavior when switching from "layout split"
>> to "layout asm" after scrolling down in the src window (the asm window
>> was then overwritten by the src window).

I wonder if we really need to change the current source symtab.
If so, I guess the TUI could just call
set_current_source_symtab_and_line directly.

The patch below removes the call to print_source_lines -- this approach
just seems too roundabout for my taste.

Andrew> I was able to reproduce the layout split -> layout asm issue you
Andrew> describe, but, I'm not convinced that the solution below is the right
Andrew> way to go.

With my patch I can't reproduce this, but I don't know whether I'm doing
it properly.

Hannes, could you possibly try this patch?

This could maybe be made even a bit smaller, but it's hard to be sure.
Despite all the cleanups, I find this area still pretty confusing.

Tom

commit 5a30635b352a9c0ff896c5044296087a3eb42073
Author: Tom Tromey <tom@tromey.com>
Date:   Sun Dec 22 16:52:56 2019 -0700

    Fix scrolling in TUI
    
    Hannes Domani pointed out that my previous patch to fix the "list"
    command in the TUI instead broke vertical scrolling.  While looking at
    this, I found that do_scroll_vertical calls print_source_lines, which
    seems like a very roundabout way to change the source window.  This
    patch removes this oddity and fixes the bug at the same time.
    
    I've added a new test case.  This is somewhat tricky, because the
    obvious approach of sending a dummy command after the scroll did not
    work -- due to how the TUI works, sennding a command causes the scroll
    to take effect.
    
    gdb/ChangeLog
    2019-12-22  Tom Tromey  <tom@tromey.com>
    
            PR tui/18932:
            * tui/tui-source.c (tui_source_window::do_scroll_vertical): Call
            update_source_window, not print_source_lines.
    
    gdb/testsuite/ChangeLog
    2019-12-22  Tom Tromey  <tom@tromey.com>
    
            PR tui/18932:
            * lib/tuiterm.exp (Term::wait_for): Rename from _accept.  Return a
            meangingful value.
            (Term::command, Term::resize): Update.
            * gdb.tui/basic.exp: Add scrolling test.
    
    Change-Id: I9636a7c8a8cade37431c6165ee996a9d556ef1c8

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0f35deff350..b441c9d5031 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-12-22  Tom Tromey  <tom@tromey.com>
+
+	PR tui/18932:
+	* tui/tui-source.c (tui_source_window::do_scroll_vertical): Call
+	update_source_window, not print_source_lines.
+
 2019-12-20  Tom Tromey  <tom@tromey.com>
 
 	* tui/tui.c (tui_show_source): Remove.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 4476549be6b..985a68bc126 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2019-12-22  Tom Tromey  <tom@tromey.com>
+
+	PR tui/18932:
+	* lib/tuiterm.exp (Term::wait_for): Rename from _accept.  Return a
+	meangingful value.
+	(Term::command, Term::resize): Update.
+	* gdb.tui/basic.exp: Add scrolling test.
+
 2019-12-20  Tom Tromey  <tom@tromey.com>
 
 	* gdb.tui/list-before.exp: New file.
diff --git a/gdb/testsuite/gdb.tui/basic.exp b/gdb/testsuite/gdb.tui/basic.exp
index f9d57d1cf6d..ca88dad1cd4 100644
--- a/gdb/testsuite/gdb.tui/basic.exp
+++ b/gdb/testsuite/gdb.tui/basic.exp
@@ -35,6 +35,17 @@ gdb_assert {![string match "No Source Available" $text]} \
 Term::command "list main"
 Term::check_contents "list main" "21 *return 0"
 
+# Get the first source line.
+set line [string_to_regexp [Term::get_line 1]]
+# Send an up arrow.
+send_gdb "\033\[A"
+# Wait for a redraw and check that the first line changed.
+if {[Term::wait_for $line] && [Term::get_line 1] != $line} {
+    pass "scroll up"
+} else {
+    fail "scroll up"
+}
+
 Term::check_box "source box" 0 0 80 15
 
 Term::command "layout asm"
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 81247d5d9a4..7761c63d6aa 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -388,8 +388,10 @@ namespace eval Term {
 	_clear_lines 0 $_rows
     }
 
-    # Accept some output from gdb and update the screen.
-    proc _accept {wait_for} {
+    # Accept some output from gdb and update the screen.  WAIT_FOR is
+    # a regexp matching the line to wait for.  Return 0 on timeout, 1
+    # on success.
+    proc wait_for {wait_for} {
 	global expect_out
 	global gdb_prompt
 	variable _cur_x
@@ -424,7 +426,7 @@ namespace eval Term {
 		timeout {
 		    # Assume a timeout means we somehow missed the
 		    # expected result, and carry on.
-		    return
+		    return 0
 		}
 	    }
 
@@ -443,6 +445,8 @@ namespace eval Term {
 		set wait_for $prompt_wait_for
 	    }
 	}
+
+	return 1
     }
 
     # Like ::clean_restart, but ensures that gdb starts in an
@@ -480,7 +484,7 @@ namespace eval Term {
     # be supplied by this function.
     proc command {cmd} {
 	send_gdb "$cmd\n"
-	_accept [string_to_regexp $cmd]
+	wait_for [string_to_regexp $cmd]
     }
 
     # Return the text of screen line N, without attributes.  Lines are
@@ -641,14 +645,14 @@ namespace eval Term {
 	# Due to the strange column resizing behavior, and because we
 	# don't care about this intermediate resize, we don't check
 	# the size here.
-	_accept "@@ resize done $_resize_count"
+	wait_for "@@ resize done $_resize_count"
 	incr _resize_count
 	# Somehow the number of columns transmitted to gdb is one less
 	# than what we request from expect.  We hide this weird
 	# details from the caller.
 	_do_resize $_rows $cols
 	stty columns [expr {$_cols + 1}] < $gdb_spawn_name
-	_accept "@@ resize done $_resize_count, size = ${_cols}x${rows}"
+	wait_for "@@ resize done $_resize_count, size = ${_cols}x${rows}"
 	incr _resize_count
     }
 }
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 0728263b8c5..bcba9e176f1 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -136,28 +136,29 @@ tui_source_window::do_scroll_vertical (int num_to_scroll)
 {
   if (!content.empty ())
     {
-      struct tui_line_or_address l;
       struct symtab *s;
       struct symtab_and_line cursal = get_current_source_symtab_and_line ();
+      struct gdbarch *arch = gdbarch;
 
       if (cursal.symtab == NULL)
-	s = find_pc_line_symtab (get_frame_pc (get_selected_frame (NULL)));
+	{
+	  struct frame_info *fi = get_selected_frame (NULL);
+	  s = find_pc_line_symtab (get_frame_pc (fi));
+	  arch = get_frame_arch (fi);
+	}
       else
 	s = cursal.symtab;
 
-      l.loa = LOA_LINE;
-      l.u.line_no = start_line_or_addr.u.line_no
-	+ num_to_scroll;
+      int line_no = start_line_or_addr.u.line_no + num_to_scroll;
       const std::vector<off_t> *offsets;
       if (g_source_cache.get_line_charpos (s, &offsets)
-	  && l.u.line_no > offsets->size ())
-	/* line = s->nlines - win_info->content_size + 1; */
-	/* elz: fix for dts 23398.  */
-	l.u.line_no = start_line_or_addr.u.line_no;
-      if (l.u.line_no <= 0)
-	l.u.line_no = 1;
-
-      print_source_lines (s, l.u.line_no, l.u.line_no + 1, 0);
+	  && line_no > offsets->size ())
+	line_no = start_line_or_addr.u.line_no;
+      if (line_no <= 0)
+	line_no = 1;
+
+      cursal.line = line_no;
+      update_source_window (arch, cursal);
     }
 }