Column resize caching #386
Conversation
| @@ -2883,14 +2889,18 @@ Terminal.prototype.resize = function(x, y) { | |||
| i = this.lines.length; | |||
| while (i--) { | |||
| while (this.lines[i].length < x) { | |||
| this.lines[i].push(ch); | |||
| this.lineCache[i] = this.lineCache[i] || [] | |||
There was a problem hiding this comment.
Thanks for putting out the PR! Unfortunately I don't think this will work as a temporary fix because this.lines frequently gets rows in the middle of the array removed/modified and also when the scrollback limit is reached, big chunks of the array are removed from the start. Covering all the cases for keeping lineCache in sync would probably be more difficult than a proper fix 😛 it could also potentially introduce performance issues.
I think a good temporary fix for this is to not remove the characters from this.lines at all, but instead only materialize up to this.cols characters from within this.refresh.
|
Okay @Tyriar - I see what you mean. I actually had such a feeling, but was lacking in any other direction to fix this. :) |
|
I'm kind of surprised that's all this needed. @parisk do you know if there would be any unintended side effects to removing this column trimming code from |
|
Thanks for your contribution @imsnif! I just ran the demo on your branch and after reducing the number of columns text would just vanish, instead of wrapping. If we get this working and apply some tests, we can definitely merge the request 👍 . |
|
@parisk - we thought this could be a temporary fix for the problem, seeing as while it does not wrap the text, it does not "swallow" it either (as is the case right now). So while reducing the number of columns makes the text vanish, it comes back when you increase them again. It's not an ideal situation, but in my opinion it's better than the current behaviour. What do you think? Other than that, I'm currently working on implementing a proper "wrapping" solution, but I'm not sure I'll have the time for it. If I will, I plan on doing a separate PR. |
|
I think it's 👍 to postpone this until 2.3.0, to make sure we ship a good experience to the end user. Whether which of these two experiences is best for the end user IMO is subjective, so let's just stick on fixing this the right way. @imsnif as this is an important issue not only for you, but for other xterm.js users as well (our company including!) what can we do to help you as much as possible with moving forward with the full solution? |
|
@parisk - I basically have a working version with line wrapping that I need to clean up locally. I'm currently writing some tests and then will do some refactoring before issuing another PR. |
|
@imsnif why does resizing need to know if they're wrapped or not? What is the performance impact of not keeping a wrapped flag in on the characters? |
|
@Tyriar - I'm using the wrapped flag on the characters in order to know whether they should be 'unwrapped' when the terminal is expanded. |
|
Closing this in favour of: #404 |
This is a quick fix for: #325
Rather than wrapping text, it just caches it temporarily and re-inserts it.
While not ideal, I think this is better than the current behaviour (just losing the text).
This is critical for one of our current projects, so if this is an acceptable (temporary?) fix, I'd be happy to write tests for it and make further changes to other integrated features (I'm new to this code base, so my apologies if I missed anything - would be happy to have anything pointed out to me).
Thanks!