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 #10733. Support interactive window restore on window reload #10785

Merged
merged 4 commits into from
Jul 14, 2022
+185 −71
Merged

Fix #10733. Support interactive window restore on window reload #10785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e98f5d7
Fix #10733. Support interactive window restore on window reload
rebornix Jul 12, 2022
54a0e44
Use string instead of UriComponents
rebornix Jul 12, 2022
b8f5974
separate start and restore.
rebornix Jul 13, 2022
0299a7e
fix creation notebook editor assignment
rebornix Jul 13, 2022
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
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2088,7 +2088,8 @@
"notebookCellExecutionState",
"portsAttributes",
"quickPickSortByLabel",
"notebookKernelSource"
"notebookKernelSource",
"interactiveWindow"
],
"scripts": {
"package": "gulp clean && gulp prePublishBundle && vsce package -o ms-toolsai-jupyter-insiders.vsix",
Expand Down
12 changes: 12 additions & 0 deletions src/interactive-window/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
import { NotebookCell } from 'vscode';
import { IJupyterSettings } from '../platform/common/types';
import { appendLineFeed, removeLinesFromFrontAndBackNoConcat } from '../platform/common/utils';
import { isUri } from '../platform/common/utils/misc';
import { uncommentMagicCommands } from './editor-integration/cellFactory';
import { CellMatcher } from './editor-integration/cellMatcher';
import { InteractiveCellMetadata } from './editor-integration/types';
import { InteractiveTab } from './types';

export function getInteractiveCellMetadata(cell: NotebookCell): InteractiveCellMetadata | undefined {
if (cell.metadata.interactive !== undefined) {
Expand Down Expand Up @@ -35,3 +37,13 @@ export function generateInteractiveCode(code: string, settings: IJupyterSettings

return withMagicsAndLinefeeds.join('');
}

export function isInteractiveInputTab(tab: unknown): tab is InteractiveTab {
let interactiveTab = tab as InteractiveTab;
return (
interactiveTab &&
interactiveTab.input &&
isUri(interactiveTab.input.uri) &&
isUri(interactiveTab.input.inputBoxUri)
);
}
122 changes: 87 additions & 35 deletions src/interactive-window/interactiveWindow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,14 @@ import {
NotebookController,
NotebookEdit
} from 'vscode';
import {
IApplicationShell,
ICommandManager,
IDocumentManager,
IWorkspaceService
} from '../platform/common/application/types';
import { ICommandManager, IDocumentManager, IWorkspaceService } from '../platform/common/application/types';
import { Commands, defaultNotebookFormat, MARKDOWN_LANGUAGE, PYTHON_LANGUAGE } from '../platform/common/constants';
import '../platform/common/extensions';
import { traceError, traceInfoIfCI } from '../platform/logging';
import { IFileSystem } from '../platform/common/platform/types';
import * as uuid from 'uuid/v4';

import { IConfigurationService, InteractiveWindowMode, Resource } from '../platform/common/types';
import { IConfigurationService, InteractiveWindowMode, IsWebExtension, Resource } from '../platform/common/types';
import { noop } from '../platform/common/utils/misc';
import {
IKernel,
Expand All @@ -53,8 +48,13 @@ import { INotebookExporter } from '../kernels/jupyter/types';
import { IExportDialog, ExportFormat } from '../notebooks/export/types';
import { generateCellsFromNotebookDocument } from './editor-integration/cellFactory';
import { CellMatcher } from './editor-integration/cellMatcher';
import { IInteractiveWindowLoadable, IInteractiveWindowDebugger, IInteractiveWindowDebuggingManager } from './types';
import { generateInteractiveCode } from './helpers';
import {
IInteractiveWindowLoadable,
IInteractiveWindowDebugger,
IInteractiveWindowDebuggingManager,
InteractiveTab
} from './types';
import { generateInteractiveCode, isInteractiveInputTab } from './helpers';
import { IControllerSelection, IVSCodeNotebookController } from '../notebooks/controllers/types';
import { DisplayOptions } from '../kernels/displayOptions';
import { getInteractiveCellMetadata } from './helpers';
Expand Down Expand Up @@ -85,9 +85,6 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
public get submitters(): Uri[] {
return this._submitters;
}
public get notebookUri(): Uri {
return this.notebookEditor.notebook.uri;
}
public get notebookDocument(): NotebookDocument {
return this.notebookEditor.notebook;
}
Expand All @@ -98,7 +95,7 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
private closedEvent = new EventEmitter<void>();
private _submitters: Uri[] = [];
private fileInKernel: Uri | undefined;
private cellMatcher;
private cellMatcher: CellMatcher;

private internalDisposables: Disposable[] = [];
private kernelDisposables: Disposable[] = [];
Expand All @@ -110,33 +107,69 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
} = {};
private pendingNotebookScrolls: NotebookRange[] = [];

private _notebookEditor!: NotebookEditor;
public get notebookEditor(): NotebookEditor {
return this._notebookEditor;
}

private _notebookUri: Uri;
public get notebookUri(): Uri {
return this._notebookUri;
}

private readonly documentManager: IDocumentManager;
private readonly fs: IFileSystem;
private readonly configuration: IConfigurationService;
private readonly jupyterExporter: INotebookExporter;
private readonly workspaceService: IWorkspaceService;
private readonly exportDialog: IExportDialog;
private readonly notebookControllerSelection: IControllerSelection;
private readonly interactiveWindowDebugger: IInteractiveWindowDebugger | undefined;
private readonly errorHandler: IDataScienceErrorHandler;
private readonly codeGeneratorFactory: ICodeGeneratorFactory;
private readonly storageFactory: IGeneratedCodeStorageFactory;
private readonly debuggingManager: IInteractiveWindowDebuggingManager;
private readonly isWebExtension: boolean;
private readonly commandManager: ICommandManager;
constructor(
private readonly documentManager: IDocumentManager,
private readonly fs: IFileSystem,
private readonly configuration: IConfigurationService,
private readonly commandManager: ICommandManager,
private readonly jupyterExporter: INotebookExporter,
private readonly workspaceService: IWorkspaceService,
private _owner: Resource,
private mode: InteractiveWindowMode,
private readonly exportDialog: IExportDialog,
private readonly notebookControllerSelection: IControllerSelection,
private readonly serviceContainer: IServiceContainer,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of passing the serviceContainer to constructors since it makes the class more coupled with the specific IoC container, but I suppose it's ok since it was already being passed in anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, I'm not a fan either. My motivation is avoiding doing serviceContainer.get<> in multiple places where we call InteractiveWindow#ctor.

@DonJayamanne pointed out that it's explicit to us how many services external InteractiveWindow import but moving serviceContainer.get<> into ctor kinds of hiding it. We might want to revisit this along with other challenges I ran into (will leave a comment about this soon)

private readonly interactiveWindowDebugger: IInteractiveWindowDebugger | undefined,
private readonly errorHandler: IDataScienceErrorHandler,
preferredController: IVSCodeNotebookController | undefined,
public readonly notebookEditor: NotebookEditor,
public readonly inputUri: Uri,
public readonly appShell: IApplicationShell,
private readonly codeGeneratorFactory: ICodeGeneratorFactory,
private readonly storageFactory: IGeneratedCodeStorageFactory,
private readonly debuggingManager: IInteractiveWindowDebuggingManager,
private readonly isWebExtension: boolean
private _owner: Resource,
public mode: InteractiveWindowMode,
private preferredController: IVSCodeNotebookController | undefined,
private readonly notebookEditorOrTab: NotebookEditor | InteractiveTab,
public readonly inputUri: Uri
) {
this.documentManager = this.serviceContainer.get<IDocumentManager>(IDocumentManager);
rchiodo marked this conversation as resolved.
Show resolved Hide resolved
this.commandManager = this.serviceContainer.get<ICommandManager>(ICommandManager);
this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
this.configuration = this.serviceContainer.get<IConfigurationService>(IConfigurationService);
this.jupyterExporter = this.serviceContainer.get<INotebookExporter>(INotebookExporter);
this.workspaceService = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
this.exportDialog = this.serviceContainer.get<IExportDialog>(IExportDialog);
this.notebookControllerSelection = this.serviceContainer.get<IControllerSelection>(IControllerSelection);
this.interactiveWindowDebugger =
this.serviceContainer.tryGet<IInteractiveWindowDebugger>(IInteractiveWindowDebugger);
this.errorHandler = this.serviceContainer.get<IDataScienceErrorHandler>(IDataScienceErrorHandler);
this.codeGeneratorFactory = this.serviceContainer.get<ICodeGeneratorFactory>(ICodeGeneratorFactory);
this.storageFactory = this.serviceContainer.get<IGeneratedCodeStorageFactory>(IGeneratedCodeStorageFactory);
this.debuggingManager = this.serviceContainer.get<IInteractiveWindowDebuggingManager>(
IInteractiveWindowDebuggingManager
);
this.isWebExtension = this.serviceContainer.get<boolean>(IsWebExtension);
this._notebookUri = isInteractiveInputTab(notebookEditorOrTab)
? notebookEditorOrTab.input.uri
: notebookEditorOrTab.notebook.uri;
if (!isInteractiveInputTab(notebookEditorOrTab)) {
this._notebookEditor = notebookEditorOrTab;
}

// Set our owner and first submitter
if (this._owner) {
this._submitters.push(this._owner);
}
}

public start() {
window.onDidChangeActiveNotebookEditor((e) => {
if (e === this.notebookEditor) {
this._onDidChangeViewState.fire();
Expand All @@ -154,14 +187,33 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
}

this.listenForControllerSelection();
if (preferredController) {
if (this.preferredController) {
// Also start connecting to our kernel but don't wait for it to finish
this.startKernel(preferredController.controller, preferredController.connection).ignoreErrors();
this.startKernel(this.preferredController.controller, this.preferredController.connection).ignoreErrors();
} else if (this.isWebExtension) {
this.insertInfoMessage(DataScience.noKernelsSpecifyRemote()).ignoreErrors();
}
}

public async restore(preferredController: IVSCodeNotebookController | undefined) {
if (preferredController) {
this.preferredController = preferredController;
}
if (!this.notebookEditor) {
if (isInteractiveInputTab(this.notebookEditorOrTab)) {
const document = await workspace.openNotebookDocument(this.notebookEditorOrTab.input.uri);
const editor = await window.showNotebookDocument(document, {
viewColumn: this.notebookEditorOrTab.group.viewColumn
});
this._notebookEditor = editor;
} else {
this._notebookEditor = this.notebookEditorOrTab;
}
}

this.start();
}

private async startKernel(
controller: NotebookController | undefined = this.currentKernelInfo.controller,
metadata: KernelConnectionMetadata | undefined = this.currentKernelInfo.metadata
Expand Down
Loading