[PATCH] gdb/testing/tui: add new _csi_{L,P,S,T}

[PATCH] gdb/testing/tui: add new _csi_{L,P,S,T}

[Andrew Burgess]

This patch was original part of this series:

  https://sourceware.org/pipermail/gdb-patches/2022-March/186429.html
  https://sourceware.org/pipermail/gdb-patches/2022-March/186433.html

But these missing control sequences are now hitting others:

  https://sourceware.org/pipermail/gdb-patches/2022-March/187075.html

So I'm pulling this patch out and posting it on its own.

I've rebased this work on top of Simon's gdb.tui/tuiterm.exp test
script, and added some tests for the new control sequences.  As a
result I found a few bugs in my original implementation, which are
fixed in this version.

For completeness I added _csi_T (pan up) to complement _csi_S (pan
down).  The _csi_T was not in my original patch.

This commit adds:

  _csi_L - insert line
  _csi_P - delete characters
  _csi_S - pan down
  _csi_T - pan up
---
 gdb/testsuite/gdb.tui/tuiterm.exp | 115 ++++++++++++++++++++++++++++++
 gdb/testsuite/lib/tuiterm.exp     | 108 ++++++++++++++++++++++++++++
 2 files changed, 223 insertions(+)

diff --git a/gdb/testsuite/gdb.tui/tuiterm.exp b/gdb/testsuite/gdb.tui/tuiterm.exp
index 6b1b044535a..4aef8483ab2 100644
--- a/gdb/testsuite/gdb.tui/tuiterm.exp
+++ b/gdb/testsuite/gdb.tui/tuiterm.exp
@@ -179,6 +179,60 @@ proc test_insert_characters { } {
     } 0 1
 }
 
+proc test_pan_down { } {
+    Term::_move_cursor 1 2
+    Term::_csi_S
+    check "pan down, default arg" {
+	"ijklmnop"
+	"qrstuvwx"
+	"yz01234 "
+	"        "
+    } 1 2
+
+    Term::_csi_S 2
+    check "pan down, explicit arg" {
+	"yz01234 "
+	"        "
+	"        "
+	"        "
+    } 1 2
+
+    Term::_csi_S 100
+    check "pan down, excessive arg" {
+	"        "
+	"        "
+	"        "
+	"        "
+    } 1 2
+}
+
+proc test_pan_up { } {
+    Term::_move_cursor 1 2
+    Term::_csi_T
+    check "pan down, default arg" {
+	"        "
+	"abcdefgh"
+	"ijklmnop"
+	"qrstuvwx"
+    } 1 2
+
+    Term::_csi_T 2
+    check "pan down, explicit arg" {
+	"        "
+	"        "
+	"        "
+	"abcdefgh"
+    } 1 2
+
+    Term::_csi_T 100
+    check "pan down, excessive arg" {
+	"        "
+	"        "
+	"        "
+	"        "
+    } 1 2
+}
+
 proc test_cursor_up { } {
     Term::_move_cursor 2 3
 
@@ -512,6 +566,35 @@ proc test_erase_character { } {
     } 1 3
 }
 
+proc test_delete_character { } {
+    Term::_move_cursor 3 2
+    Term::_csi_P
+    check "delete character, default param" {
+	"abcdefgh"
+	"ijklmnop"
+	"qrsuvwx "
+	"yz01234 "
+    } 3 2
+
+    Term::_move_cursor 1 1
+    Term::_csi_P 4
+    check "erase character, explicit param" {
+	"abcdefgh"
+	"inop    "
+	"qrsuvwx "
+	"yz01234 "
+    } 1 1
+
+    Term::_move_cursor 4 0
+    Term::_csi_P 10
+    check "attempt to erase more characters than are available" {
+	"abcd    "
+	"inop    "
+	"qrsuvwx "
+	"yz01234 "
+    } 4 0
+}
+
 proc test_cursor_backward_tabulation { } {
     Term::_move_cursor 77 2
     Term::_csi_Z
@@ -566,6 +649,34 @@ proc test_vertical_line_position_absolute { } {
     } 2 3
 }
 
+proc test_insert_line { } {
+    Term::_move_cursor 2 1
+    Term::_csi_L
+    check "insert line, default param" {
+	"abcdefgh"
+	"        "
+	"ijklmnop"
+	"qrstuvwx"
+    } 2 1
+
+    Term::_move_cursor 2 0
+    Term::_csi_L 2
+    check "insert line, explicit param" {
+	"        "
+	"        "
+	"abcdefgh"
+	"        "
+    } 2 0
+
+    Term::_csi_L 12
+    check "insert line, insert more lines than display has" {
+	"        "
+	"        "
+	"        "
+	"        "
+    } 2 0
+}
+
 # Run proc TEST_PROC_NAME with a "small" terminal.
 
 proc run_one_test_small { test_proc_name } {
@@ -603,6 +714,10 @@ foreach_with_prefix test {
     test_erase_character
     test_repeat
     test_vertical_line_position_absolute
+    test_insert_line
+    test_delete_character
+    test_pan_up
+    test_pan_down
 } {
     run_one_test_small $test
 }
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 7696fea4c7e..72f4a3498bf 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -324,6 +324,33 @@ namespace eval Term {
 	}
     }
 
