From 508589aa801644ada3654c3fa3edb45f5cd45869 Mon Sep 17 00:00:00 2001 From: Jean Pierre Date: Sat, 6 May 2023 16:37:35 -0500 Subject: [PATCH 1/9] Improve search addon behavior when there are > 1000 results --- addons/xterm-addon-search/src/SearchAddon.ts | 156 ++++++++---------- .../typings/xterm-addon-search.d.ts | 11 +- 2 files changed, 79 insertions(+), 88 deletions(-) diff --git a/addons/xterm-addon-search/src/SearchAddon.ts b/addons/xterm-addon-search/src/SearchAddon.ts index 3f71af29..7f11895c 100644 --- a/addons/xterm-addon-search/src/SearchAddon.ts +++ b/addons/xterm-addon-search/src/SearchAddon.ts @@ -5,7 +5,7 @@ import { Terminal, IDisposable, ITerminalAddon, IBufferRange, IDecoration } from 'xterm'; import { EventEmitter } from 'common/EventEmitter'; -import { Disposable, toDisposable } from 'common/Lifecycle'; +import { Disposable, toDisposable, disposeArray } from 'common/Lifecycle'; export interface ISearchOptions { regex?: boolean; @@ -54,9 +54,9 @@ const LINES_CACHE_TIME_TO_LIVE = 15 * 1000; // 15 secs export class SearchAddon extends Disposable implements ITerminalAddon { private _terminal: Terminal | undefined; private _cachedSearchTerm: string | undefined; - private _selectedDecoration: IDecoration | undefined; - private _resultDecorations: Map | undefined; - private _searchResults: Map | undefined; + private _highlightedLines: Set = new Set(); + private _highlightDecorations: (IDecoration & { match: ISearchResult })[] = []; + private _selectedDecoration: IDecoration & { match: ISearchResult } | undefined; private _onDataDisposable: IDisposable | undefined; private _onResizeDisposable: IDisposable | undefined; private _lastSearchOptions: ISearchOptions | undefined; @@ -71,11 +71,11 @@ export class SearchAddon extends Disposable implements ITerminalAddon { private _cursorMoveListener: IDisposable | undefined; private _resizeListener: IDisposable | undefined; - private _resultIndex: number | undefined; - - private readonly _onDidChangeResults = this.register(new EventEmitter<{ resultIndex: number, resultCount: number } | undefined>()); + private readonly _onDidChangeResults = this.register(new EventEmitter<{ resultIndex: number, resultCount: number }>()); public readonly onDidChangeResults = this._onDidChangeResults.event; + public readonly MATCHES_LIMIT = 1000; + public activate(terminal: Terminal): void { this._terminal = terminal; this._onDataDisposable = this.register(this._terminal.onWriteParsed(() => this._updateMatches())); @@ -94,23 +94,16 @@ export class SearchAddon extends Disposable implements ITerminalAddon { if (this._cachedSearchTerm && this._lastSearchOptions?.decorations) { this._highlightTimeout = setTimeout(() => { this.findPrevious(this._cachedSearchTerm!, { ...this._lastSearchOptions, incremental: true, noScroll: true }); - this._resultIndex = this._searchResults ? this._searchResults.size - 1 : -1; - this._onDidChangeResults.fire({ resultIndex: this._resultIndex, resultCount: this._searchResults?.size ?? -1 }); + this._fireResults(this._lastSearchOptions); }, 200); } } public clearDecorations(retainCachedSearchTerm?: boolean): void { this._selectedDecoration?.dispose(); - this._searchResults?.clear(); - this._resultDecorations?.forEach(decorations => { - for (const d of decorations) { - d.dispose(); - } - }); - this._resultDecorations?.clear(); - this._searchResults = undefined; - this._resultDecorations = undefined; + disposeArray(this._highlightDecorations); + this._highlightDecorations = []; + this._highlightedLines.clear(); if (!retainCachedSearchTerm) { this._cachedSearchTerm = undefined; } @@ -134,11 +127,16 @@ export class SearchAddon extends Disposable implements ITerminalAddon { } this._lastSearchOptions = searchOptions; if (searchOptions?.decorations) { - if (this._resultIndex !== undefined || this._cachedSearchTerm === undefined || term !== this._cachedSearchTerm) { + if (this._cachedSearchTerm === undefined || term !== this._cachedSearchTerm) { this._highlightAllMatches(term, searchOptions); } } - return this._fireResults(term, this._findNextAndSelect(term, searchOptions), searchOptions); + + const found = this._findNextAndSelect(term, searchOptions); + this._fireResults(searchOptions); + this._cachedSearchTerm = term; + + return found; } private _highlightAllMatches(term: string, searchOptions: ISearchOptions): void { @@ -153,32 +151,31 @@ export class SearchAddon extends Disposable implements ITerminalAddon { // new search, clear out the old decorations this.clearDecorations(true); - this._searchResults = new Map(); - this._resultDecorations = new Map(); - const resultDecorations = this._resultDecorations; + + const searchResultsWithHighlight: ISearchResult[] = []; + let prevResult: ISearchResult | undefined = undefined; let result = this._find(term, 0, 0, searchOptions); - while (result && !this._searchResults.get(`${result.row}-${result.col}`)) { - this._searchResults.set(`${result.row}-${result.col}`, result); + while (result && (prevResult?.row !== result.row || prevResult?.col !== result.col)) { + if (searchResultsWithHighlight.length >= this.MATCHES_LIMIT) { + break; + } + prevResult = result; + searchResultsWithHighlight.push(prevResult); result = this._find( term, - result.col + result.term.length >= this._terminal.cols ? result.row + 1 : result.row, - result.col + result.term.length >= this._terminal.cols ? 0 : result.col + 1, + prevResult.col + prevResult.term.length >= this._terminal.cols ? prevResult.row + 1 : prevResult.row, + prevResult.col + prevResult.term.length >= this._terminal.cols ? 0 : prevResult.col + 1, searchOptions ); - if (this._searchResults.size > 1000) { - this.clearDecorations(); - this._resultIndex = undefined; - return; - } } - this._searchResults.forEach(result => { + for (const result of searchResultsWithHighlight) { const resultDecoration = this._createResultDecoration(result, searchOptions.decorations!); if (resultDecoration) { - const decorationsForLine = resultDecorations.get(resultDecoration.marker.line) || []; - decorationsForLine.push(resultDecoration); - resultDecorations.set(resultDecoration.marker.line, decorationsForLine); + this._highlightedLines.add(resultDecoration.marker.line); + (resultDecoration as unknown as { match: ISearchResult }).match = result; + this._highlightDecorations.push(resultDecoration as (IDecoration & { match: ISearchResult })); } - }); + } } private _find(term: string, startRow: number, startCol: number, searchOptions?: ISearchOptions): ISearchResult | undefined { @@ -224,12 +221,10 @@ export class SearchAddon extends Disposable implements ITerminalAddon { this._terminal?.clearSelection(); this.clearDecorations(); this._cachedSearchTerm = undefined; - this._resultIndex = -1; return false; } if (this._cachedSearchTerm !== term) { - this._resultIndex = undefined; this._terminal.clearSelection(); } @@ -287,18 +282,6 @@ export class SearchAddon extends Disposable implements ITerminalAddon { result = this._findInLine(term, searchPosition, searchOptions); } - if (this._searchResults) { - if (this._searchResults.size === 0) { - this._resultIndex = -1; - } else if (this._resultIndex === undefined) { - this._resultIndex = 0; - } else { - this._resultIndex++; - if (this._resultIndex >= this._searchResults.size) { - this._resultIndex = 0; - } - } - } // Set selection and scroll if a result was found return this._selectResult(result, searchOptions?.decorations, searchOptions?.noScroll); } @@ -315,23 +298,33 @@ export class SearchAddon extends Disposable implements ITerminalAddon { } this._lastSearchOptions = searchOptions; if (searchOptions?.decorations) { - if (this._resultIndex !== undefined || this._cachedSearchTerm === undefined || term !== this._cachedSearchTerm) { + if (this._cachedSearchTerm === undefined || term !== this._cachedSearchTerm) { this._highlightAllMatches(term, searchOptions); } } - return this._fireResults(term, this._findPreviousAndSelect(term, searchOptions), searchOptions); + + const found = this._findPreviousAndSelect(term, searchOptions); + this._fireResults(searchOptions); + this._cachedSearchTerm = term; + + return found; } - private _fireResults(term: string, found: boolean, searchOptions?: ISearchOptions): boolean { + private _fireResults(searchOptions?: ISearchOptions): void { if (searchOptions?.decorations) { - if (this._resultIndex !== undefined && this._searchResults?.size !== undefined) { - this._onDidChangeResults.fire({ resultIndex: this._resultIndex, resultCount: this._searchResults.size }); - } else { - this._onDidChangeResults.fire(undefined); + let resultIndex = -1; + if (this._selectedDecoration) { + const selectedMatch = this._selectedDecoration.match; + for (let i = 0; i < this._highlightDecorations.length; i++) { + const match = this._highlightDecorations[i].match; + if (match.row === selectedMatch.row && match.col === selectedMatch.col && match.size === selectedMatch.size) { + resultIndex = i; + break; + } + } } + this._onDidChangeResults.fire({ resultIndex, resultCount: this._highlightDecorations.length }); } - this._cachedSearchTerm = term; - return found; } private _findPreviousAndSelect(term: string, searchOptions?: ISearchOptions): boolean { @@ -343,16 +336,14 @@ export class SearchAddon extends Disposable implements ITerminalAddon { result = undefined; this._terminal?.clearSelection(); this.clearDecorations(); - this._resultIndex = -1; return false; } if (this._cachedSearchTerm !== term) { - this._resultIndex = undefined; this._terminal.clearSelection(); } - let startRow = this._terminal.buffer.active.baseY + this._terminal.rows; + let startRow = this._terminal.buffer.active.baseY + this._terminal.rows - 1; let startCol = this._terminal.cols; const isReverseSearch = true; @@ -399,8 +390,8 @@ export class SearchAddon extends Disposable implements ITerminalAddon { } } // If we hit the top and didn't search from the very bottom wrap back down - if (!result && startRow !== (this._terminal.buffer.active.baseY + this._terminal.rows)) { - for (let y = (this._terminal.buffer.active.baseY + this._terminal.rows); y >= startRow; y--) { + if (!result && startRow !== (this._terminal.buffer.active.baseY + this._terminal.rows - 1)) { + for (let y = (this._terminal.buffer.active.baseY + this._terminal.rows - 1); y >= startRow; y--) { searchPosition.startRow = y; result = this._findInLine(term, searchPosition, searchOptions, isReverseSearch); if (result) { @@ -409,19 +400,6 @@ export class SearchAddon extends Disposable implements ITerminalAddon { } } - if (this._searchResults) { - if (this._searchResults.size === 0) { - this._resultIndex = -1; - } else if (this._resultIndex === undefined || this._resultIndex < 0) { - this._resultIndex = this._searchResults.size - 1; - } else { - this._resultIndex--; - if (this._resultIndex === -1) { - this._resultIndex = this._searchResults.size - 1; - } - } - } - // If there is only one result, return true. if (!result && currentSelection) return true; @@ -675,7 +653,7 @@ export class SearchAddon extends Disposable implements ITerminalAddon { if (options) { const marker = terminal.registerMarker(-terminal.buffer.active.baseY - terminal.buffer.active.cursorY + result.row); if (marker) { - this._selectedDecoration = terminal.registerDecoration({ + const decoration = terminal.registerDecoration({ marker, x: result.col, width: result.size, @@ -685,8 +663,14 @@ export class SearchAddon extends Disposable implements ITerminalAddon { color: options.activeMatchColorOverviewRuler } }); - this._selectedDecoration?.onRender((e) => this._applyStyles(e, options.activeMatchBorder, true)); - this._selectedDecoration?.onDispose(() => marker.dispose()); + if (decoration) { + const disposables: IDisposable[] = []; + disposables.push(marker); + disposables.push(decoration.onRender((e) => this._applyStyles(e, options.activeMatchBorder, true))); + disposables.push(decoration.onDispose(() => disposeArray(disposables))); + (decoration as unknown as { match: ISearchResult }).match = result; + this._selectedDecoration = decoration as (IDecoration & { match: ISearchResult }); + } } } @@ -740,13 +724,17 @@ export class SearchAddon extends Disposable implements ITerminalAddon { x: result.col, width: result.size, backgroundColor: options.matchBackground, - overviewRulerOptions: this._resultDecorations?.get(marker.line) ? undefined : { + overviewRulerOptions: this._highlightedLines.has(marker.line) ? undefined : { color: options.matchOverviewRuler, position: 'center' } }); - findResultDecoration?.onRender((e) => this._applyStyles(e, options.matchBorder, false)); - findResultDecoration?.onDispose(() => marker.dispose()); + if (findResultDecoration) { + const disposables: IDisposable[] = []; + disposables.push(marker); + disposables.push(findResultDecoration.onRender((e) => this._applyStyles(e, options.matchBorder, false))); + disposables.push(findResultDecoration.onDispose(() => disposeArray(disposables))); + } return findResultDecoration; } } diff --git a/addons/xterm-addon-search/typings/xterm-addon-search.d.ts b/addons/xterm-addon-search/typings/xterm-addon-search.d.ts index ad51262d..05cb8ebb 100644 --- a/addons/xterm-addon-search/typings/xterm-addon-search.d.ts +++ b/addons/xterm-addon-search/typings/xterm-addon-search.d.ts @@ -79,6 +79,11 @@ declare module 'xterm-addon-search' { * An xterm.js addon that provides search functionality. */ export class SearchAddon implements ITerminalAddon { + /** + * Max number of matches when decorations are enabled + */ + public readonly MATCHES_LIMIT: number; + /** * Activates the addon * @param terminal The terminal the addon is being loaded in. @@ -121,10 +126,8 @@ declare module 'xterm-addon-search' { /** * When decorations are enabled, fires when * the search results change. - * @returns -1 for resultIndex for a resultCount of 0 - * and @returns undefined when the threshold of 1k results - * is exceeded and decorations are disposed of. + * @returns -1 for resultIndex when the threshold of matches is exceeded. */ - readonly onDidChangeResults: IEvent<{ resultIndex: number, resultCount: number } | undefined>; + readonly onDidChangeResults: IEvent<{ resultIndex: number, resultCount: number }>; } } From 1c19f1d282616aa35aa1b3e92b2eec2e1b6cbd38 Mon Sep 17 00:00:00 2001 From: Jean Pierre Date: Sun, 7 May 2023 13:28:08 -0500 Subject: [PATCH 2/9] Remove incremental? --- addons/xterm-addon-search/src/SearchAddon.ts | 38 ++++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/addons/xterm-addon-search/src/SearchAddon.ts b/addons/xterm-addon-search/src/SearchAddon.ts index 7f11895c..98a69814 100644 --- a/addons/xterm-addon-search/src/SearchAddon.ts +++ b/addons/xterm-addon-search/src/SearchAddon.ts @@ -232,12 +232,12 @@ export class SearchAddon extends Disposable implements ITerminalAddon { let startRow = 0; let currentSelection: IBufferRange | undefined; if (this._terminal.hasSelection()) { - const incremental = searchOptions ? searchOptions.incremental : false; + // const incremental = searchOptions ? searchOptions.incremental : false; // Start from the selection end if there is a selection // For incremental search, use existing row currentSelection = this._terminal.getSelectionPosition()!; - startRow = incremental ? currentSelection.start.y : currentSelection.end.y; - startCol = incremental ? currentSelection.start.x : currentSelection.end.x; + startRow = currentSelection.end.y; + startCol = currentSelection.end.x; } this._initLinesCache(); @@ -347,7 +347,6 @@ export class SearchAddon extends Disposable implements ITerminalAddon { let startCol = this._terminal.cols; const isReverseSearch = true; - const incremental = searchOptions ? searchOptions.incremental : false; let currentSelection: IBufferRange | undefined; if (this._terminal.hasSelection()) { currentSelection = this._terminal.getSelectionPosition()!; @@ -362,21 +361,22 @@ export class SearchAddon extends Disposable implements ITerminalAddon { startCol }; - if (incremental) { - // Try to expand selection to right first. - result = this._findInLine(term, searchPosition, searchOptions, false); - const isOldResultHighlighted = result && result.row === startRow && result.col === startCol; - if (!isOldResultHighlighted) { - // If selection was not able to be expanded to the right, then try reverse search - if (currentSelection) { - searchPosition.startRow = currentSelection.end.y; - searchPosition.startCol = currentSelection.end.x; - } - result = this._findInLine(term, searchPosition, searchOptions, true); - } - } else { - result = this._findInLine(term, searchPosition, searchOptions, isReverseSearch); - } + // const incremental = searchOptions ? searchOptions.incremental : false; + // if (incremental) { + // // Try to expand selection to right first. + // result = this._findInLine(term, searchPosition, searchOptions, false); + // const isOldResultHighlighted = result && result.row === startRow && result.col === startCol; + // if (!isOldResultHighlighted) { + // // If selection was not able to be expanded to the right, then try reverse search + // if (currentSelection) { + // searchPosition.startRow = currentSelection.end.y; + // searchPosition.startCol = currentSelection.end.x; + // } + // result = this._findInLine(term, searchPosition, searchOptions, true); + // } + // } else { + result = this._findInLine(term, searchPosition, searchOptions, isReverseSearch); + // } // Search from startRow - 1 to top if (!result) { From e4b670231d2c9806a4b356932cff05a31c79b78a Mon Sep 17 00:00:00 2001 From: Jean Pierre Date: Mon, 8 May 2023 09:08:35 -0500 Subject: [PATCH 3/9] Add tests --- .../test/SearchAddon.api.ts | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/addons/xterm-addon-search/test/SearchAddon.api.ts b/addons/xterm-addon-search/test/SearchAddon.api.ts index f17ecd1b..dc3ecef3 100644 --- a/addons/xterm-addon-search/test/SearchAddon.api.ts +++ b/addons/xterm-addon-search/test/SearchAddon.api.ts @@ -210,6 +210,29 @@ describe('Search Tests', function (): void { { resultCount: 0, resultIndex: -1 } ]); }); + it('should fire with more than 1k matches', async () => { + await page.evaluate(` + window.calls = []; + window.search.onDidChangeResults(e => window.calls.push(e)); + `); + const data = ('a bc'.repeat(10) + '\\n\\r').repeat(150); + await writeSync(page, data); + assert.strictEqual(await page.evaluate(`window.search.findNext('a', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 1000, resultIndex: 0 } + ]); + assert.strictEqual(await page.evaluate(`window.search.findNext('a', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 1000, resultIndex: 0 }, + { resultCount: 1000, resultIndex: 1 } + ]); + assert.strictEqual(await page.evaluate(`window.search.findNext('bc', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 1000, resultIndex: 0 }, + { resultCount: 1000, resultIndex: 1 }, + { resultCount: 1000, resultIndex: 0 } + ]); + }); }); describe('findPrevious', () => { it('should not fire unless the decorations option is set', async () => { @@ -294,6 +317,29 @@ describe('Search Tests', function (): void { { resultCount: 0, resultIndex: -1 } ]); }); + it('should fire with more than 1k matches', async () => { + await page.evaluate(` + window.calls = []; + window.search.onDidChangeResults(e => window.calls.push(e)); + `); + const data = ('a bc'.repeat(10) + '\\n\\r').repeat(150); + await writeSync(page, data); + assert.strictEqual(await page.evaluate(`window.search.findPrevious('a', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 1000, resultIndex: -1 } + ]); + assert.strictEqual(await page.evaluate(`window.search.findPrevious('a', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 1000, resultIndex: -1 }, + { resultCount: 1000, resultIndex: -1 } + ]); + assert.strictEqual(await page.evaluate(`window.search.findPrevious('bc', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 1000, resultIndex: -1 }, + { resultCount: 1000, resultIndex: -1 }, + { resultCount: 1000, resultIndex: -1 } + ]); + }); }); }); From b4e2293dbc4e2277b035a08fbe73a0c6e29a916e Mon Sep 17 00:00:00 2001 From: Jean Pierre Date: Tue, 9 May 2023 16:14:28 -0500 Subject: [PATCH 4/9] Fix incremental --- addons/xterm-addon-search/src/SearchAddon.ts | 78 ++++++++----------- .../test/SearchAddon.api.ts | 25 +++++- 2 files changed, 54 insertions(+), 49 deletions(-) diff --git a/addons/xterm-addon-search/src/SearchAddon.ts b/addons/xterm-addon-search/src/SearchAddon.ts index 98a69814..9dd1ffc8 100644 --- a/addons/xterm-addon-search/src/SearchAddon.ts +++ b/addons/xterm-addon-search/src/SearchAddon.ts @@ -220,24 +220,22 @@ export class SearchAddon extends Disposable implements ITerminalAddon { if (!this._terminal || !term || term.length === 0) { this._terminal?.clearSelection(); this.clearDecorations(); - this._cachedSearchTerm = undefined; return false; } - if (this._cachedSearchTerm !== term) { - this._terminal.clearSelection(); - } + const prevSelectedPos = this._terminal.getSelectionPosition(); + this._terminal.clearSelection(); let startCol = 0; let startRow = 0; - let currentSelection: IBufferRange | undefined; - if (this._terminal.hasSelection()) { - // const incremental = searchOptions ? searchOptions.incremental : false; - // Start from the selection end if there is a selection - // For incremental search, use existing row - currentSelection = this._terminal.getSelectionPosition()!; - startRow = currentSelection.end.y; - startCol = currentSelection.end.x; + if (prevSelectedPos) { + if (this._cachedSearchTerm === term) { + startCol = prevSelectedPos.end.x; + startRow = prevSelectedPos.end.y; + } else { + startCol = prevSelectedPos.start.x; + startRow = prevSelectedPos.start.y; + } } this._initLinesCache(); @@ -276,8 +274,8 @@ export class SearchAddon extends Disposable implements ITerminalAddon { } // If there is only one result, wrap back and return selection if it exists. - if (!result && currentSelection) { - searchPosition.startRow = currentSelection.start.y; + if (!result && prevSelectedPos) { + searchPosition.startRow = prevSelectedPos.start.y; searchPosition.startCol = 0; result = this._findInLine(term, searchPosition, searchOptions); } @@ -331,52 +329,43 @@ export class SearchAddon extends Disposable implements ITerminalAddon { if (!this._terminal) { throw new Error('Cannot use addon until it has been loaded'); } - let result: ISearchResult | undefined; if (!this._terminal || !term || term.length === 0) { - result = undefined; this._terminal?.clearSelection(); this.clearDecorations(); return false; } - if (this._cachedSearchTerm !== term) { - this._terminal.clearSelection(); - } + const prevSelectedPos = this._terminal.getSelectionPosition(); + this._terminal.clearSelection(); let startRow = this._terminal.buffer.active.baseY + this._terminal.rows - 1; let startCol = this._terminal.cols; const isReverseSearch = true; - let currentSelection: IBufferRange | undefined; - if (this._terminal.hasSelection()) { - currentSelection = this._terminal.getSelectionPosition()!; - // Start from selection start if there is a selection - startRow = currentSelection.start.y; - startCol = currentSelection.start.x; - } - this._initLinesCache(); const searchPosition: ISearchPosition = { startRow, startCol }; - // const incremental = searchOptions ? searchOptions.incremental : false; - // if (incremental) { - // // Try to expand selection to right first. - // result = this._findInLine(term, searchPosition, searchOptions, false); - // const isOldResultHighlighted = result && result.row === startRow && result.col === startCol; - // if (!isOldResultHighlighted) { - // // If selection was not able to be expanded to the right, then try reverse search - // if (currentSelection) { - // searchPosition.startRow = currentSelection.end.y; - // searchPosition.startCol = currentSelection.end.x; - // } - // result = this._findInLine(term, searchPosition, searchOptions, true); - // } - // } else { - result = this._findInLine(term, searchPosition, searchOptions, isReverseSearch); - // } + let result: ISearchResult | undefined; + if (prevSelectedPos) { + searchPosition.startRow = startRow = prevSelectedPos.start.y; + searchPosition.startCol = startCol = prevSelectedPos.start.x; + if (this._cachedSearchTerm !== term) { + // Try to expand selection to right first. + result = this._findInLine(term, searchPosition, searchOptions, false); + if (!result) { + // If selection was not able to be expanded to the right, then try reverse search + searchPosition.startRow = startRow = prevSelectedPos.end.y; + searchPosition.startCol = startCol = prevSelectedPos.end.x; + } + } + } + + if (!result) { + result = this._findInLine(term, searchPosition, searchOptions, isReverseSearch); + } // Search from startRow - 1 to top if (!result) { @@ -400,9 +389,6 @@ export class SearchAddon extends Disposable implements ITerminalAddon { } } - // If there is only one result, return true. - if (!result && currentSelection) return true; - // Set selection and scroll if a result was found return this._selectResult(result, searchOptions?.decorations, searchOptions?.noScroll); } diff --git a/addons/xterm-addon-search/test/SearchAddon.api.ts b/addons/xterm-addon-search/test/SearchAddon.api.ts index dc3ecef3..df942735 100644 --- a/addons/xterm-addon-search/test/SearchAddon.api.ts +++ b/addons/xterm-addon-search/test/SearchAddon.api.ts @@ -178,7 +178,7 @@ describe('Search Tests', function (): void { window.calls = []; window.search.onDidChangeResults(e => window.calls.push(e)); `); - await writeSync(page, 'abc aabc'); + await writeSync(page, 'd abc aabc d'); assert.deepStrictEqual(await page.evaluate(`window.search.findNext('a', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); assert.deepStrictEqual(await page.evaluate('window.calls'), [ { resultCount: 3, resultIndex: 0 } @@ -201,12 +201,21 @@ describe('Search Tests', function (): void { { resultCount: 2, resultIndex: 0 }, { resultCount: 2, resultIndex: 1 } ]); + assert.deepStrictEqual(await page.evaluate(`window.search.findNext('d', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 3, resultIndex: 0 }, + { resultCount: 2, resultIndex: 0 }, + { resultCount: 2, resultIndex: 0 }, + { resultCount: 2, resultIndex: 1 }, + { resultCount: 2, resultIndex: 1 } + ]); assert.deepStrictEqual(await page.evaluate(`window.search.findNext('abcd', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), false); assert.deepStrictEqual(await page.evaluate('window.calls'), [ { resultCount: 3, resultIndex: 0 }, { resultCount: 2, resultIndex: 0 }, { resultCount: 2, resultIndex: 0 }, { resultCount: 2, resultIndex: 1 }, + { resultCount: 2, resultIndex: 1 }, { resultCount: 0, resultIndex: -1 } ]); }); @@ -230,7 +239,7 @@ describe('Search Tests', function (): void { assert.deepStrictEqual(await page.evaluate('window.calls'), [ { resultCount: 1000, resultIndex: 0 }, { resultCount: 1000, resultIndex: 1 }, - { resultCount: 1000, resultIndex: 0 } + { resultCount: 1000, resultIndex: 1 } ]); }); }); @@ -256,6 +265,7 @@ describe('Search Tests', function (): void { assert.deepStrictEqual(await page.evaluate('window.calls'), [ { resultCount: 1, resultIndex: 0 } ]); + await page.evaluate(`window.term.clearSelection()`); assert.strictEqual(await page.evaluate(`window.search.findPrevious('b', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); assert.deepStrictEqual(await page.evaluate('window.calls'), [ { resultCount: 1, resultIndex: 0 }, @@ -285,7 +295,7 @@ describe('Search Tests', function (): void { window.calls = []; window.search.onDidChangeResults(e => window.calls.push(e)); `); - await writeSync(page, 'abc aabc'); + await writeSync(page, 'd abc aabc d'); assert.deepStrictEqual(await page.evaluate(`window.search.findPrevious('a', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); assert.deepStrictEqual(await page.evaluate('window.calls'), [ { resultCount: 3, resultIndex: 2 } @@ -308,12 +318,21 @@ describe('Search Tests', function (): void { { resultCount: 2, resultIndex: 1 }, { resultCount: 2, resultIndex: 0 } ]); + assert.deepStrictEqual(await page.evaluate(`window.search.findPrevious('d', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 3, resultIndex: 2 }, + { resultCount: 2, resultIndex: 1 }, + { resultCount: 2, resultIndex: 1 }, + { resultCount: 2, resultIndex: 0 }, + { resultCount: 2, resultIndex: 1 } + ]); assert.deepStrictEqual(await page.evaluate(`window.search.findPrevious('abcd', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), false); assert.deepStrictEqual(await page.evaluate('window.calls'), [ { resultCount: 3, resultIndex: 2 }, { resultCount: 2, resultIndex: 1 }, { resultCount: 2, resultIndex: 1 }, { resultCount: 2, resultIndex: 0 }, + { resultCount: 2, resultIndex: 1 }, { resultCount: 0, resultIndex: -1 } ]); }); From cc5136aeb3ee478b7220efe4ab2f371ea7466e1d Mon Sep 17 00:00:00 2001 From: Jean Pierre Date: Fri, 12 May 2023 16:08:11 -0500 Subject: [PATCH 5/9] Address feedback --- addons/xterm-addon-search/src/SearchAddon.ts | 46 ++++++++++++------- .../test/SearchAddon.api.ts | 34 ++++++++++++++ .../typings/xterm-addon-search.d.ts | 17 ++++++- 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/addons/xterm-addon-search/src/SearchAddon.ts b/addons/xterm-addon-search/src/SearchAddon.ts index 9dd1ffc8..59afe581 100644 --- a/addons/xterm-addon-search/src/SearchAddon.ts +++ b/addons/xterm-addon-search/src/SearchAddon.ts @@ -3,7 +3,7 @@ * @license MIT */ -import { Terminal, IDisposable, ITerminalAddon, IBufferRange, IDecoration } from 'xterm'; +import { Terminal, IDisposable, ITerminalAddon, IDecoration } from 'xterm'; import { EventEmitter } from 'common/EventEmitter'; import { Disposable, toDisposable, disposeArray } from 'common/Lifecycle'; @@ -30,6 +30,10 @@ export interface ISearchPosition { startRow: number; } +export interface ISearchAddonOptions { + highlightLimit: number; +} + export interface ISearchResult { term: string; col: number; @@ -48,15 +52,22 @@ type LineCacheEntry = [ lineOffsets: number[] ]; +interface IHighlight extends IDisposable { + decoration: IDecoration; + match: ISearchResult; +} + const NON_WORD_CHARACTERS = ' ~!@#$%^&*()+`-=[]{}|\\;:"\',./<>?'; const LINES_CACHE_TIME_TO_LIVE = 15 * 1000; // 15 secs +const DEFAULT_HIGHLIGHT_LIMIT = 1000; export class SearchAddon extends Disposable implements ITerminalAddon { private _terminal: Terminal | undefined; private _cachedSearchTerm: string | undefined; private _highlightedLines: Set = new Set(); - private _highlightDecorations: (IDecoration & { match: ISearchResult })[] = []; - private _selectedDecoration: IDecoration & { match: ISearchResult } | undefined; + private _highlightDecorations: IHighlight[] = []; + private _selectedDecoration: IHighlight | undefined; + private _highlightLimit: number; private _onDataDisposable: IDisposable | undefined; private _onResizeDisposable: IDisposable | undefined; private _lastSearchOptions: ISearchOptions | undefined; @@ -74,7 +85,11 @@ export class SearchAddon extends Disposable implements ITerminalAddon { private readonly _onDidChangeResults = this.register(new EventEmitter<{ resultIndex: number, resultCount: number }>()); public readonly onDidChangeResults = this._onDidChangeResults.event; - public readonly MATCHES_LIMIT = 1000; + constructor(options?: Partial) { + super(); + + this._highlightLimit = options?.highlightLimit ?? DEFAULT_HIGHLIGHT_LIMIT; + } public activate(terminal: Terminal): void { this._terminal = terminal; @@ -93,14 +108,15 @@ export class SearchAddon extends Disposable implements ITerminalAddon { } if (this._cachedSearchTerm && this._lastSearchOptions?.decorations) { this._highlightTimeout = setTimeout(() => { - this.findPrevious(this._cachedSearchTerm!, { ...this._lastSearchOptions, incremental: true, noScroll: true }); - this._fireResults(this._lastSearchOptions); + const term = this._cachedSearchTerm; + this._cachedSearchTerm = undefined; + this.findPrevious(term!, { ...this._lastSearchOptions, incremental: true, noScroll: true }); }, 200); } } public clearDecorations(retainCachedSearchTerm?: boolean): void { - this._selectedDecoration?.dispose(); + this.clearActiveDecoration(); disposeArray(this._highlightDecorations); this._highlightDecorations = []; this._highlightedLines.clear(); @@ -156,7 +172,7 @@ export class SearchAddon extends Disposable implements ITerminalAddon { let prevResult: ISearchResult | undefined = undefined; let result = this._find(term, 0, 0, searchOptions); while (result && (prevResult?.row !== result.row || prevResult?.col !== result.col)) { - if (searchResultsWithHighlight.length >= this.MATCHES_LIMIT) { + if (searchResultsWithHighlight.length >= this._highlightLimit) { break; } prevResult = result; @@ -168,12 +184,11 @@ export class SearchAddon extends Disposable implements ITerminalAddon { searchOptions ); } - for (const result of searchResultsWithHighlight) { - const resultDecoration = this._createResultDecoration(result, searchOptions.decorations!); - if (resultDecoration) { - this._highlightedLines.add(resultDecoration.marker.line); - (resultDecoration as unknown as { match: ISearchResult }).match = result; - this._highlightDecorations.push(resultDecoration as (IDecoration & { match: ISearchResult })); + for (const match of searchResultsWithHighlight) { + const decoration = this._createResultDecoration(match, searchOptions.decorations!); + if (decoration) { + this._highlightedLines.add(decoration.marker.line); + this._highlightDecorations.push({ decoration, match, dispose() { decoration.dispose(); } }); } } } @@ -654,8 +669,7 @@ export class SearchAddon extends Disposable implements ITerminalAddon { disposables.push(marker); disposables.push(decoration.onRender((e) => this._applyStyles(e, options.activeMatchBorder, true))); disposables.push(decoration.onDispose(() => disposeArray(disposables))); - (decoration as unknown as { match: ISearchResult }).match = result; - this._selectedDecoration = decoration as (IDecoration & { match: ISearchResult }); + this._selectedDecoration = { decoration, match: result, dispose() { decoration.dispose(); } }; } } } diff --git a/addons/xterm-addon-search/test/SearchAddon.api.ts b/addons/xterm-addon-search/test/SearchAddon.api.ts index df942735..0c20084a 100644 --- a/addons/xterm-addon-search/test/SearchAddon.api.ts +++ b/addons/xterm-addon-search/test/SearchAddon.api.ts @@ -242,6 +242,23 @@ describe('Search Tests', function (): void { { resultCount: 1000, resultIndex: 1 } ]); }); + it('should fire when writing to terminal', async () => { + await page.evaluate(` + window.calls = []; + window.search.onDidChangeResults(e => window.calls.push(e)); + `); + await writeSync(page, 'abc bc c\\n\\r'.repeat(2)); + assert.strictEqual(await page.evaluate(`window.search.findNext('abc', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 2, resultIndex: 0 } + ]); + await writeSync(page, 'abc bc c\\n\\r'); + await timeout(300); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 2, resultIndex: 0 }, + { resultCount: 3, resultIndex: 0 } + ]); + }); }); describe('findPrevious', () => { it('should not fire unless the decorations option is set', async () => { @@ -359,6 +376,23 @@ describe('Search Tests', function (): void { { resultCount: 1000, resultIndex: -1 } ]); }); + it('should fire when writing to terminal', async () => { + await page.evaluate(` + window.calls = []; + window.search.onDidChangeResults(e => window.calls.push(e)); + `); + await writeSync(page, 'abc bc c\\n\\r'.repeat(2)); + assert.strictEqual(await page.evaluate(`window.search.findPrevious('abc', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 2, resultIndex: 1 } + ]); + await writeSync(page, 'abc bc c\\n\\r'); + await timeout(300); + assert.deepStrictEqual(await page.evaluate('window.calls'), [ + { resultCount: 2, resultIndex: 1 }, + { resultCount: 3, resultIndex: 1 } + ]); + }); }); }); diff --git a/addons/xterm-addon-search/typings/xterm-addon-search.d.ts b/addons/xterm-addon-search/typings/xterm-addon-search.d.ts index 05cb8ebb..3a3b7c89 100644 --- a/addons/xterm-addon-search/typings/xterm-addon-search.d.ts +++ b/addons/xterm-addon-search/typings/xterm-addon-search.d.ts @@ -75,14 +75,27 @@ declare module 'xterm-addon-search' { activeMatchColorOverviewRuler: string; } + /** + * Options for the search addon. + */ + export interface ISearchAddonOptions { + /** + * Max number of matches highlighted when decorations are enabled. + * Defaults to 1000 highlighted matches + */ + highlightLimit: number + } + /** * An xterm.js addon that provides search functionality. */ export class SearchAddon implements ITerminalAddon { + /** - * Max number of matches when decorations are enabled + * Creates a new search addon. + * @param options Options for the search addon. */ - public readonly MATCHES_LIMIT: number; + constructor(options?: Partial); /** * Activates the addon From 60f549028f0644da05c9795871ee021b5e1b55b4 Mon Sep 17 00:00:00 2001 From: Jean Pierre Date: Fri, 12 May 2023 16:19:03 -0500 Subject: [PATCH 6/9] Ensure decorations in the top layer render on top of decorations in the bottom layer --- css/xterm.css | 6 +++++- src/browser/decorations/BufferDecorationRenderer.ts | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/css/xterm.css b/css/xterm.css index 2746016b..555632a6 100644 --- a/css/xterm.css +++ b/css/xterm.css @@ -179,8 +179,12 @@ position: absolute; } +.xterm-screen .xterm-decoration-container .xterm-decoration.top-layer { + z-index: 7; +} + .xterm-decoration-overview-ruler { - z-index: 7; + z-index: 8; position: absolute; top: 0; right: 0; diff --git a/src/browser/decorations/BufferDecorationRenderer.ts b/src/browser/decorations/BufferDecorationRenderer.ts index e195c5ae..5d00d2ff 100644 --- a/src/browser/decorations/BufferDecorationRenderer.ts +++ b/src/browser/decorations/BufferDecorationRenderer.ts @@ -72,6 +72,7 @@ export class BufferDecorationRenderer extends Disposable { private _createElement(decoration: IInternalDecoration): HTMLElement { const element = document.createElement('div'); element.classList.add('xterm-decoration'); + element.classList.toggle('top-layer', decoration?.options?.layer === 'top'); element.style.width = `${Math.round((decoration.options.width || 1) * this._renderService.dimensions.css.cell.width)}px`; element.style.height = `${(decoration.options.height || 1) * this._renderService.dimensions.css.cell.height}px`; element.style.top = `${(decoration.marker.line - this._bufferService.buffers.active.ydisp) * this._renderService.dimensions.css.cell.height}px`; From 4fbeb58203a294bf62dd28b5314e7c7d7d736eb5 Mon Sep 17 00:00:00 2001 From: Jean Pierre Date: Mon, 15 May 2023 22:18:36 -0500 Subject: [PATCH 7/9] Update search demo --- demo/client.ts | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/demo/client.ts b/demo/client.ts index 68cdec65..a9a8a23c 100644 --- a/demo/client.ts +++ b/demo/client.ts @@ -132,12 +132,11 @@ function setPadding(): void { addons.fit.instance.fit(); } -function getSearchOptions(e: KeyboardEvent): ISearchOptions { +function getSearchOptions(): ISearchOptions { return { regex: (document.getElementById('regex') as HTMLInputElement).checked, wholeWord: (document.getElementById('whole-word') as HTMLInputElement).checked, caseSensitive: (document.getElementById('case-sensitive') as HTMLInputElement).checked, - incremental: e.key !== `Enter`, decorations: (document.getElementById('highlight-all-matches') as HTMLInputElement).checked ? { matchBackground: '#232422', matchBorder: '#555753', @@ -303,11 +302,23 @@ function createTerminal(): void { addDomListener(paddingElement, 'change', setPadding); - addDomListener(actionElements.findNext, 'keyup', (e) => { - addons.search.instance.findNext(actionElements.findNext.value, getSearchOptions(e)); + addDomListener(actionElements.findNext, 'keydown', (e) => { + if (e.key === 'Enter') { + addons.search.instance.findNext(actionElements.findNext.value, getSearchOptions()); + e.preventDefault(); + } }); - addDomListener(actionElements.findPrevious, 'keyup', (e) => { - addons.search.instance.findPrevious(actionElements.findPrevious.value, getSearchOptions(e)); + addDomListener(actionElements.findNext, 'input', (e) => { + addons.search.instance.findNext(actionElements.findNext.value, getSearchOptions()); + }); + addDomListener(actionElements.findPrevious, 'keydown', (e) => { + if (e.key === 'Enter') { + addons.search.instance.findPrevious(actionElements.findPrevious.value, getSearchOptions()); + e.preventDefault(); + } + }); + addDomListener(actionElements.findPrevious, 'input', (e) => { + addons.search.instance.findPrevious(actionElements.findPrevious.value, getSearchOptions()); }); addDomListener(actionElements.findNext, 'blur', (e) => { addons.search.instance.clearActiveDecoration(); From ec227c757b1904379efcdfcb8bd58983e080b27e Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Wed, 17 May 2023 06:57:13 -0700 Subject: [PATCH 8/9] Prefix class with xterm-decoration- --- css/xterm.css | 2 +- src/browser/decorations/BufferDecorationRenderer.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/css/xterm.css b/css/xterm.css index 555632a6..b14a6cf5 100644 --- a/css/xterm.css +++ b/css/xterm.css @@ -179,7 +179,7 @@ position: absolute; } -.xterm-screen .xterm-decoration-container .xterm-decoration.top-layer { +.xterm-screen .xterm-decoration-container .xterm-decoration.xterm-decoration-top-layer { z-index: 7; } diff --git a/src/browser/decorations/BufferDecorationRenderer.ts b/src/browser/decorations/BufferDecorationRenderer.ts index 5d00d2ff..fb77ce9c 100644 --- a/src/browser/decorations/BufferDecorationRenderer.ts +++ b/src/browser/decorations/BufferDecorationRenderer.ts @@ -72,7 +72,7 @@ export class BufferDecorationRenderer extends Disposable { private _createElement(decoration: IInternalDecoration): HTMLElement { const element = document.createElement('div'); element.classList.add('xterm-decoration'); - element.classList.toggle('top-layer', decoration?.options?.layer === 'top'); + element.classList.toggle('xterm-decoration-top-layer', decoration?.options?.layer === 'top'); element.style.width = `${Math.round((decoration.options.width || 1) * this._renderService.dimensions.css.cell.width)}px`; element.style.height = `${(decoration.options.height || 1) * this._renderService.dimensions.css.cell.height}px`; element.style.top = `${(decoration.marker.line - this._bufferService.buffers.active.ydisp) * this._renderService.dimensions.css.cell.height}px`; From 07e8f86bd1d691ecc8ea02219bb4f27cfa08d132 Mon Sep 17 00:00:00 2001 From: Jean Pierre Date: Wed, 17 May 2023 17:03:25 -0500 Subject: [PATCH 9/9] Avoid triggering a reflow --- addons/xterm-addon-search/src/SearchAddon.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/addons/xterm-addon-search/src/SearchAddon.ts b/addons/xterm-addon-search/src/SearchAddon.ts index 59afe581..a029fc24 100644 --- a/addons/xterm-addon-search/src/SearchAddon.ts +++ b/addons/xterm-addon-search/src/SearchAddon.ts @@ -693,9 +693,6 @@ export class SearchAddon extends Disposable implements ITerminalAddon { * @returns */ private _applyStyles(element: HTMLElement, borderColor: string | undefined, isActiveResult: boolean): void { - if (element.clientWidth <= 0) { - return; - } if (!element.classList.contains('xterm-find-result-decoration')) { element.classList.add('xterm-find-result-decoration'); if (borderColor) {