Add logic for reflowing lines and preserving data when resizing #609

LucianBuzzo · 2017-03-16T09:31:37Z

Fixes #622

coveralls · 2017-03-16T09:35:10Z

Coverage decreased (-0.4%) to 67.535% when pulling e72d106 on LucianBuzzo:lb-line-wrap into 6d25706 on sourcelair:master.

mofux · 2017-03-16T10:40:52Z

Very nice approach, love it. I found some issues though:

Create some output, then shrink the terminal by half of its previous cols, and scroll up. Even if the wrapped output is below 1000 lines of the scrollback buffer, I am unable so see the start of the scrollback.
Run ls -alh with 80 cols. Shrink the terminal to 20 cols. run ls -alh again. Resize back to 80 cols. The second ls -alh output is still wrapped. This does not happen in Terminal.app, not sure how they do it, because technically the wrapped lines are arriving as-is from the pty process. I'm wondering if there's any control sequence that marks the end of the wrapped line?

blink1073 · 2017-03-16T11:02:48Z

Thank you so much for this, @LucianBuzzo!

LucianBuzzo · 2017-03-16T12:15:45Z

@mofux A simpler approach to this issue might be to just implement the same behaviour as hyperterm and ignore reflowing altogether.
TLDR hyperterm retains the line information but crops the line visually when resizing smaller.

jerch · 2017-03-16T13:47:40Z

Run ls -alh with 80 cols. Shrink the terminal to 20 cols. run ls -alh again. Resize back to 80 cols. The second ls -alh output is still wrapped. This does not happen in Terminal.app, not sure how they do it, because technically the wrapped lines are arriving as-is from the pty process. I'm wondering if there's any control sequence that marks the end of the wrapped line?

This is possible because the pty normally just inserts '\r' at EOL. The actual newline step is inserted by the emulator itself with the autowrap setting (without '\n' char). By ignoring the '\r' at EOL the lines still belong together. From there you can flow lines in both direction - shrinking and enlarging.

Tyriar · 2017-03-16T16:27:54Z

@mofux on your first point, one of the more tricky issues that arises when implementing this is that when lines wrap and reflow and are no longer fixed, the number of rows expand and contract and the viewport/scrollbar needs to be made aware of it. Scrolling the mouse wheel one notch should jump a consistent number of actual lines as are currently wrapped, not the amount they originated with.

Consider the extreme case which is quite reasonable in a terminal environment of a huge line that consumes the entire viewport, if the terminal is then resized to 50% of the width and a reflow occurs (so the content consumes 2x viewports), you must be able to scroll a consistent number of rows.

Based on your first comment it would seem that only the first 1000 rows are shown and the scroll bar isn't touched at all. We should probably just trim rows that go beyond the scrollback when the terminal width is reduced.

On your second issue I would guess it's related to the pty as @jerch indicates, if that can be worked around that would be cool 😄

LucianBuzzo · 2017-03-16T16:36:36Z

@Tyriar @mofux I think point 2 should be straightforward to fix, I'll take a look 👍

jerch · 2017-03-16T18:25:28Z

@LucianBuzzo You can distinct the EOLs in the write method - any real EOL will be triggered by a '\n' char in the data, therefore any line jumped to by '\n' is a real new line. All other lines are autowrapped and can reflow to a single line by enlarging. Note - that will not work with programs that use the terminal as canvas with heavy cursor jump usage (like vim or any nurses stuff), but since most of those implement the WINCH signal, they will "autorepair" itself afterwards. If they dont, hmm the output will be broken anyways. It might be possible to circumvent reflow for "jumped" lines at all and go with the cropping thingy you mentioned above.

mofux · 2017-03-17T10:22:57Z

I'm probably thinking to naive here, but given what @jerch is saying, if we were only creating a new line if we see '\n', and then let the browser do the wrapping for us, wouldn't that work?

Like

.xterm-rows > div {
  word-break: break-all;
}

would then automatically force the characters into a new virtual line if they don't fit into one. Not sure what this does to the xterm viewport though.

jerch · 2017-03-17T13:25:50Z

@mofux This would work, but introduce tons of problems with the [col, row] grid idea of a terminal and make the cursor jump escape sequences very hard to calculate.

Imho an easier approach would be to mark all terminal lines as "real EOL" on a line level (pseudocode):

