changes made for Secure ShellFish

[palmin]

The work in these commits is covered by the MIT license and I hope it can be of some use even if a lot is about extracting lines in a scroll-invariant way.

[migueldeicaza]

Thank you for your contribution Anders!

Question: does the comment:

about extracting lines in a scroll-invariant way.

Is that some sort of "screen-scraping" capability? If so, that is one of the open tasks that I had, so it would be doubly useful!

[palmin]

I present the terminal lines in a UITextView containing all lines in scrollback. I need to detect changes inside SwiftTerm and pull out relevant lines.

This also allows slightly more fine-grained updates where I only need to refresh lines that have changes and not the whole interval between minimum and maximum changed line.

[migueldeicaza]

Thanks for the information, three comments:

• I got the code to build on the Mac as well, will likely push in a minute to a branch

• The patch currently regresses two tests in the test suite, so I am going to dig into that (DECSETTests.test_DECSET_ALTBUF and RISTests.test_RIS_ExitAltScreen)

• Is there a reason that you need the delegate invocation to notify you of changes in the buffer? The current idiom is that at the end of the processing, the caller code must retrieve the range of modified data (you added a method to this effect) and iterates over the values, rather than being called back on the changes.

For instance, I think that the current code might call repeatedly the front-end in some scenarios, rather than having the frontend just process the range of changed lines in one go.

[migueldeicaza]

I found the culprit for the regression.

This change breaks the xterm/iterm2 test suite, so I suspect that this was a fix that is hiding the real source of the problem, and I would love to know what triggered this, so I can find the real problem and fix that instead.

This is the change with the regression:

commit 718a144ef30cc69b85df6bdc65e39de353212a41
Author: Anders Borum <anders@algoritmer.dk>
Date:   Mon Jul 20 23:31:16 2020 +0200

    reset alt buffer cursor when switching to it

 Sources/SwiftTerm/BufferSet.swift | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
macpro$ git show -p
commit 718a144ef30cc69b85df6bdc65e39de353212a41 (HEAD, refs/bisect/bad)
Author: Anders Borum <anders@algoritmer.dk>
Date:   Mon Jul 20 23:31:16 2020 +0200

    reset alt buffer cursor when switching to it

diff --git a/Sources/SwiftTerm/BufferSet.swift b/Sources/SwiftTerm/BufferSet.swift
index b75cbe5..06cce11 100644
--- a/Sources/SwiftTerm/BufferSet.swift
+++ b/Sources/SwiftTerm/BufferSet.swift
@@ -62,8 +62,10 @@ class BufferSet {
             return
         }
         
-        alt.x = normal.x
-        alt.y = normal.y
+        // less switches to alt buffer without moving cursor and assumes
+        // switching to alt buffer resets cursor
+        alt.x = 0 // normal.x
+        alt.y = 0 // normal.y
         // Since the alt buffer is always cleared when the normal buffer is
         // activated, we want to fill it when switching to it.
         

But this other change also has an odd smell, because it sets the right margin to the wrong value (it should at best be alt.cols-1 instead of alt.rows-1)

commit 5448b319b5c03a1296b79c604e304e3b5dec0b1c
Author: Anders Borum <anders@algoritmer.dk>
Date:   Wed Jul 29 15:59:52 2020 +0200

    reset margin when starting alt buffer (fix emacs insert mode) and improve logging

diff --git a/Sources/SwiftTerm/BufferSet.swift b/Sources/SwiftTerm/BufferSet.swift
index 6383bd2..805ebe1 100644
--- a/Sources/SwiftTerm/BufferSet.swift
+++ b/Sources/SwiftTerm/BufferSet.swift
@@ -71,6 +71,10 @@ class BufferSet {
         // switching to alt buffer resets buffer
         alt.clear()
         
+        // reset margins
+        alt.marginLeft = 0
+        alt.marginRight = alt.rows - 1
+        
         alt.fillViewportRows(attribute: fillAttr)
         active = alt
     }

(My version is in branch pull-138-modified)

[migueldeicaza]

I am wondering - do you remember what this change was for?

[palmin]

I'm not sure. Sorry.

I remember some problems where I would get the wrong lines once the scrollback buffer couldn't fit the entire terminal history. But these problems could just be about the way I extracted lines.

[migueldeicaza]

One last comment - do you have any Sixel tests that you are using, I would like to add some tests for it.

[palmin]

Is there a reason that you need the delegate invocation to notify you of changes in the buffer? The current idiom is that at the end of the processing, the caller code must retrieve the range of modified data (you added a method to this effect) and iterates over the values, rather than being called back on the changes.

I'm still sort of following this idiom but instead of representing changes as a line number interval I have a set of all changes line numbers. When the processing is done I run through all lines marked as changed and update these. I needed the delegate to update this set of line numbers.

This will reduce the amount of work in situations where you have changes at the top of the buffer and the bottom but not in between such as when editing at the top of the screen and the editor updates column and line numbers at the bottom. This would require a near full buffer update per keystroke when looking at ranges and a two-line update with this more fine-grained tracking of changes.

[palmin]

But this other change also has an odd smell, because it sets the right margin to the wrong value (it should at best be alt.cols-1 instead of alt.rows-1)

That looks all wrong and I'm not sure what I was thinking or how it could even sometimes work.

I made these changes because I was seeing bad behaviour if the terminal started out narrow, was resized to being wider and then switched to alt mode. When typing in emacs the characters would stop appearing before the right end of the screen as if it was limited to the old width.

[palmin]

One last comment - do you have any Sixel tests that you are using, I would like to add some tests for it.

I do not have automated tests for this but two images proved particularly helpful when doing visual comparisons.

• This minimal example fra Wikipedia was small enough to step through.
• trontank.six has dithering on the tank and clear two color geometry in the background.

[migueldeicaza]

Thank you for your responses, a couple of follow ups:

Would it make sense to keep a list of dirty lines in the buffer, that is consulted after the render pass, rather than pushing notifications for changes per-line? That is, rather than having a range "begin/end" of dirty lines, some sort of iterator var changedLines : [Int] that is looked up before rendering? I am just not a fan of the event triggering for these changes during parsing, in particular, because I think that in the future it might be desirable to parse the contents in the background.

Thanks for the scenario on resizing - I will investigate that: #140

[palmin]

Thank you for your responses, a couple of follow ups:

Would it make sense to keep a list of dirty lines in the buffer, that is consulted after the render pass, rather than pushing notifications for changes per-line? That is, rather than having a range "begin/end" of dirty lines, some sort of iterator var changedLines : [Int] that is looked up before rendering? I am just not a fan of the event triggering for these changes during parsing, in particular, because I think that in the future it might be desirable to parse the contents in the background.

Thanks for the scenario on resizing - I will investigate that: #140

I think that would make a lot of sense.

It could perhaps be a Set<Int> to ensure each line number only appears once even if marked as dirty several times in a render pass.

[migueldeicaza]

Agreed on using Set<Int> - I am manually merging your code on pull request #139, and once that is done, I will bring the suggestion. I am tracking that as issue #142

[migueldeicaza]

Thank you for the changes! I have merged a modified version of this change, and I am tracking a couple of tasks that I need to implement for you (the Set change discussed above)

[lockheadone]

@MaxDesiatov