+    # Insert Line
+    #
+    # https://vt100.net/docs/vt510-rm/IL.html
+    proc _csi_L {args} {
+	set arg [_default [lindex $args 0] 1]
+
+	_log_cur "Insert Line ($arg)" {
+	    variable _cur_col
+	    variable _cur_row
+	    variable _rows
+	    variable _cols
+	    variable _chars
+
+	    set y [expr $_rows - 2]
+	    set next_y [expr $y + $arg]
+	    while {$y >= $_cur_row} {
+		for {set x 0} {$x < $_cols} {incr x} {
+		    set _chars($x,$next_y) $_chars($x,$y)
+		}
+		incr y -1
+		incr next_y -1
+	    }
+
+	    _clear_lines $_cur_row [expr $_cur_row + $arg]
+	}
+    }
+
     # Delete line.
     #
     # https://vt100.net/docs/vt510-rm/DL.html
@@ -349,6 +376,87 @@ namespace eval Term {
 	}
     }
 
+    # Delete Characters
+    #
+    # https://vt100.net/docs/vt510-rm/DCH.html
+    proc _csi_P {args} {
+	set count [_default [lindex $args 0] 1]
+
+	_log_cur "Delete Character ($count)" {
+	    variable _cur_col
+	    variable _cur_row
+	    variable _cols
+	    variable _chars
+
+	    set dx [expr $_cur_col]
+	    set sx [expr $_cur_col + $count]
+	    set y $_cur_row
+
+	    while {$sx < $_cols} {
+		set _chars($dx,$y) $_chars($sx,$y)
+		incr sx
+		incr dx
+	    }
+	    _clear_in_line $dx $sx $y
+	}
+    }
+
+    # Pan Down
+    #
+    # https://vt100.net/docs/vt510-rm/SU.html
+    proc _csi_S {args} {
+	set count [_default [lindex $args 0] 1]
+
+	_log_cur "Pan Down ($count)" {
+	    variable _cur_col
+	    variable _cur_row
+	    variable _cols
+	    variable _rows
+	    variable _chars
+
+	    set dy 0
+	    set y $count
+
+	    while {$y < $_rows} {
+		for {set x 0} {$x < $_cols} {incr x} {
+		    set _chars($x,$dy) $_chars($x,$y)
+		}
+		incr y 1
+		incr dy 1
+	    }
+
+	    _clear_lines $dy $_rows
+	}
+    }
+
+    # Pan Up
+    #
+    # https://vt100.net/docs/vt510-rm/SD.html
+    proc _csi_T {args} {
+	set count [_default [lindex $args 0] 1]
+
+	_log_cur "Pan Up ($count)" {
+	    variable _cur_col
+	    variable _cur_row
+	    variable _cols
+	    variable _rows
+	    variable _chars
+
+	    set y [expr $_rows - $count]
+	    set dy $_rows
+
+	    while {$dy >= $count} {
+		for {set x 0} {$x < $_cols} {incr x} {
+		    set _chars($x,$dy) $_chars($x,$y)
+		}
+		incr y -1
+		incr dy -1
+	    }
+
+	    _clear_lines 0 $count
+	}
+    }
+
     # Erase chars.
     #
     # https://vt100.net/docs/vt510-rm/ECH.html
-- 
2.25.4

Re: [PATCH] gdb/testing/tui: add new _csi_{L,P,S,T}

[Andrew Burgess]

* Andrew Burgess <aburgess@redhat.com> [2022-03-31 22:51:48 +0100]:

> This patch was original part of this series:
> 
>   https://sourceware.org/pipermail/gdb-patches/2022-March/186429.html
>   https://sourceware.org/pipermail/gdb-patches/2022-March/186433.html
> 
> But these missing control sequences are now hitting others:
> 
>   https://sourceware.org/pipermail/gdb-patches/2022-March/187075.html
> 
> So I'm pulling this patch out and posting it on its own.
> 
> I've rebased this work on top of Simon's gdb.tui/tuiterm.exp test
> script, and added some tests for the new control sequences.  As a
> result I found a few bugs in my original implementation, which are
> fixed in this version.
> 
> For completeness I added _csi_T (pan up) to complement _csi_S (pan
> down).  The _csi_T was not in my original patch.
> 
> This commit adds:
> 
>   _csi_L - insert line
>   _csi_P - delete characters
>   _csi_S - pan down
>   _csi_T - pan up

Sorry, I forgot to pull the latest master before rebasing this commit,
so I didn't spot that Simon already pushed his _csi_P patch.

Here's an update without the _csi_P stuff.

Thanks,
Andrew

---

