A second instance of a notebook is opened when debugging with two editor groups

[roblourens]

• Open a notebook in one editor group 1, and a editor group 2 with anything in it
• Start RBL in the notebook
• Focus group 2
• Open the debug view, click the stack frame for the notebook cell (there are other ways to make this happen too)
• A second notebook editor instance is opened in group 2

We just call openEditor and it should reveal the cell in the editor where it's already open. But I think that we can't do this correctly for notebook cells. Where is the code that checks whether the editor is already open?

[lramos15]

Simpler repo without debugging.

1. Open a github issue notebook
2. Create an error
3. Focus a different group
4. Click the problem in the problems panel
5. 🐛 New notebook appears

[bpasero]

This is probably a duplicate of #131619

Here is what happens:

• you click on a problem
• we open an untyped input as shown below
• we ask the notebook if it matches that input
• notebook says, it doesn't match
• we consider the right hand empty group as target

Our weak hack does not work in this case because a cell URI is opened:

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

Lines 201 to 208 in 34a321e

	// Note: intentionally doing a "weak" check on the resource
	// because `EditorInput.matches` will not work for untyped
	// editors that have no `override` defined.
	//
	// TODO@lramos15 https://github.com/microsoft/vscode/issues/131619
	if (typedEditor.resource) {
	return isEqual(typedEditor.resource, EditorResourceAccessor.getCanonicalUri(editor));
	}

We have to stop calling findGroup with untyped editors because they will never match properly.

[lramos15]

• we ask the notebook if it matches that input

This sounds like an easy enough fix then. The notebook knows how to translate a cell URI into the notebook URI. That's what it should use to determine the group

[bpasero]

Yeah it is a workaround if it is possible for a typed notebook input to know the cell it contains, would have to go somewhere here:

vscode/src/vs/workbench/contrib/notebook/common/notebookEditorInput.ts

Lines 285 to 287 in aaa8e4d

	if (otherInput instanceof NotebookEditorInput) {
	return this.viewType === otherInput.viewType && isEqual(this.resource, otherInput.resource);
	}

[roblourens]

So matches should return true if the other is a cell inside this notebook? That's easy enough but could there be any other consequences that I should watch out for?

[lramos15]

I don't think there's any consequences as you effectively want each cell to be considered the same input as the parent notebook.

[roblourens]

If you say so... not gonna touch it for September though

[rebornix]

@roblourens do we still want to change the behavior now or we do it early next week? Pushing to Nov first.

[roblourens]

I guess next month

[lramos15]

Reopening because fix was reverted

[lramos15]

@bpasero Talked with @rebornix a bit and matches cannot match for cell uris in this case as they actually want to create and resolve a new editor input which we do not do if the active editor matches the passed in one as it is normally unnecessary work. I was wondering if they should add special logic here

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

Lines 196 to 211 in 34a321e

	function matchesEditor(typedEditor: EditorInput, editor: EditorInput | IUntypedEditorInput): boolean {
	if (typedEditor.matches(editor)) {
	return true;
	}
	// Note: intentionally doing a "weak" check on the resource
	// because `EditorInput.matches` will not work for untyped
	// editors that have no `override` defined.
	//
	// TODO@lramos15 https://github.com/microsoft/vscode/issues/131619
	if (typedEditor.resource) {
	return isEqual(typedEditor.resource, EditorResourceAccessor.getCanonicalUri(editor));
	}
	return false;
	}

to have the notebook and cell uri match so that the group chosen is correct. I know it's not ideal because it is notebook specific

[bpasero]

Would #131619 address this too?

[lramos15]

@bpasero I think so as the cell uri is finding the incorrect target group. If the editor input created was opened in the correct group it would simply reveal the notebook and scroll it as expected.

[roblourens]

I don't understand what has to be done here. Any change a year later?

[lramos15]

@roblourens I don't believe we will be able to fix this until #131619 is addressed

[roblourens]

Why is that one related?

For future reference, here are my notes from our call a couple weeks ago

• In NotebookServiceImpl, add a separate editor registration for the Untitled- pattern, and look up the matching open editor input, set the cellOptions to reveal the cell, and return that
• Could possibly reduce the number of factories by just registering one for all registered editor types, in one glob

[lramos15]

Why is that one related?

For future reference, here are my notes from our call a couple weeks ago

• In NotebookServiceImpl, add a separate editor registration for the Untitled- pattern, and look up the matching open editor input, set the cellOptions to reveal the cell, and return that
• Could possibly reduce the number of factories by just registering one for all registered editor types, in one glob

Those notes are for the untitled issue this is for something else. The issue here is that the Cell URI doesn't match the notebook URI in the matches call so we improperly select the second group and then it resolves to the exact same notebook causing two to be opened. The reason we have to call this matches before resolution is complete and not after is due to webviews needing to know what group they belong to. Ideally an editor input is group agnostic which is why we hope @mjbvz can eventually resolve #131619 as there are some issues that just cannot be fixed in a proper manner without it,

[roblourens]

Oops, thanks. #166161