fix #29933

src/vs/editor/contrib/quickFix/browser/quickFixModel.ts

@@ -4,12 +4,11 @@
  *--------------------------------------------------------------------------------------------*/
 'use strict';
 
-import * as arrays from 'vs/base/common/arrays';
 import Event, { Emitter, debounceEvent } from 'vs/base/common/event';
 import { IDisposable, dispose } from 'vs/base/common/lifecycle';
 import URI from 'vs/base/common/uri';
 import { TPromise } from 'vs/base/common/winjs.base';
-import { IMarker, IMarkerService } from 'vs/platform/markers/common/markers';
+import { IMarkerService } from 'vs/platform/markers/common/markers';
 import { Range } from 'vs/editor/common/core/range';
 import { ICommonCodeEditor } from 'vs/editor/common/editorCommon';
 import { CodeActionProviderRegistry, Command } from 'vs/editor/common/modes';
@@ -39,10 +38,21 @@ export class QuickFixOracle {
 	}
 
 	trigger(type: 'manual' | 'auto'): void {
-		let range = this._rangeAtPosition();
+
+		// get selection from marker or current word
+		// unless the selection is non-empty and manually
+		// requesting code actions
+		let selection = this._editor.getSelection();
+		let range = this._getActiveMarkerOrWordRange();
+		if (type === 'manual' && !selection.isEmpty()) {
+			range = selection;
+		}
+
+		// empty selection somewhere in nowhere
 		if (!range) {
-			range = this._editor.getSelection();
+			range = selection;
 		}
+
 		this._signalChange({
 			type,
 			range,
@@ -64,7 +74,7 @@ export class QuickFixOracle {
 	}
 
 	private _onCursorChange(): void {
-		const range = this._rangeAtPosition();
+		const range = this._getActiveMarkerOrWordRange();
 		if (!Range.equalsRange(this._currentRange, range)) {
 			this._currentRange = range;
 			this._signalChange({
@@ -76,48 +86,28 @@ export class QuickFixOracle {
 		}
 	}
 
-	private _rangeAtPosition(): Range {
+	private _getActiveMarkerOrWordRange(): Range {
 
-		// (1) check with non empty selection
 		const selection = this._editor.getSelection();
-		if (!selection.isEmpty()) {
-			return selection;
-		}
+		const model = this._editor.getModel();
 
-		// (2) check with diagnostics markers
-		const marker = this._markerAtPosition();
-		if (marker) {
-			return Range.lift(marker);
+		// (1) return marker that contains a (empty/non-empty) selection
+		for (const marker of this._markerService.read({ resource: model.uri })) {
+			const range = Range.lift(marker);
+			if (range.containsRange(selection)) {
+				return range;
+			}
 		}
 
-		// (3) check with word
-		return this._wordAtPosition();
-	}
-
-	private _markerAtPosition(): IMarker {
-
-		const position = this._editor.getPosition();
-		const { uri } = this._editor.getModel();
-		const markers = this._markerService.read({ resource: uri }).sort(Range.compareRangesUsingStarts);
-
-		let idx = arrays.findFirst(markers, marker => marker.endLineNumber >= position.lineNumber);
-		while (idx < markers.length && markers[idx].endLineNumber >= position.lineNumber) {
-			const marker = markers[idx];
-			if (Range.containsPosition(marker, position)) {
-				return marker;
+		// (2) return range of current word
+		if (selection.isEmpty()) {
+			const pos = selection.getStartPosition();
+			const info = model.getWordAtPosition(pos);
+			if (info) {
+				return new Range(pos.lineNumber, info.startColumn, pos.lineNumber, info.endColumn);
 			}
-			idx++;
 		}
-		return undefined;
-	}
 
-	private _wordAtPosition(): Range {
-		const pos = this._editor.getPosition();
-		const model = this._editor.getModel();
-		const info = model.getWordAtPosition(pos);
-		if (info) {
-			return new Range(pos.lineNumber, info.startColumn, pos.lineNumber, info.endColumn);
-		}
 		return undefined;
 	}
 }

src/vs/editor/contrib/quickFix/test/browser/quickFixModel.test.ts

@@ -182,11 +182,11 @@ suite('QuickFix', () => {
 
 		return TPromise.join<any>([TPromise.timeout(20)].concat(fixes)).then(_ => {
 
-			// assert selection
-			assert.deepEqual(range, { startLineNumber: 1, startColumn: 1, endLineNumber: 1, endColumn: 13 });
+			// 'auto' triggered, non-empty selection
+			assert.equal(range, undefined);
 
-			range = undefined;
-			editor.setSelection({ startLineNumber: 1, startColumn: 2, endLineNumber: 1, endColumn: 2 });
+			// 'auto' triggered, non-empty selection BUT within a marker
+			editor.setSelection({ startLineNumber: 1, startColumn: 2, endLineNumber: 1, endColumn: 4 });
 
 			return TPromise.join([TPromise.timeout(20)].concat(fixes)).then(_ => {
 				reg.dispose();
@@ -198,4 +198,55 @@ suite('QuickFix', () => {
 		});
 	});
 
+	test('Lightbulb is in the wrong place, #29933', async function () {
+		let reg = CodeActionProviderRegistry.register(languageIdentifier.language, {
+			provideCodeActions(doc, _range) {
+				return [];
+			}
+		});
+
+		editor.getModel().setValue('// @ts-check\n2\ncon\n');
+
+		markerService.changeOne('fake', uri, [{
+			startLineNumber: 3, startColumn: 1, endLineNumber: 3, endColumn: 4,
+			message: 'error',
+			severity: 1,
+			code: '',
+			source: ''
+		}]);
+
+		// case 1 - drag selection over multiple lines -> no automatic lightbulb
+		await new TPromise(resolve => {
+
+			let oracle = new QuickFixOracle(editor, markerService, e => {
+				assert.equal(e.type, 'auto');
+				assert.equal(e.range, undefined);
+
+				oracle.dispose();
+				resolve(null);
+			}, 5);
+
+			editor.setSelection({ startLineNumber: 1, startColumn: 1, endLineNumber: 4, endColumn: 1 });
+		});
+
+		// case 2 - selection over multiple lines & manual trigger -> lightbulb
+		await new TPromise(resolve => {
+
+			editor.setSelection({ startLineNumber: 1, startColumn: 1, endLineNumber: 4, endColumn: 1 });
+
+			let oracle = new QuickFixOracle(editor, markerService, e => {
+				assert.equal(e.type, 'manual');
+				assert.ok(e.range.equalsRange({ startLineNumber: 1, startColumn: 1, endLineNumber: 4, endColumn: 1 }));
+
+				oracle.dispose();
+				resolve(null);
+			}, 5);
+
+			oracle.trigger('manual');
+		});
+
+
+		reg.dispose();
+	});
+
 });