commit 85625032b5b798ef082a634821ad8567923e9071
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Jan 26 18:41:59 2022 +0000

    gdb/testing/tui: add new _csi_{L,S,T}
    
    This patch was original part of this series:
    
      https://sourceware.org/pipermail/gdb-patches/2022-March/186429.html
      https://sourceware.org/pipermail/gdb-patches/2022-March/186433.html
    
    I've pulled this out as it might be useful ahead of the bigger series
    being merged.
    
    This commit adds:
    
      _csi_L - insert line
      _csi_S - pan down
      _csi_T - pan up

diff --git a/gdb/testsuite/gdb.tui/tuiterm.exp b/gdb/testsuite/gdb.tui/tuiterm.exp
index c82db21a92b..1dcfb7f602d 100644
--- a/gdb/testsuite/gdb.tui/tuiterm.exp
+++ b/gdb/testsuite/gdb.tui/tuiterm.exp
@@ -179,6 +179,60 @@ proc test_insert_characters { } {
     } 0 1
 }
 
+proc test_pan_down { } {
+    Term::_move_cursor 1 2
+    Term::_csi_S
+    check "pan down, default arg" {
+	"ijklmnop"
+	"qrstuvwx"
+	"yz01234 "
+	"        "
+    } 1 2
+
+    Term::_csi_S 2
+    check "pan down, explicit arg" {
+	"yz01234 "
+	"        "
+	"        "
+	"        "
+    } 1 2
+
+    Term::_csi_S 100
+    check "pan down, excessive arg" {
+	"        "
+	"        "
+	"        "
+	"        "
+    } 1 2
+}
+
+proc test_pan_up { } {
+    Term::_move_cursor 1 2
+    Term::_csi_T
+    check "pan down, default arg" {
+	"        "
+	"abcdefgh"
+	"ijklmnop"
+	"qrstuvwx"
+    } 1 2
+
+    Term::_csi_T 2
+    check "pan down, explicit arg" {
+	"        "
+	"        "
+	"        "
+	"abcdefgh"
+    } 1 2
+
+    Term::_csi_T 100
+    check "pan down, excessive arg" {
+	"        "
+	"        "
+	"        "
+	"        "
+    } 1 2
+}
+
 proc test_cursor_up { } {
     Term::_move_cursor 2 3
 
@@ -492,34 +546,6 @@ proc test_delete_line { } {
     } 3 0
 }
 
-proc test_delete_character { } {
-    Term::_move_cursor 2 1
-
-    Term::_csi_P
-    check "delete character, default param" {
-	"abcdefgh"
-	"ijlmnop "
-	"qrstuvwx"
-	"yz01234 "
-    } 2 1
-
-    Term::_csi_P 3
-    check "delete character, explicit param" {
-	"abcdefgh"
-	"ijop    "
-	"qrstuvwx"
-	"yz01234 "
-    } 2 1
-
-    Term::_csi_P 12
-    check "delete character, more than number of columns" {
-	"abcdefgh"
-	"ij      "
-	"qrstuvwx"
-	"yz01234 "
-    } 2 1
-}
-
 proc test_erase_character { } {
     Term::_move_cursor 3 2
     Term::_csi_X
@@ -540,6 +566,35 @@ proc test_erase_character { } {
     } 1 3
 }
 
+proc test_delete_character { } {
+    Term::_move_cursor 3 2
+    Term::_csi_P
+    check "delete character, default param" {
+	"abcdefgh"
+	"ijklmnop"
+	"qrsuvwx "
+	"yz01234 "
+    } 3 2
+
+    Term::_move_cursor 1 1
+    Term::_csi_P 4
+    check "erase character, explicit param" {
+	"abcdefgh"
+	"inop    "
+	"qrsuvwx "
+	"yz01234 "
+    } 1 1
+
+    Term::_move_cursor 4 0
+    Term::_csi_P 10
+    check "attempt to erase more characters than are available" {
+	"abcd    "
+	"inop    "
+	"qrsuvwx "
+	"yz01234 "
+    } 4 0
+}
+
 proc test_cursor_backward_tabulation { } {
     Term::_move_cursor 77 2
     Term::_csi_Z
@@ -594,6 +649,34 @@ proc test_vertical_line_position_absolute { } {
     } 2 3
 }
 
+proc test_insert_line { } {
+    Term::_move_cursor 2 1
+    Term::_csi_L
+    check "insert line, default param" {
+	"abcdefgh"
+	"        "
+	"ijklmnop"
+	"qrstuvwx"
+    } 2 1
+
+    Term::_move_cursor 2 0
+    Term::_csi_L 2
+    check "insert line, explicit param" {
+	"        "
+	"        "
+	"abcdefgh"
+	"        "
+    } 2 0
+
+    Term::_csi_L 12
+    check "insert line, insert more lines than display has" {
+	"        "
+	"        "
+	"        "
+	"        "
+    } 2 0
+}
+
 # Run proc TEST_PROC_NAME with a "small" terminal.
 
 proc run_one_test_small { test_proc_name } {
@@ -632,6 +715,9 @@ foreach_with_prefix test {
     test_erase_character
     test_repeat
     test_vertical_line_position_absolute
+    test_insert_line
+    test_pan_up
+    test_pan_down
 } {
     run_one_test_small $test
 }
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 9053f7dba6a..7d9d651a1fb 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -324,6 +324,33 @@ namespace eval Term {
 	}
     }
 
