From 88ea4554cce85fefb4ca71aa759c8af0273665c5 Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Tue, 5 Nov 2013 18:21:20 -0800 Subject: [PATCH 01/14] - added support for multiple code linter per file type --- src/htmlContent/problems-panel-table.html | 9 +- src/htmlContent/problems-panel.html | 4 +- src/language/CodeInspection.js | 255 +++++++++++----- src/nls/root/strings.js | 1 + src/styles/brackets.less | 38 +++ src/utils/Async.js | 43 +++ test/UnitTestSuite.js | 1 + .../spec/CodeInspection-test-files/errors.css | 3 + test/spec/CodeInspection-test-files/errors.js | 2 + .../CodeInspection-test-files/no-errors.js | 3 + test/spec/CodeInspection-test.js | 271 ++++++++++++++++++ 11 files changed, 562 insertions(+), 68 deletions(-) create mode 100644 test/spec/CodeInspection-test-files/errors.css create mode 100644 test/spec/CodeInspection-test-files/errors.js create mode 100644 test/spec/CodeInspection-test-files/no-errors.js create mode 100644 test/spec/CodeInspection-test.js diff --git a/src/htmlContent/problems-panel-table.html b/src/htmlContent/problems-panel-table.html index d8cc2b001a6..d1d5970eb43 100644 --- a/src/htmlContent/problems-panel-table.html +++ b/src/htmlContent/problems-panel-table.html @@ -1,11 +1,16 @@ {{#reportList}} + + + + {{#results}} - - + + + {{/results}} {{/reportList}}
{{providerName}} ({{numProblems}})
{{friendlyLine}}{{message}}{{codeSnippet}}{{message}}{{codeSnippet}}
diff --git a/src/htmlContent/problems-panel.html b/src/htmlContent/problems-panel.html index 6c5b06906a6..e0eea02f28e 100644 --- a/src/htmlContent/problems-panel.html +++ b/src/htmlContent/problems-panel.html @@ -1,7 +1,7 @@
-
+
×
-
+ \ No newline at end of file diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 9fa2181a264..079ccf39bd5 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -46,6 +46,7 @@ define(function (require, exports, module) { DocumentManager = require("document/DocumentManager"), EditorManager = require("editor/EditorManager"), FileUtils = require("file/FileUtils"), + CollectionUtils = require("utils/CollectionUtils"), LanguageManager = require("language/LanguageManager"), PreferencesManager = require("preferences/PreferencesManager"), PerfUtils = require("utils/PerfUtils"), @@ -54,6 +55,8 @@ define(function (require, exports, module) { AppInit = require("utils/AppInit"), Resizer = require("utils/Resizer"), StatusBar = require("widgets/StatusBar"), + Menus = require("command/Menus"), + Async = require("utils/Async"), PanelTemplate = require("text!htmlContent/problems-panel.html"), ResultsTemplate = require("text!htmlContent/problems-panel-table.html"); @@ -123,6 +126,12 @@ define(function (require, exports, module) { * @type {?Array.} */ var _lastResult; + + /** + * @private + * @type {?Array.} + */ + var _allInspectorCommands = []; /** * Enable or disable the "Go to First Error" command @@ -133,7 +142,57 @@ define(function (require, exports, module) { _gotoEnabled = gotoEnabled; } + /** + * Construct a preference key for the provider. + * limitation: this function doesn't account for provider with the same name, which could + * result in preferences from one provider overwritten with the ones from another. + * + * @param {name:string, scanFile:function(string, string):Object} provider + */ + function getProviderPrefKey(provider) { + return "provider." + provider.name + ".enabled"; + } + + /** + * Check if a given provider is enabled. + * Return true if enabled, false otherwise. + * + * @param {name:string, scanFile:function(string, string):Object} provider + */ + function getProviderState(provider) { + return _prefs.getValue(getProviderPrefKey(provider)); + } + /** + * Create a menu entry for the given provider/code inspector. + * The command that is created for this menu entry will be stored for later use. The event handler for this new menu item will handle the enable/disable toggle for the provider/code inspector. + * + * @param {name:string, scanFile:function(string, string):Object} provider + */ + function addMenuEntryForProvider(provider) { + var menuString = StringUtils.format(Strings.CMD_VIEW_ENABLE_INSPECTOR, provider.name), + commandString = "command.inspector." + provider.name; + + var inspectorCommand = CommandManager.register(menuString, commandString, function () { + this.setChecked(!this.getChecked()); + + _prefs.setValue(getProviderPrefKey(provider), this.getChecked()); + + // update results + run(); + }); + + _allInspectorCommands.push(inspectorCommand); +// +// // add a new MenuItem for each inspector +// var viewMenu = Menus.getMenu(Menus.AppMenuBar.VIEW_MENU); +// viewMenu.addMenuItem(inspectorCommand, null, Menus.AFTER, Commands.VIEW_TOGGLE_INSPECTION); +// +// var providerState = getProviderState(provider); +// inspectorCommand.setChecked(providerState); +// inspectorCommand.setEnabled(_enabled); + } + /** * The provider is passed the text of the file and its fullPath. Providers should not assume * that the file is open (i.e. DocumentManager.getOpenDocumentForPath() may return null) or @@ -146,12 +205,20 @@ define(function (require, exports, module) { * If type is unspecified, Type.WARNING is assumed. */ function register(languageId, provider) { - if (_providers[languageId]) { - console.warn("Overwriting existing inspection/linting provider for language " + languageId); + if (!_providers[languageId]) { + _providers[languageId] = []; } - _providers[languageId] = provider; + + _providers[languageId].push(provider); + + addMenuEntryForProvider(provider); } + function unregisterAll() { + CollectionUtils.forEach(_providers, function (languageId) { delete _providers[languageId]; }); + _providers = {}; + } + /** * Returns a provider for given file path, if one is available. * Decision is made depending on the file extension. @@ -174,9 +241,10 @@ define(function (require, exports, module) { */ function inspectFile(fileEntry, provider) { var response = new $.Deferred(); - provider = provider || getProviderForPath(fileEntry.fullPath); + // TODO: handle list case + var providerList = provider || getProviderForPath(fileEntry.fullPath); - if (!provider) { + if (!providerList && !providerList.length) { response.resolve(null); return response.promise(); } @@ -193,18 +261,32 @@ define(function (require, exports, module) { fileTextPromise .done(function (fileText) { var result, - perfTimerInspector = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + fileEntry.fullPath); + perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + fileEntry.fullPath); +// perfTimerInspector = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + fileEntry.fullPath); - try { - result = provider.scanFile(fileText, fileEntry.fullPath); - } catch (err) { - console.error("[CodeInspection] Provider " + provider.name + " threw an error: " + err); - response.reject(err); - return; - } + var masterPromise = Async.doInParallel_aggregateResults(providerList, function (provider, providerNum) { + var result = new $.Deferred(); + + try { + var scanResult = provider.scanFile(fileText, fileEntry.fullPath); + result.resolve(scanResult); + } catch (err) { + console.error("[CodeInspection] Provider " + provider.name + " threw an error: " + err); + response.reject(err); + return; + } + + return result.promise(); + }); + masterPromise.done(function (results) { + response.resolve(results); + }).fail(function (err) { + response.reject(err); + }); + PerfUtils.addMeasurement(perfTimerInspector); - response.resolve(result); +// response.resolve(result); }) .fail(function (err) { console.error("[CodeInspection] Could not read file for inspection: " + fileEntry.fullPath); @@ -228,65 +310,95 @@ define(function (require, exports, module) { } var currentDoc = DocumentManager.getCurrentDocument(), - provider = currentDoc && getProviderForPath(currentDoc.file.fullPath); + providerList = currentDoc && getProviderForPath(currentDoc.file.fullPath); - if (provider) { - inspectFile(currentDoc.file, provider).then(function (result) { + if (providerList && providerList.length) { + var numProblems = 0; + var aborted = false; + var _numProblemsReportedByProvider = 0; + var allErrors = { errors: [] }; + + inspectFile(currentDoc.file, providerList.slice(0)).then(function (results) { // check if current document wasn't changed while inspectFile was running if (currentDoc !== DocumentManager.getCurrentDocument()) { return; } - _lastResult = result; + // Aggregate all results + var errors = results.reduce(function (a, item) { return a + (item.results ? item.results.errors.length : 0); }, 0); + + _lastResult = results; + _lastResult.errors = errors; - if (!result || !result.errors.length) { +// if (!result || !result.errors.length) { + if (!results || !errors) { Resizer.hide($problemsPanel); - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-valid", StringUtils.format(Strings.NO_ERRORS, provider.name)); + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-valid", StringUtils.format(Strings.NO_ERRORS, "provider.name")); setGotoEnabled(false); return; } - var perfTimerDOM = PerfUtils.markStart("ProblemsPanel render:\t" + currentDoc.file.fullPath); - // Augment error objects with additional fields needed by Mustache template - var numProblems = 0; - result.errors.forEach(function (error) { - error.friendlyLine = error.pos.line + 1; - error.codeSnippet = currentDoc.getLine(error.pos.line); - error.codeSnippet = error.codeSnippet.substr(0, Math.min(175, error.codeSnippet.length)); // limit snippet width - - if (error.type !== Type.META) { - numProblems++; + results.forEach(function (result) { + _numProblemsReportedByProvider = 0; + var provider = result.item; + + if (result.results) { + result.results.errors.forEach(function (error) { + error.friendlyLine = error.pos.line + 1; + error.codeSnippet = currentDoc.getLine(error.pos.line); + error.codeSnippet = error.codeSnippet.substr(0, Math.min(175, error.codeSnippet.length)); // limit snippet width + + if (error.type !== Type.META) { + numProblems++; + _numProblemsReportedByProvider++; + } + }); + + // if the code inspector was unable to process the whole file, we keep track to show a different status + if (result && result.aborted) { + aborted = true; + } + + allErrors.errors.push({ + providerName: provider.name, + results: result.results.errors, + numProblems: _numProblemsReportedByProvider + }); } }); - - // Update results table - var html = Mustache.render(ResultsTemplate, {reportList: result.errors}); - - $problemsPanelTable - .empty() - .append(html) - .scrollTop(0); // otherwise scroll pos from previous contents is remembered - - $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE, provider.name)); - if (!_collapsed) { - Resizer.show($problemsPanel); - } - - if (numProblems === 1 && !result.aborted) { - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", StringUtils.format(Strings.SINGLE_ERROR, provider.name)); - } else { - // If inspector was unable to process the whole file, number of errors is indeterminate; indicate with a "+" - if (result.aborted) { - numProblems += "+"; - } - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", - StringUtils.format(Strings.MULTIPLE_ERRORS, provider.name, numProblems)); + }); + + // Update results table +// var html = Mustache.render(ResultsTemplate, {reportList: results.errors}); + var perfTimerDOM = PerfUtils.markStart("ProblemsPanel render:\t" + currentDoc.file.fullPath); + var provider = {name: "Code Inspection"}; + + var html = Mustache.render(ResultsTemplate, {reportList: allErrors.errors}); + + $problemsPanelTable + .empty() + .append(html) + .scrollTop(0); // otherwise scroll pos from previous contents is remembered + + $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE, provider.name)); + if (!_collapsed) { + Resizer.show($problemsPanel); + } + + if (numProblems === 1 && !aborted) { + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", StringUtils.format(Strings.SINGLE_ERROR, provider.name)); + } else { + // If inspector was unable to process the whole file, number of errors is indeterminate; indicate with a "+" + if (aborted) { + numProblems += "+"; } - setGotoEnabled(true); + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", + StringUtils.format(Strings.MULTIPLE_ERRORS, provider.name, numProblems)); + } + setGotoEnabled(true); - PerfUtils.addMeasurement(perfTimerDOM); - }); + PerfUtils.addMeasurement(perfTimerDOM); } else { // No provider for current file _lastResult = null; @@ -358,7 +470,7 @@ define(function (require, exports, module) { if (_collapsed) { Resizer.hide($problemsPanel); } else { - if (_lastResult && _lastResult.errors.length) { + if (_lastResult && _lastResult.errors) { Resizer.show($problemsPanel); } } @@ -396,13 +508,25 @@ define(function (require, exports, module) { $selectedRow = $(e.currentTarget); $selectedRow.addClass("selected"); - var lineTd = $selectedRow.find(".line-number"); - var line = parseInt(lineTd.text(), 10) - 1; // convert friendlyLine back to pos.line - var character = lineTd.data("character"); - var editor = EditorManager.getCurrentFullEditor(); - editor.setCursorPos(line, character, true); - EditorManager.focusEditor(); + // This is a inspector title row, expand/collapse on click + if ($selectedRow.hasClass("inspector-section")) { + // Clicking the inspector title section header collapses/expands result rows + $selectedRow.nextUntil(".inspector-section").toggle(); + + var $triangle = $(".disclosure-triangle", $selectedRow); + $triangle.toggleClass("expanded").toggleClass("collapsed"); + } else { + // This is a problem marker row, show the result on click + // Grab the required position data + var lineTd = $selectedRow.find(".line-number"); + var line = parseInt(lineTd.text(), 10) - 1; // convert friendlyLine back to pos.line + var character = lineTd.data("character"); + + var editor = EditorManager.getCurrentFullEditor(); + editor.setCursorPos(line, character, true); + EditorManager.focusEditor(); + } }); $("#problems-panel .close").click(function () { @@ -416,7 +540,8 @@ define(function (require, exports, module) { $("#status-inspection").click(function () { // Clicking indicator toggles error panel, if any errors in current file - if (_lastResult && _lastResult.errors.length) { +// if (_lastResult && _lastResult.errors.length) { + if (_lastResult) { toggleCollapsed(); } }); @@ -427,6 +552,8 @@ define(function (require, exports, module) { toggleCollapsed(_prefs.getValue("collapsed")); }); + // Testing + exports.unregisterAll = unregisterAll; // Public API exports.register = register; diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index 495dedcd199..8af19ee781d 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -265,6 +265,7 @@ define({ "CMD_TOGGLE_WORD_WRAP" : "Word Wrap", "CMD_LIVE_HIGHLIGHT" : "Live Preview Highlight", "CMD_VIEW_TOGGLE_INSPECTION" : "Lint Files on Save", + "CMD_VIEW_ENABLE_INSPECTOR" : " Enable {0}", "CMD_SORT_WORKINGSET_BY_ADDED" : "Sort by Added", "CMD_SORT_WORKINGSET_BY_NAME" : "Sort by Name", "CMD_SORT_WORKINGSET_BY_TYPE" : "Sort by Type", diff --git a/src/styles/brackets.less b/src/styles/brackets.less index 4cf338cfda3..b30ed86654f 100644 --- a/src/styles/brackets.less +++ b/src/styles/brackets.less @@ -1092,8 +1092,46 @@ a, img { .line { text-align: right; // make line number line up with editor line numbers } + + td { + padding-left: 3px; + vertical-align: baseline; + } + + .line-number { + color: @tc-light-weight-quiet-text; + font-family: SourceCodePro; + font-size: 11px; + padding: 0 10px 0 14px; + text-align: right; + white-space: nowrap; + } + + .line-text { + white-space: nowrap; + width: auto; + } + + .line-snippet { + white-space: nowrap; + width: 100%; + padding-left: 10px; + } } +#problems-panel .disclosure-triangle { + .jstree-sprite; + display: inline-block; + &.expanded { + // Unfortunately, the way jsTree sprites are aligned within their 18px boxes doesn't look good in + // other contexts, so we need some tweaks here instead of straight multiples of @jstree-sprite-size + background-position: 7px 5px; + -webkit-transform: translateZ(0) rotate(90deg); + } + &.collapsed { + background-position: 7px 5px; + } +} /* Line up label text and input text */ label input { diff --git a/src/utils/Async.js b/src/utils/Async.js index 95b96669f97..ea97e078a91 100644 --- a/src/utils/Async.js +++ b/src/utils/Async.js @@ -281,6 +281,48 @@ define(function (require, exports, module) { return masterDeferred.promise(); } + /** + * Executes a series of tasks in parallel, saving up error info from any that fail along the way. + * Returns a Promise that is only resolved/rejected once all tasks are complete. This is + * essentially a wrapper around doInParallel(..., false). + * + * If one or more tasks failed, the entire "master" promise is rejected at the end - with one + * argument: an array objects, one per failed task. Each error object contains: + * - item -- the entry in items whose task failed + * - error -- the first argument passed to the fail() handler when the task failed + * + * @param {!Array.<*>} items + * @param {!function(*, number):Promise} beginProcessItem + * @return {$.Promise} + */ + function doInParallel_aggregateResults(items, beginProcessItem) { + var results = []; + + var masterDeferred = new $.Deferred(); + + var parallelResult = doInParallel( + items, + function (item, i) { + var itemResult = beginProcessItem(item, i); + itemResult.done(function (result) { + results.push({ item: item, results: result }); + }); + return itemResult; + }, + false + ); + + parallelResult + .done(function () { + masterDeferred.resolve(results); + }) + .fail(function () { + masterDeferred.reject(); + }); + + return masterDeferred.promise(); + } + /** Value passed to fail() handlers that have been triggered due to withTimeout()'s timeout */ var ERROR_TIMEOUT = {}; @@ -450,6 +492,7 @@ define(function (require, exports, module) { exports.doSequentially = doSequentially; exports.doSequentiallyInBackground = doSequentiallyInBackground; exports.doInParallel_aggregateErrors = doInParallel_aggregateErrors; + exports.doInParallel_aggregateResults = doInParallel_aggregateResults; exports.withTimeout = withTimeout; exports.ERROR_TIMEOUT = ERROR_TIMEOUT; exports.chain = chain; diff --git a/test/UnitTestSuite.js b/test/UnitTestSuite.js index b1b43bbf9a9..16e2a1e1461 100644 --- a/test/UnitTestSuite.js +++ b/test/UnitTestSuite.js @@ -29,6 +29,7 @@ define(function (require, exports, module) { require("spec/Async-test"); require("spec/CodeHint-test"); require("spec/CodeHintUtils-test"); + require("spec/CodeInspection-test"); require("spec/CommandManager-test"); require("spec/CSSUtils-test"); require("spec/JSUtils-test"); diff --git a/test/spec/CodeInspection-test-files/errors.css b/test/spec/CodeInspection-test-files/errors.css new file mode 100644 index 00000000000..061a1f9fc5f --- /dev/null +++ b/test/spec/CodeInspection-test-files/errors.css @@ -0,0 +1,3 @@ +h1 { + color: black; + \ No newline at end of file diff --git a/test/spec/CodeInspection-test-files/errors.js b/test/spec/CodeInspection-test-files/errors.js new file mode 100644 index 00000000000..8254868cb3d --- /dev/null +++ b/test/spec/CodeInspection-test-files/errors.js @@ -0,0 +1,2 @@ +// mispelled function keyword +funtion foo() {}; \ No newline at end of file diff --git a/test/spec/CodeInspection-test-files/no-errors.js b/test/spec/CodeInspection-test-files/no-errors.js new file mode 100644 index 00000000000..fff4bd0ac3e --- /dev/null +++ b/test/spec/CodeInspection-test-files/no-errors.js @@ -0,0 +1,3 @@ +function foo() { + "use strict"; +} \ No newline at end of file diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js new file mode 100644 index 00000000000..6a7118f903b --- /dev/null +++ b/test/spec/CodeInspection-test.js @@ -0,0 +1,271 @@ +/* + * Copyright (c) 2013 Adobe Systems Incorporated. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + */ + +/*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */ +/*global define, describe, it, expect, beforeEach, afterEach, waitsFor, runs, brackets, waitsForDone, spyOn */ + +define(function (require, exports, module) { + "use strict"; + + var SpecRunnerUtils = require("spec/SpecRunnerUtils"), + NativeFileSystem = require("file/NativeFileSystem").NativeFileSystem; + + describe("Code Inspection", function () { + this.category = "integration"; + + var testFolder = SpecRunnerUtils.getTestPath("/spec/CodeInspection-test-files/"), + testWindow, + $, + brackets, + CodeInspection, + EditorManager, + previousState; + + var toggleJSLintResults = function (visible) { + $("#status-inspection").triggerHandler("click"); + expect($("#problems-panel").is(":visible")).toBe(visible); + }; + + beforeEach(function () { + runs(function () { + SpecRunnerUtils.createTestWindowAndRun(this, function (w) { + testWindow = w; + // Load module instances from brackets.test + $ = testWindow.$; + brackets = testWindow.brackets; + EditorManager = brackets.test.EditorManager; + CodeInspection = brackets.test.CodeInspection; + CodeInspection.toggleEnabled(true); + // enable JSLint and preserve the previous state before we enable it for testing +// previousState = CodeInspection._getProviderState({name: "JSLint"}); +// CodeInspection._setProviderState({name: "JSLint"}, true); + }); + }); + + runs(function () { + SpecRunnerUtils.loadProjectInTestWindow(testFolder); + }); + }); + + afterEach(function () { + testWindow = null; + $ = null; + brackets = null; + EditorManager = null; + SpecRunnerUtils.closeTestWindow(); + + // revert to previous state +// CodeInspection._setProviderState({name: 'JSLint'}, previousState); + }); + + it("should run one linter when a javascript document opens", function () { + var called = false, + promise; + + CodeInspection.unregisterAll(); + CodeInspection.register("javascript", { + name: "text linter", + scanFile: function (text, fullPath) { called = true; return {errors: []}; } + }); + + runs(function () { + var simpleTextFileEntry = new NativeFileSystem.FileEntry(testFolder + "/errors.js"); + promise = CodeInspection.inspectFile(simpleTextFileEntry); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + expect(called).toBeTruthy(); + }); + }); + + it("should run two linters when a javascript document opens", function () { + var linterOneCalled = false, + linterTwoCalled = false, + promise; + + CodeInspection.unregisterAll(); + CodeInspection.register("javascript", { + name: "text linter 1", + scanFile: function (text, fullPath) { linterOneCalled = true; return {errors: []}; } + }); + CodeInspection.register("javascript", { + name: "text linter 2", + scanFile: function (text, fullPath) { linterTwoCalled = true; return {errors: []}; } + }); + + runs(function () { + var simpleTextFileEntry = new NativeFileSystem.FileEntry(testFolder + "/errors.js"); + promise = CodeInspection.inspectFile(simpleTextFileEntry); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + expect(linterOneCalled).toBeTruthy(); + expect(linterTwoCalled).toBeTruthy(); + }); + }); + + it("should run one linter when a javascript document opens and return some errors", function () { + var called = false, + result, + promise; + + var lintResult = { + pos: { line: 2, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }; + + CodeInspection.unregisterAll(); + CodeInspection.register("javascript", { + name: "text linter", + scanFile: function (text, fullPath) { + called = true; + return { + errors: [lintResult] + }; + } + }); + + runs(function () { + var simpleTextFileEntry = new NativeFileSystem.FileEntry(testFolder + "/errors.js"); + promise = CodeInspection.inspectFile(simpleTextFileEntry); + promise.done(function (lintingResult) { + result = lintingResult; + }); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + expect(called).toBeTruthy(); + expect(result.length).toEqual(1); + expect(result[0].item.name).toEqual("text linter"); + expect(result[0].results.errors.length).toEqual(1); + }); + }); + + it("should run two linter when a javascript document opens and return some errors", function () { + var result, + promise; + + var lintResult = { + pos: { line: 2, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }; + + CodeInspection.unregisterAll(); + CodeInspection.register("javascript", { + name: "text linter 1", + scanFile: function (text, fullPath) { + return { + errors: [lintResult] + }; + } + }); + + CodeInspection.register("javascript", { + name: "text linter 2", + scanFile: function (text, fullPath) { + return { + errors: [lintResult] + }; + } + }); + + runs(function () { + var simpleTextFileEntry = new NativeFileSystem.FileEntry(testFolder + "/errors.js"); + promise = CodeInspection.inspectFile(simpleTextFileEntry); + promise.done(function (lintingResult) { + result = lintingResult; + }); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + expect(result.length).toEqual(2); + expect(result[0].results.errors.length).toEqual(1); + expect(result[1].results.errors.length).toEqual(1); + }); + }); + + describe("Check the Code Inspection UI", function () { + beforeEach(function () { + CodeInspection.unregisterAll(); + + var lintResult = { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }; + + CodeInspection.register("javascript", { + name: "text linter 1", + scanFile: function (text, fullPath) { + return { + errors: [lintResult] + }; + } + }); + }); + + it("should run test linter when a JavaScript document opens and indicate errors in panel", function () { + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); + + runs(function () { + expect($("#problems-panel").is(":visible")).toBe(true); + var $statusBar = $("#status-inspection"); + expect($statusBar.is(":visible")).toBe(true); + }); + }); + }); + + + xit("status icon should toggle Errors panel when errors present", function () { +// waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + runs(function () { + var promise = SpecRunnerUtils.openProjectFiles(["errors.js"]); + waitsForDone(promise); + }); + + runs(function () { + toggleJSLintResults(false); + toggleJSLintResults(true); + }); + }); + + xit("status icon should not toggle Errors panel when no errors present", function () { + waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file"); + + runs(function () { + toggleJSLintResults(false); + toggleJSLintResults(false); + }); + }); + }); +}); From 0b6fb395e26a3239e5f5dafdbec54c5eae123047 Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Fri, 8 Nov 2013 16:27:04 -0800 Subject: [PATCH 02/14] - added some unit tests for Code Linting - added a new string for the Code Linting Result Panel Title --- src/language/CodeInspection.js | 78 ++++----- src/nls/root/strings.js | 1 + src/utils/Async.js | 2 +- test/spec/CodeInspection-test.js | 269 +++++++++++++++++-------------- 4 files changed, 194 insertions(+), 156 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 079ccf39bd5..5f06f83d340 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -241,8 +241,7 @@ define(function (require, exports, module) { */ function inspectFile(fileEntry, provider) { var response = new $.Deferred(); - // TODO: handle list case - var providerList = provider || getProviderForPath(fileEntry.fullPath); + var providerList = (provider || getProviderForPath(fileEntry.fullPath)) || []; if (!providerList && !providerList.length) { response.resolve(null); @@ -260,13 +259,13 @@ define(function (require, exports, module) { fileTextPromise .done(function (fileText) { - var result, - perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + fileEntry.fullPath); -// perfTimerInspector = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + fileEntry.fullPath); + var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + fileEntry.fullPath); var masterPromise = Async.doInParallel_aggregateResults(providerList, function (provider, providerNum) { var result = new $.Deferred(); - + + var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + fileEntry.fullPath); + try { var scanResult = provider.scanFile(fileText, fileEntry.fullPath); result.resolve(scanResult); @@ -275,6 +274,8 @@ define(function (require, exports, module) { response.reject(err); return; } + + PerfUtils.addMeasurement(perfTimerProvider); return result.promise(); }); @@ -286,7 +287,6 @@ define(function (require, exports, module) { }); PerfUtils.addMeasurement(perfTimerInspector); -// response.resolve(result); }) .fail(function (err) { console.error("[CodeInspection] Could not read file for inspection: " + fileEntry.fullPath); @@ -318,20 +318,22 @@ define(function (require, exports, module) { var _numProblemsReportedByProvider = 0; var allErrors = { errors: [] }; - inspectFile(currentDoc.file, providerList.slice(0)).then(function (results) { + // run all the provider in parallel + inspectFile(currentDoc.file, providerList).then(function (results) { // check if current document wasn't changed while inspectFile was running if (currentDoc !== DocumentManager.getCurrentDocument()) { return; } - // Aggregate all results + // how many errors in total? var errors = results.reduce(function (a, item) { return a + (item.results ? item.results.errors.length : 0); }, 0); + // save for later and make the amount of errors easily available _lastResult = results; _lastResult.errors = errors; -// if (!result || !result.errors.length) { - if (!results || !errors) { + if (!results || !errors) { + _lastResult = null; Resizer.hide($problemsPanel); StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-valid", StringUtils.format(Strings.NO_ERRORS, "provider.name")); setGotoEnabled(false); @@ -356,7 +358,7 @@ define(function (require, exports, module) { }); // if the code inspector was unable to process the whole file, we keep track to show a different status - if (result && result.aborted) { + if (result.results.aborted) { aborted = true; } @@ -370,33 +372,35 @@ define(function (require, exports, module) { }); // Update results table -// var html = Mustache.render(ResultsTemplate, {reportList: results.errors}); var perfTimerDOM = PerfUtils.markStart("ProblemsPanel render:\t" + currentDoc.file.fullPath); - var provider = {name: "Code Inspection"}; - var html = Mustache.render(ResultsTemplate, {reportList: allErrors.errors}); - - $problemsPanelTable - .empty() - .append(html) - .scrollTop(0); // otherwise scroll pos from previous contents is remembered - - $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE, provider.name)); - if (!_collapsed) { - Resizer.show($problemsPanel); - } - - if (numProblems === 1 && !aborted) { - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", StringUtils.format(Strings.SINGLE_ERROR, provider.name)); - } else { - // If inspector was unable to process the whole file, number of errors is indeterminate; indicate with a "+" - if (aborted) { - numProblems += "+"; + if (numProblems) { + var html = Mustache.render(ResultsTemplate, {reportList: allErrors.errors}); + + $problemsPanelTable + .empty() + .append(html) + .scrollTop(0); // otherwise scroll pos from previous contents is remembered + + // Update the title + $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE, Strings.CODE_INSPECTION_PANEL_TITLE)); + + if (!_collapsed) { + Resizer.show($problemsPanel); } - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", - StringUtils.format(Strings.MULTIPLE_ERRORS, provider.name, numProblems)); + + if (numProblems === 1 && !aborted) { + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", StringUtils.format(Strings.SINGLE_ERROR, Strings.CODE_INSPECTION_PANEL_TITLE)); + } else { + // If inspector was unable to process the whole file, number of errors is indeterminate; indicate with a "+" + if (aborted) { + numProblems += "+"; + } + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", + StringUtils.format(Strings.MULTIPLE_ERRORS, Strings.CODE_INSPECTION_PANEL_TITLE, numProblems)); + } + setGotoEnabled(true); } - setGotoEnabled(true); PerfUtils.addMeasurement(perfTimerDOM); } else { @@ -540,13 +544,11 @@ define(function (require, exports, module) { $("#status-inspection").click(function () { // Clicking indicator toggles error panel, if any errors in current file -// if (_lastResult && _lastResult.errors.length) { - if (_lastResult) { + if (_lastResult && _lastResult.errors) { toggleCollapsed(); } }); - // Set initial UI state toggleEnabled(_prefs.getValue("enabled")); toggleCollapsed(_prefs.getValue("collapsed")); diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index 8af19ee781d..b7f8a692840 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -186,6 +186,7 @@ define({ "STATUSBAR_LINE_COUNT_PLURAL" : "\u2014 {0} Lines", // CodeInspection: errors/warnings + "CODE_INSPECTION_PANEL_TITLE" : "Code Inspection", "ERRORS_PANEL_TITLE" : "{0} Errors", "SINGLE_ERROR" : "1 {0} Error", "MULTIPLE_ERRORS" : "{1} {0} Errors", diff --git a/src/utils/Async.js b/src/utils/Async.js index ea97e078a91..12c68179f11 100644 --- a/src/utils/Async.js +++ b/src/utils/Async.js @@ -282,7 +282,7 @@ define(function (require, exports, module) { } /** - * Executes a series of tasks in parallel, saving up error info from any that fail along the way. + * Executes a series of tasks in parallel, saving up result info from any that succeeds along the way. * Returns a Promise that is only resolved/rejected once all tasks are complete. This is * essentially a wrapper around doInParallel(..., false). * diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index 6a7118f903b..4c7e77fb140 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -22,7 +22,7 @@ */ /*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */ -/*global define, describe, it, expect, beforeEach, afterEach, waitsFor, runs, brackets, waitsForDone, spyOn */ +/*global define, describe, it, expect, beforeEach, beforeFirst, afterEach, afterLast, waitsFor, runs, brackets, waitsForDone, spyOn, xit */ define(function (require, exports, module) { "use strict"; @@ -46,7 +46,7 @@ define(function (require, exports, module) { expect($("#problems-panel").is(":visible")).toBe(visible); }; - beforeEach(function () { + beforeFirst(function () { runs(function () { SpecRunnerUtils.createTestWindowAndRun(this, function (w) { testWindow = w; @@ -66,8 +66,12 @@ define(function (require, exports, module) { SpecRunnerUtils.loadProjectInTestWindow(testFolder); }); }); - + afterEach(function () { + testWindow.closeAllFiles(); + }); + + afterLast(function () { testWindow = null; $ = null; brackets = null; @@ -78,143 +82,153 @@ define(function (require, exports, module) { // CodeInspection._setProviderState({name: 'JSLint'}, previousState); }); - it("should run one linter when a javascript document opens", function () { - var called = false, - promise; - - CodeInspection.unregisterAll(); - CodeInspection.register("javascript", { - name: "text linter", - scanFile: function (text, fullPath) { called = true; return {errors: []}; } - }); - - runs(function () { - var simpleTextFileEntry = new NativeFileSystem.FileEntry(testFolder + "/errors.js"); - promise = CodeInspection.inspectFile(simpleTextFileEntry); - - waitsForDone(promise, "file linting", 5000); - }); - - runs(function () { - expect(called).toBeTruthy(); - }); - }); + function createCodeInspector(name, result) { + return { + called: false, + name: name, + // arguments to this function: text, fullPath + // omit the warning + scanFile: function () { this.called = true; return {errors: result}; } + }; + } - it("should run two linters when a javascript document opens", function () { - var linterOneCalled = false, - linterTwoCalled = false, - promise; + describe("Unit level tests", function () { + var simpleJavascriptFileEntry; - CodeInspection.unregisterAll(); - CodeInspection.register("javascript", { - name: "text linter 1", - scanFile: function (text, fullPath) { linterOneCalled = true; return {errors: []}; } - }); - CodeInspection.register("javascript", { - name: "text linter 2", - scanFile: function (text, fullPath) { linterTwoCalled = true; return {errors: []}; } - }); - - runs(function () { - var simpleTextFileEntry = new NativeFileSystem.FileEntry(testFolder + "/errors.js"); - promise = CodeInspection.inspectFile(simpleTextFileEntry); - - waitsForDone(promise, "file linting", 5000); + beforeEach(function () { + CodeInspection.unregisterAll(); + simpleJavascriptFileEntry = new NativeFileSystem.FileEntry(testFolder + "/errors.js"); }); - runs(function () { - expect(linterOneCalled).toBeTruthy(); - expect(linterTwoCalled).toBeTruthy(); - }); - }); + it("should run one linter when a javascript document opens", function () { + var codeInspector = createCodeInspector("text linter", []); + CodeInspection.register("javascript", codeInspector); - it("should run one linter when a javascript document opens and return some errors", function () { - var called = false, - result, - promise; + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); - var lintResult = { - pos: { line: 2, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING - }; + waitsForDone(promise, "file linting", 5000); + }); - CodeInspection.unregisterAll(); - CodeInspection.register("javascript", { - name: "text linter", - scanFile: function (text, fullPath) { - called = true; - return { - errors: [lintResult] - }; - } + runs(function () { + expect(codeInspector.called).toBeTruthy(); + }); }); + + it("should run two linters when a javascript document opens", function () { + var codeInspector1 = createCodeInspector("text linter 1", []); + var codeInspector2 = createCodeInspector("text linter 2", []); - runs(function () { - var simpleTextFileEntry = new NativeFileSystem.FileEntry(testFolder + "/errors.js"); - promise = CodeInspection.inspectFile(simpleTextFileEntry); - promise.done(function (lintingResult) { - result = lintingResult; + CodeInspection.register("javascript", codeInspector1); + CodeInspection.register("javascript", codeInspector2); + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + + waitsForDone(promise, "file linting", 5000); }); - waitsForDone(promise, "file linting", 5000); + runs(function () { + expect(codeInspector1.called).toBeTruthy(); + expect(codeInspector2.called).toBeTruthy(); + }); + }); + + it("should run one linter when a javascript document opens and return some errors", function () { + var result; + + var lintResult = { + pos: { line: 2, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }; + + var codeInspector1 = createCodeInspector("javascript linter", [lintResult]); + CodeInspection.register("javascript", codeInspector1); + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + promise.done(function (lintingResult) { + result = lintingResult; + }); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + expect(codeInspector1.called).toBeTruthy(); + expect(result.length).toEqual(1); + expect(result[0].item.name).toEqual("javascript linter"); + expect(result[0].results.errors.length).toEqual(1); + }); }); - runs(function () { - expect(called).toBeTruthy(); - expect(result.length).toEqual(1); - expect(result[0].item.name).toEqual("text linter"); - expect(result[0].results.errors.length).toEqual(1); + it("should run two linter when a javascript document opens and return some errors", function () { + var result; + + var lintResult = { + pos: { line: 2, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }; + + var codeInspector1 = createCodeInspector("javascript linter 1", [lintResult]); + var codeInspector2 = createCodeInspector("javascript linter 2", [lintResult]); + CodeInspection.register("javascript", codeInspector1); + CodeInspection.register("javascript", codeInspector2); + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + promise.done(function (lintingResult) { + result = lintingResult; + }); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + expect(result.length).toEqual(2); + expect(result[0].results.errors.length).toEqual(1); + expect(result[1].results.errors.length).toEqual(1); + }); }); - }); - - it("should run two linter when a javascript document opens and return some errors", function () { - var result, - promise; - var lintResult = { - pos: { line: 2, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING - }; + it("should not call any other linter for javascript document", function () { + var codeInspector1 = createCodeInspector("any other linter linter 1", []); + CodeInspection.register("whatever", codeInspector1); - CodeInspection.unregisterAll(); - CodeInspection.register("javascript", { - name: "text linter 1", - scanFile: function (text, fullPath) { - return { - errors: [lintResult] - }; - } + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + expect(codeInspector1.called).toBeFalsy(); + }); }); - CodeInspection.register("javascript", { - name: "text linter 2", - scanFile: function (text, fullPath) { - return { - errors: [lintResult] - }; - } - }); + it("should call linter even if linting on save is disabled", function () { + var codeInspector1 = createCodeInspector("javascript linter 1", []); + CodeInspection.register("javascript", codeInspector1); - runs(function () { - var simpleTextFileEntry = new NativeFileSystem.FileEntry(testFolder + "/errors.js"); - promise = CodeInspection.inspectFile(simpleTextFileEntry); - promise.done(function (lintingResult) { - result = lintingResult; + CodeInspection.toggleEnabled(false); + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + + waitsForDone(promise, "file linting", 5000); }); - waitsForDone(promise, "file linting", 5000); - }); - - runs(function () { - expect(result.length).toEqual(2); - expect(result[0].results.errors.length).toEqual(1); - expect(result[1].results.errors.length).toEqual(1); + runs(function () { + expect(codeInspector1.called).toBeTruthy(); + + CodeInspection.toggleEnabled(true); + }); }); }); - - describe("Check the Code Inspection UI", function () { + + describe("Code Inspection UI", function () { beforeEach(function () { CodeInspection.unregisterAll(); @@ -243,6 +257,27 @@ define(function (require, exports, module) { expect($statusBar.is(":visible")).toBe(true); }); }); + + it("should not show the problems panel when there is no linting error", function () { +// brackets.app.showDeveloperTools(); + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); + + CodeInspection.unregisterAll(); + CodeInspection.register("javascript", { + name: "text linter 1", + scanFile: function (text, fullPath) { + return { + errors: [] + }; + } + }); + + runs(function () { + expect($("#problems-panel").is(":visible")).toBe(false); + var $statusBar = $("#status-inspection"); + expect($statusBar.is(":visible")).toBe(true); + }); + }); }); From e5f497cc9e3d63facaafa3cddb6bbe27b86ae54e Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Sun, 10 Nov 2013 23:09:22 -0800 Subject: [PATCH 03/14] - removed code to enable/disable code linter - added more tests --- src/language/CodeInspection.js | 178 ++++++++-------------- test/spec/CodeInspection-test.js | 243 ++++++++++++++++++------------- 2 files changed, 200 insertions(+), 221 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 5f06f83d340..3b8396bd977 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -1,24 +1,24 @@ /* * Copyright (c) 2013 Adobe Systems Incorporated. All rights reserved. - * + * * Permission is hereby granted, free of charge, to any person obtaining a - * copy of this software and associated documentation files (the "Software"), - * to deal in the Software without restriction, including without limitation - * the rights to use, copy, modify, merge, publish, distribute, sublicense, - * and/or sell copies of the Software, and to permit persons to whom the + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the * Software is furnished to do so, subject to the following conditions: - * + * * The above copyright notice and this permission notice shall be included in * all copies or substantial portions of the Software. - * + * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER * DEALINGS IN THE SOFTWARE. - * + * */ @@ -38,7 +38,7 @@ */ define(function (require, exports, module) { "use strict"; - + // Load dependent modules var Commands = require("command/Commands"), PanelManager = require("view/PanelManager"), @@ -59,13 +59,13 @@ define(function (require, exports, module) { Async = require("utils/Async"), PanelTemplate = require("text!htmlContent/problems-panel.html"), ResultsTemplate = require("text!htmlContent/problems-panel-table.html"); - + var INDICATOR_ID = "status-inspection", defaultPrefs = { enabled: brackets.config["linting.enabled_by_default"], collapsed: false }; - + /** Values for problem's 'type' property */ var Type = { /** Unambiguous error, such as a syntax error */ @@ -75,13 +75,13 @@ define(function (require, exports, module) { /** Inspector unable to continue, code too complex for static analysis, etc. Not counted in error/warning tally. */ META: "problem_type_meta" }; - + /** * @private * @type {PreferenceStorage} */ var _prefs = null; - + /** * When disabled, the errors panel is closed and the status bar icon is grayed out. * Takes precedence over _collapsed. @@ -89,20 +89,20 @@ define(function (require, exports, module) { * @type {boolean} */ var _enabled = true; - + /** * When collapsed, the errors panel is closed but the status bar icon is kept up to date. * @private * @type {boolean} */ var _collapsed = false; - + /** * @private * @type {$.Element} */ var $problemsPanel; - + /** * @private * @type {$.Element} @@ -114,25 +114,19 @@ define(function (require, exports, module) { * @type {boolean} */ var _gotoEnabled = false; - + /** * @private * @type {Object.} */ var _providers = {}; - + /** * @private * @type {?Array.} */ var _lastResult; - /** - * @private - * @type {?Array.} - */ - var _allInspectorCommands = []; - /** * Enable or disable the "Go to First Error" command * @param {boolean} gotoEnabled Whether it is enabled. @@ -141,57 +135,6 @@ define(function (require, exports, module) { CommandManager.get(Commands.NAVIGATE_GOTO_FIRST_PROBLEM).setEnabled(gotoEnabled); _gotoEnabled = gotoEnabled; } - - /** - * Construct a preference key for the provider. - * limitation: this function doesn't account for provider with the same name, which could - * result in preferences from one provider overwritten with the ones from another. - * - * @param {name:string, scanFile:function(string, string):Object} provider - */ - function getProviderPrefKey(provider) { - return "provider." + provider.name + ".enabled"; - } - - /** - * Check if a given provider is enabled. - * Return true if enabled, false otherwise. - * - * @param {name:string, scanFile:function(string, string):Object} provider - */ - function getProviderState(provider) { - return _prefs.getValue(getProviderPrefKey(provider)); - } - - /** - * Create a menu entry for the given provider/code inspector. - * The command that is created for this menu entry will be stored for later use. The event handler for this new menu item will handle the enable/disable toggle for the provider/code inspector. - * - * @param {name:string, scanFile:function(string, string):Object} provider - */ - function addMenuEntryForProvider(provider) { - var menuString = StringUtils.format(Strings.CMD_VIEW_ENABLE_INSPECTOR, provider.name), - commandString = "command.inspector." + provider.name; - - var inspectorCommand = CommandManager.register(menuString, commandString, function () { - this.setChecked(!this.getChecked()); - - _prefs.setValue(getProviderPrefKey(provider), this.getChecked()); - - // update results - run(); - }); - - _allInspectorCommands.push(inspectorCommand); -// -// // add a new MenuItem for each inspector -// var viewMenu = Menus.getMenu(Menus.AppMenuBar.VIEW_MENU); -// viewMenu.addMenuItem(inspectorCommand, null, Menus.AFTER, Commands.VIEW_TOGGLE_INSPECTION); -// -// var providerState = getProviderState(provider); -// inspectorCommand.setChecked(providerState); -// inspectorCommand.setEnabled(_enabled); - } /** * The provider is passed the text of the file and its fullPath. Providers should not assume @@ -210,15 +153,13 @@ define(function (require, exports, module) { } _providers[languageId].push(provider); - - addMenuEntryForProvider(provider); } function unregisterAll() { CollectionUtils.forEach(_providers, function (languageId) { delete _providers[languageId]; }); _providers = {}; } - + /** * Returns a provider for given file path, if one is available. * Decision is made depending on the file extension. @@ -261,7 +202,7 @@ define(function (require, exports, module) { .done(function (fileText) { var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + fileEntry.fullPath); - var masterPromise = Async.doInParallel_aggregateResults(providerList, function (provider, providerNum) { + var masterPromise = Async.doInParallel_aggregateResults(providerList, function (provider) { var result = new $.Deferred(); var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + fileEntry.fullPath); @@ -274,9 +215,9 @@ define(function (require, exports, module) { response.reject(err); return; } - + PerfUtils.addMeasurement(perfTimerProvider); - + return result.promise(); }); @@ -285,7 +226,7 @@ define(function (require, exports, module) { }).fail(function (err) { response.reject(err); }); - + PerfUtils.addMeasurement(perfTimerInspector); }) .fail(function (err) { @@ -308,16 +249,16 @@ define(function (require, exports, module) { setGotoEnabled(false); return; } - + var currentDoc = DocumentManager.getCurrentDocument(), providerList = currentDoc && getProviderForPath(currentDoc.file.fullPath); - + if (providerList && providerList.length) { var numProblems = 0; var aborted = false; var _numProblemsReportedByProvider = 0; var allErrors = { errors: [] }; - + // run all the provider in parallel inspectFile(currentDoc.file, providerList).then(function (results) { // check if current document wasn't changed while inspectFile was running @@ -327,7 +268,7 @@ define(function (require, exports, module) { // how many errors in total? var errors = results.reduce(function (a, item) { return a + (item.results ? item.results.errors.length : 0); }, 0); - + // save for later and make the amount of errors easily available _lastResult = results; _lastResult.errors = errors; @@ -350,18 +291,18 @@ define(function (require, exports, module) { error.friendlyLine = error.pos.line + 1; error.codeSnippet = currentDoc.getLine(error.pos.line); error.codeSnippet = error.codeSnippet.substr(0, Math.min(175, error.codeSnippet.length)); // limit snippet width - + if (error.type !== Type.META) { numProblems++; _numProblemsReportedByProvider++; } }); - + // if the code inspector was unable to process the whole file, we keep track to show a different status if (result.results.aborted) { aborted = true; } - + allErrors.errors.push({ providerName: provider.name, results: result.results.errors, @@ -376,19 +317,19 @@ define(function (require, exports, module) { if (numProblems) { var html = Mustache.render(ResultsTemplate, {reportList: allErrors.errors}); - + $problemsPanelTable .empty() .append(html) .scrollTop(0); // otherwise scroll pos from previous contents is remembered - + // Update the title $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE, Strings.CODE_INSPECTION_PANEL_TITLE)); - + if (!_collapsed) { Resizer.show($problemsPanel); } - + if (numProblems === 1 && !aborted) { StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", StringUtils.format(Strings.SINGLE_ERROR, Strings.CODE_INSPECTION_PANEL_TITLE)); } else { @@ -416,7 +357,7 @@ define(function (require, exports, module) { setGotoEnabled(false); } } - + /** * Update DocumentManager listeners. */ @@ -436,7 +377,7 @@ define(function (require, exports, module) { $(DocumentManager).off(".codeInspection"); } } - + /** * Enable or disable all inspection. * @param {?boolean} enabled Enabled state. If omitted, the state is toggled. @@ -446,31 +387,30 @@ define(function (require, exports, module) { enabled = !_enabled; } _enabled = enabled; - + CommandManager.get(Commands.VIEW_TOGGLE_INSPECTION).setChecked(_enabled); updateListeners(); _prefs.setValue("enabled", _enabled); - + // run immediately run(); } - - - /** + + /** * Toggle the collapsed state for the panel. This explicitly collapses the panel (as opposed to * the auto collapse due to files with no errors & filetypes with no provider). When explicitly * collapsed, the panel will not reopen automatically on switch files or save. - * + * * @param {?boolean} collapsed Collapsed state. If omitted, the state is toggled. */ function toggleCollapsed(collapsed) { if (collapsed === undefined) { collapsed = !_collapsed; } - + _collapsed = collapsed; _prefs.setValue("collapsed", _collapsed); - + if (_collapsed) { Resizer.hide($problemsPanel); } else { @@ -479,7 +419,7 @@ define(function (require, exports, module) { } } } - + /** Command to go to the first Error/Warning */ function handleGotoFirstProblem() { run(); @@ -487,22 +427,22 @@ define(function (require, exports, module) { $problemsPanel.find("tr:first-child").trigger("click"); } } - - + + // Register command handlers CommandManager.register(Strings.CMD_VIEW_TOGGLE_INSPECTION, Commands.VIEW_TOGGLE_INSPECTION, toggleEnabled); CommandManager.register(Strings.CMD_GOTO_FIRST_PROBLEM, Commands.NAVIGATE_GOTO_FIRST_PROBLEM, handleGotoFirstProblem); - + // Init PreferenceStorage _prefs = PreferencesManager.getPreferenceStorage(module, defaultPrefs); - + // Initialize items dependent on HTML DOM AppInit.htmlReady(function () { // Create bottom panel to list error details var panelHtml = Mustache.render(PanelTemplate, Strings); var resultsPanel = PanelManager.createBottomPanel("errors", $(panelHtml), 100); $problemsPanel = $("#problems-panel"); - + var $selectedRow; $problemsPanelTable = $problemsPanel.find(".table-container") .on("click", "tr", function (e) { @@ -526,7 +466,7 @@ define(function (require, exports, module) { var lineTd = $selectedRow.find(".line-number"); var line = parseInt(lineTd.text(), 10) - 1; // convert friendlyLine back to pos.line var character = lineTd.data("character"); - + var editor = EditorManager.getCurrentFullEditor(); editor.setCursorPos(line, character, true); EditorManager.focusEditor(); @@ -536,27 +476,27 @@ define(function (require, exports, module) { $("#problems-panel .close").click(function () { toggleCollapsed(true); }); - + // Status bar indicator - icon & tooltip updated by run() var statusIconHtml = Mustache.render("
 
", Strings); $(statusIconHtml).insertBefore("#status-language"); StatusBar.addIndicator(INDICATOR_ID, $("#status-inspection")); - + $("#status-inspection").click(function () { // Clicking indicator toggles error panel, if any errors in current file if (_lastResult && _lastResult.errors) { toggleCollapsed(); } }); - + // Set initial UI state toggleEnabled(_prefs.getValue("enabled")); toggleCollapsed(_prefs.getValue("collapsed")); }); - + // Testing exports.unregisterAll = unregisterAll; - + // Public API exports.register = register; exports.Type = Type; diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index 4c7e77fb140..3f3612494be 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -1,24 +1,24 @@ /* * Copyright (c) 2013 Adobe Systems Incorporated. All rights reserved. - * + * * Permission is hereby granted, free of charge, to any person obtaining a - * copy of this software and associated documentation files (the "Software"), - * to deal in the Software without restriction, including without limitation - * the rights to use, copy, modify, merge, publish, distribute, sublicense, - * and/or sell copies of the Software, and to permit persons to whom the + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the * Software is furnished to do so, subject to the following conditions: - * + * * The above copyright notice and this permission notice shall be included in * all copies or substantial portions of the Software. - * + * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER * DEALINGS IN THE SOFTWARE. - * + * */ /*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */ @@ -26,9 +26,9 @@ define(function (require, exports, module) { "use strict"; - - var SpecRunnerUtils = require("spec/SpecRunnerUtils"), - NativeFileSystem = require("file/NativeFileSystem").NativeFileSystem; + + var SpecRunnerUtils = require("spec/SpecRunnerUtils"), + NativeFileSystem = require("file/NativeFileSystem").NativeFileSystem; describe("Code Inspection", function () { this.category = "integration"; @@ -38,14 +38,24 @@ define(function (require, exports, module) { $, brackets, CodeInspection, - EditorManager, - previousState; + EditorManager; var toggleJSLintResults = function (visible) { $("#status-inspection").triggerHandler("click"); expect($("#problems-panel").is(":visible")).toBe(visible); }; + function createCodeInspector(name, result) { + return { + called: false, + name: name, + // arguments to this function: text, fullPath + // omit the warning + scanFile: function () { this.called = true; return {errors: result}; } + }; + } + + beforeFirst(function () { runs(function () { SpecRunnerUtils.createTestWindowAndRun(this, function (w) { @@ -56,12 +66,9 @@ define(function (require, exports, module) { EditorManager = brackets.test.EditorManager; CodeInspection = brackets.test.CodeInspection; CodeInspection.toggleEnabled(true); - // enable JSLint and preserve the previous state before we enable it for testing -// previousState = CodeInspection._getProviderState({name: "JSLint"}); -// CodeInspection._setProviderState({name: "JSLint"}, true); }); }); - + runs(function () { SpecRunnerUtils.loadProjectInTestWindow(testFolder); }); @@ -70,36 +77,23 @@ define(function (require, exports, module) { afterEach(function () { testWindow.closeAllFiles(); }); - + afterLast(function () { testWindow = null; $ = null; brackets = null; EditorManager = null; SpecRunnerUtils.closeTestWindow(); - - // revert to previous state -// CodeInspection._setProviderState({name: 'JSLint'}, previousState); }); - - function createCodeInspector(name, result) { - return { - called: false, - name: name, - // arguments to this function: text, fullPath - // omit the warning - scanFile: function () { this.called = true; return {errors: result}; } - }; - } - + describe("Unit level tests", function () { var simpleJavascriptFileEntry; - + beforeEach(function () { CodeInspection.unregisterAll(); simpleJavascriptFileEntry = new NativeFileSystem.FileEntry(testFolder + "/errors.js"); }); - + it("should run one linter when a javascript document opens", function () { var codeInspector = createCodeInspector("text linter", []); CodeInspection.register("javascript", codeInspector); @@ -114,7 +108,7 @@ define(function (require, exports, module) { expect(codeInspector.called).toBeTruthy(); }); }); - + it("should run two linters when a javascript document opens", function () { var codeInspector1 = createCodeInspector("text linter 1", []); var codeInspector2 = createCodeInspector("text linter 2", []); @@ -124,37 +118,37 @@ define(function (require, exports, module) { runs(function () { var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); - + waitsForDone(promise, "file linting", 5000); }); - + runs(function () { expect(codeInspector1.called).toBeTruthy(); expect(codeInspector2.called).toBeTruthy(); }); }); - + it("should run one linter when a javascript document opens and return some errors", function () { var result; - + var lintResult = { pos: { line: 2, ch: 3 }, message: "Some errors here and there", type: CodeInspection.Type.WARNING }; - + var codeInspector1 = createCodeInspector("javascript linter", [lintResult]); CodeInspection.register("javascript", codeInspector1); - + runs(function () { var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); promise.done(function (lintingResult) { result = lintingResult; }); - + waitsForDone(promise, "file linting", 5000); }); - + runs(function () { expect(codeInspector1.called).toBeTruthy(); expect(result.length).toEqual(1); @@ -162,16 +156,16 @@ define(function (require, exports, module) { expect(result[0].results.errors.length).toEqual(1); }); }); - + it("should run two linter when a javascript document opens and return some errors", function () { var result; - + var lintResult = { pos: { line: 2, ch: 3 }, message: "Some errors here and there", type: CodeInspection.Type.WARNING }; - + var codeInspector1 = createCodeInspector("javascript linter 1", [lintResult]); var codeInspector2 = createCodeInspector("javascript linter 2", [lintResult]); CodeInspection.register("javascript", codeInspector1); @@ -182,10 +176,10 @@ define(function (require, exports, module) { promise.done(function (lintingResult) { result = lintingResult; }); - + waitsForDone(promise, "file linting", 5000); }); - + runs(function () { expect(result.length).toEqual(2); expect(result[0].results.errors.length).toEqual(1); @@ -199,10 +193,10 @@ define(function (require, exports, module) { runs(function () { var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); - + waitsForDone(promise, "file linting", 5000); }); - + runs(function () { expect(codeInspector1.called).toBeFalsy(); }); @@ -213,16 +207,16 @@ define(function (require, exports, module) { CodeInspection.register("javascript", codeInspector1); CodeInspection.toggleEnabled(false); - + runs(function () { var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); - + waitsForDone(promise, "file linting", 5000); }); - + runs(function () { expect(codeInspector1.called).toBeTruthy(); - + CodeInspection.toggleEnabled(true); }); }); @@ -231,26 +225,48 @@ define(function (require, exports, module) { describe("Code Inspection UI", function () { beforeEach(function () { CodeInspection.unregisterAll(); - + }); + + it("should run test linter when a JavaScript document opens and indicate errors in the panel", function () { var lintResult = { pos: { line: 1, ch: 3 }, message: "Some errors here and there", type: CodeInspection.Type.WARNING }; - CodeInspection.register("javascript", { - name: "text linter 1", - scanFile: function (text, fullPath) { - return { - errors: [lintResult] - }; - } + var codeInspector = createCodeInspector("javascript linter", [lintResult]); + CodeInspection.register("javascript", codeInspector); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); + + runs(function () { + expect($("#problems-panel").is(":visible")).toBe(true); + var $statusBar = $("#status-inspection"); + expect($statusBar.is(":visible")).toBe(true); }); }); - - it("should run test linter when a JavaScript document opens and indicate errors in panel", function () { + + it("should show problems panel after too many errors", function () { + var lintResult = [ + { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }, + { + pos: { line: 1, ch: 5 }, + message: "Stopping. (33% scanned).", + type: CodeInspection.Type.META + } + ]; + + brackets.app.showDeveloperTools(); + + var codeInspector = createCodeInspector("javascript linter", lintResult); + CodeInspection.register("javascript", codeInspector); + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); - + runs(function () { expect($("#problems-panel").is(":visible")).toBe(true); var $statusBar = $("#status-inspection"); @@ -258,48 +274,71 @@ define(function (require, exports, module) { }); }); - it("should not show the problems panel when there is no linting error", function () { -// brackets.app.showDeveloperTools(); + it("should not run test linter when a JavaScript document opens and linting is disabled", function () { + var lintResult = { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }; + + CodeInspection.toggleEnabled(false); + + var codeInspector = createCodeInspector("javascript linter", [lintResult]); + CodeInspection.register("javascript", codeInspector); + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); - - CodeInspection.unregisterAll(); - CodeInspection.register("javascript", { - name: "text linter 1", - scanFile: function (text, fullPath) { - return { - errors: [] - }; - } - }); - + runs(function () { + expect(codeInspector.called).toBeFalsy(); expect($("#problems-panel").is(":visible")).toBe(false); var $statusBar = $("#status-inspection"); expect($statusBar.is(":visible")).toBe(true); + + CodeInspection.toggleEnabled(true); }); }); - }); - - - xit("status icon should toggle Errors panel when errors present", function () { -// waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); - runs(function () { - var promise = SpecRunnerUtils.openProjectFiles(["errors.js"]); - waitsForDone(promise); + + it("should not show the problems panel when there is no linting error", function () { + var codeInspector = createCodeInspector("javascript linter", []); + CodeInspection.register("javascript", codeInspector); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); + + runs(function () { + expect($("#problems-panel").is(":visible")).toBe(false); + var $statusBar = $("#status-inspection"); + expect($statusBar.is(":visible")).toBe(true); + }); }); - - runs(function () { - toggleJSLintResults(false); - toggleJSLintResults(true); + + it("status icon should toggle Errors panel when errors present", function () { + var lintResult = { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }; + + var codeInspector = createCodeInspector("javascript linter", [lintResult]); + CodeInspection.register("javascript", codeInspector); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + + runs(function () { + toggleJSLintResults(false); + toggleJSLintResults(true); + }); }); - }); - - xit("status icon should not toggle Errors panel when no errors present", function () { - waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file"); - - runs(function () { - toggleJSLintResults(false); - toggleJSLintResults(false); + + it("status icon should not toggle Errors panel when no errors present", function () { + var codeInspector = createCodeInspector("javascript linter", []); + CodeInspection.register("javascript", codeInspector); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file"); + + runs(function () { + toggleJSLintResults(false); + toggleJSLintResults(false); + }); }); }); }); From eb6b7bce956f9bcb80e483c3438ec15b5e2eca1b Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Mon, 11 Nov 2013 09:37:04 -0800 Subject: [PATCH 04/14] - update the comments --- src/utils/Async.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/utils/Async.js b/src/utils/Async.js index d206032e887..f503b1aae3b 100644 --- a/src/utils/Async.js +++ b/src/utils/Async.js @@ -286,10 +286,11 @@ define(function (require, exports, module) { * Returns a Promise that is only resolved/rejected once all tasks are complete. This is * essentially a wrapper around doInParallel(..., false). * - * If one or more tasks failed, the entire "master" promise is rejected at the end - with one + * If one or more tasks failed, the entire "master" promise is rejected at the end without any further details. + * Otherwise the promise gets resolved with the results from all tasks * argument: an array objects, one per failed task. Each error object contains: * - item -- the entry in items whose task failed - * - error -- the first argument passed to the fail() handler when the task failed + * - result -- the first argument passed to the done() handler when the task succeeds * * @param {!Array.<*>} items * @param {!function(*, number):Promise} beginProcessItem From 632d862c003163bf3d09cbd802c35fffca070632 Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Thu, 14 Nov 2013 09:50:29 -0800 Subject: [PATCH 05/14] - first round of changes based on code review comments --- src/htmlContent/problems-panel-table.html | 2 +- src/language/CodeInspection.js | 68 ++++++++++++----------- test/spec/CodeInspection-test.js | 57 +++++++++++++------ 3 files changed, 77 insertions(+), 50 deletions(-) diff --git a/src/htmlContent/problems-panel-table.html b/src/htmlContent/problems-panel-table.html index d1d5970eb43..1709be5dd87 100644 --- a/src/htmlContent/problems-panel-table.html +++ b/src/htmlContent/problems-panel-table.html @@ -1,7 +1,7 @@ {{#reportList}} - + {{#results}} diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index cbbb901fb5a..a64e29051f9 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -46,7 +46,6 @@ define(function (require, exports, module) { DocumentManager = require("document/DocumentManager"), EditorManager = require("editor/EditorManager"), FileUtils = require("file/FileUtils"), - CollectionUtils = require("utils/CollectionUtils"), LanguageManager = require("language/LanguageManager"), PreferencesManager = require("preferences/PreferencesManager"), PerfUtils = require("utils/PerfUtils"), @@ -55,7 +54,6 @@ define(function (require, exports, module) { AppInit = require("utils/AppInit"), Resizer = require("utils/Resizer"), StatusBar = require("widgets/StatusBar"), - Menus = require("command/Menus"), Async = require("utils/Async"), PanelTemplate = require("text!htmlContent/problems-panel.html"), ResultsTemplate = require("text!htmlContent/problems-panel-table.html"); @@ -117,7 +115,7 @@ define(function (require, exports, module) { /** * @private - * @type {Object.} + * @type {Object.):Object}>>} */ var _providers = {}; @@ -127,6 +125,12 @@ define(function (require, exports, module) { */ var _lastResult; + /** + * @private + * @type {boolean} + */ + var _hasErrors; + /** * Enable or disable the "Go to First Error" command * @param {boolean} gotoEnabled Whether it is enabled. @@ -155,19 +159,18 @@ define(function (require, exports, module) { _providers[languageId].push(provider); } - function unregisterAll() { - CollectionUtils.forEach(_providers, function (languageId) { delete _providers[languageId]; }); + function _unregisterAll() { _providers = {}; } /** - * Returns a provider for given file path, if one is available. + * Returns a list of provider for given file path, if available. * Decision is made depending on the file extension. * * @param {!string} filePath - * @return ?{{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}} provider + * @return ?{Array.<{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}}>} provider */ - function getProviderForPath(filePath) { + function getProvidersForPath(filePath) { return _providers[LanguageManager.getLanguageForPath(filePath).getId()]; } @@ -176,30 +179,30 @@ define(function (require, exports, module) { * This method doesn't update the Brackets UI, just provides inspection results. * These results will reflect any unsaved changes present in the file that is currently opened. * - * @param {!FileEntry} fileEntry File that will be inspected for errors. + * @param {!File} file File that will be inspected for errors. * @param ?{{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}} provider - * @return {$.Promise} a jQuery promise that will be resolved with ?{!errors:Array, aborted:boolean} + * @return {$.Promise} a jQuery promise that will be resolved with Array.<{ item: string, results: ?{!errors:Array, aborted:boolean}}> */ - function inspectFile(fileEntry, provider) { + function inspectFile(file, provider) { var response = new $.Deferred(); - var providerList = (provider || getProviderForPath(fileEntry.fullPath)) || []; + var providerList = (provider || getProvidersForPath(file.fullPath)) || []; - if (!providerList && !providerList.length) { + if (!providerList.length) { response.resolve(null); return response.promise(); } - DocumentManager.getDocumentText(fileEntry) + DocumentManager.getDocumentText(file) .done(function (fileText) { - var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + fileEntry.fullPath); + var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + file.fullPath); var masterPromise = Async.doInParallel_aggregateResults(providerList, function (provider) { var result = new $.Deferred(); - var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + fileEntry.fullPath); + var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath); try { - var scanResult = provider.scanFile(fileText, fileEntry.fullPath); + var scanResult = provider.scanFile(fileText, file.fullPath); result.resolve(scanResult); } catch (err) { console.error("[CodeInspection] Provider " + provider.name + " threw an error: " + err); @@ -221,7 +224,7 @@ define(function (require, exports, module) { PerfUtils.addMeasurement(perfTimerInspector); }) .fail(function (err) { - console.error("[CodeInspection] Could not read file for inspection: " + fileEntry.fullPath); + console.error("[CodeInspection] Could not read file for inspection: " + file.fullPath); response.reject(err); }); @@ -242,12 +245,12 @@ define(function (require, exports, module) { } var currentDoc = DocumentManager.getCurrentDocument(), - providerList = currentDoc && getProviderForPath(currentDoc.file.fullPath); + providerList = currentDoc && getProvidersForPath(currentDoc.file.fullPath); if (providerList && providerList.length) { var numProblems = 0; var aborted = false; - var _numProblemsReportedByProvider = 0; + var numProblemsReportedByProvider = 0; var allErrors = { errors: [] }; // run all the provider in parallel @@ -262,19 +265,19 @@ define(function (require, exports, module) { // save for later and make the amount of errors easily available _lastResult = results; - _lastResult.errors = errors; + _hasErrors = errors; if (!results || !errors) { _lastResult = null; Resizer.hide($problemsPanel); - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-valid", StringUtils.format(Strings.NO_ERRORS, "provider.name")); + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-valid", StringUtils.format(Strings.NO_ERRORS, Strings.CODE_INSPECTION_PANEL_TITLE)); setGotoEnabled(false); return; } // Augment error objects with additional fields needed by Mustache template results.forEach(function (result) { - _numProblemsReportedByProvider = 0; + numProblemsReportedByProvider = 0; var provider = result.item; if (result.results) { @@ -285,19 +288,18 @@ define(function (require, exports, module) { if (error.type !== Type.META) { numProblems++; - _numProblemsReportedByProvider++; + numProblemsReportedByProvider++; + } + // if the code inspector was unable to process the whole file, we keep track to show a different status + if (error.aborted) { + aborted = true; } }); - // if the code inspector was unable to process the whole file, we keep track to show a different status - if (result.results.aborted) { - aborted = true; - } - allErrors.errors.push({ providerName: provider.name, results: result.results.errors, - numProblems: _numProblemsReportedByProvider + numProblems: numProblemsReportedByProvider }); } }); @@ -405,7 +407,7 @@ define(function (require, exports, module) { if (_collapsed) { Resizer.hide($problemsPanel); } else { - if (_lastResult && _lastResult.errors) { + if (_lastResult && _hasErrors) { Resizer.show($problemsPanel); } } @@ -475,7 +477,7 @@ define(function (require, exports, module) { $("#status-inspection").click(function () { // Clicking indicator toggles error panel, if any errors in current file - if (_lastResult && _lastResult.errors) { + if (_lastResult && _hasErrors) { toggleCollapsed(); } }); @@ -486,7 +488,7 @@ define(function (require, exports, module) { }); // Testing - exports.unregisterAll = unregisterAll; + exports.unregisterAll = _unregisterAll; // Public API exports.register = register; diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index 832fdb3af0c..d41c63dc561 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -22,7 +22,7 @@ */ /*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */ -/*global define, describe, it, expect, beforeEach, beforeFirst, afterEach, afterLast, waitsFor, runs, brackets, waitsForDone, spyOn, xit */ +/*global define, describe, it, expect, beforeEach, beforeFirst, afterEach, afterLast, waitsFor, runs, brackets, waitsForDone, spyOn, xit, jasmine */ define(function (require, exports, module) { "use strict"; @@ -46,13 +46,16 @@ define(function (require, exports, module) { }; function createCodeInspector(name, result) { - return { - called: false, + var provider = { name: name, // arguments to this function: text, fullPath // omit the warning - scanFile: function () { this.called = true; return {errors: result}; } + scanFile: function () { return {errors: result}; } }; + + spyOn(provider, "scanFile").andCallThrough(); + + return provider; } beforeFirst(function () { @@ -93,7 +96,7 @@ define(function (require, exports, module) { simpleJavascriptFileEntry = new FileSystem.getFileForPath(testFolder + "/errors.js"); }); - it("should run one linter when a javascript document opens", function () { + it("should run a single registered linter", function () { var codeInspector = createCodeInspector("text linter", []); CodeInspection.register("javascript", codeInspector); @@ -104,11 +107,11 @@ define(function (require, exports, module) { }); runs(function () { - expect(codeInspector.called).toBeTruthy(); + expect(codeInspector.scanFile).toHaveBeenCalled(); }); }); - it("should run two linters when a javascript document opens", function () { + it("should run two linters", function () { var codeInspector1 = createCodeInspector("text linter 1", []); var codeInspector2 = createCodeInspector("text linter 2", []); @@ -122,12 +125,12 @@ define(function (require, exports, module) { }); runs(function () { - expect(codeInspector1.called).toBeTruthy(); - expect(codeInspector2.called).toBeTruthy(); + expect(codeInspector1.scanFile).toHaveBeenCalled(); + expect(codeInspector2.scanFile).toHaveBeenCalled(); }); }); - it("should run one linter when a javascript document opens and return some errors", function () { + it("should run one linter return some errors", function () { var result; var lintResult = { @@ -149,14 +152,14 @@ define(function (require, exports, module) { }); runs(function () { - expect(codeInspector1.called).toBeTruthy(); + expect(codeInspector1.scanFile).toHaveBeenCalled(); expect(result.length).toEqual(1); expect(result[0].item.name).toEqual("javascript linter"); expect(result[0].results.errors.length).toEqual(1); }); }); - it("should run two linter when a javascript document opens and return some errors", function () { + it("should run two linter and return some errors", function () { var result; var lintResult = { @@ -197,7 +200,7 @@ define(function (require, exports, module) { }); runs(function () { - expect(codeInspector1.called).toBeFalsy(); + expect(codeInspector1.scanFile).not.toHaveBeenCalled(); }); }); @@ -214,11 +217,28 @@ define(function (require, exports, module) { }); runs(function () { - expect(codeInspector1.called).toBeTruthy(); + expect(codeInspector1.scanFile).toHaveBeenCalled(); CodeInspection.toggleEnabled(true); }); }); + + it("should return no result if there is no linter registered", function () { + var expectedResult; + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + promise.done(function (result) { + expectedResult = result; + }); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + expect(expectedResult).toBeNull(); + }); + }); }); describe("Code Inspection UI", function () { @@ -255,7 +275,8 @@ define(function (require, exports, module) { { pos: { line: 1, ch: 5 }, message: "Stopping. (33% scanned).", - type: CodeInspection.Type.META + type: CodeInspection.Type.META, + aborted: true } ]; @@ -268,6 +289,10 @@ define(function (require, exports, module) { expect($("#problems-panel").is(":visible")).toBe(true); var $statusBar = $("#status-inspection"); expect($statusBar.is(":visible")).toBe(true); + + var tooltip = $statusBar.attr("title"); + // tooltip will contain + in the title if the inspection was aborted + expect(tooltip.lastIndexOf("+")).not.toBe(-1); }); }); @@ -286,7 +311,7 @@ define(function (require, exports, module) { waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); runs(function () { - expect(codeInspector.called).toBeFalsy(); + expect(codeInspector.scanFile).not.toHaveBeenCalled(); expect($("#problems-panel").is(":visible")).toBe(false); var $statusBar = $("#status-inspection"); expect($statusBar.is(":visible")).toBe(true); From 1e8f97166134691972408a414aae571c6006dab7 Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Thu, 14 Nov 2013 18:37:54 -0800 Subject: [PATCH 06/14] - some code improvements based on code review --- src/language/CodeInspection.js | 35 ++++++++++++++------------ src/styles/brackets.less | 42 +++++++++++++------------------- test/spec/CodeInspection-test.js | 42 ++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 41 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index a64e29051f9..ef10e54b8d5 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -39,6 +39,8 @@ define(function (require, exports, module) { "use strict"; + var _ = require("thirdparty/lodash"); + // Load dependent modules var Commands = require("command/Commands"), PanelManager = require("view/PanelManager"), @@ -186,6 +188,7 @@ define(function (require, exports, module) { function inspectFile(file, provider) { var response = new $.Deferred(); var providerList = (provider || getProvidersForPath(file.fullPath)) || []; + var results = []; if (!providerList.length) { response.resolve(null); @@ -196,14 +199,12 @@ define(function (require, exports, module) { .done(function (fileText) { var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + file.fullPath); - var masterPromise = Async.doInParallel_aggregateResults(providerList, function (provider) { - var result = new $.Deferred(); - + _.forEach(providerList, function (provider) { var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath); try { var scanResult = provider.scanFile(fileText, file.fullPath); - result.resolve(scanResult); + results.push({ item: provider, results: scanResult }); } catch (err) { console.error("[CodeInspection] Provider " + provider.name + " threw an error: " + err); response.reject(err); @@ -211,15 +212,9 @@ define(function (require, exports, module) { } PerfUtils.addMeasurement(perfTimerProvider); - - return result.promise(); - }); - - masterPromise.done(function (results) { - response.resolve(results); - }).fail(function (err) { - response.reject(err); }); + + response.resolve(results); PerfUtils.addMeasurement(perfTimerInspector); }) @@ -251,7 +246,8 @@ define(function (require, exports, module) { var numProblems = 0; var aborted = false; var numProblemsReportedByProvider = 0; - var allErrors = { errors: [] }; + var allErrors = []; + var html; // run all the provider in parallel inspectFile(currentDoc.file, providerList).then(function (results) { @@ -296,21 +292,21 @@ define(function (require, exports, module) { } }); - allErrors.errors.push({ + allErrors.push({ providerName: provider.name, results: result.results.errors, numProblems: numProblemsReportedByProvider }); } }); + + html = Mustache.render(ResultsTemplate, {reportList: allErrors}); }); // Update results table var perfTimerDOM = PerfUtils.markStart("ProblemsPanel render:\t" + currentDoc.file.fullPath); if (numProblems) { - var html = Mustache.render(ResultsTemplate, {reportList: allErrors.errors}); - $problemsPanelTable .empty() .append(html) @@ -337,6 +333,13 @@ define(function (require, exports, module) { } PerfUtils.addMeasurement(perfTimerDOM); + + // don't show a header if there is only one provider available for this file type + if (providerList.length === 1) { + $problemsPanelTable.find(".inspector-section").hide(); + } else { + $problemsPanelTable.find(".inspector-section").show(); + } } else { // No provider for current file _lastResult = null; diff --git a/src/styles/brackets.less b/src/styles/brackets.less index dc749aec1d0..fb130c38f68 100644 --- a/src/styles/brackets.less +++ b/src/styles/brackets.less @@ -928,7 +928,7 @@ a, img { } } -#search-results .disclosure-triangle { +#search-results #problems-panel .disclosure-triangle { .jstree-sprite; display: inline-block; &.expanded { @@ -942,6 +942,22 @@ a, img { } } + +#problems-panel .disclosure-triangle { + .jstree-sprite; + display: inline-block; + &.expanded { + // Unfortunately, the way jsTree sprites are aligned within their 18px boxes doesn't look good in + // other contexts, so we need some tweaks here instead of straight multiples of @jstree-sprite-size + background-position: 7px 5px; + -webkit-transform: translateZ(0) rotate(90deg); + } + &.collapsed { + background-position: 7px 5px; + } +} + + /* Modal bar for Find/Quick Open */ .modal-bar { @@ -1203,16 +1219,6 @@ a, img { td { padding-left: 3px; - vertical-align: baseline; - } - - .line-number { - color: @tc-light-weight-quiet-text; - font-family: SourceCodePro; - font-size: 11px; - padding: 0 10px 0 14px; - text-align: right; - white-space: nowrap; } .line-text { @@ -1227,20 +1233,6 @@ a, img { } } -#problems-panel .disclosure-triangle { - .jstree-sprite; - display: inline-block; - &.expanded { - // Unfortunately, the way jsTree sprites are aligned within their 18px boxes doesn't look good in - // other contexts, so we need some tweaks here instead of straight multiples of @jstree-sprite-size - background-position: 7px 5px; - -webkit-transform: translateZ(0) rotate(90deg); - } - &.collapsed { - background-position: 7px 5px; - } -} - /* Line up label text and input text */ label input { position: relative; diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index d41c63dc561..a525c90fc81 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -351,6 +351,48 @@ define(function (require, exports, module) { }); }); + it("should run two linter and display two expanded collapsible sections in the errors panel", function () { + var lintResult = { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }; + + var codeInspector1 = createCodeInspector("javascript linter 1", [lintResult]); + var codeInspector2 = createCodeInspector("javascript linter 2", [lintResult]); + CodeInspection.register("javascript", codeInspector1); + CodeInspection.register("javascript", codeInspector2); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); + + runs(function () { + var $inspectorSections = $(".inspector-section td"); + expect($inspectorSections.length).toEqual(2); + expect($inspectorSections[0].innerHTML.lastIndexOf("javascript linter 1 (1)")).not.toBe(-1); + expect($inspectorSections[1].innerHTML.lastIndexOf("javascript linter 2 (1)")).not.toBe(-1); + + var $expandedInspectorSections = $inspectorSections.find(".expanded"); + expect($expandedInspectorSections.length).toEqual(2); + }); + }); + + it("should run the linter and display no collapsible header section in the errors panel", function () { + var lintResult = { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }; + + var codeInspector1 = createCodeInspector("javascript linter 1", [lintResult]); + CodeInspection.register("javascript", codeInspector1); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); + + runs(function () { + expect($(".inspector-section").is(":visible")).toBeFalsy(); + }); + }); + it("status icon should not toggle Errors panel when no errors present", function () { var codeInspector = createCodeInspector("javascript linter", []); CodeInspection.register("javascript", codeInspector); From b191616ea1ca9ce06bef82feb6b034fcc7929102 Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Wed, 4 Dec 2013 19:03:23 -0800 Subject: [PATCH 07/14] - some rework based on PR comments --- src/language/CodeInspection.js | 119 ++++++++--------- src/nls/root/strings.js | 3 +- src/styles/brackets.less | 18 +-- src/utils/Async.js | 45 ------- test/spec/CodeInspection-test.js | 213 ++++++++++++++++++++++--------- 5 files changed, 219 insertions(+), 179 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index ef10e54b8d5..b90d8df00b6 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -117,16 +117,10 @@ define(function (require, exports, module) { /** * @private - * @type {Object.):Object}>>} + * @type {Object.>} */ var _providers = {}; - /** - * @private - * @type {?Array.} - */ - var _lastResult; - /** * @private * @type {boolean} @@ -142,25 +136,6 @@ define(function (require, exports, module) { _gotoEnabled = gotoEnabled; } - /** - * The provider is passed the text of the file and its fullPath. Providers should not assume - * that the file is open (i.e. DocumentManager.getOpenDocumentForPath() may return null) or - * that the file on disk matches the text given (file may have unsaved changes). - * - * @param {string} languageId - * @param {{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}} provider - * - * Each error is: { pos:{line,ch}, endPos:?{line,ch}, message:string, type:?Type } - * If type is unspecified, Type.WARNING is assumed. - */ - function register(languageId, provider) { - if (!_providers[languageId]) { - _providers[languageId] = []; - } - - _providers[languageId].push(provider); - } - function _unregisterAll() { _providers = {}; } @@ -182,15 +157,15 @@ define(function (require, exports, module) { * These results will reflect any unsaved changes present in the file that is currently opened. * * @param {!File} file File that will be inspected for errors. - * @param ?{{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}} provider - * @return {$.Promise} a jQuery promise that will be resolved with Array.<{ item: string, results: ?{!errors:Array, aborted:boolean}}> + * @param ?[{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}] providerList + * @return {$.Promise} a jQuery promise that will be resolved with Array.<{provider: {{name:string, scanFile:function(string, string)}, issues: ?{!errors:Array, aborted:boolean}}}> */ - function inspectFile(file, provider) { - var response = new $.Deferred(); - var providerList = (provider || getProvidersForPath(file.fullPath)) || []; - var results = []; + function inspectFile(file, providerList) { + var response = new $.Deferred(), + _providerList = (providerList || getProvidersForPath(file.fullPath)) || [], + results = []; - if (!providerList.length) { + if (!_providerList.length) { response.resolve(null); return response.promise(); } @@ -199,12 +174,12 @@ define(function (require, exports, module) { .done(function (fileText) { var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + file.fullPath); - _.forEach(providerList, function (provider) { + _providerList.forEach(function (provider) { var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath); try { var scanResult = provider.scanFile(fileText, file.fullPath); - results.push({ item: provider, results: scanResult }); + results.push({provider: provider, issues: scanResult}); } catch (err) { console.error("[CodeInspection] Provider " + provider.name + " threw an error: " + err); response.reject(err); @@ -232,7 +207,7 @@ define(function (require, exports, module) { */ function run() { if (!_enabled) { - _lastResult = null; + _hasErrors = false; Resizer.hide($problemsPanel); StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-disabled", Strings.LINT_DISABLED); setGotoEnabled(false); @@ -257,14 +232,13 @@ define(function (require, exports, module) { } // how many errors in total? - var errors = results.reduce(function (a, item) { return a + (item.results ? item.results.errors.length : 0); }, 0); + var errors = results.reduce(function (a, item) { return a + (item.issues ? item.issues.errors.length : 0); }, 0); // save for later and make the amount of errors easily available - _lastResult = results; - _hasErrors = errors; + _hasErrors = !!errors; - if (!results || !errors) { - _lastResult = null; + if (!errors) { + _hasErrors = null; Resizer.hide($problemsPanel); StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-valid", StringUtils.format(Strings.NO_ERRORS, Strings.CODE_INSPECTION_PANEL_TITLE)); setGotoEnabled(false); @@ -274,10 +248,10 @@ define(function (require, exports, module) { // Augment error objects with additional fields needed by Mustache template results.forEach(function (result) { numProblemsReportedByProvider = 0; - var provider = result.item; + var provider = result.provider; - if (result.results) { - result.results.errors.forEach(function (error) { + if (result.issues) { + result.issues.errors.forEach(function (error) { error.friendlyLine = error.pos.line + 1; error.codeSnippet = currentDoc.getLine(error.pos.line); error.codeSnippet = error.codeSnippet.substr(0, Math.min(175, error.codeSnippet.length)); // limit snippet width @@ -286,15 +260,16 @@ define(function (require, exports, module) { numProblems++; numProblemsReportedByProvider++; } - // if the code inspector was unable to process the whole file, we keep track to show a different status - if (error.aborted) { - aborted = true; - } }); + // if the code inspector was unable to process the whole file, we keep track to show a different status + if (result.issues.aborted) { + aborted = true; + } + allErrors.push({ providerName: provider.name, - results: result.results.errors, + results: result.issues.errors, numProblems: numProblemsReportedByProvider }); } @@ -313,7 +288,7 @@ define(function (require, exports, module) { .scrollTop(0); // otherwise scroll pos from previous contents is remembered // Update the title - $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE, Strings.CODE_INSPECTION_PANEL_TITLE)); + $problemsPanel.find(".title").text(Strings.CODE_INSPECTION_PANEL_TITLE); if (!_collapsed) { Resizer.show($problemsPanel); @@ -342,7 +317,7 @@ define(function (require, exports, module) { } } else { // No provider for current file - _lastResult = null; + _hasErrors = false; Resizer.hide($problemsPanel); var language = currentDoc && LanguageManager.getLanguageForPath(currentDoc.file.fullPath); if (language) { @@ -354,6 +329,33 @@ define(function (require, exports, module) { } } + /** + * The provider is passed the text of the file and its fullPath. Providers should not assume + * that the file is open (i.e. DocumentManager.getOpenDocumentForPath() may return null) or + * that the file on disk matches the text given (file may have unsaved changes). + * + * @param {string} languageId + * @param {{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}} provider + * + * Each error is: { pos:{line,ch}, endPos:?{line,ch}, message:string, type:?Type } + * If type is unspecified, Type.WARNING is assumed. + */ + function register(languageId, provider, replaceProvider) { + if (!_providers[languageId]) { + _providers[languageId] = []; + } + + if (replaceProvider) { + // remove the provider with the same name from the list + console.log("[CodeInspection] Replacing code inspector '" + provider.name + "' with new implementation"); + _.remove(_providers[languageId], function (registeredProvider) { + return registeredProvider.name === provider.name; + }); + } + + _providers[languageId].push(provider); + } + /** * Update DocumentManager listeners. */ @@ -410,7 +412,8 @@ define(function (require, exports, module) { if (_collapsed) { Resizer.hide($problemsPanel); } else { - if (_lastResult && _hasErrors) { +// if (_lastResult && _hasErrors) { + if (_hasErrors) { Resizer.show($problemsPanel); } } @@ -480,7 +483,7 @@ define(function (require, exports, module) { $("#status-inspection").click(function () { // Clicking indicator toggles error panel, if any errors in current file - if (_lastResult && _hasErrors) { + if (_hasErrors) { toggleCollapsed(); } }); @@ -491,11 +494,11 @@ define(function (require, exports, module) { }); // Testing - exports.unregisterAll = _unregisterAll; + exports._unregisterAll = _unregisterAll; // Public API - exports.register = register; - exports.Type = Type; - exports.toggleEnabled = toggleEnabled; - exports.inspectFile = inspectFile; + exports.register = register; + exports.Type = Type; + exports.toggleEnabled = toggleEnabled; + exports.inspectFile = inspectFile; }); diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index 4586956b04d..c78ca870f14 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -187,7 +187,7 @@ define({ // CodeInspection: errors/warnings "CODE_INSPECTION_PANEL_TITLE" : "Code Inspection", - "ERRORS_PANEL_TITLE" : "{0} Errors", + "ERRORS_PANEL_TITLE_SINGLE" : "{0} Issues", "SINGLE_ERROR" : "1 {0} Error", "MULTIPLE_ERRORS" : "{1} {0} Errors", "NO_ERRORS" : "No {0} errors - good job!", @@ -271,7 +271,6 @@ define({ "CMD_TOGGLE_WORD_WRAP" : "Word Wrap", "CMD_LIVE_HIGHLIGHT" : "Live Preview Highlight", "CMD_VIEW_TOGGLE_INSPECTION" : "Lint Files on Save", - "CMD_VIEW_ENABLE_INSPECTOR" : " Enable {0}", "CMD_SORT_WORKINGSET_BY_ADDED" : "Sort by Added", "CMD_SORT_WORKINGSET_BY_NAME" : "Sort by Name", "CMD_SORT_WORKINGSET_BY_TYPE" : "Sort by Type", diff --git a/src/styles/brackets.less b/src/styles/brackets.less index fb130c38f68..b6cc5fa37fe 100644 --- a/src/styles/brackets.less +++ b/src/styles/brackets.less @@ -928,7 +928,7 @@ a, img { } } -#search-results #problems-panel .disclosure-triangle { +#search-results, #problems-panel .disclosure-triangle { .jstree-sprite; display: inline-block; &.expanded { @@ -942,22 +942,6 @@ a, img { } } - -#problems-panel .disclosure-triangle { - .jstree-sprite; - display: inline-block; - &.expanded { - // Unfortunately, the way jsTree sprites are aligned within their 18px boxes doesn't look good in - // other contexts, so we need some tweaks here instead of straight multiples of @jstree-sprite-size - background-position: 7px 5px; - -webkit-transform: translateZ(0) rotate(90deg); - } - &.collapsed { - background-position: 7px 5px; - } -} - - /* Modal bar for Find/Quick Open */ .modal-bar { diff --git a/src/utils/Async.js b/src/utils/Async.js index f503b1aae3b..1e97a9cc91c 100644 --- a/src/utils/Async.js +++ b/src/utils/Async.js @@ -280,51 +280,7 @@ define(function (require, exports, module) { return masterDeferred.promise(); } - - /** - * Executes a series of tasks in parallel, saving up result info from any that succeeds along the way. - * Returns a Promise that is only resolved/rejected once all tasks are complete. This is - * essentially a wrapper around doInParallel(..., false). - * - * If one or more tasks failed, the entire "master" promise is rejected at the end without any further details. - * Otherwise the promise gets resolved with the results from all tasks - * argument: an array objects, one per failed task. Each error object contains: - * - item -- the entry in items whose task failed - * - result -- the first argument passed to the done() handler when the task succeeds - * - * @param {!Array.<*>} items - * @param {!function(*, number):Promise} beginProcessItem - * @return {$.Promise} - */ - function doInParallel_aggregateResults(items, beginProcessItem) { - var results = []; - - var masterDeferred = new $.Deferred(); - - var parallelResult = doInParallel( - items, - function (item, i) { - var itemResult = beginProcessItem(item, i); - itemResult.done(function (result) { - results.push({ item: item, results: result }); - }); - return itemResult; - }, - false - ); - parallelResult - .done(function () { - masterDeferred.resolve(results); - }) - .fail(function () { - masterDeferred.reject(); - }); - - return masterDeferred.promise(); - } - - /** Value passed to fail() handlers that have been triggered due to withTimeout()'s timeout */ var ERROR_TIMEOUT = {}; @@ -468,7 +424,6 @@ define(function (require, exports, module) { exports.doSequentially = doSequentially; exports.doSequentiallyInBackground = doSequentiallyInBackground; exports.doInParallel_aggregateErrors = doInParallel_aggregateErrors; - exports.doInParallel_aggregateResults = doInParallel_aggregateResults; exports.withTimeout = withTimeout; exports.ERROR_TIMEOUT = ERROR_TIMEOUT; exports.chain = chain; diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index a525c90fc81..9ef5a102b80 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -50,7 +50,7 @@ define(function (require, exports, module) { name: name, // arguments to this function: text, fullPath // omit the warning - scanFile: function () { return {errors: result}; } + scanFile: function () { return result; } }; spyOn(provider, "scanFile").andCallThrough(); @@ -58,6 +58,10 @@ define(function (require, exports, module) { return provider; } + function successLintResult() { + return {errors: []}; + } + beforeFirst(function () { runs(function () { SpecRunnerUtils.createTestWindowAndRun(this, function (w) { @@ -92,12 +96,12 @@ define(function (require, exports, module) { var simpleJavascriptFileEntry; beforeEach(function () { - CodeInspection.unregisterAll(); + CodeInspection._unregisterAll(); simpleJavascriptFileEntry = new FileSystem.getFileForPath(testFolder + "/errors.js"); }); it("should run a single registered linter", function () { - var codeInspector = createCodeInspector("text linter", []); + var codeInspector = createCodeInspector("text linter", successLintResult()); CodeInspection.register("javascript", codeInspector); runs(function () { @@ -112,8 +116,8 @@ define(function (require, exports, module) { }); it("should run two linters", function () { - var codeInspector1 = createCodeInspector("text linter 1", []); - var codeInspector2 = createCodeInspector("text linter 2", []); + var codeInspector1 = createCodeInspector("text linter 1", successLintResult()); + var codeInspector2 = createCodeInspector("text linter 2", successLintResult()); CodeInspection.register("javascript", codeInspector1); CodeInspection.register("javascript", codeInspector2); @@ -134,12 +138,16 @@ define(function (require, exports, module) { var result; var lintResult = { - pos: { line: 2, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING + errors: [ + { + pos: { line: 2, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + } + ] }; - var codeInspector1 = createCodeInspector("javascript linter", [lintResult]); + var codeInspector1 = createCodeInspector("javascript linter", lintResult); CodeInspection.register("javascript", codeInspector1); runs(function () { @@ -154,8 +162,8 @@ define(function (require, exports, module) { runs(function () { expect(codeInspector1.scanFile).toHaveBeenCalled(); expect(result.length).toEqual(1); - expect(result[0].item.name).toEqual("javascript linter"); - expect(result[0].results.errors.length).toEqual(1); + expect(result[0].provider.name).toEqual("javascript linter"); + expect(result[0].issues.errors.length).toEqual(1); }); }); @@ -163,13 +171,17 @@ define(function (require, exports, module) { var result; var lintResult = { - pos: { line: 2, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING + errors: [ + { + pos: { line: 2, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + } + ] }; - var codeInspector1 = createCodeInspector("javascript linter 1", [lintResult]); - var codeInspector2 = createCodeInspector("javascript linter 2", [lintResult]); + var codeInspector1 = createCodeInspector("javascript linter 1", lintResult); + var codeInspector2 = createCodeInspector("javascript linter 2", lintResult); CodeInspection.register("javascript", codeInspector1); CodeInspection.register("javascript", codeInspector2); @@ -184,13 +196,13 @@ define(function (require, exports, module) { runs(function () { expect(result.length).toEqual(2); - expect(result[0].results.errors.length).toEqual(1); - expect(result[1].results.errors.length).toEqual(1); + expect(result[0].issues.errors.length).toEqual(1); + expect(result[1].issues.errors.length).toEqual(1); }); }); it("should not call any other linter for javascript document", function () { - var codeInspector1 = createCodeInspector("any other linter linter 1", []); + var codeInspector1 = createCodeInspector("any other linter linter 1", successLintResult()); CodeInspection.register("whatever", codeInspector1); runs(function () { @@ -205,7 +217,7 @@ define(function (require, exports, module) { }); it("should call linter even if linting on save is disabled", function () { - var codeInspector1 = createCodeInspector("javascript linter 1", []); + var codeInspector1 = createCodeInspector("javascript linter 1", successLintResult()); CodeInspection.register("javascript", codeInspector1); CodeInspection.toggleEnabled(false); @@ -243,17 +255,21 @@ define(function (require, exports, module) { describe("Code Inspection UI", function () { beforeEach(function () { - CodeInspection.unregisterAll(); + CodeInspection._unregisterAll(); }); it("should run test linter when a JavaScript document opens and indicate errors in the panel", function () { var lintResult = { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING + errors: [ + { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + } + ] }; - var codeInspector = createCodeInspector("javascript linter", [lintResult]); + var codeInspector = createCodeInspector("javascript linter", lintResult); CodeInspection.register("javascript", codeInspector); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); @@ -266,19 +282,21 @@ define(function (require, exports, module) { }); it("should show problems panel after too many errors", function () { - var lintResult = [ - { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING - }, - { - pos: { line: 1, ch: 5 }, - message: "Stopping. (33% scanned).", - type: CodeInspection.Type.META, - aborted: true - } - ]; + var lintResult = { + errors: [ + { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }, + { + pos: { line: 1, ch: 5 }, + message: "Stopping. (33% scanned).", + type: CodeInspection.Type.META + } + ], + aborted: true + }; var codeInspector = createCodeInspector("javascript linter", lintResult); CodeInspection.register("javascript", codeInspector); @@ -298,14 +316,18 @@ define(function (require, exports, module) { it("should not run test linter when a JavaScript document opens and linting is disabled", function () { var lintResult = { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING + errors: [ + { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + } + ] }; CodeInspection.toggleEnabled(false); - var codeInspector = createCodeInspector("javascript linter", [lintResult]); + var codeInspector = createCodeInspector("javascript linter", lintResult); CodeInspection.register("javascript", codeInspector); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); @@ -321,7 +343,7 @@ define(function (require, exports, module) { }); it("should not show the problems panel when there is no linting error", function () { - var codeInspector = createCodeInspector("javascript linter", []); + var codeInspector = createCodeInspector("javascript linter", successLintResult()); CodeInspection.register("javascript", codeInspector); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); @@ -335,12 +357,16 @@ define(function (require, exports, module) { it("status icon should toggle Errors panel when errors present", function () { var lintResult = { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING + errors: [ + { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + } + ] }; - var codeInspector = createCodeInspector("javascript linter", [lintResult]); + var codeInspector = createCodeInspector("javascript linter", lintResult); CodeInspection.register("javascript", codeInspector); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); @@ -353,13 +379,17 @@ define(function (require, exports, module) { it("should run two linter and display two expanded collapsible sections in the errors panel", function () { var lintResult = { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING + errors: [ + { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + } + ] }; - var codeInspector1 = createCodeInspector("javascript linter 1", [lintResult]); - var codeInspector2 = createCodeInspector("javascript linter 2", [lintResult]); + var codeInspector1 = createCodeInspector("javascript linter 1", lintResult); + var codeInspector2 = createCodeInspector("javascript linter 2", lintResult); CodeInspection.register("javascript", codeInspector1); CodeInspection.register("javascript", codeInspector2); @@ -378,12 +408,16 @@ define(function (require, exports, module) { it("should run the linter and display no collapsible header section in the errors panel", function () { var lintResult = { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING + errors: [ + { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + } + ] }; - var codeInspector1 = createCodeInspector("javascript linter 1", [lintResult]); + var codeInspector1 = createCodeInspector("javascript linter 1", lintResult); CodeInspection.register("javascript", codeInspector1); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); @@ -394,7 +428,7 @@ define(function (require, exports, module) { }); it("status icon should not toggle Errors panel when no errors present", function () { - var codeInspector = createCodeInspector("javascript linter", []); + var codeInspector = createCodeInspector("javascript linter", successLintResult()); CodeInspection.register("javascript", codeInspector); waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file"); @@ -405,5 +439,70 @@ define(function (require, exports, module) { }); }); }); + + describe("Code Inspector Registration", function () { + beforeEach(function () { + CodeInspection._unregisterAll(); + }); + + it("should overwrite inspector 1 with inspector 2 and inspector 2 should be called", function () { + var codeInspector1 = createCodeInspector("javascript inspector", successLintResult()); + CodeInspection.register("javascript", codeInspector1); + var codeInspector2 = createCodeInspector("javascript inspector", successLintResult()); + CodeInspection.register("javascript", codeInspector2, true); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file", 5000); + + runs(function () { + expect(codeInspector1.scanFile).not.toHaveBeenCalled(); + expect(codeInspector2.scanFile).toHaveBeenCalled(); + }); + }); + + it("should call inspector 1 and inspector 2", function () { + var codeInspector1 = createCodeInspector("javascript inspector 1", successLintResult()); + CodeInspection.register("javascript", codeInspector1); + var codeInspector2 = createCodeInspector("javascript inspector 2", successLintResult()); + CodeInspection.register("javascript", codeInspector2); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file", 5000); + + runs(function () { + expect(codeInspector1.scanFile).toHaveBeenCalled(); + expect(codeInspector2.scanFile).toHaveBeenCalled(); + }); + }); + + it("should keep inspector 1 because the name of inspector 2 is different", function () { + var codeInspector1 = createCodeInspector("javascript inspector 1", successLintResult()); + CodeInspection.register("javascript", codeInspector1); + var codeInspector2 = createCodeInspector("javascript inspector 2", successLintResult()); + CodeInspection.register("javascript", codeInspector2, true); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file", 5000); + + runs(function () { + expect(codeInspector1.scanFile).toHaveBeenCalled(); + expect(codeInspector2.scanFile).toHaveBeenCalled(); + }); + }); + + it("should register the same inspector multiple times and overwrite it with some other inspector with the same name", function () { + var codeInspector1 = createCodeInspector("javascript inspector", successLintResult()); + CodeInspection.register("javascript", codeInspector1); + var codeInspector2 = createCodeInspector("javascript inspector", successLintResult()); + CodeInspection.register("javascript", codeInspector2); + var codeInspector3 = createCodeInspector("javascript inspector", successLintResult()); + CodeInspection.register("javascript", codeInspector3, true); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file", 5000); + + runs(function () { + expect(codeInspector1.scanFile).not.toHaveBeenCalled(); + expect(codeInspector2.scanFile).not.toHaveBeenCalled(); + expect(codeInspector3.scanFile).toHaveBeenCalled(); + }); + }); + }); }); }); From 0f0f012c442f12c045420436e2f6fe02890b9b1d Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Thu, 5 Dec 2013 01:01:54 -0800 Subject: [PATCH 08/14] - refactored unit tests - fixed find panel css issues - better solution for problems panel title --- src/styles/brackets.less | 10 +- test/spec/CodeInspection-test.js | 166 +++++++++++++------------------ 2 files changed, 73 insertions(+), 103 deletions(-) diff --git a/src/styles/brackets.less b/src/styles/brackets.less index b6cc5fa37fe..50b01e6aac6 100644 --- a/src/styles/brackets.less +++ b/src/styles/brackets.less @@ -928,7 +928,7 @@ a, img { } } -#search-results, #problems-panel .disclosure-triangle { +#search-results .disclosure-triangle, #problems-panel .disclosure-triangle { .jstree-sprite; display: inline-block; &.expanded { @@ -1201,10 +1201,6 @@ a, img { text-align: right; // make line number line up with editor line numbers } - td { - padding-left: 3px; - } - .line-text { white-space: nowrap; width: auto; @@ -1215,6 +1211,10 @@ a, img { width: 100%; padding-left: 10px; } + + .inspector-section > td { + padding-left: 5px; + } } /* Line up label text and input text */ diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index 9ef5a102b80..0fab842da30 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -58,10 +58,22 @@ define(function (require, exports, module) { return provider; } - function successLintResult() { + function successfulLintResult() { return {errors: []}; } - + + function failLintResult() { + return { + errors: [ + { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + } + ] + }; + } + beforeFirst(function () { runs(function () { SpecRunnerUtils.createTestWindowAndRun(this, function (w) { @@ -101,7 +113,7 @@ define(function (require, exports, module) { }); it("should run a single registered linter", function () { - var codeInspector = createCodeInspector("text linter", successLintResult()); + var codeInspector = createCodeInspector("text linter", successfulLintResult()); CodeInspection.register("javascript", codeInspector); runs(function () { @@ -116,8 +128,8 @@ define(function (require, exports, module) { }); it("should run two linters", function () { - var codeInspector1 = createCodeInspector("text linter 1", successLintResult()); - var codeInspector2 = createCodeInspector("text linter 2", successLintResult()); + var codeInspector1 = createCodeInspector("text linter 1", successfulLintResult()); + var codeInspector2 = createCodeInspector("text linter 2", successfulLintResult()); CodeInspection.register("javascript", codeInspector1); CodeInspection.register("javascript", codeInspector2); @@ -137,17 +149,7 @@ define(function (require, exports, module) { it("should run one linter return some errors", function () { var result; - var lintResult = { - errors: [ - { - pos: { line: 2, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING - } - ] - }; - - var codeInspector1 = createCodeInspector("javascript linter", lintResult); + var codeInspector1 = createCodeInspector("javascript linter", failLintResult()); CodeInspection.register("javascript", codeInspector1); runs(function () { @@ -170,18 +172,8 @@ define(function (require, exports, module) { it("should run two linter and return some errors", function () { var result; - var lintResult = { - errors: [ - { - pos: { line: 2, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING - } - ] - }; - - var codeInspector1 = createCodeInspector("javascript linter 1", lintResult); - var codeInspector2 = createCodeInspector("javascript linter 2", lintResult); + var codeInspector1 = createCodeInspector("javascript linter 1", failLintResult()); + var codeInspector2 = createCodeInspector("javascript linter 2", failLintResult()); CodeInspection.register("javascript", codeInspector1); CodeInspection.register("javascript", codeInspector2); @@ -202,7 +194,7 @@ define(function (require, exports, module) { }); it("should not call any other linter for javascript document", function () { - var codeInspector1 = createCodeInspector("any other linter linter 1", successLintResult()); + var codeInspector1 = createCodeInspector("any other linter linter 1", successfulLintResult()); CodeInspection.register("whatever", codeInspector1); runs(function () { @@ -217,7 +209,7 @@ define(function (require, exports, module) { }); it("should call linter even if linting on save is disabled", function () { - var codeInspector1 = createCodeInspector("javascript linter 1", successLintResult()); + var codeInspector1 = createCodeInspector("javascript linter 1", successfulLintResult()); CodeInspection.register("javascript", codeInspector1); CodeInspection.toggleEnabled(false); @@ -259,17 +251,7 @@ define(function (require, exports, module) { }); it("should run test linter when a JavaScript document opens and indicate errors in the panel", function () { - var lintResult = { - errors: [ - { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING - } - ] - }; - - var codeInspector = createCodeInspector("javascript linter", lintResult); + var codeInspector = createCodeInspector("javascript linter", failLintResult()); CodeInspection.register("javascript", codeInspector); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); @@ -315,19 +297,9 @@ define(function (require, exports, module) { }); it("should not run test linter when a JavaScript document opens and linting is disabled", function () { - var lintResult = { - errors: [ - { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING - } - ] - }; - CodeInspection.toggleEnabled(false); - var codeInspector = createCodeInspector("javascript linter", lintResult); + var codeInspector = createCodeInspector("javascript linter", failLintResult()); CodeInspection.register("javascript", codeInspector); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); @@ -343,7 +315,7 @@ define(function (require, exports, module) { }); it("should not show the problems panel when there is no linting error", function () { - var codeInspector = createCodeInspector("javascript linter", successLintResult()); + var codeInspector = createCodeInspector("javascript linter", successfulLintResult()); CodeInspection.register("javascript", codeInspector); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); @@ -356,17 +328,7 @@ define(function (require, exports, module) { }); it("status icon should toggle Errors panel when errors present", function () { - var lintResult = { - errors: [ - { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING - } - ] - }; - - var codeInspector = createCodeInspector("javascript linter", lintResult); + var codeInspector = createCodeInspector("javascript linter", failLintResult()); CodeInspection.register("javascript", codeInspector); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); @@ -378,18 +340,8 @@ define(function (require, exports, module) { }); it("should run two linter and display two expanded collapsible sections in the errors panel", function () { - var lintResult = { - errors: [ - { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING - } - ] - }; - - var codeInspector1 = createCodeInspector("javascript linter 1", lintResult); - var codeInspector2 = createCodeInspector("javascript linter 2", lintResult); + var codeInspector1 = createCodeInspector("javascript linter 1", failLintResult()); + var codeInspector2 = createCodeInspector("javascript linter 2", failLintResult()); CodeInspection.register("javascript", codeInspector1); CodeInspection.register("javascript", codeInspector2); @@ -407,17 +359,7 @@ define(function (require, exports, module) { }); it("should run the linter and display no collapsible header section in the errors panel", function () { - var lintResult = { - errors: [ - { - pos: { line: 1, ch: 3 }, - message: "Some errors here and there", - type: CodeInspection.Type.WARNING - } - ] - }; - - var codeInspector1 = createCodeInspector("javascript linter 1", lintResult); + var codeInspector1 = createCodeInspector("javascript linter 1", failLintResult()); CodeInspection.register("javascript", codeInspector1); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000); @@ -428,7 +370,7 @@ define(function (require, exports, module) { }); it("status icon should not toggle Errors panel when no errors present", function () { - var codeInspector = createCodeInspector("javascript linter", successLintResult()); + var codeInspector = createCodeInspector("javascript linter", successfulLintResult()); CodeInspection.register("javascript", codeInspector); waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file"); @@ -438,17 +380,45 @@ define(function (require, exports, module) { toggleJSLintResults(false); }); }); + + it("should show the name of the linter if only one linter reported errors", function () { + var codeInspector = createCodeInspector("JavaScript Linter", failLintResult()); + CodeInspection.register("javascript", codeInspector); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + + runs(function () { + var $problemPanelTitle = $("#problems-panel .title").text(); + expect($problemPanelTitle).toBe("JavaScript Linter Issues"); + }); + }); + + it("should show the generic title if more than one linter reported errors", function () { + var lintResult = failLintResult(); + + var codeInspector1 = createCodeInspector("JavaScript Linter1", lintResult); + CodeInspection.register("javascript", codeInspector1); + var codeInspector2 = createCodeInspector("JavaScript Linter2", lintResult); + CodeInspection.register("javascript", codeInspector2); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + + runs(function () { + var $problemPanelTitle = $("#problems-panel .title").text(); + expect($problemPanelTitle).toBe("2 Issues"); + }); + }); }); describe("Code Inspector Registration", function () { beforeEach(function () { - CodeInspection._unregisterAll(); + CodeInspection._unregisterAll() }); it("should overwrite inspector 1 with inspector 2 and inspector 2 should be called", function () { - var codeInspector1 = createCodeInspector("javascript inspector", successLintResult()); + var codeInspector1 = createCodeInspector("javascript inspector", successfulLintResult()); CodeInspection.register("javascript", codeInspector1); - var codeInspector2 = createCodeInspector("javascript inspector", successLintResult()); + var codeInspector2 = createCodeInspector("javascript inspector", successfulLintResult()); CodeInspection.register("javascript", codeInspector2, true); waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file", 5000); @@ -460,9 +430,9 @@ define(function (require, exports, module) { }); it("should call inspector 1 and inspector 2", function () { - var codeInspector1 = createCodeInspector("javascript inspector 1", successLintResult()); + var codeInspector1 = createCodeInspector("javascript inspector 1", successfulLintResult()); CodeInspection.register("javascript", codeInspector1); - var codeInspector2 = createCodeInspector("javascript inspector 2", successLintResult()); + var codeInspector2 = createCodeInspector("javascript inspector 2", successfulLintResult()); CodeInspection.register("javascript", codeInspector2); waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file", 5000); @@ -474,9 +444,9 @@ define(function (require, exports, module) { }); it("should keep inspector 1 because the name of inspector 2 is different", function () { - var codeInspector1 = createCodeInspector("javascript inspector 1", successLintResult()); + var codeInspector1 = createCodeInspector("javascript inspector 1", successfulLintResult()); CodeInspection.register("javascript", codeInspector1); - var codeInspector2 = createCodeInspector("javascript inspector 2", successLintResult()); + var codeInspector2 = createCodeInspector("javascript inspector 2", successfulLintResult()); CodeInspection.register("javascript", codeInspector2, true); waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file", 5000); @@ -488,11 +458,11 @@ define(function (require, exports, module) { }); it("should register the same inspector multiple times and overwrite it with some other inspector with the same name", function () { - var codeInspector1 = createCodeInspector("javascript inspector", successLintResult()); + var codeInspector1 = createCodeInspector("javascript inspector", successfulLintResult()); CodeInspection.register("javascript", codeInspector1); - var codeInspector2 = createCodeInspector("javascript inspector", successLintResult()); + var codeInspector2 = createCodeInspector("javascript inspector", successfulLintResult()); CodeInspection.register("javascript", codeInspector2); - var codeInspector3 = createCodeInspector("javascript inspector", successLintResult()); + var codeInspector3 = createCodeInspector("javascript inspector", successfulLintResult()); CodeInspection.register("javascript", codeInspector3, true); waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file", 5000); From e377576b9ce32813fe92b280b7d41deb648bf07f Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Thu, 5 Dec 2013 13:32:57 -0800 Subject: [PATCH 09/14] - remove obsolete code --- src/language/CodeInspection.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index b90d8df00b6..3bd36a40964 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -312,8 +312,10 @@ define(function (require, exports, module) { // don't show a header if there is only one provider available for this file type if (providerList.length === 1) { $problemsPanelTable.find(".inspector-section").hide(); + $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE_SINGLE, providerList[0].name)); } else { $problemsPanelTable.find(".inspector-section").show(); + $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE_SINGLE, numProblems)); } } else { // No provider for current file @@ -412,7 +414,6 @@ define(function (require, exports, module) { if (_collapsed) { Resizer.hide($problemsPanel); } else { -// if (_lastResult && _hasErrors) { if (_hasErrors) { Resizer.show($problemsPanel); } From 3ac969174dbec3138c3c15dc664ea1d26c62cdd0 Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Fri, 6 Dec 2013 00:04:09 -0800 Subject: [PATCH 10/14] - updated the problems panel title and the tooltip for various combinations of provider and reported errors --- src/language/CodeInspection.js | 43 ++++++++++++---- src/nls/root/strings.js | 9 ++-- test/spec/CodeInspection-test.js | 88 ++++++++++++++++++++++++++++++-- 3 files changed, 122 insertions(+), 18 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 3bd36a40964..1d869c35b8e 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -223,6 +223,7 @@ define(function (require, exports, module) { var numProblemsReportedByProvider = 0; var allErrors = []; var html; + var providerReportingProblems = 0; // run all the provider in parallel inspectFile(currentDoc.file, providerList).then(function (results) { @@ -240,7 +241,14 @@ define(function (require, exports, module) { if (!errors) { _hasErrors = null; Resizer.hide($problemsPanel); - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-valid", StringUtils.format(Strings.NO_ERRORS, Strings.CODE_INSPECTION_PANEL_TITLE)); + + var message = Strings.NO_ERRORS_MULTIPLE_PROVIDER; + if (providerList.length === 1) { + message = StringUtils.format(Strings.NO_ERRORS, providerList[0].name); + } + + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-valid", message); + setGotoEnabled(false); return; } @@ -272,6 +280,10 @@ define(function (require, exports, module) { results: result.issues.errors, numProblems: numProblemsReportedByProvider }); + + if (numProblemsReportedByProvider) { + providerReportingProblems++; + } } }); @@ -287,13 +299,13 @@ define(function (require, exports, module) { .append(html) .scrollTop(0); // otherwise scroll pos from previous contents is remembered - // Update the title - $problemsPanel.find(".title").text(Strings.CODE_INSPECTION_PANEL_TITLE); - if (!_collapsed) { Resizer.show($problemsPanel); } + // Update the title + $problemsPanel.find(".title").text(Strings.CODE_INSPECTION_PANEL_TITLE); + if (numProblems === 1 && !aborted) { StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", StringUtils.format(Strings.SINGLE_ERROR, Strings.CODE_INSPECTION_PANEL_TITLE)); } else { @@ -302,20 +314,33 @@ define(function (require, exports, module) { numProblems += "+"; } StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", - StringUtils.format(Strings.MULTIPLE_ERRORS, Strings.CODE_INSPECTION_PANEL_TITLE, numProblems)); + StringUtils.format(Strings.ERRORS_PANEL_TITLE_SINGLE, numProblems)); } + setGotoEnabled(true); } PerfUtils.addMeasurement(perfTimerDOM); // don't show a header if there is only one provider available for this file type - if (providerList.length === 1) { + if (providerReportingProblems === 1) { $problemsPanelTable.find(".inspector-section").hide(); - $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE_SINGLE, providerList[0].name)); - } else { + + var message; + if (numProblems === 1) { + message = StringUtils.format(Strings.SINGLE_ERROR, providerList[0].name); + } else { + message = StringUtils.format(Strings.MULTIPLE_ERRORS, providerList[0].name, numProblems); + } + + $problemsPanel.find(".title").text(message); + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", message); + } else if (providerReportingProblems > 1) { $problemsPanelTable.find(".inspector-section").show(); - $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE_SINGLE, numProblems)); + + var message2 = StringUtils.format(Strings.ERRORS_PANEL_TITLE_SINGLE, numProblems); + $problemsPanel.find(".title").text(message2); + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", message2); } } else { // No provider for current file diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index c78ca870f14..2a3f9f011de 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -187,10 +187,11 @@ define({ // CodeInspection: errors/warnings "CODE_INSPECTION_PANEL_TITLE" : "Code Inspection", - "ERRORS_PANEL_TITLE_SINGLE" : "{0} Issues", - "SINGLE_ERROR" : "1 {0} Error", - "MULTIPLE_ERRORS" : "{1} {0} Errors", - "NO_ERRORS" : "No {0} errors - good job!", + "ERRORS_PANEL_TITLE_SINGLE" : "{0} Problems", + "SINGLE_ERROR" : "1 {0} Problem", + "MULTIPLE_ERRORS" : "{1} {0} Problems", + "NO_ERRORS" : "No {0} problems found - good job!", + "NO_ERRORS_MULTIPLE_PROVIDER" : "No problems found - good job!", "LINT_DISABLED" : "Linting is disabled", "NO_LINT_AVAILABLE" : "No linter available for {0}", "NOTHING_TO_LINT" : "Nothing to lint", diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index 0fab842da30..9512773a817 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -381,7 +381,7 @@ define(function (require, exports, module) { }); }); - it("should show the name of the linter if only one linter reported errors", function () { + it("should show the error count and the name of the linter in the panel title for one error", function () { var codeInspector = createCodeInspector("JavaScript Linter", failLintResult()); CodeInspection.register("javascript", codeInspector); @@ -389,11 +389,50 @@ define(function (require, exports, module) { runs(function () { var $problemPanelTitle = $("#problems-panel .title").text(); - expect($problemPanelTitle).toBe("JavaScript Linter Issues"); + expect($problemPanelTitle).toBe("1 JavaScript Linter Problem"); + + var $statusBar = $("#status-inspection"); + expect($statusBar.is(":visible")).toBe(true); + + var tooltip = $statusBar.attr("title"); + expect(tooltip).toBe("1 JavaScript Linter Problem"); + }); + }); + + it("should show the error count and the name of the linter in the panel title and tooltip for multiple errors", function () { + var lintResult = { + errors: [ + { + pos: { line: 1, ch: 3 }, + message: "Some errors here and there", + type: CodeInspection.Type.WARNING + }, + { + pos: { line: 1, ch: 5 }, + message: "Some errors there and there and over there", + type: CodeInspection.Type.WARNING + } + ] + }; + + var codeInspector = createCodeInspector("JavaScript Linter", lintResult); + CodeInspection.register("javascript", codeInspector); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + + runs(function () { + var $problemPanelTitle = $("#problems-panel .title").text(); + expect($problemPanelTitle).toBe("2 JavaScript Linter Problems"); + + var $statusBar = $("#status-inspection"); + expect($statusBar.is(":visible")).toBe(true); + + var tooltip = $statusBar.attr("title"); + expect(tooltip).toBe("2 JavaScript Linter Problems"); }); }); - it("should show the generic title if more than one linter reported errors", function () { + it("should show the generic panel title if more than one inspector reported problems", function () { var lintResult = failLintResult(); var codeInspector1 = createCodeInspector("JavaScript Linter1", lintResult); @@ -405,14 +444,53 @@ define(function (require, exports, module) { runs(function () { var $problemPanelTitle = $("#problems-panel .title").text(); - expect($problemPanelTitle).toBe("2 Issues"); + expect($problemPanelTitle).toBe("2 Problems"); + + var $statusBar = $("#status-inspection"); + expect($statusBar.is(":visible")).toBe(true); + + var tooltip = $statusBar.attr("title"); + // tooltip will contain + in the title if the inspection was aborted + expect(tooltip).toBe("2 Problems"); + }); + }); + + it("should show no problems tooltip in status bar for multiple inspectors", function () { + var codeInspector = createCodeInspector("JavaScript Linter1", successfulLintResult()); + CodeInspection.register("javascript", codeInspector); + codeInspector = createCodeInspector("JavaScript Linter2", successfulLintResult()); + CodeInspection.register("javascript", codeInspector); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + + runs(function () { + var $statusBar = $("#status-inspection"); + expect($statusBar.is(":visible")).toBe(true); + + var tooltip = $statusBar.attr("title"); + expect(tooltip).toBe("No problems found - good job!"); + }); + }); + + it("should show no problems tooltip in status bar for 1 inspector", function () { + var codeInspector = createCodeInspector("JavaScript Linter1", successfulLintResult()); + CodeInspection.register("javascript", codeInspector); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + + runs(function () { + var $statusBar = $("#status-inspection"); + expect($statusBar.is(":visible")).toBe(true); + + var tooltip = $statusBar.attr("title"); + expect(tooltip).toBe("No JavaScript Linter1 problems found - good job!"); }); }); }); describe("Code Inspector Registration", function () { beforeEach(function () { - CodeInspection._unregisterAll() + CodeInspection._unregisterAll(); }); it("should overwrite inspector 1 with inspector 2 and inspector 2 should be called", function () { From 59e908677211ba35bb5c23d40521817e571ce621 Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Fri, 6 Dec 2013 00:16:01 -0800 Subject: [PATCH 11/14] - refactored the panel title and statusbar update code --- src/language/CodeInspection.js | 67 +++++++++++++++++----------------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 1d869c35b8e..6b27bf51327 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -201,6 +201,38 @@ define(function (require, exports, module) { return response.promise(); } + function updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, providerList, aborted) { + // don't show a header if there is only one provider available for this file type + var message; + + if (providerReportingProblems === 1) { + $problemsPanelTable.find(".inspector-section").hide(); + + if (numProblems === 1 && !aborted) { + message = StringUtils.format(Strings.SINGLE_ERROR, providerList[0].name); + } else { + if (aborted) { + numProblems += "+"; + } + + message = StringUtils.format(Strings.MULTIPLE_ERRORS, providerList[0].name, numProblems); + } + + $problemsPanel.find(".title").text(message); + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", message); + } else if (providerReportingProblems > 1) { + $problemsPanelTable.find(".inspector-section").show(); + + if (aborted) { + numProblems += "+"; + } + + message = StringUtils.format(Strings.ERRORS_PANEL_TITLE_SINGLE, numProblems); + $problemsPanel.find(".title").text(message); + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", message); + } + } + /** * Run inspector applicable to current document. Updates status bar indicator and refreshes error list in * bottom panel. @@ -303,45 +335,14 @@ define(function (require, exports, module) { Resizer.show($problemsPanel); } - // Update the title - $problemsPanel.find(".title").text(Strings.CODE_INSPECTION_PANEL_TITLE); - - if (numProblems === 1 && !aborted) { - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", StringUtils.format(Strings.SINGLE_ERROR, Strings.CODE_INSPECTION_PANEL_TITLE)); - } else { - // If inspector was unable to process the whole file, number of errors is indeterminate; indicate with a "+" - if (aborted) { - numProblems += "+"; - } - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", - StringUtils.format(Strings.ERRORS_PANEL_TITLE_SINGLE, numProblems)); - } + updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, providerList, aborted); setGotoEnabled(true); } PerfUtils.addMeasurement(perfTimerDOM); - // don't show a header if there is only one provider available for this file type - if (providerReportingProblems === 1) { - $problemsPanelTable.find(".inspector-section").hide(); - - var message; - if (numProblems === 1) { - message = StringUtils.format(Strings.SINGLE_ERROR, providerList[0].name); - } else { - message = StringUtils.format(Strings.MULTIPLE_ERRORS, providerList[0].name, numProblems); - } - - $problemsPanel.find(".title").text(message); - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", message); - } else if (providerReportingProblems > 1) { - $problemsPanelTable.find(".inspector-section").show(); - - var message2 = StringUtils.format(Strings.ERRORS_PANEL_TITLE_SINGLE, numProblems); - $problemsPanel.find(".title").text(message2); - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", message2); - } + updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, providerList, aborted); } else { // No provider for current file _hasErrors = false; From 4160ab4f806b1235ebd410fa5102f42e7743f511 Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Thu, 19 Dec 2013 17:30:05 -0800 Subject: [PATCH 12/14] - some changes based on review comments --- src/htmlContent/problems-panel-table.html | 2 +- src/language/CodeInspection.js | 111 ++++++++++++---------- src/nls/root/strings.js | 3 +- test/spec/CodeInspection-test.js | 27 +----- 4 files changed, 67 insertions(+), 76 deletions(-) diff --git a/src/htmlContent/problems-panel-table.html b/src/htmlContent/problems-panel-table.html index 1709be5dd87..2e382840afd 100644 --- a/src/htmlContent/problems-panel-table.html +++ b/src/htmlContent/problems-panel-table.html @@ -2,7 +2,7 @@ {{#reportList}} - + {{#results}} diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 6b27bf51327..26309e0be85 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -157,15 +157,16 @@ define(function (require, exports, module) { * These results will reflect any unsaved changes present in the file that is currently opened. * * @param {!File} file File that will be inspected for errors. - * @param ?[{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}] providerList - * @return {$.Promise} a jQuery promise that will be resolved with Array.<{provider: {{name:string, scanFile:function(string, string)}, issues: ?{!errors:Array, aborted:boolean}}}> + * @param ?{{name:string, scanFile:function(string, string):?{!errors:Array.<{pos:{line:integer, pos:integer}, message:string, type:String}>, aborted:boolean}} provider + * @return {$.Promise} a jQuery promise that will be resolved with ?{!errors:Array.<{pos:{line:integer, pos:integer}, message:string, type:String}>, aborted:boolean} */ function inspectFile(file, providerList) { var response = new $.Deferred(), - _providerList = (providerList || getProvidersForPath(file.fullPath)) || [], results = []; - if (!_providerList.length) { + providerList = (providerList || getProvidersForPath(file.fullPath)) || []; + + if (!providerList.length) { response.resolve(null); return response.promise(); } @@ -174,12 +175,12 @@ define(function (require, exports, module) { .done(function (fileText) { var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + file.fullPath); - _providerList.forEach(function (provider) { + providerList.forEach(function (provider) { var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath); try { var scanResult = provider.scanFile(fileText, file.fullPath); - results.push({provider: provider, issues: scanResult}); + results.push({provider: provider, result: scanResult}); } catch (err) { console.error("[CodeInspection] Provider " + provider.name + " threw an error: " + err); response.reject(err); @@ -188,10 +189,10 @@ define(function (require, exports, module) { PerfUtils.addMeasurement(perfTimerProvider); }); - - response.resolve(results); PerfUtils.addMeasurement(perfTimerInspector); + + response.resolve(results); }) .fail(function (err) { console.error("[CodeInspection] Could not read file for inspection: " + file.fullPath); @@ -201,33 +202,41 @@ define(function (require, exports, module) { return response.promise(); } - function updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, providerList, aborted) { - // don't show a header if there is only one provider available for this file type + /** + * Update the title of the problem panel and the tooltip of the status bar icon. The title and the tooltip will + * change based on the number of problems reported and how many provider reported problems. + * + * @param {Number} numProblems - how many problems have been reported + * @param {Array.<{name:string, scanFile:function(string, string):Object}>} providerReportingProblems - provider that reported problems + * @param {boolean} aborted - code inspection was aborted previously + */ + function updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, aborted) { var message; - if (providerReportingProblems === 1) { + if (providerReportingProblems.length === 1) { + // don't show a header if there is only one provider available for this file type $problemsPanelTable.find(".inspector-section").hide(); if (numProblems === 1 && !aborted) { - message = StringUtils.format(Strings.SINGLE_ERROR, providerList[0].name); + message = StringUtils.format(Strings.SINGLE_ERROR, providerReportingProblems[0].name); } else { if (aborted) { numProblems += "+"; } - message = StringUtils.format(Strings.MULTIPLE_ERRORS, providerList[0].name, numProblems); + message = StringUtils.format(Strings.MULTIPLE_ERRORS, providerReportingProblems[0].name, numProblems); } $problemsPanel.find(".title").text(message); StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", message); - } else if (providerReportingProblems > 1) { + } else if (providerReportingProblems.length > 1) { $problemsPanelTable.find(".inspector-section").show(); if (aborted) { numProblems += "+"; } - message = StringUtils.format(Strings.ERRORS_PANEL_TITLE_SINGLE, numProblems); + message = StringUtils.format(Strings.ERRORS_PANEL_TITLE_MULTIPLE, numProblems); $problemsPanel.find(".title").text(message); StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", message); } @@ -252,10 +261,9 @@ define(function (require, exports, module) { if (providerList && providerList.length) { var numProblems = 0; var aborted = false; - var numProblemsReportedByProvider = 0; var allErrors = []; var html; - var providerReportingProblems = 0; + var providerReportingProblems = []; // run all the provider in parallel inspectFile(currentDoc.file, providerList).then(function (results) { @@ -265,13 +273,12 @@ define(function (require, exports, module) { } // how many errors in total? - var errors = results.reduce(function (a, item) { return a + (item.issues ? item.issues.errors.length : 0); }, 0); + var errors = results.reduce(function (a, item) { return a + (item.result ? item.result.errors.length : 0); }, 0); // save for later and make the amount of errors easily available _hasErrors = !!errors; if (!errors) { - _hasErrors = null; Resizer.hide($problemsPanel); var message = Strings.NO_ERRORS_MULTIPLE_PROVIDER; @@ -286,35 +293,35 @@ define(function (require, exports, module) { } // Augment error objects with additional fields needed by Mustache template - results.forEach(function (result) { - numProblemsReportedByProvider = 0; - var provider = result.provider; - - if (result.issues) { - result.issues.errors.forEach(function (error) { - error.friendlyLine = error.pos.line + 1; - error.codeSnippet = currentDoc.getLine(error.pos.line); - error.codeSnippet = error.codeSnippet.substr(0, Math.min(175, error.codeSnippet.length)); // limit snippet width + results.forEach(function (inspectionResult) { + var provider = inspectionResult.provider; + + if (inspectionResult.result) { + inspectionResult.result.errors.forEach(function (error) { + // the CSS linter return an "you have specific issues ion your file message without line number and snippet + if (!isNaN(error.pos.line)) { + error.friendlyLine = error.pos.line + 1; + error.codeSnippet = currentDoc.getLine(error.pos.line); + error.codeSnippet = error.codeSnippet.substr(0, Math.min(175, error.codeSnippet.length)); // limit snippet width + } if (error.type !== Type.META) { numProblems++; - numProblemsReportedByProvider++; } }); // if the code inspector was unable to process the whole file, we keep track to show a different status - if (result.issues.aborted) { + if (inspectionResult.result.aborted) { aborted = true; } - allErrors.push({ - providerName: provider.name, - results: result.issues.errors, - numProblems: numProblemsReportedByProvider - }); + if (inspectionResult.result.errors) { + allErrors.push({ + providerName: provider.name, + results: inspectionResult.result.errors + }); - if (numProblemsReportedByProvider) { - providerReportingProblems++; + providerReportingProblems.push(provider); } } }); @@ -335,14 +342,12 @@ define(function (require, exports, module) { Resizer.show($problemsPanel); } - updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, providerList, aborted); - setGotoEnabled(true); } PerfUtils.addMeasurement(perfTimerDOM); - updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, providerList, aborted); + updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, aborted); } else { // No provider for current file _hasErrors = false; @@ -368,20 +373,22 @@ define(function (require, exports, module) { * Each error is: { pos:{line,ch}, endPos:?{line,ch}, message:string, type:?Type } * If type is unspecified, Type.WARNING is assumed. */ - function register(languageId, provider, replaceProvider) { + function register(languageId, provider) { if (!_providers[languageId]) { _providers[languageId] = []; } - if (replaceProvider) { - // remove the provider with the same name from the list - console.log("[CodeInspection] Replacing code inspector '" + provider.name + "' with new implementation"); + if (languageId === "javascript") { + // This is a special case to enable extension provider to replace the JSLint provider + // in favor of their own implementation _.remove(_providers[languageId], function (registeredProvider) { - return registeredProvider.name === provider.name; + return registeredProvider.name === "JSLint"; }); } _providers[languageId].push(provider); + + run(); } /** @@ -454,7 +461,6 @@ define(function (require, exports, module) { } } - // Register command handlers CommandManager.register(Strings.CMD_VIEW_TOGGLE_INSPECTION, Commands.VIEW_TOGGLE_INSPECTION, toggleEnabled); CommandManager.register(Strings.CMD_GOTO_FIRST_PROBLEM, Commands.NAVIGATE_GOTO_FIRST_PROBLEM, handleGotoFirstProblem); @@ -491,11 +497,14 @@ define(function (require, exports, module) { // Grab the required position data var lineTd = $selectedRow.find(".line-number"); var line = parseInt(lineTd.text(), 10) - 1; // convert friendlyLine back to pos.line - var character = lineTd.data("character"); - - var editor = EditorManager.getCurrentFullEditor(); - editor.setCursorPos(line, character, true); - EditorManager.focusEditor(); + // if there is no line number available, don't do anything + if (!isNaN(line)) { + var character = lineTd.data("character"); + + var editor = EditorManager.getCurrentFullEditor(); + editor.setCursorPos(line, character, true); + EditorManager.focusEditor(); + } } }); diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index 2a3f9f011de..e4e116ff2d8 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -186,8 +186,7 @@ define({ "STATUSBAR_LINE_COUNT_PLURAL" : "\u2014 {0} Lines", // CodeInspection: errors/warnings - "CODE_INSPECTION_PANEL_TITLE" : "Code Inspection", - "ERRORS_PANEL_TITLE_SINGLE" : "{0} Problems", + "ERRORS_PANEL_TITLE_MULTIPLE" : "{0} Problems", "SINGLE_ERROR" : "1 {0} Problem", "MULTIPLE_ERRORS" : "{1} {0} Problems", "NO_ERRORS" : "No {0} problems found - good job!", diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index 9512773a817..47a512d18b2 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -165,7 +165,7 @@ define(function (require, exports, module) { expect(codeInspector1.scanFile).toHaveBeenCalled(); expect(result.length).toEqual(1); expect(result[0].provider.name).toEqual("javascript linter"); - expect(result[0].issues.errors.length).toEqual(1); + expect(result[0].result.errors.length).toEqual(1); }); }); @@ -188,8 +188,8 @@ define(function (require, exports, module) { runs(function () { expect(result.length).toEqual(2); - expect(result[0].issues.errors.length).toEqual(1); - expect(result[1].issues.errors.length).toEqual(1); + expect(result[0].result.errors.length).toEqual(1); + expect(result[1].result.errors.length).toEqual(1); }); }); @@ -493,8 +493,8 @@ define(function (require, exports, module) { CodeInspection._unregisterAll(); }); - it("should overwrite inspector 1 with inspector 2 and inspector 2 should be called", function () { - var codeInspector1 = createCodeInspector("javascript inspector", successfulLintResult()); + it("should unregister JSLint linter if a new javascript linter is registered", function () { + var codeInspector1 = createCodeInspector("JSLint", successfulLintResult()); CodeInspection.register("javascript", codeInspector1); var codeInspector2 = createCodeInspector("javascript inspector", successfulLintResult()); CodeInspection.register("javascript", codeInspector2, true); @@ -534,23 +534,6 @@ define(function (require, exports, module) { expect(codeInspector2.scanFile).toHaveBeenCalled(); }); }); - - it("should register the same inspector multiple times and overwrite it with some other inspector with the same name", function () { - var codeInspector1 = createCodeInspector("javascript inspector", successfulLintResult()); - CodeInspection.register("javascript", codeInspector1); - var codeInspector2 = createCodeInspector("javascript inspector", successfulLintResult()); - CodeInspection.register("javascript", codeInspector2); - var codeInspector3 = createCodeInspector("javascript inspector", successfulLintResult()); - CodeInspection.register("javascript", codeInspector3, true); - - waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test file", 5000); - - runs(function () { - expect(codeInspector1.scanFile).not.toHaveBeenCalled(); - expect(codeInspector2.scanFile).not.toHaveBeenCalled(); - expect(codeInspector3.scanFile).toHaveBeenCalled(); - }); - }); }); }); }); From 12d33b998806ec10f791602a30359d32271c0ac0 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Wed, 8 Jan 2014 19:37:59 -0800 Subject: [PATCH 13/14] Address remaining code review comments and update/fix documentation --- src/language/CodeInspection.js | 64 +++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 26309e0be85..8e764bf13c4 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -152,13 +152,18 @@ define(function (require, exports, module) { } /** - * Runs a file inspection over passed file, specifying a provider is optional. + * Runs a file inspection over passed file. Uses the given list of providers if specified, otherwise uses + * the set of providers that are registered for the file's language. * This method doesn't update the Brackets UI, just provides inspection results. - * These results will reflect any unsaved changes present in the file that is currently opened. + * These results will reflect any unsaved changes present in the file if currently open. + * + * The Promise yields an array of provider-result pair objects (the result is the return value of the + * provider's scanFile() - see register() for details). The result object may be null if there were no + * errors from that provider. The array is empty if there are no providers registered for this file. * * @param {!File} file File that will be inspected for errors. - * @param ?{{name:string, scanFile:function(string, string):?{!errors:Array.<{pos:{line:integer, pos:integer}, message:string, type:String}>, aborted:boolean}} provider - * @return {$.Promise} a jQuery promise that will be resolved with ?{!errors:Array.<{pos:{line:integer, pos:integer}, message:string, type:String}>, aborted:boolean} + * @param ?{Array.<{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}}>} providerList + * @return {$.Promise} a jQuery promise that will be resolved with !Array.<{provider:Object, result: ?{!errors:Array, aborted:boolean}}> */ function inspectFile(file, providerList) { var response = new $.Deferred(), @@ -206,30 +211,30 @@ define(function (require, exports, module) { * Update the title of the problem panel and the tooltip of the status bar icon. The title and the tooltip will * change based on the number of problems reported and how many provider reported problems. * - * @param {Number} numProblems - how many problems have been reported - * @param {Array.<{name:string, scanFile:function(string, string):Object}>} providerReportingProblems - provider that reported problems - * @param {boolean} aborted - code inspection was aborted previously + * @param {Number} numProblems - total number of problems across all providers + * @param {Array.<{name:string, scanFile:function(string, string):Object}>} providersReportingProblems - providers that reported problems + * @param {boolean} aborted - true if any provider returned a result with the 'aborted' flag set */ - function updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, aborted) { + function updatePanelTitleAndStatusBar(numProblems, providersReportingProblems, aborted) { var message; - if (providerReportingProblems.length === 1) { + if (providersReportingProblems.length === 1) { // don't show a header if there is only one provider available for this file type $problemsPanelTable.find(".inspector-section").hide(); if (numProblems === 1 && !aborted) { - message = StringUtils.format(Strings.SINGLE_ERROR, providerReportingProblems[0].name); + message = StringUtils.format(Strings.SINGLE_ERROR, providersReportingProblems[0].name); } else { if (aborted) { numProblems += "+"; } - message = StringUtils.format(Strings.MULTIPLE_ERRORS, providerReportingProblems[0].name, numProblems); + message = StringUtils.format(Strings.MULTIPLE_ERRORS, providersReportingProblems[0].name, numProblems); } $problemsPanel.find(".title").text(message); StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", message); - } else if (providerReportingProblems.length > 1) { + } else if (providersReportingProblems.length > 1) { $problemsPanelTable.find(".inspector-section").show(); if (aborted) { @@ -263,9 +268,9 @@ define(function (require, exports, module) { var aborted = false; var allErrors = []; var html; - var providerReportingProblems = []; + var providersReportingProblems = []; - // run all the provider in parallel + // run all the providers registered for this file type inspectFile(currentDoc.file, providerList).then(function (results) { // check if current document wasn't changed while inspectFile was running if (currentDoc !== DocumentManager.getCurrentDocument()) { @@ -275,8 +280,7 @@ define(function (require, exports, module) { // how many errors in total? var errors = results.reduce(function (a, item) { return a + (item.result ? item.result.errors.length : 0); }, 0); - // save for later and make the amount of errors easily available - _hasErrors = !!errors; + _hasErrors = Boolean(errors); if (!errors) { Resizer.hide($problemsPanel); @@ -292,13 +296,15 @@ define(function (require, exports, module) { return; } + var perfTimerDOM = PerfUtils.markStart("ProblemsPanel render:\t" + currentDoc.file.fullPath); + // Augment error objects with additional fields needed by Mustache template results.forEach(function (inspectionResult) { var provider = inspectionResult.provider; if (inspectionResult.result) { inspectionResult.result.errors.forEach(function (error) { - // the CSS linter return an "you have specific issues ion your file message without line number and snippet + // some inspectors don't always provide a line number if (!isNaN(error.pos.line)) { error.friendlyLine = error.pos.line + 1; error.codeSnippet = currentDoc.getLine(error.pos.line); @@ -321,18 +327,14 @@ define(function (require, exports, module) { results: inspectionResult.result.errors }); - providerReportingProblems.push(provider); + providersReportingProblems.push(provider); } } }); + // Update results table html = Mustache.render(ResultsTemplate, {reportList: allErrors}); - }); - - // Update results table - var perfTimerDOM = PerfUtils.markStart("ProblemsPanel render:\t" + currentDoc.file.fullPath); - - if (numProblems) { + $problemsPanelTable .empty() .append(html) @@ -342,12 +344,12 @@ define(function (require, exports, module) { Resizer.show($problemsPanel); } + updatePanelTitleAndStatusBar(numProblems, providersReportingProblems, aborted); setGotoEnabled(true); - } - PerfUtils.addMeasurement(perfTimerDOM); - - updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, aborted); + PerfUtils.addMeasurement(perfTimerDOM); + }); + } else { // No provider for current file _hasErrors = false; @@ -366,6 +368,10 @@ define(function (require, exports, module) { * The provider is passed the text of the file and its fullPath. Providers should not assume * that the file is open (i.e. DocumentManager.getOpenDocumentForPath() may return null) or * that the file on disk matches the text given (file may have unsaved changes). + * + * Registering any provider for the "javascript" language automatically unregisters the built-in + * Brackets JSLint provider. This is a temporary convenience until UI exists for disabling + * registered providers. * * @param {string} languageId * @param {{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}} provider @@ -388,7 +394,7 @@ define(function (require, exports, module) { _providers[languageId].push(provider); - run(); + run(); // in case a file of this type is open currently } /** From e4f215350d836c6f0cfd958a49d25e5750f218b1 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Thu, 9 Jan 2014 11:30:42 -0800 Subject: [PATCH 14/14] More documentation corrections --- src/language/CodeInspection.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index dd4350a5e3f..4d8e81b1418 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -145,7 +145,7 @@ define(function (require, exports, module) { * Decision is made depending on the file extension. * * @param {!string} filePath - * @return ?{Array.<{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}}>} provider + * @return ?{Array.<{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}>} provider */ function getProvidersForPath(filePath) { return _providers[LanguageManager.getLanguageForPath(filePath).getId()]; @@ -159,11 +159,12 @@ define(function (require, exports, module) { * * The Promise yields an array of provider-result pair objects (the result is the return value of the * provider's scanFile() - see register() for details). The result object may be null if there were no - * errors from that provider. The array is empty if there are no providers registered for this file. + * errors from that provider. + * If there are no providers registered for this file, the Promise yields null instead. * * @param {!File} file File that will be inspected for errors. - * @param ?{Array.<{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}}>} providerList - * @return {$.Promise} a jQuery promise that will be resolved with !Array.<{provider:Object, result: ?{!errors:Array, aborted:boolean}}> + * @param {?Array.<{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}>} providerList + * @return {$.Promise} a jQuery promise that will be resolved with ?Array.<{provider:Object, result: ?{errors:!Array, aborted:boolean}}> */ function inspectFile(file, providerList) { var response = new $.Deferred(), @@ -374,7 +375,7 @@ define(function (require, exports, module) { * registered providers. * * @param {string} languageId - * @param {{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}} provider + * @param {{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}} provider * * Each error is: { pos:{line,ch}, endPos:?{line,ch}, message:string, type:?Type } * If type is unspecified, Type.WARNING is assumed.
{{providerName}} ({{numProblems}})
{{providerName}} ({{numProblems}}){{providerName}} ({{results.length}})