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.

Sign up for GitHub

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

Fix interactive window debug cell, codelens tracking, go to cell, expand/collapse all from toolbar when focus is not on notebook editor #6469

Merged
merged 7 commits into from
Jul 1, 2021
Merged

Fix interactive window debug cell, codelens tracking, go to cell, expand/collapse all from toolbar when focus is not on notebook editor #6469

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fade08c
fix #6423
rebornix Jun 30, 2021
2bcf7eb
Debug cell fixes
joyceerhl Jul 1, 2021
cbc46cb
update execution count.
rebornix Jul 1, 2021
f2c60f4
Expand/collapse cell should not rely on `activeNotebookEditor`
joyceerhl Jul 1, 2021
78a27a3
Format
joyceerhl Jul 1, 2021
39692b3
Refactor add/debug code codepaths
joyceerhl Jul 1, 2021
15c0642
Prettier
joyceerhl Jul 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 82 additions & 72 deletions src/client/datascience/interactive-window/nativeInteractiveWindow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,20 +225,8 @@ export class NativeInteractiveWindow implements IInteractiveWindowLoadable {
}
}

public async addCode(code: string, file: Uri): Promise<boolean> {
await this.updateOwners(file);
await this.addNotebookCell(code);
try {
await this.commandManager.executeCommand('notebook.cell.execute', {
ranges: [{ start: this.notebookDocument.cellCount - 1, end: this.notebookDocument.cellCount }],
document: this.notebookDocument.uri,
autoReveal: true
});
return true;
} catch (e) {
traceError(e);
return false;
}
public async addCode(code: string, file: Uri, line: number): Promise<boolean> {
return this.submitCodeImpl(code, file, line, false);
}

public async debugCode(code: string, fileUri: Uri, line: number): Promise<boolean> {
Expand Down Expand Up @@ -269,66 +257,82 @@ export class NativeInteractiveWindow implements IInteractiveWindowLoadable {

// Call the internal method if we were able to save
if (saved) {
await this.updateOwners(fileUri);
const notebookCell = await this.addNotebookCell(code);
const notebook = this.kernel?.notebook;
if (!notebook) {
return false;
}
try {
const finishedAddingCode = createDeferred<void>();
return this.submitCodeImpl(code, fileUri, line, true);
}

// Before we try to execute code make sure that we have an initial directory set
// Normally set via the workspace, but we might not have one here if loading a single loose file
if (file !== Identifiers.EmptyFileName) {
await notebook.setLaunchingFile(file);
}
return result;
}

private async submitCodeImpl(
code: string,
fileUri: Uri,
line: number,
isDebug: boolean
) {
await this.updateOwners(fileUri);
const notebookCell = await this.addNotebookCell(code, fileUri, line);
const notebook = this.kernel?.notebook;
if (!notebook) {
return false;
}
const file = fileUri.fsPath;
let result = true;
try {
const finishedAddingCode = createDeferred<void>();

// Before we try to execute code make sure that we have an initial directory set
// Normally set via the workspace, but we might not have one here if loading a single loose file
if (file !== Identifiers.EmptyFileName) {
await notebook.setLaunchingFile(file);
}

if (isDebug) {
await this.jupyterDebugger.startDebugging(notebook);
}

// If the file isn't unknown, set the active kernel's __file__ variable to point to that same file.
await this.setFileInKernel(file, this.notebookDocument);
// If the file isn't unknown, set the active kernel's __file__ variable to point to that same file.
await this.setFileInKernel(file, this.notebookDocument);

const owningResource = this.owningResource;
const id = uuid();
const observable = this.kernel!.notebook!.executeObservable(code, file, line, id, false);
const temporaryExecution = this.notebookController!.controller.createNotebookCellExecution(notebookCell);
temporaryExecution?.start();

// Sign up for cell changes
observable.subscribe(
async (cells: ICell[]) => {
// Then send the combined output to the UI
const converted = (cells[0].data as nbformat.ICodeCell).outputs.map(cellOutputToVSCCellOutput);
await temporaryExecution.replaceOutput(converted);
const executionCount = (cells[0].data as nbformat.ICodeCell).execution_count;
if (executionCount) {
temporaryExecution.executionOrder = parseInt(executionCount.toString(), 10);
}

const owningResource = this.owningResource;
const id = uuid();
const observable = this.kernel!.notebook!.executeObservable(code, file, line, id, false);
const temporaryExecution = this.notebookController!.controller.createNotebookCellExecution(
notebookCell
);
temporaryExecution?.start();

// Sign up for cell changes
observable.subscribe(
async (cells: ICell[]) => {
// Then send the combined output to the UI
const converted = (cells[0].data as nbformat.ICodeCell).outputs.map(cellOutputToVSCCellOutput);
await temporaryExecution.replaceOutput(converted);

// Any errors will move our result to false (if allowed)
if (this.configuration.getSettings(owningResource).stopOnError) {
result = result && cells.find((c) => c.state === CellState.error) === undefined;
}
},
(error) => {
traceError(`Error executing a cell: `, error);
if (!(error instanceof CancellationError)) {
this.applicationShell.showErrorMessage(error.toString()).then(noop, noop);
}
},
() => {
temporaryExecution.end(result);
finishedAddingCode.resolve();
// Any errors will move our result to false (if allowed)
if (this.configuration.getSettings(owningResource).stopOnError) {
result = result && cells.find((c) => c.state === CellState.error) === undefined;
}
);
},
(error) => {
traceError(`Error executing a cell: `, error);
if (!(error instanceof CancellationError)) {
this.applicationShell.showErrorMessage(error.toString()).then(noop, noop);
}
},
() => {
temporaryExecution.end(result);
finishedAddingCode.resolve();
}
);

// Wait for the cell to finish
await finishedAddingCode.promise;
traceInfo(`Finished execution for ${id}`);
} finally {
await this.jupyterDebugger.stopDebugging(notebook);
}
// Wait for the cell to finish
await finishedAddingCode.promise;
traceInfo(`Finished execution for ${id}`);
} finally {
await this.jupyterDebugger.stopDebugging(notebook);
}

return result;
}

Expand Down Expand Up @@ -477,7 +481,7 @@ export class NativeInteractiveWindow implements IInteractiveWindowLoadable {
throw new Error('Method not implemented.');
}

public expandAllCells() {
public async expandAllCells() {
const edit = new WorkspaceEdit();
this.notebookDocument.getCells().forEach((cell, index) => {
const metadata = {
Expand All @@ -487,16 +491,16 @@ export class NativeInteractiveWindow implements IInteractiveWindowLoadable {
};
edit.replaceNotebookCellMetadata(this.notebookDocument.uri, index, metadata);
});
return workspace.applyEdit(edit);
await workspace.applyEdit(edit);
}

public collapseAllCells() {
public async collapseAllCells() {
const edit = new WorkspaceEdit();
this.notebookDocument.getCells().forEach((cell, index) => {
const metadata = { ...(cell.metadata || {}), inputCollapsed: true, outputCollapsed: false };
edit.replaceNotebookCellMetadata(this.notebookDocument.uri, index, metadata);
});
return workspace.applyEdit(edit);
await workspace.applyEdit(edit);
}

public scrollToCell(_id: string): void {
Expand Down Expand Up @@ -594,7 +598,7 @@ export class NativeInteractiveWindow implements IInteractiveWindowLoadable {
await this.show();
}

private async addNotebookCell(code: string): Promise<NotebookCell> {
private async addNotebookCell(code: string, file: Uri, line: number): Promise<NotebookCell> {
// Ensure we have a controller to execute code against
if (!this.notebookController) {
await this.commandManager.executeCommand('notebook.selectKernel');
Expand Down Expand Up @@ -629,7 +633,13 @@ export class NativeInteractiveWindow implements IInteractiveWindowLoadable {
strippedCode,
isMarkdown ? MARKDOWN_LANGUAGE : language
);
notebookCellData.metadata = { interactiveWindowCellMarker };
notebookCellData.metadata = {
interactiveWindowCellMarker,
interactive: {
file: file.fsPath,
line: line
}
};
edit.replaceNotebookCells(
this.notebookDocument.uri,
new NotebookRange(this.notebookDocument.cellCount, this.notebookDocument.cellCount),
Expand Down
28 changes: 20 additions & 8 deletions src/client/datascience/interactive-window/nativeInteractiveWindowCommandListener.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,18 @@ export class NativeInteractiveWindowCommandListener {
)
);
this.disposableRegistry.push(
commandManager.registerCommand(Commands.ExpandAllCells, () => this.expandAllCells())
commandManager.registerCommand(
Commands.ExpandAllCells,
(context?: { notebookEditor: { notebookUri: Uri } }) =>
this.expandAllCells(context?.notebookEditor.notebookUri)
)
);
this.disposableRegistry.push(
commandManager.registerCommand(Commands.CollapseAllCells, () => this.collapseAllCells())
commandManager.registerCommand(
Commands.CollapseAllCells,
(context?: { notebookEditor: { notebookUri: Uri } }) =>
this.collapseAllCells(context?.notebookEditor.notebookUri)
)
);
this.disposableRegistry.push(
commandManager.registerCommand(Commands.ExportOutputAsNotebook, () => this.exportCells())
Expand Down Expand Up @@ -422,15 +430,19 @@ export class NativeInteractiveWindowCommandListener {
}
}

private expandAllCells() {
const interactiveWindow = this.interactiveWindowProvider.activeWindow;
private expandAllCells(uri?: Uri) {
const interactiveWindow = uri
? this.interactiveWindowProvider.windows.find((window) => window.notebookUri!.toString() === uri.toString())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the code it looks like notebookUri is always defined, but the "!" feels a tiny bit less typesafe here since it is optional on the interface. Would ? accomplish the same thing since undefined won't equal uri.toString()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, looks like you have ? below for exportAs (was just reading on more). Can we use that in these locations as well.

: this.interactiveWindowProvider.activeWindow;
if (interactiveWindow) {
interactiveWindow.expandAllCells();
}
}

private collapseAllCells() {
const interactiveWindow = this.interactiveWindowProvider.activeWindow;
private collapseAllCells(uri?: Uri) {
const interactiveWindow = uri
? this.interactiveWindowProvider.windows.find((window) => window.notebookUri!.toString() === uri.toString())
: this.interactiveWindowProvider.activeWindow;
if (interactiveWindow) {
interactiveWindow.collapseAllCells();
}
Expand All @@ -445,7 +457,7 @@ export class NativeInteractiveWindowCommandListener {

private exportAs(uri?: Uri) {
const interactiveWindow = uri
? this.interactiveWindowProvider.windows.find((window) => window.notebookUri!.toString() === uri.toString())
? this.interactiveWindowProvider.windows.find((window) => window.notebookUri?.toString() === uri.toString())
: this.interactiveWindowProvider.activeWindow;
if (interactiveWindow) {
interactiveWindow.exportAs();
Expand All @@ -454,7 +466,7 @@ export class NativeInteractiveWindowCommandListener {

private export(uri?: Uri) {
const interactiveWindow = uri
? this.interactiveWindowProvider.windows.find((window) => window.notebookUri!.toString() === uri.toString())
? this.interactiveWindowProvider.windows.find((window) => window.notebookUri?.toString() === uri.toString())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this 'find matching interactive window' could be on the intearctiveWindowProvider? And then everyone of these functions would use that code instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we're still using the IInteractiveWindowProvider interface which is shared for webview, so I'd have to put that method on the old provider too (and it doesn't make sense there). I tracked this item in a list of todos for webview code cleanup #6488 feel free to add more there as well :)

: this.interactiveWindowProvider.activeWindow;
if (interactiveWindow) {
interactiveWindow.export();
Expand Down