+    # Insert Line
+    #
+    # https://vt100.net/docs/vt510-rm/IL.html
+    proc _csi_L {args} {
+	set arg [_default [lindex $args 0] 1]
+
+	_log_cur "Insert Line ($arg)" {
+	    variable _cur_col
+	    variable _cur_row
+	    variable _rows
+	    variable _cols
+	    variable _chars
+
+	    set y [expr $_rows - 2]
+	    set next_y [expr $y + $arg]
+	    while {$y >= $_cur_row} {
+		for {set x 0} {$x < $_cols} {incr x} {
+		    set _chars($x,$next_y) $_chars($x,$y)
+		}
+		incr y -1
+		incr next_y -1
+	    }
+
+	    _clear_lines $_cur_row [expr $_cur_row + $arg]
+	}
+    }
+
     # Delete line.
     #
     # https://vt100.net/docs/vt510-rm/DL.html
@@ -376,6 +403,62 @@ namespace eval Term {
 	}
     }
 
+    # Pan Down
+    #
+    # https://vt100.net/docs/vt510-rm/SU.html
+    proc _csi_S {args} {
+	set count [_default [lindex $args 0] 1]
+
+	_log_cur "Pan Down ($count)" {
+	    variable _cur_col
+	    variable _cur_row
+	    variable _cols
+	    variable _rows
+	    variable _chars
+
+	    set dy 0
+	    set y $count
+
+	    while {$y < $_rows} {
+		for {set x 0} {$x < $_cols} {incr x} {
+		    set _chars($x,$dy) $_chars($x,$y)
+		}
+		incr y 1
+		incr dy 1
+	    }
+
+	    _clear_lines $dy $_rows
+	}
+    }
+
+    # Pan Up
+    #
+    # https://vt100.net/docs/vt510-rm/SD.html
+    proc _csi_T {args} {
+	set count [_default [lindex $args 0] 1]
+
+	_log_cur "Pan Up ($count)" {
+	    variable _cur_col
+	    variable _cur_row
+	    variable _cols
+	    variable _rows
+	    variable _chars
+
+	    set y [expr $_rows - $count]
+	    set dy $_rows
+
+	    while {$dy >= $count} {
+		for {set x 0} {$x < $_cols} {incr x} {
+		    set _chars($x,$dy) $_chars($x,$y)
+		}
+		incr y -1
+		incr dy -1
+	    }
+
+	    _clear_lines 0 $count
+	}
+    }
+
     # Erase chars.
     #
     # https://vt100.net/docs/vt510-rm/ECH.html

Re: [PATCH] gdb/testing/tui: add new _csi_{L,P,S,T}

[Simon Marchi]

> commit 85625032b5b798ef082a634821ad8567923e9071
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed Jan 26 18:41:59 2022 +0000
> 
>     gdb/testing/tui: add new _csi_{L,S,T}
>     
>     This patch was original part of this series:
>     
>       https://sourceware.org/pipermail/gdb-patches/2022-March/186429.html
>       https://sourceware.org/pipermail/gdb-patches/2022-March/186433.html
>     
>     I've pulled this out as it might be useful ahead of the bigger series
>     being merged.
>     
>     This commit adds:
>     
>       _csi_L - insert line
>       _csi_S - pan down
>       _csi_T - pan up

I wonder if to implement pan up/down correctly, we would need to
implement having a larger framebuffer (the _chars array) and having a
window / viewport that is a subsection of that.  In other words, if you
pan up 1 line and then down 1 line, do you lose the contents of the
bottom line or not?

If this is enough for our needs, I'm fine with the more naive version,
but it would be good to note any limitation.

> 
> diff --git a/gdb/testsuite/gdb.tui/tuiterm.exp b/gdb/testsuite/gdb.tui/tuiterm.exp
> index c82db21a92b..1dcfb7f602d 100644
> --- a/gdb/testsuite/gdb.tui/tuiterm.exp
> +++ b/gdb/testsuite/gdb.tui/tuiterm.exp
> @@ -179,6 +179,60 @@ proc test_insert_characters { } {
>      } 0 1
>  }
>  
> +proc test_pan_down { } {
> +    Term::_move_cursor 1 2
> +    Term::_csi_S
> +    check "pan down, default arg" {
> +	"ijklmnop"
> +	"qrstuvwx"
> +	"yz01234 "
> +	"        "
> +    } 1 2

I couldn't find a good reference, but in your opinion, does the cursor
stay at the same place relative to the display, or does it follow the
content and moves up?

> @@ -540,6 +566,35 @@ proc test_erase_character { } {
>      } 1 3
>  }
>  
> +proc test_delete_character { } {
> +    Term::_move_cursor 3 2
> +    Term::_csi_P
> +    check "delete character, default param" {
> +	"abcdefgh"
> +	"ijklmnop"
> +	"qrsuvwx "
> +	"yz01234 "
> +    } 3 2
> +
> +    Term::_move_cursor 1 1
> +    Term::_csi_P 4
> +    check "erase character, explicit param" {
> +	"abcdefgh"
> +	"inop    "
> +	"qrsuvwx "
> +	"yz01234 "
> +    } 1 1
> +
> +    Term::_move_cursor 4 0
> +    Term::_csi_P 10
> +    check "attempt to erase more characters than are available" {
> +	"abcd    "
> +	"inop    "
> +	"qrsuvwx "
> +	"yz01234 "
> +    } 4 0
> +}

