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 overscan issue and enable linkifier on partial shown matches #1693

Merged
merged 9 commits into from Sep 26, 2018
Merged
Show file tree
Hide file tree
Changes from 8 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
33 changes: 32 additions & 1 deletion src/Buffer.test.ts
Expand Up @@ -3,7 +3,7 @@
* @license MIT
*/

import { assert } from 'chai';
import { assert, expect } from 'chai';
import { ITerminal } from './Types';
import { Buffer, DEFAULT_ATTR, CHAR_DATA_CHAR_INDEX } from './Buffer';
import { CircularList } from './common/CircularList';
Expand Down Expand Up @@ -514,4 +514,35 @@ describe('Buffer', () => {
}
});
});
describe('BufferStringIterator', function(): void {
it('iterator does not ovrflow buffer limits', function(): void {
const terminal = new TestTerminal({rows: 5, cols: 10, scrollback: 5});
const data = [
'aaaaaaaaaa',
'aaaaaaaaa\n',
'aaaaaaaaaa',
'aaaaaaaaa\n',
'aaaaaaaaaa',
'aaaaaaaaaa',
'aaaaaaaaaa',
'aaaaaaaaa\n',
'aaaaaaaaaa',
'aaaaaaaaaa'
];
terminal.writeSync(data.join(''));
// brute force test with insane values
expect(() => {
jerch marked this conversation as resolved.
Show resolved Hide resolved
for (let overscan = 0; overscan < 20; ++overscan) {
for (let start = -10; start < 20; ++start) {
for (let end = -10; end < 20; ++end) {
const it = terminal.buffer.iterator(false, start, end, overscan, overscan);
while (it.hasNext()) {
it.next();
}
}
}
}
}).to.not.throw();
});
});
});
36 changes: 33 additions & 3 deletions src/Buffer.ts
Expand Up @@ -371,8 +371,8 @@ export class Buffer implements IBuffer {
this.markers.splice(this.markers.indexOf(marker), 1);
}

public iterator(trimRight: boolean, startIndex?: number, endIndex?: number): IBufferStringIterator {
return new BufferStringIterator(this, trimRight, startIndex, endIndex);
public iterator(trimRight: boolean, startIndex?: number, endIndex?: number, startOverscan?: number, endOverscan?: number): IBufferStringIterator {
return new BufferStringIterator(this, trimRight, startIndex, endIndex, startOverscan, endOverscan);
}
}

Expand Down Expand Up @@ -401,15 +401,35 @@ export class Marker extends EventEmitter implements IMarker {
}
}

