- Notifications
You must be signed in to change notification settings - Fork1.8k
WIP: Trying to fix #5176#5272
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
WIP: Trying to fix #5176#5272
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Tyriar commentedJan 6, 2025
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. |
Tyriar left a comment
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, soPriorityTaskQueue is more appropriate. It works much the same, just it's backed bysetTimeout, notrequestIdleCallback.
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 71this._idleCallback = undefined; which in my opinion should be removed and inserted above line 95this.start() that way new enqueue calls wont trigger new setTimeouts.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
AnouarTouati commentedJan 6, 2025
I am currently highlighting all of the matches so this will not be a problem. |
AnouarTouati commentedJan 6, 2025
No, this is what the incremental flag is doing
Notice that the second B is selected. That's all it does. It does not help performance. I removed this behavior. |
Broke regex search
Moved constants to an Enum
c71ef1b to6ecf521CompareTyriar commentedJan 6, 2025
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
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
AnouarTouati left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
AnouarTouati left a comment
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. | ||
| */ | ||
| readonlyonDidChangeResults:IEvent<{resultIndex:number,resultCount:number}>; | ||
| readonlyonDidChangeResults: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){ | ||
| letresultIndex=-1; | ||
| if(this._selectedDecoration.value){ | ||
| constselectedMatch=this._selectedDecoration.value.match; | ||
| for(leti=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(awaitctx.page.evaluate(`window.search.findNext('$^1_3{}test$#')`),true); | ||
| awaitctx.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(awaitctx.page.evaluate('window.calls'),[ | ||
| {resultCount:2,resultIndex:1} | ||
| ]); | ||
| awaitctx.proxy.write('\\x1b[CHi Hi Hi'); | ||
| awaitctx.page.evaluate(`window.search.findPrevious('h', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`); | ||
| awaittimeout(TIMEOUT); | ||
| deepStrictEqual(awaitctx.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 havesearchCompletedtrue.
AnouarTouati commentedJan 19, 2025
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 |
Uh oh!
There was an error while loading.Please reload this page.
This draft is WIP attempt tofix#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