This patch still removes the existing _csi_P test and adds a new one, is
this intentional?

Simon

Re: [PATCH] gdb/testing/tui: add new _csi_{L,P,S,T}

[Andrew Burgess]

Simon Marchi <simon.marchi@polymtl.ca> writes:

>> commit 85625032b5b798ef082a634821ad8567923e9071
>> Author: Andrew Burgess <aburgess@redhat.com>
>> Date:   Wed Jan 26 18:41:59 2022 +0000
>> 
>>     gdb/testing/tui: add new _csi_{L,S,T}
>>     
>>     This patch was original part of this series:
>>     
>>       https://sourceware.org/pipermail/gdb-patches/2022-March/186429.html
>>       https://sourceware.org/pipermail/gdb-patches/2022-March/186433.html
>>     
>>     I've pulled this out as it might be useful ahead of the bigger series
>>     being merged.
>>     
>>     This commit adds:
>>     
>>       _csi_L - insert line
>>       _csi_S - pan down
>>       _csi_T - pan up
>
> I wonder if to implement pan up/down correctly, we would need to
> implement having a larger framebuffer (the _chars array) and having a
> window / viewport that is a subsection of that.  In other words, if you
> pan up 1 line and then down 1 line, do you lose the contents of the
> bottom line or not?

My reading of the spec is that what I've implemented here is correct,
except that it lacks support for the scroll margins.

However, the scroll margins require an additional csi in order to
configure them, so we'll know if/when they start being used.

Anyway, I've added an extra comment to pan up/down to indicate that the
code is written without considering the scroll margins.

> If this is enough for our needs, I'm fine with the more naive version,
> but it would be good to note any limitation.

Sorry for the request, I really have tried searching, but I can't find
any spec for pan up/down that mentions a feature like you're
describing.  If you can point me at the right parts of the spec then I'm
happy to write the words.  Or you can take what's here, add the words
and push it.  Or I can push this, and you can add the words in a
separate commit.  Really whatever works.

>
>> 
>> diff --git a/gdb/testsuite/gdb.tui/tuiterm.exp b/gdb/testsuite/gdb.tui/tuiterm.exp
>> index c82db21a92b..1dcfb7f602d 100644
>> --- a/gdb/testsuite/gdb.tui/tuiterm.exp
>> +++ b/gdb/testsuite/gdb.tui/tuiterm.exp
>> @@ -179,6 +179,60 @@ proc test_insert_characters { } {
>>      } 0 1
>>  }
>>  
>> +proc test_pan_down { } {
>> +    Term::_move_cursor 1 2
>> +    Term::_csi_S
>> +    check "pan down, default arg" {
>> +	"ijklmnop"
>> +	"qrstuvwx"
>> +	"yz01234 "
>> +	"        "
>> +    } 1 2
>
> I couldn't find a good reference, but in your opinion, does the cursor
> stay at the same place relative to the display, or does it follow the
> content and moves up?

Like you I couldn't find any clear spec.  But, when I was hitting this
sequence during testing leaving the cursor in place seemed to be what
was expected.

Additionally I checked the xterm source code.  Obviously not a spec,
but, it's been around for a while.... if you check out RevScroll (in
util.c), which is what gets used for handling pan down, then it is
crystal clear in the comment that the cursor remains in place.

So, I'm pretty confident with the choice I made here.

>
>> @@ -540,6 +566,35 @@ proc test_erase_character { } {
>>      } 1 3
>>  }
>>  
>> +proc test_delete_character { } {
>> +    Term::_move_cursor 3 2
>> +    Term::_csi_P
>> +    check "delete character, default param" {
>> +	"abcdefgh"
>> +	"ijklmnop"
>> +	"qrsuvwx "
>> +	"yz01234 "
>> +    } 3 2
>> +
>> +    Term::_move_cursor 1 1
>> +    Term::_csi_P 4
>> +    check "erase character, explicit param" {
>> +	"abcdefgh"
>> +	"inop    "
>> +	"qrsuvwx "
>> +	"yz01234 "
>> +    } 1 1
>> +
>> +    Term::_move_cursor 4 0
>> +    Term::_csi_P 10
>> +    check "attempt to erase more characters than are available" {
>> +	"abcd    "
>> +	"inop    "
>> +	"qrsuvwx "
>> +	"yz01234 "
>> +    } 4 0
>> +}
>
> This patch still removes the existing _csi_P test and adds a new one, is
> this intentional?

