feat: Support multi cursor line moves #68695

2mas · 2019-02-14T10:36:28Z

Fixes #68694 moving multiple consequitive lines

msftclas · 2019-02-14T12:54:17Z

All CLA requirements met.

2mas · 2019-02-14T13:50:41Z

Needs some more work, lines get scrambled

2mas · 2019-02-16T10:20:36Z

Perhaps a comment about why selections needs to be reversed and executed individually should be added for clarity, tell me if I can assist more

2mas · 2019-02-28T07:10:13Z

Latest commit covers scenario when selection-direction goes upwards. Reversing the processing of selections should only occur when lines are moved in the same direction as the selections are made.

Also had to make the selections distinct by line. Keeping the selections can probably be solved too since their column-positions wont be changed. If you're interested in this approach in this PR I can give that a try aswell

2mas · 2019-03-01T14:32:43Z

Decided to implement the stashing of selections anyway, also added a block when top/end of doc is reached. I'm probably done by now. :)

2mas · 2019-03-28T12:12:48Z

@rebornix any status on this? Just curious

rebornix · 2019-03-28T14:29:56Z

@2mas let's shoot for next week (beginning of next iteration). BTW, I'd love to see some tests as well but I can do that if the time doesn't work for you.

2mas · 2019-03-31T06:42:51Z

Awesome, I’ll see if I can come up with something (limited available time I’m afraid).

2mas · 2019-04-09T12:49:22Z

Added a few testcases and resolved a conflict with master, should be good to test-drive now.

vivodi · 2024-12-29T16:31:22Z

Is this still relevant?

2mas · 2025-01-01T20:23:15Z

Is this still relevant?

It seems so, the issue is still there in the editor and keeps annoying users apparently. - #11809 (comment)

No idea about the state of the code in my PR today though, spent some time on it years ago to make it good to testdrive and merge but it could have fallen short in prioritization since its probably not impacting the majority of the users, or there is something else about the solution that stops it

rebornix · 2025-01-21T00:51:26Z

This change requires comprehensive testing before merging. At the same time I'm not sure if we are investing more in the textmate indentation rules. I'll leave this to @aiday-mar to decide.

aiday-mar

Hi thank you for making this PR and apologies for the delay in reviewing this PR. I think this PR would be a very valuable contribution. I have looked at it, and I think the approach is overall correct but I have several questions as well as changes I think we should make before merging. It would be good to also test this PR on many different corner cases to make sure it does not introduce bugs. I will do that too. There is also a merge conflict which needs to be resolved first.

aiday-mar · 2025-06-05T12:24:04Z

src/vs/editor/contrib/linesOperations/linesOperations.ts

+
+			editor.pushUndoStop();
+		} else {
+			editor.pushUndoStop();


Could we unify this part of the code between the two cases? We could construct the commands array entirely first and then call executeCommands on this commands array. Then after this, if we have moved multiple lines, we can set the selections.

This didn't work out for me. For some reason it just doesn't process all of the commands

aiday-mar · 2025-06-05T12:27:40Z

src/vs/editor/contrib/linesOperations/linesOperations.ts


 		for (const selection of selections) {
-			commands.push(new MoveLinesCommand(selection, this.down, autoIndent));
+			if (movingMultipleLines) {
+				// If we are at the beginning/end of document and multiple lines are moved, abort.


Do we need to do this check for whether the selection is at the beginning or end of the document? The other case when we are not moving multiple lines does not do such a check, and it would be good to unify as much as possible the code. Instead of doing the check, when we calculate the new selections, if one of the selections is on the first line and we move up, the new selection should be the same as the old selection, because as I assume, the operation would be a no-op in this case.

This is actually one thing that would be best if you decide on probably, we abort the whole thing here to keep the "format" of the selections, for example if we have these two lines selected on top of the document and move up (line 5 & 6), it will be a no-op currently and we maintain one line between them.

If we remove this block, it will continue with the second selection and move it up, this may be a good thing to do:

And if we move it up again it will replace the first selection (since the first one is processed first and stays in place/no-op, the second one will be processed afterwards and take its place, putting the first one below). To prevent this there could be a check for an existing selection on the line above/below and no-op in that case. I'll see if I can come up with something clean and easy for this

I think this behavior is okay. We could just not treat this case when the selection is at the beginning or at the end and wait to see what users think.

aiday-mar · 2025-06-05T12:32:52Z

src/vs/editor/contrib/linesOperations/linesOperations.ts

+					newSelections.push(new Selection(newStartLine, startCol, newEndLine, endCol));
+				}
+
+				// Work only with one selection per line


Let's put this filter operation before the for loop above, so as not to loop over uneccessary selections in the for loop.

Also instead of doing a filter operation followed by a map operation followed by an indexOf operation which is O(n^2) where n is the number of selections could we just loop once over the selections, keep track of the endLineNumber of the last processed selection and if the current selection does not match this endLineNumber add the current selection to a pre-created selections array. This will allow us to reduce the complexity to O(n).

Great catch, will fix

aiday-mar · 2025-06-05T12:33:19Z

src/vs/editor/contrib/linesOperations/linesOperations.ts

+					return arr.map(sel => sel['endLineNumber']).indexOf(s['endLineNumber']) === idx;
+				});
+
+				selectionDirectionDown = selections[0].endLineNumber < selections[selections.length - 1].endLineNumber;