/**
* Iterator to get unwrapped content strings from the buffer.
* The iterator returns at least the string data between the borders
* `startIndex` and `endIndex` (exclusive) and will expand the lines
* by `startOverscan` to the top and by `endOverscan` to the bottom,
* if no new line was found in between.
* It will never read/return string data beyond `startIndex - startOverscan`
* or `endIndex + endOverscan`. Therefore the first and last line might be truncated.
* It is possible to always get the full string for the first and last line as well
* by setting the overscan values to the actual buffer length. This not recommended
* since it might return the whole buffer within a single string in a worst case scenario.
*/
export class BufferStringIterator implements IBufferStringIterator {
private _current: number;

constructor (
private _buffer: IBuffer,
private _trimRight: boolean,
private _startIndex: number = 0,
private _endIndex: number = _buffer.lines.length
private _endIndex: number = _buffer.lines.length,
private _startOverscan: number = 0,
private _endOverscan: number = 0
) {
if (this._startIndex < 0) {
this._startIndex = 0;
}
if (this._endIndex > this._buffer.lines.length) {
this._endIndex = this._buffer.lines.length;
}
this._current = this._startIndex;
}

Expand All @@ -419,6 +439,16 @@ export class BufferStringIterator implements IBufferStringIterator {

public next(): IBufferStringIteratorResult {
const range = this._buffer.getWrappedRangeForLine(this._current);
// limit search window to overscan value at both borders
if (range.first < this._startIndex - this._startOverscan) {
range.first = this._startIndex - this._startOverscan;
}
if (range.last > this._endIndex + this._endOverscan) {
range.last = this._endIndex + this._endOverscan;
}
// limit to current buffer length
range.first = Math.max(range.first, 0);
range.last = Math.min(range.last, this._buffer.lines.length);
let result = '';
for (let i = range.first; i <= range.last; ++i) {
// TODO: always apply trimRight after fixing #1685
Expand Down
33 changes: 20 additions & 13 deletions src/Linkifier.ts
Expand Up @@ -21,6 +21,13 @@ export class Linkifier extends EventEmitter implements ILinkifier {
*/
protected static readonly TIME_BEFORE_LINKIFY = 200;

/**
* Limit of the unwrapping line expansion (overscan) at the top and bottom
* of the actual viewport in ASCII characters.
* A limit of 2000 should match most sane urls.
*/
protected static readonly OVERSCAN_CHAR_LIMIT = 2000;

protected _linkMatchers: ILinkMatcher[] = [];

private _mouseZoneManager: IMouseZoneManager;
Expand Down Expand Up @@ -92,11 +99,19 @@ export class Linkifier extends EventEmitter implements ILinkifier {
// Invalidate bad end row values (if a resize happened)
const absoluteRowIndexEnd = buffer.ydisp + Math.min(this._rowsToLinkify.end, this._terminal.rows) + 1;

// iterate over the range of unwrapped content strings within start..end (excluding)
// _doLinkifyRow gets full unwrapped lines with the start row as buffer offset for every matcher
// for wrapped content over several rows the iterator might return rows outside the viewport
// we skip those later in _doLinkifyRow
const iterator = buffer.iterator(false, absoluteRowIndexStart, absoluteRowIndexEnd);
// Iterate over the range of unwrapped content strings within start..end
// (excluding).
// _doLinkifyRow gets full unwrapped lines with the start row as buffer offset
// for every matcher.
// The unwrapping is needed to also match content that got wrapped across
// several buffer lines. To avoid a worst case szenario where the whole buffer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sz -> sc

// contains just a single unwrapped string we limit this line expansion beyond
// the viewport to +OVERSCAN_CHAR_LIMIT chars (overscan) at top and bottom.
// This comes with the tradeoff that matches longer than OVERSCAN_CHAR_LIMIT
// chars will not match anymore at the viewport borders.
const overscanLineLimit = Math.ceil(Linkifier.OVERSCAN_CHAR_LIMIT / this._terminal.cols);
const iterator = this._terminal.buffer.iterator(
false, absoluteRowIndexStart, absoluteRowIndexEnd, overscanLineLimit, overscanLineLimit);
while (iterator.hasNext()) {
const lineData: IBufferStringIteratorResult = iterator.next();
for (let i = 0; i < this._linkMatchers.length; i++) {
Expand Down Expand Up @@ -208,14 +223,6 @@ export class Linkifier extends EventEmitter implements ILinkifier {
// get the buffer index as [absolute row, col] for the match
const bufferIndex = this._terminal.buffer.stringIndexToBufferIndex(rowIndex, stringIndex);

// skip rows outside of the viewport
if (bufferIndex[0] - this._terminal.buffer.ydisp < 0) {
continue;
}
if (bufferIndex[0] - this._terminal.buffer.ydisp > this._terminal.rows) {
break;
}

const line = this._terminal.buffer.lines.get(bufferIndex[0]);
const char = line.get(bufferIndex[1]);
let fg: number | undefined;
Expand Down
2 changes: 1 addition & 1 deletion src/Types.ts
Expand Up @@ -296,7 +296,7 @@ export interface IBuffer {
nextStop(x?: number): number;
prevStop(x?: number): number;
stringIndexToBufferIndex(lineIndex: number, stringIndex: number): number[];
iterator(trimRight: boolean, startIndex?: number, endIndex?: number): IBufferStringIterator;
iterator(trimRight: boolean, startIndex?: number, endIndex?: number, startOverscan?: number, endOverscan?: number): IBufferStringIterator;
}

export interface IBufferSet extends IEventEmitter {
Expand Down