FFS.  I knew I should have left this for the morning.  Sorry.  This is
fixed in the update below.

Thanks,
Andrew

---

commit 9a2606c4c602682a5f5521bd72693d7ffe29978a
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Jan 26 18:41:59 2022 +0000

    gdb/testing/tui: add new _csi_{L,S,T}
    
    This patch was original part of this series:
    
      https://sourceware.org/pipermail/gdb-patches/2022-March/186429.html
      https://sourceware.org/pipermail/gdb-patches/2022-March/186433.html
    
    I've pulled this out as it might be useful ahead of the bigger series
    being merged.
    
    This commit adds:
    
      _csi_L - insert line
      _csi_S - pan down
      _csi_T - pan up

diff --git a/gdb/testsuite/gdb.tui/tuiterm.exp b/gdb/testsuite/gdb.tui/tuiterm.exp
index c82db21a92b..98f1cd7fe8c 100644
--- a/gdb/testsuite/gdb.tui/tuiterm.exp
+++ b/gdb/testsuite/gdb.tui/tuiterm.exp
@@ -179,6 +179,60 @@ proc test_insert_characters { } {
     } 0 1
 }
 
+proc test_pan_down { } {
+    Term::_move_cursor 1 2
+    Term::_csi_S
+    check "pan down, default arg" {
+	"ijklmnop"
+	"qrstuvwx"
+	"yz01234 "
+	"        "
+    } 1 2
+
+    Term::_csi_S 2
+    check "pan down, explicit arg" {
+	"yz01234 "
+	"        "
+	"        "
+	"        "
+    } 1 2
+
+    Term::_csi_S 100
+    check "pan down, excessive arg" {
+	"        "
+	"        "
+	"        "
+	"        "
+    } 1 2
+}
+
+proc test_pan_up { } {
+    Term::_move_cursor 1 2
+    Term::_csi_T
+    check "pan down, default arg" {
+	"        "
+	"abcdefgh"
+	"ijklmnop"
+	"qrstuvwx"
+    } 1 2
+
+    Term::_csi_T 2
+    check "pan down, explicit arg" {
+	"        "
+	"        "
+	"        "
+	"abcdefgh"
+    } 1 2
+
+    Term::_csi_T 100
+    check "pan down, excessive arg" {
+	"        "
+	"        "
+	"        "
+	"        "
+    } 1 2
+}
+
 proc test_cursor_up { } {
     Term::_move_cursor 2 3
 
@@ -594,6 +648,34 @@ proc test_vertical_line_position_absolute { } {
     } 2 3
 }
 
+proc test_insert_line { } {
+    Term::_move_cursor 2 1
+    Term::_csi_L
+    check "insert line, default param" {
+	"abcdefgh"
+	"        "
+	"ijklmnop"
+	"qrstuvwx"
+    } 2 1
+
+    Term::_move_cursor 2 0
+    Term::_csi_L 2
+    check "insert line, explicit param" {
+	"        "
+	"        "
+	"abcdefgh"
+	"        "
+    } 2 0
+
+    Term::_csi_L 12
+    check "insert line, insert more lines than display has" {
+	"        "
+	"        "
+	"        "
+	"        "
+    } 2 0
+}
+
 # Run proc TEST_PROC_NAME with a "small" terminal.
 
 proc run_one_test_small { test_proc_name } {
@@ -632,6 +714,9 @@ foreach_with_prefix test {
     test_erase_character
     test_repeat
     test_vertical_line_position_absolute
+    test_insert_line
+    test_pan_up
+    test_pan_down
 } {
     run_one_test_small $test
 }
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 9053f7dba6a..e660840eed9 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -324,6 +324,33 @@ namespace eval Term {
 	}
     }
 
+    # Insert Line
+    #
+    # https://vt100.net/docs/vt510-rm/IL.html
+    proc _csi_L {args} {
+	set arg [_default [lindex $args 0] 1]
+
+	_log_cur "Insert Line ($arg)" {
+	    variable _cur_col
+	    variable _cur_row
+	    variable _rows
+	    variable _cols
+	    variable _chars
+
+	    set y [expr $_rows - 2]
+	    set next_y [expr $y + $arg]
+	    while {$y >= $_cur_row} {
+		for {set x 0} {$x < $_cols} {incr x} {
+		    set _chars($x,$next_y) $_chars($x,$y)
+		}
+		incr y -1
+		incr next_y -1
+	    }
+
+	    _clear_lines $_cur_row [expr $_cur_row + $arg]
+	}
+    }
+
     # Delete line.
     #
     # https://vt100.net/docs/vt510-rm/DL.html
@@ -376,6 +403,74 @@ namespace eval Term {
 	}
     }
 