I am not sure I understand this piece of code. Why do we calculate the selectionDirectionDown. What does it mean?

We need to know if the selection direction is down or up (Add Next Occurrence vs Add Previous Occurrence) and if we are initiating a down-move, then we need to process the selections in reversed order from the bottom and up, otherwise they would replace each other. I will try to simplify and clarify this with a comment

In that case is not sufficient to just check if we are initiating a downwards movement? I am not exactly sure, but it seems to me selections are not necessarily ordered from top to bottom.

Could you give a concrete example for what this code solves?

Yes, if I do this:

And select some lines with Add Previous Occurrence:

And move down:

vs

Add Previous Occurrence:

move down:

It's because "line 3" gets processed first -> moved to row 6, then the next selection is processed on row 6, which is now "line 3" -> moved to row 7, next selection gets processed on row 7, which is "line 3" -> ends up on row 8. So this is why process order must be right :)

aiday-mar · 2025-06-05T12:34:40Z

src/vs/editor/contrib/linesOperations/linesOperations.ts

+			if (movingMultipleLines) {
+				// Stash selections while processing, set new selections +/- line-change only
+				for (const selection of selections) {
+					let startCol = selection.getStartPosition().column;


To unify the code between the two cases, you can just calculate the new selections array in both cases (whether there are multiple selections or not). And then at the very end set the new selections in all cases. This will help avoid these if/else branching.

2mas · 2025-06-10T07:45:43Z

Hi thank you for making this PR and apologies for the delay in reviewing this PR. I think this PR would be a very valuable contribution. I have looked at it, and I think the approach is overall correct but I have several questions as well as changes I think we should make before merging. It would be good to also test this PR on many different corner cases to make sure it does not introduce bugs. I will do that too. There is also a merge conflict which needs to be resolved first.

Hi! Nice to see that you find this useful, let me get back with resolved conflicts and some adjustments and clarifications.

I have some issues running this project locally though, but I managed to get the web-editor working at least, but tests are only running half-way for some reason, perhaps I can rely on CI anyway, I'll do my best effort

aiday-mar · 2025-06-10T07:47:36Z

What issues are you seeing while running this project locally? You should be able to compile as well as run the tests and test with OSS

…tions, otherwise its already selected

Correcting multiple move-lines when having made selections in reversed order

… line. Applies stash after line movement to keep previous selections. Aborting when moving multiple lines and the line is currently at top or end of document

2mas · 2025-06-10T12:09:14Z

What issues are you seeing while running this project locally? You should be able to compile as well as run the tests and test with OSS

Firstly tried running test.sh and code.sh in WSL when cloning into devcontainer which failed, can't remember what was logged. I got closest to full experience with running directly on Windows with node v22.15.1, python 3.13.0 and VS 2022 + workload and individual components installed as per docs, I get these types of issues when running locally in Windows:

npm install
npm run watch

.\scripts\code.bat actually worked on some more attempts now, seems to be some intermittent error previously
.\scripts\test.bat doesn't seem to run all tests, it prints this at the end:

Found --crash-reporter-directory argument. Setting crashDumps directory to be 'C:\Projects\vscode\.build\crashes'
[20020:0610/140701.484:INFO:CONSOLE(2)] "The vm module of Node.js is unsupported in Electron's renderer process due to incompatibilities with the Blink rendering engine. Crashes are likely and avoiding the module is highly recommended. This module may be removed in a future release.", source: node:electron/js2c/renderer_init (2)
[20020:0610/140701.797:INFO:CONSOLE(2)] "Platform browser has already been set. Overwriting the platform with [object Object].", source: C:\Projects\vscode\node_modules\@vscode\vscode-languagedetection\dist\lib\index.js (2)
(node:30204) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `Code - OSS --trace-deprecation ...` to show where the warning was created)
(node:30204) [DEP0025] DeprecationWarning: sys is deprecated. Use util instead.
(node:30204) ExperimentalWarning: WASI is an experimental feature and might change at any time

...

322 passing (2s)
  2 pending
  2 failing

...

2) "after each" hook for "English File menu renders mnemonics":
      + expected - actual

      -false
      +true
  AssertionError [ERR_ASSERTION]: Error: Unexpected console output in test run. Please ensure no console.[log|error|info|warn] usage in tests or runtime errors.
      at Context.<anonymous> (renderer.js:291:11)

