From cc5cc6b296740ad8299cd39f783c00d58024d467 Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Tue, 17 Sep 2013 00:11:08 -0700 Subject: [PATCH 1/2] - modified the problems panel to support multiple code inspector provider for the same language - enable/disable the provider via menuitem for every registered provider - fixed the JSLint unit tests to account to the new API --- src/extensions/default/JSLint/unittests.js | 22 +- src/htmlContent/problems-panel-table.html | 9 +- src/htmlContent/problems-panel.html | 2 +- src/language/CodeInspection.js | 288 +++++++++++++++------ src/nls/root/strings.js | 2 + src/styles/brackets.less | 60 +++-- 6 files changed, 267 insertions(+), 116 deletions(-) diff --git a/src/extensions/default/JSLint/unittests.js b/src/extensions/default/JSLint/unittests.js index 05d8637b356..6f938d137a9 100644 --- a/src/extensions/default/JSLint/unittests.js +++ b/src/extensions/default/JSLint/unittests.js @@ -36,8 +36,9 @@ define(function (require, exports, module) { $, brackets, CodeInspection, - EditorManager; - + EditorManager, + previousState; + var toggleJSLintResults = function (visible) { $("#status-inspection").triggerHandler("click"); expect($("#problems-panel").is(":visible")).toBe(visible); @@ -50,9 +51,12 @@ define(function (require, exports, module) { // Load module instances from brackets.test $ = testWindow.$; brackets = testWindow.brackets; - EditorManager = testWindow.brackets.test.EditorManager; - CodeInspection = testWindow.brackets.test.CodeInspection; + 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); }); }); @@ -67,17 +71,16 @@ define(function (require, exports, module) { brackets = null; EditorManager = null; SpecRunnerUtils.closeTestWindow(); + + // revert to previous state + CodeInspection._setProviderState({name: 'JSLint'}, previousState); }); it("should run JSLint linter when a JavaScript document opens", function () { - runs(function () { - spyOn(testWindow, "JSLINT").andCallThrough(); - }); - waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); runs(function () { - expect(testWindow.JSLINT).toHaveBeenCalled(); + expect($("#problems-panel").is(":visible")).toBe(true); }); }); @@ -98,6 +101,5 @@ define(function (require, exports, module) { toggleJSLintResults(false); }); }); - }); }); 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..9d873fd7949 100644 --- a/src/htmlContent/problems-panel.html +++ b/src/htmlContent/problems-panel.html @@ -1,6 +1,6 @@
-
+
×
diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index c49c7fca97f..6aa11961ba5 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -53,6 +53,7 @@ define(function (require, exports, module) { AppInit = require("utils/AppInit"), Resizer = require("utils/Resizer"), StatusBar = require("widgets/StatusBar"), + Menus = require("command/Menus"), PanelTemplate = require("text!htmlContent/problems-panel.html"), ResultsTemplate = require("text!htmlContent/problems-panel-table.html"); @@ -118,6 +119,12 @@ define(function (require, exports, module) { */ var _lastResult; + /** + * @private + * @type {?Array.} + */ + var _allInspectorCommands = []; + /** * Enable or disable the "Go to First Error" command * @param {boolean} gotoEnabled Whether it is enabled. @@ -126,24 +133,48 @@ 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"; + } /** - * 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). + * Check if a given provider is enabled. + * Return true if enabled, false otherwise. * - * @param {string} languageId - * @param {{name:string, scanFile:function(string, string):?{!errors:Array, aborted:boolean}} provider + * @param {name:string, scanFile:function(string, string):Object} provider + */ + function getProviderState(provider) { + return _prefs.getValue(getProviderPrefKey(provider)); + } + + /** + * Store the state (enabled/disabled) for a given provider. + * Return true if enabled, false otherwise. * - * Each error is: { pos:{line,ch}, endPos:?{line,ch}, message:string, type:?Type } - * If type is unspecified, Type.WARNING is assumed. + * @param {name:string, scanFile:function(string, string):Object} provider + * @param boolean enabled */ - function register(languageId, provider) { - if (_providers[languageId]) { - console.warn("Overwriting existing inspection/linting provider for language " + languageId); - } - _providers[languageId] = provider; + function setProviderState(provider, enabled) { + _prefs.setValue(getProviderPrefKey(provider), enabled); + } + + /** + * Enable/disable all menu entries for provider/code inspector. + * + * param boolean enabled + */ + function toggleEnableAllInspectorMenuItems(enabled) { + _allInspectorCommands.forEach(function (command) { + command.setEnabled(enabled); + }); } /** @@ -160,85 +191,124 @@ define(function (require, exports, module) { var currentDoc = DocumentManager.getCurrentDocument(); + var numProblems = 0, + aborted = false, + resultList = []; + var perfTimerDOM, perfTimerInspector; var language = currentDoc ? LanguageManager.getLanguageForPath(currentDoc.file.fullPath) : ""; var languageId = language && language.getId(); - var provider = language && _providers[languageId]; + var providers = (language && _providers[languageId]) || []; - if (provider) { - perfTimerInspector = PerfUtils.markStart("CodeInspection '" + languageId + "':\t" + currentDoc.file.fullPath); + if (providers.length > 0) { + providers.forEach(function (provider) { + perfTimerInspector = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + currentDoc.file.fullPath); - var result = provider.scanFile(currentDoc.getText(), currentDoc.file.fullPath); - _lastResult = result; - - PerfUtils.addMeasurement(perfTimerInspector); - perfTimerDOM = PerfUtils.markStart("ProblemsPanel render:\t" + currentDoc.file.fullPath); - - if (result && result.errors.length) { - // 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++; + if (getProviderState(provider) === true) { + var result = provider.scanFile(currentDoc.getText(), currentDoc.file.fullPath); + _lastResult = result; + + PerfUtils.addMeasurement(perfTimerInspector); + perfTimerDOM = PerfUtils.markStart("ProblemsPanel render:\t" + currentDoc.file.fullPath); + + if (result && result.errors.length) { + // Augment error objects with additional fields needed by Mustache template + var _numProblemsReportedByProvider = 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++; + _numProblemsReportedByProvider++; + } + }); + + resultList.push({ + providerName: provider.name, + results: result.errors, + numProblems: _numProblemsReportedByProvider + }); } - }); - - // Update results table - var html = Mustache.render(ResultsTemplate, {reportList: result.errors}); - var $selectedRow; - - $problemsPanel.find(".table-container") - .empty() - .append(html) - .scrollTop(0) // otherwise scroll pos from previous contents is remembered - .on("click", "tr", function (e) { + + // 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; + } + + PerfUtils.addMeasurement(perfTimerDOM); + } + }); + + // Update results table + var html = Mustache.render(ResultsTemplate, {reportList: resultList}); + var $selectedRow; + + $problemsPanel.find(".table-container") + .empty() + .append(html) + .scrollTop(0) // otherwise scroll pos from previous contents is remembered + .off(".table-container") // Remove the old events + .on("click", function (e) { + var $row = $(e.target).closest("tr"); + + console.log("Target: " + e.target.toString()); + console.log("Header clicked" + $row.toString()); + if ($row.length) { if ($selectedRow) { $selectedRow.removeClass("selected"); } - - $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(); - }); - - $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 += "+"; + + $row.addClass("selected"); + $selectedRow = $row; + + // This is a inspector title row, expand/collapse on click + if ($row.hasClass("inspector-section")) { + // Clicking the inspector title section header collapses/expands result rows + $row.nextUntil(".inspector-section").toggle(); + + var $triangle = $(".disclosure-triangle", $row); + $triangle.toggleClass("expanded").toggleClass("collapsed"); + // This is a problem marker row, show the result on click + } else { + // Grab the required position data + var $lineTd = $selectedRow.find("td.line-number"), + line = parseInt($lineTd.text(), 10) - 1, // convert friendlyLine back to pos.line + character = $lineTd.data("character"), + editor = EditorManager.getCurrentFullEditor(); + + editor.setCursorPos(line, character, true); + EditorManager.focusEditor(); + } } - StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", - StringUtils.format(Strings.MULTIPLE_ERRORS, provider.name, numProblems)); - } - setGotoEnabled(true); + }); + $problemsPanel.find(".title").text(StringUtils.format(Strings.ERRORS_PANEL_TITLE, Strings.PROBLEMS_PANEL_TITLE)); + if (!_collapsed) { + Resizer.show($problemsPanel); + } + + if (numProblems === 1 && !aborted) { + StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-errors", StringUtils.format(Strings.SINGLE_ERROR, Strings.PROBLEMS_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.PROBLEMS_PANEL_TITLE, numProblems)); + } + setGotoEnabled(true); + + if (!numProblems) { 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.PROBLEMS_PANEL_TITLE)); setGotoEnabled(false); } - - PerfUtils.addMeasurement(perfTimerDOM); - } else { // No provider for current file _lastResult = null; @@ -252,6 +322,57 @@ define(function (require, exports, module) { } } + /** + * 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 + * 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); + + addMenuEntryForProvider(provider); + } + /** * Update DocumentManager listeners. */ @@ -286,11 +407,12 @@ define(function (require, exports, module) { updateListeners(); _prefs.setValue("enabled", _enabled); + toggleEnableAllInspectorMenuItems(_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 @@ -319,7 +441,7 @@ define(function (require, exports, module) { function handleGotoFirstProblem() { run(); if (_gotoEnabled) { - $problemsPanel.find("tr:first-child").trigger("click"); + $problemsPanel.find("tr:nth-child(2)").trigger("click"); } } @@ -362,7 +484,11 @@ define(function (require, exports, module) { // Public API - exports.register = register; - exports.Type = Type; - exports.toggleEnabled = toggleEnabled; + exports.register = register; + exports.Type = Type; + exports.toggleEnabled = toggleEnabled; + + // Testing API + exports._setProviderState = setProviderState; + exports._getProviderState = getProviderState; }); diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index 0c56df4ce58..cdb6c6d747f 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 + "PROBLEMS_PANEL_TITLE" : "Code Inspection", "ERRORS_PANEL_TITLE" : "{0} Errors", "SINGLE_ERROR" : "1 {0} Error", "MULTIPLE_ERRORS" : "{1} {0} Errors", @@ -265,6 +266,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 55affed2ceb..08706f09252 100644 --- a/src/styles/brackets.less +++ b/src/styles/brackets.less @@ -926,28 +926,6 @@ a, img { padding: 0 5px; } -.tickmark-track { - position: absolute; - bottom: 0; - top: 0; - right: 0; - width: 16px; - z-index: @z-index-cm-max; - pointer-events: none; - - .tickmark { - position: absolute; - width: 16px; - body.platform-mac & { width: 15px; } - - height: 1px; - background-color: #eddd23; - border-top: 1px solid #e0d123; - border-bottom: 1px solid #d4c620; - opacity: 0.85; // allow thumb to show through - } -} - /* Quick Open search bar & dropdown */ @@ -1080,8 +1058,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 { From 09bc17fbfff1ea5d8ebde125e13c337f89f72ea5 Mon Sep 17 00:00:00 2001 From: Ingo Richter Date: Mon, 23 Sep 2013 14:24:48 -0700 Subject: [PATCH 2/2] Fixed the issues mentioned in the code review. Removed the debug/console output. --- src/language/CodeInspection.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 6aa11961ba5..8e60b18beff 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -183,6 +183,7 @@ define(function (require, exports, module) { */ function run() { if (!_enabled) { + _lastResult = null; Resizer.hide($problemsPanel); StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-disabled", Strings.LINT_DISABLED); setGotoEnabled(false); @@ -206,7 +207,7 @@ define(function (require, exports, module) { providers.forEach(function (provider) { perfTimerInspector = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + currentDoc.file.fullPath); - if (getProviderState(provider) === true) { + if (getProviderState(provider)) { var result = provider.scanFile(currentDoc.getText(), currentDoc.file.fullPath); _lastResult = result; @@ -245,7 +246,9 @@ define(function (require, exports, module) { }); // Update results table - var html = Mustache.render(ResultsTemplate, {reportList: resultList}); + // compile creates a cached function to render the template + var resultsTemplate = Mustache.compile(ResultsTemplate); + var html = resultsTemplate({reportList: resultList}); var $selectedRow; $problemsPanel.find(".table-container") @@ -256,8 +259,6 @@ define(function (require, exports, module) { .on("click", function (e) { var $row = $(e.target).closest("tr"); - console.log("Target: " + e.target.toString()); - console.log("Header clicked" + $row.toString()); if ($row.length) { if ($selectedRow) { $selectedRow.removeClass("selected");