Merge pull request #855 from Tyriar/847_proper_scrollback_value

Make buffer length equal rows + scrollback, make alt buffer always 0 scrollback

src/Buffer.test.ts

@@ -20,13 +20,13 @@ describe('Buffer', () => {
     terminal.cols = INIT_COLS;
     terminal.rows = INIT_ROWS;
     terminal.options.scrollback = 1000;
-    buffer = new Buffer(terminal);
+    buffer = new Buffer(terminal, true);
   });
 
   describe('constructor', () => {
-    it('should create a CircularList with max length equal to scrollback, for its lines', () => {
+    it('should create a CircularList with max length equal to rows + scrollback, for its lines', () => {
       assert.instanceOf(buffer.lines, CircularList);
-      assert.equal(buffer.lines.maxLength, terminal.options.scrollback);
+      assert.equal(buffer.lines.maxLength, terminal.rows + terminal.options.scrollback);
     });
     it('should set the Buffer\'s scrollBottom value equal to the terminal\'s rows -1', () => {
       assert.equal(buffer.scrollBottom, terminal.rows - 1);
@@ -88,6 +88,21 @@ describe('Buffer', () => {
         assert.equal(buffer.ydisp, 5);
         assert.equal(buffer.ybase, 5);
       });
+
+      describe('no scrollback', () => {
+        it('should trim from the top of the buffer when the cursor reaches the bottom', () => {
+          terminal.options.scrollback = 0;
+          buffer = new Buffer(terminal, true);
+          assert.equal(buffer.lines.maxLength, INIT_ROWS);
+          buffer.y = INIT_ROWS - 1;
+          buffer.fillViewportRows();
+          buffer.lines.get(5)[0][1] = 'a';
+          buffer.lines.get(INIT_ROWS - 1)[0][1] = 'b';
+          buffer.resize(INIT_COLS, INIT_ROWS - 5);
+          assert.equal(buffer.lines.get(0)[0][1], 'a');
+          assert.equal(buffer.lines.get(INIT_ROWS - 1 - 5)[0][1], 'b');
+        });
+      });
     });
 
     describe('row size increased', () => {
@@ -156,4 +171,20 @@ describe('Buffer', () => {
       });
     });
   });
+
+  describe('buffer marked to have no scrollback', () => {
+    it('should always have a scrollback of 0', () => {
+      assert.equal(terminal.options.scrollback, 1000);
+      // Test size on initialization
+      buffer = new Buffer(terminal, false);
+      buffer.fillViewportRows();
+      assert.equal(buffer.lines.maxLength, INIT_ROWS);
+      // Test size on buffer increase
+      buffer.resize(INIT_COLS, INIT_ROWS * 2);
+      assert.equal(buffer.lines.maxLength, INIT_ROWS * 2);
+      // Test size on buffer decrease
+      buffer.resize(INIT_COLS, INIT_ROWS / 2);
+      assert.equal(buffer.lines.maxLength, INIT_ROWS / 2);
+    });
+  });
 });

src/Buffer.ts