errorlevel: 1

aiday-mar · 2025-06-10T13:28:14Z

What exactly is the error you see when running npm i and npm run watch on Windows? This could be happening because you need to merge main and recompile first.

…s, also handle order of selections

…execution

2mas · 2025-06-12T15:01:42Z

I have pushed some updates and covered some edge cases as well, for example if one puts cursors on multiple lines with no order, we will still sort them and apply the move only once per line.

There are some conditions in the code that look a bit complex, tried to explain as comments. Also, I'm not able to run tests locally and I can see from CI that something is wrong, maybe you can assist me with this?

2mas · 2025-06-13T06:59:56Z

PR title updated to better reflect what this does, previous one was not so good since multiple lines can be moved within a single selection, and this PR solves more than that

aiday-mar · 2025-06-13T07:49:20Z

Hi thanks for this PR. Today I am a bit busy, but I will make sure to look at this soon.

2mas · 2025-06-13T12:58:12Z

Hi thanks for this PR. Today I am a bit busy, but I will make sure to look at this soon.

No stress at all. I got the test runner working now (not the .bat, but the integrated runner in vscode started working) so test suite is now repaired

aiday-mar · 2025-06-17T10:59:10Z

Hi thanks for the PR. I pushed some polish changes. I need to discuss this PR with my team, but hopefully this can get merged in.

2mas · 2025-06-17T11:29:09Z

Hi! Order of the initial selections are now lost since the list is already sorted from beginning, but since it requires an extra loop for it, it may not be super important, up to you. Probably its the cause for failing tests.

However the continue doesn't work that well, I'll show a small example

Moving down gets them unordered and selections are lost

Would it be better to keep the return here, otherwise the nearest neighbour doesn't know that the top/bottom line was a no-op and takes its place?

--
I had some uncommitted code to remove the movingMultipleLines branching btw, it wasn't actually necessary since there was a return to abort the operation. I'll add it here if you find something useful to pick from it (also covers the case with multiple cursors on a single row only, they will disappear as it now):

public run(accessor: ServicesAccessor, editor: ICodeEditor): void {
		const languageConfigurationService = accessor.get(ILanguageConfigurationService);
		const autoIndent = editor.getOption(EditorOption.autoIndent);

		const selections = editor.getSelections() || [];
		const distinctSelectionsPerLine: Selection[] = [];
		const selectionsAfterLineMove: Selection[] = [];

		for (const selection of selections) {
			const startCol = selection.getStartPosition().column;
			const startLine = selection.getStartPosition().lineNumber;
			const newStartLine = this.down ? startLine + 1 : startLine - 1;

			const endCol = selection.getEndPosition().column;
			const endLine = selection.getEndPosition().lineNumber;
			const newEndLine = this.down ? endLine + 1 : endLine - 1;

			selectionsAfterLineMove.push(new Selection(newStartLine, startCol, newEndLine, endCol));
		}

		selections.sort((a, b) => a.endLineNumber - b.endLineNumber);
		let lastLineNumber = 0;

		for (const selection of selections) {
			if (lastLineNumber !== selection.endLineNumber) {
				distinctSelectionsPerLine.push(selection);
				lastLineNumber = selection.endLineNumber;
			}
		}

		if (this.down) {
			distinctSelectionsPerLine.reverse();
		}

		const model = editor.getModel();
		const lineCount = model !== null ? model.getLineCount() : 0;

		editor.pushUndoStop();

		for (const selection of distinctSelectionsPerLine) {
			if ((selection.startLineNumber === 1 && !this.down) || (selection.endLineNumber === lineCount && this.down)) {
				editor.pushUndoStop();
				return;
			}

			editor.executeCommand(this.id, new MoveLinesCommand(selection, this.down, autoIndent, languageConfigurationService));
		}

		editor.setSelections(selectionsAfterLineMove);
		editor.pushUndoStop();
	}

