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

Minor refactoring #7837

Merged
merged 3 commits into from
Oct 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 10 additions & 14 deletions src/client/datascience/commands/activeEditorContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ import { IDisposable, IDisposableRegistry } from '../../common/types';
import { isNotebookCell } from '../../common/utils/misc';
import { EditorContexts } from '../constants';
import { getActiveInteractiveWindow } from '../interactive-window/helpers';
import { IKernelProvider } from '../jupyter/kernels/types';
import { IKernel, IKernelProvider } from '../jupyter/kernels/types';
import { InteractiveWindowView, JupyterNotebookView } from '../notebook/constants';
import { getNotebookMetadata, isPythonNotebook } from '../notebook/helpers/helpers';
import { IInteractiveWindow, IInteractiveWindowProvider, INotebook, INotebookProvider } from '../types';
import { IInteractiveWindow, IInteractiveWindowProvider } from '../types';

@injectable()
export class ActiveEditorContextService implements IExtensionSingleActivationService, IDisposable {
Expand All @@ -41,7 +41,6 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
@inject(IDocumentManager) private readonly docManager: IDocumentManager,
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(IDisposableRegistry) disposables: IDisposableRegistry,
@inject(INotebookProvider) private readonly notebookProvider: INotebookProvider,
@inject(IVSCodeNotebook) private readonly vscNotebook: IVSCodeNotebook,
@inject(IKernelProvider) private readonly kernelProvider: IKernelProvider
) {
Expand Down Expand Up @@ -89,7 +88,7 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
}
public async activate(): Promise<void> {
this.docManager.onDidChangeActiveTextEditor(this.onDidChangeActiveTextEditor, this, this.disposables);
this.notebookProvider.onSessionStatusChanged(this.onDidKernelStatusChange, this, this.disposables);
this.kernelProvider.onKernelStatusChanged(this.onDidKernelStatusChange, this, this.disposables);
this.interactiveProvider.onDidChangeActiveInteractiveWindow(
this.onDidChangeActiveInteractiveWindow,
this,
Expand Down Expand Up @@ -185,17 +184,14 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
this.canInterruptInteractiveWindowKernelContext.set(false).ignoreErrors();
}
}
private onDidKernelStatusChange({ notebook }: { status: ServerStatus; notebook: INotebook }) {
// Ok, kernel status has changed.
const activeEditor = this.vscNotebook.activeNotebookEditor;
const activeInteractiveWindow = getActiveInteractiveWindow(this.interactiveProvider);
if (
activeInteractiveWindow &&
activeInteractiveWindow.notebookUri?.toString() === notebook.identity.toString()
) {
private onDidKernelStatusChange({ kernel }: { status: ServerStatus; kernel: IKernel }) {
if (kernel.notebookDocument.notebookType === InteractiveWindowView) {
this.updateContextOfActiveInteractiveWindowKernel();
} else if (activeEditor && activeEditor.document.uri.toString() === notebook.identity.toString()) {
this.updateContextOfActiveNotebookKernel(activeEditor);
} else if (
kernel.notebookDocument.notebookType === JupyterNotebookView &&
kernel.notebookDocument === this.vscNotebook.activeNotebookEditor?.document
) {
this.updateContextOfActiveNotebookKernel(this.vscNotebook.activeNotebookEditor);
}
}
private onDidChangeActiveTextEditor(e?: TextEditor) {
Expand Down
17 changes: 13 additions & 4 deletions src/client/datascience/data-viewing/dataViewer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,13 @@ import { StopWatch } from '../../common/utils/stopWatch';
import { sendTelemetryEvent } from '../../telemetry';
import { HelpLinks, Telemetry } from '../constants';
import { JupyterDataRateLimitError } from '../jupyter/jupyterDataRateLimitError';
import { ICodeCssGenerator, INotebook, IThemeFinder, WebViewViewChangeEventArgs } from '../types';
import {
ICodeCssGenerator,
IJupyterVariableDataProvider,
INotebook,
IThemeFinder,
WebViewViewChangeEventArgs
} from '../types';
import { WebviewPanelHost } from '../webviews/webviewPanelHost';
import { DataViewerMessageListener } from './dataViewerMessageListener';
import {
Expand All @@ -36,7 +42,7 @@ const PREFERRED_VIEWGROUP = 'JupyterDataViewerPreferredViewColumn';
const dataExplorerDir = path.join(EXTENSION_ROOT_DIR, 'out', 'datascience-ui', 'viewers');
@injectable()
export class DataViewer extends WebviewPanelHost<IDataViewerMapping> implements IDataViewer, IDisposable {
private dataProvider: IDataViewerDataProvider | undefined;
private dataProvider: IDataViewerDataProvider | IJupyterVariableDataProvider | undefined;
private rowsTimer: StopWatch | undefined;
private pendingRowsCount: number = 0;
private dataFrameInfoPromise: Promise<IDataFrameInfo> | undefined;
Expand Down Expand Up @@ -86,7 +92,10 @@ export class DataViewer extends WebviewPanelHost<IDataViewerMapping> implements
this.onDidDispose(this.dataViewerDisposed, this);
}

public async showData(dataProvider: IDataViewerDataProvider, title: string): Promise<void> {
public async showData(
dataProvider: IDataViewerDataProvider | IJupyterVariableDataProvider,
title: string
): Promise<void> {
if (!this.isDisposed) {
// Save the data provider
this.dataProvider = dataProvider;
Expand Down Expand Up @@ -116,7 +125,7 @@ export class DataViewer extends WebviewPanelHost<IDataViewerMapping> implements
public get notebook(): INotebook | undefined {
if (this.dataProvider && 'notebook' in this.dataProvider) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return (this.dataProvider as any).notebook;
return this.dataProvider.notebook;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the type of dataProvider the benefit is strong typeing yes, but this way when we look at removing INotebook we won't miss such instances (i.e. this change was for future proofing changes related to INotebook)

}
}

Expand Down
11 changes: 7 additions & 4 deletions src/client/datascience/interactive-window/interactiveWindow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,14 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
return this._identity;
}
public get notebookUri(): Uri | undefined {
return this.notebookDocument?.uri;
return this._notebookDocument?.uri;
}
public get notebookEditor(): NotebookEditor | undefined {
return this._notebookEditor;
}
public get notebookDocument(): NotebookDocument | undefined {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exposed a property notebookDocument, we have a lot of notebookUri.toString() comparisons of strings, instead of just comparing the object refs.

return this._notebookDocument;
}
private _onDidChangeViewState = new EventEmitter<void>();
private closedEvent: EventEmitter<IInteractiveWindow> = new EventEmitter<IInteractiveWindow>();
private _owner: Uri | undefined;
Expand All @@ -124,7 +127,7 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
private _editorReadyPromise: Promise<NotebookEditor>;
private _controllerReadyPromise: Deferred<VSCodeNotebookController>;
private _kernelReadyPromise: Promise<IKernel> | undefined;
private notebookDocument: NotebookDocument | undefined;
private _notebookDocument: NotebookDocument | undefined;
private executionPromise: Promise<boolean> | undefined;
private _notebookEditor: NotebookEditor | undefined;

Expand Down Expand Up @@ -162,7 +165,7 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
this._kernelReadyPromise = this.createKernelReadyPromise();

workspace.onDidCloseNotebookDocument((notebookDocument) => {
if (notebookDocument === this.notebookDocument) {
if (notebookDocument === this._notebookDocument) {
this.closedEvent.fire(this);
}
});
Expand Down Expand Up @@ -205,7 +208,7 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
throw new Error('Failed to request creation of interactive window from VS Code.');
}
this._notebookEditor = notebookEditor;
this.notebookDocument = notebookEditor.document;
this._notebookDocument = notebookEditor.document;
this.internalDisposables.push(
window.onDidChangeActiveNotebookEditor((e) => {
if (e === this._notebookEditor) {
Expand Down
10 changes: 7 additions & 3 deletions src/client/datascience/jupyter/kernels/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import * as path from 'path';
import type { Kernel } from '@jupyterlab/services';
import { IJupyterKernelSpec, INotebook } from '../../types';
import { IJupyterKernelSpec } from '../../types';
import { JupyterKernelSpec } from './jupyterKernelSpec';
// eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-require-imports
const NamedRegexp = require('named-js-regexp') as typeof import('named-js-regexp');
Expand All @@ -16,6 +16,7 @@ import { IConfigurationService, IPathUtils, Resource } from '../../../common/typ
import { PythonEnvironment } from '../../../pythonEnvironments/info';
import {
DefaultKernelConnectionMetadata,
IKernel,
KernelConnectionMetadata,
KernelSpecConnectionMetadata,
LiveKernelConnectionMetadata,
Expand Down Expand Up @@ -738,11 +739,14 @@ export function findPreferredKernel(
}

export async function sendTelemetryForPythonKernelExecutable(
notebook: INotebook,
kernel: IKernel,
resource: Resource,
kernelConnection: KernelConnectionMetadata,
executionService: IPythonExecutionFactory
) {
if (!kernel.notebook) {
return;
}
if (!kernelConnection.interpreter || !isPythonKernelConnection(kernelConnection)) {
return;
}
Expand All @@ -751,7 +755,7 @@ export async function sendTelemetryForPythonKernelExecutable(
}
try {
traceInfoIf(isCI, 'Begin sendTelemetryForPythonKernelExecutable');
const outputs = await executeSilently(notebook.session, 'import sys\nprint(sys.executable)');
const outputs = await executeSilently(kernel.notebook.session, 'import sys\nprint(sys.executable)');
if (outputs.length === 0) {
return;
}
Expand Down
11 changes: 6 additions & 5 deletions src/client/datascience/jupyter/kernels/kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
// Licensed under the MIT License.

'use strict';

import type { KernelMessage } from '@jupyterlab/services';
import type { Kernel as JupyterKernel, KernelMessage } from '@jupyterlab/services';
import { Observable } from 'rxjs/Observable';
import { Subject } from 'rxjs/Subject';
import {
Expand Down Expand Up @@ -38,7 +37,6 @@ import { getNotebookMetadata } from '../../notebook/helpers/helpers';
import {
IDataScienceErrorHandler,
IJupyterServerUriStorage,
IJupyterSession,
INotebook,
INotebookEditorProvider,
INotebookProvider,
Expand Down Expand Up @@ -409,7 +407,7 @@ export class Kernel implements IKernel {

if (this.connection?.localLaunch && this.notebook) {
await sendTelemetryForPythonKernelExecutable(
this.notebook,
this,
this.resourceUri,
this.kernelConnectionMetadata,
this.pythonExecutionFactory
Expand Down Expand Up @@ -611,7 +609,10 @@ export class Kernel implements IKernel {
}
}

export async function executeSilently(session: IJupyterSession, code: string): Promise<nbformat.IOutput[]> {
export async function executeSilently(
session: Pick<JupyterKernel.IKernelConnection, 'requestExecute'>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer using our types, directly using Jupyter Lab API types.
Much easier to refactor later on (as we have fewer layers)

code: string
): Promise<nbformat.IOutput[]> {
// eslint-disable-next-line @typescript-eslint/no-require-imports
const jupyterLab = require('@jupyterlab/services') as typeof import('@jupyterlab/services');

Expand Down
14 changes: 14 additions & 0 deletions src/client/datascience/jupyter/kernels/kernelProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import { inject, injectable } from 'inversify';
import { Event, EventEmitter, NotebookDocument } from 'vscode';
import { ServerStatus } from '../../../../datascience-ui/interactive-common/mainState';
import { IApplicationShell, IVSCodeNotebook, IWorkspaceService } from '../../../common/application/types';
import { traceInfo, traceWarning } from '../../../common/logger';
import { IFileSystem } from '../../../common/platform/types';
Expand All @@ -23,6 +24,7 @@ import { INotebookControllerManager } from '../../notebook/types';
import {
IDataScienceErrorHandler,
IJupyterServerUriStorage,
INotebook,
INotebookEditorProvider,
INotebookProvider
} from '../../types';
Expand All @@ -36,6 +38,8 @@ export class KernelProvider implements IKernelProvider {
private readonly pendingDisposables = new Set<IAsyncDisposable>();
private readonly _onDidRestartKernel = new EventEmitter<IKernel>();
private readonly _onDidDisposeKernel = new EventEmitter<IKernel>();
private readonly _onKernelStatusChanged = new EventEmitter<{ status: ServerStatus; kernel: IKernel }>();
public readonly onKernelStatusChanged = this._onKernelStatusChanged.event;
public get kernels() {
const kernels = new Set<IKernel>();
this.notebook.notebookDocuments.forEach((item) => {
Expand Down Expand Up @@ -64,6 +68,7 @@ export class KernelProvider implements IKernelProvider {
@inject(IServiceContainer) private readonly serviceContainer: IServiceContainer
) {
this.asyncDisposables.push(this);
this.notebookProvider.onSessionStatusChanged(this.onNotebookSessionChanged, this, this.disposables);
}

public get onDidDisposeKernel(): Event<IKernel> {
Expand All @@ -81,6 +86,9 @@ export class KernelProvider implements IKernelProvider {
const items = Array.from(this.pendingDisposables.values());
this.pendingDisposables.clear();
await Promise.all(items);
this._onDidDisposeKernel.dispose();
this._onDidRestartKernel.dispose();
this._onKernelStatusChanged.dispose();
}
public getOrCreate(notebook: NotebookDocument, options: KernelOptions): IKernel {
const existingKernelInfo = this.kernelsByNotebook.get(notebook);
Expand Down Expand Up @@ -151,4 +159,10 @@ export class KernelProvider implements IKernelProvider {
}
this.kernelsByNotebook.delete(notebook);
}
private onNotebookSessionChanged({ status, notebook }: { status: ServerStatus; notebook: INotebook }) {
const kernel = this.kernels.find((item) => item.notebook === notebook);
if (kernel) {
this._onKernelStatusChanged.fire({ status, kernel });
}
}
}
1 change: 1 addition & 0 deletions src/client/datascience/jupyter/kernels/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ export interface IKernelProvider extends IAsyncDisposable {
readonly kernels: Readonly<IKernel[]>;
onDidRestartKernel: Event<IKernel>;
onDidDisposeKernel: Event<IKernel>;
onKernelStatusChanged: Event<{ status: ServerStatus; kernel: IKernel }>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from INotebook to IKernel, easier & looking at NotebookWatcher, we were using Inotebook to detect changes in IKernel,
this change also makes it easier to remove INotbeook (fewer usages)

/**
* Get hold of the active kernel for a given Notebook.
*/
Expand Down
13 changes: 0 additions & 13 deletions src/client/datascience/notebookExtensibility.ts

This file was deleted.

2 changes: 2 additions & 0 deletions src/client/datascience/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
Disposable,
Event,
NotebookCell,
NotebookDocument,
NotebookEditor,
QuickPickItem,
Range,
Expand Down Expand Up @@ -469,6 +470,7 @@ export interface IInteractiveWindow extends IInteractiveBase {
readonly submitters: Uri[];
readonly identity: Uri;
readonly notebookUri?: Uri;
readonly notebookDocument?: NotebookDocument;
readonly readyPromise: Promise<void>;
closed: Event<IInteractiveWindow>;
addCode(code: string, file: Uri, line: number, editor?: TextEditor, runningStopWatch?: StopWatch): Promise<boolean>;
Expand Down
Loading