Skip to content

Remove notifyChangesToDocument as this is no longer necessary #14237

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

Merged
merged 2 commits into from
Oct 2, 2020
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
42 changes: 6 additions & 36 deletions src/client/datascience/jupyter/kernels/cellExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import {
} from '../../notebook/helpers/helpers';
import { MultiCancellationTokenSource } from '../../notebook/helpers/multiCancellationToken';
import { NotebookEditor } from '../../notebook/notebookEditor';
import { INotebookContentProvider } from '../../notebook/types';
import {
IDataScienceErrorHandler,
IJupyterSession,
Expand All @@ -41,7 +40,6 @@ const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed'

export class CellExecutionFactory {
constructor(
private readonly contentProvider: INotebookContentProvider,
private readonly errorHandler: IDataScienceErrorHandler,
private readonly editorProvider: INotebookEditorProvider,
private readonly appShell: IApplicationShell,
Expand All @@ -53,7 +51,6 @@ export class CellExecutionFactory {
return CellExecution.fromCell(
this.vscNotebook.notebookEditors.find((e) => e.document === cell.notebook)!,
cell,
this.contentProvider,
this.errorHandler,
this.editorProvider,
this.appShell
Expand Down Expand Up @@ -104,7 +101,6 @@ export class CellExecution {
private constructor(
public readonly editor: VSCNotebookEditor,
public readonly cell: NotebookCell,
private readonly contentProvider: INotebookContentProvider,
private readonly errorHandler: IDataScienceErrorHandler,
private readonly editorProvider: INotebookEditorProvider,
private readonly applicationService: IApplicationShell
Expand All @@ -116,12 +112,11 @@ export class CellExecution {
public static fromCell(
editor: VSCNotebookEditor,
cell: NotebookCell,
contentProvider: INotebookContentProvider,
errorHandler: IDataScienceErrorHandler,
editorProvider: INotebookEditorProvider,
appService: IApplicationShell
) {
return new CellExecution(editor, cell, contentProvider, errorHandler, editorProvider, appService);
return new CellExecution(editor, cell, errorHandler, editorProvider, appService);
}

public async start(kernelPromise: Promise<IKernel>, notebook: INotebook) {
Expand All @@ -136,8 +131,6 @@ export class CellExecution {
});
});
this.stopWatch.reset();
// Changes to metadata must be saved in ipynb, hence mark doc has dirty.
this.contentProvider.notifyChangesToDocument(this.cell.notebook);
this.notifyCellExecution();

// Begin the request that will modify our cell.
Expand Down Expand Up @@ -182,13 +175,10 @@ export class CellExecution {
})
);
await updateCellWithErrorStatus(this.editor, this.cell, error);
this.contentProvider.notifyChangesToDocument(this.cell.notebook);
this.errorHandler.handleError((error as unknown) as Error).ignoreErrors();

this._completed = true;
this._result.resolve(this.cell.metadata.runState);
// Changes to metadata must be saved in ipynb, hence mark doc has dirty.
this.contentProvider.notifyChangesToDocument(this.cell.notebook);
}

private async completedSuccessfully() {
Expand Down Expand Up @@ -222,8 +212,6 @@ export class CellExecution {

this._completed = true;
this._result.resolve(this.cell.metadata.runState);
// Changes to metadata must be saved in ipynb, hence mark doc has dirty.
this.contentProvider.notifyChangesToDocument(this.cell.notebook);
}

/**
Expand Down Expand Up @@ -258,8 +246,6 @@ export class CellExecution {
);
this._completed = true;
this._result.resolve(this.cell.metadata.runState);
// Changes to metadata must be saved in ipynb, hence mark doc has dirty.
this.contentProvider.notifyChangesToDocument(this.cell.notebook);
}

/**
Expand All @@ -273,7 +259,6 @@ export class CellExecution {
runState: vscodeNotebookEnums.NotebookCellRunState.Running
})
);
this.contentProvider.notifyChangesToDocument(this.cell.notebook);
}

private sendPerceivedCellExecute() {
Expand Down Expand Up @@ -371,8 +356,6 @@ export class CellExecution {
// tslint:disable-next-line:no-require-imports
const jupyterLab = require('@jupyterlab/services') as typeof import('@jupyterlab/services');

// Keep track of we need to send an update to VS code or not.
let shouldUpdate = true;
try {
if (jupyterLab.KernelMessage.isExecuteResultMsg(msg)) {
await this.handleExecuteResult(msg as KernelMessage.IExecuteResultMsg, clearState);
Expand All @@ -382,28 +365,23 @@ export class CellExecution {
// Status is handled by the result promise. While it is running we are active. Otherwise we're stopped.
// So ignore status messages.
const statusMsg = msg as KernelMessage.IStatusMsg;
shouldUpdate = false;
this.handleStatusMessage(statusMsg, clearState);
} else if (jupyterLab.KernelMessage.isStreamMsg(msg)) {
await this.handleStreamMessage(msg as KernelMessage.IStreamMsg, clearState);
} else if (jupyterLab.KernelMessage.isDisplayDataMsg(msg)) {
await this.handleDisplayData(msg as KernelMessage.IDisplayDataMsg, clearState);
} else if (jupyterLab.KernelMessage.isUpdateDisplayDataMsg(msg)) {
// No new data to update UI, hence do not send updates.
shouldUpdate = false;
} else if (jupyterLab.KernelMessage.isClearOutputMsg(msg)) {
await this.handleClearOutput(msg as KernelMessage.IClearOutputMsg, clearState);
} else if (jupyterLab.KernelMessage.isErrorMsg(msg)) {
await this.handleError(msg as KernelMessage.IErrorMsg, clearState);
} else if (jupyterLab.KernelMessage.isUpdateDisplayDataMsg(msg)) {
// Noop.
} else if (jupyterLab.KernelMessage.isCommOpenMsg(msg)) {
// No new data to update UI, hence do not send updates.
shouldUpdate = false;
// Noop.
} else if (jupyterLab.KernelMessage.isCommMsgMsg(msg)) {
// No new data to update UI, hence do not send updates.
shouldUpdate = false;
// Noop.
} else if (jupyterLab.KernelMessage.isCommCloseMsg(msg)) {
// No new data to update UI, hence do not send updates.
shouldUpdate = false;
// Noop.
} else {
traceWarning(`Unknown message ${msg.header.msg_type} : hasData=${'data' in msg.content}`);
}
Expand All @@ -412,11 +390,6 @@ export class CellExecution {
if ('execution_count' in msg.content && typeof msg.content.execution_count === 'number') {
updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count).then(noop, noop);
}

// Show our update if any new output.
if (shouldUpdate) {
this.contentProvider.notifyChangesToDocument(this.cell.notebook);
}
} catch (err) {
// If not a restart error, then tell the subscriber
this.completedWithErrors(err).then(noop, noop);
Expand Down Expand Up @@ -588,9 +561,6 @@ export class CellExecution {
if ('execution_count' in msg.content && typeof msg.content.execution_count === 'number') {
await updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count);
}

// Send this event.
this.contentProvider.notifyChangesToDocument(this.cell.notebook);
}
}
}
3 changes: 0 additions & 3 deletions src/client/datascience/jupyter/kernels/kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import { createDeferred, Deferred } from '../../../common/utils/async';
import { noop } from '../../../common/utils/misc';
import { IInterpreterService } from '../../../interpreter/contracts';
import { CodeSnippets } from '../../constants';
import { INotebookContentProvider } from '../../notebook/types';
import { getDefaultNotebookContent, updateNotebookMetadata } from '../../notebookStorage/baseModel';
import {
IDataScienceErrorHandler,
Expand Down Expand Up @@ -82,7 +81,6 @@ export class Kernel implements IKernel {
commandManager: ICommandManager,
interpreterService: IInterpreterService,
private readonly errorHandler: IDataScienceErrorHandler,
contentProvider: INotebookContentProvider,
editorProvider: INotebookEditorProvider,
private readonly kernelProvider: IKernelProvider,
private readonly kernelSelectionUsage: IKernelSelectionUsage,
Expand All @@ -94,7 +92,6 @@ export class Kernel implements IKernel {
commandManager,
interpreterService,
errorHandler,
contentProvider,
editorProvider,
kernelSelectionUsage,
appShell,
Expand Down
14 changes: 2 additions & 12 deletions src/client/datascience/jupyter/kernels/kernelExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import { captureTelemetry } from '../../../telemetry';
import { Commands, Telemetry, VSCodeNativeTelemetry } from '../../constants';
import { handleUpdateDisplayDataMessage } from '../../notebook/helpers/executionHelpers';
import { MultiCancellationTokenSource } from '../../notebook/helpers/multiCancellationToken';
import { INotebookContentProvider } from '../../notebook/types';
import { IDataScienceErrorHandler, INotebook, INotebookEditorProvider } from '../../types';
import { CellExecution, CellExecutionFactory } from './cellExecution';
import type { IKernel, IKernelProvider, IKernelSelectionUsage } from './types';
Expand All @@ -40,19 +39,12 @@ export class KernelExecution implements IDisposable {
private readonly commandManager: ICommandManager,
private readonly interpreterService: IInterpreterService,
errorHandler: IDataScienceErrorHandler,
private readonly contentProvider: INotebookContentProvider,
editorProvider: INotebookEditorProvider,
readonly kernelSelectionUsage: IKernelSelectionUsage,
readonly appShell: IApplicationShell,
readonly vscNotebook: IVSCodeNotebook
) {
this.executionFactory = new CellExecutionFactory(
this.contentProvider,
errorHandler,
editorProvider,
appShell,
vscNotebook
);
this.executionFactory = new CellExecutionFactory(errorHandler, editorProvider, appShell, vscNotebook);
}

@captureTelemetry(Telemetry.ExecuteNativeCell, undefined, true)
Expand Down Expand Up @@ -174,9 +166,7 @@ export class KernelExecution implements IDisposable {
const jupyterLab = require('@jupyterlab/services') as typeof import('@jupyterlab/services');
const editor = this.vscNotebook.notebookEditors.find((e) => e.document === document);
if (jupyterLab.KernelMessage.isUpdateDisplayDataMsg(msg) && editor) {
if (await handleUpdateDisplayDataMessage(msg, editor)) {
this.contentProvider.notifyChangesToDocument(document);
}
await handleUpdateDisplayDataMessage(msg, editor);
}
}

Expand Down
3 changes: 0 additions & 3 deletions src/client/datascience/jupyter/kernels/kernelProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../../co
import { traceInfo, traceWarning } from '../../../common/logger';
import { IAsyncDisposableRegistry, IConfigurationService, IDisposableRegistry } from '../../../common/types';
import { IInterpreterService } from '../../../interpreter/contracts';
import { INotebookContentProvider } from '../../notebook/types';
import { IDataScienceErrorHandler, INotebookEditorProvider, INotebookProvider } from '../../types';
import { Kernel } from './kernel';
import { KernelSelector } from './kernelSelector';
Expand All @@ -27,7 +26,6 @@ export class KernelProvider implements IKernelProvider {
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(IInterpreterService) private readonly interpreterService: IInterpreterService,
@inject(IDataScienceErrorHandler) private readonly errorHandler: IDataScienceErrorHandler,
@inject(INotebookContentProvider) private readonly contentProvider: INotebookContentProvider,
@inject(INotebookEditorProvider) private readonly editorProvider: INotebookEditorProvider,
@inject(KernelSelector) private readonly kernelSelectionUsage: IKernelSelectionUsage,
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
Expand All @@ -54,7 +52,6 @@ export class KernelProvider implements IKernelProvider {
this.commandManager,
this.interpreterService,
this.errorHandler,
this.contentProvider,
this.editorProvider,
this,
this.kernelSelectionUsage,
Expand Down
8 changes: 2 additions & 6 deletions src/client/datascience/notebook/contentProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { inject, injectable } from 'inversify';
import { CancellationToken, EventEmitter, Uri } from 'vscode';
import type {
NotebookCommunication,
NotebookContentProvider as VSCNotebookContentProvider,
NotebookData,
NotebookDocument,
NotebookDocumentBackup,
Expand All @@ -22,10 +23,8 @@ import { INotebookStorageProvider } from '../notebookStorage/notebookStorageProv
import { VSCodeNotebookModel } from '../notebookStorage/vscNotebookModel';
import { notebookModelToVSCNotebookData } from './helpers/helpers';
import { NotebookEditorCompatibilitySupport } from './notebookEditorCompatibilitySupport';
import { INotebookContentProvider } from './types';
// tslint:disable-next-line: no-var-requires no-require-imports
const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed');

/**
* This class is responsible for reading a notebook file (ipynb or other files) and returning VS Code with the NotebookData.
* Its up to extension authors to read the files and return it in a format that VSCode understands.
Expand All @@ -35,7 +34,7 @@ const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed'
* When saving, VSC will provide their model and we need to take that and merge it with an existing ipynb json (if any, to preserve metadata).
*/
@injectable()
export class NotebookContentProvider implements INotebookContentProvider {
export class NotebookContentProvider implements VSCNotebookContentProvider {
private notebookChanged = new EventEmitter<NotebookDocumentContentChangeEvent>();
public get onDidChangeNotebook() {
return this.notebookChanged.event;
Expand All @@ -45,9 +44,6 @@ export class NotebookContentProvider implements INotebookContentProvider {
@inject(NotebookEditorCompatibilitySupport)
private readonly compatibilitySupport: NotebookEditorCompatibilitySupport
) {}
public notifyChangesToDocument(_document: NotebookDocument) {
// this.notebookChanged.fire({ document });
}
public async resolveNotebook(_document: NotebookDocument, _webview: NotebookCommunication): Promise<void> {
// Later
}
Expand Down
19 changes: 6 additions & 13 deletions src/client/datascience/notebook/helpers/executionHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed'
export async function handleUpdateDisplayDataMessage(
msg: KernelMessage.IUpdateDisplayDataMsg,
editor: NotebookEditor
): Promise<boolean> {
): Promise<void> {
const document = editor.document;
let updated = false;
// Find any cells that have this same display_id
for (const cell of document.cells) {
if (cell.cellKind !== vscodeNotebookEnums.CellKind.Code) {
return false;
continue;
}
let updated = false;

const outputs = createIOutputFromCellOutputs(cell.outputs);
const changedOutputs = outputs.map((output) => {
Expand All @@ -51,15 +51,10 @@ export async function handleUpdateDisplayDataMessage(
}
});

if (!updated) {
continue;
if (updated) {
await updateCellOutput(editor, cell, changedOutputs);
}

await updateCellOutput(editor, cell, changedOutputs);
updated = true;
}

return updated;
}

/**
Expand Down Expand Up @@ -87,7 +82,7 @@ export async function updateCellExecutionCount(
editor: NotebookEditor,
cell: NotebookCell,
executionCount: number
): Promise<boolean> {
): Promise<void> {
if (cell.metadata.executionOrder !== executionCount && executionCount) {
const cellIndex = editor.document.cells.indexOf(cell);
await editor.edit((edit) =>
Expand All @@ -96,9 +91,7 @@ export async function updateCellExecutionCount(
executionOrder: executionCount
})
);
return true;
}
return false;
}

/**
Expand Down
3 changes: 2 additions & 1 deletion src/client/datascience/notebook/integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
'use strict';
import { inject, injectable } from 'inversify';
import { ConfigurationTarget } from 'vscode';
import { NotebookContentProvider as VSCNotebookContentProvider } from '../../../../types/vscode-proposed';
import { IExtensionSingleActivationService } from '../../activation/types';
import {
IApplicationEnvironment,
Expand Down Expand Up @@ -33,7 +34,7 @@ export class NotebookIntegration implements IExtensionSingleActivationService {
@inject(IVSCodeNotebook) private readonly vscNotebook: IVSCodeNotebook,
@inject(IExperimentsManager) private readonly experiment: IExperimentsManager,
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
@inject(INotebookContentProvider) private readonly notebookContentProvider: INotebookContentProvider,
@inject(INotebookContentProvider) private readonly notebookContentProvider: VSCNotebookContentProvider,
@inject(VSCodeKernelPickerProvider) private readonly kernelProvider: VSCodeKernelPickerProvider,
@inject(IApplicationEnvironment) private readonly env: IApplicationEnvironment,
@inject(IApplicationShell) private readonly shell: IApplicationShell,
Expand Down
3 changes: 2 additions & 1 deletion src/client/datascience/notebook/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

'use strict';

import { NotebookContentProvider as VSCNotebookContentProvider } from '../../../../types/vscode-proposed';
import { IExtensionSingleActivationService } from '../../activation/types';
import { IInterpreterStatusbarVisibilityFilter } from '../../interpreter/contracts';
import { IServiceManager } from '../../ioc/types';
Expand All @@ -17,7 +18,7 @@ import { NotebookSurveyBanner, NotebookSurveyDataLogger } from './survey';
import { INotebookContentProvider } from './types';

export function registerTypes(serviceManager: IServiceManager) {
serviceManager.addSingleton<INotebookContentProvider>(INotebookContentProvider, NotebookContentProvider);
serviceManager.addSingleton<VSCNotebookContentProvider>(INotebookContentProvider, NotebookContentProvider);
serviceManager.addSingleton<IExtensionSingleActivationService>(
IExtensionSingleActivationService,
NotebookIntegration
Expand Down
10 changes: 0 additions & 10 deletions src/client/datascience/notebook/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,4 @@

'use strict';

import type { NotebookContentProvider as VSCodeNotebookContentProvider, NotebookDocument } from 'vscode-proposed';

export const INotebookContentProvider = Symbol('INotebookContentProvider');
export interface INotebookContentProvider extends VSCodeNotebookContentProvider {
/**
* Notify VS Code that document has changed.
* The change is not something that can be undone by using the `undo`.
* E.g. updating execution count of a cell, or making a notebook readonly, or updating kernel info in ipynb metadata.
*/
notifyChangesToDocument(document: NotebookDocument): void;
}
Loading