Updates to Go To quick access (#273668)

* Don't wait for async setting update of zero-based offset value

* Fix labels/descriptions

* Update unit-tests

Author: dmitrivMS

src/vs/editor/contrib/quickAccess/browser/gotoLineQuickAccess.ts

@@ -24,8 +24,23 @@ export abstract class AbstractGotoLineQuickAccessProvider extends AbstractEditor
 
 	static PREFIX = ':';
 
-	constructor(private useZeroBasedOffset: { value: boolean } = { value: false }) {
+	private _useZeroBasedOffset: boolean;
+
+	constructor(private useZeroBasedOffsetSetting?: { value: boolean }) {
 		super({ canAcceptInBackground: true });
+		this._useZeroBasedOffset = useZeroBasedOffsetSetting?.value === true;
+	}
+
+	private get useZeroBasedOffset() {
+		return this._useZeroBasedOffset;
+	}
+
+	private set useZeroBasedOffset(value: boolean) {
+		this._useZeroBasedOffset = value;
+		if (this.useZeroBasedOffsetSetting) {
+			// Asynchronously persist the setting change
+			this.useZeroBasedOffsetSetting.value = value;
+		}
 	}
 
 	protected provideWithoutTextEditor(picker: IQuickPick<IGotoLineQuickPickItem, { useSeparators: true }>): IDisposable {
@@ -45,7 +60,7 @@ export abstract class AbstractGotoLineQuickAccessProvider extends AbstractEditor
 		disposables.add(picker.onDidAccept(event => {
 			const [item] = picker.selectedItems;
 			if (item) {
-				if (!this.isValidLineNumber(editor, item.lineNumber)) {
+				if (!item.lineNumber) {
 					return;
 				}
 
@@ -60,34 +75,29 @@ export abstract class AbstractGotoLineQuickAccessProvider extends AbstractEditor
 		// React to picker changes
 		const updatePickerAndEditor = () => {
 			const inputText = picker.value.trim().substring(AbstractGotoLineQuickAccessProvider.PREFIX.length);
-			const inOffsetMode = inputText.startsWith(':');
-			const position = this.parsePosition(editor, inputText);
-			const label = this.getPickLabel(editor, position.lineNumber, position.column, inOffsetMode);
+			const { inOffsetMode, lineNumber, column, label } = this.parsePosition(editor, inputText);
 
 			// Show toggle only when input text starts with '::'.
-			toggle.visible = inOffsetMode;
+			toggle.visible = !!inOffsetMode;
 
 			// Picker
 			picker.items = [{
-				lineNumber: position.lineNumber,
-				column: position.column,
+				lineNumber,
+				column,
 				label,
-				detail: inputText.length ?
-					undefined : // Don't show hint once the user has started typing.
-					localize('gotoLineQuickAccessDescription', "Use :line[:column] or ::offset to go to a position. Negative values are counted from the end.")
 			}];
 
 			// ARIA Label
 			picker.ariaLabel = label;
 
 			// Clear decorations for invalid range
-			if (!this.isValidLineNumber(editor, position.lineNumber)) {
+			if (!lineNumber) {
 				this.clearDecorations(editor);
 				return;
 			}
 
 			// Reveal
-			const range = this.toRange(position.lineNumber, position.column);
+			const range = this.toRange(lineNumber, column);
 			editor.revealRangeInCenter(range, ScrollType.Smooth);
 
 			// Decorate
@@ -96,17 +106,17 @@ export abstract class AbstractGotoLineQuickAccessProvider extends AbstractEditor
 
 		// Add a toggle to switch between 1- and 0-based offsets.
 		const toggle = new Toggle({
-			title: localize('gotoLineToggle', "Use zero-based offset"),
+			title: localize('gotoLineToggle', "Use Zero-Based Offset"),
 			icon: Codicon.indexZero,
-			isChecked: this.useZeroBasedOffset.value,
+			isChecked: this.useZeroBasedOffset,
 			inputActiveOptionBorder: asCssVariable(inputActiveOptionBorder),
 			inputActiveOptionForeground: asCssVariable(inputActiveOptionForeground),
 			inputActiveOptionBackground: asCssVariable(inputActiveOptionBackground)
 		});
 
 		disposables.add(
 			toggle.onChange(() => {
-				this.useZeroBasedOffset.value = !this.useZeroBasedOffset.value;
+				this.useZeroBasedOffset = !this.useZeroBasedOffset;
 				updatePickerAndEditor();
 			}));
 
@@ -139,95 +149,78 @@ export abstract class AbstractGotoLineQuickAccessProvider extends AbstractEditor
 		};
 	}
 
-	protected parsePosition(editor: IEditor, value: string): IPosition {
+	protected parsePosition(editor: IEditor, value: string): Partial<IPosition> & { inOffsetMode?: boolean; label: string } {
 		const model = this.getModel(editor);
+		if (!model) {
+			return {
+				label: localize('gotoLine.noEditor', "Open a text editor first to go to a line.")
+			};
+		}
 
 		// Support ::<offset> notation to navigate to a specific offset in the model.
 		if (value.startsWith(':')) {
 			let offset = parseInt(value.substring(1), 10);
-			if (!isNaN(offset) && model) {
+			const maxOffset = model.getValueLength();
+			if (isNaN(offset)) {
+				// No valid offset specified.
+				return {
+					inOffsetMode: true,
+					label: localize('gotoLine.offsetPrompt', "Type a character number in the file from 1 to {0} to go to.", maxOffset)
+				};
+			} else {
 				const reverse = offset < 0;
-				if (!this.useZeroBasedOffset.value) {
+				if (!this.useZeroBasedOffset) {
 					// Convert 1-based offset to model's 0-based.
 					offset -= Math.sign(offset);
 				}
 				if (reverse) {
 					// Offset from the end of the buffer
-					offset += model.getValueLength();
+					offset += maxOffset;
 				}
-				return model.getPositionAt(offset);
+				const pos = model.getPositionAt(offset);
+				return {
+					...pos,
+					inOffsetMode: true,
+					label: localize('gotoLine.goToPosition', "Press Enter to go to line {0} and column {1}.", pos.lineNumber, pos.column)
+				};
+			}
+		} else {
+			// Support line-col formats of `line,col`, `line:col`, `line#col`
+			const parts = value.split(/,|:|#/);
+
+			const maxLine = model.getLineCount();
+			let lineNumber = parseInt(parts[0]?.trim(), 10);
+			if (parts.length < 1 || isNaN(lineNumber)) {
+				return {
+					label: localize('gotoLine.linePrompt', "Type a line number from 1 to {0} to go to.", maxLine)
+				};
 			}
-		}
-
-		// Support line-col formats of `line,col`, `line:col`, `line#col`
-		let [lineNumber, column] = value.split(/,|:|#/).map(part => parseInt(part, 10)).filter(part => !isNaN(part));
 
-		// Handle negative line numbers and clip to valid range.
-		const maxLine = (model?.getLineCount() ?? 0) + 1;
-		lineNumber = lineNumber >= 0 ? lineNumber : maxLine + lineNumber;
-		lineNumber = Math.min(Math.max(1, lineNumber), maxLine);
+			// Handle negative line numbers and clip to valid range.
+			lineNumber = lineNumber >= 0 ? lineNumber : (maxLine + 1) + lineNumber;
+			lineNumber = Math.min(Math.max(1, lineNumber), maxLine);
 
-		// Handle negative column numbers and clip to valid range.
-		if (column !== undefined && model) {
 			const maxColumn = model.getLineMaxColumn(lineNumber);
-			column = column >= 0 ? column : maxColumn + column;
-			column = Math.min(Math.max(1, column), maxColumn);
-		}
-
-		return { lineNumber, column };
-	}
-
-	private getPickLabel(editor: IEditor, lineNumber: number, column: number | undefined, inOffsetMode: boolean): string {
-
-		// Location valid: indicate this as picker label
-		if (this.isValidLineNumber(editor, lineNumber)) {
-			if (this.isValidColumn(editor, lineNumber, column)) {
-				return localize('gotoLineColumnLabel', "Go to line {0} and character {1}.", lineNumber, column);
+			let column = parseInt(parts[1]?.trim(), 10);
+			if (parts.length < 2 || isNaN(column)) {
+				return {
+					lineNumber,
+					column: 1,
+					label: parts.length < 2 ?
+						localize('gotoLine.lineColumnPrompt', "Press Enter to go to line {0}. Type : to enter column number.", lineNumber) :
+						localize('gotoLine.columnPrompt', "Press Enter to go to line {0} or enter column number from 1 to {1}.", lineNumber, maxColumn)
+				};
 			}
 
-			return localize('gotoLineLabel', "Go to line {0}.", lineNumber);
-		}
-
-		// Location invalid: show generic label
-		const position = editor.getPosition() || { lineNumber: 1, column: 1 };
-
-		// When in offset mode, prompt for an offset.
-		if (inOffsetMode) {
-			return localize('gotoLineOffsetLabel', "Current Line: {0}, Character: {1}. Type a character offset to navigate to.", position.lineNumber, position.column);
-		}
-
-		const lineCount = this.lineCount(editor);
-		if (lineCount > 1) {
-			return localize('gotoLineLabelEmptyWithLimit', "Current Line: {0}, Character: {1}. Type a line number between 1 and {2} to navigate to.", position.lineNumber, position.column, lineCount);
-		}
-
-		return localize('gotoLineLabelEmpty', "Current Line: {0}, Character: {1}. Type a line number to navigate to.", position.lineNumber, position.column);
-	}
-
-	private isValidLineNumber(editor: IEditor, lineNumber: number | undefined): boolean {
-		if (!lineNumber || typeof lineNumber !== 'number') {
-			return false;
-		}
-
-		return lineNumber > 0 && lineNumber <= this.lineCount(editor);
-	}
-
-	private isValidColumn(editor: IEditor, lineNumber: number, column: number | undefined): boolean {
-		if (!column || typeof column !== 'number') {
-			return false;
-		}
+			// Handle negative column numbers and clip to valid range.
+			column = column >= 0 ? column : maxColumn + column;
+			column = Math.min(Math.max(1, column), maxColumn);
 
-		const model = this.getModel(editor);
-		if (!model) {
-			return false;
+			return {
+				lineNumber,
+				column,
+				label: localize('gotoLine.goToPosition', "Press Enter to go to line {0} and column {1}.", lineNumber, column)
+			};
 		}
-
-		const positionCandidate = { lineNumber, column };
-
-		return model.validatePosition(positionCandidate).equals(positionCandidate);
-	}
-
-	private lineCount(editor: IEditor): number {
-		return this.getModel(editor)?.getLineCount() ?? 0;
 	}
 }

src/vs/editor/contrib/quickAccess/test/browser/gotoLineQuickAccess.test.ts

@@ -35,7 +35,7 @@ suite('AbstractGotoLineQuickAccessProvider', () => {
 		], {}, (editor, _) => {
 			const { lineNumber, column } = provider.parsePositionTest(editor, input);
 			assert.strictEqual(lineNumber, expectedLine);
-			assert.strictEqual(column, expectedColumn);
+			assert.strictEqual(column, expectedColumn ?? 1);
 		});
 	}
 
@@ -48,9 +48,9 @@ suite('AbstractGotoLineQuickAccessProvider', () => {
 		runTest('1', 1);
 		runTest('2', 2);
 		runTest('5', 5);
-		runTest('6', 6);
-		runTest('7', 6);
-		runTest('100', 6);
+		runTest('6', 5);
+		runTest('7', 5);
+		runTest('100', 5);
 
 		// :line,column
 		runTest('2:-100', 2, 1);

src/vs/workbench/contrib/codeEditor/browser/quickaccess/gotoLineQuickAccess.ts

@@ -104,6 +104,6 @@ registerAction2(GotoLineAction);
 Registry.as<IQuickAccessRegistry>(QuickaccesExtensions.Quickaccess).registerQuickAccessProvider({
 	ctor: GotoLineQuickAccessProvider,
 	prefix: AbstractGotoLineQuickAccessProvider.PREFIX,
-	placeholder: localize('gotoLineQuickAccessPlaceholder', "Type the line number and optional column to go to (e.g. 42:5 for line 42 and column 5)."),
+	placeholder: localize('gotoLineQuickAccessPlaceholder', "Type the line number and optional column to go to (e.g. :42:5 for line 42, column 5). Type :: to go to a character offset (e.g. ::1024 for character 1024 from the start of the file). Use negative values to navigate backwards."),
 	helpEntries: [{ description: localize('gotoLineQuickAccess', "Go to Line/Column"), commandId: GotoLineAction.ID }]
 });