Reverse lines: keep selection #284988
Reverse lines: keep selection #284988
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the "Reverse Lines" action to improve user experience by better managing selections during line reversal operations.
Key changes:
- Preserves within-line text selections when reversing the entire document
- Automatically selects reversed line ranges when reversing multiple lines (to make the action's effect visible to users)
- Updates selection handling logic to maintain the correct cursor position and selection direction
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/vs/editor/contrib/linesOperations/browser/linesOperations.ts | Refactored the ReverseLinesAction to preserve single-line selections during whole-document reversal and to select the full range of lines when reversing multi-line selections |
| src/vs/editor/contrib/linesOperations/test/browser/linesOperations.test.ts | Updated tests to verify the new selection preservation behavior for whole-document reversal and the new line-selection behavior for multi-line reversals |
| let selections = originalSelections; | ||
| if (selections.length === 1 && selections[0].isEmpty()) { | ||
| let ranges: Range[] = originalSelections; | ||
| if (ranges.length === 1 && ranges[0].isEmpty()) { |
There was a problem hiding this comment.
The condition should include single-line selections to match the test expectations. The test "preserves selected text when reversing whole document" expects the whole document to be reversed when there's a single-line selection at (2,5,2,8), but with the current condition, the whole document is only reversed when the selection is empty. Consider changing this to check if the selection is either empty or single-line.
| if (ranges.length === 1 && ranges[0].isEmpty()) { | |
| if (ranges.length === 1 && (ranges[0].isEmpty() || ranges[0].isSingleLine())) { |
| // Create selection: from (newSelectionStart, newSelectionStartColumn) to (newPosition, newPositionColumn) | ||
| // After reversal: from (3, 2) to (1, 3) | ||
| return new Selection(newSelectionStart, newSelectionStartColumn, newPosition, newPositionColumn); | ||
| // Select range that was sorted (so action is clear to user), reversing original selection direction (so cursor remains on same line of text) |
There was a problem hiding this comment.
The comment says "range that was sorted" but this is the reverse lines action, not the sort lines action. The comment should say "range that was reversed" for clarity.
| // Select range that was sorted (so action is clear to user), reversing original selection direction (so cursor remains on same line of text) | |
| // Select range that was reversed (so action is clear to user), reversing original selection direction (so cursor remains on same line of text) |
| const expectedSelectedText: string[] = ['al', 'bob', 'char']; | ||
| assert.deepStrictEqual(editor.getSelections().map(s => model.getValueInRange(s)), expectedSelectedText); | ||
| editor.setSelection(new Selection(2, 5, 2, 8)); | ||
| const expectedSelectedText = "the"; |
There was a problem hiding this comment.
Use single quotes for strings that don't need to be externalized. According to the coding guidelines, double quotes should only be used for user-facing strings that need localization.
| editor.setSelection(new Selection(1, 2, 3, 3)); | ||
| // Select from line 2 to 3 | ||
| editor.setSelection(new Selection(2, 9, 3, 8)); | ||
| assert.strictEqual(model.getValueInRange(editor.getSelection()), "builder\ncharlie"); |
There was a problem hiding this comment.
Use single quotes for strings that don't need to be externalized. According to the coding guidelines, double quotes should only be used for user-facing strings that need localization.
| // Select from line 2 to 3 | ||
| editor.setSelection(new Selection(2, 9, 3, 8)); | ||
| assert.strictEqual(model.getValueInRange(editor.getSelection()), "builder\ncharlie"); | ||
| const expectedCurrentLine = "charlie the creator"; |
There was a problem hiding this comment.
Use single quotes for strings that don't need to be externalized. According to the coding guidelines, double quotes should only be used for user-facing strings that need localization.
| assert.deepStrictEqual(model.getLinesContent(), ['alice', 'charlie the creator', 'bob the builder', 'david'], "Lines 2 and 3 should be reversed"); | ||
| assert.deepEqual(model.getLineContent(editor.getPosition().lineNumber), expectedCurrentLine, "Text on current line should remain the same"); | ||
| assertSelection(editor, new Selection(3, 16, 2, 1)); | ||
| assert.strictEqual(model.getValueInRange(editor.getSelection()), "charlie the creator\nbob the builder", "Lines 2 and 3 should be selected entirely"); |
There was a problem hiding this comment.
Use single quotes for strings that don't need to be externalized. According to the coding guidelines, double quotes should only be used for user-facing strings that need localization.
hickford
force-pushed
the
selection
branch
2 times, most recently
from
ff6e494 to
65f8b89
Compare
When reversing the whole document, preserve selected text. Otherwise, select the lines that were reversed. This makes clear to the user what happened.
2 participants
When reversing the whole document, preserve selected text.
Otherwise, select the lines that were reversed. This makes clear to the user what happened.