From d0dc387921604b899700478bacec6614ebcdb96b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Fri, 14 Sep 2018 23:16:24 +0200 Subject: [PATCH 1/6] fix overscan issue and enable linkify of partial shown matches --- src/Buffer.ts | 26 +++++++++++++++++++++++--- src/Linkifier.ts | 31 ++++++++++++++++++------------- src/Types.ts | 2 +- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/src/Buffer.ts b/src/Buffer.ts index 29b034ac..d7760b11 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -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); } } @@ -401,6 +401,18 @@ 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, but 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; @@ -408,7 +420,9 @@ export class BufferStringIterator implements IBufferStringIterator { 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 ) { this._current = this._startIndex; } @@ -419,6 +433,12 @@ export class BufferStringIterator implements IBufferStringIterator { public next(): IBufferStringIteratorResult { const range = this._buffer.getWrappedRangeForLine(this._current); + 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; + } let result = ''; for (let i = range.first; i <= range.last; ++i) { // TODO: always apply trimRight after fixing #1685 diff --git a/src/Linkifier.ts b/src/Linkifier.ts index 4d84b1a7..b4023bbf 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -21,6 +21,12 @@ 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. + */ + protected static readonly OVERSCAN_LIMIT = 5; + protected _linkMatchers: ILinkMatcher[] = []; private _mouseZoneManager: IMouseZoneManager; @@ -88,11 +94,17 @@ export class Linkifier extends EventEmitter implements ILinkifier { return; } - // 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 = this._terminal.buffer.iterator(false, absoluteRowIndexStart, this._terminal.buffer.ydisp + this._rowsToLinkify.end + 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. + // The unwrapping is needed to also match content that got wrapped at the right side. + // To avoid a worst case szenario where the whole buffer contains just a single unwrapped string + // we limit this line expansion beyond the actual viewport to -5 and +5 real buffer lines (overscan). + // This comes with the tradeoff that match longer than 5 buffer lines will not match anymore at the + // viewport borders. + const iterator = this._terminal.buffer.iterator( + false, absoluteRowIndexStart, this._terminal.buffer.ydisp + this._rowsToLinkify.end + 1, + Linkifier.OVERSCAN_LIMIT, Linkifier.OVERSCAN_LIMIT); + console.log('linkify rows', absoluteRowIndexStart, this._terminal.buffer.ydisp + this._rowsToLinkify.end + 1); while (iterator.hasNext()) { const lineData: IBufferStringIteratorResult = iterator.next(); for (let i = 0; i < this._linkMatchers.length; i++) { @@ -204,14 +216,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; @@ -254,6 +258,7 @@ export class Linkifier extends EventEmitter implements ILinkifier { x2 = this._terminal.cols; y2--; } + console.log(x1, y1, x2, y2); this._mouseZoneManager.add(new MouseZone( x1 + 1, diff --git a/src/Types.ts b/src/Types.ts index 69c40151..40b3bd02 100644 --- a/src/Types.ts +++ b/src/Types.ts @@ -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 { From 842ee6c1644d7064bf73c3c2a9cd79b67bbb7753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sat, 15 Sep 2018 00:22:00 +0200 Subject: [PATCH 2/6] change overscan limit to 2k chars --- src/Linkifier.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/Linkifier.ts b/src/Linkifier.ts index b4023bbf..fee5401e 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -23,9 +23,9 @@ export class Linkifier extends EventEmitter implements ILinkifier { /** * Limit of the unwrapping line expansion (overscan) at the top and bottom - * of the actual viewport. + * of the actual viewport in ASCII characters. */ - protected static readonly OVERSCAN_LIMIT = 5; + protected static readonly OVERSCAN_CHAR_LIMIT = 2000; protected _linkMatchers: ILinkMatcher[] = []; @@ -96,15 +96,16 @@ export class Linkifier extends EventEmitter implements ILinkifier { // 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 at the right side. + // 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 contains just a single unwrapped string - // we limit this line expansion beyond the actual viewport to -5 and +5 real buffer lines (overscan). - // This comes with the tradeoff that match longer than 5 buffer lines will not match anymore at the - // viewport borders. + // we limit this line expansion beyond the actual viewport to +OVERSCAN_CHAR_LIMIT chars (overscan) + // at the top and the 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, this._terminal.buffer.ydisp + this._rowsToLinkify.end + 1, - Linkifier.OVERSCAN_LIMIT, Linkifier.OVERSCAN_LIMIT); - console.log('linkify rows', absoluteRowIndexStart, this._terminal.buffer.ydisp + this._rowsToLinkify.end + 1); + overscanLineLimit, overscanLineLimit); while (iterator.hasNext()) { const lineData: IBufferStringIteratorResult = iterator.next(); for (let i = 0; i < this._linkMatchers.length; i++) { From 5284a5a742b3b6ccc62e89474d51027dbafe81dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sat, 15 Sep 2018 00:41:55 +0200 Subject: [PATCH 3/6] fix spelling --- src/Buffer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Buffer.ts b/src/Buffer.ts index d7760b11..815dc84b 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -410,7 +410,7 @@ export class Marker extends EventEmitter implements IMarker { * 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, but not recommended + * 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 { From 10687b138d04c77013ec24cd71b5b885a0e2f9d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Fri, 21 Sep 2018 20:23:41 +0200 Subject: [PATCH 4/6] range checks for buffer iterator --- src/Buffer.test.ts | 33 ++++++++++++++++++++++++++++++++- src/Buffer.ts | 10 ++++++++++ src/Linkifier.ts | 1 - 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/Buffer.test.ts b/src/Buffer.test.ts index 32492ee0..0dcdd02d 100644 --- a/src/Buffer.test.ts +++ b/src/Buffer.test.ts @@ -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'; @@ -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(() => { + 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(); + }); + }); }); diff --git a/src/Buffer.ts b/src/Buffer.ts index 815dc84b..be5a47f1 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -424,6 +424,12 @@ export class BufferStringIterator implements IBufferStringIterator { 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; } @@ -433,12 +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 diff --git a/src/Linkifier.ts b/src/Linkifier.ts index 3be89f80..7d39d146 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -262,7 +262,6 @@ export class Linkifier extends EventEmitter implements ILinkifier { x2 = this._terminal.cols; y2--; } - console.log(x1, y1, x2, y2); this._mouseZoneManager.add(new MouseZone( x1 + 1, From a30bb22c7a222656df37c98e17068ba110c4276f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sat, 22 Sep 2018 15:20:31 +0200 Subject: [PATCH 5/6] fix comments to <80 --- src/Linkifier.ts | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/Linkifier.ts b/src/Linkifier.ts index 7d39d146..e5158e73 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -24,6 +24,7 @@ export class Linkifier extends EventEmitter implements ILinkifier { /** * 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; @@ -98,14 +99,16 @@ 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. - // 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 contains just a single unwrapped string - // we limit this line expansion beyond the actual viewport to +OVERSCAN_CHAR_LIMIT chars (overscan) - // at the top and the bottom. - // This comes with the tradeoff that matches longer than OVERSCAN_CHAR_LIMIT chars will not match - // anymore at the viewport borders. + // 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 + // 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); From fefc6c6a344ccc647109ea5dfe544b571ff131de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Wed, 26 Sep 2018 22:07:02 +0200 Subject: [PATCH 6/6] fix typo --- src/Linkifier.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Linkifier.ts b/src/Linkifier.ts index e5158e73..2ec3ed40 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -104,7 +104,7 @@ export class Linkifier extends EventEmitter implements ILinkifier { // _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 + // several buffer lines. To avoid a worst case scenario where the whole buffer // 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