[CLOSED] Fixes #2608 (Quick Open shows red on first use)

[core-ai-bot]

Issue by dangoor
Wednesday Mar 20, 2013 at 15:03 GMT
Originally opened as adobe/brackets#3184

---

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:

        var isNoResults = (results.length === 0 && fileList && !this._isValidLineNumberQuery(this.$searchField.val()));

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.

---

dangoor included the following code: https://github.com/adobe/brackets/pull/3184/commits

[core-ai-bot]

Comment by dangoor
Wednesday Mar 27, 2013 at 13:40 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Wednesday Apr 03, 2013 at 08:14 GMT

---

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 :-/

[core-ai-bot]

Comment by dangoor
Wednesday Apr 03, 2013 at 13:12 GMT

---

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.)

[core-ai-bot]

Comment by peterflynn
Thursday Apr 04, 2013 at 01:12 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Thursday Apr 04, 2013 at 05:41 GMT

---

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 isNoResults is undefined, while jQuery.toggleClasss() in all its awesome jQueryism-ness requires a strict boolean instead of just truthy or falsy. If I change the code to toggleClass(..., !!isNoResults) then it seems to fix all these cases.

[core-ai-bot]

Comment by peterflynn
Thursday Apr 04, 2013 at 05:46 GMT

---

Actually, I'd suggest using Boolean(isNoResults) rather than !! -- looks like that's what our other code does, and it's a bit cleaner looking.

[core-ai-bot]

Comment by peterflynn
Thursday Apr 04, 2013 at 06:29 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Thursday Apr 04, 2013 at 06:32 GMT

---

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.

[core-ai-bot]

Comment by dangoor
Thursday Apr 04, 2013 at 13:03 GMT

---

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 this.addMatchers call into a beforeEach.

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.

[core-ai-bot]

Comment by dangoor
Thursday Apr 04, 2013 at 13:05 GMT

---

Given the trivial nature of the updates I needed to make, I'll go ahead and merge.