aiday-mar · 2025-06-17T13:07:24Z

Hi! Thanks I will have a look again at your comment later this week!

2mas closed this Feb 14, 2019

2mas reopened this Feb 14, 2019

kieferrm requested a review from rebornix February 15, 2019 03:31

kieferrm assigned rebornix Feb 15, 2019

rebornix added this to the On Deck milestone Mar 28, 2019

rebornix added editor-autoindent Editor auto indentation issues feature-request Request for new features or functionality labels Oct 8, 2020

joaomoreno changed the base branch from master to main February 15, 2021 08:51

2mas mentioned this pull request Oct 28, 2021

Move line up/down should support multiple cursors #11809

Open

rebornix assigned aiday-mar and unassigned rebornix Jan 21, 2025

aiday-mar requested review from aiday-mar and removed request for rebornix May 7, 2025 10:05

aiday-mar requested changes Jun 5, 2025

View reviewed changes

aiday-mar added the changes-requested label Jun 5, 2025

aiday-mar changed the title ~~Fixes #68694 moving multiple consequitive lines~~ Allowing to move multiple consecutive lines Jun 5, 2025

2mas added 7 commits June 10, 2025 13:28

Fixes microsoft#68694 moving multiple consequitive lines

9aaee17

Null-check + only apply setSelections if there are multiple new selec…

d4a0b63

…tions, otherwise its already selected

correction, reversing selections back when moving lines down

2660c00

Fixes having multiple selections on same line and moving them

1e009f7

Correcting multiple move-lines when having made selections in reversed order

Stashing all selections before proceeding with selections distinct by…

d55b443

… line. Applies stash after line movement to keep previous selections. Aborting when moving multiple lines and the line is currently at top or end of document

fix strict-null-check

cf9dcee

Adding testcases

076ad04

2mas force-pushed the fix-multiple-consecutive-line-movements branch from 493b319 to 076ad04 Compare June 10, 2025 12:09

2mas added 3 commits June 12, 2025 12:31

fix: move out distinct line filter out of loop + sort by line

69b0f33

fix: improve line movement logic for multiple selections and add test…

8ac7d24

…s, also handle order of selections

fix: update suite name for moving multiple lines and refactor action …

d85b593

…execution

2mas changed the title ~~Allowing to move multiple consecutive lines~~ feat: Support multi cursor line moves Jun 13, 2025

2mas added 2 commits June 13, 2025 14:37

fix test execution

62675c4

fix: remove unnecessary import and comment test suite meaning

ed4182e

polish

c335174

aiday-mar enabled auto-merge (squash) June 17, 2025 10:58

feat: Support multi cursor line moves #68695

Are you sure you want to change the base?

feat: Support multi cursor line moves #68695

Conversation

2mas commented Feb 14, 2019

Uh oh!

msftclas commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2mas commented Feb 14, 2019

Uh oh!

2mas commented Feb 16, 2019

Uh oh!

2mas commented Feb 28, 2019

Uh oh!

2mas commented Mar 1, 2019

Uh oh!

2mas commented Mar 28, 2019

Uh oh!

rebornix commented Mar 28, 2019

Uh oh!

2mas commented Mar 31, 2019

Uh oh!

2mas commented Apr 9, 2019

Uh oh!

vivodi commented Dec 29, 2024

Uh oh!

2mas commented Jan 1, 2025

Uh oh!

rebornix commented Jan 21, 2025

Uh oh!

aiday-mar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiday-mar Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2mas commented Jun 10, 2025

Uh oh!

aiday-mar commented Jun 10, 2025

Uh oh!

2mas commented Jun 10, 2025

Uh oh!

aiday-mar commented Jun 10, 2025

Uh oh!

2mas commented Jun 12, 2025

Uh oh!

2mas commented Jun 13, 2025

Uh oh!

aiday-mar commented Jun 13, 2025

Uh oh!

2mas commented Jun 13, 2025

Uh oh!

aiday-mar commented Jun 17, 2025

msftclas commented Feb 14, 2019 •

edited

Loading

aiday-mar left a comment •

edited

Loading

aiday-mar Jun 5, 2025 •

edited

Loading

2mas commented Jun 17, 2025 •

edited

Loading