From 33e46682b1a8cbb7bc0946749adee2b1b1113ebd Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 30 Jan 2019 15:18:28 -0800 Subject: [PATCH 1/2] Fix various problems with reflow - No longer reflow lines where the full unwrapped line contains the cursor - Add guards to prevent y and ybase becoming invalid values Part of Microsoft/vscode#67364 Fixes #1910 --- src/Buffer.ts | 44 +++++++++++++++++++++++++++++--------------- src/BufferReflow.ts | 14 ++++++++++---- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/src/Buffer.ts b/src/Buffer.ts index 52d9572d..bc2d5f20 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -3,13 +3,13 @@ * @license MIT */ -import { CircularList, IInsertEvent, IDeleteEvent } from './common/CircularList'; -import { CharData, ITerminal, IBuffer, IBufferLine, BufferIndex, IBufferStringIterator, IBufferStringIteratorResult } from './Types'; -import { EventEmitter } from './common/EventEmitter'; import { IMarker } from 'xterm'; import { BufferLine } from './BufferLine'; +import { reflowLargerApplyNewLayout, reflowLargerCreateNewLayout, reflowLargerGetLinesToRemove, reflowSmallerGetNewLineLengths } from './BufferReflow'; +import { CircularList, IDeleteEvent, IInsertEvent } from './common/CircularList'; +import { EventEmitter } from './common/EventEmitter'; import { DEFAULT_COLOR } from './renderer/atlas/Types'; -import { reflowSmallerGetNewLineLengths, reflowLargerGetLinesToRemove, reflowLargerCreateNewLayout, reflowLargerApplyNewLayout } from './BufferReflow'; +import { BufferIndex, CharData, IBuffer, IBufferLine, IBufferStringIterator, IBufferStringIteratorResult, ITerminal } from './Types'; export const DEFAULT_ATTR = (0 << 18) | (DEFAULT_COLOR << 9) | (256 << 0); export const CHAR_DATA_ATTR_INDEX = 0; @@ -212,7 +212,7 @@ export class Buffer implements IBuffer { this.scrollBottom = newRows - 1; if (this._hasScrollback) { - this._reflow(newCols); + this._reflow(newCols, newRows); // Trim the end of the line off if cols shrunk if (this._cols > newCols) { @@ -226,7 +226,7 @@ export class Buffer implements IBuffer { this._rows = newRows; } - private _reflow(newCols: number): void { + private _reflow(newCols: number, newRows: number): void { if (this._cols === newCols) { return; } @@ -235,12 +235,12 @@ export class Buffer implements IBuffer { if (newCols > this._cols) { this._reflowLarger(newCols); } else { - this._reflowSmaller(newCols); + this._reflowSmaller(newCols, newRows); } } private _reflowLarger(newCols: number): void { - const toRemove: number[] = reflowLargerGetLinesToRemove(this.lines, newCols); + const toRemove: number[] = reflowLargerGetLinesToRemove(this.lines, newCols, this.ybase + this.y); if (toRemove.length > 0) { const newLayoutResult = reflowLargerCreateNewLayout(this.lines, toRemove); reflowLargerApplyNewLayout(this.lines, newLayoutResult.layout); @@ -253,9 +253,13 @@ export class Buffer implements IBuffer { let viewportAdjustments = countRemoved; while (viewportAdjustments-- > 0) { if (this.ybase === 0) { - this.y--; - // Add an extra row at the bottom of the viewport - this.lines.push(new BufferLine(newCols, FILL_CHAR_DATA)); + if (this.y > 0) { + this.y--; + } + if (this.lines.length < this._rows) { + // Add an extra row at the bottom of the viewport + this.lines.push(new BufferLine(newCols, FILL_CHAR_DATA)); + } } else { if (this.ydisp === this.ybase) { this.ydisp--; @@ -265,7 +269,7 @@ export class Buffer implements IBuffer { } } - private _reflowSmaller(newCols: number): void { + private _reflowSmaller(newCols: number, newRows: number): void { // Gather all BufferLines that need to be inserted into the Buffer here so that they can be // batched up and only committed once const toInsert = []; @@ -285,6 +289,13 @@ export class Buffer implements IBuffer { wrappedLines.unshift(nextLine); } + // If these lines contain the cursor don't touch them, the program will handle fixing up + // wrapped lines with the cursor + const absoluteY = this.ybase + this.y; + if (absoluteY >= y && absoluteY < y + wrappedLines.length) { + continue; + } + const lastLineLength = wrappedLines[wrappedLines.length - 1].getTrimmedLength(); const destLineLengths = reflowSmallerGetNewLineLengths(wrappedLines, this._cols, newCols); const linesToAdd = destLineLengths.length - wrappedLines.length; @@ -357,10 +368,13 @@ export class Buffer implements IBuffer { this.ydisp++; } } else { - if (this.ybase === this.ydisp) { - this.ydisp++; + // Ensure ybase does not exceed its maximum value + if (this.ybase < Math.min(this.lines.maxLength, this.lines.length + countToInsert) - newRows) { + if (this.ybase === this.ydisp) { + this.ydisp++; + } + this.ybase++; } - this.ybase++; } } } diff --git a/src/BufferReflow.ts b/src/BufferReflow.ts index 59934e46..24ab69e6 100644 --- a/src/BufferReflow.ts +++ b/src/BufferReflow.ts @@ -3,10 +3,10 @@ * @license MIT */ +import { FILL_CHAR_DATA } from './Buffer'; import { BufferLine } from './BufferLine'; import { CircularList, IDeleteEvent } from './common/CircularList'; import { IBufferLine } from './Types'; -import { FILL_CHAR_DATA } from './Buffer'; export interface INewLayoutResult { layout: number[]; @@ -19,7 +19,7 @@ export interface INewLayoutResult { * @param lines The buffer lines. * @param newCols The columns after resize. */ -export function reflowLargerGetLinesToRemove(lines: CircularList, newCols: number): number[] { +export function reflowLargerGetLinesToRemove(lines: CircularList, newCols: number, bufferAbsoluteY: number): number[] { // Gather all BufferLines that need to be removed from the Buffer here so that they can be // batched up and only committed once const toRemove: number[] = []; @@ -39,6 +39,13 @@ export function reflowLargerGetLinesToRemove(lines: CircularList, n nextLine = lines.get(++i) as BufferLine; } + // If these lines contain the cursor don't touch them, the program will handle fixing up wrapped + // lines with the cursor + if (bufferAbsoluteY >= y && bufferAbsoluteY < i) { + y += wrappedLines.length - 1; + continue; + } + // Copy buffer data to new locations let destLineIndex = 0; let destCol = wrappedLines[destLineIndex].getTrimmedLength(); @@ -64,7 +71,7 @@ export function reflowLargerGetLinesToRemove(lines: CircularList, n } // Make sure the last cell isn't wide, if it is copy it to the current dest - if (destCol === 0) { + if (destCol === 0 && destLineIndex !== 0) { if (wrappedLines[destLineIndex - 1].getWidth(newCols - 1) === 2) { wrappedLines[destLineIndex].copyCellsFrom(wrappedLines[destLineIndex - 1], newCols - 1, destCol++, 1, false); // Null out the end of the last row @@ -166,7 +173,6 @@ export function reflowLargerApplyNewLayout(lines: CircularList, new */ export function reflowSmallerGetNewLineLengths(wrappedLines: BufferLine[], oldCols: number, newCols: number): number[] { const newLineLengths: number[] = []; - const cellsNeeded = wrappedLines.map(l => l.getTrimmedLength()).reduce((p, c) => p + c); // Use srcCol and srcLine to find the new wrapping point, use that to get the cellsAvailable and From db31a44da3da0100ae26d0fb6a20eeecf61881f6 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 30 Jan 2019 22:14:47 -0800 Subject: [PATCH 2/2] Fix tests --- src/Buffer.test.ts | 66 +++++++++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 27 deletions(-) diff --git a/src/Buffer.test.ts b/src/Buffer.test.ts index a56fd63a..2a244472 100644 --- a/src/Buffer.test.ts +++ b/src/Buffer.test.ts @@ -265,6 +265,7 @@ describe('Buffer', () => { const char = String.fromCharCode(code); firstLine.set(i, [null, char, 1, code]); } + buffer.y = 1; assert.equal(buffer.lines.get(0).length, 5); assert.equal(buffer.lines.get(0).translateToString(), 'abcde'); buffer.resize(1, 10); @@ -296,7 +297,7 @@ describe('Buffer', () => { buffer.fillViewportRows(); terminal.options.scrollback = 1; buffer.resize(10, 5); - const lastLine = buffer.lines.get(4); + const lastLine = buffer.lines.get(3); for (let i = 0; i < 10; i++) { const code = 'a'.charCodeAt(0) + i; const char = String.fromCharCode(code); @@ -308,27 +309,27 @@ describe('Buffer', () => { assert.equal(buffer.y, 4); assert.equal(buffer.ybase, 1); assert.equal(buffer.lines.length, 6); - assert.equal(buffer.lines.get(0).translateToString(), ' '); - assert.equal(buffer.lines.get(1).translateToString(), 'ab'); - assert.equal(buffer.lines.get(2).translateToString(), 'cd'); - assert.equal(buffer.lines.get(3).translateToString(), 'ef'); - assert.equal(buffer.lines.get(4).translateToString(), 'gh'); - assert.equal(buffer.lines.get(5).translateToString(), 'ij'); + assert.equal(buffer.lines.get(0).translateToString(), 'ab'); + assert.equal(buffer.lines.get(1).translateToString(), 'cd'); + assert.equal(buffer.lines.get(2).translateToString(), 'ef'); + assert.equal(buffer.lines.get(3).translateToString(), 'gh'); + assert.equal(buffer.lines.get(4).translateToString(), 'ij'); + assert.equal(buffer.lines.get(5).translateToString(), ' '); buffer.resize(1, 5); assert.equal(buffer.y, 4); assert.equal(buffer.ybase, 1); assert.equal(buffer.lines.length, 6); - assert.equal(buffer.lines.get(0).translateToString(), 'e'); - assert.equal(buffer.lines.get(1).translateToString(), 'f'); - assert.equal(buffer.lines.get(2).translateToString(), 'g'); - assert.equal(buffer.lines.get(3).translateToString(), 'h'); - assert.equal(buffer.lines.get(4).translateToString(), 'i'); - assert.equal(buffer.lines.get(5).translateToString(), 'j'); + assert.equal(buffer.lines.get(0).translateToString(), 'f'); + assert.equal(buffer.lines.get(1).translateToString(), 'g'); + assert.equal(buffer.lines.get(2).translateToString(), 'h'); + assert.equal(buffer.lines.get(3).translateToString(), 'i'); + assert.equal(buffer.lines.get(4).translateToString(), 'j'); + assert.equal(buffer.lines.get(5).translateToString(), ' '); buffer.resize(10, 5); - assert.equal(buffer.y, 0); + assert.equal(buffer.y, 1); assert.equal(buffer.ybase, 0); assert.equal(buffer.lines.length, 5); - assert.equal(buffer.lines.get(0).translateToString(), 'efghij '); + assert.equal(buffer.lines.get(0).translateToString(), 'fghij '); assert.equal(buffer.lines.get(1).translateToString(), ' '); assert.equal(buffer.lines.get(2).translateToString(), ' '); assert.equal(buffer.lines.get(3).translateToString(), ' '); @@ -339,6 +340,7 @@ describe('Buffer', () => { // 3+ lines removed on a reflow actually remove the right lines buffer.fillViewportRows(); buffer.resize(10, 10); + buffer.y = 2; const firstLine = buffer.lines.get(0); const secondLine = buffer.lines.get(1); for (let i = 0; i < 10; i++) { @@ -358,8 +360,8 @@ describe('Buffer', () => { assert.equal(buffer.lines.get(i).translateToString(), ' '); } buffer.resize(2, 10); - assert.equal(buffer.ybase, 0); - assert.equal(buffer.lines.length, 10); + assert.equal(buffer.ybase, 1); + assert.equal(buffer.lines.length, 11); assert.equal(buffer.lines.get(0).translateToString(), 'ab'); assert.equal(buffer.lines.get(1).translateToString(), 'cd'); assert.equal(buffer.lines.get(2).translateToString(), 'ef'); @@ -370,7 +372,10 @@ describe('Buffer', () => { assert.equal(buffer.lines.get(7).translateToString(), '45'); assert.equal(buffer.lines.get(8).translateToString(), '67'); assert.equal(buffer.lines.get(9).translateToString(), '89'); + assert.equal(buffer.lines.get(10).translateToString(), ' '); buffer.resize(10, 10); + assert.equal(buffer.ybase, 0); + assert.equal(buffer.lines.length, 10); assert.equal(buffer.lines.get(0).translateToString(), 'abcdefghij'); assert.equal(buffer.lines.get(1).translateToString(), '0123456789'); for (let i = 2; i < 10; i++) { @@ -379,22 +384,23 @@ describe('Buffer', () => { }); it('should transfer combined char data over to reflowed lines', () => { buffer.fillViewportRows(); - buffer.resize(4, 2); + buffer.resize(4, 3); + buffer.y = 2; const firstLine = buffer.lines.get(0); firstLine.set(0, [ null, 'a', 1, 'a'.charCodeAt(0) ]); firstLine.set(1, [ null, 'b', 1, 'b'.charCodeAt(0) ]); firstLine.set(2, [ null, 'c', 1, 'c'.charCodeAt(0) ]); firstLine.set(3, [ null, '😁', 1, '😁'.charCodeAt(0) ]); - assert.equal(buffer.lines.length, 2); + assert.equal(buffer.lines.length, 3); assert.equal(buffer.lines.get(0).translateToString(), 'abc😁'); assert.equal(buffer.lines.get(1).translateToString(), ' '); - buffer.resize(2, 2); + buffer.resize(2, 3); assert.equal(buffer.lines.get(0).translateToString(), 'ab'); assert.equal(buffer.lines.get(1).translateToString(), 'c😁'); }); it('should adjust markers when reflowing', () => { buffer.fillViewportRows(); - buffer.resize(10, 15); + buffer.resize(10, 16); for (let i = 0; i < 10; i++) { const code = 'a'.charCodeAt(0) + i; const char = String.fromCharCode(code); @@ -410,6 +416,7 @@ describe('Buffer', () => { const char = String.fromCharCode(code); buffer.lines.get(2).set(i, [null, char, 1, code]); } + buffer.y = 3; // Buffer: // abcdefghij // 0123456789 @@ -423,7 +430,7 @@ describe('Buffer', () => { assert.equal(firstMarker.line, 0); assert.equal(secondMarker.line, 1); assert.equal(thirdMarker.line, 2); - buffer.resize(2, 15); + buffer.resize(2, 16); assert.equal(buffer.lines.get(0).translateToString(), 'ab'); assert.equal(buffer.lines.get(1).translateToString(), 'cd'); assert.equal(buffer.lines.get(2).translateToString(), 'ef'); @@ -442,7 +449,7 @@ describe('Buffer', () => { assert.equal(firstMarker.line, 0, 'first marker should remain unchanged'); assert.equal(secondMarker.line, 5, 'second marker should be shifted since the first line wrapped'); assert.equal(thirdMarker.line, 10, 'third marker should be shifted since the first and second lines wrapped'); - buffer.resize(10, 15); + buffer.resize(10, 16); assert.equal(buffer.lines.get(0).translateToString(), 'abcdefghij'); assert.equal(buffer.lines.get(1).translateToString(), '0123456789'); assert.equal(buffer.lines.get(2).translateToString(), 'klmnopqrst'); @@ -456,7 +463,7 @@ describe('Buffer', () => { it('should dispose markers whose rows are trimmed during a reflow', () => { buffer.fillViewportRows(); terminal.options.scrollback = 1; - buffer.resize(10, 10); + buffer.resize(10, 11); for (let i = 0; i < 10; i++) { const code = 'a'.charCodeAt(0) + i; const char = String.fromCharCode(code); @@ -472,6 +479,7 @@ describe('Buffer', () => { const char = String.fromCharCode(code); buffer.lines.get(2).set(i, [null, char, 1, code]); } + buffer.y = 10; // Buffer: // abcdefghij // 0123456789 @@ -479,14 +487,14 @@ describe('Buffer', () => { const firstMarker = buffer.addMarker(0); const secondMarker = buffer.addMarker(1); const thirdMarker = buffer.addMarker(2); - buffer.y = 2; + buffer.y = 3; assert.equal(buffer.lines.get(0).translateToString(), 'abcdefghij'); assert.equal(buffer.lines.get(1).translateToString(), '0123456789'); assert.equal(buffer.lines.get(2).translateToString(), 'klmnopqrst'); assert.equal(firstMarker.line, 0); assert.equal(secondMarker.line, 1); assert.equal(thirdMarker.line, 2); - buffer.resize(2, 10); + buffer.resize(2, 11); assert.equal(buffer.lines.get(0).translateToString(), 'ij'); assert.equal(buffer.lines.get(1).translateToString(), '01'); assert.equal(buffer.lines.get(2).translateToString(), '23'); @@ -503,7 +511,7 @@ describe('Buffer', () => { assert.equal(firstMarker.isDisposed, true, 'first marker was trimmed'); assert.equal(secondMarker.isDisposed, false); assert.equal(thirdMarker.isDisposed, false); - buffer.resize(10, 10); + buffer.resize(10, 11); assert.equal(buffer.lines.get(0).translateToString(), 'ij '); assert.equal(buffer.lines.get(1).translateToString(), '0123456789'); assert.equal(buffer.lines.get(2).translateToString(), 'klmnopqrst'); @@ -513,6 +521,7 @@ describe('Buffer', () => { it('should wrap wide characters correctly when reflowing larger', () => { buffer.fillViewportRows(); buffer.resize(12, 10); + buffer.y = 2; for (let i = 0; i < 12; i += 4) { buffer.lines.get(0).set(i, [null, '汉', 2, '汉'.charCodeAt(0)]); buffer.lines.get(1).set(i, [null, '汉', 2, '汉'.charCodeAt(0)]); @@ -547,6 +556,7 @@ describe('Buffer', () => { it('should wrap wide characters correctly when reflowing smaller', () => { buffer.fillViewportRows(); buffer.resize(12, 10); + buffer.y = 2; for (let i = 0; i < 12; i += 4) { buffer.lines.get(0).set(i, [null, '汉', 2, '汉'.charCodeAt(0)]); buffer.lines.get(1).set(i, [null, '汉', 2, '汉'.charCodeAt(0)]); @@ -937,6 +947,7 @@ describe('Buffer', () => { describe('&& ydisp === ybase', () => { it('should trim lines and keep ydisp = ybase', () => { buffer.ydisp = 10; + buffer.y = 13; buffer.resize(2, 10); assert.equal(buffer.ydisp, 10); assert.equal(buffer.ybase, 10); @@ -962,6 +973,7 @@ describe('Buffer', () => { describe('&& ydisp !== ybase', () => { it('should trim lines and not change ydisp', () => { buffer.ydisp = 5; + buffer.y = 13; buffer.resize(2, 10); assert.equal(buffer.ydisp, 5); assert.equal(buffer.ybase, 10);