Find in Files Improvements (Part 3: Auto-update)

[TomMalbran]

Here comes the part 3 of the Find in Files improvements PR #3151.

This PR updates the find in files results on documents changes. It listens the change event on the current document and searches over the modified lines, adding the new results or deleting the old ones. It also updates the line numbers of the rest of the results, it can remove a complete file from the results when there aren't any anymore or add a new one.

[peterflynn]

One question I have is whether we can/should assume that the current document is the only one that could get modified. (Due to inline editors that's definitely not true, but we could fix that by watching the active editor's document, and then my question above still applies). Certainly an extension could modify any document at any time... We could avoid that risk by adding a generic pub-sub-style message that dispatches on all edits to any document.

Another question is about performance: this adds code that runs on every keystroke all the time. Would it be safer to execute async with throttling/debouncing?

[JeffryBooher]

@TomMalbran Not sure what's going wrong but I'm getting inconsistent results on Windows 8:

I wanted to search for something that had a lot of results but scattered across only a few files. So I right clicked on the "test/spec" folder and searched for "runs" (1014 matches).

I went to the first occurrence (test/spec/LowLevelFileIO-test.js:92) and pressed enter at the end of line 91 which moves the result down to line 93. The find result changed to line 97. If I deleted the blank line by selecting it and pressing delete, the find result changes back to line 92. If I do it again but undo the insertion by pressing Ctrl+Z, the find result changes to line 96. I tried it again and couldn't repro. Then I hit enter a second time to insert 2 blank lines and the find result changed to line 98. It seems like every time I attempt a change I get a different result.

I haven't tried Mac yet. Maybe you weren't seeing this behavior on the Mac?

[TomMalbran]

@peterflynn Right I forgot to test with inline editors and I fixed that in the last commit. But there is still the issue of changing the file without actually opening an editor.

That event could work, maybe it could be triggered from Document.js as an export event, to keep both in the same file. If this works I'll try to implement it it.

The change events that CodeMirror sends are not actually sent on every keystroke. CodeMirror already bundles really close events. I think it sends every history change event. So if several keystrokes are close enough that it can be undone with a single action, then all those changes are sent as one. We are already using this change events all over the code, including for Inline Editors and it doesn't seem to be too slow. But if it feels slow we could update the view every a few seconds (or once per screen refresh using requestAnimation), and not on every change. I think that the view update might be the part that costs the most, unless you end up updating a single line file.

@JeffryBooher I fixed that. It was working on the master Find in Files Improvements PR, but I removed a bit of code that I shouldn't had.

[gruehle]

@TomMalbran - We discussed this at the architecture meeting today. Everyone is concerned about the performance impact. Yes, we do a bunch of processing on change events in other places, but we're also aware that the typing performance in Brackets is already poor at times.

It would be good to get an understanding of the impact of this change. The JavaScript itself probably isn't too bad, but it looks like the results panel is rebuilt on every change event, if there are search results in the current document. Big DOM changes like this can really affect performance. We should run with the Chrome profiler to get an accurate measurement of the impact.

Here are a couple possible ways to mitigate the impact:

1. If the change only affects the line numbers, only update the line numbers in the table without rebuilding the whole table.
2. Do the updates on a timer. When the change event occurs, wait 300-500 milliseconds before processing. If another change event occurs in the meantime, reset the timer. If we do the work at "idle" time like this, it may be good enough to simply re-run the search and rebuild the table every time.

[TomMalbran]

Ok. The content of the panel is rebuilt after something changed. It should be easy to delay this with a timer. The function could still update the results object, but just redraw the panel on idle time.

I will update the code and see how that works.

[TomMalbran]

I updated the code to use a new documentChange event as @peterflynn proposed and a new time out to not redraw the results panel on every change, but the internal search results object is still updated on each change.

[JeffryBooher]

Nominating for Sprint 31

[peterflynn]

@TomMalbran

We talked about this a bit more in yesterday's architecture meeting. Two notes:

• I was reminded that CodeMirror does far less event batching than portrayed above. It's not true that "if several keystrokes are close enough that it can be undone with a single action, then all those changes are sent as one" -- although CodeMirror often batches programmatic edits together, for user input you almost always get one "change" event per keystroke. I think the 'debouncing' timeout you added mitigates that performance concern, though.
• A concern also came up about how the pagination UI plays into auto-updating, since this can change the number of results in the middle of the list. For example, what if my edits create new results, and the results list in view grows beyond the pagination size? Or what if I've already paged past the results for the current file, and then I edit that file to change the number of results? How is the current view updated, and how will the Next/Prev buttons behave?

[TomMalbran]

@peterflynn Good point. The pagination should be fixed, as in enable the next arrow when needed, since the whole results are update. But it doesn't look like it changes the current start when there are less results available. The idea would be to handle this case like is done when a file is deleted: move the current start to the last page.

[TomMalbran]

@peterflynn I moved the code used to fix the current start when a file is deleted, to the restore results part, so it fixed this last issue. Is very easy to remove lots of results by doing a replace all on the searched string.

[TomMalbran]

Any thing else that I should do before we can merge this? I think I solved all the concerns/issues already.

[JeffryBooher]

@TomMalbran Sorry, I've been heads down on another story. I'll merge it later today.

[TomMalbran]

No problem. Just checking on this :)

[JeffryBooher]

Seems like it works pretty well. I tried it on brackets source and it was very responsive to changes and pagination was OK.

[TomMalbran]

Awesome. @JeffryBooher Thanks!

I used several times and it works great, specially when re-factoring the code. It makes it so much easier.