Merged
Conversation
Tyriar
requested changes
Oct 31, 2018
Member
Author
|
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). |
Tyriar
approved these changes
Dec 13, 2018
Member
Tyriar
left a comment
There was a problem hiding this comment.
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) 🚀
This was referenced Dec 14, 2018
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.translateBufferLineToStringwith 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._forEachCelldropped 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).