-
Notifications
You must be signed in to change notification settings - Fork 27.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Join Lines Redesigned #71678
Join Lines Redesigned #71678
Conversation
} | ||
const range = new Range(line, model.getLineMaxColumn(line) - whitespaceAtEnd[0].length, line + 1, model.getLineFirstNonWhitespaceColumn(line + 1)); | ||
if (model.getLineContent(line).length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be mode.getLineLength(line)?
} | ||
private _getLinesToJoin(editor: IActiveCodeEditor): IJoinLinesOperation[] { | ||
// Construct Join operations | ||
let operations: IJoinLinesOperation[] = editor.getSelections().map((s) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit pick: Could s be named better?
return new Selection(previousValue.startLineNumber, previousValue.startColumn, currentValue.endLineNumber, currentValue.endColumn); | ||
} | ||
let endLineNumber = s.endLineNumber; | ||
if (s.startLineNumber < s.endLineNumber && s.endColumn === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the significance of the magic number 1? Could this is improved by adding a name constant to reveal what 1 means?
} | ||
private _getLinesToJoin(editor: IActiveCodeEditor): IJoinLinesOperation[] { | ||
// Construct Join operations | ||
let operations: IJoinLinesOperation[] = editor.getSelections().map((s) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think operations can be a const?
if (model === null) { | ||
return; | ||
// Merge Join operations which are adjacent or overlapping | ||
let mergedOperations: IJoinLinesOperation[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be a constant
} | ||
|
||
lineOffset += deleteSelection.endLineNumber - deleteSelection.startLineNumber; | ||
let start = model.getLineFirstNonWhitespaceColumn(startLineNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can start be constant?
let selectionEndPositionOffset = model.getLineContent(selection.endLineNumber).length - selection.endColumn; | ||
let removeLines = this._getLinesToJoin(editor); | ||
let ops: IIdentifiedSingleEditOperation[] = []; | ||
let conString: String[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be a constant
let selectionEndPositionOffset = model.getLineContent(selection.endLineNumber).length - selection.endColumn; | ||
let removeLines = this._getLinesToJoin(editor); | ||
let ops: IIdentifiedSingleEditOperation[] = []; | ||
let conString: String[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When shortening names it makes it harder to understand what it is used for? What is con? Would naming the variable more clearly make it more readable?
editor.pushUndoStop(); | ||
editor.executeEdits(this.id, edits, endCursorState); | ||
editor.executeEdits(this.id, ops, cursorState); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is now rather large. Can be be broken down into smaller private functions?
I would recommend to implement this experience in an extension. Thanks for your contribution though ❤️ |
This PR fixes #64655.
It redesigns multi-cursor Join Lines to allow for multiple lines to be merged in.
Before:
Examples:
As a note, it will merge in adjacent selections.