for line in terminal:
    line.eol = true;

and set this to false once a wrap happens in write (pseudocode):

write(s):
    ...
    if (wrapped):
        line.eol = false;
    ...

During resizing the reflow logic would just have to search for a line with eol == true and fit all following lines into the new width up to the next real EOL line or sumthing like that.

The tricky part is the handling of curses like apps without WINCH. A heuristic approach would be to set eol to true again as soon as a cursor jump happens into a line by an escape sequence. Another heuristic hint could be the ICANON flag of the pty, if it is available for the emulator.

Tyriar · 2017-03-17T16:28:43Z

if we were only creating a new line if we see '\n', and then let the browser do the wrapping for us, wouldn't that work?

@mofux doing it that way would be ideal in many respects such getting links spanning multiple rows trivially. However, I imagine there's a bunch of problems with doing it as @jerch mentions so it's probably better to make the links more clever instead. Another potential problem area for this is when I get "virtual selection" working #207, if things are even a pixel off it would look wrong.

Tyriar

Great work @LucianBuzzo, let me know if you have any questions.

Tyriar · 2017-03-17T16:33:18Z

src/Renderer.ts

+
+      // If there are overflow lines use the last one
+      if (overflowBuffer.length) {
+        line = overflowBuffer.pop();


I believe this would suffer from the issue I mentioned in #609 (comment) where is a really long line in the terminal that is reflowed within a smaller width gets larger than the viewport, you won't be able to see the rows. And resulting from this scrolling will feel awkward because it will jump the amount of rows that the line is now wrapped, instead of the correct 1.

Tyriar · 2017-03-17T16:38:42Z

src/Renderer.ts

@@ -159,11 +185,15 @@ export class Renderer {
      for (; i < width; i++) {
        if (!line[i]) {
          // Continue if the character is not available, this means a resize is currently in progress
-          continue;
+          data = this._terminal.defAttr;


data, ch and ch_width can be shared among each character, should we expose a Terminal.defaultCharacter variable or something (perhaps function if defAttr changes). Terminal.blankLine reuses these empty characters for a single line.

Tyriar · 2017-03-17T16:42:41Z

src/utils/LineWrap.ts

+ *
+ * @return {array} - The trimmed terminal line
+ */
+export const trimBlank = (line, min, blank) => {


Not necessarily in this PR, but it might make sense to pull all this into a new class BufferModel which extends CircularList, I've felt a class like that has been needed to encapsulate operations on the buffer for a while. This way we can potentially solve the scrolling problem I've mentioned by making the buffer itself aware of the reflows.

LucianBuzzo · 2017-03-21T04:29:25Z

I've updated this PR to include a more complete line reflow solution that should fix the issues raised in comments here.
I still need to fix sticky scrolling and add a caching/memoization layer in front of the buffer, but for the most part it seems to work quite well.

Tyriar · 2017-03-21T21:12:05Z

src/Viewport.ts

@@ -66,9 +67,10 @@ export class Viewport {
   * Updates dimensions and synchronizes the scroll area if necessary.
   */
  public syncScrollArea(): void {
-    if (this.lastRecordedBufferLength !== this.terminal.lines.length) {
+    let totalLines = this.terminal.lines.totalLinesAtWidth(this.terminal.cols);


syncScrollArea is too performance critical, we cannot call totalLinesAtWidth here. I think Buffer needs some reflow function to be called when cols change, this will do all the heavy lifting of mapping the underlying CircularList lines with the reflowed lines only when a resize occurs.

Tyriar · 2017-03-27T17:01:08Z

@LucianBuzzo is this ready for a full review?

LucianBuzzo · 2017-03-28T07:12:58Z

@Tyriar It's very close! I made some changes that dramatically improved performance and it seems to be working very well.
Could you please give it a shakedown and see if there's anything obviously broken?

mofux · 2017-03-28T13:41:41Z

I found a bug with Mac's default top application, tested using the latest version of this pull request. Running top, then resizing the terminal to 40 cols starts to wrap the lines of tops output. In contrast, the default mac Terminal.app will truncate the lines. Anyway, when quitting top the terminal is dysfunctional, leaving you with characters all over the place, and I am unable to recover with clear.

LucianBuzzo · 2017-03-29T16:12:05Z

@mofux The latest commit should fix the issue you're seeing.

mofux · 2017-03-29T17:36:54Z

@LucianBuzzo top works perfect now, thanks! Testing some more, I have found another issue with vim. If I write some multiline text into vim and then resize from 81 cols to 40 cols for example, the text is wrapped incorrectly, and strange things start to happen when typing some more...

LucianBuzzo · 2017-03-30T10:17:35Z

@mofux When resizing the terminal with vim running, vim appears to actually send the last character twice. Here is the log output straight from the socket �[m�[H�[2J�[1;30Hqwertt�[2;1Hy.
As you can see the t character in qwerty appears twice at the end of the line.
On the master branch the additional character is never rendered so it's not an issue.
I'm not sure why this happens and plan to do some more investigation.

LucianBuzzo · 2017-03-30T10:25:55Z

Hmm so this problem doesn't happen with neovim.

mofux · 2017-03-30T14:04:54Z

You are right, I can confirm this is only happening in vim but not nvim. The big question is, why does the extra character that is sent not rendered?

LucianBuzzo · 2017-03-30T14:06:44Z

@mofux I think there's a typo in your "big question"

mofux · 2017-03-30T14:59:45Z

I am not sure if it is related, but when I change the pty name in demo/app.js from xterm-color to vt100, I get an exception as soon as I start typing in vim:

I actually wanted to test if a different term environment yields the same "duplicate character on line break" error that we are seeing in vim.

LucianBuzzo · 2017-03-31T09:00:57Z

@mofux, this issue should now be fixed for vt100. I looked and we're getting the same vim output :(

Tyriar · 2017-04-01T05:35:32Z

Found an issue:

Open demo
Run ls
Resize to 60 cols, the output should be wrapped such that there is no scroll bar. It should fill the space below the prompt when resized if there is "empty" space available

Tyriar · 2017-04-01T05:42:22Z

Another issue:

Type in a lot of content into prompt, the output should wrap like normal and no wrap onto the same line (as in master)
Backspace to remove the content

branch behavior

master behavior

Tyriar · 2017-04-01T05:44:55Z

Another issue (edit: same as #609 (comment)):

Open vim
Press i
Paste in this: Lorem ipsum dolor sit amet, consectetur adipiscing elit. Suspendisse euismod urna quis nisi dapibus sodales tristique a ante. Nunc quis lectus dui.

Notice the line is wrapped incorrectly and the scroll bar is active:

Master behavior:

LucianBuzzo · 2017-04-02T06:21:50Z

@Tyriar vim seems to be sending duplicate characters. Any ideas how to handle this or why it is happening?

jerch · 2017-04-02T10:55:38Z

@LucianBuzzo I think this is related to DECAWM, vim seems to write over EOL. No clue why it does...

Edit: vim does this also for TERM=vt100 (other escape seq are quite different though)

LucianBuzzo · 2017-04-07T10:04:46Z

I've been trying to find out what's causing this vim issue. I'm not sure that it's related to DECAWM and I've been unable to find out any info on the duplicate character behaviour.

LucianBuzzo · 2017-04-11T08:30:18Z

Here's the relevant code that causes this https://github.com/vim/vim/blob/master/src/screen.c#L5707
I'm a bit puzzled as to how to get around this behaviour, or how other emulators deal with it.

Update: Some digging with this new information turned up this illuminating SO thread. I should be able to start making some progress again now!

! Wether this works also with _wrapped_ selections, depends on
!  - the terminal emulator:  Neither MIT X11R5/6 nor Suns openwin xterm
!    know about that.  Use the 'xfree xterm' or 'rxvt'.  Both compile on
!    all major platforms.
!  - It only works if xterm is wrapping the line itself
!    (not always really obvious for the user, though).
!  - Among the different vi's, vim actually supports this with a
!    clever and little hackish trick (see screen.c):
!
!    But before: vim inspects the _name_ of the value of TERM.
!    This must be similar to "xterm" (like "xterm-xfree86", which is
!    better than "xterm-color", btw, see his FAQ).
!    The terminfo entry _itself_ doesn't matter here
!    (e.g.: 'xterm' and 'vs100' are the same entry, but with
!     the latter it doesn't work).
!
!    If vim has to wrap a word, it appends a space at the first part,
!    this space will be wrapped by xterm.  Going on with writing, vim
!    in turn then positions the cursor again at the _beginning_ of this
!    next line. Thus, the space is not visible.  But xterm now believes
!    that the two lines are actually a single one--as xterm _has_ done
!    some wrapping also...

mofux · 2017-04-11T09:09:20Z

Hmmm.... my theory is that other terminals will also reflow the additional character into the next line, but it then gets overwritten because the cursor is manually positioned to where the extra character would be (see the tty raw data, there is a control sequence that manually sets the cursor to the start of this new line, in your example above �[m�[H�[2J�[1;30Hqwertt�[2;1Hy its the �[2;1H that sets the cursor to row 2 column 1, which is where the extra character would be). I believe that we have to account the wrapped line as a real line, so when the cursor is set to row 2, it has to go to the wrapped line.

LucianBuzzo · 2017-04-11T09:11:30Z

@mofux, yep this is my conclusion as well. Currently, my implementation moves the cursor to the source row, rather than the rendered row, which is why it isn't overwriting the duped character.
Oh well, at least I know how to progress with this feature now!

parisk · 2017-04-21T11:27:08Z

This thread has been inactive for about 10 days, so I just want to remind @LucianBuzzo that if you need any input at any point here, just ask 😄 !

I think that this is a great enhancement to the xterm.js library, so I will put priority on this, in order to help out.

EDIT: There is no pressure here though.

LucianBuzzo · 2017-04-21T12:23:58Z

@parisk I pushed some commits a few days ago fixing the vim issue and the scroll to bottom issue reported by @Tyriar
AFAICT We only need to sort the line wrap issue on the command prompt and we're there.
If you can give the branch a shakedown and see if anything is broken or showing weird behaviour that would be ace! 👍

Tyriar · 2017-04-21T15:43:42Z

src/Buffer.ts

+   * see https://jsperf.com/math-ceil-vs-bitwise
+   *
+   * @param {number} n - The value to round up.
+   *


Style nit: be more concise here:

@param ... @return .. @example ...

Tyriar · 2017-04-21T15:49:28Z

src/Buffer.ts

+    let f = (n << 0);
+    return f === n ? f : f + 1;
+  }
+  /**


Style nit: Empty line here

Tyriar · 2017-04-21T15:51:57Z

src/InputHandler.ts

-      // TODO: needs a global min terminal width of 2
-      if (this._terminal.x + ch_width - 1 >= this._terminal.cols) {
-        // autowrap - DECAWM
-        if (this._terminal.wraparoundMode) {


I think we still want to respect this

Tyriar · 2017-04-21T15:55:21Z

src/Interfaces.ts

@@ -60,7 +61,7 @@ export interface ILinkifier {
  deregisterLinkMatcher(matcherId: number): boolean;
 }

-interface ICircularList<T> {
+interface IBuffer<T> {


Keeping the IBuffer and ICircularList interfaces separate would be cleaner, you should be able to do this:

interface IBuffer extends ICircularList<string> { ... }

Also I don't think IBuffer should be generic.

Tyriar · 2017-04-21T15:56:31Z

src/Interfaces.ts

@@ -23,7 +23,8 @@ export interface ITerminal {
  textarea: HTMLTextAreaElement;
  ybase: number;
  ydisp: number;
-  lines: ICircularList<string>;
+  scrollBase: number;


It used to be an ICircularList<string> but I'm not sure this is true, I think lines is an ICircularList<any[]>? (Until we move xterm.js fully to TS)

Tyriar · 2017-04-21T15:58:56Z

src/xterm.js

@@ -1230,7 +1241,7 @@ Terminal.prototype.scrollToTop = function() {
 * Scrolls the display of the terminal to the bottom.
 */
 Terminal.prototype.scrollToBottom = function() {
-  this.scrollDisp(this.ybase - this.ydisp);
+  this.scrollDisp(this.scrollBase - this.ydisp);


What's the difference between ybase and scrollbase?

scrollBase compensates for wrapped lines.

LucianBuzzo · 2017-04-24T12:12:31Z

I'm closing this PR in favour of #644

parisk mentioned this pull request Mar 17, 2017

Line wrap #404

Closed

LucianBuzzo mentioned this pull request Mar 17, 2017

VIM support imsnif/xterm.js#1

Open

Tyriar reviewed Mar 17, 2017

View reviewed changes

LucianBuzzo mentioned this pull request Mar 19, 2017

Prevent line data from being discarded on resize. #616

Merged

LucianBuzzo added 4 commits March 21, 2017 00:27

Add logic for reflowing lines and preserving data when resizing

f10fc2c

Stopped characters being written into new line on overflow

bec37d3

Don't add blank characters to lines

115c215

Scrollable height is now correctly calculated

a625570

Tyriar requested changes Mar 21, 2017

View reviewed changes

LucianBuzzo added 2 commits March 23, 2017 11:24

Keep wrapped line total as internal state.

32c41e8

Improved performance of buffer operations

f88255d

Tyriar mentioned this pull request Mar 27, 2017

Copy from terminal should not retain new line characters for wrapped lines microsoft/vscode#23142

Closed

Fixed issue with top command not working

30433c7

Tyriar mentioned this pull request Mar 30, 2017

Support reflowing lines on resize #622

Closed

Fixed terminal when running in vt100

e70438b

Tyriar mentioned this pull request Mar 31, 2017

Implement virtual selection #207

Closed

Tyriar mentioned this pull request Apr 3, 2017

Convert xterm.js to TypeScript! #493

Closed

5 tasks

LucianBuzzo mentioned this pull request Apr 10, 2017

Vim sends last character of line twice when running in xterm.js vim/vim#1626

Closed

mofux mentioned this pull request Apr 18, 2017

[Question/Need help] How to make the content of the terminal reactive ? #637

Closed

LucianBuzzo added 2 commits April 19, 2017 09:19

Added logic to handle vim's duplicate character behaviour

2b37495

Fixed scrollBottom logic

c8acd98

Merge branch 'master' into lb-line-wrap

e411a7e

Tyriar reviewed Apr 21, 2017

View reviewed changes

LucianBuzzo mentioned this pull request Apr 24, 2017

Add logic for reflowing lines #644

Closed

LucianBuzzo closed this Apr 24, 2017

Add logic for reflowing lines and preserving data when resizing #609

Add logic for reflowing lines and preserving data when resizing #609

Conversation

LucianBuzzo commented Mar 16, 2017 • edited by Tyriar Loading

coveralls commented Mar 16, 2017

mofux commented Mar 16, 2017

blink1073 commented Mar 16, 2017

LucianBuzzo commented Mar 16, 2017

jerch commented Mar 16, 2017

Tyriar commented Mar 16, 2017

LucianBuzzo commented Mar 16, 2017

jerch commented Mar 16, 2017

mofux commented Mar 17, 2017 • edited Loading

jerch commented Mar 17, 2017 • edited Loading

Tyriar commented Mar 17, 2017

Tyriar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucianBuzzo commented Mar 21, 2017

Choose a reason for hiding this comment

Tyriar commented Mar 27, 2017

LucianBuzzo commented Mar 28, 2017

mofux commented Mar 28, 2017

LucianBuzzo commented Mar 29, 2017

mofux commented Mar 29, 2017

LucianBuzzo commented Mar 30, 2017

LucianBuzzo commented Mar 30, 2017

mofux commented Mar 30, 2017

LucianBuzzo commented Mar 30, 2017

mofux commented Mar 30, 2017

LucianBuzzo commented Mar 31, 2017

Tyriar commented Apr 1, 2017

Tyriar commented Apr 1, 2017

Tyriar commented Apr 1, 2017 • edited Loading

LucianBuzzo commented Apr 2, 2017

jerch commented Apr 2, 2017 • edited Loading

LucianBuzzo commented Apr 7, 2017

LucianBuzzo commented Apr 11, 2017 • edited Loading

mofux commented Apr 11, 2017

LucianBuzzo commented Apr 11, 2017

parisk commented Apr 21, 2017 • edited Loading

LucianBuzzo commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucianBuzzo commented Apr 24, 2017

LucianBuzzo commented Mar 16, 2017 •

edited by Tyriar

Loading

mofux commented Mar 17, 2017 •

edited

Loading

jerch commented Mar 17, 2017 •

edited

Loading

Tyriar commented Apr 1, 2017 •

edited

Loading

jerch commented Apr 2, 2017 •

edited

Loading

LucianBuzzo commented Apr 11, 2017 •

edited

Loading

parisk commented Apr 21, 2017 •

edited

Loading