From 10f8896d46b5d2c501918aa641445997cf9768ad Mon Sep 17 00:00:00 2001 From: Erik Tierney Date: Thu, 18 Apr 2013 14:20:41 -0400 Subject: [PATCH] cleanup based on codereview comments: Update jsdoc use ExtensionUtils.getModulePath instead of manually slicing up path use FileUtils.readAsText instead of DocumentManager when we only need the text on disk Call CollectionUtils.hasProperty instead of Object.prototype.hasOwnProperty... Add comment explaining logic around finding location of called function for function type hinting remove unnecessary calls to setCursorPos remove stale comments in unittests pull out hint invalidation check into a function and remove check for query.length === 0, not necessary anymore --- .../JavaScriptCodeHints/ScopeManager.js | 49 ++++++++++++++----- .../default/JavaScriptCodeHints/Session.js | 12 +++++ .../default/JavaScriptCodeHints/main.js | 40 ++++++++------- .../JavaScriptCodeHints/tern-worker.js | 16 ++++++ .../default/JavaScriptCodeHints/unittests.js | 14 +++--- 5 files changed, 92 insertions(+), 39 deletions(-) diff --git a/src/extensions/default/JavaScriptCodeHints/ScopeManager.js b/src/extensions/default/JavaScriptCodeHints/ScopeManager.js index 60dfa861751..0c2506aea56 100644 --- a/src/extensions/default/JavaScriptCodeHints/ScopeManager.js +++ b/src/extensions/default/JavaScriptCodeHints/ScopeManager.js @@ -39,6 +39,8 @@ define(function (require, exports, module) { NativeFileSystem = brackets.getModule("file/NativeFileSystem").NativeFileSystem, ProjectManager = brackets.getModule("project/ProjectManager"), CollectionUtils = brackets.getModule("utils/CollectionUtils"), + ExtensionUtils = brackets.getModule("utils/ExtensionUtils"), + FileUtils = brackets.getModule("file/FileUtils"), HintUtils = require("HintUtils"); var ternEnvironment = [], @@ -50,8 +52,8 @@ define(function (require, exports, module) { ternPromise = null, resolvedFiles = {}, // file -> resolved file _ternWorker = (function () { - var path = module.uri.substring(0, module.uri.lastIndexOf("/") + 1); - return new Worker(path + "tern-worker.js"); + var path = ExtensionUtils.getModulePath(module, "tern-worker.js"); + return new Worker(path); }()); var MAX_TEXT_LENGTH = 1000000, // about 1MB @@ -83,16 +85,20 @@ define(function (require, exports, module) { * Read in the json files that have type information for the builtins, dom,etc */ function initTernEnv() { - var path = module.uri.substring(0, module.uri.lastIndexOf("/") + 1) + "thirdparty/tern/defs/", + var path = ExtensionUtils.getModulePath(module, "thirdparty/tern/defs/"), files = builtinFiles, library; files.forEach(function (i) { - DocumentManager.getDocumentForPath(path + i).done(function (document) { - library = JSON.parse(document.getText()); - builtinLibraryNames.push(library["!name"]); - ternEnvironment.push(library); - }).fail(function (error) { + NativeFileSystem.resolveNativeFileSystemPath(path + i, function (fileEntry) { + FileUtils.readAsText(fileEntry).done(function (text) { + library = JSON.parse(text); + builtinLibraryNames.push(library["!name"]); + ternEnvironment.push(library); + }).fail(function (error) { + console.log("failed to read tern config file " + i); + }); + }, function (error) { console.log("failed to read tern config file " + i); }); }); @@ -172,21 +178,22 @@ define(function (require, exports, module) { * Add a pending request waiting for the tern-worker to complete. * * @param {string} file - the name of the file + * @param {number} offset - the offset into the file the request is for * @param {string} type - the type of request - * @param {jQuery.Deferred} deferredRequest - the $.Deferred object to save + * @return {jQuery.Promise} - the promise for the request */ function addPendingRequest(file, offset, type) { var requests, key = file + "@" + offset, $deferredRequest; - if (Object.prototype.hasOwnProperty.call(pendingTernRequests, key)) { + if (CollectionUtils.hasProperty(pendingTernRequests, key)) { requests = pendingTernRequests[key]; } else { requests = {}; pendingTernRequests[key] = requests; } - if (Object.prototype.hasOwnProperty.call(requests, type)) { + if (CollectionUtils.hasProperty(requests, type)) { $deferredRequest = requests[type]; } else { requests[type] = $deferredRequest = $.Deferred(); @@ -196,6 +203,10 @@ define(function (require, exports, module) { /** * Get a Promise for the completions from TernJS, for the file & offset passed in. + * @param {string} dir - the directory the file is in + * @param {string} file - the name of the file + * @param {number} offset - the offset in the file the hints should be calculate at + * @param {string} text - the text of the file * @return {jQuery.Promise} - a promise that will resolve to an array of completions when * it is done */ @@ -214,7 +225,10 @@ define(function (require, exports, module) { /** * Get a Promise for all of the known properties from TernJS, for the directory and file. * The properties will be used as guesses in tern. - * + * @param {string} dir - the directory the file is in + * @param {string} file - the name of the file + * @param {number} offset - the offset in the file the hints should be calculate at + * @param {string} text - the text of the file * @return {jQuery.Promise} - a promise that will resolve to an array of properties when * it is done */ @@ -232,6 +246,13 @@ define(function (require, exports, module) { /** * Get a Promise for the function type from TernJS. + * @param {string} dir - the directory the file is in + * @param {string} file - the name of the file + * @param {{line:number, ch:number}} pos - the line, column info for what we want the function type of. + * Unfortunately tern requires line/col for this request instead of offset, but we cache all the request + * promises by file & offset, so we need the pos and offset for this method + * @param {number} offset - the offset in the file the hints should be calculate at + * @param {string} text - the text of the file * @return {jQuery.Promise} - a promise that will resolve to the function type of the function being called. */ function getTernFunctionType(dir, file, pos, offset, text) { @@ -256,6 +277,7 @@ define(function (require, exports, module) { * on some temporary context) should copy them first. See, e.g., * Session.getHints(). * + * @param {Session} session - the active hinting session * @param {Document} document - the document for which scope info is * desired * @param {number} offset - the offset into the document at which scope @@ -307,8 +329,9 @@ define(function (require, exports, module) { /** * Get any pending $.Deferred object waiting on the specified file and request type * @param {string} file - the file + * @param {number} offset - the offset in the file the request was at * @param {string} type - the type of request - * @param {jQuery.Deferred} - the $.Deferred for the request + * @return {jQuery.Deferred} - the $.Deferred for the request */ function getPendingRequest(file, offset, type) { var key = file + "@" + offset; diff --git a/src/extensions/default/JavaScriptCodeHints/Session.js b/src/extensions/default/JavaScriptCodeHints/Session.js index ea1337800cd..e03556d80c0 100644 --- a/src/extensions/default/JavaScriptCodeHints/Session.js +++ b/src/extensions/default/JavaScriptCodeHints/Session.js @@ -244,6 +244,8 @@ define(function (require, exports, module) { token = this.getToken(cursor); if (token) { + // if this token is part of a function call, then the tokens lexical info + // will be annotated with "call" if (token.state.lexical.info === "call") { inFunctionCall = true; if (this.getQuery().length > 0) { @@ -251,6 +253,16 @@ define(function (require, exports, module) { showFunctionType = false; } else { showFunctionType = true; + // we need to find the location of the called function so that we can request the functions type. + // the token's lexical info will contain the column where the open "(" for the + // function call occurrs, but for whatever reason it does not have the line, so + // we have to walk back and try to find the correct location. We do this by walking + // up the lines starting with the line the token is on, and seeing if any of the lines + // have "(" at the column indicated by the tokens lexical state. + // We walk back 9 lines, as that should be far enough to find most function calls, + // and it will prevent us from walking back thousands of lines if something went wrong. + // there is nothing magical about 9 lines, and it can be adjusted if it doesn't seem to be + // working well var col = token.state.lexical.column, line, e, diff --git a/src/extensions/default/JavaScriptCodeHints/main.js b/src/extensions/default/JavaScriptCodeHints/main.js index 90f88981565..fee811023e6 100644 --- a/src/extensions/default/JavaScriptCodeHints/main.js +++ b/src/extensions/default/JavaScriptCodeHints/main.js @@ -160,6 +160,23 @@ define(function (require, exports, module) { function JSHints() { } + /** + * determine if the cached hint information should be invalidated and re-calculated + * + * @param {Session} session - the active hinting session + * @return {boolean} - true if the hints should be recalculated + */ + JSHints.prototype.needNewHints = function (session) { + var cursor = session.getCursor(), + type = session.getType(); + + return !cachedHints || + cachedLine !== cursor.line || + type.property !== cachedType.property || + type.context !== cachedType.context || + type.showFunctionType !== cachedType.showFunctionType; + }; + /** * Determine whether hints are available for a given editor context * @@ -178,15 +195,7 @@ define(function (require, exports, module) { type = session.getType(), query = session.getQuery(); - // Invalidate cached information if: 1) no scope exists; 2) the - // cursor has moved a line; 3) the scope is dirty; or 4) if the - // cursor has moved into a different scope. Cached information - // is also reset on editor change. - if (!cachedHints || - cachedLine !== cursor.line || - type.property !== cachedType.property || - type.context !== cachedType.context || - type.showFunctionType !== cachedType.showFunctionType) { + if (this.needNewHints(session)) { //console.log("clear hints"); cachedLine = null; cachedHints = null; @@ -215,10 +224,7 @@ define(function (require, exports, module) { // Compute fresh hints if none exist, or if the session // type has changed since the last hint computation - if (!cachedHints || - type.property !== cachedType.property || - type.context !== cachedType.context || - type.showFunctionType !== cachedType.showFunctionType || query.length === 0) { + if (this.needNewHints(session)) { var offset = session.getOffset(), scopeResponse = ScopeManager.requestHints(session, session.editor.document, offset), self = this; @@ -398,15 +404,11 @@ define(function (require, exports, module) { if (resolvedPath) { CommandManager.execute(Commands.FILE_OPEN, {fullPath: resolvedPath}) .done(function () { - session.editor.setCursorPos(jumpResp.start); - session.editor.setSelection(jumpResp.start, jumpResp.end); - session.editor.centerOnCursor(); + session.editor.setSelection(jumpResp.start, jumpResp.end, true); }); } } else { - session.editor.setCursorPos(jumpResp.start); - session.editor.setSelection(jumpResp.start, jumpResp.end); - session.editor.centerOnCursor(); + session.editor.setSelection(jumpResp.start, jumpResp.end, true); } } diff --git a/src/extensions/default/JavaScriptCodeHints/tern-worker.js b/src/extensions/default/JavaScriptCodeHints/tern-worker.js index 8409fe6b79d..34b78c607b0 100644 --- a/src/extensions/default/JavaScriptCodeHints/tern-worker.js +++ b/src/extensions/default/JavaScriptCodeHints/tern-worker.js @@ -46,6 +46,9 @@ importScripts("thirdparty/requirejs/require.js"); /** * Provide the contents of the requested file to tern + * @param {string} name - the name of the file + * @param {Function} next - the function to call with the text of the file + * once it has been read in. */ function getFile(name, next) { // save the callback @@ -73,6 +76,11 @@ importScripts("thirdparty/requirejs/require.js"); /** * Create a new tern server. + * + * @param {Object} env - an Object with the environment, as read in from + * the json files in thirdparty/tern/defs + * @param {string} dir - the current directory + * @param {Array.} files - a list of filenames tern should be aware of */ function initTernServer(env, dir, files) { var ternOptions = { @@ -89,6 +97,14 @@ importScripts("thirdparty/requirejs/require.js"); } + /** + * Build an object that can be used as a request to tern + * @param {string} dir - the current directory + * @param {string} file - the filename the request is in + * @param {string} query - the type of request being made + * @param {number} offset - the offset in the file the request is at + * @param {string} text - the text of the file + */ function buildRequest(dir, file, query, offset, text) { query = {type: query}; query.start = offset; diff --git a/src/extensions/default/JavaScriptCodeHints/unittests.js b/src/extensions/default/JavaScriptCodeHints/unittests.js index 503752ba9aa..bf72a48d7cf 100644 --- a/src/extensions/default/JavaScriptCodeHints/unittests.js +++ b/src/extensions/default/JavaScriptCodeHints/unittests.js @@ -507,7 +507,7 @@ define(function (require, exports, module) { testEditor.setCursorPos(start); var hintObj = expectHints(JSCodeHints.jsHintProvider); - selectHint(JSCodeHints.jsHintProvider, hintObj, "A2"); // hint 2 is "A2" + selectHint(JSCodeHints.jsHintProvider, hintObj, "A2"); runs(function () { //expect(testEditor.getCursorPos()).toEqual(end); expect(testDoc.getRange(start, end)).toEqual("A2"); @@ -522,7 +522,7 @@ define(function (require, exports, module) { testEditor.setCursorPos(start); var hintObj = expectHints(JSCodeHints.jsHintProvider); hintsPresent(hintObj, ["A1", "A2", "A3"]); - selectHint(JSCodeHints.jsHintProvider, hintObj, "A1"); // hint 1 is "A1" + selectHint(JSCodeHints.jsHintProvider, hintObj, "A1"); runs(function () { //expect(testEditor.getCursorPos()).toEqual(end); expect(testDoc.getRange(before, end)).toEqual("A1"); @@ -537,7 +537,7 @@ define(function (require, exports, module) { testDoc.replaceRange("A1.", start, start); testEditor.setCursorPos(middle); var hintObj = expectHints(JSCodeHints.jsHintProvider); - selectHint(JSCodeHints.jsHintProvider, hintObj, "propA"); // hint 0 is "propA" + selectHint(JSCodeHints.jsHintProvider, hintObj, "propA"); runs(function () { expect(testEditor.getCursorPos()).toEqual(end); @@ -554,7 +554,7 @@ define(function (require, exports, module) { testDoc.replaceRange("A1.prop", start, start); testEditor.setCursorPos(middle); var hintObj = expectHints(JSCodeHints.jsHintProvider); - selectHint(JSCodeHints.jsHintProvider, hintObj, "propA"); // hint 0 is "propA" + selectHint(JSCodeHints.jsHintProvider, hintObj, "propA"); runs(function () { expect(testEditor.getCursorPos()).toEqual(end); @@ -571,7 +571,7 @@ define(function (require, exports, module) { testDoc.replaceRange("A1.pro", start, start); testEditor.setCursorPos(middle); var hintObj = expectHints(JSCodeHints.jsHintProvider); - selectHint(JSCodeHints.jsHintProvider, hintObj, "propA"); // hint 0 is "propA" + selectHint(JSCodeHints.jsHintProvider, hintObj, "propA"); runs(function () { expect(testEditor.getCursorPos()).toEqual(end); expect(testDoc.getRange(start, end)).toEqual("A1.propA"); @@ -587,7 +587,7 @@ define(function (require, exports, module) { testDoc.replaceRange("A1.propB", start, start); testEditor.setCursorPos(middle); var hintObj = expectHints(JSCodeHints.jsHintProvider); - selectHint(JSCodeHints.jsHintProvider, hintObj, "propA"); // hint 0 is "propA" + selectHint(JSCodeHints.jsHintProvider, hintObj, "propA"); runs(function () { expect(testEditor.getCursorPos()).toEqual(end); expect(testDoc.getRange(start, end)).toEqual("A1.propA"); @@ -604,7 +604,7 @@ define(function (require, exports, module) { testDoc.replaceRange("(A1.prop)", start, start); testEditor.setCursorPos(middle); var hintObj = expectHints(JSCodeHints.jsHintProvider); - selectHint(JSCodeHints.jsHintProvider, hintObj, "propA"); // hint 0 is "propA" + selectHint(JSCodeHints.jsHintProvider, hintObj, "propA"); runs(function () { expect(testEditor.getCursorPos()).toEqual(end);