differentiate notebook moving to new group and splitting

[rebornix]

@bpasero the short summary for the PR is differentiating Move Editor to New Group and Splitting Editors. Please let me know if there is any better way to archive the same thing.

Currently EditorOverride doesn't have enough information about whether users are splitting an editor or just moving an editor to another editor group. It's fine for Monaco Editor as recreating the view/models for the editor is cheap while for Notebook (and potentially any editor using webview), we can't do that as recreating a new editor means the intermediate of the webview is lost.

The workflow for moving an editor to a new group

- editorGroupView.openEditor(input, options)
  - editorOverride.open(input, options)
  - NotebookEditor.setInput()
    - new `NotebookEditorWidget`
- editorGroupView.closeEditor(input)

For moving notebook editor to a new group, we want to reuse the NotebookEditorWidget referenced in previous Notebook Editor, however we don't know if it's safe to do so as editorGroupView.closeEditor(input) happens after we create the new editor in the new group. In this PR, I tried to give this information to EditorOverride and they can decide if they should create a complete new editor or not.

[rebornix]

@bpasero here we check if it's splitting or moving by reading shouldCreateNewWhenOverride, if it's just moving the editor, the new editor input will claim ownership of the NotebookEditorWidget (which is a detached DOM node).
Write

[bpasero]

I am not a big fan of adding a new editor option that is only being set internally from the editor group view. The intent of editor options is to use them from the outside when opening an editor, but not hide internals in there.

Wouldn't it make more sense to maybe add some kind of context to the editor override facilities that tell you why an editor is opening and then act accordingly? That seems like the right place to me to put this kind of information.

[rebornix]

@bpasero thanks for the feedback, makes sense that we should not pollute the EditorOptions as it's widely used.

The move/copy knowledge is inside the EditorGroupView and the knowledge needs to be transferred from group to another when opening a new editor in a new group, I'd like to propose following changes, considering now how editor override works.

Firstly, the info is transferred through openEditor:

export interface IEditorGroup {
-	 openEditor(editor: IEditorInput, options?: IEditorOptions | ITextEditorOptions): Promise<IEditorPane | null>;
+        openEditor(editor: IEditorInput, options?: IEditorOptions | ITextEditorOptions, keepCopy?: boolean): Promise<IEditorPane | null>;
}

and then we pass the info to editorOverrides

async openEditor(editor: EditorInput, options?: EditorOptions, keepCopy?: boolean): Promise<IEditorPane | null> {

		// Guard against invalid inputs
		if (!editor) {
			return null;
		}

		// Editor opening event allows for prevention
-		const event = new EditorOpeningEvent(this._group.id, editor, options);
+		const event = new EditorOpeningEvent(this._group.id, editor, options, keepCopy);
		this._onWillOpenEditor.fire(event);
		const prevented = event.isPrevented();
		if (prevented) {
			return withUndefinedAsNull(await prevented());
		}

		// Proceed with opening
		return withUndefinedAsNull(await this.doOpenEditor(editor, options));
	}

and lastly this._onWillOpenEditor.fire(event); will trigger the editor overrides, which is the current design.

@bpasero how do you like it now? The challenge here is editor overrides are triggered when an editor is opened
in the group view (through onWillOpenEditor event), it means that the group needs to know that if an editor is "move" or "copy".

[bpasero]

@rebornix Maybe not a bare keepCopy boolean but a context enum that indicates the source of operation? Similar to this:

vscode/src/vs/workbench/common/editor.ts

Line 318 in da8f44c

reason?: SaveReason;

[rebornix]

Yes, an enum is better. I'll update the PR>

[rebornix]

OpenEditorInGroupContext enum is introduced to indicate the reason how the editor is opened in the new group. We may want to extend to something like { reason, balaba } if later on we want to extend the context.

[bpasero]

Overall good with me, some polish maybe:

• OpenEditorInGroupContext => OpenEditorContext
• the enum should initialize its first value to be 1 and not 0 so that we prevent bad checks
• I see no tests that verify the event carries the proper context, please add it