From 109d467b95970a479de0a33083aabfa8e3e09996 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 6 Aug 2017 13:28:58 -0700 Subject: [PATCH 1/5] Make buffer length = rows + scrollback Fixes #847 --- src/Buffer.test.ts | 4 ++-- src/Buffer.ts | 2 +- src/BufferSet.ts | 12 ++++++++++++ src/Terminal.ts | 24 +++++++----------------- src/test/test.js | 17 +---------------- src/utils/CircularList.ts | 3 +++ 6 files changed, 26 insertions(+), 36 deletions(-) diff --git a/src/Buffer.test.ts b/src/Buffer.test.ts index 99bd6d0fe..1e2209155 100644 --- a/src/Buffer.test.ts +++ b/src/Buffer.test.ts @@ -24,9 +24,9 @@ describe('Buffer', () => { }); 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); diff --git a/src/Buffer.ts b/src/Buffer.ts index eb346d5bc..9d2222ff6 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -67,7 +67,7 @@ export class Buffer implements IBuffer { this.scrollBottom = 0; this.scrollTop = 0; this.tabs = {}; - this._lines = new CircularList(this._terminal.options.scrollback); + this._lines = new CircularList(this._terminal.rows + this._terminal.options.scrollback); this.scrollBottom = this._terminal.rows - 1; } diff --git a/src/BufferSet.ts b/src/BufferSet.ts index 24d6c3146..9136c5fb4 100644 --- a/src/BufferSet.ts +++ b/src/BufferSet.ts @@ -76,6 +76,17 @@ export class BufferSet extends EventEmitter implements IBufferSet { this.emit('activate', this._alt); } + /** + * Refreshes the max length of the buffer set, this should be called whenever + * terminal rows or scrollback changes. + * @param rows The number of terminal rows. + */ + public refreshMaxLength(rows: number): void { + this._normal.lines.maxLength = rows + this._terminal.options.scrollback; + // TODO: The alt buffer should never have scrollback, see https://github.com/sourcelair/xterm.js/issues/802 + this._alt.lines.maxLength = rows + this._terminal.options.scrollback; + } + /** * Resizes both normal and alt buffers, adjusting their data accordingly. * @param newCols The new number of columns. @@ -84,5 +95,6 @@ export class BufferSet extends EventEmitter implements IBufferSet { public resize(newCols: number, newRows: number): void { this._normal.resize(newCols, newRows); this._alt.resize(newCols, newRows); + this.refreshMaxLength(newRows); } } diff --git a/src/Terminal.ts b/src/Terminal.ts index cd44e7fb7..2afb45023 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -439,18 +439,10 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT } switch (key) { 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); @@ -459,8 +451,6 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT this.refresh(0, this.rows - 1); } } - this.buffer.lines.maxLength = value; - this.viewport.syncScrollArea(); } break; } @@ -473,6 +463,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.refreshMaxLength(this.rows); + this.viewport.syncScrollArea(); + break; case 'tabStopWidth': this.setupStops(); break; } } @@ -1935,10 +1929,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; diff --git a/src/test/test.js b/src/test/test.js index 5c4dd5f22..51d1644c5 100644 --- a/src/test/test.js +++ b/src/test/test.js @@ -44,16 +44,6 @@ describe('xterm.js', function() { }); describe('setOption', function() { - let originalWarn; - let warnCallCount; - beforeEach(() => { - originalWarn = console.warn; - warnCallCount = 0; - console.warn = () => warnCallCount++; - }); - afterEach(() => { - console.warn = originalWarn; - }); it('should set the option correctly', function() { xterm.setOption('cursorBlink', true); assert.equal(xterm.cursorBlink, true); @@ -63,12 +53,7 @@ describe('xterm.js', function() { assert.equal(xterm.options.cursorBlink, false); }); it('should throw when setting a non-existant option', function() { - assert.throws(xterm.setOption.bind(xterm, 'fake', true)); - }); - it('should warn and do nothing when scrollback is less than number of rows', function() { - xterm.setOption('scrollback', xterm.rows - 1); - assert.equal(xterm.getOption('scrollback'), 1000); - assert.equal(warnCallCount, 1); + assert.throws(() => xterm.setOption('fake', true)); }); }); diff --git a/src/utils/CircularList.ts b/src/utils/CircularList.ts index 373d78b05..23d6c3373 100644 --- a/src/utils/CircularList.ts +++ b/src/utils/CircularList.ts @@ -25,6 +25,9 @@ export class CircularList extends EventEmitter implements ICircularList { } public set maxLength(newMaxLength: number) { + if (this.maxLength === newMaxLength) { + return; + } // Reconstruct array, starting at index 0. Only transfer values from the // indexes 0 to length. let newArray = new Array(newMaxLength); From 27f7679494620e6a2467370c792c58894bd450a5 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 6 Aug 2017 14:17:27 -0700 Subject: [PATCH 2/5] Add some padding and a shrink threshold to CircularList This is needed so that resize is still performant. Now the list size is only rebuild (O(n)) when it goes beyond 40 entries of the initial max length. The List is also rebuilt when the size shrinks below 120 of the actual backing max length (init_max_length + 40 - 120). --- src/utils/CircularList.test.ts | 29 ++++++++++++++++++++++ src/utils/CircularList.ts | 45 +++++++++++++++++++++++++++------- 2 files changed, 65 insertions(+), 9 deletions(-) diff --git a/src/utils/CircularList.test.ts b/src/utils/CircularList.test.ts index e27e7f7a6..891ba3c50 100644 --- a/src/utils/CircularList.test.ts +++ b/src/utils/CircularList.test.ts @@ -5,6 +5,13 @@ import { assert } from 'chai'; import { CircularList } from './CircularList'; +class TestCircularList extends CircularList { + public get array(): T[] { return this._array; } +} + +const MAX_LENGTH_PADDING = 40; +const MAX_LENGTH_SHRINK_REBUILD_THRESHOLD = 120; + describe('CircularList', () => { describe('push', () => { it('should push values onto the array', () => { @@ -68,6 +75,28 @@ describe('CircularList', () => { list.maxLength = 4; assert.equal(list.maxLength, 4); }); + + it('should resize the backing array\'s size only when it increases beyond the allocated padding', () => { + const initSize = 200; + const list = new TestCircularList(initSize); + const expectedBackingMaxLength = initSize + MAX_LENGTH_PADDING; + assert.equal(list.array.length, expectedBackingMaxLength); + list.maxLength = expectedBackingMaxLength; + assert.equal(list.array.length, expectedBackingMaxLength); + list.maxLength = expectedBackingMaxLength + 1; + assert.equal(list.array.length, expectedBackingMaxLength + 1 + MAX_LENGTH_PADDING); + }); + + it('should resize the backing array\'s size only when it reduces beyond the shrink threshold', () => { + const initSize = 200; + const list = new TestCircularList(initSize); + const expectedThresholdMaxLength = initSize + MAX_LENGTH_PADDING - MAX_LENGTH_SHRINK_REBUILD_THRESHOLD; + assert.equal(list.array.length, initSize + MAX_LENGTH_PADDING); + list.maxLength = expectedThresholdMaxLength; + assert.equal(list.array.length, initSize + MAX_LENGTH_PADDING); + list.maxLength = expectedThresholdMaxLength - 1; + assert.equal(list.array.length, expectedThresholdMaxLength - 1 + MAX_LENGTH_PADDING); + }); }); describe('length', () => { diff --git a/src/utils/CircularList.ts b/src/utils/CircularList.ts index 23d6c3373..ed8c065d7 100644 --- a/src/utils/CircularList.ts +++ b/src/utils/CircularList.ts @@ -8,33 +8,60 @@ import { EventEmitter } from '../EventEmitter'; import { ICircularList } from '../Interfaces'; +/** + * Padding added to the max length of the CircularList, this allows increasing + * the max length of the list without reconstructing the backing array and then + * copying over the values. + */ +const MAX_LENGTH_PADDING = 40; + +const MAX_LENGTH_SHRINK_REBUILD_THRESHOLD = MAX_LENGTH_PADDING * 3; + export class CircularList extends EventEmitter implements ICircularList { - private _array: T[]; + protected _array: T[]; private _startIndex: number; private _length: number; - constructor(maxLength: number) { + constructor( + private _maxLength: number + ) { super(); - this._array = new Array(maxLength); + this._array = new Array(this._maxLength + MAX_LENGTH_PADDING); this._startIndex = 0; this._length = 0; } public get maxLength(): number { - return this._array.length; + return this._maxLength; } public set maxLength(newMaxLength: number) { - if (this.maxLength === newMaxLength) { + // There was no change in maxLength, return early. + if (this._maxLength === newMaxLength) { return; } + + // Ensure the array has padding to expand into. + if (this._array.length >= newMaxLength) { + // Ensure that the array hasn't shrunk significantly, if it has we + // should rebuild the array with a new max length to reclaim some memory. + if (this._array.length - newMaxLength <= MAX_LENGTH_SHRINK_REBUILD_THRESHOLD) { + // Increase the max length and return. + this._maxLength = newMaxLength; + return; + } + } + // Reconstruct array, starting at index 0. Only transfer values from the - // indexes 0 to length. - let newArray = new Array(newMaxLength); + // indexes 0 to length. The new array's size includes some padding that + // allows the array's size to increase a little without rebuilding the + // array. + let newArray = new Array(newMaxLength + MAX_LENGTH_PADDING); 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; } @@ -92,9 +119,9 @@ export class CircularList extends EventEmitter implements ICircularList { */ 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); From a57f55dcbf7962662d8ba3e6da75bbea96eb331b Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 6 Aug 2017 18:18:49 -0700 Subject: [PATCH 3/5] Support buffers with scrollback of 0, force alt as 0 Fixes #802 --- src/Buffer.test.ts | 18 +++++++++++++++++- src/Buffer.ts | 46 ++++++++++++++++++++++++++++++++++++++------- src/BufferSet.ts | 17 ++--------------- src/InputHandler.ts | 27 +++++--------------------- src/Terminal.ts | 11 +++++------ 5 files changed, 68 insertions(+), 51 deletions(-) diff --git a/src/Buffer.test.ts b/src/Buffer.test.ts index 1e2209155..2e2ae50c1 100644 --- a/src/Buffer.test.ts +++ b/src/Buffer.test.ts @@ -20,7 +20,7 @@ describe('Buffer', () => { terminal.cols = INIT_COLS; terminal.rows = INIT_ROWS; terminal.options.scrollback = 1000; - buffer = new Buffer(terminal); + buffer = new Buffer(terminal, false); }); describe('constructor', () => { @@ -145,4 +145,20 @@ describe('Buffer', () => { }); }); }); + + describe('alt buffer', () => { + it('should always have a scrollback of 0', () => { + assert.equal(terminal.options.scrollback, 1000); + // Test size on initialization + buffer = new Buffer(terminal, true); + 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); + }); + }); }); diff --git a/src/Buffer.ts b/src/Buffer.ts index 9d2222ff6..5fe977798 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -28,14 +28,12 @@ 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 _isAltBuffer Whether the buffer is the alt buffer. */ constructor( - private _terminal: ITerminal + private _terminal: ITerminal, + private _isAltBuffer: boolean ) { this.clear(); } @@ -44,6 +42,20 @@ 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 is an alt buffer. + * @param rows The terminal rows to use in the calculation. + */ + private _getCorrectBufferLength(rows: number): number { + // The alt buffer should never have scrollback. + // See http://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h2-The-Alternate-Screen-Buffer + if (this._isAltBuffer) { + return rows; + } + return rows + this._terminal.options.scrollback; + } + /** * Fills the buffer's viewport with blank lines. */ @@ -67,7 +79,7 @@ export class Buffer implements IBuffer { this.scrollBottom = 0; this.scrollTop = 0; this.tabs = {}; - this._lines = new CircularList(this._terminal.rows + this._terminal.options.scrollback); + this._lines = new CircularList(this._getCorrectBufferLength(this._terminal.rows)); this.scrollBottom = this._terminal.rows - 1; } @@ -82,6 +94,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? @@ -131,6 +150,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; diff --git a/src/BufferSet.ts b/src/BufferSet.ts index 9136c5fb4..851fc47df 100644 --- a/src/BufferSet.ts +++ b/src/BufferSet.ts @@ -21,9 +21,9 @@ export class BufferSet extends EventEmitter implements IBufferSet { */ constructor(private _terminal: ITerminal) { super(); - this._normal = new Buffer(this._terminal); + this._normal = new Buffer(this._terminal, false); this._normal.fillViewportRows(); - this._alt = new Buffer(this._terminal); + this._alt = new Buffer(this._terminal, true); this._activeBuffer = this._normal; } @@ -71,22 +71,10 @@ 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); } - /** - * Refreshes the max length of the buffer set, this should be called whenever - * terminal rows or scrollback changes. - * @param rows The number of terminal rows. - */ - public refreshMaxLength(rows: number): void { - this._normal.lines.maxLength = rows + this._terminal.options.scrollback; - // TODO: The alt buffer should never have scrollback, see https://github.com/sourcelair/xterm.js/issues/802 - this._alt.lines.maxLength = rows + this._terminal.options.scrollback; - } - /** * Resizes both normal and alt buffers, adjusting their data accordingly. * @param newCols The new number of columns. @@ -95,6 +83,5 @@ export class BufferSet extends EventEmitter implements IBufferSet { public resize(newCols: number, newRows: number): void { this._normal.resize(newCols, newRows); this._alt.resize(newCols, newRows); - this.refreshMaxLength(newRows); } } diff --git a/src/InputHandler.ts b/src/InputHandler.ts index 5362cae9a..4036ac158 100644 --- a/src/InputHandler.ts +++ b/src/InputHandler.ts @@ -442,23 +442,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(); @@ -480,18 +470,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(); diff --git a/src/Terminal.ts b/src/Terminal.ts index 2afb45023..743537b10 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -464,7 +464,7 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT this.element.classList.toggle(`xterm-cursor-style-bar`, value === 'bar'); break; case 'scrollback': - this.buffers.refreshMaxLength(this.rows); + this.buffers.resize(this.cols, this.rows); this.viewport.syncScrollArea(); break; case 'tabStopWidth': this.setupStops(); break; @@ -1177,17 +1177,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; } From e7ee3ec63c005f7073d8daab8dd95eaff2f33c83 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 6 Aug 2017 18:56:37 -0700 Subject: [PATCH 4/5] Fix trimming on a buffer with 0 scrollback Unfortunately this removed the padding idea from CircularList as it would only work when the list is not full. --- src/Buffer.test.ts | 15 +++++++++++++++ src/Buffer.ts | 12 ++++++------ src/utils/CircularList.test.ts | 25 ------------------------- src/utils/CircularList.ts | 28 +++------------------------- 4 files changed, 24 insertions(+), 56 deletions(-) diff --git a/src/Buffer.test.ts b/src/Buffer.test.ts index 2e2ae50c1..d96e6bdef 100644 --- a/src/Buffer.test.ts +++ b/src/Buffer.test.ts @@ -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, false); + 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', () => { diff --git a/src/Buffer.ts b/src/Buffer.ts index 5fe977798..74f440ea6 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -154,12 +154,12 @@ export class Buffer implements IBuffer { // 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); - // } + 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; } diff --git a/src/utils/CircularList.test.ts b/src/utils/CircularList.test.ts index 891ba3c50..f404b6f28 100644 --- a/src/utils/CircularList.test.ts +++ b/src/utils/CircularList.test.ts @@ -9,9 +9,6 @@ class TestCircularList extends CircularList { public get array(): T[] { return this._array; } } -const MAX_LENGTH_PADDING = 40; -const MAX_LENGTH_SHRINK_REBUILD_THRESHOLD = 120; - describe('CircularList', () => { describe('push', () => { it('should push values onto the array', () => { @@ -75,28 +72,6 @@ describe('CircularList', () => { list.maxLength = 4; assert.equal(list.maxLength, 4); }); - - it('should resize the backing array\'s size only when it increases beyond the allocated padding', () => { - const initSize = 200; - const list = new TestCircularList(initSize); - const expectedBackingMaxLength = initSize + MAX_LENGTH_PADDING; - assert.equal(list.array.length, expectedBackingMaxLength); - list.maxLength = expectedBackingMaxLength; - assert.equal(list.array.length, expectedBackingMaxLength); - list.maxLength = expectedBackingMaxLength + 1; - assert.equal(list.array.length, expectedBackingMaxLength + 1 + MAX_LENGTH_PADDING); - }); - - it('should resize the backing array\'s size only when it reduces beyond the shrink threshold', () => { - const initSize = 200; - const list = new TestCircularList(initSize); - const expectedThresholdMaxLength = initSize + MAX_LENGTH_PADDING - MAX_LENGTH_SHRINK_REBUILD_THRESHOLD; - assert.equal(list.array.length, initSize + MAX_LENGTH_PADDING); - list.maxLength = expectedThresholdMaxLength; - assert.equal(list.array.length, initSize + MAX_LENGTH_PADDING); - list.maxLength = expectedThresholdMaxLength - 1; - assert.equal(list.array.length, expectedThresholdMaxLength - 1 + MAX_LENGTH_PADDING); - }); }); describe('length', () => { diff --git a/src/utils/CircularList.ts b/src/utils/CircularList.ts index ed8c065d7..e188fb5d8 100644 --- a/src/utils/CircularList.ts +++ b/src/utils/CircularList.ts @@ -8,15 +8,6 @@ import { EventEmitter } from '../EventEmitter'; import { ICircularList } from '../Interfaces'; -/** - * Padding added to the max length of the CircularList, this allows increasing - * the max length of the list without reconstructing the backing array and then - * copying over the values. - */ -const MAX_LENGTH_PADDING = 40; - -const MAX_LENGTH_SHRINK_REBUILD_THRESHOLD = MAX_LENGTH_PADDING * 3; - export class CircularList extends EventEmitter implements ICircularList { protected _array: T[]; private _startIndex: number; @@ -26,7 +17,7 @@ export class CircularList extends EventEmitter implements ICircularList { private _maxLength: number ) { super(); - this._array = new Array(this._maxLength + MAX_LENGTH_PADDING); + this._array = new Array(this._maxLength); this._startIndex = 0; this._length = 0; } @@ -41,22 +32,9 @@ export class CircularList extends EventEmitter implements ICircularList { return; } - // Ensure the array has padding to expand into. - if (this._array.length >= newMaxLength) { - // Ensure that the array hasn't shrunk significantly, if it has we - // should rebuild the array with a new max length to reclaim some memory. - if (this._array.length - newMaxLength <= MAX_LENGTH_SHRINK_REBUILD_THRESHOLD) { - // Increase the max length and return. - this._maxLength = newMaxLength; - return; - } - } - // Reconstruct array, starting at index 0. Only transfer values from the - // indexes 0 to length. The new array's size includes some padding that - // allows the array's size to increase a little without rebuilding the - // array. - let newArray = new Array(newMaxLength + MAX_LENGTH_PADDING); + // indexes 0 to length. + let newArray = new Array(newMaxLength); for (let i = 0; i < Math.min(newMaxLength, this.length); i++) { newArray[i] = this._array[this._getCyclicIndex(i)]; } From 7b0d0e810c7ff509406afbf532063eb6b35b9261 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 16 Aug 2017 08:45:30 -0700 Subject: [PATCH 5/5] _isAltBuffer -> _hasScrollback --- src/Buffer.test.ts | 8 ++++---- src/Buffer.ts | 11 +++++------ src/BufferSet.ts | 7 +++++-- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/Buffer.test.ts b/src/Buffer.test.ts index 9a3d91681..6baa230dd 100644 --- a/src/Buffer.test.ts +++ b/src/Buffer.test.ts @@ -20,7 +20,7 @@ describe('Buffer', () => { terminal.cols = INIT_COLS; terminal.rows = INIT_ROWS; terminal.options.scrollback = 1000; - buffer = new Buffer(terminal, false); + buffer = new Buffer(terminal, true); }); describe('constructor', () => { @@ -92,7 +92,7 @@ describe('Buffer', () => { 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, false); + buffer = new Buffer(terminal, true); assert.equal(buffer.lines.maxLength, INIT_ROWS); buffer.y = INIT_ROWS - 1; buffer.fillViewportRows(); @@ -172,11 +172,11 @@ describe('Buffer', () => { }); }); - describe('alt 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, true); + buffer = new Buffer(terminal, false); buffer.fillViewportRows(); assert.equal(buffer.lines.maxLength, INIT_ROWS); // Test size on buffer increase diff --git a/src/Buffer.ts b/src/Buffer.ts index bf2ceb713..fd6f8aefa 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -32,11 +32,12 @@ export class Buffer implements IBuffer { /** * Create a new Buffer. * @param _terminal The terminal the Buffer will belong to. - * @param _isAltBuffer Whether the buffer is the alt buffer. + * @param _hasScrollback Whether the buffer should respecr the scrollback of + * the terminal.. */ constructor( private _terminal: ITerminal, - private _isAltBuffer: boolean + private _hasScrollback: boolean ) { this.clear(); } @@ -47,13 +48,11 @@ export class Buffer implements IBuffer { /** * Gets the correct buffer length based on the rows provided, the terminal's - * scrollback and whether this is an alt buffer. + * 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 { - // The alt buffer should never have scrollback. - // See http://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h2-The-Alternate-Screen-Buffer - if (this._isAltBuffer) { + if (!this._hasScrollback) { return rows; } return rows + this._terminal.options.scrollback; diff --git a/src/BufferSet.ts b/src/BufferSet.ts index 851fc47df..d37a650f9 100644 --- a/src/BufferSet.ts +++ b/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, false); + this._normal = new Buffer(this._terminal, true); this._normal.fillViewportRows(); - this._alt = new Buffer(this._terminal, true); + + // 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; }