Avoid walking whitespace twice when going left & small stylistic changes

src/vs/editor/browser/controller/mouseTarget.ts

@@ -1050,13 +1050,13 @@ export class MouseTargetFactory {
 		} else if ((<any>document.body).createTextRange) {
 			result = this._doHitTestWithMoveToPoint(ctx, request.pos.toClientCoordinates());
 		} else {
-			return {
+			result = {
 				position: null,
 				hitTarget: null
 			};
 		}
 		// Snap to the nearest soft tab boundary if atomic soft tabs are enabled.
-		if (result.position !== null && ctx.atomicSoftTabs) {
+		if (result.position && ctx.atomicSoftTabs) {
 			result.position = this._snapToSoftTabBoundary(result.position, ctx.model);
 		}
 		return result;

src/vs/editor/common/controller/cursorAtomicMoveOperations.ts

@@ -6,21 +6,31 @@
 import { CharCode } from 'vs/base/common/charCode';
 import { CursorColumns } from 'vs/editor/common/controller/cursorCommon';
 
-export enum Direction {
+export const enum Direction {
 	Left,
 	Right,
 	Nearest,
 }
 
 export class AtomicTabMoveOperations {
-	// Get the visible column at the position. If we get to a non-whitespace character first
-	// or past the end of string then return -1. Note `position` and the return
-	// value are 0-based.
-	public static whitespaceVisibleColumn(lineContent: string, position: number, tabSize: number) {
+	/**
+	 * Get the visible column at the position. If we get to a non-whitespace character first
+	 * or past the end of string then return -1.
+	 *
+	 * **Note** `position` and the return value are 0-based.
+	 */
+	public static whitespaceVisibleColumn(lineContent: string, position: number, tabSize: number): [number, number, number] {
+		const lineLength = lineContent.length;
 		let visibleColumn = 0;
-		for (let i = 0; i < lineContent.length; ++i) {
+		let prevTabStopPosition = -1;
+		let prevTabStopVisibleColumn = -1;
+		for (let i = 0; i < lineLength; i++) {
 			if (i === position) {
-				return visibleColumn;
+				return [prevTabStopPosition, prevTabStopVisibleColumn, visibleColumn];
+			}
+			if (visibleColumn % tabSize === 0) {
+				prevTabStopPosition = i;
+				prevTabStopVisibleColumn = visibleColumn;
 			}
 			const chCode = lineContent.charCodeAt(i);
 			switch (chCode) {
@@ -32,26 +42,30 @@ export class AtomicTabMoveOperations {
 					visibleColumn = CursorColumns.nextRenderTabStop(visibleColumn, tabSize);
 					break;
 				default:
-					return -1;
+					return [-1, -1, -1];
 			}
 		}
-		if (position === lineContent.length) {
-			return visibleColumn;
+		if (position === lineLength) {
+			return [prevTabStopPosition, prevTabStopVisibleColumn, visibleColumn];
 		}
-		return -1;
+		return [-1, -1, -1];
 	}
 
-	// Return the position that should result from a move left, right or to the
-	// nearest tab, if atomic tabs are enabled. Left and right are used for the
-	// arrow key movements, nearest is used for mouse selection. It returns
-	// -1 if atomic tabs are not relevant and you should fall back to normal
-	// behaviour.
-	//
-	// Note, `position` and the return value are 0-based.
+	/**
+	 * Return the position that should result from a move left, right or to the
+	 * nearest tab, if atomic tabs are enabled. Left and right are used for the
+	 * arrow key movements, nearest is used for mouse selection. It returns
+	 * -1 if atomic tabs are not relevant and you should fall back to normal
+	 * behaviour.
+	 *
+	 * **Note**: `position` and the return value are 0-based.
+	 */
 	public static atomicPosition(lineContent: string, position: number, tabSize: number, direction: Direction): number {
+		const lineLength = lineContent.length;
+
 		// Get the 0-based visible column corresponding to the position, or return
 		// -1 if it is not in the initial whitespace.
-		let visibleColumn = AtomicTabMoveOperations.whitespaceVisibleColumn(lineContent, position, tabSize);
+		const [prevTabStopPosition, prevTabStopVisibleColumn, visibleColumn] = AtomicTabMoveOperations.whitespaceVisibleColumn(lineContent, position, tabSize);
 
 		if (visibleColumn === -1) {
 			return -1;
@@ -78,77 +92,68 @@ export class AtomicTabMoveOperations {
 				break;
 		}
 
-		// The code below won't work if visibleColumn is zero and left is true because
-		// it takes the mod of a negative number, which behaves oddly. In that case
-		// we already know what to return.
-		if (left && visibleColumn === 0) {
+		// If going left, we can just use the info about the last tab stop position and
+		// last tab stop visible column that we computed in the first walk over the whitespace.
+		if (left) {
+			if (prevTabStopPosition === -1) {
+				return -1;
+			}
+			// If the direction is left, we need to keep scanning right to ensure
+			// that targetVisibleColumn + tabSize is before non-whitespace.
+			// This is so that when we press left at the end of a partial
+			// indentation it only goes one character. For example '      foo' with
+			// tabSize 4, should jump from position 6 to position 5, not 4.
+			let currentVisibleColumn = prevTabStopVisibleColumn;
+			for (let i = prevTabStopPosition; i < lineLength; ++i) {
+				if (currentVisibleColumn === prevTabStopVisibleColumn + tabSize) {
+					// It is a full indentation.
+					return prevTabStopPosition;
+				}
+
+				const chCode = lineContent.charCodeAt(i);
+				switch (chCode) {
+					case CharCode.Space:
+						currentVisibleColumn += 1;
+						break;
+					case CharCode.Tab:
+						currentVisibleColumn = CursorColumns.nextRenderTabStop(currentVisibleColumn, tabSize);
+						break;
+					default:
+						return -1;
+				}
+			}
+			if (currentVisibleColumn === prevTabStopVisibleColumn + tabSize) {
+				return prevTabStopPosition;
+			}
+			// It must have been a partial indentation.
 			return -1;
 		}
 
-		const tmp = visibleColumn + (left ? -1 : tabSize);
-		const targetVisibleColumn = tmp - tmp % tabSize;
-
-		// Find the target visible column. If going right we can just continue from
-		// where whitespaceVisibleColumn got to. If going left it's easiest to start
-		// from the beginning because the width of tab characters depend on the
-		// characters to their left. E.g. ' \t' is one tabSize, but so is '\t'.
-		if (left) {
-			visibleColumn = 0;
-		}
-		for (let i = (left ? 0 : position); i < lineContent.length; ++i) {
-			if (visibleColumn === targetVisibleColumn) {
-				// This is the position we want to get to, but we have one more case
-				// to handle if going left.
-				if (left) {
-					// If the direction is left, we need to keep scanning right to ensure
-					// that targetVisibleColumn + tabSize is before non-whitespace.
-					// This is so that when we press left at the end of a partial
-					// indentation it only goes one character. For example '      foo' with
-					// tabSize 4, should jump from position 6 to position 5, not 4.
-					for (let k = i; k < lineContent.length; ++k) {
-						if (visibleColumn === targetVisibleColumn + tabSize) {
-							// It is a full indentation.
-							return i;
-						}
+		// We are going right.
+		const targetVisibleColumn = CursorColumns.nextRenderTabStop(visibleColumn, tabSize);
 
-						const chCode = lineContent.charCodeAt(k);
-						switch (chCode) {
-							case CharCode.Space:
-								visibleColumn += 1;
-								break;
-							case CharCode.Tab:
-								visibleColumn = CursorColumns.nextRenderTabStop(visibleColumn, tabSize);
-								break;
-							default:
-								return -1;
-						}
-					}
-					if (visibleColumn === targetVisibleColumn + tabSize) {
-						return i;
-					}
-					// It must have been a partial indentation.
-					return -1;
-				} else {
-					// If going right then we must have been in a complete indentation.
-					return i;
-				}
+		// We can just continue from where whitespaceVisibleColumn got to.
+		let currentVisibleColumn = visibleColumn;
+		for (let i = position; i < lineLength; i++) {
+			if (currentVisibleColumn === targetVisibleColumn) {
+				return i;
 			}
 
 			const chCode = lineContent.charCodeAt(i);
 			switch (chCode) {
 				case CharCode.Space:
-					visibleColumn += 1;
+					currentVisibleColumn += 1;
 					break;
 				case CharCode.Tab:
-					visibleColumn = CursorColumns.nextRenderTabStop(visibleColumn, tabSize);
+					currentVisibleColumn = CursorColumns.nextRenderTabStop(currentVisibleColumn, tabSize);
 					break;
 				default:
 					return -1;
 			}
 		}
 		// This condition handles when the target column is at the end of the line.
-		if (visibleColumn === targetVisibleColumn) {
-			return lineContent.length;
+		if (currentVisibleColumn === targetVisibleColumn) {
+			return lineLength;
 		}
 		return -1;
 	}

src/vs/editor/common/controller/cursorMoveOperations.ts

@@ -30,33 +30,25 @@ export class MoveOperations {
 		if (column > model.getLineMinColumn(lineNumber)) {
 			column = column - strings.prevCharLength(model.getLineContent(lineNumber), column - 1);
 		} else if (lineNumber > 1) {
-			lineNumber -= 1;
+			lineNumber = lineNumber - 1;
 			column = model.getLineMaxColumn(lineNumber);
 		}
 		return new Position(lineNumber, column);
 	}
 
-	public static leftPositionatomicSoftTabs(model: ICursorSimpleModel, lineNumber: number, column: number, tabSize: number): Position {
+	public static leftPositionAtomicSoftTabs(model: ICursorSimpleModel, lineNumber: number, column: number, tabSize: number): Position {
 		const minColumn = model.getLineMinColumn(lineNumber);
-		if (column > minColumn) {
-			const lineContent = model.getLineContent(lineNumber);
-
-			const newPosition = AtomicTabMoveOperations.atomicPosition(lineContent, column - minColumn, tabSize, Direction.Left);
-			if (newPosition !== -1) {
-				column = minColumn + newPosition;
-			} else {
-				column -= strings.prevCharLength(model.getLineContent(lineNumber), column - 1);
-			}
-		} else if (lineNumber > 1) {
-			lineNumber -= 1;
-			column = model.getLineMaxColumn(lineNumber);
+		const lineContent = model.getLineContent(lineNumber);
+		const newPosition = AtomicTabMoveOperations.atomicPosition(lineContent, column - minColumn, tabSize, Direction.Left);
+		if (newPosition === -1) {
+			return this.leftPosition(model, lineNumber, column);
 		}
-		return new Position(lineNumber, column);
+		return new Position(lineNumber, minColumn + newPosition);
 	}
 
 	public static left(config: CursorConfiguration, model: ICursorSimpleModel, lineNumber: number, column: number): CursorPosition {
 		const pos = config.atomicSoftTabs
-			? MoveOperations.leftPositionatomicSoftTabs(model, lineNumber, column, config.tabSize)
+			? MoveOperations.leftPositionAtomicSoftTabs(model, lineNumber, column, config.tabSize)
 			: MoveOperations.leftPosition(model, lineNumber, column);
 		return new CursorPosition(pos.lineNumber, pos.column, 0);
 	}
@@ -82,33 +74,25 @@ export class MoveOperations {
 		if (column < model.getLineMaxColumn(lineNumber)) {
 			column = column + strings.nextCharLength(model.getLineContent(lineNumber), column - 1);
 		} else if (lineNumber < model.getLineCount()) {
-			lineNumber += 1;
+			lineNumber = lineNumber + 1;
 			column = model.getLineMinColumn(lineNumber);
 		}
 		return new Position(lineNumber, column);
 	}
 
-	public static rightPositionatomicSoftTabs(model: ICursorSimpleModel, lineNumber: number, column: number, tabSize: number, indentSize: number): Position {
+	public static rightPositionAtomicSoftTabs(model: ICursorSimpleModel, lineNumber: number, column: number, tabSize: number, indentSize: number): Position {
 		const minColumn = model.getLineMinColumn(lineNumber);
-		if (column < model.getLineMaxColumn(lineNumber)) {
-			const lineContent = model.getLineContent(lineNumber);
-
-			const newPosition = AtomicTabMoveOperations.atomicPosition(lineContent, column - minColumn, tabSize, Direction.Right);
-			if (newPosition !== -1) {
-				column = minColumn + newPosition;
-			} else {
-				column += strings.nextCharLength(model.getLineContent(lineNumber), column - 1);
-			}
-		} else if (lineNumber < model.getLineCount()) {
-			lineNumber += 1;
-			column = minColumn;
+		const lineContent = model.getLineContent(lineNumber);
+		const newPosition = AtomicTabMoveOperations.atomicPosition(lineContent, column - minColumn, tabSize, Direction.Right);
+		if (newPosition === -1) {
+			return this.rightPosition(model, lineNumber, column);
 		}
-		return new Position(lineNumber, column);
+		return new Position(lineNumber, minColumn + newPosition);
 	}
 
 	public static right(config: CursorConfiguration, model: ICursorSimpleModel, lineNumber: number, column: number): CursorPosition {
 		const pos = config.atomicSoftTabs
-			? MoveOperations.rightPositionatomicSoftTabs(model, lineNumber, column, config.tabSize, config.indentSize)
+			? MoveOperations.rightPositionAtomicSoftTabs(model, lineNumber, column, config.tabSize, config.indentSize)
 			: MoveOperations.rightPosition(model, lineNumber, column);
 		return new CursorPosition(pos.lineNumber, pos.column, 0);
 	}

src/vs/editor/test/common/controller/cursorAtomicMoveOperations.test.ts

@@ -13,48 +13,72 @@ suite('Cursor move command test', () => {
 			{
 				lineContent: '        ',
 				tabSize: 4,
-				expected: [0, 1, 2, 3, 4, 5, 6, 7, 8, -1],
+				expectedPrevTabStopPosition: [-1, 0, 0, 0, 0, 4, 4, 4, 4, -1],
+				expectedPrevTabStopVisibleColumn: [-1, 0, 0, 0, 0, 4, 4, 4, 4, -1],
+				expectedVisibleColumn: [0, 1, 2, 3, 4, 5, 6, 7, 8, -1],
 			},
 			{
 				lineContent: '  ',
 				tabSize: 4,
-				expected: [0, 1, 2, -1],
+				expectedPrevTabStopPosition: [-1, 0, 0, -1],
+				expectedPrevTabStopVisibleColumn: [-1, 0, 0, -1],
+				expectedVisibleColumn: [0, 1, 2, -1],
 			},
 			{
 				lineContent: '\t',
 				tabSize: 4,
-				expected: [0, 4, -1],
+				expectedPrevTabStopPosition: [-1, 0, -1],
+				expectedPrevTabStopVisibleColumn: [-1, 0, -1],
+				expectedVisibleColumn: [0, 4, -1],
 			},
 			{
 				lineContent: '\t ',
 				tabSize: 4,
-				expected: [0, 4, 5, -1],
+				expectedPrevTabStopPosition: [-1, 0, 1, -1],
+				expectedPrevTabStopVisibleColumn: [-1, 0, 4, -1],
+				expectedVisibleColumn: [0, 4, 5, -1],
 			},
 			{
 				lineContent: ' \t\t ',
 				tabSize: 4,
-				expected: [0, 1, 4, 8, 9, -1],
+				expectedPrevTabStopPosition: [-1, 0, 0, 2, 3, -1],
+				expectedPrevTabStopVisibleColumn: [-1, 0, 0, 4, 8, -1],
+				expectedVisibleColumn: [0, 1, 4, 8, 9, -1],
 			},
 			{
 				lineContent: ' \tA',
 				tabSize: 4,
-				expected: [0, 1, 4, -1, -1],
+				expectedPrevTabStopPosition: [-1, 0, 0, -1, -1],
+				expectedPrevTabStopVisibleColumn: [-1, 0, 0, -1, -1],
+				expectedVisibleColumn: [0, 1, 4, -1, -1],
 			},
 			{
 				lineContent: 'A',
 				tabSize: 4,
-				expected: [0, -1, -1],
+				expectedPrevTabStopPosition: [-1, -1, -1],
+				expectedPrevTabStopVisibleColumn: [-1, -1, -1],
+				expectedVisibleColumn: [0, -1, -1],
 			},
 			{
 				lineContent: '',
 				tabSize: 4,
-				expected: [0, -1],
+				expectedPrevTabStopPosition: [-1, -1],
+				expectedPrevTabStopVisibleColumn: [-1, -1],
+				expectedVisibleColumn: [0, -1],
 			},
 		];
 
 		for (const testCase of testCases) {
-			const actual = testCase.expected.map((_, i) => AtomicTabMoveOperations.whitespaceVisibleColumn(testCase.lineContent, i, testCase.tabSize));
-			assert.deepStrictEqual(actual, testCase.expected);
+			const maxPosition = testCase.expectedVisibleColumn.length;
+			for (let position = 0; position < maxPosition; position++) {
+				const actual = AtomicTabMoveOperations.whitespaceVisibleColumn(testCase.lineContent, position, testCase.tabSize);
+				const expected = [
+					testCase.expectedPrevTabStopPosition[position],
+					testCase.expectedPrevTabStopVisibleColumn[position],
+					testCase.expectedVisibleColumn[position]
+				];
+				assert.deepStrictEqual(actual, expected);
+			}
 		}
 	});