Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notebook Editor: scoped context key service #99318

Closed
rebornix opened this issue Jun 3, 2020 · 9 comments
Closed

Notebook Editor: scoped context key service #99318

rebornix opened this issue Jun 3, 2020 · 9 comments
Assignees
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders notebook

Comments

@rebornix
Copy link
Member

rebornix commented Jun 3, 2020

The context key service we used in the notebook editor is not scoped to the DOM node, which might lead to potential context key issues

@IContextKeyService readonly contextKeyService: IContextKeyService,

We should create context key service which scoped to NotebookEditorWidget root DOM node.

@roblourens
Copy link
Member

I'm pretty sure that if you open two notebooks, the second is overwriting the context keys of the first. How this works for code editors - they need to unwrap an ICodeEditor here:

const codeEditor = getCodeEditor(activeEditorPane.getControl());

@bpasero I'm confused about that code, it looks like I would need to extend it for every editor type in order to get the editor title control to render actions using the editor's contextKeyService?

@rebornix
Copy link
Member Author

rebornix commented Jun 8, 2020

@roblourens we can use

const activeControl = activeEditorPane.getControl();
if (isCompositeEditor(activeControl) && isCodeEditor(activeControl.activeCodeEditor)) {
	return activeControl.activeCodeEditor;
}

@rebornix
Copy link
Member Author

rebornix commented Jun 8, 2020

maybe we should copy logic from

const activeTextEditorControl = this.editorService.activeTextEditorControl;
if (isCodeEditor(activeTextEditorControl)) {
return activeTextEditorControl;
}
if (isDiffEditor(activeTextEditorControl)) {
return activeTextEditorControl.getModifiedEditor();
}
const activeControl = this.editorService.activeEditorPane?.getControl();
if (isCompositeEditor(activeControl) && isCodeEditor(activeControl.activeCodeEditor)) {
return activeControl.activeCodeEditor;
}
return null;

to

export function getCodeEditor(thing: any): ICodeEditor | null {
if (isCodeEditor(thing)) {
return thing;
}
if (isDiffEditor(thing)) {
return thing.getModifiedEditor();
}
return null;
}

@jrieken is it safe to do that?

@roblourens
Copy link
Member

Well, I don't want the context key service from the code editor inside the notebook editor, I want it from the notebook editor itself.

@rebornix
Copy link
Member Author

rebornix commented Jun 8, 2020

right, I misread it. I think then we can extend

export interface ICompositeCodeEditor {

by adding invokeWithinContext

@bpasero
Copy link
Member

bpasero commented Jun 9, 2020

@roblourens I cannot recall, I think Jo wrote that code to ensure the title menu gets the most appropriate context key service before rendering.

@jrieken
Copy link
Member

jrieken commented Jun 9, 2020

I want it from the notebook editor itself.

Each editor group gets a dedicated context key service - a simple way to enumerate them is to paste this into the console document.querySelectorAll('[data-keybinding-context]').

I am not quite sure what the actual issue is but I can image that there a problem because context key hierarchies work along the dom structure and given that notebook editors might not always follow that structure, right?

@roblourens
Copy link
Member

The real issue is what to do when we move a notebook from one group to another, and that partly determines whether the editor should get its own group or not. Currently we keep the notebook widget alive when the Editor is recreated in the new group, to avoid disposing the webview. So we can

  • Reinstantiate the notebook widget, and only keep the webview alive. Peng says this is hard because the notebook list and the webview are tightly coupled.
  • Give the notebook widget its own contextKeyService and reparent this into the new editor group context key service. Currently this isn't possible but doesn't look too difficult. But is definitely weird

@roblourens roblourens modified the milestones: June 2020, July 2020 Jun 30, 2020
@roblourens roblourens modified the milestones: July 2020, August 2020 Aug 3, 2020
@roblourens
Copy link
Member

After fixing https://github.com/microsoft/vscode/issues, this is an issue again

  • Drag notebook from group A to group B
  • Close group A
  • Now looking up context keys fails

image

roblourens added a commit that referenced this issue Aug 14, 2020
- Give NotebookEditorWidget its own ScopedContextKeyService
- Give the title control a way to get the context key service inside any editor
Fix #99318
@rebornix rebornix removed their assignment Sep 1, 2020
@roblourens roblourens modified the milestones: August 2020, September 2020 Sep 2, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders notebook
Projects
None yet
Development

No branches or pull requests

5 participants
@roblourens @rebornix @bpasero @jrieken and others