Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fixes #2608 (Quick Open shows red on first use) #3184

Merged
merged 5 commits into from
Apr 4, 2013
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/search/QuickOpen.js
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ define(function (require, exports, module) {
*/
QuickNavigateDialog.prototype._handleResultsReady = function (e, results) {
// Give visual clue when there are no results
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var isNoResults = (results.length === 0 && !this._isValidLineNumberQuery(this.$searchField.val()));
var isNoResults = (results.length === 0 && (fileList || currentPlugin) && !this._isValidLineNumberQuery(this.$searchField.val()));
this.$searchField.toggleClass("no-results", isNoResults);
};

Expand Down Expand Up @@ -786,12 +786,12 @@ 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;
fileListPromise = null;
});
this._filenameMatcher.reset();
}.bind(this));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand this right, nothing will actually refresh the results list once the new index has been built... but the next time the user changes the query text, even within the same Quick Open session, the results will start reflecting the new index?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also... do we need both this _filenameMatcher.reset() and matcher.reset() above? I think they will always be the same object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right that there's no automatic refresh until the user changes the query string. You're also right that only one call to .reset() is needed.

Do you think it's worthwhile to force a refresh when the file list updates? It's not immediately obvious to me how we'd do that, but I'm guessing there's got to be a way we can prod the smartautocomplete widget into acting as if the query changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think we need to bother with a forced refresh... just wanted to make sure I understood the intent of the code.

};

function getCurrentEditorSelectedText() {
Expand Down Expand Up @@ -833,8 +833,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);
Expand Down
17 changes: 12 additions & 5 deletions src/utils/StringMatch.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

/**
Expand All @@ -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.
*
Expand Down
36 changes: 23 additions & 13 deletions test/spec/StringMatch-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({});
Expand Down Expand Up @@ -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");
});
});
});
});