Merge pull request #923 from Tyriar/922_fix_scroll_RI

Fix issues with scroll and reverseIndex

src/Buffer.ts

@@ -46,6 +46,10 @@ export class Buffer implements IBuffer {
     return this._lines;
   }
 
+  public get hasScrollback(): boolean {
+    return this._hasScrollback && this.lines.maxLength > this._terminal.rows;
+  }
+
   /**
    * Gets the correct buffer length based on the rows provided, the terminal's
    * scrollback and whether this buffer is flagged to have scrollback or not.
@@ -78,10 +82,9 @@ export class Buffer implements IBuffer {
     this.ybase = 0;
     this.y = 0;
     this.x = 0;
-    this.scrollBottom = 0;
-    this.scrollTop = 0;
     this.tabs = {};
     this._lines = new CircularList<LineData>(this._getCorrectBufferLength(this._terminal.rows));
+    this.scrollTop = 0;
     this.scrollBottom = this._terminal.rows - 1;
   }
 
@@ -91,11 +94,6 @@ export class Buffer implements IBuffer {
    * @param newRows The new number of rows.
    */
   public resize(newCols: number, newRows: number): void {
-    // Don't resize the buffer if it's empty and hasn't been used yet.
-    if (this._lines.length === 0) {
-      return;
-    }
-
     // Increase max length if needed before adjustments to allow space to fill
     // as required.
     const newMaxLength = this._getCorrectBufferLength(newRows);
@@ -103,83 +101,88 @@ export class Buffer implements IBuffer {
       this._lines.maxLength = newMaxLength;
     }
 
-    // Deal with columns increasing (we don't do anything when columns reduce)
-    if (this._terminal.cols < newCols) {
-      const ch: CharData = [this._terminal.defAttr, ' ', 1]; // does xterm use the default attr?
-      for (let i = 0; i < this._lines.length; i++) {
-        // TODO: This should be removed, with tests setup for the case that was
-        // causing the underlying bug, see https://github.com/sourcelair/xterm.js/issues/824
-        if (this._lines.get(i) === undefined) {
-          this._lines.set(i, this._terminal.blankLine(undefined, undefined, newCols));
-        }
-        while (this._lines.get(i).length < newCols) {
-          this._lines.get(i).push(ch);
+    // The following adjustments should only happen if the buffer has been
+    // initialized/filled.
+    if (this._lines.length > 0) {
+      // Deal with columns increasing (we don't do anything when columns reduce)
+      if (this._terminal.cols < newCols) {
+        const ch: CharData = [this._terminal.defAttr, ' ', 1]; // does xterm use the default attr?
+        for (let i = 0; i < this._lines.length; i++) {
+          // TODO: This should be removed, with tests setup for the case that was
+          // causing the underlying bug, see https://github.com/sourcelair/xterm.js/issues/824
+          if (this._lines.get(i) === undefined) {
+            this._lines.set(i, this._terminal.blankLine(undefined, undefined, newCols));
+          }
+          while (this._lines.get(i).length < newCols) {
+            this._lines.get(i).push(ch);
+          }
         }
       }
-    }
 
-    // Resize rows in both directions as needed
-    let addToY = 0;
-    if (this._terminal.rows < newRows) {
-      for (let y = this._terminal.rows; y < newRows; y++) {
-        if (this._lines.length < newRows + this.ybase) {
-          if (this.ybase > 0 && this._lines.length <= this.ybase + this.y + addToY + 1) {
-            // There is room above the buffer and there are no empty elements below the line,
-            // scroll up
-            this.ybase--;
-            addToY++;
-            if (this.ydisp > 0) {
-              // Viewport is at the top of the buffer, must increase downwards
-              this.ydisp--;
+      // Resize rows in both directions as needed
+      let addToY = 0;
+      if (this._terminal.rows < newRows) {
+        for (let y = this._terminal.rows; y < newRows; y++) {
+          if (this._lines.length < newRows + this.ybase) {
+            if (this.ybase > 0 && this._lines.length <= this.ybase + this.y + addToY + 1) {
+              // There is room above the buffer and there are no empty elements below the line,
+              // scroll up
+              this.ybase--;
+              addToY++;
+              if (this.ydisp > 0) {
+                // Viewport is at the top of the buffer, must increase downwards
+                this.ydisp--;
+              }
+            } else {
+              // Add a blank line if there is no buffer left at the top to scroll to, or if there
+              // are blank lines after the cursor
+              this._lines.push(this._terminal.blankLine(undefined, undefined, newCols));
+            }
+          }
+        }
+      } else { // (this._terminal.rows >= newRows)
+        for (let y = this._terminal.rows; y > newRows; y--) {
+          if (this._lines.length > newRows + this.ybase) {
+            if (this._lines.length > this.ybase + this.y + 1) {
+              // The line is a blank line below the cursor, remove it
+              this._lines.pop();
+            } else {
+              // The line is the cursor, scroll down
+              this.ybase++;
+              this.ydisp++;
             }
-          } else {
-            // Add a blank line if there is no buffer left at the top to scroll to, or if there
-            // are blank lines after the cursor
-            this._lines.push(this._terminal.blankLine(undefined, undefined, newCols));
           }
         }
       }
-    } else { // (this._terminal.rows >= newRows)
-      for (let y = this._terminal.rows; y > newRows; y--) {
-        if (this._lines.length > newRows + this.ybase) {
-          if (this._lines.length > this.ybase + this.y + 1) {
-            // The line is a blank line below the cursor, remove it
-            this._lines.pop();
-          } else {
-            // The line is the cursor, scroll down
-            this.ybase++;
-            this.ydisp++;
-          }
+
+      // Reduce max length if needed after adjustments, this is done after as it
+      // would otherwise cut data from the bottom of the buffer.
+      if (newMaxLength < this._lines.maxLength) {
+        // Trim from the top of the buffer and adjust ybase and ydisp.
+        const amountToTrim = this._lines.length - newMaxLength;
+        if (amountToTrim > 0) {
+          this._lines.trimStart(amountToTrim);
+          this.ybase = Math.max(this.ybase - amountToTrim, 0);
+          this.ydisp = Math.max(this.ydisp - amountToTrim, 0);
         }
+        this._lines.maxLength = newMaxLength;
       }
-    }
 
-    // Reduce max length if needed after adjustments, this is done after as it
-    // would otherwise cut data from the bottom of the buffer.
-    if (newMaxLength < this._lines.maxLength) {
-      // Trim from the top of the buffer and adjust ybase and ydisp.
-      const amountToTrim = this._lines.length - newMaxLength;
-      if (amountToTrim > 0) {
-        this._lines.trimStart(amountToTrim);
-        this.ybase = Math.max(this.ybase - amountToTrim, 0);
-        this.ydisp = Math.max(this.ydisp - amountToTrim, 0);
+      // Make sure that the cursor stays on screen
+      if (this.y >= newRows) {
+        this.y = newRows - 1;
       }
-      this._lines.maxLength = newMaxLength;
+      if (addToY) {
+        this.y += addToY;
+      }
+
+      if (this.x >= newCols) {
+        this.x = newCols - 1;
+      }
+
+      this.scrollTop = 0;
     }
 
-    // Make sure that the cursor stays on screen
-    if (this.y >= newRows) {
-      this.y = newRows - 1;
-    }
-    if (addToY) {
-      this.y += addToY;
-    }
-
-    if (this.x >= newCols) {
-      this.x = newCols - 1;
-    }
-
-    this.scrollTop = 0;
     this.scrollBottom = newRows - 1;
   }
 

src/Terminal.test.ts

@@ -3,6 +3,9 @@ import { Terminal } from './Terminal';
 import { MockViewport, MockCompositionHelper } from './utils/TestUtils.test';
 import { CHAR_DATA_CHAR_INDEX, CHAR_DATA_WIDTH_INDEX } from './Buffer';
 
+const INIT_COLS = 80;
+const INIT_ROWS = 24;
+
 class TestTerminal extends Terminal {
   public evaluateKeyEscapeSequence(ev: any): {cancel: boolean, key: string, scrollDisp: number} { return this._evaluateKeyEscapeSequence(<KeyboardEvent>ev); }
   public keyDown(ev: any): boolean { return this._keyDown(ev); }
@@ -13,7 +16,10 @@ describe('term.js addons', () => {
   let term: TestTerminal;
 
   beforeEach(() => {
-    term = new TestTerminal();
+    term = new TestTerminal({
+      cols: INIT_COLS,
+      rows: INIT_ROWS
+    });
     term.refresh = () => {};
     term.viewport = new MockViewport();
     (<any>term).compositionHelper = new MockCompositionHelper();
@@ -283,6 +289,138 @@ describe('term.js addons', () => {
         assert.equal(term.buffer.ydisp, startYDisp - 1);
       });
     });
+
+    describe('scroll() function', () => {
+      describe('when scrollback > 0', () => {
+        it('should create a new line and scroll', () => {
+          term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX] = 'a';
+          term.buffer.lines.get(INIT_ROWS - 1)[0][CHAR_DATA_CHAR_INDEX] = 'b';
+          term.buffer.y = INIT_ROWS - 1; // Move cursor to last line
+          term.scroll();
+          assert.equal(term.buffer.lines.length, INIT_ROWS + 1);
+          assert.equal(term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX], 'a');
+          assert.equal(term.buffer.lines.get(INIT_ROWS - 1)[0][CHAR_DATA_CHAR_INDEX], 'b');
+          assert.equal(term.buffer.lines.get(INIT_ROWS)[0][CHAR_DATA_CHAR_INDEX], ' ');
+        });
+
+        it('should properly scroll inside a scroll region (scrollTop set)', () => {
+          term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX] = 'a';
+          term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX] = 'b';
+          term.buffer.lines.get(2)[0][CHAR_DATA_CHAR_INDEX] = 'c';
+          term.buffer.y = INIT_ROWS - 1; // Move cursor to last line
+          term.buffer.scrollTop = 1;
+          term.scroll();
+          assert.equal(term.buffer.lines.length, INIT_ROWS);
+          assert.equal(term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX], 'a');
+          assert.equal(term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX], 'c');
+        });
+
+        it('should properly scroll inside a scroll region (scrollBottom set)', () => {
+          term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX] = 'a';
+          term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX] = 'b';
+          term.buffer.lines.get(2)[0][CHAR_DATA_CHAR_INDEX] = 'c';
+          term.buffer.lines.get(3)[0][CHAR_DATA_CHAR_INDEX] = 'd';
+          term.buffer.lines.get(4)[0][CHAR_DATA_CHAR_INDEX] = 'e';
+          term.buffer.y = 3;
+          term.buffer.scrollBottom = 3;
+          term.scroll();
+          assert.equal(term.buffer.lines.length, INIT_ROWS + 1);
+          assert.equal(term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX], 'a', '\'a\' should be pushed to the scrollback');
+          assert.equal(term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX], 'b');
+          assert.equal(term.buffer.lines.get(2)[0][CHAR_DATA_CHAR_INDEX], 'c');
+          assert.equal(term.buffer.lines.get(3)[0][CHAR_DATA_CHAR_INDEX], 'd');
+          assert.equal(term.buffer.lines.get(4)[0][CHAR_DATA_CHAR_INDEX], ' ', 'a blank line should be added at scrollBottom\'s index');
+          assert.equal(term.buffer.lines.get(5)[0][CHAR_DATA_CHAR_INDEX], 'e');
+        });
+
+        it('should properly scroll inside a scroll region (scrollTop and scrollBottom set)', () => {
+          term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX] = 'a';
+          term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX] = 'b';
+          term.buffer.lines.get(2)[0][CHAR_DATA_CHAR_INDEX] = 'c';
+          term.buffer.lines.get(3)[0][CHAR_DATA_CHAR_INDEX] = 'd';
+          term.buffer.lines.get(4)[0][CHAR_DATA_CHAR_INDEX] = 'e';
+          term.buffer.y = INIT_ROWS - 1; // Move cursor to last line
+          term.buffer.scrollTop = 1;
+          term.buffer.scrollBottom = 3;
+          term.scroll();
+          assert.equal(term.buffer.lines.length, INIT_ROWS);
+          assert.equal(term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX], 'a');
+          assert.equal(term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX], 'c', '\'b\' should be removed from the buffer');
+          assert.equal(term.buffer.lines.get(2)[0][CHAR_DATA_CHAR_INDEX], 'd');
+          assert.equal(term.buffer.lines.get(3)[0][CHAR_DATA_CHAR_INDEX], ' ', 'a blank line should be added at scrollBottom\'s index');
+          assert.equal(term.buffer.lines.get(4)[0][CHAR_DATA_CHAR_INDEX], 'e');
+        });
+      });
+
+      describe('when scrollback === 0', () => {
+        beforeEach(() => {
+          term.setOption('scrollback', 0);
+          assert.equal(term.buffer.lines.maxLength, INIT_ROWS);
+        });
+
+        it('should create a new line and shift everything up', () => {
+          term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX] = 'a';
+          term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX] = 'b';
+          term.buffer.lines.get(INIT_ROWS - 1)[0][CHAR_DATA_CHAR_INDEX] = 'c';
+          term.buffer.y = INIT_ROWS - 1; // Move cursor to last line
+          assert.equal(term.buffer.lines.length, INIT_ROWS);
+          term.scroll();
+          assert.equal(term.buffer.lines.length, INIT_ROWS);
+          // 'a' gets pushed out of buffer
+          assert.equal(term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX], 'b');
+          assert.equal(term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX], ' ');
+          assert.equal(term.buffer.lines.get(INIT_ROWS - 2)[0][CHAR_DATA_CHAR_INDEX], 'c');
+          assert.equal(term.buffer.lines.get(INIT_ROWS - 1)[0][CHAR_DATA_CHAR_INDEX], ' ');
+        });
+
+        it('should properly scroll inside a scroll region (scrollTop set)', () => {
+          term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX] = 'a';
+          term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX] = 'b';
+          term.buffer.lines.get(2)[0][CHAR_DATA_CHAR_INDEX] = 'c';
+          term.buffer.y = INIT_ROWS - 1; // Move cursor to last line
+          term.buffer.scrollTop = 1;
+          term.scroll();
+          assert.equal(term.buffer.lines.length, INIT_ROWS);
+          assert.equal(term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX], 'a');
+          assert.equal(term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX], 'c');
+        });
+
+        it('should properly scroll inside a scroll region (scrollBottom set)', () => {
+          term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX] = 'a';
+          term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX] = 'b';
+          term.buffer.lines.get(2)[0][CHAR_DATA_CHAR_INDEX] = 'c';
+          term.buffer.lines.get(3)[0][CHAR_DATA_CHAR_INDEX] = 'd';
+          term.buffer.lines.get(4)[0][CHAR_DATA_CHAR_INDEX] = 'e';
+          term.buffer.y = 3;
+          term.buffer.scrollBottom = 3;
+          term.scroll();
+          assert.equal(term.buffer.lines.length, INIT_ROWS);
+          assert.equal(term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX], 'b');
+          assert.equal(term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX], 'c');
+          assert.equal(term.buffer.lines.get(2)[0][CHAR_DATA_CHAR_INDEX], 'd');
+          assert.equal(term.buffer.lines.get(3)[0][CHAR_DATA_CHAR_INDEX], ' ', 'a blank line should be added at scrollBottom\'s index');
+          assert.equal(term.buffer.lines.get(4)[0][CHAR_DATA_CHAR_INDEX], 'e');
+        });
+
+        it('should properly scroll inside a scroll region (scrollTop and scrollBottom set)', () => {
+          term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX] = 'a';
+          term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX] = 'b';
+          term.buffer.lines.get(2)[0][CHAR_DATA_CHAR_INDEX] = 'c';
+          term.buffer.lines.get(3)[0][CHAR_DATA_CHAR_INDEX] = 'd';
+          term.buffer.lines.get(4)[0][CHAR_DATA_CHAR_INDEX] = 'e';
+          term.buffer.y = INIT_ROWS - 1; // Move cursor to last line
+          term.buffer.scrollTop = 1;
+          term.buffer.scrollBottom = 3;
+          term.scroll();
+          assert.equal(term.buffer.lines.length, INIT_ROWS);
+          assert.equal(term.buffer.lines.get(0)[0][CHAR_DATA_CHAR_INDEX], 'a');
+          assert.equal(term.buffer.lines.get(1)[0][CHAR_DATA_CHAR_INDEX], 'c', '\'b\' should be removed from the buffer');
+          assert.equal(term.buffer.lines.get(2)[0][CHAR_DATA_CHAR_INDEX], 'd');
+          assert.equal(term.buffer.lines.get(3)[0][CHAR_DATA_CHAR_INDEX], ' ', 'a blank line should be added at scrollBottom\'s index');
+          assert.equal(term.buffer.lines.get(4)[0][CHAR_DATA_CHAR_INDEX], 'e');
+        });
+      });
+    });
   });
 
   describe('evaluateKeyEscapeSequence', () => {

src/Terminal.ts

@@ -1163,52 +1163,44 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
 
   /**
    * Scroll the terminal down 1 row, creating a blank line.
-   * @param {boolean} isWrapped Whether the new line is wrapped from the previous
-   * line.
+   * @param isWrapped Whether the new line is wrapped from the previous line.
    */
   public scroll(isWrapped?: boolean): void {
-    let row;
+    const newLine = this.blankLine(undefined, isWrapped);
+    const topRow = this.buffer.ybase + this.buffer.scrollTop;
+    let bottomRow = this.buffer.ybase + this.buffer.scrollBottom;
 
-    // Make room for the new row in lines
-    const bufferNeedsTrimming = this.buffer.lines.length === this.buffer.lines.maxLength;
-    if (bufferNeedsTrimming) {
-      this.buffer.lines.trimStart(1);
-      this.buffer.ybase--;
-      this.buffer.ydisp = Math.max(this.buffer.ydisp - 1, 0);
+    if (this.buffer.scrollTop === 0) {
+      // Determine whether the buffer is going to be trimmed after insertion.
+      const willBufferBeTrimmed = this.buffer.lines.length === this.buffer.lines.maxLength;
+
+      // Insert the line using the fastest method
+      if (bottomRow === this.buffer.lines.length - 1) {
+        this.buffer.lines.push(newLine);
+      } else {
+        this.buffer.lines.splice(bottomRow + 1, 0, newLine);
+      }
+
+      // Only adjust ybase and ydisp when the buffer is not trimmed
+      if (!willBufferBeTrimmed) {
+        this.buffer.ybase++;
+        this.buffer.ydisp++;
+      }
+    } else {
+      // scrollTop is non-zero which means no line will be going to the
+      // scrollback, instead we can just shift them in-place.
+      const scrollRegionHeight = bottomRow - topRow + 1/*as it's zero-based*/;
+      this.buffer.lines.shiftElements(topRow + 1, scrollRegionHeight - 1, -1);
+      this.buffer.lines.set(bottomRow, newLine);
     }
 
-    this.buffer.ybase++;
-
-    // Scroll the viewport down to the bottom if the user is not scrolling
+    // Move the viewport to the bottom of the buffer unless the user is
+    // scrolling.
     if (!this.userScrolling) {
       this.buffer.ydisp = this.buffer.ybase;
     }
 
-    // last line
-    row = this.buffer.ybase + this.rows - 1;
-
-    // subtract the bottom scroll region
-    row -= this.rows - 1 - this.buffer.scrollBottom;
-
-    if (row === this.buffer.lines.length) {
-      // Optimization: pushing is faster than splicing when they amount to the same behavior
-      this.buffer.lines.push(this.blankLine(undefined, isWrapped));
-    } else {
-      // add our new line
-      this.buffer.lines.splice(row, 0, this.blankLine(undefined, isWrapped));
-    }
-
-    if (this.buffer.scrollTop !== 0) {
-      if (this.buffer.ybase !== 0) {
-        this.buffer.ybase--;
-        if (!this.userScrolling) {
-          this.buffer.ydisp = this.buffer.ybase;
-        }
-      }
-      this.buffer.lines.splice(this.buffer.ybase + this.buffer.scrollTop, 1);
-    }
-
-    // this.maxRange();
+    // Flag rows that need updating
     this.updateRange(this.buffer.scrollTop);
     this.updateRange(this.buffer.scrollBottom);
 
@@ -2189,7 +2181,8 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
       // possibly move the code below to term.reverseScroll();
       // test: echo -ne '\e[1;1H\e[44m\eM\e[0m'
       // blankLine(true) is xterm/linux behavior
-      this.buffer.lines.shiftElements(this.buffer.y + this.buffer.ybase, this.rows - 1, 1);
+      const scrollRegionHeight = this.buffer.scrollBottom - this.buffer.scrollTop;
+      this.buffer.lines.shiftElements(this.buffer.y + this.buffer.ybase, scrollRegionHeight, 1);
       this.buffer.lines.set(this.buffer.y + this.buffer.ybase, this.blankLine(true));
       this.updateRange(this.buffer.scrollTop);
       this.updateRange(this.buffer.scrollBottom);