Fixes #2608 (Quick Open shows red on first use) #3184
Conversation
…en more responsive on large projects by not immediately invalidating the cached file list.
ghost
assigned Now that the file list in Quick Open is more lazily retrieved, we should clear out the old StringMatcher caches to avoid possibly stale data sticking around even after the file list has arrived.
|
The latest change refreshes the StringMatcher caching when the updated file list arrives. I hope we can get this in soon, because Quick Open has become a lot less usable recently in Brackets and this helps a lot. |
| @@ -391,7 +391,7 @@ define(function (require, exports, module) { | |||
| */ | |||
| QuickNavigateDialog.prototype._handleResultsReady = function (e, results) { | |||
| // Give visual clue when there are no results | |||
| var isNoResults = (results.length === 0 && !this._isValidLineNumberQuery(this.$searchField.val())); | |||
| var isNoResults = (results.length === 0 && fileList && !this._isValidLineNumberQuery(this.$searchField.val())); | |||
There was a problem hiding this comment.
I think it needs to be ...&& (fileList || [search mode is not Quick Open]) &&...
There was a problem hiding this comment.
Which I guess would be (fileList || currentPlugin), since I think we can be guaranteed that _filterCallback() (which updates currentPlugin) runs before _handleResultsReady().
There was a problem hiding this comment.
Ahh, yes, you're right.
|
We should probably add a simple unit test for StringMatcher.reset(). I notice there's also a QuickOpen test suite but it only contains one testcase, related to scrolling... so it's probably not worth trying to add anything more to cover this new behavior. I would like to spend some time testing by hand tomorrow before we merge this though... knowing that Quick Open timing has been a very delicate area in the past :-/ |
* Fixes the detection of no results * removes extra call to matcher.reset() * adds a test for StringMatcher.reset
|
I've added a test for StringMatcher.reset. I've noticed that I'm now getting a popup saying that the indexer has reached the maximum number of files. We'll probably need a better solution to this soon. (Or, we can kick the can down the road by providing a way to exclude node_modules, which is now 74% of the total files in my brackets directory.) |
|
Unit test looks good. Will test a little bit tonight before merging. I haven't hit the 10k file max yet on my machine... I guess I have less stuff in extensions/dev or something :-) But yeah, this patch is definitely just a stopgap. I hope we get file watchers soon... and exclusions wouldn't hurt too -- not just for Quick Open, but also Find in Files, etc. |
|
Hmm, in testing on my slowish Boot Camp machine I'm seeing some cases where a red background appears briefly. For example: refresh Brackets; as soon as it's done, launch Quick Open and type something like "docman." You'll see a red background up until the list is available (i.e. until the initial file index build is complete). I think this is because |
| @@ -391,7 +391,7 @@ define(function (require, exports, module) { | |||
| */ | |||
| QuickNavigateDialog.prototype._handleResultsReady = function (e, results) { | |||
| // Give visual clue when there are no results | |||
There was a problem hiding this comment.
Also, while you're making another commit... could you add something here to clarify what we mean by "no results"? E.g. "...when there are no results - unless we're in 'Go to Line' mode, where there are never results; or when we're in file search mode and still waiting for the index to get rebuilt."
|
Actually, I'd suggest using |
|
Even with the boolean fix, I'm still seeing it turn red in some cases -- when I use Go to Definition, hit Escape, and then use plain Quick Open. It seems to come and go though, so it may be rare enough (especially on faster systems) to not worry about. It's definitely still an improvement over the current behavior on master. |
|
I think that remaining bug may be because of the case at the top of _filterCallback(), where it returns without updating currentPlugin. Still think it's probably not worth trying to fix right now though. |
|
I discovered that my test didn't actually work (bitten by the long-standing caching issue – I didn't have the dev tools open). I just needed to move the I added the boolean fix and updated the comment. I wasn't able to reproduce the remaining "field turning red", and I agree with you that the new behavior is probably good enough for now. |
|
Given the trivial nature of the updates I needed to make, I'll go ahead and merge. |
Fixes #2608 (Quick Open shows red on first use)
Fixes #2608 (Quick Open shows red on first use) and also makes Quick Open more responsive on large projects by not immediately invalidating the cached file list.
Just eliminating the "red on first use" is a one line change:
But I was also keen to make Quick Open more useful for Brackets again by updating the file list in the background, providing possibly mildly stale results in the meantime.
Ideally, we'd have a spinner while the loading is happening, but I have other things on my plate right now and didn't want to try to build a spinner into the modal bar right now.