+    # Pan Down
+    #
+    # https://vt100.net/docs/vt510-rm/SU.html
+    proc _csi_S {args} {
+	set count [_default [lindex $args 0] 1]
+
+	_log_cur "Pan Down ($count)" {
+	    variable _cur_col
+	    variable _cur_row
+	    variable _cols
+	    variable _rows
+	    variable _chars
+
+	    # The following code is written without consideration for
+	    # the scroll margins.  At this time this comment was
+	    # written the tuiterm library doesn't support the scroll
+	    # margins.  If/when that changes, then the following will
+	    # need to be updated.
+
+	    set dy 0
+	    set y $count
+
+	    while {$y < $_rows} {
+		for {set x 0} {$x < $_cols} {incr x} {
+		    set _chars($x,$dy) $_chars($x,$y)
+		}
+		incr y 1
+		incr dy 1
+	    }
+
+	    _clear_lines $dy $_rows
+	}
+    }
+
+    # Pan Up
+    #
+    # https://vt100.net/docs/vt510-rm/SD.html
+    proc _csi_T {args} {
+	set count [_default [lindex $args 0] 1]
+
+	_log_cur "Pan Up ($count)" {
+	    variable _cur_col
+	    variable _cur_row
+	    variable _cols
+	    variable _rows
+	    variable _chars
+
+	    # The following code is written without consideration for
+	    # the scroll margins.  At this time this comment was
+	    # written the tuiterm library doesn't support the scroll
+	    # margins.  If/when that changes, then the following will
+	    # need to be updated.
+
+	    set y [expr $_rows - $count]
+	    set dy $_rows
+
+	    while {$dy >= $count} {
+		for {set x 0} {$x < $_cols} {incr x} {
+		    set _chars($x,$dy) $_chars($x,$y)
+		}
+		incr y -1
+		incr dy -1
+	    }
+
+	    _clear_lines 0 $count
+	}
+    }
+
     # Erase chars.
     #
     # https://vt100.net/docs/vt510-rm/ECH.html

Re: [PATCH] gdb/testing/tui: add new _csi_{L,P,S,T}

[Simon Marchi]

On 2022-04-01 05:34, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>>> commit 85625032b5b798ef082a634821ad8567923e9071
>>> Author: Andrew Burgess <aburgess@redhat.com>
>>> Date:   Wed Jan 26 18:41:59 2022 +0000
>>>
>>>     gdb/testing/tui: add new _csi_{L,S,T}
>>>     
>>>     This patch was original part of this series:
>>>     
>>>       https://sourceware.org/pipermail/gdb-patches/2022-March/186429.html
>>>       https://sourceware.org/pipermail/gdb-patches/2022-March/186433.html
>>>     
>>>     I've pulled this out as it might be useful ahead of the bigger series
>>>     being merged.
>>>     
>>>     This commit adds:
>>>     
>>>       _csi_L - insert line
>>>       _csi_S - pan down
>>>       _csi_T - pan up
>>
>> I wonder if to implement pan up/down correctly, we would need to
>> implement having a larger framebuffer (the _chars array) and having a
>> window / viewport that is a subsection of that.  In other words, if you
>> pan up 1 line and then down 1 line, do you lose the contents of the
>> bottom line or not?
> 
> My reading of the spec is that what I've implemented here is correct,
> except that it lacks support for the scroll margins.
> 
> However, the scroll margins require an additional csi in order to
> configure them, so we'll know if/when they start being used.
> 
> Anyway, I've added an extra comment to pan up/down to indicate that the
> code is written without considering the scroll margins.
> 
>> If this is enough for our needs, I'm fine with the more naive version,
>> but it would be good to note any limitation.
> 
> Sorry for the request, I really have tried searching, but I can't find
> any spec for pan up/down that mentions a feature like you're
> describing.  If you can point me at the right parts of the spec then I'm
> happy to write the words.  Or you can take what's here, add the words
> and push it.  Or I can push this, and you can add the words in a
> separate commit.  Really whatever works.

If you think your implementation is the expected behavior, I am fine
with it, because I don't know more than you.  I don't know how to
interpret the description:

  This control function moves the user window up a specified number of lines in page memory.

>>>
>>> diff --git a/gdb/testsuite/gdb.tui/tuiterm.exp b/gdb/testsuite/gdb.tui/tuiterm.exp
>>> index c82db21a92b..1dcfb7f602d 100644
>>> --- a/gdb/testsuite/gdb.tui/tuiterm.exp
>>> +++ b/gdb/testsuite/gdb.tui/tuiterm.exp
>>> @@ -179,6 +179,60 @@ proc test_insert_characters { } {
>>>      } 0 1
>>>  }
>>>  
>>> +proc test_pan_down { } {
>>> +    Term::_move_cursor 1 2
>>> +    Term::_csi_S
>>> +    check "pan down, default arg" {
>>> +	"ijklmnop"
>>> +	"qrstuvwx"
>>> +	"yz01234 "
>>> +	"        "
>>> +    } 1 2
>>
>> I couldn't find a good reference, but in your opinion, does the cursor
>> stay at the same place relative to the display, or does it follow the
>> content and moves up?
> 
> Like you I couldn't find any clear spec.  But, when I was hitting this
> sequence during testing leaving the cursor in place seemed to be what
> was expected.
> 
> Additionally I checked the xterm source code.  Obviously not a spec,
> but, it's been around for a while.... if you check out RevScroll (in
> util.c), which is what gets used for handling pan down, then it is
> crystal clear in the comment that the cursor remains in place.

