From e4235277fabd3c3d58e9980fe9e9513f4f6c556e Mon Sep 17 00:00:00 2001 From: Kevin Dangoor Date: Wed, 20 Mar 2013 10:58:34 -0400 Subject: [PATCH 1/5] Fixes #2608 (QuickOpen shows red on first use) and also makes QuickOpen more responsive on large projects by not immediately invalidating the cached file list. --- src/search/QuickOpen.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/search/QuickOpen.js b/src/search/QuickOpen.js index 358bded1342..0a51f3d67fc 100644 --- a/src/search/QuickOpen.js +++ b/src/search/QuickOpen.js @@ -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())); this.$searchField.toggleClass("no-results", isNoResults); }; @@ -786,7 +786,6 @@ define(function (require, exports, module) { // Start fetching the file list, which will be needed the first time the user enters an un-prefixed query. If FileIndexManager's // caches are out of date, this list might take some time to asynchronously build. See searchFileList() for how this is handled. - fileList = null; fileListPromise = FileIndexManager.getFileInfoList("all") .done(function (files) { fileList = files; @@ -833,8 +832,11 @@ define(function (require, exports, module) { beginSearch("@", getCurrentEditorSelectedText()); } } - - + + // Listen for a change of project to invalidate our file list + $(ProjectManager).on("projectOpen", function () { + fileList = null; + }); // TODO: allow QuickOpenJS to register it's own commands and key bindings CommandManager.register(Strings.CMD_QUICK_OPEN, Commands.NAVIGATE_QUICK_OPEN, doFileSearch); From a34d98c18711901e791e1a9c1bd771d57c09f044 Mon Sep 17 00:00:00 2001 From: Kevin Dangoor Date: Wed, 27 Mar 2013 09:36:43 -0400 Subject: [PATCH 2/5] Reset StringMatcher caches when file list is retrieved. 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. --- src/search/QuickOpen.js | 5 ++++- src/utils/StringMatch.js | 17 ++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/search/QuickOpen.js b/src/search/QuickOpen.js index 0a51f3d67fc..0302b9202c6 100644 --- a/src/search/QuickOpen.js +++ b/src/search/QuickOpen.js @@ -473,6 +473,8 @@ define(function (require, exports, module) { // keeps listening to it anyway. So the last Promise to resolve "wins" the UI update even if it's for // a stale query. Guard from that by checking that filter text hasn't changed while we were waiting: if (!queryIsStale(query)) { + // New data, so we'll clear the matcher's caches + matcher.reset(); // We're still the current query. Synchronously re-run the search call and resolve with its results asyncResult.resolve(searchFileList(query, matcher)); } else { @@ -790,7 +792,8 @@ define(function (require, exports, module) { .done(function (files) { fileList = files; fileListPromise = null; - }); + this._filenameMatcher.reset(); + }.bind(this)); }; function getCurrentEditorSelectedText() { diff --git a/src/utils/StringMatch.js b/src/utils/StringMatch.js index 760dbdd382f..f8b282189b7 100644 --- a/src/utils/StringMatch.js +++ b/src/utils/StringMatch.js @@ -761,11 +761,7 @@ define(function (require, exports, module) { * (This object's caches are all stored in "_" prefixed properties.) */ function StringMatcher() { - // We keep track of the last query to know when we need to invalidate. - this._lastQuery = null; - - this._specialsCache = {}; - this._noMatchCache = {}; + this.reset(); } /** @@ -782,6 +778,17 @@ define(function (require, exports, module) { */ StringMatcher.prototype._noMatchCache = null; + /** + * Clears the caches. Use this in the event that the caches may be invalid. + */ + StringMatcher.prototype.reset = function () { + // We keep track of the last query to know when we need to invalidate. + this._lastQuery = null; + + this._specialsCache = {}; + this._noMatchCache = {}; + }; + /** * Performs a single match using the stringMatch function. See stringMatch for full documentation. * From 7641f188d754fadd544fcdf6e900903fa33c4b24 Mon Sep 17 00:00:00 2001 From: Kevin Dangoor Date: Wed, 3 Apr 2013 09:09:15 -0400 Subject: [PATCH 3/5] Addresses review feedback from @peterflynn. * Fixes the detection of no results * removes extra call to matcher.reset() * adds a test for StringMatcher.reset --- src/search/QuickOpen.js | 4 +--- test/spec/StringMatch-test.js | 36 ++++++++++++++++++++++------------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/search/QuickOpen.js b/src/search/QuickOpen.js index 0302b9202c6..bf4c0a6ff0e 100644 --- a/src/search/QuickOpen.js +++ b/src/search/QuickOpen.js @@ -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 && fileList && !this._isValidLineNumberQuery(this.$searchField.val())); + var isNoResults = (results.length === 0 && (fileList || currentPlugin) && !this._isValidLineNumberQuery(this.$searchField.val())); this.$searchField.toggleClass("no-results", isNoResults); }; @@ -473,8 +473,6 @@ define(function (require, exports, module) { // keeps listening to it anyway. So the last Promise to resolve "wins" the UI update even if it's for // a stale query. Guard from that by checking that filter text hasn't changed while we were waiting: if (!queryIsStale(query)) { - // New data, so we'll clear the matcher's caches - matcher.reset(); // We're still the current query. Synchronously re-run the search call and resolve with its results asyncResult.resolve(searchFileList(query, matcher)); } else { diff --git a/test/spec/StringMatch-test.js b/test/spec/StringMatch-test.js index 89eee836375..a75e9944a27 100644 --- a/test/spec/StringMatch-test.js +++ b/test/spec/StringMatch-test.js @@ -594,20 +594,20 @@ define(function (require, exports, module) { }); describe("StringMatcher", function () { + this.addMatchers({ + toBeInCache: function (matcher, cacheName) { + var value = matcher[cacheName][this.actual]; + var notText = this.isNot ? " not" : ""; + + this.message = function () { + return "Expected " + cacheName + " to" + notText + " contain key " + this.actual; + }; + + return value !== undefined; + } + }); + it("should manage its caches properly", function () { - this.addMatchers({ - toBeInCache: function (matcher, cacheName) { - var value = matcher[cacheName][this.actual]; - var notText = this.isNot ? " not" : ""; - - this.message = function () { - return "Expected " + cacheName + " to" + notText + " contain key " + this.actual; - }; - - return value !== undefined; - } - }); - var matcher = new StringMatch.StringMatcher(); expect(matcher._noMatchCache).toEqual({}); expect(matcher._specialsCache).toEqual({}); @@ -642,6 +642,16 @@ define(function (require, exports, module) { var lengthResult = matcher.match("length", "l"); expect(lengthResult).toBeTruthy(); }); + + it("can reset the caches", function () { + var matcher = new StringMatch.StringMatcher(); + matcher.match("foo", "spec/live"); + expect("foo").toBeInCache(matcher, "_specialsCache"); + expect("foo").toBeInCache(matcher, "_noMatchCache"); + matcher.reset(); + expect("foo").not.toBeInCache(matcher, "_specialsCache"); + expect("foo").not.toBeInCache(matcher, "_noMatchCache"); + }); }); }); }); From 9384b87750b344110b132c2e8f5e4b5d3d70e4da Mon Sep 17 00:00:00 2001 From: Kevin Dangoor Date: Thu, 4 Apr 2013 08:51:45 -0400 Subject: [PATCH 4/5] Eliminates "red" Quick Open field while waiting for initial index build --- src/search/QuickOpen.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/search/QuickOpen.js b/src/search/QuickOpen.js index bf4c0a6ff0e..094f024a872 100644 --- a/src/search/QuickOpen.js +++ b/src/search/QuickOpen.js @@ -390,9 +390,10 @@ define(function (require, exports, module) { * list items are re-rendered. Both happen synchronously just after we return. Called even when results is empty. */ QuickNavigateDialog.prototype._handleResultsReady = function (e, results) { - // Give visual clue when there are no results + // Give visual clue when there are no results (unless we're in "Go To Line" mode, where there + // are never results, or we're in file search mode and waiting for the index to get rebuilt) var isNoResults = (results.length === 0 && (fileList || currentPlugin) && !this._isValidLineNumberQuery(this.$searchField.val())); - this.$searchField.toggleClass("no-results", isNoResults); + this.$searchField.toggleClass("no-results", Boolean(isNoResults)); }; /** From f419483714035914ee51a9bfd20d2645fd56ace3 Mon Sep 17 00:00:00 2001 From: Kevin Dangoor Date: Thu, 4 Apr 2013 08:59:16 -0400 Subject: [PATCH 5/5] addMatchers needs to be in a beforeEach or a test. --- test/spec/StringMatch-test.js | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/test/spec/StringMatch-test.js b/test/spec/StringMatch-test.js index a75e9944a27..a6236d2b05f 100644 --- a/test/spec/StringMatch-test.js +++ b/test/spec/StringMatch-test.js @@ -594,17 +594,19 @@ define(function (require, exports, module) { }); describe("StringMatcher", function () { - this.addMatchers({ - toBeInCache: function (matcher, cacheName) { - var value = matcher[cacheName][this.actual]; - var notText = this.isNot ? " not" : ""; - - this.message = function () { - return "Expected " + cacheName + " to" + notText + " contain key " + this.actual; - }; - - return value !== undefined; - } + beforeEach(function () { + this.addMatchers({ + toBeInCache: function (matcher, cacheName) { + var value = matcher[cacheName][this.actual]; + var notText = this.isNot ? " not" : ""; + + this.message = function () { + return "Expected " + cacheName + " to" + notText + " contain key " + this.actual; + }; + + return value !== undefined; + } + }); }); it("should manage its caches properly", function () {