Support linked undo operations

[jrieken]

This is left-over work from composite undo and describes the problem that you cannot undo a Java class rename in one operation but need two undo's.

[joaomoreno]

Pushing out to August.

[rebornix]

One use case from notebook, to split a cell by cursor position, what we do now is

• delete content after the cursor in the cell
• insert new cells into the notebook document

right now when performing above two operations, we explicitly ask them not to push undo elements into the stack, and we create a custom undo element instead. It would be simpler with linking two undo operations.

[rebornix]

@alexdima @jrieken it seems there is an issue with edits with the same undo/redo comparison key in the same group, I tried to apply bulk edits like

await this._bulkEditService.apply(
	[
		new ResourceTextEdit(cell.uri, { range: textModel.getFullModelRange(), text: newLinesContents[0] }),
		new ResourceNotebookCellEdit(this._notebook.uri,
			{
				editType: CellEditType.Replace,
				index: index + 1,
				count: 0,
				cells: newLinesContents.slice(1).map(line => ({
					cellKind: kind,
					language,
					source: line,
					outputs: [],
					metadata: {}
				}))
			}
		)
	],
	{ quotableLabel: 'Split Notebook Cell' }
);

The first edit is happening on file:///.../api.github-issues#00001 and the second edit is on file:///.../api.github-issues, and their comparison key is the same. When performing Undo, it seems it will only undo edit one by one instead of undoing them all.

[rebornix]

Undo with above setup will not run into this code path

vscode/src/vs/platform/undoRedo/common/undoRedoService.ts

Lines 1038 to 1042 in 6891688

	const [matchedElement, matchedStrResource] = this._findClosestUndoElementInGroup(element.groupId);
	if (element !== matchedElement && matchedStrResource) {
	// there is an element in the same group that should be undone before this one
	return this.undo(matchedStrResource);
	}

[alexdima]

@rebornix I did not adopt the undo redo grouping for ResourceNotebookCellEdit, basically the call to IUndoRedoService,pushUndo must also pass in the exact same UndoRedoGroup instance.

[rebornix]

@alexdima I did that (passing the undoRedoGroup created by the BulkEdit service to ResourceNotebookCellEdit at the end) 3222df5 but still saw this issue.

[alexdima]

I can try debugging if you could tell me the exact repro steps or you can try debugging by using const DEBUG = true in src/vs/platform/undoRedo/common/undoRedoService.ts

[rebornix]

@alexdima thanks, DEBUG helps me figure out why these edits are in two different groups. The bulk edit (#101789 (comment)) includes a text edit and a notebook edit, and we pass in the same undoRedo group. However when applying the text edit, since tasks.length === 1, it does not use the undoRedo group passed in (if there are more than one text edit, they then are put in the same group).

vscode/src/vs/workbench/contrib/bulkEdit/browser/bulkTextEdits.ts

Lines 215 to 239 in 4aa873b

	const validation = this._validateTasks(tasks);
	if (!validation.canApply) {
	throw new Error(`${validation.reason.toString()} has changed in the meantime`);
	}
	if (tasks.length === 1) {
	// This edit touches a single model => keep things simple
	for (const task of tasks) {
	task.model.pushStackElement();
	task.apply();
	task.model.pushStackElement();
	this._progress.report(undefined);
	}
	} else {
	// prepare multi model undo element
	const multiModelEditStackElement = new MultiModelEditStackElement(
	this._label,
	tasks.map(t => new SingleModelEditStackElement(t.model, t.getBeforeCursorState()))
	);
	this._undoRedoService.pushElement(multiModelEditStackElement, this._undoRedoGroup);
	for (const task of tasks) {
	task.apply();
	this._progress.report(undefined);
	}
	multiModelEditStackElement.close();
	}

I'm wondering if we should allow pass in undoRedoGroup in task.model.pushStackElement() (and all its inner pushStackElement call)

[alexdima]

@jrieken Can you please help here? I think this is something specific about bulkTextEdits.ts , like in this very special case it should behave as a cross file edit and not as a single file edit.

[jrieken]

@alexdima Is there any harm if the simple path (task.length === 1) is being removed and we always go through the service?

[alexdima]

@jrieken I think it should be fine, but I suggest we do it in the debt week, and test it out a bit. Maybe you have a way to find out this special use-case where the text edit is a part of a larger bulk edit and only do it then?

I think many extensions use some vscode API that ends up here with a single file (e.g. liveshare that I know about), and pushing multi edit stack elements with a single buffer will work, but it might end up always prompting in the undo case ("undo across all files?" even if it is a single file).

[jrieken]

created #107677 for the follow-up work