-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
WIP: Trying to fix #5176 #5272
base: master
Are you sure you want to change the base?
WIP: Trying to fix #5176 #5272
Conversation
Adding responses to #5176 (comment) here.
This is to aid in incremental searches
Something we want to keep is the highlighting of results in the "overview ruler": Limiting these results too heavily will hurt the UX by not showing additional results. Make sure you add the overview ruler in the demo when working on this:
If I'm understanding you correctly, searching for This is by design, incremental search means we can avoid doing much of the work, provided regex is not enabled which I think disables incremental completely.
The overview ruler is again an important piece here, see this result which is after running There are 19 results, and every one of these is represented in the overview ruler, allowing the user to know immediately how far back the term(s) are. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks promising! 👏
/** | ||
* Number of matches in each chunk | ||
*/ | ||
private _chunkSize: number = 200; | ||
/** | ||
* Time in ms | ||
* 1 ms seems to work fine as we just need to let other parts of the code to take over | ||
* and return here when their work is done | ||
*/ | ||
private _timeBetweenChunkOperations = 1; | ||
|
||
/** | ||
* This should be high enough so not to trigger a lot of searches | ||
* and subsequently a lot of canceled searches which clean up their own | ||
* decorations and cause flickers | ||
*/ | ||
private _debounceTimeWindow = 300; | ||
/** | ||
* Using this mainly for resizing event | ||
*/ | ||
private _longerDebounceTimeWindow = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's actually a perfect helper for just this scenario in the main code:
This will let you continue to search until the deadline is reached, essentially scaling the solution to high end CPUs that can handle searching a lot more immediately instead of the arbitrary 200 line chunk limit. This is how we draw ascii glyphs to "warm up" the texture atlas without doing any of this optional work in a blocking manner.
For your scenario it's important work that the user is waiting on, not optional, so PriorityTaskQueue
is more appropriate. It works much the same, just it's backed by setTimeout
, not requestIdleCallback
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is what I understood and correct me if I am wrong.
I will be en-queuing call back functions (Tasks) that process X number of lines. The task queue will run as many tasks as possible until we exceed the time limit at which point it yields, then do a new setTimeout to start processing of next batch of Tasks.
About the race condition comment. I think it arises from line 71 this._idleCallback = undefined;
which in my opinion should be removed and inserted above line 95 this.start()
that way new enqueue calls wont trigger new setTimeouts.
I am currently highlighting all of the matches so this will not be a problem. |
Broke regex search
Moved constants to an Enum
c71ef1b
to
6ecf521
Compare
I'd have to play around with it, we just want to make sure it's consistent |
and fixed multipe highlighting of regex matches
…eal with returning -1 on max highlight reached or about firing without decorations option set.
…on. 11 tests of 12 pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the "incremental" functionality while working on the code. Do I need to put back to pass these tests? Or have it incremental as in getting a subset of the previous search result i.e, only search inside in the previous set of matches instead of the whole buffer/terminal.
// test('Incremental Find Previous', async () => { | ||
// await ctx.proxy.writeln(`package.jsonc\n`); | ||
// await ctx.proxy.write('package.json pack package.lock'); | ||
// await ctx.page.evaluate(`window.search.findPrevious('pack', {incremental: true})`); | ||
// let selectionPosition: { start: { x: number, y: number }, end: { x: number, y: number } } = (await ctx.proxy.getSelectionPosition())!; | ||
// let line: string = await (await ctx.proxy.buffer.active.getLine(selectionPosition.start.y))!.translateToString(); | ||
// // We look further ahead in the line to ensure that pack was selected from package.lock | ||
// deepStrictEqual(line.substring(selectionPosition.start.x, selectionPosition.end.x + 8), 'package.lock'); | ||
// await ctx.page.evaluate(`window.search.findPrevious('package.j', {incremental: true})`); | ||
// selectionPosition = (await ctx.proxy.getSelectionPosition())!; | ||
// deepStrictEqual(line.substring(selectionPosition.start.x, selectionPosition.end.x + 3), 'package.json'); | ||
// await ctx.page.evaluate(`window.search.findPrevious('package.jsonc', {incremental: true})`); | ||
// // We have to reevaluate line because it should have switched starting rows at this point | ||
// selectionPosition = (await ctx.proxy.getSelectionPosition())!; | ||
// line = await (await ctx.proxy.buffer.active.getLine(selectionPosition.start.y))!.translateToString(); | ||
// deepStrictEqual(line.substring(selectionPosition.start.x, selectionPosition.end.x), 'package.jsonc'); | ||
// }); | ||
// test('Incremental Find Next', async () => { | ||
// await ctx.proxy.writeln(`package.lock pack package.json package.ups\n`); | ||
// await ctx.proxy.write('package.jsonc'); | ||
// await ctx.page.evaluate(`window.search.findNext('pack', {incremental: true})`); | ||
// let selectionPosition: { start: { x: number, y: number }, end: { x: number, y: number } } = (await ctx.proxy.getSelectionPosition())!; | ||
// let line: string = await (await ctx.proxy.buffer.active.getLine(selectionPosition.start.y))!.translateToString(); | ||
// // We look further ahead in the line to ensure that pack was selected from package.lock | ||
// deepStrictEqual(line.substring(selectionPosition.start.x, selectionPosition.end.x + 8), 'package.lock'); | ||
// await ctx.page.evaluate(`window.search.findNext('package.j', {incremental: true})`); | ||
// selectionPosition = (await ctx.proxy.getSelectionPosition())!; | ||
// deepStrictEqual(line.substring(selectionPosition.start.x, selectionPosition.end.x + 3), 'package.json'); | ||
// await ctx.page.evaluate(`window.search.findNext('package.jsonc', {incremental: true})`); | ||
// // We have to reevaluate line because it should have switched starting rows at this point | ||
// selectionPosition = (await ctx.proxy.getSelectionPosition())!; | ||
// line = await (await ctx.proxy.buffer.active.getLine(selectionPosition.start.y))!.translateToString(); | ||
// deepStrictEqual(line.substring(selectionPosition.start.x, selectionPosition.end.x), 'package.jsonc'); | ||
// }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out "Incremental" test.
// test('should fire with correct event values (incremental)', async () => { | ||
// await ctx.page.evaluate(` | ||
// window.calls = []; | ||
// window.search.onDidChangeResults(e => window.calls.push(e)); | ||
// `); | ||
// await ctx.proxy.write('d abc aabc d'); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findNext('a', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 3, resultIndex: 0 } | ||
// ]); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findNext('ab', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 3, resultIndex: 0 }, | ||
// { resultCount: 2, resultIndex: 0 } | ||
// ]); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findNext('abc', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 3, resultIndex: 0 }, | ||
// { resultCount: 2, resultIndex: 0 }, | ||
// { resultCount: 2, resultIndex: 0 } | ||
// ]); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findNext('abc', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 3, resultIndex: 0 }, | ||
// { resultCount: 2, resultIndex: 0 }, | ||
// { resultCount: 2, resultIndex: 0 }, | ||
// { resultCount: 2, resultIndex: 1 } | ||
// ]); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findNext('d', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 3, resultIndex: 0 }, | ||
// { resultCount: 2, resultIndex: 0 }, | ||
// { resultCount: 2, resultIndex: 0 }, | ||
// { resultCount: 2, resultIndex: 1 }, | ||
// { resultCount: 2, resultIndex: 1 } | ||
// ]); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findNext('abcd', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), false); | ||
// deepStrictEqual(await ctx.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 } | ||
// ]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out "Incremental" test.
// test('should fire with correct event values (incremental)', async () => { | ||
// await ctx.page.evaluate(` | ||
// window.calls = []; | ||
// window.search.onDidChangeResults(e => window.calls.push(e)); | ||
// `); | ||
// await ctx.proxy.write('d abc aabc d'); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findPrevious('a', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 3, resultIndex: 2 } | ||
// ]); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findPrevious('ab', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 3, resultIndex: 2 }, | ||
// { resultCount: 2, resultIndex: 1 } | ||
// ]); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findPrevious('abc', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 3, resultIndex: 2 }, | ||
// { resultCount: 2, resultIndex: 1 }, | ||
// { resultCount: 2, resultIndex: 1 } | ||
// ]); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findPrevious('abc', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 3, resultIndex: 2 }, | ||
// { resultCount: 2, resultIndex: 1 }, | ||
// { resultCount: 2, resultIndex: 1 }, | ||
// { resultCount: 2, resultIndex: 0 } | ||
// ]); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findPrevious('d', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 3, resultIndex: 2 }, | ||
// { resultCount: 2, resultIndex: 1 }, | ||
// { resultCount: 2, resultIndex: 1 }, | ||
// { resultCount: 2, resultIndex: 0 }, | ||
// { resultCount: 2, resultIndex: 1 } | ||
// ]); | ||
// deepStrictEqual(await ctx.page.evaluate(`window.search.findPrevious('abcd', { incremental: true, decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), false); | ||
// deepStrictEqual(await ctx.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 } | ||
// ]); | ||
// }); | ||
// test('should fire with more than 1k matches', async () => { | ||
// await ctx.page.evaluate(` | ||
// window.calls = []; | ||
// window.search.onDidChangeResults(e => window.calls.push(e)); | ||
// `); | ||
// const data = ('a bc'.repeat(10) + '\\n\\r').repeat(150); | ||
// await ctx.proxy.write(data); | ||
// strictEqual(await ctx.page.evaluate(`window.search.findPrevious('a', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 1000, resultIndex: -1 } | ||
// ]); | ||
// strictEqual(await ctx.page.evaluate(`window.search.findPrevious('a', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 1000, resultIndex: -1 }, | ||
// { resultCount: 1000, resultIndex: -1 } | ||
// ]); | ||
// strictEqual(await ctx.page.evaluate(`window.search.findPrevious('bc', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true); | ||
// deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
// { resultCount: 1000, resultIndex: -1 }, | ||
// { resultCount: 1000, resultIndex: -1 }, | ||
// { resultCount: 1000, resultIndex: -1 } | ||
// ]); | ||
// }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out "Incremental" test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the API changes needed for async search.
* When decorations are enabled, fires when | ||
* the search results change. | ||
* @returns -1 for resultIndex when the threshold of matches is exceeded. | ||
* Fired everytime search progresses; until the search completes. | ||
* @property {number} resultIndex - not final until seachedCompleyed is true. | ||
* @property {number} resultCount - not final until searchCompleted is true. | ||
* @property {boolean} searchCompleted. | ||
* @returns an IDisposable to stop listening. | ||
*/ | ||
readonly onDidChangeResults: IEvent<{ resultIndex: number, resultCount: number }>; | ||
readonly onDidChangeResults: IEvent<{ resultIndex: number, resultCount: number, searchCompleted: boolean }>; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need to add another property (boolean) for when the matches exceed the highlight limit?
private _fireResults(): void { | ||
this._onDidChangeResults.fire({ resultIndex:this._currentMatchIndex, resultCount: this._matches.length,searchCompleted: this._searchCompleted }); | ||
} | ||
private *_chunkSearchGenerator(term: string): Generator<{direction: string,chunkSize: number}>{ | ||
|
||
private _fireResults(searchOptions?: ISearchOptions): void { | ||
if (searchOptions?.decorations) { | ||
let resultIndex = -1; | ||
if (this._selectedDecoration.value) { | ||
const selectedMatch = this._selectedDecoration.value.match; | ||
for (let i = 0; i < this._highlightDecorations.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fire whenever we have a new chunk regardless of searchOptions.decorations is set or not.
deepStrictEqual(await ctx.page.evaluate(`window.search.findNext('$^1_3{}test$#')`), true); | ||
await ctx.page.evaluate(`window.search.findNext('$^1_3{}test$#')`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find not longer returns a boolean for search result since it is now async. so I changed all tests to not expect a boolean. added getSelection assertion instead, when needed.
deepStrictEqual(await ctx.page.evaluate('window.calls'), [ | ||
{ resultCount: 2, resultIndex: 1 } | ||
]); | ||
await ctx.proxy.write('\\x1b[CHi Hi Hi'); | ||
await ctx.page.evaluate(`window.search.findPrevious('h', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`); | ||
await timeout(TIMEOUT); | ||
deepStrictEqual(await ctx.page.evaluate('window.calls[window.calls.length-1]'), | ||
{ resultCount: 3, resultIndex: 0, searchCompleted: true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not firing once per search, but multiple per search (on each chunk ), so we need to get the last entry which should have searchCompleted
true
.
I fixed all the bugs I encountered, except for the flicker. async.mp4Also I tested forcing it to remove all decorations before a new search starts, and finding all matches before rending it (by setting sync.mp4 |
This draft is WIP attempt to fix #5176 @jerch
This is obviously a breaking change.
I tried to go with the conservative approach to keep the code changes to a minimum. But, I had to re-write most of the code.
If this is the direction you want the code to go in. I can keep working on this and maintaining the search addon from now on.
Also ignore the commits I was experimenting with stuff. Will squash them later.
2025-01-06.02-40-30.mp4