-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix trimming of right side #1775
Conversation
Up for next review. Note that this also needs user testing before it can be added, since it is used at many places (not sure about the test coverage). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conflicts with master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good, let's ship it after the comments are resolved if you're confident we can throw away the Buffer. translateBufferLineToString
code (seems to work from my tests) 🚀
The current trimming of lines cannot distinguish between inserted spaces and empty cells since it trims spaces. This is related and discussed in #1685, also #1769 is affected by this.
This PR is a testbed to evaluate the empty cell representation with a different char than space to overcome the drawbacks. It currently inserts an empty string instead, for TypedArray this automatically falls back to '\x00' (treated as '').
Although doing an additional char replace this is even faster with TypedArray than the current impl (tested with xterm-benchmark with https://gist.github.com/jerch/acca11c68bfe5c8172c909a1cb9fa6db):
Why bothering at all, doesnt it work as it is? Well we have several issues open that boil down to this problem. All consumers of
Buffer.translateBufferLineToString
with trimming enabled are affected by this. It gets even worse when a consumer tries to unwrap lines, esp. if a resize was done in between (resizing itself is a different problem though that will be addressed with reflow).Edit: The canvas renderer seems to be faster too, the time forWas due to wrongly skipping empty cells..../lib/renderer/TextRenderLayer.js.TextRenderLayer._forEachCell
dropped from ~ 350 ms to ~ 220 ms (tested in chrome, to lazy to write a xterm-benchmark case). Any idea why? (I am not familiar with the code)Would be good to get some early feedback before going down the rabbit hole (yeah it is likely to affect several parts of the code base).