From 797f9bf0ab073e7c4b765e607ae5d79939abace0 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Wed, 2 Dec 2015 13:02:18 +0100 Subject: [PATCH] make quick fix be a command and a score --- .../src/features/codeActionProvider.ts | 16 ++++---- src/vs/editor/common/modes.ts | 7 ++-- .../contrib/gotoError/browser/gotoError.ts | 4 +- .../contrib/quickFix/browser/quickFix.ts | 2 +- .../browser/quickFixSelectionWidget.ts | 16 ++++---- .../contrib/quickFix/common/quickFix.ts | 7 ++-- src/vs/languages/css/common/cssWorker.ts | 17 +++++---- .../css/test/common/css-worker.test.ts | 2 +- .../typescript/common/features/quickFix.ts | 31 ++++++++++------ .../common/features/quickFixMainActions.ts | 4 +- .../test/common/features/quickfix.test.ts | 22 +++++------ .../common/extHostLanguageFeatureCommands.ts | 6 +-- .../api/common/extHostLanguageFeatures.ts | 37 +++++-------------- .../extHostLanguageFeatureCommands.test.ts | 4 +- .../api/extHostLanguageFeatures.test.ts | 14 +++---- 15 files changed, 91 insertions(+), 98 deletions(-) diff --git a/extensions/csharp-o/src/features/codeActionProvider.ts b/extensions/csharp-o/src/features/codeActionProvider.ts index 6cb11e69873..a5f50f41ae6 100644 --- a/extensions/csharp-o/src/features/codeActionProvider.ts +++ b/extensions/csharp-o/src/features/codeActionProvider.ts @@ -47,7 +47,12 @@ export default class OmnisharpCodeActionProvider extends AbstractProvider implem return { title: ca.Name, command: this._commandId, - arguments: [ca.Identifier, document, range] + arguments: [{ + Filename: document.fileName, + Selection: OmnisharpCodeActionProvider._asRange(range), + Identifier: ca.Identifier, + WantsTextChanges: true + }] }; }); }, (error) => { @@ -55,14 +60,7 @@ export default class OmnisharpCodeActionProvider extends AbstractProvider implem }); } - private _runCodeAction(id: string, document: TextDocument, range: Range): Promise { - - let req: V2.RunCodeActionRequest = { - Filename: document.fileName, - Selection: OmnisharpCodeActionProvider._asRange(range), - Identifier: id, - WantsTextChanges: true - }; + private _runCodeAction(req: V2.RunCodeActionRequest): Promise { return this._server.makeRequest(V2.RunCodeAction, req).then(response => { diff --git a/src/vs/editor/common/modes.ts b/src/vs/editor/common/modes.ts index b5598128503..94fc265b2aa 100644 --- a/src/vs/editor/common/modes.ts +++ b/src/vs/editor/common/modes.ts @@ -471,10 +471,8 @@ export interface ISuggestSupport { * Interface used to quick fix typing errors while accesing member fields. */ export interface IQuickFix { - label: string; - id: any; + command: ICommand; score: number; - documentation?: string; } export interface IQuickFixResult { @@ -484,7 +482,8 @@ export interface IQuickFixResult { export interface IQuickFixSupport { getQuickFixes(resource: URI, range: IMarker | EditorCommon.IRange): TPromise; - runQuickFixAction(resource: URI, range: EditorCommon.IRange, id: any):TPromise; + //TODO@joh this should be removed in the furture such that we can trust the command and it's args + runQuickFixAction(resource: URI, range: EditorCommon.IRange, quickFix: IQuickFix):TPromise; } export interface IParameter { diff --git a/src/vs/editor/contrib/gotoError/browser/gotoError.ts b/src/vs/editor/contrib/gotoError/browser/gotoError.ts index b206a903bde..86416bb54de 100644 --- a/src/vs/editor/contrib/gotoError/browser/gotoError.ts +++ b/src/vs/editor/contrib/gotoError/browser/gotoError.ts @@ -276,9 +276,9 @@ class MarkerNavigationWidget extends ZoneWidget.ZoneWidget { } container.span({ class: 'quickfixentry', - text: fix.label + text: fix.command.title }).on(DOM.EventType.CLICK,() => { - mode.quickFixSupport.runQuickFixAction(this.editor.getModel().getAssociatedResource(), marker, fix.id).then(result => { + mode.quickFixSupport.runQuickFixAction(this.editor.getModel().getAssociatedResource(), marker, fix).then(result => { return bulkEdit(this._eventService, this._editorService, this.editor, result.edits); }); return true; diff --git a/src/vs/editor/contrib/quickFix/browser/quickFix.ts b/src/vs/editor/contrib/quickFix/browser/quickFix.ts index 160d467807f..5a252a4df89 100644 --- a/src/vs/editor/contrib/quickFix/browser/quickFix.ts +++ b/src/vs/editor/contrib/quickFix/browser/quickFix.ts @@ -77,7 +77,7 @@ export class QuickFixController implements EditorCommon.IEditorContribution { return; } - fix.support.runQuickFixAction(this.editor.getModel().getAssociatedResource(), range, fix.id).then(result => { + fix.support.runQuickFixAction(this.editor.getModel().getAssociatedResource(), range, { command: fix.command, score: fix.score }).then(result => { if (result) { if (result.message) { this.messageService.show(Severity.Info, result.message); diff --git a/src/vs/editor/contrib/quickFix/browser/quickFixSelectionWidget.ts b/src/vs/editor/contrib/quickFix/browser/quickFixSelectionWidget.ts index 76345771792..482a1446b22 100644 --- a/src/vs/editor/contrib/quickFix/browser/quickFixSelectionWidget.ts +++ b/src/vs/editor/contrib/quickFix/browser/quickFixSelectionWidget.ts @@ -24,8 +24,10 @@ import {IQuickFix2} from '../common/quickFix'; var $ = dom.emmet; -function isQuickFix(quickfix: any) : boolean { - return quickfix && quickfix.id && quickfix.label; +function isQuickFix(quickfix: any): quickfix is IQuickFix2 { + return quickfix + && typeof (quickfix).command === 'object' + && typeof ( quickfix).command.title === 'string'; } // To be used as a tree element when we want to show a message @@ -119,9 +121,9 @@ class Controller extends TreeDefaults.DefaultController { function getHeight(tree:Tree.ITree, element:any): number { var fix = element; - if (!(element instanceof Message) && !!fix.documentation && tree.isFocused(fix)) { - return 35; - } + // if (!(element instanceof Message) && !!fix.documentation && tree.isFocused(fix)) { + // return 35; + // } return 19; } @@ -159,8 +161,8 @@ class Renderer implements Tree.IRenderer { } var quickFix = element; - templateData.main.textContent = quickFix.label; - templateData.documentationLabel.textContent = quickFix.documentation || ''; + templateData.main.textContent = quickFix.command.title; + templateData.documentationLabel.textContent = /*quickFix.documentation ||*/ ''; } public disposeTemplate(tree: Tree.ITree, templateId: string, templateData: any): void { diff --git a/src/vs/editor/contrib/quickFix/common/quickFix.ts b/src/vs/editor/contrib/quickFix/common/quickFix.ts index 9c4d90c2b43..e084770c029 100644 --- a/src/vs/editor/contrib/quickFix/common/quickFix.ts +++ b/src/vs/editor/contrib/quickFix/common/quickFix.ts @@ -19,6 +19,7 @@ export const QuickFixRegistry = new LanguageFeatureRegistry('q export interface IQuickFix2 extends IQuickFix { support: IQuickFixSupport; + id: string; } export function getQuickFixes(model: IModel, range: IRange): TPromise { @@ -29,12 +30,12 @@ export function getQuickFixes(model: IModel, range: IRange): TPromise= propertyName.length / 2 /*score_lim*/) { result.push({ - label: nls.localize('css.quickfix.rename', "Rename to '{0}'", p), - id: JSON.stringify({ type: 'rename', name: p }), - score: score + command: { + id: 'css.renameProptery', + title: nls.localize('css.quickfix.rename', "Rename to '{0}'", p), + arguments: [{ type: 'rename', name: p }] + }, + score }); } } @@ -426,12 +429,12 @@ export class CSSWorker extends AbstractModeWorker { }); } - public runQuickFixAction(resource:URI, range:EditorCommon.IRange, id: any):winjs.TPromise{ - var command = JSON.parse(id); - switch (command.type) { + public runQuickFixAction(resource: URI, range: EditorCommon.IRange, quickFix: Modes.IQuickFix): winjs.TPromise{ + let [{type, name}] = quickFix.command.arguments; + switch (type) { case 'rename': { return winjs.TPromise.as({ - edits: [{ resource, range, newText: command.name }] + edits: [{ resource, range, newText: name }] }); } } diff --git a/src/vs/languages/css/test/common/css-worker.test.ts b/src/vs/languages/css/test/common/css-worker.test.ts index 7008b440d9f..ea8371e985a 100644 --- a/src/vs/languages/css/test/common/css-worker.test.ts +++ b/src/vs/languages/css/test/common/css-worker.test.ts @@ -130,7 +130,7 @@ suite('Validation - CSS', () => { }; var assertQuickFix= function(fixes: Modes.IQuickFix[], model: mm.MirrorModel, expectedContent:string[]) { - var labels = fixes.map(f => f.label); + var labels = fixes.map(f => f.command.title); for (var index = 0; index < expectedContent.length; index++) { assert.ok(labels.indexOf(expectedContent[index]) !== -1, 'Quick fix not found: ' + expectedContent[index]); diff --git a/src/vs/languages/typescript/common/features/quickFix.ts b/src/vs/languages/typescript/common/features/quickFix.ts index 8699d805925..b7ebdcd22fd 100644 --- a/src/vs/languages/typescript/common/features/quickFix.ts +++ b/src/vs/languages/typescript/common/features/quickFix.ts @@ -15,7 +15,7 @@ import nls = require('vs/nls'); import arrays = require('vs/base/common/arrays'); import {IMarker} from 'vs/platform/markers/common/markers'; -export function evaluate(languageService: ts.LanguageService, resource: URI, range: EditorCommon.IRange, id: any): Modes.IQuickFixResult { +export function evaluate(languageService: ts.LanguageService, resource: URI, range: EditorCommon.IRange, quickFix: Modes.IQuickFix): Modes.IQuickFixResult { var filename = resource.toString(), sourceFile = languageService.getSourceFile(filename), @@ -26,7 +26,7 @@ export function evaluate(languageService: ts.LanguageService, resource: URI, ran return null; } - var command = JSON.parse(id); + var [command] = quickFix.command.arguments; switch (command.type) { case 'rename': { var start = sourceFile.getLineAndCharacterOfPosition(token.getStart()); @@ -129,9 +129,12 @@ function computeRenameProposals(languageService:ts.LanguageService, resource:URI } fixes.push({ - label: nls.localize('typescript.quickfix.rename', "Rename to '{0}'", entry.name), - id: JSON.stringify({ type: 'rename', name: entry.name }), - score: score + command: { + id: 'ts.renameTo', + title: nls.localize('typescript.quickfix.rename', "Rename to '{0}'", entry.name), + arguments: [{ type: 'rename', name: entry.name }] + }, + score }); } }); @@ -204,19 +207,25 @@ function computeAddTypeDefinitionProposals(languageService: ts.LanguageService, if (typingsMap.hasOwnProperty(currentWord)) { var mapping = typingsMap[currentWord]; var dtsRefs: string[] = Array.isArray(mapping) ? mapping : [ mapping ]; - dtsRefs.forEach((dtsRef) => { + dtsRefs.forEach((dtsRef, idx) => { result.push({ - label: nls.localize('typescript.quickfix.typeDefinitions', "Download type definition {0}", dtsRef.split('/')[1]), - id: JSON.stringify({ type: 'typedefinitions', name: dtsRef }), - score: 1 + command: { + id: 'ts.downloadDts', + title: nls.localize('typescript.quickfix.typeDefinitions', "Download type definition {0}", dtsRef.split('/')[1]), + arguments: [{ type: 'typedefinitions', name: dtsRef }] + }, + score: idx }); }); } if (strings.endsWith(resource.path, '.js')) { result.push({ - label: nls.localize('typescript.quickfix.addAsGlobal', "Mark '{0}' as global", currentWord), - id: JSON.stringify({ type: 'addglobal', name: currentWord }), + command: { + id: 'ts.addAsGlobal', + title: nls.localize('typescript.quickfix.addAsGlobal', "Mark '{0}' as global", currentWord), + arguments: [{ type: 'addglobal', name: currentWord }] + }, score: 1 }); } diff --git a/src/vs/languages/typescript/common/features/quickFixMainActions.ts b/src/vs/languages/typescript/common/features/quickFixMainActions.ts index 0037b92210b..603be62fe2d 100644 --- a/src/vs/languages/typescript/common/features/quickFixMainActions.ts +++ b/src/vs/languages/typescript/common/features/quickFixMainActions.ts @@ -34,8 +34,8 @@ export class QuickFixMainActions { this._contextService = contextService; } - public evaluate(resource: URI, range: EditorCommon.IRange, id: any) : winjs.TPromise { - var command = JSON.parse(id); + public evaluate(resource: URI, range: EditorCommon.IRange, quickFix: Modes.IQuickFix) : winjs.TPromise { + var [command] = quickFix.command.arguments; switch (command.type) { case 'typedefinitions': { return this.evaluateAddTypeDefinitionProposal(command.name, resource); diff --git a/src/vs/languages/typescript/test/common/features/quickfix.test.ts b/src/vs/languages/typescript/test/common/features/quickfix.test.ts index aef59bcd220..85fe8aacb8f 100644 --- a/src/vs/languages/typescript/test/common/features/quickfix.test.ts +++ b/src/vs/languages/typescript/test/common/features/quickfix.test.ts @@ -35,35 +35,35 @@ suite('TS - quick fix', () => { assertQuickFix('class C { private hello = 0; private world = this.hell0; }', 'a.ts', (elements) => { assert.equal(elements.length, 1); - assert.equal(elements[0].label, "Rename to 'hello'"); + assert.equal(elements[0].command.title, "Rename to 'hello'"); }); assertQuickFix('_.foo();', 'a.ts', (elements) => { assert.equal(elements.length, 2); - assert.equal(elements[0].label, "Download type definition underscore.d.ts"); - assert.equal(elements[1].label, "Download type definition lodash.d.ts"); + assert.equal(elements[0].command.title, "Download type definition underscore.d.ts"); + assert.equal(elements[1].command.title, "Download type definition lodash.d.ts"); }); assertQuickFix('describe("x");', 'a.js', (elements) => { assert.equal(elements.length, 3); - assert.equal(elements[0].label, "Download type definition mocha.d.ts"); - assert.equal(elements[1].label, "Download type definition jasmine.d.ts"); - assert.equal(elements[2].label, "Mark 'describe' as global"); + assert.equal(elements[0].command.title, "Download type definition mocha.d.ts"); + assert.equal(elements[1].command.title, "Download type definition jasmine.d.ts"); + assert.equal(elements[2].command.title, "Mark 'describe' as global"); }); assertQuickFix('angular.foo = 1;', 'a.ts', (elements) => { assert.equal(elements.length, 1); - assert.equal(elements[0].label, "Download type definition angular.d.ts"); + assert.equal(elements[0].command.title, "Download type definition angular.d.ts"); }); assertQuickFix('var x = __dirname;', 'a.ts', (elements) => { assert.equal(elements.length, 1); - assert.equal(elements[0].label, "Download type definition node.d.ts"); + assert.equal(elements[0].command.title, "Download type definition node.d.ts"); }); assertQuickFix('ko.observable(null);', 'a.ts', (elements) => { assert.equal(elements.length, 1); - assert.equal(elements[0].label, "Download type definition knockout.d.ts"); + assert.equal(elements[0].command.title, "Download type definition knockout.d.ts"); }); for (var id in quickfix.typingsMap) { @@ -76,12 +76,12 @@ suite('TS - quick fix', () => { assertQuickFix('foo.observable(null);', 'a.js', (elements) => { assert.equal(elements.length, 1); - assert.equal(elements[0].label, "Mark 'foo' as global"); + assert.equal(elements[0].command.title, "Mark 'foo' as global"); }); assertQuickFix('toString();', 'a.js', (elements) => { assert.equal(elements.length, 1); - assert.equal(elements[0].label, "Mark 'toString' as global"); + assert.equal(elements[0].command.title, "Mark 'toString' as global"); }); }); diff --git a/src/vs/workbench/api/common/extHostLanguageFeatureCommands.ts b/src/vs/workbench/api/common/extHostLanguageFeatureCommands.ts index 043fcfef2fb..2b50a57fde0 100644 --- a/src/vs/workbench/api/common/extHostLanguageFeatureCommands.ts +++ b/src/vs/workbench/api/common/extHostLanguageFeatureCommands.ts @@ -195,10 +195,10 @@ export class ExtHostLanguageFeatureCommands { range: typeConverters.fromRange(range) }; return this._commands.executeCommand('_executeCodeActionProvider', args).then(value => { - if (Array.isArray(value)) { - // TODO@joh this isn't proper! - return value.map(quickFix => ({ title: quickFix.label })); + if (!Array.isArray(value)) { + return; } + return value.map(quickFix => typeConverters.Command.to(quickFix.command)); }); } diff --git a/src/vs/workbench/api/common/extHostLanguageFeatures.ts b/src/vs/workbench/api/common/extHostLanguageFeatures.ts index 1cfa90ea104..33a3d29d443 100644 --- a/src/vs/workbench/api/common/extHostLanguageFeatures.ts +++ b/src/vs/workbench/api/common/extHostLanguageFeatures.ts @@ -284,7 +284,6 @@ class QuickFixAdapter implements modes.IQuickFixSupport { private _documents: PluginHostModelService; private _commands: PluginHostCommands; private _provider: vscode.CodeActionProvider; - private _cache: { [key: string]: vscode.Command[] } = Object.create(null); constructor(documents: PluginHostModelService, commands: PluginHostCommands, provider: vscode.CodeActionProvider) { this._documents = documents; @@ -294,10 +293,6 @@ class QuickFixAdapter implements modes.IQuickFixSupport { getQuickFixes(resource: URI, range: IRange, marker?: IMarker[]): TPromise { - // return this._executeCommand(resource, range, markers); - const key = resource.toString(); - delete this._cache[key]; - const doc = this._documents.getDocument(resource); const ran = TypeConverters.toRange(range); const diagnostics = marker.map(marker => { @@ -311,32 +306,18 @@ class QuickFixAdapter implements modes.IQuickFixSupport { if (!Array.isArray(commands)) { return; } - - this._cache[key] = commands; - return commands.map((command, i) => { return { - id: String(i), - label: command.title, - score: 1 + command: TypeConverters.Command.from(command), + score: i }; }); }); } - runQuickFixAction(resource: URI, range: IRange, id: string): any { - - let commands = this._cache[resource.toString()]; - if (!commands) { - return TPromise.wrapError('no command for ' + resource.toString()); - } - - let command = commands[Number(id)]; - if (!command) { - return TPromise.wrapError('no command for ' + resource.toString()); - } - - return this._commands.executeCommand(command.command, ...command.arguments); + runQuickFixAction(resource: URI, range: IRange, quickFix: modes.IQuickFix): any { + let {command} = quickFix; + return this._commands.executeCommand(command.id, ...command.arguments); } } @@ -755,8 +736,8 @@ export class ExtHostLanguageFeatures { return this._withAdapter(handle, QuickFixAdapter, adapter => adapter.getQuickFixes(resource, range, marker)); } - $runQuickFixAction(handle: number, resource: URI, range: IRange, id: string): any { - return this._withAdapter(handle, QuickFixAdapter, adapter => adapter.runQuickFixAction(resource, range, id)); + $runQuickFixAction(handle: number, resource: URI, range: IRange, quickFix: modes.IQuickFix): any { + return this._withAdapter(handle, QuickFixAdapter, adapter => adapter.runQuickFixAction(resource, range, quickFix)); } // --- formatting @@ -960,8 +941,8 @@ export class MainThreadLanguageFeatures { }); return this._proxy.$getQuickFixes(handle, resource, range, markers); }, - runQuickFixAction: (resource: URI, range: IRange, id: string) => { - return this._proxy.$runQuickFixAction(handle, resource, range, id); + runQuickFixAction: (resource: URI, range: IRange, quickFix: modes.IQuickFix) => { + return this._proxy.$runQuickFixAction(handle, resource, range, quickFix); } }); return undefined; diff --git a/src/vs/workbench/test/common/api/extHostLanguageFeatureCommands.test.ts b/src/vs/workbench/test/common/api/extHostLanguageFeatureCommands.test.ts index 4e95502558d..8a1f1fe20ac 100644 --- a/src/vs/workbench/test/common/api/extHostLanguageFeatureCommands.test.ts +++ b/src/vs/workbench/test/common/api/extHostLanguageFeatureCommands.test.ts @@ -270,8 +270,8 @@ suite('ExtHostLanguageFeatureCommands', function() { assert.equal(value.length, 1); let [first] = value; assert.equal(first.title, 'Title'); - // assert.equal(first.command, 'testing'); - // assert.deepEqual(first.arguments, [1, 2, true]); + assert.equal(first.command, 'testing'); + assert.deepEqual(first.arguments, [1, 2, true]); done(); }); }); diff --git a/src/vs/workbench/test/common/api/extHostLanguageFeatures.test.ts b/src/vs/workbench/test/common/api/extHostLanguageFeatures.test.ts index 9a599e3250e..6ee0c6a18cd 100644 --- a/src/vs/workbench/test/common/api/extHostLanguageFeatures.test.ts +++ b/src/vs/workbench/test/common/api/extHostLanguageFeatures.test.ts @@ -615,8 +615,8 @@ suite('ExtHostLanguageFeatures', function() { disposables.push(extHost.registerCodeActionProvider(defaultSelector, { provideCodeActions(): any { return [ - { command: 'test', title: 'Testing1' }, - { command: 'test', title: 'Testing2' } + { command: 'test1', title: 'Testing1' }, + { command: 'test2', title: 'Testing2' } ]; } })); @@ -626,10 +626,10 @@ suite('ExtHostLanguageFeatures', function() { assert.equal(value.length, 2); let [first, second] = value; - assert.equal(first.label, 'Testing1'); - assert.equal(first.id, String(0)); - assert.equal(second.label, 'Testing2'); - assert.equal(second.id, String(1)); + assert.equal(first.command.title, 'Testing1'); + assert.equal(first.command.id, 'test1'); + assert.equal(second.command.title, 'Testing2'); + assert.equal(second.command.id, 'test2'); done(); }); }); @@ -653,7 +653,7 @@ suite('ExtHostLanguageFeatures', function() { assert.equal(value.length, 1); let [entry] = value; - entry.support.runQuickFixAction(model.getAssociatedResource(), model.getFullModelRange(), entry.id).then(value => { + entry.support.runQuickFixAction(model.getAssociatedResource(), model.getFullModelRange(), entry).then(value => { assert.equal(value, undefined); assert.equal(actualArgs.length, 4);