Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1932 reflow issue in 3.12.1 #2010

Merged
merged 2 commits into from Apr 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/Buffer.test.ts
Expand Up @@ -518,6 +518,29 @@ describe('Buffer', () => {
assert.equal(secondMarker.line, 1, 'second marker should be restored');
assert.equal(thirdMarker.line, 2, 'third marker should be restored');
});
it('should correctly reflow wrapped lines that end in null space (via tab char)', () => {
buffer.fillViewportRows();
buffer.resize(4, 10);
buffer.y = 2;
buffer.lines.get(0).set(0, [null, 'a', 1, 'a'.charCodeAt(0)]);
buffer.lines.get(0).set(1, [null, 'b', 1, 'b'.charCodeAt(0)]);
buffer.lines.get(1).set(0, [null, 'c', 1, 'c'.charCodeAt(0)]);
buffer.lines.get(1).set(1, [null, 'd', 1, 'd'.charCodeAt(0)]);
buffer.lines.get(1).isWrapped = true;
// Buffer:
// "ab " (wrapped)
// "cd"
buffer.resize(5, 10);
assert.equal(buffer.ybase, 0);
assert.equal(buffer.lines.length, 10);
assert.equal(buffer.lines.get(0).translateToString(true), 'ab c');
assert.equal(buffer.lines.get(1).translateToString(false), 'd ');
buffer.resize(6, 10);
assert.equal(buffer.ybase, 0);
assert.equal(buffer.lines.length, 10);
assert.equal(buffer.lines.get(0).translateToString(true), 'ab cd');
assert.equal(buffer.lines.get(1).translateToString(false), ' ');
});
it('should wrap wide characters correctly when reflowing larger', () => {
buffer.fillViewportRows();
buffer.resize(12, 10);
Expand Down Expand Up @@ -553,6 +576,32 @@ describe('Buffer', () => {
assert.equal(buffer.lines.get(1).translateToString(true), '语汉语汉语');
assert.equal(buffer.lines.get(1).translateToString(false), '语汉语汉语 ');
});
it('should correctly reflow wrapped lines that end in null space (via tab char)', () => {
buffer.fillViewportRows();
buffer.resize(4, 10);
buffer.y = 2;
buffer.lines.get(0).set(0, [null, 'a', 1, 'a'.charCodeAt(0)]);
buffer.lines.get(0).set(1, [null, 'b', 1, 'b'.charCodeAt(0)]);
buffer.lines.get(1).set(0, [null, 'c', 1, 'c'.charCodeAt(0)]);
buffer.lines.get(1).set(1, [null, 'd', 1, 'd'.charCodeAt(0)]);
buffer.lines.get(1).isWrapped = true;
// Buffer:
// "ab " (wrapped)
// "cd"
buffer.resize(3, 10);
assert.equal(buffer.y, 2);
assert.equal(buffer.ybase, 0);
assert.equal(buffer.lines.length, 10);
assert.equal(buffer.lines.get(0).translateToString(false), 'ab ');
assert.equal(buffer.lines.get(1).translateToString(false), ' cd');
buffer.resize(2, 10);
assert.equal(buffer.y, 3);
assert.equal(buffer.ybase, 0);
assert.equal(buffer.lines.length, 10);
assert.equal(buffer.lines.get(0).translateToString(false), 'ab');
assert.equal(buffer.lines.get(1).translateToString(false), ' ');
assert.equal(buffer.lines.get(2).translateToString(false), 'cd');
});
it('should wrap wide characters correctly when reflowing smaller', () => {
buffer.fillViewportRows();
buffer.resize(12, 10);
Expand Down
8 changes: 4 additions & 4 deletions src/Buffer.ts
Expand Up @@ -5,7 +5,7 @@

import { IMarker } from 'xterm';
import { BufferLine } from './BufferLine';
import { reflowLargerApplyNewLayout, reflowLargerCreateNewLayout, reflowLargerGetLinesToRemove, reflowSmallerGetNewLineLengths } from './BufferReflow';
import { reflowLargerApplyNewLayout, reflowLargerCreateNewLayout, reflowLargerGetLinesToRemove, reflowSmallerGetNewLineLengths, getWrappedLineTrimmedLength } from './BufferReflow';
import { CircularList, IDeleteEvent, IInsertEvent } from './common/CircularList';
import { EventEmitter } from './common/EventEmitter';
import { DEFAULT_COLOR } from './renderer/atlas/Types';
Expand Down Expand Up @@ -244,7 +244,7 @@ export class Buffer implements IBuffer {
}

private _reflowLarger(newCols: number, newRows: number): void {
const toRemove: number[] = reflowLargerGetLinesToRemove(this.lines, newCols, this.ybase + this.y);
const toRemove: number[] = reflowLargerGetLinesToRemove(this.lines, this._cols, newCols, this.ybase + this.y);
if (toRemove.length > 0) {
const newLayoutResult = reflowLargerCreateNewLayout(this.lines, toRemove);
reflowLargerApplyNewLayout(this.lines, newLayoutResult.layout);
Expand Down Expand Up @@ -348,8 +348,8 @@ export class Buffer implements IBuffer {
srcCol -= cellsToCopy;
if (srcCol === 0) {
srcLineIndex--;
// TODO: srcCol shoudl take trimmed length into account
srcCol = wrappedLines[Math.max(srcLineIndex, 0)].getTrimmedLength(); // this._cols;
const wrappedLinesIndex = Math.max(srcLineIndex, 0);
srcCol = getWrappedLineTrimmedLength(wrappedLines, wrappedLinesIndex, this._cols);
}
}

Expand Down
29 changes: 23 additions & 6 deletions src/BufferReflow.ts
Expand Up @@ -3,7 +3,7 @@
* @license MIT
*/

import { FILL_CHAR_DATA } from './Buffer';
import { FILL_CHAR_DATA, CHAR_DATA_CHAR_INDEX, CHAR_DATA_WIDTH_INDEX } from './Buffer';
import { BufferLine } from './BufferLine';
import { CircularList, IDeleteEvent } from './common/CircularList';
import { IBufferLine } from './Types';
Expand All @@ -19,7 +19,7 @@ export interface INewLayoutResult {
* @param lines The buffer lines.
* @param newCols The columns after resize.
*/
export function reflowLargerGetLinesToRemove(lines: CircularList<IBufferLine>, newCols: number, bufferAbsoluteY: number): number[] {
export function reflowLargerGetLinesToRemove(lines: CircularList<IBufferLine>, oldCols: number, 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[] = [];
Expand Down Expand Up @@ -48,11 +48,11 @@ export function reflowLargerGetLinesToRemove(lines: CircularList<IBufferLine>, n

// Copy buffer data to new locations
let destLineIndex = 0;
let destCol = wrappedLines[destLineIndex].getTrimmedLength();
let destCol = getWrappedLineTrimmedLength(wrappedLines, destLineIndex, oldCols);
let srcLineIndex = 1;
let srcCol = 0;
while (srcLineIndex < wrappedLines.length) {
const srcTrimmedTineLength = wrappedLines[srcLineIndex].getTrimmedLength();
const srcTrimmedTineLength = getWrappedLineTrimmedLength(wrappedLines, srcLineIndex, oldCols);
const srcRemainingCells = srcTrimmedTineLength - srcCol;
const destRemainingCells = newCols - destCol;
const cellsToCopy = Math.min(srcRemainingCells, destRemainingCells);
Expand Down Expand Up @@ -173,7 +173,7 @@ export function reflowLargerApplyNewLayout(lines: CircularList<IBufferLine>, 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);
const cellsNeeded = wrappedLines.map((l, i) => getWrappedLineTrimmedLength(wrappedLines, i, oldCols)).reduce((p, c) => p + c);

// Use srcCol and srcLine to find the new wrapping point, use that to get the cellsAvailable and
// linesNeeded
Expand All @@ -187,7 +187,7 @@ export function reflowSmallerGetNewLineLengths(wrappedLines: BufferLine[], oldCo
break;
}
srcCol += newCols;
const oldTrimmedLength = wrappedLines[srcLine].getTrimmedLength();
const oldTrimmedLength = getWrappedLineTrimmedLength(wrappedLines, srcLine, oldCols);
if (srcCol > oldTrimmedLength) {
srcCol -= oldTrimmedLength;
srcLine++;
Expand All @@ -203,3 +203,20 @@ export function reflowSmallerGetNewLineLengths(wrappedLines: BufferLine[], oldCo

return newLineLengths;
}

export function getWrappedLineTrimmedLength(lines: BufferLine[], i: number, cols: number): number {
// If this is the last row in the wrapped line, get the actual trimmed length
if (i === lines.length - 1) {
return lines[i].getTrimmedLength();
}
// Detect whether the following line starts with a wide character and the end of the current line
// is null, if so then we can be pretty sure the null character should be excluded from the line
// length]
const cell = lines[i].get(cols - 1);
const endsInNull = cell[CHAR_DATA_CHAR_INDEX] === '' && cell[CHAR_DATA_WIDTH_INDEX] === 1;
const followingLineStartsWithWide = lines[i + 1].getWidth(0) === 2;
if (endsInNull && followingLineStartsWithWide) {
return cols - 1;
}
return cols;
}