Cool, I think that xterm is as close as there is to a spec.

> So, I'm pretty confident with the choice I made here.

LGTM then!

Simon

Re: [PATCH] gdb/testing/tui: add new _csi_{L,P,S,T}

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 2022-04-01 05:34, Andrew Burgess wrote:
>> Simon Marchi <simon.marchi@polymtl.ca> writes:
>> 
>>>> commit 85625032b5b798ef082a634821ad8567923e9071
>>>> Author: Andrew Burgess <aburgess@redhat.com>
>>>> Date:   Wed Jan 26 18:41:59 2022 +0000
>>>>
>>>>     gdb/testing/tui: add new _csi_{L,S,T}
>>>>     
>>>>     This patch was original part of this series:
>>>>     
>>>>       https://sourceware.org/pipermail/gdb-patches/2022-March/186429.html
>>>>       https://sourceware.org/pipermail/gdb-patches/2022-March/186433.html
>>>>     
>>>>     I've pulled this out as it might be useful ahead of the bigger series
>>>>     being merged.
>>>>     
>>>>     This commit adds:
>>>>     
>>>>       _csi_L - insert line
>>>>       _csi_S - pan down
>>>>       _csi_T - pan up
>>>
>>> I wonder if to implement pan up/down correctly, we would need to
>>> implement having a larger framebuffer (the _chars array) and having a
>>> window / viewport that is a subsection of that.  In other words, if you
>>> pan up 1 line and then down 1 line, do you lose the contents of the
>>> bottom line or not?
>> 
>> My reading of the spec is that what I've implemented here is correct,
>> except that it lacks support for the scroll margins.
>> 
>> However, the scroll margins require an additional csi in order to
>> configure them, so we'll know if/when they start being used.
>> 
>> Anyway, I've added an extra comment to pan up/down to indicate that the
>> code is written without considering the scroll margins.
>> 
>>> If this is enough for our needs, I'm fine with the more naive version,
>>> but it would be good to note any limitation.
>> 
>> Sorry for the request, I really have tried searching, but I can't find
>> any spec for pan up/down that mentions a feature like you're
>> describing.  If you can point me at the right parts of the spec then I'm
>> happy to write the words.  Or you can take what's here, add the words
>> and push it.  Or I can push this, and you can add the words in a
>> separate commit.  Really whatever works.
>
> If you think your implementation is the expected behavior, I am fine
> with it, because I don't know more than you.  I don't know how to
> interpret the description:
>
>   This control function moves the user window up a specified number of lines in page memory.

Yeah, I don't know.  I've tried to read through the docs but I don't
really understand what the requirements are here.

I've gone ahead and pushed the patch as it currently stands.  I do
believe that this is a good starting place, but anyone who understands
this better is more than welcome to make improvements.

Thanks for all your feedback.

Andrew


>
>>>>
>>>> diff --git a/gdb/testsuite/gdb.tui/tuiterm.exp b/gdb/testsuite/gdb.tui/tuiterm.exp
>>>> index c82db21a92b..1dcfb7f602d 100644
>>>> --- a/gdb/testsuite/gdb.tui/tuiterm.exp
>>>> +++ b/gdb/testsuite/gdb.tui/tuiterm.exp
>>>> @@ -179,6 +179,60 @@ proc test_insert_characters { } {
>>>>      } 0 1
>>>>  }
>>>>  
>>>> +proc test_pan_down { } {
>>>> +    Term::_move_cursor 1 2
>>>> +    Term::_csi_S
>>>> +    check "pan down, default arg" {
>>>> +	"ijklmnop"
>>>> +	"qrstuvwx"
>>>> +	"yz01234 "
>>>> +	"        "
>>>> +    } 1 2
>>>
>>> I couldn't find a good reference, but in your opinion, does the cursor
>>> stay at the same place relative to the display, or does it follow the
>>> content and moves up?
>> 
>> Like you I couldn't find any clear spec.  But, when I was hitting this
>> sequence during testing leaving the cursor in place seemed to be what
>> was expected.
>> 
>> Additionally I checked the xterm source code.  Obviously not a spec,
>> but, it's been around for a while.... if you check out RevScroll (in
>> util.c), which is what gets used for handling pan down, then it is
>> crystal clear in the comment that the cursor remains in place.
>
> Cool, I think that xterm is as close as there is to a spec.
>
>> So, I'm pretty confident with the choice I made here.
>
> LGTM then!
>
> Simon