diff --git a/src/vs/editor/contrib/quickFix/browser/lightBulbWidget.css b/src/vs/editor/contrib/quickFix/browser/lightBulbWidget.css index e7f140a1af1..132d0f70451 100644 --- a/src/vs/editor/contrib/quickFix/browser/lightBulbWidget.css +++ b/src/vs/editor/contrib/quickFix/browser/lightBulbWidget.css @@ -8,7 +8,8 @@ align-items: center; justify-content: center; height: 16px; - width: 16px; + width: 20px; + padding-left: 2px; } .monaco-editor .lightbulb-glyph.hidden { @@ -18,6 +19,7 @@ .monaco-editor .lightbulb-glyph:hover { cursor: pointer; + transform: scale(1.3, 1.3); } .monaco-editor.vs .lightbulb-glyph { diff --git a/src/vs/editor/contrib/quickFix/browser/lightBulbWidget.ts b/src/vs/editor/contrib/quickFix/browser/lightBulbWidget.ts index 3db81fda26c..472d3098fdc 100644 --- a/src/vs/editor/contrib/quickFix/browser/lightBulbWidget.ts +++ b/src/vs/editor/contrib/quickFix/browser/lightBulbWidget.ts @@ -6,72 +6,86 @@ import 'vs/css!./lightBulbWidget'; import { CancellationTokenSource } from 'vs/base/common/cancellation'; -import { IDisposable } from 'vs/base/common/lifecycle'; +import { dispose, IDisposable } from 'vs/base/common/lifecycle'; import Event, { Emitter } from 'vs/base/common/event'; import * as dom from 'vs/base/browser/dom'; -import { TrackedRangeStickiness } from 'vs/editor/common/editorCommon'; -import { ICodeEditor, MouseTargetType } from 'vs/editor/browser/editorBrowser'; +import { ICodeEditor, IContentWidget, IContentWidgetPosition, ContentWidgetPositionPreference } from 'vs/editor/browser/editorBrowser'; import { QuickFixComputeEvent } from './quickFixModel'; -import { IMarkdownString } from 'vs/base/common/htmlContent'; -export class LightBulbWidget implements IDisposable { +export class LightBulbWidget implements IDisposable, IContentWidget { - private readonly _options = { - stickiness: TrackedRangeStickiness.NeverGrowsWhenTypingAtEdges, - glyphMarginClassName: 'lightbulb-glyph', - glyphMarginHoverMessage: undefined - }; + private static readonly _posPref = [ContentWidgetPositionPreference.EXACT]; + private readonly _domNode: HTMLDivElement; private readonly _editor: ICodeEditor; + private readonly _disposables: IDisposable[] = []; private readonly _onClick = new Emitter<{ x: number, y: number }>(); - private readonly _mouseDownSubscription: IDisposable; - private _decorationIds: string[] = []; - private _currentLine: number; + readonly onClick: Event<{ x: number, y: number }> = this._onClick.event; + + private _position: IContentWidgetPosition; private _model: QuickFixComputeEvent; private _futureFixes = new CancellationTokenSource(); constructor(editor: ICodeEditor) { this._editor = editor; - this._mouseDownSubscription = this._editor.onMouseDown(e => { + this._editor.addContentWidget(this); - // not on glyh margin or not on 💡 - if (e.target.type !== MouseTargetType.GUTTER_GLYPH_MARGIN - || this._currentLine === undefined - || this._currentLine !== e.target.position.lineNumber - ) { - return; - } + this._domNode = document.createElement('div'); + this._domNode.className = 'lightbulb-glyph'; + this._disposables.push(dom.addStandardDisposableListener(this._domNode, 'click', e => { // a bit of extra work to make sure the menu // doesn't cover the line-text - const { top, height } = dom.getDomNodePagePosition(e.target.element); + const { top, height } = dom.getDomNodePagePosition(this._domNode); const { lineHeight } = this._editor.getConfiguration(); + + let pad = Math.floor(lineHeight / 3); + if (this._position.position.lineNumber < this._model.position.lineNumber) { + pad += lineHeight; + } + this._onClick.fire({ - x: e.event.posx, - y: top + height + Math.floor(lineHeight / 3) + x: e.posx, + y: top + height + pad }); - }); + })); + + this._disposables.push(this._editor.onDidChangeCursorSelection(e => { + // hide lightbulb when selection starts to + // enclose it + if (this._position && e.selection.containsPosition(this._position.position)) { + this.hide(); + } + })); } dispose(): void { - this._mouseDownSubscription.dispose(); - this.hide(); + dispose(this._disposables); + this._editor.removeContentWidget(this); } - get onClick(): Event<{ x: number, y: number }> { - return this._onClick.event; + getId(): string { + return 'LightBulbWidget'; } - set model(e: QuickFixComputeEvent) { - this._model = e; + getDomNode(): HTMLElement { + return this._domNode; + } + + getPosition(): IContentWidgetPosition { + return this._position; + } + + set model(value: QuickFixComputeEvent) { this.hide(); + this._model = value; this._futureFixes = new CancellationTokenSource(); const { token } = this._futureFixes; - e.fixes.done(fixes => { + this._model.fixes.done(fixes => { if (!token.isCancellationRequested && fixes && fixes.length > 0) { - this.show(e); + this.show(this._model); } else { this.hide(); } @@ -85,26 +99,42 @@ export class LightBulbWidget implements IDisposable { } set title(value: string) { - // TODO(joh,alex) this isn't working well because the hover hover - // message sticks around after clicking the light bulb - // this._options.glyphMarginHoverMessage = value; + this._domNode.title = value; } - get title() { - return this._options.glyphMarginHoverMessage && this._options.glyphMarginHoverMessage.value; + get title(): string { + return this._domNode.title; } show(e: QuickFixComputeEvent): void { - this._currentLine = e.range.startLineNumber; - this._decorationIds = this._editor.deltaDecorations(this._decorationIds, [{ - options: this._options, - range: { ...e.range, endLineNumber: e.range.startLineNumber } - }]); + const { fontInfo } = this._editor.getConfiguration(); + const { lineNumber } = e.position; + const model = this._editor.getModel(); + const indent = model.getIndentLevel(lineNumber); + const lineHasSpace = fontInfo.spaceWidth * indent > 22; + + let effectiveLineNumber = lineNumber; + if (!lineHasSpace) { + if (lineNumber > 1) { + effectiveLineNumber -= 1; + } else { + effectiveLineNumber += 1; + } + } + + this._position = { + position: { lineNumber: effectiveLineNumber, column: 1 }, + preference: LightBulbWidget._posPref + }; + + this._editor.layoutContentWidget(this); + this._model = e; } hide(): void { - this._decorationIds = this._editor.deltaDecorations(this._decorationIds, []); + this._position = null; + this._model = null; this._futureFixes.cancel(); - this._currentLine = undefined; + this._editor.layoutContentWidget(this); } } diff --git a/src/vs/editor/contrib/quickFix/browser/quickFixModel.ts b/src/vs/editor/contrib/quickFix/browser/quickFixModel.ts index 324cbf3e152..5b173774b49 100644 --- a/src/vs/editor/contrib/quickFix/browser/quickFixModel.ts +++ b/src/vs/editor/contrib/quickFix/browser/quickFixModel.ts @@ -16,11 +16,9 @@ import { CodeActionProviderRegistry, Command } from 'vs/editor/common/modes'; import { getCodeActions } from './quickFix'; import { Position } from 'vs/editor/common/core/position'; - export class QuickFixOracle { private _disposables: IDisposable[] = []; - private _currentRange: Range; constructor( private _editor: ICommonCodeEditor, @@ -39,35 +37,25 @@ export class QuickFixOracle { } trigger(type: 'manual' | 'auto'): void { - - // get selection from marker or current word - // unless the selection is non-empty and manually - // requesting code actions - const range = (type === 'manual' && this._getRangeOfNonEmptySelection()) - || this._getRangeOfMarker() - || this._getRangeOfWord() - || this._editor.getSelection(); - - this._createEventAndSignalChange(type, range); + let rangeOrSelection = this._getRangeOfMarker() || this._getRangeOfSelectionUnlessWhitespaceEnclosed(); + if (type === 'manual') { + rangeOrSelection = this._editor.getSelection(); + } + this._createEventAndSignalChange(type, rangeOrSelection); } private _onMarkerChanges(resources: URI[]): void { const { uri } = this._editor.getModel(); for (const resource of resources) { if (resource.toString() === uri.toString()) { - this._currentRange = undefined; - this._onCursorChange(); + this.trigger('auto'); return; } } } private _onCursorChange(): void { - const rangeOrSelection = this._getRangeOfMarker() || this._getRangeOfNonEmptySelection(); - if (!Range.equalsRange(this._currentRange, rangeOrSelection)) { - this._currentRange = rangeOrSelection; - this._createEventAndSignalChange('auto', rangeOrSelection); - } + this.trigger('auto'); } private _getRangeOfMarker(): Range { @@ -81,15 +69,33 @@ export class QuickFixOracle { return undefined; } - private _getRangeOfWord(): Range { - const pos = this._editor.getPosition(); - const info = this._editor.getModel().getWordAtPosition(pos); - return info ? new Range(pos.lineNumber, info.startColumn, pos.lineNumber, info.endColumn) : undefined; - } - - private _getRangeOfNonEmptySelection(): Selection { + private _getRangeOfSelectionUnlessWhitespaceEnclosed(): Selection { + const model = this._editor.getModel(); const selection = this._editor.getSelection(); - return !selection.isEmpty() ? selection : undefined; + if (selection.isEmpty()) { + const { lineNumber, column } = selection.getPosition(); + const line = model.getLineContent(lineNumber); + if (line.length === 0) { + // empty line + return undefined; + } else if (column === 1) { + // look only right + if (/\s/.test(line[0])) { + return undefined; + } + } else if (column === model.getLineMaxColumn(lineNumber)) { + // look only left + if (/\s/.test(line[line.length - 1])) { + return undefined; + } + } else { + // look left and right + if (/\s/.test(line[column - 2]) && /\s/.test(line[column - 1])) { + return undefined; + } + } + } + return selection; } private _createEventAndSignalChange(type: 'auto' | 'manual', rangeOrSelection: Range | Selection): void { diff --git a/src/vs/editor/contrib/quickFix/test/browser/quickFixModel.test.ts b/src/vs/editor/contrib/quickFix/test/browser/quickFixModel.test.ts index 41473c56060..adb843d7a0e 100644 --- a/src/vs/editor/contrib/quickFix/test/browser/quickFixModel.test.ts +++ b/src/vs/editor/contrib/quickFix/test/browser/quickFixModel.test.ts @@ -14,18 +14,8 @@ import { MarkerService } from 'vs/platform/markers/common/markerService'; import { QuickFixOracle } from 'vs/editor/contrib/quickFix/browser/quickFixModel'; import { CodeActionProviderRegistry, LanguageIdentifier } from 'vs/editor/common/modes'; import { IDisposable } from 'vs/base/common/lifecycle'; -import Event from 'vs/base/common/event'; import { Range } from 'vs/editor/common/core/range'; -function promiseOnce(event: Event): TPromise { - return new TPromise(resolve => { - let reg = event(e => { - reg.dispose(); - resolve(e); - }); - }); -} - suite('QuickFix', () => { const languageIdentifier = new LanguageIdentifier('foo-lang', 3); @@ -78,7 +68,7 @@ suite('QuickFix', () => { }); - test('Orcale -> position changed', done => { + test('Orcale -> position changed', () => { markerService.changeOne('fake', uri, [{ startLineNumber: 1, startColumn: 1, endLineNumber: 1, endColumn: 6, @@ -90,72 +80,23 @@ suite('QuickFix', () => { editor.setPosition({ lineNumber: 2, column: 1 }); - const oracle = new QuickFixOracle(editor, markerService, e => { - assert.equal(e.type, 'auto'); - assert.ok(e.fixes); + return new TPromise((resolve, reject) => { - e.fixes.then(fixes => { - oracle.dispose(); - assert.equal(fixes.length, 1); - done(); - }, done); + const oracle = new QuickFixOracle(editor, markerService, e => { + assert.equal(e.type, 'auto'); + assert.ok(e.fixes); + e.fixes.then(fixes => { + oracle.dispose(); + assert.equal(fixes.length, 1); + resolve(undefined); + }, reject); + }); + // start here + editor.setPosition({ lineNumber: 1, column: 1 }); }); - - // start here - editor.setPosition({ lineNumber: 1, column: 1 }); - }); - test('Oracle -> ask once per marker/word', async () => { - - const start = promiseOnce(markerService.onMarkerChanged); - - markerService.changeOne('fake', uri, [{ - startLineNumber: 1, startColumn: 1, endLineNumber: 1, endColumn: 6, - message: 'error', - severity: 1, - code: '', - source: '' - }]); - - await start; - - let counter = 0; - let reg = CodeActionProviderRegistry.register(languageIdentifier.language, { - provideCodeActions() { - counter += 1; - return []; - } - }); - - let fixes: TPromise[] = []; - let oracle = new QuickFixOracle(editor, markerService, e => { - fixes.push(e.fixes); - }, 10); - - editor.setPosition({ lineNumber: 1, column: 3 }); // marker - editor.setPosition({ lineNumber: 1, column: 6 }); // (same) marker - - await TPromise.join([TPromise.timeout(20)].concat(fixes)); - assert.equal(counter, 1); - - // no auto trigger when empty selection - editor.setPosition({ lineNumber: 1, column: 8 }); // whitespace - editor.setPosition({ lineNumber: 2, column: 2 }); // word - editor.setPosition({ lineNumber: 2, column: 6 }); // (same) word - await TPromise.join([TPromise.timeout(20)].concat(fixes)); - assert.equal(counter, 1); - - // auto trigger on non-empty selection - editor.setSelection({ startLineNumber: 2, startColumn: 2, endLineNumber: 2, endColumn: 6 }); - await TPromise.join([TPromise.timeout(20)].concat(fixes)); - assert.equal(counter, 2); - - reg.dispose(); - oracle.dispose(); - }); - - test('Oracle -> selection wins over marker', () => { + test('Oracle -> marker wins over selection', () => { let range: Range; let reg = CodeActionProviderRegistry.register(languageIdentifier.language, { diff --git a/src/vs/workbench/api/node/extHostLanguageFeatures.ts b/src/vs/workbench/api/node/extHostLanguageFeatures.ts index 91463ee325f..63184e86417 100644 --- a/src/vs/workbench/api/node/extHostLanguageFeatures.ts +++ b/src/vs/workbench/api/node/extHostLanguageFeatures.ts @@ -273,7 +273,7 @@ class QuickFixAdapter { private _diagnostics: ExtHostDiagnostics; private _provider: vscode.CodeActionProvider; - constructor(documents: ExtHostDocuments, commands: CommandsConverter, diagnostics: ExtHostDiagnostics, heapService: ExtHostHeapService, provider: vscode.CodeActionProvider) { + constructor(documents: ExtHostDocuments, commands: CommandsConverter, diagnostics: ExtHostDiagnostics, provider: vscode.CodeActionProvider) { this._documents = documents; this._commands = commands; this._diagnostics = diagnostics; @@ -283,13 +283,13 @@ class QuickFixAdapter { provideCodeActions(resource: URI, range: IRange): TPromise { const doc = this._documents.getDocumentData(resource).document; - const ran = TypeConverters.toRange(range); + const ran = TypeConverters.toRange(range); const allDiagnostics: vscode.Diagnostic[] = []; this._diagnostics.forEach(collection => { if (collection.has(resource)) { for (let diagnostic of collection.get(resource)) { - if (diagnostic.range.intersection(ran)) { + if (ran.contains(diagnostic.range)) { allDiagnostics.push(diagnostic); } } @@ -916,7 +916,7 @@ export class ExtHostLanguageFeatures implements ExtHostLanguageFeaturesShape { registerCodeActionProvider(selector: vscode.DocumentSelector, provider: vscode.CodeActionProvider): vscode.Disposable { const handle = this._nextHandle(); - this._adapter.set(handle, new QuickFixAdapter(this._documents, this._commands.converter, this._diagnostics, this._heapService, provider)); + this._adapter.set(handle, new QuickFixAdapter(this._documents, this._commands.converter, this._diagnostics, provider)); this._proxy.$registerQuickFixSupport(handle, selector); return this._createDisposable(handle); }