Notebook: Undo/Redo

[rebornix]

Issue Type: Feature Request

Notebook is a resource which contains multiple text models so it leads to interesting requirements for workspace Undo/Redo. Right now there are three major scenarios

• Users are editing the content of a cell (focus is now in a monaco editor), Undo/Redo should work inside this editor. We already have this support.
• Users add/delete a cell, Undo/Redo should remove/re-add cells. We can archive this by undoRedoService.pushElement(resource) and the resource passed in is the resource uri of the notebook document
• 🔴 Users do find & replace all on the notebook document. The change is a WorkspaceEdit and Undo/Redo is not working properly
  • We use BulkEditService to apply the WorksapceEdit. WorkspaceEdit consists of edits for multiple cells, which are all TextModels.
  • To Undo/Redo the WorkspaceEdit, we have to focus the text editor for the first modified cell of the WorkspaceEdit. However, we want to allow Undo/Redo when the focus is not in any text editor.

I'm wondering if we can add something like undoRedoAllowedResources property to the IWorkspaceUndoRedoElement, thus in the notebook case, it will always be [notebookUri, firstModifiedCellUri].

export interface IWorkspaceUndoRedoElement {
	readonly type: UndoRedoElementType.Workspace;
	readonly resources: readonly URI[];
+       readonly undoRedoAllowedResources: URI[];
	readonly label: string;
	undo(): Promise<void> | void;
	redo(): Promise<void> | void;
	split(): IResourceUndoRedoElement[];
}

cc @mjbvz

VS Code version: Code - Insiders 1.44.0-insider (798481c, 2020-03-16T07:10:59.884Z)
OS version: Darwin x64 19.2.0

[vscodebot]

This feature request is now a candidate for our backlog. The community has 60 days to upvote the issue. If it receives 20 upvotes we will move it to our backlog. If not, we will close it. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

[mjbvz]

See #90110 for the issue with custom editors. Whatever solution we come up with may also work for notebooks

[rebornix]

Discussed offline with @alexdima about the expected behavior of Undo/Redo in Notebook, it makes better sense that cells(which are text documents under the hood) in a notebook document share the same undo stack, which is also the undo stack for the notebook document itself.

Considering these operations on a notebook document which contains 3 cells

1. F2 on a variable in cell#1, which modifies both cell#1 and `cell#2. (WorkspaceEdit: [cell#1 uri, cell#2 uri])
2. Delete cell3 (ResourceEdit: notebook document uri)
3. Add a new line in cell#2. (ResourceEdit: cell#2 uri)

Now focus in cell#1 and run Undo three times should revert above three steps one by one in revered order. It doesn't work currently as resources don't share undo stacks.

One idea to solve this problem is having an unique Uri comparison key for Notebook resources, and the UndoService will use this key to decide if they are on the same stack or not. @jrieken already made the Notebook Document Uri and Cell Uri share the same path but differs in query so using Uri.path as the Uri comparison key would work. Another idea @jrieken proposed is using fragment whose semantic is correct in the notebook scenario.

[jrieken]

fyi - discussions/problems around our current uri comparison challenge: #92502

One idea to solve this problem is having an unique Uri comparison key for Notebook resources, and the UndoService will use this key to decide if they are on the same stack or not.

That's some we should explore but that knowledge, e.g what compare-logic to use when, must somehow be pluggable (maybe per scheme) because things like rename-workspace edits don't know that they work on notebook cells