@@ -31,14 +31,13 @@ export class Buffer implements IBuffer {
 
   /**
    * Create a new Buffer.
-   * @param {Terminal} _terminal - The terminal the Buffer will belong to
-   * @param {number} ydisp - The scroll position of the Buffer in the viewport
-   * @param {number} ybase - The scroll position of the y cursor (ybase + y = the y position within the Buffer)
-   * @param {number} y - The cursor's y position after ybase
-   * @param {number} x - The cursor's x position after ybase
+   * @param _terminal The terminal the Buffer will belong to.
+   * @param _hasScrollback Whether the buffer should respecr the scrollback of
+   * the terminal..
    */
   constructor(
-    private _terminal: ITerminal
+    private _terminal: ITerminal,
+    private _hasScrollback: boolean
   ) {
     this.clear();
   }
@@ -47,6 +46,18 @@ export class Buffer implements IBuffer {
     return this._lines;
   }
 
+  /**
+   * 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.
+   * @param rows The terminal rows to use in the calculation.
+   */
+  private _getCorrectBufferLength(rows: number): number {
+    if (!this._hasScrollback) {
+      return rows;
+    }
+    return rows + this._terminal.options.scrollback;
+  }
+
   /**
    * Fills the buffer's viewport with blank lines.
    */
@@ -70,7 +81,7 @@ export class Buffer implements IBuffer {
     this.scrollBottom = 0;
     this.scrollTop = 0;
     this.tabs = {};
-    this._lines = new CircularList<LineData>(this._terminal.options.scrollback);
+    this._lines = new CircularList<LineData>(this._getCorrectBufferLength(this._terminal.rows));
     this.scrollBottom = this._terminal.rows - 1;
   }
 
@@ -85,6 +96,13 @@ export class Buffer implements IBuffer {
       return;
     }
 
+    // Increase max length if needed before adjustments to allow space to fill
+    // as required.
+    const newMaxLength = this._getCorrectBufferLength(newRows);
+    if (newMaxLength > this._lines.maxLength) {
+      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?
@@ -136,6 +154,19 @@ export class Buffer implements IBuffer {
       }
     }
 
+    // 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;
+    }
+
     // Make sure that the cursor stays on screen
     if (this.y >= newRows) {
       this.y = newRows - 1;

src/BufferSet.ts

@@ -21,9 +21,12 @@ export class BufferSet extends EventEmitter implements IBufferSet {
    */
   constructor(private _terminal: ITerminal) {
     super();
-    this._normal = new Buffer(this._terminal);
+    this._normal = new Buffer(this._terminal, true);
     this._normal.fillViewportRows();
-    this._alt = new Buffer(this._terminal);
+
+    // The alt buffer should never have scrollback.
+    // See http://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h2-The-Alternate-Screen-Buffer
+    this._alt = new Buffer(this._terminal, false);
     this._activeBuffer = this._normal;
   }
 
@@ -71,7 +74,6 @@ export class BufferSet extends EventEmitter implements IBufferSet {
     // Since the alt buffer is always cleared when the normal buffer is
     // activated, we want to fill it when switching to it.
     this._alt.fillViewportRows();
-
     this._activeBuffer = this._alt;
     this.emit('activate', this._alt);
   }

src/InputHandler.ts

@@ -443,23 +443,13 @@ export class InputHandler implements IInputHandler {
     }
     let row: number = this._terminal.buffer.y + this._terminal.buffer.ybase;
 
-    let j: number;
-    j = this._terminal.rows - 1 - this._terminal.buffer.scrollBottom;
-    j = this._terminal.rows - 1 + this._terminal.buffer.ybase - j + 1;
-
+    let scrollBottomRowsOffset = this._terminal.rows - 1 - this._terminal.buffer.scrollBottom;
+    let scrollBottomAbsolute = this._terminal.rows - 1 + this._terminal.buffer.ybase - scrollBottomRowsOffset + 1;
     while (param--) {
-      if (this._terminal.buffer.lines.length === this._terminal.buffer.lines.maxLength) {
-        // Trim the start of lines to make room for the new line
-        this._terminal.buffer.lines.trimStart(1);
-        this._terminal.buffer.ybase--;
-        this._terminal.buffer.ydisp--;
-        row--;
-        j--;
-      }
       // test: echo -e '\e[44m\e[1L\e[0m'
       // blankLine(true) - xterm/linux behavior
+      this._terminal.buffer.lines.splice(scrollBottomAbsolute - 1, 1);
       this._terminal.buffer.lines.splice(row, 0, this._terminal.blankLine(true));
-      this._terminal.buffer.lines.splice(j, 1);
     }
 
     // this.maxRange();
@@ -481,18 +471,11 @@ export class InputHandler implements IInputHandler {
     let j: number;
     j = this._terminal.rows - 1 - this._terminal.buffer.scrollBottom;
     j = this._terminal.rows - 1 + this._terminal.buffer.ybase - j;
-
     while (param--) {
-      if (this._terminal.buffer.lines.length === this._terminal.buffer.lines.maxLength) {
-        // Trim the start of lines to make room for the new line
-        this._terminal.buffer.lines.trimStart(1);
-        this._terminal.buffer.ybase -= 1;
-        this._terminal.buffer.ydisp -= 1;
-      }
       // test: echo -e '\e[44m\e[1M\e[0m'
       // blankLine(true) - xterm/linux behavior
-      this._terminal.buffer.lines.splice(j + 1, 0, this._terminal.blankLine(true));
-      this._terminal.buffer.lines.splice(row, 1);
+      this._terminal.buffer.lines.splice(row - 1, 1);
+      this._terminal.buffer.lines.splice(j, 0, this._terminal.blankLine(true));
     }
 
     // this.maxRange();

src/Terminal.test.ts

@@ -95,16 +95,6 @@ describe('term.js addons', () => {
   });
 
   describe('setOption', () => {
-    let originalWarn;
-    let warnCallCount;
-    beforeEach(() => {
-      originalWarn = console.warn;
-      warnCallCount = 0;
-      console.warn = () => warnCallCount++;
-    });
-    afterEach(() => {
-      console.warn = originalWarn;
-    });
     it('should set the option correctly', () => {
       term.setOption('cursorBlink', true);
       assert.equal(term.options.cursorBlink, true);
@@ -114,11 +104,6 @@ describe('term.js addons', () => {
     it('should throw when setting a non-existant option', () => {
       assert.throws(term.setOption.bind(term, 'fake', true));
     });
-    it('should warn and do nothing when scrollback is less than number of rows', () => {
-      term.setOption('scrollback', term.rows - 1);
-      assert.equal(term.getOption('scrollback'), 1000);
-      assert.equal(warnCallCount, 1);
-    });
   });
 
   describe('clear', () => {

src/Terminal.ts

@@ -453,18 +453,10 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
         }
         break;
       case 'scrollback':
-        if (value < this.rows) {
-          let msg = 'Setting the scrollback value less than the number of rows ';
-
-          msg += `(${this.rows}) is not allowed.`;
-
-          console.warn(msg);
-          return;
-        }
-
         if (this.options[key] !== value) {
-          if (this.buffer.lines.length > value) {
-            const amountToTrim = this.buffer.lines.length - value;
+          const newBufferLength = this.rows + value;
+          if (this.buffer.lines.length > newBufferLength) {
+            const amountToTrim = this.buffer.lines.length - newBufferLength;
             const needsRefresh = (this.buffer.ydisp - amountToTrim < 0);
             this.buffer.lines.trimStart(amountToTrim);
             this.buffer.ybase = Math.max(this.buffer.ybase - amountToTrim, 0);
@@ -473,8 +465,6 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
               this.refresh(0, this.rows - 1);
             }
           }
-          this.buffer.lines.maxLength = value;
-          this.viewport.syncScrollArea();
         }
         break;
     }
@@ -487,6 +477,10 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
         this.element.classList.toggle(`xterm-cursor-style-underline`, value === 'underline');
         this.element.classList.toggle(`xterm-cursor-style-bar`, value === 'bar');
         break;
+      case 'scrollback':
+        this.buffers.resize(this.cols, this.rows);
+        this.viewport.syncScrollArea();
+        break;
       case 'tabStopWidth': this.setupStops(); break;
     }
   }
@@ -1178,17 +1172,16 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
     let row;
 
     // Make room for the new row in lines
-    if (this.buffer.lines.length === this.buffer.lines.maxLength) {
+    const bufferNeedsTrimming = this.buffer.lines.length === this.buffer.lines.maxLength;
+    if (bufferNeedsTrimming) {
       this.buffer.lines.trimStart(1);
       this.buffer.ybase--;
-      if (this.buffer.ydisp !== 0) {
-        this.buffer.ydisp--;
-      }
+      this.buffer.ydisp = Math.max(this.buffer.ydisp - 1, 0);
     }
 
     this.buffer.ybase++;
 
-    // TODO: Why is this done twice?
+    // Scroll the viewport down to the bottom if the user is not scrolling
     if (!this.userScrolling) {
       this.buffer.ydisp = this.buffer.ybase;
     }
@@ -1918,10 +1911,6 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
       return;
     }
 
-    if (y > this.getOption('scrollback')) {
-      this.setOption('scrollback', y);
-    }
-
     let line;
     let el;
     let i;

src/utils/CircularList.test.ts

@@ -5,6 +5,10 @@
 import { assert } from 'chai';
 import { CircularList } from './CircularList';
 
+class TestCircularList<T> extends CircularList<T> {
+  public get array(): T[] { return this._array; }
+}
+
 describe('CircularList', () => {
   describe('push', () => {
     it('should push values onto the array', () => {

src/utils/CircularList.ts

@@ -9,29 +9,37 @@ import { EventEmitter } from '../EventEmitter';
 import { ICircularList } from '../Interfaces';
 
 export class CircularList<T> extends EventEmitter implements ICircularList<T> {
-  private _array: T[];
+  protected _array: T[];
   private _startIndex: number;
   private _length: number;
 
-  constructor(maxLength: number) {
+  constructor(
+    private _maxLength: number
+  ) {
     super();
-    this._array = new Array<T>(maxLength);
+    this._array = new Array<T>(this._maxLength);
     this._startIndex = 0;
     this._length = 0;
   }
 
   public get maxLength(): number {
-    return this._array.length;
+    return this._maxLength;
   }
 
   public set maxLength(newMaxLength: number) {
+    // There was no change in maxLength, return early.
+    if (this._maxLength === newMaxLength) {
+      return;
+    }
+
     // Reconstruct array, starting at index 0. Only transfer values from the
     // indexes 0 to length.
     let newArray = new Array<T>(newMaxLength);
     for (let i = 0; i < Math.min(newMaxLength, this.length); i++) {
       newArray[i] = this._array[this._getCyclicIndex(i)];
     }
     this._array = newArray;
+    this._maxLength = newMaxLength;
     this._startIndex = 0;
   }
 
@@ -89,9 +97,9 @@ export class CircularList<T> extends EventEmitter implements ICircularList<T> {
    */
   public push(value: T): void {
     this._array[this._getCyclicIndex(this._length)] = value;
-    if (this._length === this.maxLength) {
+    if (this._length === this._maxLength) {
       this._startIndex++;
-      if (this._startIndex === this.maxLength) {
+      if (this._startIndex === this._maxLength) {
         this._startIndex = 0;
       }
       this.emit('trim', 1);