From de0eca362bb04001d156f9deaedc68b8df07c81e Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 1 Sep 2020 14:02:20 -0700 Subject: [PATCH 01/24] Update cell output and metadata using Edit API --- .../jupyter/kernels/cellExecution.ts | 23 +-- .../datascience/jupyter/kernels/kernel.ts | 8 +- .../jupyter/kernels/kernelExecution.ts | 20 ++- .../jupyter/kernels/kernelProvider.ts | 8 +- .../notebook/helpers/executionHelpers.ts | 32 ++-- .../datascience/notebook/helpers/helpers.ts | 95 +++++++---- .../datascience/notebook/notebookEditor.ts | 16 +- .../notebookStorage/vscNotebookModel.ts | 8 +- .../notebook/cellOutput.ds.test.ts | 151 +----------------- 9 files changed, 142 insertions(+), 219 deletions(-) diff --git a/src/client/datascience/jupyter/kernels/cellExecution.ts b/src/client/datascience/jupyter/kernels/cellExecution.ts index a0be2d08c54e..bfe470bf4476 100644 --- a/src/client/datascience/jupyter/kernels/cellExecution.ts +++ b/src/client/datascience/jupyter/kernels/cellExecution.ts @@ -6,8 +6,9 @@ import { nbformat } from '@jupyterlab/coreutils'; import type { KernelMessage } from '@jupyterlab/services/lib/kernel/messages'; import { CancellationToken, CellOutputKind, CellStreamOutput, NotebookCell, NotebookCellRunState } from 'vscode'; +import type { NotebookEditor as VSCNotebookEditor } from '../../../../../types/vscode-proposed'; import { concatMultilineString, formatStreamText } from '../../../../datascience-ui/common'; -import { IApplicationShell } from '../../../common/application/types'; +import { IApplicationShell, IVSCodeNotebook } from '../../../common/application/types'; import { traceInfo, traceWarning } from '../../../common/logger'; import { RefBool } from '../../../common/refBool'; import { createDeferred } from '../../../common/utils/async'; @@ -43,12 +44,14 @@ export class CellExecutionFactory { private readonly contentProvider: INotebookContentProvider, private readonly errorHandler: IDataScienceErrorHandler, private readonly editorProvider: INotebookEditorProvider, - private readonly appShell: IApplicationShell + private readonly appShell: IApplicationShell, + private readonly vscNotebook: IVSCodeNotebook ) {} public create(cell: NotebookCell) { // tslint:disable-next-line: no-use-before-declare return CellExecution.fromCell( + this.vscNotebook.notebookEditors.find((e) => e.document === cell.notebook)!, cell, this.contentProvider, this.errorHandler, @@ -89,6 +92,7 @@ export class CellExecution { private _completed?: boolean; private constructor( + public readonly editor: VSCNotebookEditor, public readonly cell: NotebookCell, private readonly contentProvider: INotebookContentProvider, private readonly errorHandler: IDataScienceErrorHandler, @@ -100,19 +104,20 @@ export class CellExecution { } public static fromCell( + editor: VSCNotebookEditor, cell: NotebookCell, contentProvider: INotebookContentProvider, errorHandler: IDataScienceErrorHandler, editorProvider: INotebookEditorProvider, appService: IApplicationShell ) { - return new CellExecution(cell, contentProvider, errorHandler, editorProvider, appService); + return new CellExecution(editor, cell, contentProvider, errorHandler, editorProvider, appService); } public start(kernelPromise: Promise, notebook: INotebook) { this.started = true; // Ensure we clear the cell state and trigger a change. - clearCellForExecution(this.cell); + clearCellForExecution(this.editor, this.cell); this.cell.metadata.runStartTime = new Date().getTime(); this.stopWatch.reset(); // Changes to metadata must be saved in ipynb, hence mark doc has dirty. @@ -149,7 +154,7 @@ export class CellExecution { private completedWithErrors(error: Partial) { this.sendPerceivedCellExecute(); this.cell.metadata.lastRunDuration = this.stopWatch.elapsedTime; - updateCellWithErrorStatus(this.cell, error); + updateCellWithErrorStatus(this.editor, this.cell, error); this.contentProvider.notifyChangesToDocument(this.cell.notebook); this.errorHandler.handleError((error as unknown) as Error).ignoreErrors(); @@ -169,7 +174,7 @@ export class CellExecution { this.cell.metadata.statusMessage = ''; this.cell.metadata.lastRunDuration = this.stopWatch.elapsedTime; - updateCellExecutionTimes(this.cell, { + updateCellExecutionTimes(this.editor, this.cell, { startTime: this.cell.metadata.runStartTime, duration: this.cell.metadata.lastRunDuration }); @@ -353,7 +358,7 @@ export class CellExecution { // Set execution count, all messages should have it if ('execution_count' in msg.content && typeof msg.content.execution_count === 'number') { - if (updateCellExecutionCount(this.cell, msg.content.execution_count)) { + if (updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count)) { shouldUpdate = true; } } @@ -449,7 +454,7 @@ export class CellExecution { private handleExecuteInput(msg: KernelMessage.IExecuteInputMsg, _clearState: RefBool) { if (msg.content.execution_count) { - updateCellExecutionCount(this.cell, msg.content.execution_count); + updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count); } } @@ -530,7 +535,7 @@ export class CellExecution { // Set execution count, all messages should have it if ('execution_count' in msg.content && typeof msg.content.execution_count === 'number') { - updateCellExecutionCount(this.cell, msg.content.execution_count); + updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count); } // Send this event. diff --git a/src/client/datascience/jupyter/kernels/kernel.ts b/src/client/datascience/jupyter/kernels/kernel.ts index 53d82be3a28b..459b36f57ac9 100644 --- a/src/client/datascience/jupyter/kernels/kernel.ts +++ b/src/client/datascience/jupyter/kernels/kernel.ts @@ -17,7 +17,7 @@ import { Uri } from 'vscode'; import { ServerStatus } from '../../../../datascience-ui/interactive-common/mainState'; -import { IApplicationShell, ICommandManager } from '../../../common/application/types'; +import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../../common/application/types'; import { traceError } from '../../../common/logger'; import { IDisposableRegistry } from '../../../common/types'; import { createDeferred, Deferred } from '../../../common/utils/async'; @@ -86,7 +86,8 @@ export class Kernel implements IKernel { editorProvider: INotebookEditorProvider, private readonly kernelProvider: IKernelProvider, private readonly kernelSelectionUsage: IKernelSelectionUsage, - appShell: IApplicationShell + appShell: IApplicationShell, + vscNotebook: IVSCodeNotebook ) { this.kernelExecution = new KernelExecution( kernelProvider, @@ -96,7 +97,8 @@ export class Kernel implements IKernel { contentProvider, editorProvider, kernelSelectionUsage, - appShell + appShell, + vscNotebook ); } public async executeCell(cell: NotebookCell): Promise { diff --git a/src/client/datascience/jupyter/kernels/kernelExecution.ts b/src/client/datascience/jupyter/kernels/kernelExecution.ts index 7dbbad1fd46e..98e96105994b 100644 --- a/src/client/datascience/jupyter/kernels/kernelExecution.ts +++ b/src/client/datascience/jupyter/kernels/kernelExecution.ts @@ -5,7 +5,7 @@ import { KernelMessage } from '@jupyterlab/services'; import { NotebookCell, NotebookCellRunState, NotebookDocument } from 'vscode'; -import { IApplicationShell, ICommandManager } from '../../../common/application/types'; +import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../../common/application/types'; import { IDisposable } from '../../../common/types'; import { noop } from '../../../common/utils/misc'; import { IInterpreterService } from '../../../interpreter/contracts'; @@ -43,9 +43,16 @@ export class KernelExecution implements IDisposable { private readonly contentProvider: INotebookContentProvider, editorProvider: INotebookEditorProvider, readonly kernelSelectionUsage: IKernelSelectionUsage, - readonly appShell: IApplicationShell + readonly appShell: IApplicationShell, + readonly vscNotebook: IVSCodeNotebook ) { - this.executionFactory = new CellExecutionFactory(this.contentProvider, errorHandler, editorProvider, appShell); + this.executionFactory = new CellExecutionFactory( + this.contentProvider, + errorHandler, + editorProvider, + appShell, + vscNotebook + ); } @captureTelemetry(Telemetry.ExecuteNativeCell, undefined, true) @@ -163,8 +170,11 @@ export class KernelExecution implements IDisposable { private onIoPubMessage(document: NotebookDocument, msg: KernelMessage.IIOPubMessage) { // tslint:disable-next-line:no-require-imports const jupyterLab = require('@jupyterlab/services') as typeof import('@jupyterlab/services'); - if (jupyterLab.KernelMessage.isUpdateDisplayDataMsg(msg) && handleUpdateDisplayDataMessage(msg, document)) { - this.contentProvider.notifyChangesToDocument(document); + const editor = this.vscNotebook.notebookEditors.find((e) => e.document === document); + if (jupyterLab.KernelMessage.isUpdateDisplayDataMsg(msg) && editor) { + if (handleUpdateDisplayDataMessage(msg, editor)) { + this.contentProvider.notifyChangesToDocument(document); + } } } diff --git a/src/client/datascience/jupyter/kernels/kernelProvider.ts b/src/client/datascience/jupyter/kernels/kernelProvider.ts index 47b895dc3f6f..50475f2a1e58 100644 --- a/src/client/datascience/jupyter/kernels/kernelProvider.ts +++ b/src/client/datascience/jupyter/kernels/kernelProvider.ts @@ -6,7 +6,7 @@ import * as fastDeepEqual from 'fast-deep-equal'; import { inject, injectable } from 'inversify'; import { Uri } from 'vscode'; -import { IApplicationShell, ICommandManager } from '../../../common/application/types'; +import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../../common/application/types'; import { traceInfo, traceWarning } from '../../../common/logger'; import { IAsyncDisposableRegistry, IConfigurationService, IDisposableRegistry } from '../../../common/types'; import { IInterpreterService } from '../../../interpreter/contracts'; @@ -30,7 +30,8 @@ export class KernelProvider implements IKernelProvider { @inject(INotebookContentProvider) private readonly contentProvider: INotebookContentProvider, @inject(INotebookEditorProvider) private readonly editorProvider: INotebookEditorProvider, @inject(KernelSelector) private readonly kernelSelectionUsage: IKernelSelectionUsage, - @inject(IApplicationShell) private readonly appShell: IApplicationShell + @inject(IApplicationShell) private readonly appShell: IApplicationShell, + @inject(IVSCodeNotebook) private readonly vscNotebook: IVSCodeNotebook ) {} public get(uri: Uri): IKernel | undefined { return this.kernelsByUri.get(uri.toString())?.kernel; @@ -57,7 +58,8 @@ export class KernelProvider implements IKernelProvider { this.editorProvider, this, this.kernelSelectionUsage, - this.appShell + this.appShell, + this.vscNotebook ); this.asyncDisposables.push(kernel); this.kernelsByUri.set(uri.toString(), { options, kernel }); diff --git a/src/client/datascience/notebook/helpers/executionHelpers.ts b/src/client/datascience/notebook/helpers/executionHelpers.ts index 7372cc206ed6..a6066df9ed81 100644 --- a/src/client/datascience/notebook/helpers/executionHelpers.ts +++ b/src/client/datascience/notebook/helpers/executionHelpers.ts @@ -6,7 +6,7 @@ import type { nbformat } from '@jupyterlab/coreutils'; import type { KernelMessage } from '@jupyterlab/services'; import * as fastDeepEqual from 'fast-deep-equal'; -import { NotebookCell, NotebookCellRunState, NotebookDocument } from 'vscode'; +import type { NotebookCell, NotebookEditor } from '../../../../../types/vscode-proposed'; import { createErrorOutput } from '../../../../datascience-ui/common/cellFactory'; import { createIOutputFromCellOutputs, createVSCCellOutputsFromOutputs, translateErrorOutput } from './helpers'; // tslint:disable-next-line: no-var-requires no-require-imports @@ -20,8 +20,9 @@ const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed' */ export function handleUpdateDisplayDataMessage( msg: KernelMessage.IUpdateDisplayDataMsg, - document: NotebookDocument + editor: NotebookEditor ): boolean { + const document = editor.document; // Find any cells that have this same display_id return ( document.cells.filter((cellToCheck, index) => { @@ -56,7 +57,7 @@ export function handleUpdateDisplayDataMessage( } const vscCell = document.cells[index]; - updateCellOutput(vscCell, changedOutputs); + updateCellOutput(editor, vscCell, changedOutputs); return true; }).length > 0 ); @@ -65,17 +66,24 @@ export function handleUpdateDisplayDataMessage( /** * Updates the VSC cell with the error output. */ -export function updateCellWithErrorStatus(cell: NotebookCell, ex: Partial) { +export function updateCellWithErrorStatus(editor: NotebookEditor, cell: NotebookCell, ex: Partial) { + editor.edit((edit) => { + const cellIndex = editor.document.cells.indexOf(cell); + edit.replaceMetadata(cellIndex, { ...cell.metadata, runState: vscodeNotebookEnums.NotebookCellRunState.Error }); + edit.replaceOutput(cellIndex, [translateErrorOutput(createErrorOutput(ex))]); + }); cell.outputs = [translateErrorOutput(createErrorOutput(ex))]; - cell.metadata.runState = NotebookCellRunState.Error; } /** * @returns {boolean} Returns `true` if execution count has changed. */ -export function updateCellExecutionCount(vscCell: NotebookCell, executionCount: number): boolean { - if (vscCell.metadata.executionOrder !== executionCount && executionCount) { - vscCell.metadata.executionOrder = executionCount; +export function updateCellExecutionCount(editor: NotebookEditor, cell: NotebookCell, executionCount: number): boolean { + if (cell.metadata.executionOrder !== executionCount && executionCount) { + editor.edit((edit) => { + const cellIndex = editor.document.cells.indexOf(cell); + edit.replaceMetadata(cellIndex, { ...cell.metadata, executionOrder: executionCount }); + }); return true; } return false; @@ -87,7 +95,11 @@ export function updateCellExecutionCount(vscCell: NotebookCell, executionCount: * Here we update both the VSCode Cell as well as our ICell (cell in our INotebookModel). * @returns {(boolean | undefined)} Returns `true` if output has changed. */ -export function updateCellOutput(vscCell: NotebookCell, outputs: nbformat.IOutput[]): boolean | undefined { +export function updateCellOutput( + editor: NotebookEditor, + vscCell: NotebookCell, + outputs: nbformat.IOutput[] +): boolean | undefined { const newOutput = createVSCCellOutputsFromOutputs(outputs); // If there was no output and still no output, then nothing to do. if (vscCell.outputs.length === 0 && newOutput.length === 0) { @@ -98,6 +110,6 @@ export function updateCellOutput(vscCell: NotebookCell, outputs: nbformat.IOutpu if (vscCell.outputs.length === newOutput.length && fastDeepEqual(vscCell.outputs, newOutput)) { return; } - vscCell.outputs = newOutput; + editor.edit((edit) => edit.replaceOutput(0, newOutput)); return true; } diff --git a/src/client/datascience/notebook/helpers/helpers.ts b/src/client/datascience/notebook/helpers/helpers.ts index 6e444f6b9290..d6f8b61f994e 100644 --- a/src/client/datascience/notebook/helpers/helpers.ts +++ b/src/client/datascience/notebook/helpers/helpers.ts @@ -13,7 +13,8 @@ import type { NotebookCellData, NotebookCellMetadata, NotebookData, - NotebookDocument + NotebookDocument, + NotebookEditor } from 'vscode-proposed'; import { NotebookCellRunState } from '../../../../../typings/vscode-proposed'; import { concatMultilineString, splitMultilineString } from '../../../../datascience-ui/common'; @@ -338,38 +339,59 @@ export function createIOutputFromCellOutputs(cellOutputs: CellOutput[]): nbforma .map((output) => output!!); } -export function clearCellForExecution(cell: NotebookCell) { - cell.metadata.statusMessage = undefined; - cell.metadata.executionOrder = undefined; - cell.metadata.lastRunDuration = undefined; - cell.metadata.runStartTime = undefined; - cell.outputs = []; +export function clearCellForExecution(editor: NotebookEditor, cell: NotebookCell) { + editor.edit((edit) => { + const cellIndex = editor.document.cells.indexOf(cell); + edit.replaceMetadata(cellIndex, { + ...cell.metadata, + statusMessage: undefined, + executionOrder: undefined, + lastRunDuration: undefined, + runStartTime: undefined + }); + edit.replaceOutput(cellIndex, []); + }); - updateCellExecutionTimes(cell); + updateCellExecutionTimes(editor, cell); } /** * Store execution start and end times. * Stored as ISO for portability. */ -export function updateCellExecutionTimes(cell: NotebookCell, times?: { startTime?: number; duration?: number }) { - if (!times || !times.duration || !times.startTime) { - if (cell.metadata.custom?.metadata?.vscode?.start_execution_time) { - delete cell.metadata.custom.metadata.vscode.start_execution_time; - } - if (cell.metadata.custom?.metadata?.vscode?.end_execution_time) { - delete cell.metadata.custom.metadata.vscode.end_execution_time; +export function updateCellExecutionTimes( + editor: NotebookEditor, + cell: NotebookCell, + times?: { startTime?: number; duration?: number } +) { + editor.edit((edit) => { + const cellIndex = editor.document.cells.indexOf(cell); + if (!times || !times.duration || !times.startTime) { + const cellMetadata = cloneDeep(cell.metadata); + let updated = false; + if (cellMetadata.custom?.metadata?.vscode?.start_execution_time) { + delete cellMetadata.custom.metadata.vscode.start_execution_time; + updated = true; + } + if (cellMetadata.custom?.metadata?.vscode?.end_execution_time) { + delete cellMetadata.custom.metadata.vscode.end_execution_time; + updated = true; + } + if (updated) { + edit.replaceMetadata(cellIndex, { ...cellMetadata }); + } + return; } - return; - } - const startTimeISO = new Date(times.startTime).toISOString(); - const endTimeISO = new Date(times.startTime + times.duration).toISOString(); - cell.metadata.custom = cell.metadata.custom || {}; - cell.metadata.custom.metadata = cell.metadata.custom.metadata || {}; - cell.metadata.custom.metadata.vscode = cell.metadata.custom.metadata.vscode || {}; - cell.metadata.custom.metadata.vscode.end_execution_time = endTimeISO; - cell.metadata.custom.metadata.vscode.start_execution_time = startTimeISO; + const startTimeISO = new Date(times.startTime).toISOString(); + const endTimeISO = new Date(times.startTime + times.duration).toISOString(); + const customMetadata = cloneDeep(cell.metadata.custom || {}); + customMetadata.metadata = customMetadata.metadata || {}; + customMetadata.metadata.vscode = customMetadata.metadata.vscode || {}; + customMetadata.metadata.vscode.end_execution_time = endTimeISO; + customMetadata.metadata.vscode.start_execution_time = startTimeISO; + edit.replaceMetadata(cellIndex, { ...cell.metadata, custom: customMetadata }); + }); } function createCodeCellFromVSCNotebookCell(cell: NotebookCell): nbformat.ICodeCell { @@ -640,7 +662,11 @@ export function getCellStatusMessageBasedOnFirstCellErrorOutput(outputs?: CellOu /** * Updates a notebook document as a result of trusting it. */ -export function updateVSCNotebookAfterTrustingNotebook(document: NotebookDocument, originalCells: ICell[]) { +export function updateVSCNotebookAfterTrustingNotebook( + editor: NotebookEditor, + document: NotebookDocument, + originalCells: ICell[] +) { const areAllCellsEditableAndRunnable = document.cells.every((cell) => { if (cell.cellKind === vscodeNotebookEnums.CellKind.Markdown) { return cell.metadata.editable; @@ -664,13 +690,16 @@ export function updateVSCNotebookAfterTrustingNotebook(document: NotebookDocumen document.metadata.editable = true; document.metadata.runnable = true; - document.cells.forEach((cell, index) => { - cell.metadata.editable = true; - if (cell.cellKind !== vscodeNotebookEnums.CellKind.Markdown) { - cell.metadata.runnable = true; - // Restore the output once we trust the notebook. - // tslint:disable-next-line: no-any - cell.outputs = createVSCCellOutputsFromOutputs(originalCells[index].data.outputs as any); - } + editor.edit((edit) => { + document.cells.forEach((cell, index) => { + if (cell.cellKind === vscodeNotebookEnums.CellKind.Markdown) { + edit.replaceMetadata(index, { ...cell.metadata, editable: true }); + } else { + edit.replaceMetadata(index, { ...cell.metadata, editable: true, runnable: true }); + // Restore the output once we trust the notebook. + // tslint:disable-next-line: no-any + edit.replaceOutput(index, createVSCCellOutputsFromOutputs(originalCells[index].data.outputs as any)); + } + }); }); } diff --git a/src/client/datascience/notebook/notebookEditor.ts b/src/client/datascience/notebook/notebookEditor.ts index 9da8d917571c..cb65db236b6a 100644 --- a/src/client/datascience/notebook/notebookEditor.ts +++ b/src/client/datascience/notebook/notebookEditor.ts @@ -152,18 +152,22 @@ export class NotebookEditor implements INotebookEditor { if (!this.vscodeNotebook.activeNotebookEditor) { return; } - this.vscodeNotebook.activeNotebookEditor.document.cells.forEach((cell) => { - cell.metadata.inputCollapsed = false; - cell.metadata.outputCollapsed = false; + const cells = this.vscodeNotebook.activeNotebookEditor.document.cells; + this.vscodeNotebook.activeNotebookEditor.edit((edit) => { + cells.forEach((cell, index) => { + edit.replaceMetadata(index, { ...cell.metadata, inputCollapsed: false, outputCollapsed: false }); + }); }); } public collapseAllCells(): void { if (!this.vscodeNotebook.activeNotebookEditor) { return; } - this.vscodeNotebook.activeNotebookEditor.document.cells.forEach((cell) => { - cell.metadata.inputCollapsed = true; - cell.metadata.outputCollapsed = true; + const cells = this.vscodeNotebook.activeNotebookEditor.document.cells; + this.vscodeNotebook.activeNotebookEditor.edit((edit) => { + cells.forEach((cell, index) => { + edit.replaceMetadata(index, { ...cell.metadata, inputCollapsed: true, outputCollapsed: true }); + }); }); } public notifyExecution(cell: NotebookCell) { diff --git a/src/client/datascience/notebookStorage/vscNotebookModel.ts b/src/client/datascience/notebookStorage/vscNotebookModel.ts index 2d1ab7563406..405d4bbe7fd9 100644 --- a/src/client/datascience/notebookStorage/vscNotebookModel.ts +++ b/src/client/datascience/notebookStorage/vscNotebookModel.ts @@ -98,8 +98,12 @@ export class VSCodeNotebookModel extends BaseNotebookModel { } public trust() { super.trust(); - if (this.document) { - updateVSCNotebookAfterTrustingNotebook(this.document, this._cells); + const editor = + this.vscodeNotebook && this.document + ? this.vscodeNotebook.notebookEditors.find((e) => e.document === this.document) + : undefined; + if (this.document && editor) { + updateVSCNotebookAfterTrustingNotebook(editor, this.document, this._cells); // We don't need old cells. this._cells = []; } diff --git a/src/test/datascience/notebook/cellOutput.ds.test.ts b/src/test/datascience/notebook/cellOutput.ds.test.ts index e7a0c1dd0f63..6f081338859f 100644 --- a/src/test/datascience/notebook/cellOutput.ds.test.ts +++ b/src/test/datascience/notebook/cellOutput.ds.test.ts @@ -8,37 +8,20 @@ import { join } from 'path'; import { Subject } from 'rxjs/Subject'; import * as sinon from 'sinon'; import { anything, instance, mock, reset, when } from 'ts-mockito'; -import { commands, Uri } from 'vscode'; -import { IVSCodeNotebook } from '../../../client/common/application/types'; +import { Uri } from 'vscode'; import { IDisposable } from '../../../client/common/types'; -import { - CellState, - ICell, - INotebook, - INotebookEditorProvider, - INotebookProvider -} from '../../../client/datascience/types'; +import { ICell, INotebook, INotebookEditorProvider, INotebookProvider } from '../../../client/datascience/types'; import { IExtensionTestApi } from '../../common'; import { EXTENSION_ROOT_DIR_FOR_TESTS, initialize } from '../../initialize'; import { - assertHasExecutionCompletedSuccessfully, - assertHasExecutionCompletedWithErrors, - assertHasOutputInVSCell, canRunTests, closeNotebooksAndCleanUpAfterTests, createTemporaryNotebook, deleteAllCellsAndWait, - insertPythonCellAndWait, - trustAllNotebooks, - waitForExecutionCompletedSuccessfully, - waitForExecutionOrderInVSCCell, - waitForTextOutputInVSCode, - waitForVSCCellHasEmptyOutput, - waitForVSCCellIsRunning + trustAllNotebooks } from './helper'; // tslint:disable-next-line: no-var-requires no-require-imports -const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed'); // tslint:disable: no-any no-invalid-this suite('DataScience - VSCode Notebook - (fake execution) (Clearing Output)', function () { @@ -46,7 +29,6 @@ suite('DataScience - VSCode Notebook - (fake execution) (Clearing Output)', func let api: IExtensionTestApi; let editorProvider: INotebookEditorProvider; - let vscodeNotebook: IVSCodeNotebook; let notebookProvider: INotebookProvider; let nb: INotebook; let cellObservableResult: Subject; @@ -58,7 +40,6 @@ suite('DataScience - VSCode Notebook - (fake execution) (Clearing Output)', func return this.skip(); } await trustAllNotebooks(); - vscodeNotebook = api.serviceContainer.get(IVSCodeNotebook); notebookProvider = api.serviceContainer.get(INotebookProvider); editorProvider = api.serviceContainer.get(INotebookEditorProvider); }); @@ -79,29 +60,6 @@ suite('DataScience - VSCode Notebook - (fake execution) (Clearing Output)', func const testIPynb = Uri.file(await createTemporaryNotebook(templateIPynb, disposables2)); await editorProvider.open(testIPynb); }); - test('Clearing output when not executing', async function () { - // tslint:disable-next-line: no-unused-expression - return this.skip(); - const cells = vscodeNotebook.activeNotebookEditor?.document.cells!; - - // Verify we have execution counts and output. - assertHasExecutionCompletedSuccessfully(cells[0]); - assertHasExecutionCompletedWithErrors(cells[1]); - assertHasExecutionCompletedSuccessfully(cells[2]); - assertHasOutputInVSCell(cells[0]); - assertHasOutputInVSCell(cells[1]); - assertHasOutputInVSCell(cells[2]); - - // Clear the cells - await commands.executeCommand('notebook.clearAllCellsOutputs'); - - for (let cellIndex = 0; cellIndex < 3; cellIndex += 1) { - // https://github.com/microsoft/vscode-python/issues/13159 - // await waitForExecutionOrderInVSCCell(cells[cellIndex], undefined); - - await waitForVSCCellHasEmptyOutput(cells[cellIndex]); - } - }); }); suite('Use same notebook for tests', () => { suiteSetup(async () => { @@ -128,108 +86,5 @@ suite('DataScience - VSCode Notebook - (fake execution) (Clearing Output)', func cellObservableResult.unsubscribe(); cell2ObservableResult.unsubscribe(); }); - - test('Clear cell status, output and execution count before executing a cell', async function () { - // tslint:disable-next-line: no-unused-expression - return this.skip(); - - await insertPythonCellAndWait('# Some bogus cell', 0); - const vscCell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; - // Setup original state in cell. - vscCell.outputs = [{ outputKind: vscodeNotebookEnums.CellOutputKind.Text, text: 'Output1' }]; - vscCell.metadata.statusMessage = 'Error Message'; - vscCell.metadata.executionOrder = 999; - - // Once we execute the cell, the execution count & output should be cleared. - await commands.executeCommand('notebook.cell.execute'); - await waitForExecutionOrderInVSCCell(vscCell, undefined); - await waitForVSCCellHasEmptyOutput(vscCell); - await waitForVSCCellIsRunning(vscCell); - - // Now send some output. - const executionCount = 22; - cellObservableResult.next([ - { - data: { - cell_type: 'code', - execution_count: 22, - metadata: {}, - outputs: [{ output_type: 'stream', name: 'stdout', text: 'Hello' }], - source: '' - }, - file: '', - id: vscCell.uri.toString(), - line: 1, - state: CellState.executing - } - ]); - - // Confirm output was received by VS Code. - await waitForExecutionOrderInVSCCell(vscCell, executionCount); - await waitForTextOutputInVSCode(vscCell, 'Hello', 0); - - // Complete the execution. - cellObservableResult.complete(); - - // Confirm output is the same and status is a success. - await waitForExecutionCompletedSuccessfully(vscCell); - await waitForExecutionOrderInVSCCell(vscCell, executionCount); - await waitForTextOutputInVSCode(vscCell, 'Hello', 0); - }); - test('Clear cell output while executing will only clear output when executing a cell', async function () { - // tslint:disable-next-line: no-unused-expression - return this.skip(); - await insertPythonCellAndWait('# Some bogus cell', 0); - const vscCell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; - // Setup original state in cell. - vscCell.outputs = [{ outputKind: vscodeNotebookEnums.CellOutputKind.Text, text: 'Output1' }]; - vscCell.metadata.statusMessage = 'Error Message'; - vscCell.metadata.executionOrder = 999; - - // Once we execute the cell, the execution count & output should be cleared. - await commands.executeCommand('notebook.cell.execute'); - - await waitForExecutionOrderInVSCCell(vscCell, undefined); - await waitForVSCCellHasEmptyOutput(vscCell); - await waitForVSCCellIsRunning(vscCell); - - // Now send some output. - const executionCount = 22; - cellObservableResult.next([ - { - data: { - cell_type: 'code', - execution_count: 22, - metadata: {}, - outputs: [{ output_type: 'stream', name: 'stdout', text: 'Hello' }], - source: '' - }, - file: '', - id: vscCell.uri.toString(), - line: 1, - state: CellState.executing - } - ]); - - // Confirm output was received by VS Code. - await waitForExecutionOrderInVSCCell(vscCell, executionCount); - await waitForTextOutputInVSCode(vscCell, 'Hello', 0); - - // Clear output. - await commands.executeCommand('notebook.clearAllCellsOutputs'); - - // Confirm output was cleared & execution order has not been cleared & cell is still running. - await waitForVSCCellHasEmptyOutput(vscCell); - await waitForExecutionOrderInVSCCell(vscCell, executionCount); - await waitForVSCCellIsRunning(vscCell); - - // Complete the execution. - cellObservableResult.complete(); - - // Confirm output is the same and status is a success. - await waitForExecutionCompletedSuccessfully(vscCell); - await waitForExecutionOrderInVSCCell(vscCell, executionCount); - await waitForVSCCellHasEmptyOutput(vscCell); - }); }); }); From e89fcf5ed7aac58dfdc54de2da93bf881ef0fb62 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 1 Sep 2020 16:18:27 -0700 Subject: [PATCH 02/24] Remove code --- src/client/datascience/notebook/helpers/executionHelpers.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/client/datascience/notebook/helpers/executionHelpers.ts b/src/client/datascience/notebook/helpers/executionHelpers.ts index a6066df9ed81..29e1564bb2bb 100644 --- a/src/client/datascience/notebook/helpers/executionHelpers.ts +++ b/src/client/datascience/notebook/helpers/executionHelpers.ts @@ -72,7 +72,6 @@ export function updateCellWithErrorStatus(editor: NotebookEditor, cell: Notebook edit.replaceMetadata(cellIndex, { ...cell.metadata, runState: vscodeNotebookEnums.NotebookCellRunState.Error }); edit.replaceOutput(cellIndex, [translateErrorOutput(createErrorOutput(ex))]); }); - cell.outputs = [translateErrorOutput(createErrorOutput(ex))]; } /** From 6f38ea9ecde2bfec950d57971542f760e9cfa5ae Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 9 Sep 2020 16:23:42 -0700 Subject: [PATCH 03/24] Use WorkspaceEdit instead of NotebookEditor --- .../jupyter/kernels/cellExecution.ts | 48 ++++++-- .../notebook/helpers/executionHelpers.ts | 32 ++--- .../datascience/notebook/helpers/helpers.ts | 116 +++++++++--------- .../datascience/notebook/notebookEditor.ts | 43 ++++--- 4 files changed, 133 insertions(+), 106 deletions(-) diff --git a/src/client/datascience/jupyter/kernels/cellExecution.ts b/src/client/datascience/jupyter/kernels/cellExecution.ts index bfe470bf4476..d30a026962ff 100644 --- a/src/client/datascience/jupyter/kernels/cellExecution.ts +++ b/src/client/datascience/jupyter/kernels/cellExecution.ts @@ -5,7 +5,14 @@ import { nbformat } from '@jupyterlab/coreutils'; import type { KernelMessage } from '@jupyterlab/services/lib/kernel/messages'; -import { CancellationToken, CellOutputKind, CellStreamOutput, NotebookCell, NotebookCellRunState } from 'vscode'; +import { + CancellationToken, + CellOutputKind, + CellStreamOutput, + NotebookCell, + NotebookCellRunState, + WorkspaceEdit +} from 'vscode'; import type { NotebookEditor as VSCNotebookEditor } from '../../../../../types/vscode-proposed'; import { concatMultilineString, formatStreamText } from '../../../../datascience-ui/common'; import { IApplicationShell, IVSCodeNotebook } from '../../../common/application/types'; @@ -166,24 +173,32 @@ export class CellExecution { private completedSuccessfully() { this.sendPerceivedCellExecute(); + let statusMessage = ''; // If we requested a cancellation, then assume it did not even run. // If it did, then we'd get an interrupt error in the output. - this.cell.metadata.runState = this.token.isCancellationRequested + let runState = this.token.isCancellationRequested ? vscodeNotebookEnums.NotebookCellRunState.Idle : vscodeNotebookEnums.NotebookCellRunState.Success; - this.cell.metadata.statusMessage = ''; - this.cell.metadata.lastRunDuration = this.stopWatch.elapsedTime; - updateCellExecutionTimes(this.editor, this.cell, { + updateCellExecutionTimes(this.cell, { startTime: this.cell.metadata.runStartTime, + lastRunDuration: this.stopWatch.elapsedTime, duration: this.cell.metadata.lastRunDuration }); + // If there are any errors in the cell, then change status to error. if (this.cell.outputs.some((output) => output.outputKind === vscodeNotebookEnums.CellOutputKind.Error)) { - this.cell.metadata.runState = vscodeNotebookEnums.NotebookCellRunState.Error; - this.cell.metadata.statusMessage = getCellStatusMessageBasedOnFirstCellErrorOutput(this.cell.outputs); + runState = vscodeNotebookEnums.NotebookCellRunState.Error; + statusMessage = getCellStatusMessageBasedOnFirstCellErrorOutput(this.cell.outputs); } + const cellIndex = this.editor.document.cells.indexOf(this.cell); + new WorkspaceEdit().replaceCellMetadata(this.cell.notebook.uri, cellIndex, { + ...this.cell.metadata, + runState, + statusMessage + }); + this._completed = true; this._result.resolve(this.cell.metadata.runState); // Changes to metadata must be saved in ipynb, hence mark doc has dirty. @@ -209,12 +224,16 @@ export class CellExecution { * At this point we revert cell state & indicate that it has nto started & it is not busy. */ private dequeue() { - if (this.oldCellRunState === vscodeNotebookEnums.NotebookCellRunState.Running) { - this.cell.metadata.runState = vscodeNotebookEnums.NotebookCellRunState.Idle; - } else { - this.cell.metadata.runState = this.oldCellRunState; - } + const runState = + this.oldCellRunState === vscodeNotebookEnums.NotebookCellRunState.Running + ? vscodeNotebookEnums.NotebookCellRunState.Idle + : this.oldCellRunState; this.cell.metadata.runStartTime = undefined; + new WorkspaceEdit().replaceCellMetadata(this.cell.notebook.uri, this.cell.notebook.cells.indexOf(this.cell), { + ...this.cell.metadata, + runStartTime: undefined, + runState + }); this._completed = true; this._result.resolve(this.cell.metadata.runState); // Changes to metadata must be saved in ipynb, hence mark doc has dirty. @@ -226,7 +245,10 @@ export class CellExecution { * (mark it as busy). */ private enqueue() { - this.cell.metadata.runState = vscodeNotebookEnums.NotebookCellRunState.Running; + new WorkspaceEdit().replaceCellMetadata(this.cell.notebook.uri, this.cell.notebook.cells.indexOf(this.cell), { + ...this.cell.metadata, + runState: vscodeNotebookEnums.NotebookCellRunState.Running + }); this.contentProvider.notifyChangesToDocument(this.cell.notebook); } diff --git a/src/client/datascience/notebook/helpers/executionHelpers.ts b/src/client/datascience/notebook/helpers/executionHelpers.ts index 29e1564bb2bb..ca04a8b367aa 100644 --- a/src/client/datascience/notebook/helpers/executionHelpers.ts +++ b/src/client/datascience/notebook/helpers/executionHelpers.ts @@ -6,6 +6,7 @@ import type { nbformat } from '@jupyterlab/coreutils'; import type { KernelMessage } from '@jupyterlab/services'; import * as fastDeepEqual from 'fast-deep-equal'; +import { WorkspaceEdit } from 'vscode'; import type { NotebookCell, NotebookEditor } from '../../../../../types/vscode-proposed'; import { createErrorOutput } from '../../../../datascience-ui/common/cellFactory'; import { createIOutputFromCellOutputs, createVSCCellOutputsFromOutputs, translateErrorOutput } from './helpers'; @@ -66,12 +67,13 @@ export function handleUpdateDisplayDataMessage( /** * Updates the VSC cell with the error output. */ -export function updateCellWithErrorStatus(editor: NotebookEditor, cell: NotebookCell, ex: Partial) { - editor.edit((edit) => { - const cellIndex = editor.document.cells.indexOf(cell); - edit.replaceMetadata(cellIndex, { ...cell.metadata, runState: vscodeNotebookEnums.NotebookCellRunState.Error }); - edit.replaceOutput(cellIndex, [translateErrorOutput(createErrorOutput(ex))]); +export function updateCellWithErrorStatus(cell: NotebookCell, ex: Partial) { + const cellIndex = cell.notebook.cells.indexOf(cell); + new WorkspaceEdit().replaceCellMetadata(cell.document.uri, cellIndex, { + ...cell.metadata, + runState: vscodeNotebookEnums.NotebookCellRunState.Error }); + new WorkspaceEdit().replaceCellOutput(cell.document.uri, cellIndex, [translateErrorOutput(createErrorOutput(ex))]); } /** @@ -79,9 +81,10 @@ export function updateCellWithErrorStatus(editor: NotebookEditor, cell: Notebook */ export function updateCellExecutionCount(editor: NotebookEditor, cell: NotebookCell, executionCount: number): boolean { if (cell.metadata.executionOrder !== executionCount && executionCount) { - editor.edit((edit) => { - const cellIndex = editor.document.cells.indexOf(cell); - edit.replaceMetadata(cellIndex, { ...cell.metadata, executionOrder: executionCount }); + const cellIndex = editor.document.cells.indexOf(cell); + new WorkspaceEdit().replaceCellMetadata(cell.document.uri, cellIndex, { + ...cell.metadata, + executionOrder: executionCount }); return true; } @@ -94,21 +97,18 @@ export function updateCellExecutionCount(editor: NotebookEditor, cell: NotebookC * Here we update both the VSCode Cell as well as our ICell (cell in our INotebookModel). * @returns {(boolean | undefined)} Returns `true` if output has changed. */ -export function updateCellOutput( - editor: NotebookEditor, - vscCell: NotebookCell, - outputs: nbformat.IOutput[] -): boolean | undefined { +export function updateCellOutput(cell: NotebookCell, outputs: nbformat.IOutput[]): boolean | undefined { const newOutput = createVSCCellOutputsFromOutputs(outputs); // If there was no output and still no output, then nothing to do. - if (vscCell.outputs.length === 0 && newOutput.length === 0) { + if (cell.outputs.length === 0 && newOutput.length === 0) { return; } // Compare outputs (at the end of the day everything is serializable). // Hence this is a safe comparison. - if (vscCell.outputs.length === newOutput.length && fastDeepEqual(vscCell.outputs, newOutput)) { + if (cell.outputs.length === newOutput.length && fastDeepEqual(cell.outputs, newOutput)) { return; } - editor.edit((edit) => edit.replaceOutput(0, newOutput)); + const cellIndex = cell.notebook.cells.indexOf(cell); + new WorkspaceEdit().replaceCellOutput(cell.document.uri, cellIndex, newOutput); return true; } diff --git a/src/client/datascience/notebook/helpers/helpers.ts b/src/client/datascience/notebook/helpers/helpers.ts index d6f8b61f994e..f4164c4cd849 100644 --- a/src/client/datascience/notebook/helpers/helpers.ts +++ b/src/client/datascience/notebook/helpers/helpers.ts @@ -27,7 +27,9 @@ import { JupyterNotebookView } from '../constants'; // tslint:disable-next-line: no-var-requires no-require-imports const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed'); // tslint:disable-next-line: no-require-imports +import { url } from 'inspector'; import cloneDeep = require('lodash/cloneDeep'); +import { WorkspaceEdit } from 'vscode'; import { isUntitledFile } from '../../../common/utils/misc'; import { KernelConnectionMetadata } from '../../jupyter/kernels/types'; import { updateNotebookMetadata } from '../../notebookStorage/baseModel'; @@ -339,20 +341,18 @@ export function createIOutputFromCellOutputs(cellOutputs: CellOutput[]): nbforma .map((output) => output!!); } -export function clearCellForExecution(editor: NotebookEditor, cell: NotebookCell) { - editor.edit((edit) => { - const cellIndex = editor.document.cells.indexOf(cell); - edit.replaceMetadata(cellIndex, { - ...cell.metadata, - statusMessage: undefined, - executionOrder: undefined, - lastRunDuration: undefined, - runStartTime: undefined - }); - edit.replaceOutput(cellIndex, []); +export function clearCellForExecution(cell: NotebookCell) { + const cellIndex = cell.notebook.cells.indexOf(cell); + new WorkspaceEdit().replaceCellMetadata(cell.notebook.uri, cellIndex, { + ...cell.metadata, + statusMessage: undefined, + executionOrder: undefined, + lastRunDuration: undefined, + runStartTime: undefined }); + new WorkspaceEdit().replaceCellOutput(cell.notebook.uri, cellIndex, []); - updateCellExecutionTimes(editor, cell); + updateCellExecutionTimes(cell); } /** @@ -360,37 +360,40 @@ export function clearCellForExecution(editor: NotebookEditor, cell: NotebookCell * Stored as ISO for portability. */ export function updateCellExecutionTimes( - editor: NotebookEditor, cell: NotebookCell, - times?: { startTime?: number; duration?: number } + times?: { startTime?: number; duration?: number; lastRunDuration?: number } ) { - editor.edit((edit) => { - const cellIndex = editor.document.cells.indexOf(cell); - if (!times || !times.duration || !times.startTime) { - const cellMetadata = cloneDeep(cell.metadata); - let updated = false; - if (cellMetadata.custom?.metadata?.vscode?.start_execution_time) { - delete cellMetadata.custom.metadata.vscode.start_execution_time; - updated = true; - } - if (cellMetadata.custom?.metadata?.vscode?.end_execution_time) { - delete cellMetadata.custom.metadata.vscode.end_execution_time; - updated = true; - } - if (updated) { - edit.replaceMetadata(cellIndex, { ...cellMetadata }); - } - return; + const cellIndex = cell.notebook.cells.indexOf(cell); + + if (!times || !times.duration || !times.startTime) { + const cellMetadata = cloneDeep(cell.metadata); + let updated = false; + if (cellMetadata.custom?.metadata?.vscode?.start_execution_time) { + delete cellMetadata.custom.metadata.vscode.start_execution_time; + updated = true; + } + if (cellMetadata.custom?.metadata?.vscode?.end_execution_time) { + delete cellMetadata.custom.metadata.vscode.end_execution_time; + updated = true; } + if (updated) { + new WorkspaceEdit().replaceCellMetadata(cell.notebook.uri, cellIndex, { ...cellMetadata }); + } + return; + } - const startTimeISO = new Date(times.startTime).toISOString(); - const endTimeISO = new Date(times.startTime + times.duration).toISOString(); - const customMetadata = cloneDeep(cell.metadata.custom || {}); - customMetadata.metadata = customMetadata.metadata || {}; - customMetadata.metadata.vscode = customMetadata.metadata.vscode || {}; - customMetadata.metadata.vscode.end_execution_time = endTimeISO; - customMetadata.metadata.vscode.start_execution_time = startTimeISO; - edit.replaceMetadata(cellIndex, { ...cell.metadata, custom: customMetadata }); + const startTimeISO = new Date(times.startTime).toISOString(); + const endTimeISO = new Date(times.startTime + times.duration).toISOString(); + const customMetadata = cloneDeep(cell.metadata.custom || {}); + customMetadata.metadata = customMetadata.metadata || {}; + customMetadata.metadata.vscode = customMetadata.metadata.vscode || {}; + customMetadata.metadata.vscode.end_execution_time = endTimeISO; + customMetadata.metadata.vscode.start_execution_time = startTimeISO; + const lastRunDuration = times.lastRunDuration ?? cell.metadata.lastRunDuration; + new WorkspaceEdit().replaceCellMetadata(cell.notebook.uri, cellIndex, { + ...cell.metadata, + custom: customMetadata, + lastRunDuration }); } @@ -662,11 +665,7 @@ export function getCellStatusMessageBasedOnFirstCellErrorOutput(outputs?: CellOu /** * Updates a notebook document as a result of trusting it. */ -export function updateVSCNotebookAfterTrustingNotebook( - editor: NotebookEditor, - document: NotebookDocument, - originalCells: ICell[] -) { +export function updateVSCNotebookAfterTrustingNotebook(document: NotebookDocument, originalCells: ICell[]) { const areAllCellsEditableAndRunnable = document.cells.every((cell) => { if (cell.cellKind === vscodeNotebookEnums.CellKind.Markdown) { return cell.metadata.editable; @@ -690,16 +689,23 @@ export function updateVSCNotebookAfterTrustingNotebook( document.metadata.editable = true; document.metadata.runnable = true; - editor.edit((edit) => { - document.cells.forEach((cell, index) => { - if (cell.cellKind === vscodeNotebookEnums.CellKind.Markdown) { - edit.replaceMetadata(index, { ...cell.metadata, editable: true }); - } else { - edit.replaceMetadata(index, { ...cell.metadata, editable: true, runnable: true }); - // Restore the output once we trust the notebook. - // tslint:disable-next-line: no-any - edit.replaceOutput(index, createVSCCellOutputsFromOutputs(originalCells[index].data.outputs as any)); - } - }); + const workspaceEdit = new WorkspaceEdit(); + document.cells.forEach((cell, index) => { + if (cell.cellKind === vscodeNotebookEnums.CellKind.Markdown) { + workspaceEdit.replaceCellMetadata(document.uri, index, { ...cell.metadata, editable: true }); + } else { + workspaceEdit.replaceCellMetadata(document.uri, index, { + ...cell.metadata, + editable: true, + runnable: true + }); + // Restore the output once we trust the notebook. + // tslint:disable-next-line: no-any + workspaceEdit.replaceCellOutput( + document.uri, + index, + createVSCCellOutputsFromOutputs(originalCells[index].data.outputs as any) + ); + } }); } diff --git a/src/client/datascience/notebook/notebookEditor.ts b/src/client/datascience/notebook/notebookEditor.ts index cb65db236b6a..3c92f53ee3bb 100644 --- a/src/client/datascience/notebook/notebookEditor.ts +++ b/src/client/datascience/notebook/notebookEditor.ts @@ -3,7 +3,7 @@ 'use strict'; -import { ConfigurationTarget, Event, EventEmitter, Uri, WebviewPanel } from 'vscode'; +import { ConfigurationTarget, Event, EventEmitter, Uri, WebviewPanel, WorkspaceEdit } from 'vscode'; import type { NotebookCell, NotebookDocument } from 'vscode-proposed'; import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../common/application/types'; import { traceError } from '../../common/logger'; @@ -133,29 +133,26 @@ export class NotebookEditor implements INotebookEditor { return; } const defaultLanguage = getDefaultCodeLanguage(this.model); - this.vscodeNotebook.activeNotebookEditor.edit((editor) => { - const totalLength = this.document.cells.length; - editor.insert( - this.document.cells.length, - '', - defaultLanguage, - vscodeNotebookEnums.CellKind.Code, - [], - undefined - ); - for (let i = totalLength - 1; i >= 0; i = i - 1) { - editor.delete(i); + new WorkspaceEdit().replaceCells(this.document.uri, 0, 0, [ + { + cellKind: vscodeNotebookEnums.CellKind.Code, + language: defaultLanguage, + metadata: {}, + outputs: [], + source: '' } - }); + ]); } public expandAllCells(): void { if (!this.vscodeNotebook.activeNotebookEditor) { return; } - const cells = this.vscodeNotebook.activeNotebookEditor.document.cells; - this.vscodeNotebook.activeNotebookEditor.edit((edit) => { - cells.forEach((cell, index) => { - edit.replaceMetadata(index, { ...cell.metadata, inputCollapsed: false, outputCollapsed: false }); + const notebook = this.vscodeNotebook.activeNotebookEditor.document; + notebook.cells.forEach((cell, index) => { + new WorkspaceEdit().replaceCellMetadata(notebook.uri, index, { + ...cell.metadata, + inputCollapsed: false, + outputCollapsed: false }); }); } @@ -163,10 +160,12 @@ export class NotebookEditor implements INotebookEditor { if (!this.vscodeNotebook.activeNotebookEditor) { return; } - const cells = this.vscodeNotebook.activeNotebookEditor.document.cells; - this.vscodeNotebook.activeNotebookEditor.edit((edit) => { - cells.forEach((cell, index) => { - edit.replaceMetadata(index, { ...cell.metadata, inputCollapsed: true, outputCollapsed: true }); + const notebook = this.vscodeNotebook.activeNotebookEditor.document; + notebook.cells.forEach((cell, index) => { + new WorkspaceEdit().replaceCellMetadata(notebook.uri, index, { + ...cell.metadata, + inputCollapsed: true, + outputCollapsed: true }); }); } From 3a3d83739bc759f36bb038602f06b5ade2d71692 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 9 Sep 2020 16:25:45 -0700 Subject: [PATCH 04/24] Fixes --- src/client/datascience/jupyter/kernels/cellExecution.ts | 4 ++-- .../datascience/notebook/helpers/executionHelpers.ts | 2 +- src/client/datascience/notebook/helpers/helpers.ts | 4 +--- .../datascience/notebookStorage/vscNotebookModel.ts | 8 ++------ 4 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/client/datascience/jupyter/kernels/cellExecution.ts b/src/client/datascience/jupyter/kernels/cellExecution.ts index d30a026962ff..2de7a6a5887c 100644 --- a/src/client/datascience/jupyter/kernels/cellExecution.ts +++ b/src/client/datascience/jupyter/kernels/cellExecution.ts @@ -124,7 +124,7 @@ export class CellExecution { public start(kernelPromise: Promise, notebook: INotebook) { this.started = true; // Ensure we clear the cell state and trigger a change. - clearCellForExecution(this.editor, this.cell); + clearCellForExecution(this.cell); this.cell.metadata.runStartTime = new Date().getTime(); this.stopWatch.reset(); // Changes to metadata must be saved in ipynb, hence mark doc has dirty. @@ -161,7 +161,7 @@ export class CellExecution { private completedWithErrors(error: Partial) { this.sendPerceivedCellExecute(); this.cell.metadata.lastRunDuration = this.stopWatch.elapsedTime; - updateCellWithErrorStatus(this.editor, this.cell, error); + updateCellWithErrorStatus(this.cell, error); this.contentProvider.notifyChangesToDocument(this.cell.notebook); this.errorHandler.handleError((error as unknown) as Error).ignoreErrors(); diff --git a/src/client/datascience/notebook/helpers/executionHelpers.ts b/src/client/datascience/notebook/helpers/executionHelpers.ts index ca04a8b367aa..fff9abae28cd 100644 --- a/src/client/datascience/notebook/helpers/executionHelpers.ts +++ b/src/client/datascience/notebook/helpers/executionHelpers.ts @@ -58,7 +58,7 @@ export function handleUpdateDisplayDataMessage( } const vscCell = document.cells[index]; - updateCellOutput(editor, vscCell, changedOutputs); + updateCellOutput(vscCell, changedOutputs); return true; }).length > 0 ); diff --git a/src/client/datascience/notebook/helpers/helpers.ts b/src/client/datascience/notebook/helpers/helpers.ts index f4164c4cd849..69a71f548ed3 100644 --- a/src/client/datascience/notebook/helpers/helpers.ts +++ b/src/client/datascience/notebook/helpers/helpers.ts @@ -13,8 +13,7 @@ import type { NotebookCellData, NotebookCellMetadata, NotebookData, - NotebookDocument, - NotebookEditor + NotebookDocument } from 'vscode-proposed'; import { NotebookCellRunState } from '../../../../../typings/vscode-proposed'; import { concatMultilineString, splitMultilineString } from '../../../../datascience-ui/common'; @@ -27,7 +26,6 @@ import { JupyterNotebookView } from '../constants'; // tslint:disable-next-line: no-var-requires no-require-imports const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed'); // tslint:disable-next-line: no-require-imports -import { url } from 'inspector'; import cloneDeep = require('lodash/cloneDeep'); import { WorkspaceEdit } from 'vscode'; import { isUntitledFile } from '../../../common/utils/misc'; diff --git a/src/client/datascience/notebookStorage/vscNotebookModel.ts b/src/client/datascience/notebookStorage/vscNotebookModel.ts index 405d4bbe7fd9..2d1ab7563406 100644 --- a/src/client/datascience/notebookStorage/vscNotebookModel.ts +++ b/src/client/datascience/notebookStorage/vscNotebookModel.ts @@ -98,12 +98,8 @@ export class VSCodeNotebookModel extends BaseNotebookModel { } public trust() { super.trust(); - const editor = - this.vscodeNotebook && this.document - ? this.vscodeNotebook.notebookEditors.find((e) => e.document === this.document) - : undefined; - if (this.document && editor) { - updateVSCNotebookAfterTrustingNotebook(editor, this.document, this._cells); + if (this.document) { + updateVSCNotebookAfterTrustingNotebook(this.document, this._cells); // We don't need old cells. this._cells = []; } From 38cd1b9c2cdfcaaec99db35b436258b77bad252b Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 9 Sep 2020 16:53:21 -0700 Subject: [PATCH 05/24] Fixes to tests --- .../jupyter/kernels/cellExecution.ts | 10 +- src/test/datascience/notebook/edit.ds.test.ts | 173 ------------------ .../notebook/executionService.ds.test.ts | 33 ++-- src/test/datascience/notebook/helper.ts | 97 +++++----- .../notebook/interrupRestart.ds.test.ts | 4 +- .../datascience/notebook/saving.ds.test.ts | 8 +- 6 files changed, 81 insertions(+), 244 deletions(-) delete mode 100644 src/test/datascience/notebook/edit.ds.test.ts diff --git a/src/client/datascience/jupyter/kernels/cellExecution.ts b/src/client/datascience/jupyter/kernels/cellExecution.ts index 2de7a6a5887c..99e730dfabd1 100644 --- a/src/client/datascience/jupyter/kernels/cellExecution.ts +++ b/src/client/datascience/jupyter/kernels/cellExecution.ts @@ -125,7 +125,10 @@ export class CellExecution { this.started = true; // Ensure we clear the cell state and trigger a change. clearCellForExecution(this.cell); - this.cell.metadata.runStartTime = new Date().getTime(); + new WorkspaceEdit().replaceCellMetadata(this.cell.notebook.uri, this.cell.notebook.cells.indexOf(this.cell), { + ...this.cell.metadata, + runStartTime: new Date().getTime() + }); this.stopWatch.reset(); // Changes to metadata must be saved in ipynb, hence mark doc has dirty. this.contentProvider.notifyChangesToDocument(this.cell.notebook); @@ -160,7 +163,10 @@ export class CellExecution { private completedWithErrors(error: Partial) { this.sendPerceivedCellExecute(); - this.cell.metadata.lastRunDuration = this.stopWatch.elapsedTime; + new WorkspaceEdit().replaceCellMetadata(this.cell.notebook.uri, this.cell.notebook.cells.indexOf(this.cell), { + ...this.cell.metadata, + lastRunDuration: this.stopWatch.elapsedTime + }); updateCellWithErrorStatus(this.cell, error); this.contentProvider.notifyChangesToDocument(this.cell.notebook); this.errorHandler.handleError((error as unknown) as Error).ignoreErrors(); diff --git a/src/test/datascience/notebook/edit.ds.test.ts b/src/test/datascience/notebook/edit.ds.test.ts deleted file mode 100644 index 03ea4f780c86..000000000000 --- a/src/test/datascience/notebook/edit.ds.test.ts +++ /dev/null @@ -1,173 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -// tslint:disable: no-var-requires no-require-imports no-invalid-this no-any - -import { assert } from 'chai'; -import * as path from 'path'; -import * as sinon from 'sinon'; -import { commands, Uri } from 'vscode'; -import { IDisposable } from '../../../client/common/types'; -import { ICell, INotebookEditorProvider, INotebookModel } from '../../../client/datascience/types'; -import { splitMultilineString } from '../../../datascience-ui/common'; -import { IExtensionTestApi, waitForCondition } from '../../common'; -import { EXTENSION_ROOT_DIR_FOR_TESTS } from '../../constants'; -import { initialize } from '../../initialize'; -import { - canRunTests, - closeNotebooksAndCleanUpAfterTests, - createTemporaryNotebook, - deleteAllCellsAndWait, - deleteCell, - insertMarkdownCell, - insertMarkdownCellAndWait, - insertPythonCell, - insertPythonCellAndWait, - trustAllNotebooks -} from './helper'; - -suite('DataScience - VSCode Notebook (Edit)', function () { - this.timeout(10_000); - - const templateIPynb = path.join( - EXTENSION_ROOT_DIR_FOR_TESTS, - 'src', - 'test', - 'datascience', - 'notebook', - 'test.ipynb' - ); - let testIPynb: Uri; - let api: IExtensionTestApi; - let editorProvider: INotebookEditorProvider; - const disposables: IDisposable[] = []; - suiteSetup(async function () { - this.timeout(10_000); - api = await initialize(); - if (!(await canRunTests())) { - return this.skip(); - } - await trustAllNotebooks(); - editorProvider = api.serviceContainer.get(INotebookEditorProvider); - }); - suiteTeardown(() => closeNotebooksAndCleanUpAfterTests(disposables)); - [true, false].forEach((isUntitled) => { - suite(isUntitled ? 'Untitled Notebook' : 'Existing Notebook', () => { - let model: INotebookModel; - setup(async () => { - sinon.restore(); - await trustAllNotebooks(); - // Don't use same file (due to dirty handling, we might save in dirty.) - // Cuz we won't save to file, hence extension will backup in dirty file and when u re-open it will open from dirty. - testIPynb = Uri.file(await createTemporaryNotebook(templateIPynb, disposables)); - - // Reset for tests, do this every time, as things can change due to config changes etc. - const editor = isUntitled ? await editorProvider.createNew() : await editorProvider.open(testIPynb); - model = editor.model!; - }); - teardown(() => closeNotebooksAndCleanUpAfterTests(disposables)); - async function assertTextInCell(cell: ICell, text: string) { - await waitForCondition( - async () => (cell.data.source as string[]).join('') === splitMultilineString(text).join(''), - 1_000, - `Text ${text} is not in ${(cell.data.source as string[]).join('')}` - ); - } - test('Insert and edit cell', async () => { - await deleteAllCellsAndWait(); - await insertPythonCellAndWait('HELLO'); - await assertTextInCell(model.cells[0], 'HELLO'); - }); - - test('Deleting a cell in an nb should update our NotebookModel', async () => { - // Delete first cell. - await deleteCell(0); - - // Verify model state is correct. - await waitForCondition(async () => model.cells.length === 0, 5_000, 'Not deleted'); - }); - test('Adding a markdown cell in an nb should update our NotebookModel', async () => { - await insertMarkdownCell('HELLO'); - - // Verify model has been updated - await waitForCondition(async () => model.cells.length === 2, 5_000, 'Not inserted'); - }); - test('Adding a markdown cell then deleting it should update our NotebookModel', async () => { - await insertMarkdownCell('HELLO'); - - // Verify events were fired. - await waitForCondition(async () => model.cells.length === 2, 5_000, 'Not inserted'); - - // Delete second cell. - await deleteCell(1); - - await waitForCondition(async () => model.cells.length === 1, 5_000, 'Not Deleted'); - }); - test('Adding a code cell in an nb should update our NotebookModel', async () => { - await insertPythonCell('HELLO'); - - await waitForCondition(async () => model.cells.length === 2, 5_000, 'Not Inserted'); - }); - test('Adding a code cell in specific position should update our NotebookModel', async () => { - await insertPythonCell('HELLO', 1); - - // Verify events were fired. - await waitForCondition(async () => model.cells.length === 2, 5_000, 'Not Inserted'); - assert.equal(model.cells.length, 2); - }); - function assertCodeCell(index: number, text: string) { - const cell = model.cells[index]; - assert.equal(cell.data.cell_type, 'code'); - assert.deepEqual(cell.data.source, text === '' ? [''] : splitMultilineString(text)); - return true; - } - function assertMarkdownCell(index: number, text?: string) { - const cell = model.cells[index]; - assert.equal(cell.data.cell_type, 'markdown'); - assert.deepEqual( - cell.data.source, - text === undefined ? [] : text === '' ? [''] : splitMultilineString(text) - ); - return true; - } - test('Change cell to markdown', async () => { - await deleteAllCellsAndWait(); - await insertPythonCellAndWait('HELLO'); - - await commands.executeCommand('notebook.cell.changeToMarkdown'); - - await waitForCondition(async () => assertMarkdownCell(0, 'HELLO'), 1_000, 'Not Changed'); - }); - test('Change cell to code', async function () { - this.timeout(10_000); - await deleteAllCellsAndWait(); - await insertMarkdownCellAndWait('HELLO'); - - await commands.executeCommand('notebook.cell.changeToCode'); - - await waitForCondition(async () => assertCodeCell(0, 'HELLO'), 1_000, 'Not Changed'); - }); - test('Toggle cells (code->markdown->code->markdown)', async () => { - await deleteAllCellsAndWait(); - await insertPythonCellAndWait('HELLO'); - - await commands.executeCommand('notebook.cell.changeToMarkdown'); - - await waitForCondition(async () => assertMarkdownCell(0, 'HELLO'), 1_000, 'Not Changed'); - - await commands.executeCommand('notebook.cell.changeToCode'); - - await waitForCondition(async () => assertCodeCell(0, 'HELLO'), 1_000, 'Not Changed'); - - await commands.executeCommand('notebook.cell.changeToMarkdown'); - - await waitForCondition(async () => assertMarkdownCell(0, 'HELLO'), 1_000, 'Not Changed'); - }); - test('Cut cell', async () => { - await commands.executeCommand('notebook.cell.cut'); - - await waitForCondition(async () => model.cells.length === 0, 5_000, 'Not Cut'); - }); - }); - }); -}); diff --git a/src/test/datascience/notebook/executionService.ds.test.ts b/src/test/datascience/notebook/executionService.ds.test.ts index a2165b660984..dea97b8af244 100644 --- a/src/test/datascience/notebook/executionService.ds.test.ts +++ b/src/test/datascience/notebook/executionService.ds.test.ts @@ -57,7 +57,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { setup(deleteAllCellsAndWait); suiteTeardown(() => closeNotebooksAndCleanUpAfterTests(disposables)); test('Execute cell using VSCode Kernel', async () => { - await insertPythonCellAndWait('print("Hello World")', 0); + await insertPythonCellAndWait('print("Hello World")'); const cell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; await executeCell(cell); @@ -70,7 +70,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { ); }); test('Executed events are triggered', async () => { - await insertPythonCellAndWait('print("Hello World")', 0); + await insertPythonCellAndWait('print("Hello World")'); const cell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; const executed = createEventHandler(editorProvider.activeEditor!, 'executed', disposables); @@ -88,7 +88,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { await codeExecuted.assertFired(1_000); }); test('Empty cell will not get executed', async () => { - await insertPythonCellAndWait('', 0); + await insertPythonCellAndWait(''); const cell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; await executeCell(cell); @@ -97,8 +97,8 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { assert.isUndefined(cell?.metadata.runState); }); test('Empty cells will not get executed when running whole document', async () => { - await insertPythonCellAndWait('', 0); - await insertPythonCellAndWait('print("Hello World")', 1); + await insertPythonCellAndWait(''); + await insertPythonCellAndWait('print("Hello World")'); const cells = vscodeNotebook.activeNotebookEditor?.document.cells!; await executeActiveDocument(); @@ -112,7 +112,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { assert.isUndefined(cells[0].metadata.runState); }); test('Execute cell should mark a notebook as being dirty', async () => { - await insertPythonCellAndWait('print("Hello World")', 0); + await insertPythonCellAndWait('print("Hello World")'); const contentProvider = api.serviceContainer.get(INotebookContentProvider); const cell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; const changedEvent = createEventHandler(contentProvider, 'onDidChangeNotebook', disposables); @@ -128,7 +128,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { assert.ok(changedEvent.fired, 'Notebook should be dirty after executing a cell'); }); test('Verify Cell output, execution count and status', async () => { - await insertPythonCellAndWait('print("Hello World")', 0); + await insertPythonCellAndWait('print("Hello World")'); const cell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; await executeActiveDocument(); @@ -147,8 +147,8 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { assert.ok(cell.metadata.executionOrder, 'Execution count should be > 0'); }); test('Verify multiple cells get executed', async () => { - await insertPythonCellAndWait('print("Foo Bar")', 0); - await insertPythonCellAndWait('print("Hello World")', 1); + await insertPythonCellAndWait('print("Foo Bar")'); + await insertPythonCellAndWait('print("Hello World")'); const cells = vscodeNotebook.activeNotebookEditor?.document.cells!; await executeActiveDocument(); @@ -163,14 +163,14 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { // Verify output. assertHasTextOutputInVSCode(cells[0], 'Foo Bar', 0); - assertHasTextOutputInVSCode(cells[1], 'Hello World', 0); + assertHasTextOutputInVSCode(cells[1], 'Hello World', 1); // Verify execution count. assert.ok(cells[0].metadata.executionOrder, 'Execution count should be > 0'); assert.equal(cells[1].metadata.executionOrder! - 1, cells[0].metadata.executionOrder!); }); test('Verify metadata for successfully executed cell', async () => { - await insertPythonCellAndWait('print("Foo Bar")', 0); + await insertPythonCellAndWait('print("Foo Bar")'); const cell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; await executeActiveDocument(); @@ -189,7 +189,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { assert.equal(cell.metadata.statusMessage, '', 'Incorrect Status message'); }); test('Verify output & metadata for executed cell with errors', async () => { - await insertPythonCellAndWait('print(abcd)', 0); + await insertPythonCellAndWait('print(abcd)'); const cell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; await executeActiveDocument(); @@ -215,9 +215,9 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { assert.include(cell.metadata.statusMessage!, 'abcd', 'Must contain error message'); }); test('Updating display data', async () => { - await insertPythonCellAndWait('from IPython.display import Markdown\n', 0); - await insertPythonCellAndWait('dh = display(display_id=True)\n', 1); - await insertPythonCellAndWait('dh.update(Markdown("foo"))\n', 2); + await insertPythonCellAndWait('from IPython.display import Markdown\n'); + await insertPythonCellAndWait('dh = display(display_id=True)\n'); + await insertPythonCellAndWait('dh.update(Markdown("foo"))\n'); const displayCell = vscodeNotebook.activeNotebookEditor?.document.cells![1]!; const updateCell = vscodeNotebook.activeNotebookEditor?.document.cells![2]!; @@ -252,8 +252,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { time.sleep(0.1) print(i) - print("End")`, - 0 + print("End")` ); const cell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; diff --git a/src/test/datascience/notebook/helper.ts b/src/test/datascience/notebook/helper.ts index 125443ab63a1..b0ddcaa59485 100644 --- a/src/test/datascience/notebook/helper.ts +++ b/src/test/datascience/notebook/helper.ts @@ -10,7 +10,7 @@ import * as path from 'path'; import * as sinon from 'sinon'; import * as tmp from 'tmp'; import { instance, mock } from 'ts-mockito'; -import { commands, Memento, TextDocument, Uri } from 'vscode'; +import { commands, Memento, TextDocument, Uri, WorkspaceEdit } from 'vscode'; import { NotebookCell, NotebookDocument } from '../../../../types/vscode-proposed'; import { CellDisplayOutput } from '../../../../typings/vscode-proposed'; import { IApplicationEnvironment, IVSCodeNotebook } from '../../../client/common/application/types'; @@ -43,73 +43,78 @@ async function getServices() { }; } -export async function insertMarkdownCell(source: string, index: number = 0) { +export async function insertMarkdownCell(source: string) { const { vscodeNotebook } = await getServices(); - const vscEditor = vscodeNotebook.activeNotebookEditor; - await new Promise((resolve) => - vscEditor?.edit((builder) => { - builder.insert(index, source, MARKDOWN_LANGUAGE, vscodeNotebookEnums.CellKind.Markdown, [], undefined); - resolve(); - }) - ); + const activeEditor = vscodeNotebook.activeNotebookEditor; + if (!activeEditor) { + assert.fail('No active editor'); + return; + } + new WorkspaceEdit().replaceCells(activeEditor.document.uri, activeEditor.document.cells.length, 0, [ + { + cellKind: vscodeNotebookEnums.CellKind.Markdown, + language: MARKDOWN_LANGUAGE, + source, + metadata: {}, + outputs: [] + } + ]); await waitForCondition( - async () => vscEditor?.document.cells[index].document.getText().trim() === source.trim(), + async () => activeEditor?.document.cells[0].document.getText().trim() === source.trim(), 5_000, 'Cell not inserted' ); } -export async function insertPythonCell(source: string, index: number = 0) { +export async function insertPythonCell(source: string) { const { vscodeNotebook } = await getServices(); - const vscEditor = vscodeNotebook.activeNotebookEditor; - await new Promise((resolve) => - vscEditor?.edit((builder) => { - builder.insert(index, source, PYTHON_LANGUAGE, vscodeNotebookEnums.CellKind.Code, [], undefined); - resolve(); - }) - ); - + const activeEditor = vscodeNotebook.activeNotebookEditor; + if (!activeEditor) { + assert.fail('No active editor'); + return; + } + new WorkspaceEdit().replaceCells(activeEditor.document.uri, activeEditor.document.cells.length, 0, [ + { + cellKind: vscodeNotebookEnums.CellKind.Code, + language: PYTHON_LANGUAGE, + source, + metadata: {}, + outputs: [] + } + ]); await waitForCondition( - async () => vscEditor?.document.cells[index].document.getText().trim() === source.trim(), + async () => activeEditor?.document.cells[0].document.getText().trim() === source.trim(), 5_000, 'Cell not inserted' ); } -export async function insertPythonCellAndWait(source: string, index: number = 0) { - await insertPythonCell(source, index); +export async function insertPythonCellAndWait(source: string) { + await insertPythonCell(source); } -export async function insertMarkdownCellAndWait(source: string, index: number = 0) { - await insertMarkdownCell(source, index); +export async function insertMarkdownCellAndWait(source: string) { + await insertMarkdownCell(source); } export async function deleteCell(index: number = 0) { const { vscodeNotebook } = await getServices(); const activeEditor = vscodeNotebook.activeNotebookEditor; - await new Promise((resolve) => - activeEditor?.edit((builder) => { - builder.delete(index); - resolve(); - }) - ); + if (!activeEditor || activeEditor.document.cells.length === 0) { + return; + } + if (!activeEditor) { + assert.fail('No active editor'); + return; + } + new WorkspaceEdit().replaceCells(activeEditor.document.uri, index, 1, []); } -export async function deleteAllCellsAndWait(index: number = 0) { +export async function deleteAllCellsAndWait() { const { vscodeNotebook } = await getServices(); const activeEditor = vscodeNotebook.activeNotebookEditor; - if (!activeEditor) { + if (!activeEditor || activeEditor.document.cells.length === 0) { return; } - const vscCells = activeEditor.document.cells!; - let previousCellOut = vscCells.length; - while (previousCellOut) { - await new Promise((resolve) => - activeEditor?.edit((builder) => { - builder.delete(index); - resolve(); - }) - ); - // Wait for cell to get deleted. - await waitForCondition(async () => vscCells.length === previousCellOut - 1, 1_000, 'Cell not deleted'); - previousCellOut = vscCells.length; - } + new WorkspaceEdit().replaceCells(activeEditor.document.uri, 0, activeEditor.document.cells.length, []); + // Wait for cell to get deleted. + await waitForCondition(async () => activeEditor.document.cells.length === 0, 1_000, 'Cell not deleted'); } export async function createTemporaryFile(options: { @@ -197,7 +202,7 @@ export async function startJupyter(closeInitialEditor: boolean) { const disposables: IDisposable[] = []; try { await editorProvider.createNew(); - await insertPythonCell('print("Hello World")', 0); + await insertPythonCell('print("Hello World")'); const cell = vscodeNotebook.activeNotebookEditor!.document.cells[0]!; await executeActiveDocument(); // Wait for Jupyter to start. diff --git a/src/test/datascience/notebook/interrupRestart.ds.test.ts b/src/test/datascience/notebook/interrupRestart.ds.test.ts index ca5e2b78ae34..8c80d6adc5d3 100644 --- a/src/test/datascience/notebook/interrupRestart.ds.test.ts +++ b/src/test/datascience/notebook/interrupRestart.ds.test.ts @@ -72,7 +72,7 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)', suiteTeardown(() => closeNotebooksAndCleanUpAfterTests(disposables.concat(suiteDisposables))); test('Cancelling token will cancel cell execution', async () => { - await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)', 0); + await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)'); const cell = vscEditor.document.cells[0]; const appShell = api.serviceContainer.get(IApplicationShell); const showInformationMessage = sinon.stub(appShell, 'showInformationMessage'); @@ -106,7 +106,7 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)', } }); test('Restarting kernel will cancel cell execution & we can re-run a cell', async () => { - await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)', 0); + await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)'); const cell = vscEditor.document.cells[0]; await executeActiveDocument(); diff --git a/src/test/datascience/notebook/saving.ds.test.ts b/src/test/datascience/notebook/saving.ds.test.ts index ff1fb04f2cc2..ce00d260cfda 100644 --- a/src/test/datascience/notebook/saving.ds.test.ts +++ b/src/test/datascience/notebook/saving.ds.test.ts @@ -132,10 +132,10 @@ suite('DataScience - VSCode Notebook - (Saving)', function () { const testIPynb = Uri.file(await createTemporaryNotebook(templateIPynb, disposables)); await editorProvider.open(testIPynb); - await insertPythonCellAndWait('print(1)', 0); - await insertPythonCellAndWait('print(a)', 1); - await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)', 2); - await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)', 3); + await insertPythonCellAndWait('print(1)'); + await insertPythonCellAndWait('print(a)'); + await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)'); + await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)'); let cell1: NotebookCell; let cell2: NotebookCell; let cell3: NotebookCell; From 4191505b7d2b986956a2329238e49f035ffea996 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 9 Sep 2020 16:56:47 -0700 Subject: [PATCH 06/24] More fixes --- src/test/datascience/notebook/helper.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/test/datascience/notebook/helper.ts b/src/test/datascience/notebook/helper.ts index b0ddcaa59485..bb9e7243e0e7 100644 --- a/src/test/datascience/notebook/helper.ts +++ b/src/test/datascience/notebook/helper.ts @@ -61,7 +61,9 @@ export async function insertMarkdownCell(source: string) { ]); await waitForCondition( - async () => activeEditor?.document.cells[0].document.getText().trim() === source.trim(), + async () => + activeEditor?.document.cells[activeEditor.document.cells.length - 1].document.getText().trim() === + source.trim(), 5_000, 'Cell not inserted' ); @@ -83,7 +85,9 @@ export async function insertPythonCell(source: string) { } ]); await waitForCondition( - async () => activeEditor?.document.cells[0].document.getText().trim() === source.trim(), + async () => + activeEditor?.document.cells[activeEditor.document.cells.length - 1].document.getText().trim() === + source.trim(), 5_000, 'Cell not inserted' ); From 3ee2611ff27946bc11fa664e09cdc50bc6defce8 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 9 Sep 2020 17:25:46 -0700 Subject: [PATCH 07/24] Fixes --- .../notebook/cellOutput.ds.test.ts | 90 ------------------- .../notebook/contentProvider.ds.test.ts | 2 +- 2 files changed, 1 insertion(+), 91 deletions(-) delete mode 100644 src/test/datascience/notebook/cellOutput.ds.test.ts diff --git a/src/test/datascience/notebook/cellOutput.ds.test.ts b/src/test/datascience/notebook/cellOutput.ds.test.ts deleted file mode 100644 index 6f081338859f..000000000000 --- a/src/test/datascience/notebook/cellOutput.ds.test.ts +++ /dev/null @@ -1,90 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -'use strict'; - -// tslint:disable:no-require-imports no-var-requires -import { join } from 'path'; -import { Subject } from 'rxjs/Subject'; -import * as sinon from 'sinon'; -import { anything, instance, mock, reset, when } from 'ts-mockito'; -import { Uri } from 'vscode'; -import { IDisposable } from '../../../client/common/types'; -import { ICell, INotebook, INotebookEditorProvider, INotebookProvider } from '../../../client/datascience/types'; -import { IExtensionTestApi } from '../../common'; -import { EXTENSION_ROOT_DIR_FOR_TESTS, initialize } from '../../initialize'; -import { - canRunTests, - closeNotebooksAndCleanUpAfterTests, - createTemporaryNotebook, - deleteAllCellsAndWait, - trustAllNotebooks -} from './helper'; - -// tslint:disable-next-line: no-var-requires no-require-imports - -// tslint:disable: no-any no-invalid-this -suite('DataScience - VSCode Notebook - (fake execution) (Clearing Output)', function () { - this.timeout(10_000); - - let api: IExtensionTestApi; - let editorProvider: INotebookEditorProvider; - let notebookProvider: INotebookProvider; - let nb: INotebook; - let cellObservableResult: Subject; - let cell2ObservableResult: Subject; - - suiteSetup(async function () { - api = await initialize(); - if (!(await canRunTests())) { - return this.skip(); - } - await trustAllNotebooks(); - notebookProvider = api.serviceContainer.get(INotebookProvider); - editorProvider = api.serviceContainer.get(INotebookEditorProvider); - }); - suiteTeardown(() => closeNotebooksAndCleanUpAfterTests([])); - suite('Different notebooks in each test', () => { - const disposables2: IDisposable[] = []; - const templateIPynb = join( - EXTENSION_ROOT_DIR_FOR_TESTS, - 'src', - 'test', - 'datascience', - 'notebook', - 'with3CellsAndOutput.ipynb' - ); - suiteTeardown(() => closeNotebooksAndCleanUpAfterTests(disposables2)); - setup(async () => { - await trustAllNotebooks(); - const testIPynb = Uri.file(await createTemporaryNotebook(templateIPynb, disposables2)); - await editorProvider.open(testIPynb); - }); - }); - suite('Use same notebook for tests', () => { - suiteSetup(async () => { - await trustAllNotebooks(); - // Open a notebook and use this for all tests in this test suite. - await editorProvider.createNew(); - }); - setup(async () => { - sinon.restore(); - const getOrCreateNotebook = sinon.stub(notebookProvider, 'getOrCreateNotebook'); - nb = mock(); - (instance(nb) as any).then = undefined; - getOrCreateNotebook.resolves(instance(nb)); - - cellObservableResult = new Subject(); - cell2ObservableResult = new Subject(); - reset(nb); - when(nb.executeObservable(anything(), anything(), anything(), anything(), anything())).thenReturn( - cellObservableResult.asObservable() - ); - await deleteAllCellsAndWait(); - }); - teardown(() => { - cellObservableResult.unsubscribe(); - cell2ObservableResult.unsubscribe(); - }); - }); -}); diff --git a/src/test/datascience/notebook/contentProvider.ds.test.ts b/src/test/datascience/notebook/contentProvider.ds.test.ts index fa283388337f..d39c1683569d 100644 --- a/src/test/datascience/notebook/contentProvider.ds.test.ts +++ b/src/test/datascience/notebook/contentProvider.ds.test.ts @@ -41,13 +41,13 @@ suite('DataScience - VSCode Notebook - (Open)', function () { if (!(await canRunTests())) { return this.skip(); } - await trustAllNotebooks(); }); setup(async () => { sinon.restore(); // Don't use same file (due to dirty handling, we might save in dirty.) // Cuz we won't save to file, hence extension will backup in dirty file and when u re-open it will open from dirty. testIPynb = Uri.file(await createTemporaryNotebook(templateIPynb, disposables)); + await trustAllNotebooks(); }); teardown(async () => closeNotebooksAndCleanUpAfterTests(disposables)); test('Verify Notebook Json', async () => { From 285af17c31614e6c9aacd3557d58e460db022f0e Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 10 Sep 2020 09:22:02 -0700 Subject: [PATCH 08/24] Fixes --- src/client/datascience/notebook/notebookEditor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/datascience/notebook/notebookEditor.ts b/src/client/datascience/notebook/notebookEditor.ts index 3c92f53ee3bb..bb48303c9b15 100644 --- a/src/client/datascience/notebook/notebookEditor.ts +++ b/src/client/datascience/notebook/notebookEditor.ts @@ -133,7 +133,7 @@ export class NotebookEditor implements INotebookEditor { return; } const defaultLanguage = getDefaultCodeLanguage(this.model); - new WorkspaceEdit().replaceCells(this.document.uri, 0, 0, [ + new WorkspaceEdit().replaceCells(this.document.uri, 0, this.document.cells.length - 1, [ { cellKind: vscodeNotebookEnums.CellKind.Code, language: defaultLanguage, From 850cf3f33e38584d5647f666685da34652ac2bfd Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 10 Sep 2020 09:22:16 -0700 Subject: [PATCH 09/24] More fixes --- src/test/datascience/notebook/helper.ts | 54 ++++++++++++++++--------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/src/test/datascience/notebook/helper.ts b/src/test/datascience/notebook/helper.ts index bb9e7243e0e7..1b50f9c8ffdc 100644 --- a/src/test/datascience/notebook/helper.ts +++ b/src/test/datascience/notebook/helper.ts @@ -50,15 +50,22 @@ export async function insertMarkdownCell(source: string) { assert.fail('No active editor'); return; } - new WorkspaceEdit().replaceCells(activeEditor.document.uri, activeEditor.document.cells.length, 0, [ - { - cellKind: vscodeNotebookEnums.CellKind.Markdown, - language: MARKDOWN_LANGUAGE, - source, - metadata: {}, - outputs: [] - } - ]); + new WorkspaceEdit().replaceCells( + activeEditor.document.uri, + activeEditor.document.cells.length - 1, + activeEditor.document.cells.length - 1, + [ + { + cellKind: vscodeNotebookEnums.CellKind.Markdown, + language: MARKDOWN_LANGUAGE, + source, + metadata: { + hasExecutionOrder: false + }, + outputs: [] + } + ] + ); await waitForCondition( async () => @@ -75,15 +82,22 @@ export async function insertPythonCell(source: string) { assert.fail('No active editor'); return; } - new WorkspaceEdit().replaceCells(activeEditor.document.uri, activeEditor.document.cells.length, 0, [ - { - cellKind: vscodeNotebookEnums.CellKind.Code, - language: PYTHON_LANGUAGE, - source, - metadata: {}, - outputs: [] - } - ]); + new WorkspaceEdit().replaceCells( + activeEditor.document.uri, + activeEditor.document.cells.length - 1, + activeEditor.document.cells.length - 1, + [ + { + cellKind: vscodeNotebookEnums.CellKind.Code, + language: PYTHON_LANGUAGE, + source, + metadata: { + hasExecutionOrder: false + }, + outputs: [] + } + ] + ); await waitForCondition( async () => activeEditor?.document.cells[activeEditor.document.cells.length - 1].document.getText().trim() === @@ -108,7 +122,7 @@ export async function deleteCell(index: number = 0) { assert.fail('No active editor'); return; } - new WorkspaceEdit().replaceCells(activeEditor.document.uri, index, 1, []); + new WorkspaceEdit().replaceCells(activeEditor.document.uri, index, index, []); } export async function deleteAllCellsAndWait() { const { vscodeNotebook } = await getServices(); @@ -116,7 +130,7 @@ export async function deleteAllCellsAndWait() { if (!activeEditor || activeEditor.document.cells.length === 0) { return; } - new WorkspaceEdit().replaceCells(activeEditor.document.uri, 0, activeEditor.document.cells.length, []); + new WorkspaceEdit().replaceCells(activeEditor.document.uri, 0, activeEditor.document.cells.length - 1, []); // Wait for cell to get deleted. await waitForCondition(async () => activeEditor.document.cells.length === 0, 1_000, 'Cell not deleted'); } From 5cc78477d1e3b53b5a60859d32858fb88ea3fad6 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 10 Sep 2020 10:29:05 -0700 Subject: [PATCH 10/24] Fixes --- src/client/datascience/notebook/helpers/helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/datascience/notebook/helpers/helpers.ts b/src/client/datascience/notebook/helpers/helpers.ts index 69a71f548ed3..d601a312ce63 100644 --- a/src/client/datascience/notebook/helpers/helpers.ts +++ b/src/client/datascience/notebook/helpers/helpers.ts @@ -698,10 +698,10 @@ export function updateVSCNotebookAfterTrustingNotebook(document: NotebookDocumen runnable: true }); // Restore the output once we trust the notebook. - // tslint:disable-next-line: no-any workspaceEdit.replaceCellOutput( document.uri, index, + // tslint:disable-next-line: no-any createVSCCellOutputsFromOutputs(originalCells[index].data.outputs as any) ); } From 7b0f67749cd076cb3e016f6a21ac5e26acd9b267 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 12:08:42 -0700 Subject: [PATCH 11/24] Update types --- types/vscode-proposed/index.d.ts | 110 ++++++++++++++++++++-------- types/vscode.proposed.d.ts | 112 +++++++++++++++++++++-------- typings/vscode-proposed/index.d.ts | 110 ++++++++++++++++++++-------- 3 files changed, 242 insertions(+), 90 deletions(-) diff --git a/types/vscode-proposed/index.d.ts b/types/vscode-proposed/index.d.ts index 2abb5824d26f..76648e3bfc73 100644 --- a/types/vscode-proposed/index.d.ts +++ b/types/vscode-proposed/index.d.ts @@ -17,6 +17,8 @@ import { } from 'vscode'; // Copy nb section from https://github.com/microsoft/vscode/blob/master/src/vs/vscode.proposed.d.ts. +//#region @rebornix: Notebook + export enum CellKind { Markdown = 1, Code = 2 @@ -160,6 +162,7 @@ export interface NotebookCellMetadata { } export interface NotebookCell { + readonly index: number; readonly notebook: NotebookDocument; readonly uri: Uri; readonly cellKind: CellKind; @@ -213,6 +216,20 @@ export interface NotebookDocumentMetadata { runState?: NotebookRunState; } +export interface NotebookDocumentContentOptions { + /** + * Controls if outputs change will trigger notebook document content change and if it will be used in the diff editor + * Default to false. If the content provider doesn't persisit the outputs in the file document, this should be set to true. + */ + transientOutputs: boolean; + + /** + * Controls if a meetadata property change will trigger notebook document content change and if it will be used in the diff editor + * Default to false. If the content provider doesn't persisit a metadata property in the file document, it should be set to true. + */ + transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; +} + export interface NotebookDocument { readonly uri: Uri; readonly version: number; @@ -221,6 +238,7 @@ export interface NotebookDocument { readonly isDirty: boolean; readonly isUntitled: boolean; readonly cells: ReadonlyArray; + readonly contentOptions: NotebookDocumentContentOptions; languages: string[]; metadata: NotebookDocumentMetadata; } @@ -245,15 +263,21 @@ export interface NotebookConcatTextDocument { } export interface WorkspaceEdit { - replaceCells( + replaceNotebookMetadata(uri: Uri, value: NotebookDocumentMetadata): void; + replaceNotebookCells( uri: Uri, start: number, end: number, cells: NotebookCellData[], metadata?: WorkspaceEditEntryMetadata ): void; - replaceCellOutput(uri: Uri, index: number, outputs: CellOutput[], metadata?: WorkspaceEditEntryMetadata): void; - replaceCellMetadata( + replaceNotebookCellOutput( + uri: Uri, + index: number, + outputs: CellOutput[], + metadata?: WorkspaceEditEntryMetadata + ): void; + replaceNotebookCellMetadata( uri: Uri, index: number, cellMetadata: NotebookCellMetadata, @@ -261,26 +285,18 @@ export interface WorkspaceEdit { ): void; } -export interface NotebookEditorCellEdit { +export interface NotebookEditorEdit { + replaceMetadata(value: NotebookDocumentMetadata): void; replaceCells(start: number, end: number, cells: NotebookCellData[]): void; - replaceOutput(index: number, outputs: CellOutput[]): void; - replaceMetadata(index: number, metadata: NotebookCellMetadata): void; - - /** @deprecated */ - insert( - index: number, - content: string | string[], - language: string, - type: CellKind, - outputs: CellOutput[], - metadata: NotebookCellMetadata | undefined - ): void; - /** @deprecated */ - delete(index: number): void; + replaceCellOutput(index: number, outputs: CellOutput[]): void; + replaceCellMetadata(index: number, metadata: NotebookCellMetadata): void; } export interface NotebookCellRange { readonly start: number; + /** + * exclusive + */ readonly end: number; } @@ -359,7 +375,19 @@ export interface NotebookEditor { */ asWebviewUri(localResource: Uri): Uri; - edit(callback: (editBuilder: NotebookEditorCellEdit) => void): Thenable; + /** + * Perform an edit on the notebook associated with this notebook editor. + * + * The given callback-function is invoked with an [edit-builder](#NotebookEditorEdit) which must + * be used to make edits. Note that the edit-builder is only valid while the + * callback executes. + * + * @param callback A function which can create edits using an [edit-builder](#NotebookEditorEdit). + * @return A promise that resolves with a value indicating if the edits could be applied. + */ + edit(callback: (editBuilder: NotebookEditorEdit) => void): Thenable; + + setDecorations(decorationType: NotebookEditorDecorationType, range: NotebookCellRange): void; revealRange(range: NotebookCellRange, revealType?: NotebookEditorRevealType): void; } @@ -374,6 +402,10 @@ export interface NotebookRenderRequest { outputId: string; } +export interface NotebookDocumentMetadataChangeEvent { + readonly document: NotebookDocument; +} + export interface NotebookCellsChangeData { readonly start: number; readonly deletedCount: number; @@ -541,6 +573,10 @@ export interface NotebookCommunication { } export interface NotebookContentProvider { + readonly options?: NotebookDocumentContentOptions; + readonly onDidChangeNotebookContentOptions?: Event; + readonly onDidChangeNotebook: Event; + /** * Content providers should always use [file system providers](#FileSystemProvider) to * resolve the raw content for `uri` as the resouce is not necessarily a file on disk. @@ -549,7 +585,6 @@ export interface NotebookContentProvider { resolveNotebook(document: NotebookDocument, webview: NotebookCommunication): Promise; saveNotebook(document: NotebookDocument, cancellation: CancellationToken): Promise; saveNotebookAs(targetResource: Uri, document: NotebookDocument, cancellation: CancellationToken): Promise; - readonly onDidChangeNotebook: Event; backupNotebook( document: NotebookDocument, context: NotebookDocumentBackupContext, @@ -570,9 +605,11 @@ export interface NotebookKernel { cancelAllCellsExecution(document: NotebookDocument): void; } +export type NotebookFilenamePattern = GlobPattern | { include: GlobPattern; exclude: GlobPattern }; + export interface NotebookDocumentFilter { viewType?: string | string[]; - filenamePattern?: GlobPattern | { include: GlobPattern; exclude: GlobPattern }; + filenamePattern?: NotebookFilenamePattern; } export interface NotebookKernelProvider { @@ -614,21 +651,30 @@ export interface NotebookCellStatusBarItem { dispose(): void; } +export interface NotebookDecorationRenderOptions { + backgroundColor?: string | ThemeColor; + borderColor?: string | ThemeColor; + top: ThemableDecorationAttachmentRenderOptions; +} + +export interface NotebookEditorDecorationType { + readonly key: string; + dispose(): void; +} + export namespace notebook { export function registerNotebookContentProvider( notebookType: string, provider: NotebookContentProvider, - options?: { - /** - * Controls if outputs change will trigger notebook document content change and if it will be used in the diff editor - * Default to false. If the content provider doesn't persisit the outputs in the file document, this should be set to true. - */ - transientOutputs: boolean; + options?: NotebookDocumentContentOptions & { /** - * Controls if a meetadata property change will trigger notebook document content change and if it will be used in the diff editor - * Default to false. If the content provider doesn't persisit a metadata property in the file document, it should be set to true. + * Not ready for production or development use yet. */ - transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; + viewOptions?: { + displayName: string; + filenamePattern: NotebookFilenamePattern[]; + exclusive?: boolean; + }; } ): Disposable; @@ -637,6 +683,9 @@ export namespace notebook { provider: NotebookKernelProvider ): Disposable; + export function createNotebookEditorDecorationType( + options: NotebookDecorationRenderOptions + ): NotebookEditorDecorationType; export const onDidOpenNotebookDocument: Event; export const onDidCloseNotebookDocument: Event; export const onDidSaveNotebookDocument: Event; @@ -653,6 +702,7 @@ export namespace notebook { export const onDidChangeActiveNotebookEditor: Event; export const onDidChangeNotebookEditorSelection: Event; export const onDidChangeNotebookEditorVisibleRanges: Event; + export const onDidChangeNotebookDocumentMetadata: Event; export const onDidChangeNotebookCells: Event; export const onDidChangeCellOutputs: Event; export const onDidChangeCellLanguage: Event; diff --git a/types/vscode.proposed.d.ts b/types/vscode.proposed.d.ts index 9f80ab43e6df..2918de77cad6 100644 --- a/types/vscode.proposed.d.ts +++ b/types/vscode.proposed.d.ts @@ -3,6 +3,10 @@ // Copy nb section from https://github.com/microsoft/vscode/blob/master/src/vs/vscode.proposed.d.ts. declare module 'vscode' { + //#region @rebornix: Notebook + + //#region @rebornix: Notebook + export enum CellKind { Markdown = 1, Code = 2 @@ -146,6 +150,7 @@ declare module 'vscode' { } export interface NotebookCell { + readonly index: number; readonly notebook: NotebookDocument; readonly uri: Uri; readonly cellKind: CellKind; @@ -199,6 +204,20 @@ declare module 'vscode' { runState?: NotebookRunState; } + export interface NotebookDocumentContentOptions { + /** + * Controls if outputs change will trigger notebook document content change and if it will be used in the diff editor + * Default to false. If the content provider doesn't persisit the outputs in the file document, this should be set to true. + */ + transientOutputs: boolean; + + /** + * Controls if a meetadata property change will trigger notebook document content change and if it will be used in the diff editor + * Default to false. If the content provider doesn't persisit a metadata property in the file document, it should be set to true. + */ + transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; + } + export interface NotebookDocument { readonly uri: Uri; readonly version: number; @@ -207,6 +226,7 @@ declare module 'vscode' { readonly isDirty: boolean; readonly isUntitled: boolean; readonly cells: ReadonlyArray; + readonly contentOptions: NotebookDocumentContentOptions; languages: string[]; metadata: NotebookDocumentMetadata; } @@ -231,15 +251,21 @@ declare module 'vscode' { } export interface WorkspaceEdit { - replaceCells( + replaceNotebookMetadata(uri: Uri, value: NotebookDocumentMetadata): void; + replaceNotebookCells( uri: Uri, start: number, end: number, cells: NotebookCellData[], metadata?: WorkspaceEditEntryMetadata ): void; - replaceCellOutput(uri: Uri, index: number, outputs: CellOutput[], metadata?: WorkspaceEditEntryMetadata): void; - replaceCellMetadata( + replaceNotebookCellOutput( + uri: Uri, + index: number, + outputs: CellOutput[], + metadata?: WorkspaceEditEntryMetadata + ): void; + replaceNotebookCellMetadata( uri: Uri, index: number, cellMetadata: NotebookCellMetadata, @@ -247,26 +273,18 @@ declare module 'vscode' { ): void; } - export interface NotebookEditorCellEdit { + export interface NotebookEditorEdit { + replaceMetadata(value: NotebookDocumentMetadata): void; replaceCells(start: number, end: number, cells: NotebookCellData[]): void; - replaceOutput(index: number, outputs: CellOutput[]): void; - replaceMetadata(index: number, metadata: NotebookCellMetadata): void; - - /** @deprecated */ - insert( - index: number, - content: string | string[], - language: string, - type: CellKind, - outputs: CellOutput[], - metadata: NotebookCellMetadata | undefined - ): void; - /** @deprecated */ - delete(index: number): void; + replaceCellOutput(index: number, outputs: CellOutput[]): void; + replaceCellMetadata(index: number, metadata: NotebookCellMetadata): void; } export interface NotebookCellRange { readonly start: number; + /** + * exclusive + */ readonly end: number; } @@ -345,7 +363,19 @@ declare module 'vscode' { */ asWebviewUri(localResource: Uri): Uri; - edit(callback: (editBuilder: NotebookEditorCellEdit) => void): Thenable; + /** + * Perform an edit on the notebook associated with this notebook editor. + * + * The given callback-function is invoked with an [edit-builder](#NotebookEditorEdit) which must + * be used to make edits. Note that the edit-builder is only valid while the + * callback executes. + * + * @param callback A function which can create edits using an [edit-builder](#NotebookEditorEdit). + * @return A promise that resolves with a value indicating if the edits could be applied. + */ + edit(callback: (editBuilder: NotebookEditorEdit) => void): Thenable; + + setDecorations(decorationType: NotebookEditorDecorationType, range: NotebookCellRange): void; revealRange(range: NotebookCellRange, revealType?: NotebookEditorRevealType): void; } @@ -360,6 +390,10 @@ declare module 'vscode' { outputId: string; } + export interface NotebookDocumentMetadataChangeEvent { + readonly document: NotebookDocument; + } + export interface NotebookCellsChangeData { readonly start: number; readonly deletedCount: number; @@ -527,6 +561,10 @@ declare module 'vscode' { } export interface NotebookContentProvider { + readonly options?: NotebookDocumentContentOptions; + readonly onDidChangeNotebookContentOptions?: Event; + readonly onDidChangeNotebook: Event; + /** * Content providers should always use [file system providers](#FileSystemProvider) to * resolve the raw content for `uri` as the resouce is not necessarily a file on disk. @@ -535,7 +573,6 @@ declare module 'vscode' { resolveNotebook(document: NotebookDocument, webview: NotebookCommunication): Promise; saveNotebook(document: NotebookDocument, cancellation: CancellationToken): Promise; saveNotebookAs(targetResource: Uri, document: NotebookDocument, cancellation: CancellationToken): Promise; - readonly onDidChangeNotebook: Event; backupNotebook( document: NotebookDocument, context: NotebookDocumentBackupContext, @@ -556,9 +593,11 @@ declare module 'vscode' { cancelAllCellsExecution(document: NotebookDocument): void; } + export type NotebookFilenamePattern = GlobPattern | { include: GlobPattern; exclude: GlobPattern }; + export interface NotebookDocumentFilter { viewType?: string | string[]; - filenamePattern?: GlobPattern | { include: GlobPattern; exclude: GlobPattern }; + filenamePattern?: NotebookFilenamePattern; } export interface NotebookKernelProvider { @@ -600,21 +639,30 @@ declare module 'vscode' { dispose(): void; } + export interface NotebookDecorationRenderOptions { + backgroundColor?: string | ThemeColor; + borderColor?: string | ThemeColor; + top: ThemableDecorationAttachmentRenderOptions; + } + + export interface NotebookEditorDecorationType { + readonly key: string; + dispose(): void; + } + export namespace notebook { export function registerNotebookContentProvider( notebookType: string, provider: NotebookContentProvider, - options?: { - /** - * Controls if outputs change will trigger notebook document content change and if it will be used in the diff editor - * Default to false. If the content provider doesn't persisit the outputs in the file document, this should be set to true. - */ - transientOutputs: boolean; + options?: NotebookDocumentContentOptions & { /** - * Controls if a meetadata property change will trigger notebook document content change and if it will be used in the diff editor - * Default to false. If the content provider doesn't persisit a metadata property in the file document, it should be set to true. + * Not ready for production or development use yet. */ - transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; + viewOptions?: { + displayName: string; + filenamePattern: NotebookFilenamePattern[]; + exclusive?: boolean; + }; } ): Disposable; @@ -623,6 +671,9 @@ declare module 'vscode' { provider: NotebookKernelProvider ): Disposable; + export function createNotebookEditorDecorationType( + options: NotebookDecorationRenderOptions + ): NotebookEditorDecorationType; export const onDidOpenNotebookDocument: Event; export const onDidCloseNotebookDocument: Event; export const onDidSaveNotebookDocument: Event; @@ -639,6 +690,7 @@ declare module 'vscode' { export const onDidChangeActiveNotebookEditor: Event; export const onDidChangeNotebookEditorSelection: Event; export const onDidChangeNotebookEditorVisibleRanges: Event; + export const onDidChangeNotebookDocumentMetadata: Event; export const onDidChangeNotebookCells: Event; export const onDidChangeCellOutputs: Event; export const onDidChangeCellLanguage: Event; diff --git a/typings/vscode-proposed/index.d.ts b/typings/vscode-proposed/index.d.ts index 2abb5824d26f..76648e3bfc73 100644 --- a/typings/vscode-proposed/index.d.ts +++ b/typings/vscode-proposed/index.d.ts @@ -17,6 +17,8 @@ import { } from 'vscode'; // Copy nb section from https://github.com/microsoft/vscode/blob/master/src/vs/vscode.proposed.d.ts. +//#region @rebornix: Notebook + export enum CellKind { Markdown = 1, Code = 2 @@ -160,6 +162,7 @@ export interface NotebookCellMetadata { } export interface NotebookCell { + readonly index: number; readonly notebook: NotebookDocument; readonly uri: Uri; readonly cellKind: CellKind; @@ -213,6 +216,20 @@ export interface NotebookDocumentMetadata { runState?: NotebookRunState; } +export interface NotebookDocumentContentOptions { + /** + * Controls if outputs change will trigger notebook document content change and if it will be used in the diff editor + * Default to false. If the content provider doesn't persisit the outputs in the file document, this should be set to true. + */ + transientOutputs: boolean; + + /** + * Controls if a meetadata property change will trigger notebook document content change and if it will be used in the diff editor + * Default to false. If the content provider doesn't persisit a metadata property in the file document, it should be set to true. + */ + transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; +} + export interface NotebookDocument { readonly uri: Uri; readonly version: number; @@ -221,6 +238,7 @@ export interface NotebookDocument { readonly isDirty: boolean; readonly isUntitled: boolean; readonly cells: ReadonlyArray; + readonly contentOptions: NotebookDocumentContentOptions; languages: string[]; metadata: NotebookDocumentMetadata; } @@ -245,15 +263,21 @@ export interface NotebookConcatTextDocument { } export interface WorkspaceEdit { - replaceCells( + replaceNotebookMetadata(uri: Uri, value: NotebookDocumentMetadata): void; + replaceNotebookCells( uri: Uri, start: number, end: number, cells: NotebookCellData[], metadata?: WorkspaceEditEntryMetadata ): void; - replaceCellOutput(uri: Uri, index: number, outputs: CellOutput[], metadata?: WorkspaceEditEntryMetadata): void; - replaceCellMetadata( + replaceNotebookCellOutput( + uri: Uri, + index: number, + outputs: CellOutput[], + metadata?: WorkspaceEditEntryMetadata + ): void; + replaceNotebookCellMetadata( uri: Uri, index: number, cellMetadata: NotebookCellMetadata, @@ -261,26 +285,18 @@ export interface WorkspaceEdit { ): void; } -export interface NotebookEditorCellEdit { +export interface NotebookEditorEdit { + replaceMetadata(value: NotebookDocumentMetadata): void; replaceCells(start: number, end: number, cells: NotebookCellData[]): void; - replaceOutput(index: number, outputs: CellOutput[]): void; - replaceMetadata(index: number, metadata: NotebookCellMetadata): void; - - /** @deprecated */ - insert( - index: number, - content: string | string[], - language: string, - type: CellKind, - outputs: CellOutput[], - metadata: NotebookCellMetadata | undefined - ): void; - /** @deprecated */ - delete(index: number): void; + replaceCellOutput(index: number, outputs: CellOutput[]): void; + replaceCellMetadata(index: number, metadata: NotebookCellMetadata): void; } export interface NotebookCellRange { readonly start: number; + /** + * exclusive + */ readonly end: number; } @@ -359,7 +375,19 @@ export interface NotebookEditor { */ asWebviewUri(localResource: Uri): Uri; - edit(callback: (editBuilder: NotebookEditorCellEdit) => void): Thenable; + /** + * Perform an edit on the notebook associated with this notebook editor. + * + * The given callback-function is invoked with an [edit-builder](#NotebookEditorEdit) which must + * be used to make edits. Note that the edit-builder is only valid while the + * callback executes. + * + * @param callback A function which can create edits using an [edit-builder](#NotebookEditorEdit). + * @return A promise that resolves with a value indicating if the edits could be applied. + */ + edit(callback: (editBuilder: NotebookEditorEdit) => void): Thenable; + + setDecorations(decorationType: NotebookEditorDecorationType, range: NotebookCellRange): void; revealRange(range: NotebookCellRange, revealType?: NotebookEditorRevealType): void; } @@ -374,6 +402,10 @@ export interface NotebookRenderRequest { outputId: string; } +export interface NotebookDocumentMetadataChangeEvent { + readonly document: NotebookDocument; +} + export interface NotebookCellsChangeData { readonly start: number; readonly deletedCount: number; @@ -541,6 +573,10 @@ export interface NotebookCommunication { } export interface NotebookContentProvider { + readonly options?: NotebookDocumentContentOptions; + readonly onDidChangeNotebookContentOptions?: Event; + readonly onDidChangeNotebook: Event; + /** * Content providers should always use [file system providers](#FileSystemProvider) to * resolve the raw content for `uri` as the resouce is not necessarily a file on disk. @@ -549,7 +585,6 @@ export interface NotebookContentProvider { resolveNotebook(document: NotebookDocument, webview: NotebookCommunication): Promise; saveNotebook(document: NotebookDocument, cancellation: CancellationToken): Promise; saveNotebookAs(targetResource: Uri, document: NotebookDocument, cancellation: CancellationToken): Promise; - readonly onDidChangeNotebook: Event; backupNotebook( document: NotebookDocument, context: NotebookDocumentBackupContext, @@ -570,9 +605,11 @@ export interface NotebookKernel { cancelAllCellsExecution(document: NotebookDocument): void; } +export type NotebookFilenamePattern = GlobPattern | { include: GlobPattern; exclude: GlobPattern }; + export interface NotebookDocumentFilter { viewType?: string | string[]; - filenamePattern?: GlobPattern | { include: GlobPattern; exclude: GlobPattern }; + filenamePattern?: NotebookFilenamePattern; } export interface NotebookKernelProvider { @@ -614,21 +651,30 @@ export interface NotebookCellStatusBarItem { dispose(): void; } +export interface NotebookDecorationRenderOptions { + backgroundColor?: string | ThemeColor; + borderColor?: string | ThemeColor; + top: ThemableDecorationAttachmentRenderOptions; +} + +export interface NotebookEditorDecorationType { + readonly key: string; + dispose(): void; +} + export namespace notebook { export function registerNotebookContentProvider( notebookType: string, provider: NotebookContentProvider, - options?: { - /** - * Controls if outputs change will trigger notebook document content change and if it will be used in the diff editor - * Default to false. If the content provider doesn't persisit the outputs in the file document, this should be set to true. - */ - transientOutputs: boolean; + options?: NotebookDocumentContentOptions & { /** - * Controls if a meetadata property change will trigger notebook document content change and if it will be used in the diff editor - * Default to false. If the content provider doesn't persisit a metadata property in the file document, it should be set to true. + * Not ready for production or development use yet. */ - transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; + viewOptions?: { + displayName: string; + filenamePattern: NotebookFilenamePattern[]; + exclusive?: boolean; + }; } ): Disposable; @@ -637,6 +683,9 @@ export namespace notebook { provider: NotebookKernelProvider ): Disposable; + export function createNotebookEditorDecorationType( + options: NotebookDecorationRenderOptions + ): NotebookEditorDecorationType; export const onDidOpenNotebookDocument: Event; export const onDidCloseNotebookDocument: Event; export const onDidSaveNotebookDocument: Event; @@ -653,6 +702,7 @@ export namespace notebook { export const onDidChangeActiveNotebookEditor: Event; export const onDidChangeNotebookEditorSelection: Event; export const onDidChangeNotebookEditorVisibleRanges: Event; + export const onDidChangeNotebookDocumentMetadata: Event; export const onDidChangeNotebookCells: Event; export const onDidChangeCellOutputs: Event; export const onDidChangeCellLanguage: Event; From 272bdc73a3bec6de4b78f655afed6b4ed57e68ed Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 13:11:32 -0700 Subject: [PATCH 12/24] Updates --- .../jupyter/kernels/cellExecution.ts | 110 +++++++++--------- .../datascience/jupyter/kernels/kernel.ts | 6 +- .../jupyter/kernels/kernelExecution.ts | 42 +++---- .../notebook/helpers/executionHelpers.ts | 109 +++++++++-------- .../datascience/notebook/helpers/helpers.ts | 84 +++++++------ .../datascience/notebook/notebookEditor.ts | 69 +++++++---- .../notebookStorage/vscNotebookModel.ts | 5 +- src/test/datascience/notebook/helper.ts | 42 ++++--- .../notebook/rendererExension.unit.test.ts | 38 +++++- src/test/mocks/vsc/extHostedTypes.ts | 9 +- types/vscode-proposed/index.d.ts | 8 -- types/vscode.proposed.d.ts | 2 - typings/vscode-proposed/index.d.ts | 9 -- 13 files changed, 298 insertions(+), 235 deletions(-) diff --git a/src/client/datascience/jupyter/kernels/cellExecution.ts b/src/client/datascience/jupyter/kernels/cellExecution.ts index 99e730dfabd1..6b981feaa8c7 100644 --- a/src/client/datascience/jupyter/kernels/cellExecution.ts +++ b/src/client/datascience/jupyter/kernels/cellExecution.ts @@ -5,14 +5,7 @@ import { nbformat } from '@jupyterlab/coreutils'; import type { KernelMessage } from '@jupyterlab/services/lib/kernel/messages'; -import { - CancellationToken, - CellOutputKind, - CellStreamOutput, - NotebookCell, - NotebookCellRunState, - WorkspaceEdit -} from 'vscode'; +import { CancellationToken, CellOutputKind, CellStreamOutput, NotebookCell, NotebookCellRunState } from 'vscode'; import type { NotebookEditor as VSCNotebookEditor } from '../../../../../types/vscode-proposed'; import { concatMultilineString, formatStreamText } from '../../../../datascience-ui/common'; import { IApplicationShell, IVSCodeNotebook } from '../../../common/application/types'; @@ -97,6 +90,7 @@ export class CellExecution { private started?: boolean; private _completed?: boolean; + private readonly initPromise: Promise; private constructor( public readonly editor: VSCNotebookEditor, @@ -107,7 +101,7 @@ export class CellExecution { private readonly applicationService: IApplicationShell ) { this.oldCellRunState = cell.metadata.runState; - this.enqueue(); + this.initPromise = this.enqueue(); } public static fromCell( @@ -121,13 +115,16 @@ export class CellExecution { return new CellExecution(editor, cell, contentProvider, errorHandler, editorProvider, appService); } - public start(kernelPromise: Promise, notebook: INotebook) { + public async start(kernelPromise: Promise, notebook: INotebook) { + await this.initPromise; this.started = true; // Ensure we clear the cell state and trigger a change. - clearCellForExecution(this.cell); - new WorkspaceEdit().replaceCellMetadata(this.cell.notebook.uri, this.cell.notebook.cells.indexOf(this.cell), { - ...this.cell.metadata, - runStartTime: new Date().getTime() + await clearCellForExecution(this.editor, this.cell); + await this.editor.edit((edit) => { + edit.replaceCellMetadata(this.cell.notebook.cells.indexOf(this.cell), { + ...this.cell.metadata, + runStartTime: new Date().getTime() + }); }); this.stopWatch.reset(); // Changes to metadata must be saved in ipynb, hence mark doc has dirty. @@ -136,19 +133,16 @@ export class CellExecution { // Begin the request that will modify our cell. kernelPromise - .then((_k) => { - this.execute(notebook.session, notebook.getLoggers()); - }) - .catch((e) => { - this.completedWithErrors(e); - }); + .then((_k) => this.execute(notebook.session, notebook.getLoggers())) + .catch((e) => this.completedWithErrors(e)); } /** * Cancel execution. * If execution has commenced, then interrupt (via cancellation token) else dequeue from execution. */ - public cancel() { + public async cancel() { + await this.initPromise; // We need to notify cancellation only if execution is in progress, // coz if not, we can safely reset the states. if (this.started && !this._completed) { @@ -156,18 +150,20 @@ export class CellExecution { } if (!this.started) { - this.dequeue(); + await this.dequeue(); } this._result.resolve(this.cell.metadata.runState); } - private completedWithErrors(error: Partial) { + private async completedWithErrors(error: Partial) { this.sendPerceivedCellExecute(); - new WorkspaceEdit().replaceCellMetadata(this.cell.notebook.uri, this.cell.notebook.cells.indexOf(this.cell), { - ...this.cell.metadata, - lastRunDuration: this.stopWatch.elapsedTime - }); - updateCellWithErrorStatus(this.cell, error); + await this.editor.edit((edit) => + edit.replaceCellMetadata(this.cell.notebook.cells.indexOf(this.cell), { + ...this.cell.metadata, + lastRunDuration: this.stopWatch.elapsedTime + }) + ); + await updateCellWithErrorStatus(this.editor, this.cell, error); this.contentProvider.notifyChangesToDocument(this.cell.notebook); this.errorHandler.handleError((error as unknown) as Error).ignoreErrors(); @@ -177,7 +173,7 @@ export class CellExecution { this.contentProvider.notifyChangesToDocument(this.cell.notebook); } - private completedSuccessfully() { + private async completedSuccessfully() { this.sendPerceivedCellExecute(); let statusMessage = ''; // If we requested a cancellation, then assume it did not even run. @@ -186,7 +182,7 @@ export class CellExecution { ? vscodeNotebookEnums.NotebookCellRunState.Idle : vscodeNotebookEnums.NotebookCellRunState.Success; - updateCellExecutionTimes(this.cell, { + await updateCellExecutionTimes(this.editor, this.cell, { startTime: this.cell.metadata.runStartTime, lastRunDuration: this.stopWatch.elapsedTime, duration: this.cell.metadata.lastRunDuration @@ -199,11 +195,13 @@ export class CellExecution { } const cellIndex = this.editor.document.cells.indexOf(this.cell); - new WorkspaceEdit().replaceCellMetadata(this.cell.notebook.uri, cellIndex, { - ...this.cell.metadata, - runState, - statusMessage - }); + await this.editor.edit((edit) => + edit.replaceCellMetadata(cellIndex, { + ...this.cell.metadata, + runState, + statusMessage + }) + ); this._completed = true; this._result.resolve(this.cell.metadata.runState); @@ -229,17 +227,19 @@ export class CellExecution { * This cell will no longer be processed for execution (even though it was meant to be). * At this point we revert cell state & indicate that it has nto started & it is not busy. */ - private dequeue() { + private async dequeue() { const runState = this.oldCellRunState === vscodeNotebookEnums.NotebookCellRunState.Running ? vscodeNotebookEnums.NotebookCellRunState.Idle : this.oldCellRunState; this.cell.metadata.runStartTime = undefined; - new WorkspaceEdit().replaceCellMetadata(this.cell.notebook.uri, this.cell.notebook.cells.indexOf(this.cell), { - ...this.cell.metadata, - runStartTime: undefined, - runState - }); + await this.editor.edit((edit) => + edit.replaceCellMetadata(this.cell.notebook.cells.indexOf(this.cell), { + ...this.cell.metadata, + runStartTime: undefined, + runState + }) + ); this._completed = true; this._result.resolve(this.cell.metadata.runState); // Changes to metadata must be saved in ipynb, hence mark doc has dirty. @@ -250,11 +250,13 @@ export class CellExecution { * Place in queue for execution with kernel. * (mark it as busy). */ - private enqueue() { - new WorkspaceEdit().replaceCellMetadata(this.cell.notebook.uri, this.cell.notebook.cells.indexOf(this.cell), { - ...this.cell.metadata, - runState: vscodeNotebookEnums.NotebookCellRunState.Running - }); + private async enqueue() { + await this.editor.edit((edit) => + edit.replaceCellMetadata(this.cell.notebook.cells.indexOf(this.cell), { + ...this.cell.metadata, + runState: vscodeNotebookEnums.NotebookCellRunState.Running + }) + ); this.contentProvider.notifyChangesToDocument(this.cell.notebook); } @@ -314,13 +316,13 @@ export class CellExecution { // When the request finishes we are done request.done .then(() => this.completedSuccessfully()) - .catch((e) => { + .catch(async (e) => { // @jupyterlab/services throws a `Canceled` error when the kernel is interrupted. // Such an error must be ignored. if (e && e instanceof Error && e.message === 'Canceled') { - this.completedSuccessfully(); + await this.completedSuccessfully(); } else { - this.completedWithErrors(e); + await this.completedWithErrors(e); } }) .finally(() => { @@ -328,10 +330,10 @@ export class CellExecution { }) .ignoreErrors(); } else { - this.completedWithErrors(new Error('Session cannot generate requrests')); + this.completedWithErrors(new Error('Session cannot generate requrests')).then(noop, noop); } } else { - this.completedSuccessfully(); + this.completedSuccessfully().then(noop, noop); } } @@ -386,9 +388,7 @@ export class CellExecution { // Set execution count, all messages should have it if ('execution_count' in msg.content && typeof msg.content.execution_count === 'number') { - if (updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count)) { - shouldUpdate = true; - } + updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count).then(noop, noop); } // Show our update if any new output. @@ -397,7 +397,7 @@ export class CellExecution { } } catch (err) { // If not a restart error, then tell the subscriber - this.completedWithErrors(err); + this.completedWithErrors(err).then(noop, noop); } } diff --git a/src/client/datascience/jupyter/kernels/kernel.ts b/src/client/datascience/jupyter/kernels/kernel.ts index 459b36f57ac9..db8918cd818a 100644 --- a/src/client/datascience/jupyter/kernels/kernel.ts +++ b/src/client/datascience/jupyter/kernels/kernel.ts @@ -109,11 +109,11 @@ export class Kernel implements IKernel { await this.start({ disableUI: false, token: this.startCancellation.token }); await this.kernelExecution.executeAllCells(document); } - public cancelCell(cell: NotebookCell) { + public async cancelCell(cell: NotebookCell) { this.startCancellation.cancel(); - this.kernelExecution.cancelCell(cell); + await this.kernelExecution.cancelCell(cell); } - public cancelAllCells(document: NotebookDocument) { + public async cancelAllCells(document: NotebookDocument) { this.startCancellation.cancel(); this.kernelExecution.cancelAllCells(document); } diff --git a/src/client/datascience/jupyter/kernels/kernelExecution.ts b/src/client/datascience/jupyter/kernels/kernelExecution.ts index 98e96105994b..f5fd0215bb06 100644 --- a/src/client/datascience/jupyter/kernels/kernelExecution.ts +++ b/src/client/datascience/jupyter/kernels/kernelExecution.ts @@ -105,35 +105,29 @@ export class KernelExecution implements IDisposable { ); try { - let executingAPreviousCellHasFailed = false; - await codeCellsToExecute.reduce( - (previousPromise, cellToExecute) => - previousPromise.then((previousCellState) => { - // If a previous cell has failed or execution cancelled, the get out. - if ( - executingAPreviousCellHasFailed || - cancelTokenSource.token.isCancellationRequested || - previousCellState === vscodeNotebookEnums.NotebookCellRunState.Error - ) { - executingAPreviousCellHasFailed = true; - codeCellsToExecute.forEach((cell) => cell.cancel()); // Cancel pending cells. - return; - } - const result = this.executeIndividualCell(kernel, cellToExecute); - result.finally(() => this.cellExecutions.delete(cellToExecute.cell)).catch(noop); - return result; - }), - Promise.resolve(undefined) - ); + for (const cellToExecute of codeCellsToExecute) { + const result = this.executeIndividualCell(kernel, cellToExecute); + result.finally(() => this.cellExecutions.delete(cellToExecute.cell)).catch(noop); + const executionResult = await result; + // If a cell has failed or execution cancelled, the get out. + if ( + cancelTokenSource.token.isCancellationRequested || + executionResult === vscodeNotebookEnums.NotebookCellRunState.Error + ) { + await Promise.all(codeCellsToExecute.map((cell) => cell.cancel())); // Cancel pending cells. + break; + } + } } finally { + await Promise.all(codeCellsToExecute.map((cell) => cell.cancel())); // Cancel pending cells. this.documentExecutions.delete(document); document.metadata.runState = vscodeNotebookEnums.NotebookRunState.Idle; } } - public cancelCell(cell: NotebookCell): void { + public async cancelCell(cell: NotebookCell) { if (this.cellExecutions.get(cell)) { - this.cellExecutions.get(cell)!.cancel(); + await this.cellExecutions.get(cell)!.cancel(); } } @@ -167,12 +161,12 @@ export class KernelExecution implements IDisposable { return kernel; } - private onIoPubMessage(document: NotebookDocument, msg: KernelMessage.IIOPubMessage) { + private async onIoPubMessage(document: NotebookDocument, msg: KernelMessage.IIOPubMessage) { // tslint:disable-next-line:no-require-imports 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 (handleUpdateDisplayDataMessage(msg, editor)) { + if (await handleUpdateDisplayDataMessage(msg, editor)) { this.contentProvider.notifyChangesToDocument(document); } } diff --git a/src/client/datascience/notebook/helpers/executionHelpers.ts b/src/client/datascience/notebook/helpers/executionHelpers.ts index fff9abae28cd..366348d85dfb 100644 --- a/src/client/datascience/notebook/helpers/executionHelpers.ts +++ b/src/client/datascience/notebook/helpers/executionHelpers.ts @@ -6,7 +6,6 @@ import type { nbformat } from '@jupyterlab/coreutils'; import type { KernelMessage } from '@jupyterlab/services'; import * as fastDeepEqual from 'fast-deep-equal'; -import { WorkspaceEdit } from 'vscode'; import type { NotebookCell, NotebookEditor } from '../../../../../types/vscode-proposed'; import { createErrorOutput } from '../../../../datascience-ui/common/cellFactory'; import { createIOutputFromCellOutputs, createVSCCellOutputsFromOutputs, translateErrorOutput } from './helpers'; @@ -19,73 +18,84 @@ const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed' * Notebook model is what we use to update/track changes to ipynb. * @returns {boolean} Returns `true` if output has changed. */ -export function handleUpdateDisplayDataMessage( +export async function handleUpdateDisplayDataMessage( msg: KernelMessage.IUpdateDisplayDataMsg, editor: NotebookEditor -): boolean { +): Promise { const document = editor.document; + let updated = false; // Find any cells that have this same display_id - return ( - document.cells.filter((cellToCheck, index) => { - if (cellToCheck.cellKind !== vscodeNotebookEnums.CellKind.Code) { - return false; - } - - let updated = false; - const outputs = createIOutputFromCellOutputs(cellToCheck.outputs); - const changedOutputs = outputs.map((output) => { - if ( - (output.output_type === 'display_data' || output.output_type === 'execute_result') && - output.transient && - // tslint:disable-next-line: no-any - (output.transient as any).display_id === msg.content.transient.display_id - ) { - // Remember we have updated output for this cell. - updated = true; + for (const cell of document.cells) { + if (cell.cellKind !== vscodeNotebookEnums.CellKind.Code) { + return false; + } - return { - ...output, - data: msg.content.data, - metadata: msg.content.metadata - }; - } else { - return output; - } - }); + const outputs = createIOutputFromCellOutputs(cell.outputs); + const changedOutputs = outputs.map((output) => { + if ( + (output.output_type === 'display_data' || output.output_type === 'execute_result') && + output.transient && + // tslint:disable-next-line: no-any + (output.transient as any).display_id === msg.content.transient.display_id + ) { + // Remember we have updated output for this cell. + updated = true; - if (!updated) { - return false; + return { + ...output, + data: msg.content.data, + metadata: msg.content.metadata + }; + } else { + return output; } + }); + + if (!updated) { + continue; + } + + await updateCellOutput(editor, cell, changedOutputs); + updated = true; + } - const vscCell = document.cells[index]; - updateCellOutput(vscCell, changedOutputs); - return true; - }).length > 0 - ); + return updated; } /** * Updates the VSC cell with the error output. */ -export function updateCellWithErrorStatus(cell: NotebookCell, ex: Partial) { +export async function updateCellWithErrorStatus( + notebookEditor: NotebookEditor, + cell: NotebookCell, + ex: Partial +) { const cellIndex = cell.notebook.cells.indexOf(cell); - new WorkspaceEdit().replaceCellMetadata(cell.document.uri, cellIndex, { - ...cell.metadata, - runState: vscodeNotebookEnums.NotebookCellRunState.Error + await notebookEditor.edit((edit) => { + edit.replaceCellMetadata(cellIndex, { + ...cell.metadata, + runState: vscodeNotebookEnums.NotebookCellRunState.Error + }); + edit.replaceCellOutput(cellIndex, [translateErrorOutput(createErrorOutput(ex))]); }); - new WorkspaceEdit().replaceCellOutput(cell.document.uri, cellIndex, [translateErrorOutput(createErrorOutput(ex))]); } /** * @returns {boolean} Returns `true` if execution count has changed. */ -export function updateCellExecutionCount(editor: NotebookEditor, cell: NotebookCell, executionCount: number): boolean { +export async function updateCellExecutionCount( + editor: NotebookEditor, + cell: NotebookCell, + executionCount: number +): Promise { if (cell.metadata.executionOrder !== executionCount && executionCount) { const cellIndex = editor.document.cells.indexOf(cell); - new WorkspaceEdit().replaceCellMetadata(cell.document.uri, cellIndex, { - ...cell.metadata, - executionOrder: executionCount - }); + await editor.edit((edit) => + edit.replaceCellMetadata(cellIndex, { + ...cell.metadata, + executionOrder: executionCount + }) + ); return true; } return false; @@ -94,10 +104,8 @@ export function updateCellExecutionCount(editor: NotebookEditor, cell: NotebookC /** * Updates our Cell Model with the cell output. * As we execute a cell we get output from jupyter. This code will ensure the cell is updated with the output. - * Here we update both the VSCode Cell as well as our ICell (cell in our INotebookModel). - * @returns {(boolean | undefined)} Returns `true` if output has changed. */ -export function updateCellOutput(cell: NotebookCell, outputs: nbformat.IOutput[]): boolean | undefined { +export async function updateCellOutput(editor: NotebookEditor, cell: NotebookCell, outputs: nbformat.IOutput[]) { const newOutput = createVSCCellOutputsFromOutputs(outputs); // If there was no output and still no output, then nothing to do. if (cell.outputs.length === 0 && newOutput.length === 0) { @@ -109,6 +117,5 @@ export function updateCellOutput(cell: NotebookCell, outputs: nbformat.IOutput[] return; } const cellIndex = cell.notebook.cells.indexOf(cell); - new WorkspaceEdit().replaceCellOutput(cell.document.uri, cellIndex, newOutput); - return true; + await editor.edit((edit) => edit.replaceCellOutput(cellIndex, newOutput)); } diff --git a/src/client/datascience/notebook/helpers/helpers.ts b/src/client/datascience/notebook/helpers/helpers.ts index d601a312ce63..8010da9ebb48 100644 --- a/src/client/datascience/notebook/helpers/helpers.ts +++ b/src/client/datascience/notebook/helpers/helpers.ts @@ -13,7 +13,8 @@ import type { NotebookCellData, NotebookCellMetadata, NotebookData, - NotebookDocument + NotebookDocument, + NotebookEditor } from 'vscode-proposed'; import { NotebookCellRunState } from '../../../../../typings/vscode-proposed'; import { concatMultilineString, splitMultilineString } from '../../../../datascience-ui/common'; @@ -27,7 +28,6 @@ import { JupyterNotebookView } from '../constants'; const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed'); // tslint:disable-next-line: no-require-imports import cloneDeep = require('lodash/cloneDeep'); -import { WorkspaceEdit } from 'vscode'; import { isUntitledFile } from '../../../common/utils/misc'; import { KernelConnectionMetadata } from '../../jupyter/kernels/types'; import { updateNotebookMetadata } from '../../notebookStorage/baseModel'; @@ -339,25 +339,27 @@ export function createIOutputFromCellOutputs(cellOutputs: CellOutput[]): nbforma .map((output) => output!!); } -export function clearCellForExecution(cell: NotebookCell) { +export async function clearCellForExecution(editor: NotebookEditor, cell: NotebookCell) { const cellIndex = cell.notebook.cells.indexOf(cell); - new WorkspaceEdit().replaceCellMetadata(cell.notebook.uri, cellIndex, { - ...cell.metadata, - statusMessage: undefined, - executionOrder: undefined, - lastRunDuration: undefined, - runStartTime: undefined + await editor.edit((edit) => { + edit.replaceCellMetadata(cellIndex, { + ...cell.metadata, + statusMessage: undefined, + executionOrder: undefined, + lastRunDuration: undefined, + runStartTime: undefined + }); + edit.replaceCellOutput(cellIndex, []); }); - new WorkspaceEdit().replaceCellOutput(cell.notebook.uri, cellIndex, []); - - updateCellExecutionTimes(cell); + await updateCellExecutionTimes(editor, cell); } /** * Store execution start and end times. * Stored as ISO for portability. */ -export function updateCellExecutionTimes( +export async function updateCellExecutionTimes( + editor: NotebookEditor, cell: NotebookCell, times?: { startTime?: number; duration?: number; lastRunDuration?: number } ) { @@ -375,7 +377,7 @@ export function updateCellExecutionTimes( updated = true; } if (updated) { - new WorkspaceEdit().replaceCellMetadata(cell.notebook.uri, cellIndex, { ...cellMetadata }); + await editor.edit((edit) => edit.replaceCellMetadata(cellIndex, { ...cellMetadata })); } return; } @@ -388,11 +390,13 @@ export function updateCellExecutionTimes( customMetadata.metadata.vscode.end_execution_time = endTimeISO; customMetadata.metadata.vscode.start_execution_time = startTimeISO; const lastRunDuration = times.lastRunDuration ?? cell.metadata.lastRunDuration; - new WorkspaceEdit().replaceCellMetadata(cell.notebook.uri, cellIndex, { - ...cell.metadata, - custom: customMetadata, - lastRunDuration - }); + await editor.edit((edit) => + edit.replaceCellMetadata(cellIndex, { + ...cell.metadata, + custom: customMetadata, + lastRunDuration + }) + ); } function createCodeCellFromVSCNotebookCell(cell: NotebookCell): nbformat.ICodeCell { @@ -663,7 +667,11 @@ export function getCellStatusMessageBasedOnFirstCellErrorOutput(outputs?: CellOu /** * Updates a notebook document as a result of trusting it. */ -export function updateVSCNotebookAfterTrustingNotebook(document: NotebookDocument, originalCells: ICell[]) { +export async function updateVSCNotebookAfterTrustingNotebook( + editor: NotebookEditor, + document: NotebookDocument, + originalCells: ICell[] +) { const areAllCellsEditableAndRunnable = document.cells.every((cell) => { if (cell.cellKind === vscodeNotebookEnums.CellKind.Markdown) { return cell.metadata.editable; @@ -687,23 +695,23 @@ export function updateVSCNotebookAfterTrustingNotebook(document: NotebookDocumen document.metadata.editable = true; document.metadata.runnable = true; - const workspaceEdit = new WorkspaceEdit(); - document.cells.forEach((cell, index) => { - if (cell.cellKind === vscodeNotebookEnums.CellKind.Markdown) { - workspaceEdit.replaceCellMetadata(document.uri, index, { ...cell.metadata, editable: true }); - } else { - workspaceEdit.replaceCellMetadata(document.uri, index, { - ...cell.metadata, - editable: true, - runnable: true - }); - // Restore the output once we trust the notebook. - workspaceEdit.replaceCellOutput( - document.uri, - index, - // tslint:disable-next-line: no-any - createVSCCellOutputsFromOutputs(originalCells[index].data.outputs as any) - ); - } + await editor.edit((edit) => { + document.cells.forEach((cell, index) => { + if (cell.cellKind === vscodeNotebookEnums.CellKind.Markdown) { + edit.replaceCellMetadata(index, { ...cell.metadata, editable: true }); + } else { + edit.replaceCellMetadata(index, { + ...cell.metadata, + editable: true, + runnable: true + }); + // Restore the output once we trust the notebook. + edit.replaceCellOutput( + index, + // tslint:disable-next-line: no-any + createVSCCellOutputsFromOutputs(originalCells[index].data.outputs as any) + ); + } + }); }); } diff --git a/src/client/datascience/notebook/notebookEditor.ts b/src/client/datascience/notebook/notebookEditor.ts index bb48303c9b15..65bfaf7a1b90 100644 --- a/src/client/datascience/notebook/notebookEditor.ts +++ b/src/client/datascience/notebook/notebookEditor.ts @@ -3,7 +3,7 @@ 'use strict'; -import { ConfigurationTarget, Event, EventEmitter, Uri, WebviewPanel, WorkspaceEdit } from 'vscode'; +import { ConfigurationTarget, Event, EventEmitter, Uri, WebviewPanel } from 'vscode'; import type { NotebookCell, NotebookDocument } from 'vscode-proposed'; import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../common/application/types'; import { traceError } from '../../common/logger'; @@ -133,41 +133,62 @@ export class NotebookEditor implements INotebookEditor { return; } const defaultLanguage = getDefaultCodeLanguage(this.model); - new WorkspaceEdit().replaceCells(this.document.uri, 0, this.document.cells.length - 1, [ - { - cellKind: vscodeNotebookEnums.CellKind.Code, - language: defaultLanguage, - metadata: {}, - outputs: [], - source: '' - } - ]); + const editor = this.vscodeNotebook.notebookEditors.find((item) => item.document === this.document); + if (editor) { + editor + .edit((edit) => + edit.replaceCells(0, this.document.cells.length - 1, [ + { + cellKind: vscodeNotebookEnums.CellKind.Code, + language: defaultLanguage, + metadata: {}, + outputs: [], + source: '' + } + ]) + ) + .then(noop, noop); + } } public expandAllCells(): void { if (!this.vscodeNotebook.activeNotebookEditor) { return; } const notebook = this.vscodeNotebook.activeNotebookEditor.document; - notebook.cells.forEach((cell, index) => { - new WorkspaceEdit().replaceCellMetadata(notebook.uri, index, { - ...cell.metadata, - inputCollapsed: false, - outputCollapsed: false - }); - }); + const editor = this.vscodeNotebook.notebookEditors.find((item) => item.document === this.document); + if (editor) { + editor + .edit((edit) => { + notebook.cells.forEach((cell, index) => { + edit.replaceCellMetadata(index, { + ...cell.metadata, + inputCollapsed: false, + outputCollapsed: false + }); + }); + }) + .then(noop, noop); + } } public collapseAllCells(): void { if (!this.vscodeNotebook.activeNotebookEditor) { return; } const notebook = this.vscodeNotebook.activeNotebookEditor.document; - notebook.cells.forEach((cell, index) => { - new WorkspaceEdit().replaceCellMetadata(notebook.uri, index, { - ...cell.metadata, - inputCollapsed: true, - outputCollapsed: true - }); - }); + const editor = this.vscodeNotebook.notebookEditors.find((item) => item.document === this.document); + if (editor) { + editor + .edit((edit) => { + notebook.cells.forEach((cell, index) => { + edit.replaceCellMetadata(index, { + ...cell.metadata, + inputCollapsed: true, + outputCollapsed: true + }); + }); + }) + .then(noop, noop); + } } public notifyExecution(cell: NotebookCell) { this._executed.fire(this); diff --git a/src/client/datascience/notebookStorage/vscNotebookModel.ts b/src/client/datascience/notebookStorage/vscNotebookModel.ts index 2d1ab7563406..cae34b736e51 100644 --- a/src/client/datascience/notebookStorage/vscNotebookModel.ts +++ b/src/client/datascience/notebookStorage/vscNotebookModel.ts @@ -99,7 +99,10 @@ export class VSCodeNotebookModel extends BaseNotebookModel { public trust() { super.trust(); if (this.document) { - updateVSCNotebookAfterTrustingNotebook(this.document, this._cells); + const editor = this.vscodeNotebook?.notebookEditors.find((item) => item.document === this.document); + if (editor) { + updateVSCNotebookAfterTrustingNotebook(editor, this.document, this._cells); + } // We don't need old cells. this._cells = []; } diff --git a/src/test/datascience/notebook/helper.ts b/src/test/datascience/notebook/helper.ts index 1b50f9c8ffdc..fced9bed281f 100644 --- a/src/test/datascience/notebook/helper.ts +++ b/src/test/datascience/notebook/helper.ts @@ -10,7 +10,7 @@ import * as path from 'path'; import * as sinon from 'sinon'; import * as tmp from 'tmp'; import { instance, mock } from 'ts-mockito'; -import { commands, Memento, TextDocument, Uri, WorkspaceEdit } from 'vscode'; +import { commands, Memento, TextDocument, Uri } from 'vscode'; import { NotebookCell, NotebookDocument } from '../../../../types/vscode-proposed'; import { CellDisplayOutput } from '../../../../typings/vscode-proposed'; import { IApplicationEnvironment, IVSCodeNotebook } from '../../../client/common/application/types'; @@ -50,11 +50,8 @@ export async function insertMarkdownCell(source: string) { assert.fail('No active editor'); return; } - new WorkspaceEdit().replaceCells( - activeEditor.document.uri, - activeEditor.document.cells.length - 1, - activeEditor.document.cells.length - 1, - [ + await activeEditor.edit((edit) => + edit.replaceCells(activeEditor.document.cells.length - 1, activeEditor.document.cells.length - 1, [ { cellKind: vscodeNotebookEnums.CellKind.Markdown, language: MARKDOWN_LANGUAGE, @@ -64,7 +61,7 @@ export async function insertMarkdownCell(source: string) { }, outputs: [] } - ] + ]) ); await waitForCondition( @@ -82,11 +79,8 @@ export async function insertPythonCell(source: string) { assert.fail('No active editor'); return; } - new WorkspaceEdit().replaceCells( - activeEditor.document.uri, - activeEditor.document.cells.length - 1, - activeEditor.document.cells.length - 1, - [ + await activeEditor.edit((edit) => + edit.replaceCells(activeEditor.document.cells.length - 1, activeEditor.document.cells.length - 1, [ { cellKind: vscodeNotebookEnums.CellKind.Code, language: PYTHON_LANGUAGE, @@ -96,7 +90,7 @@ export async function insertPythonCell(source: string) { }, outputs: [] } - ] + ]) ); await waitForCondition( async () => @@ -122,7 +116,7 @@ export async function deleteCell(index: number = 0) { assert.fail('No active editor'); return; } - new WorkspaceEdit().replaceCells(activeEditor.document.uri, index, index, []); + await activeEditor.edit((edit) => edit.replaceCells(index, index, [])); } export async function deleteAllCellsAndWait() { const { vscodeNotebook } = await getServices(); @@ -130,7 +124,7 @@ export async function deleteAllCellsAndWait() { if (!activeEditor || activeEditor.document.cells.length === 0) { return; } - new WorkspaceEdit().replaceCells(activeEditor.document.uri, 0, activeEditor.document.cells.length - 1, []); + await activeEditor.edit((edit) => edit.replaceCells(0, activeEditor.document.cells.length - 1, [])); // Wait for cell to get deleted. await waitForCondition(async () => activeEditor.document.cells.length === 0, 1_000, 'Cell not deleted'); } @@ -454,6 +448,23 @@ export function createNotebookDocument( uri: model.file, isUntitled: false, viewType, + contentOptions: { + transientOutputs: false, + transientMetadata: { + breakpointMargin: true, + editable: true, + hasExecutionOrder: true, + inputCollapsed: true, + lastRunDuration: true, + outputCollapsed: true, + runStartTime: true, + runnable: true, + executionOrder: false, + custom: false, + runState: false, + statusMessage: false + } + }, metadata: { cellEditable: model.isTrusted, cellHasExecutionOrder: true, @@ -470,6 +481,7 @@ export function createNotebookDocument( metadata: vscCell.metadata || {}, uri: model.file.with({ fragment: `cell${index}` }), notebook: doc, + index, document: instance(mock()), outputs: vscCell.outputs }; diff --git a/src/test/datascience/notebook/rendererExension.unit.test.ts b/src/test/datascience/notebook/rendererExension.unit.test.ts index 559da209cd19..20004f99c406 100644 --- a/src/test/datascience/notebook/rendererExension.unit.test.ts +++ b/src/test/datascience/notebook/rendererExension.unit.test.ts @@ -29,7 +29,24 @@ suite('DataScience - NativeNotebook Renderer Extension', () => { languages: [], metadata: {}, isUntitled: false, - viewType: JupyterNotebookView + viewType: JupyterNotebookView, + contentOptions: { + transientOutputs: false, + transientMetadata: { + breakpointMargin: true, + editable: true, + hasExecutionOrder: true, + inputCollapsed: true, + lastRunDuration: true, + outputCollapsed: true, + runStartTime: true, + runnable: true, + executionOrder: false, + custom: false, + runState: false, + statusMessage: false + } + } }; const nonJupyterNotebook: NotebookDocument = { cells: [], @@ -40,7 +57,24 @@ suite('DataScience - NativeNotebook Renderer Extension', () => { isDirty: false, languages: [], metadata: {}, - viewType: 'somethingElse' + viewType: 'somethingElse', + contentOptions: { + transientOutputs: false, + transientMetadata: { + breakpointMargin: true, + editable: true, + hasExecutionOrder: true, + inputCollapsed: true, + lastRunDuration: true, + outputCollapsed: true, + runStartTime: true, + runnable: true, + executionOrder: false, + custom: false, + runState: false, + statusMessage: false + } + } }; const extension: Extension<{}> = { activate: () => Promise.resolve({}), diff --git a/src/test/mocks/vsc/extHostedTypes.ts b/src/test/mocks/vsc/extHostedTypes.ts index 35d5f7b82132..9416be3be6b5 100644 --- a/src/test/mocks/vsc/extHostedTypes.ts +++ b/src/test/mocks/vsc/extHostedTypes.ts @@ -547,7 +547,10 @@ export namespace vscMockExtHostedTypes { } export class WorkspaceEdit implements vscode.WorkspaceEdit { - replaceCells( + replaceNotebookMetadata(_uri: vscode.Uri, _value: vscode.NotebookDocumentMetadata): void{ + // + } + replaceNotebookCells( _uri: vscode.Uri, _start: number, _end: number, @@ -557,7 +560,7 @@ export namespace vscMockExtHostedTypes { // Noop. } - replaceCellOutput( + replaceNotebookCellOutput( _uri: vscode.Uri, _index: number, _outputs: vscode.CellOutput[], @@ -566,7 +569,7 @@ export namespace vscMockExtHostedTypes { // Noop. } - replaceCellMetadata( + replaceNotebookCellMetadata( _uri: vscode.Uri, _index: number, _cellMetadata: vscode.NotebookCellMetadata, diff --git a/types/vscode-proposed/index.d.ts b/types/vscode-proposed/index.d.ts index 76648e3bfc73..e81ab771f432 100644 --- a/types/vscode-proposed/index.d.ts +++ b/types/vscode-proposed/index.d.ts @@ -651,11 +651,6 @@ export interface NotebookCellStatusBarItem { dispose(): void; } -export interface NotebookDecorationRenderOptions { - backgroundColor?: string | ThemeColor; - borderColor?: string | ThemeColor; - top: ThemableDecorationAttachmentRenderOptions; -} export interface NotebookEditorDecorationType { readonly key: string; @@ -683,9 +678,6 @@ export namespace notebook { provider: NotebookKernelProvider ): Disposable; - export function createNotebookEditorDecorationType( - options: NotebookDecorationRenderOptions - ): NotebookEditorDecorationType; export const onDidOpenNotebookDocument: Event; export const onDidCloseNotebookDocument: Event; export const onDidSaveNotebookDocument: Event; diff --git a/types/vscode.proposed.d.ts b/types/vscode.proposed.d.ts index 2918de77cad6..013502d552c8 100644 --- a/types/vscode.proposed.d.ts +++ b/types/vscode.proposed.d.ts @@ -5,8 +5,6 @@ declare module 'vscode' { //#region @rebornix: Notebook - //#region @rebornix: Notebook - export enum CellKind { Markdown = 1, Code = 2 diff --git a/typings/vscode-proposed/index.d.ts b/typings/vscode-proposed/index.d.ts index 76648e3bfc73..a96d13f576bf 100644 --- a/typings/vscode-proposed/index.d.ts +++ b/typings/vscode-proposed/index.d.ts @@ -651,12 +651,6 @@ export interface NotebookCellStatusBarItem { dispose(): void; } -export interface NotebookDecorationRenderOptions { - backgroundColor?: string | ThemeColor; - borderColor?: string | ThemeColor; - top: ThemableDecorationAttachmentRenderOptions; -} - export interface NotebookEditorDecorationType { readonly key: string; dispose(): void; @@ -683,9 +677,6 @@ export namespace notebook { provider: NotebookKernelProvider ): Disposable; - export function createNotebookEditorDecorationType( - options: NotebookDecorationRenderOptions - ): NotebookEditorDecorationType; export const onDidOpenNotebookDocument: Event; export const onDidCloseNotebookDocument: Event; export const onDidSaveNotebookDocument: Event; From c13dd4e7a3b70168c68ffd049a063d2d530fff6f Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 14:16:16 -0700 Subject: [PATCH 13/24] Ensure we use API to update --- .../jupyter/kernels/cellExecution.ts | 1 - .../datascience/notebook/helpers/helpers.ts | 40 ++++++------ types/vscode-proposed/index.d.ts | 55 ++++++++-------- types/vscode.proposed.d.ts | 63 ++++++++----------- typings/vscode-proposed/index.d.ts | 54 ++++++++-------- 5 files changed, 103 insertions(+), 110 deletions(-) diff --git a/src/client/datascience/jupyter/kernels/cellExecution.ts b/src/client/datascience/jupyter/kernels/cellExecution.ts index 6b981feaa8c7..d7bfe2942846 100644 --- a/src/client/datascience/jupyter/kernels/cellExecution.ts +++ b/src/client/datascience/jupyter/kernels/cellExecution.ts @@ -232,7 +232,6 @@ export class CellExecution { this.oldCellRunState === vscodeNotebookEnums.NotebookCellRunState.Running ? vscodeNotebookEnums.NotebookCellRunState.Idle : this.oldCellRunState; - this.cell.metadata.runStartTime = undefined; await this.editor.edit((edit) => edit.replaceCellMetadata(this.cell.notebook.cells.indexOf(this.cell), { ...this.cell.metadata, diff --git a/src/client/datascience/notebook/helpers/helpers.ts b/src/client/datascience/notebook/helpers/helpers.ts index 8010da9ebb48..5a7c0599c83a 100644 --- a/src/client/datascience/notebook/helpers/helpers.ts +++ b/src/client/datascience/notebook/helpers/helpers.ts @@ -278,17 +278,6 @@ function createVSCNotebookCellDataFromCodeCell(model: INotebookModel, cell: ICel runState = vscodeNotebookEnums.NotebookCellRunState.Success; } - const notebookCellMetadata: NotebookCellMetadata = { - editable: model.isTrusted, - executionOrder: typeof cell.data.execution_count === 'number' ? cell.data.execution_count : undefined, - hasExecutionOrder: true, - runState, - runnable: model.isTrusted - }; - - if (statusMessage) { - notebookCellMetadata.statusMessage = statusMessage; - } const vscodeMetadata = (cell.data.metadata.vscode as unknown) as IBaseCellVSCodeMetadata | undefined; const startExecutionTime = vscodeMetadata?.start_execution_time ? new Date(Date.parse(vscodeMetadata.start_execution_time)).getTime() @@ -297,11 +286,24 @@ function createVSCNotebookCellDataFromCodeCell(model: INotebookModel, cell: ICel ? new Date(Date.parse(vscodeMetadata.end_execution_time)).getTime() : undefined; + let runStartTime: undefined | number; + let lastRunDuration: undefined | number; if (startExecutionTime && typeof endExecutionTime === 'number') { - notebookCellMetadata.runStartTime = startExecutionTime; - notebookCellMetadata.lastRunDuration = endExecutionTime - startExecutionTime; + runStartTime = startExecutionTime; + lastRunDuration = endExecutionTime - startExecutionTime; } + const notebookCellMetadata: NotebookCellMetadata = { + editable: model.isTrusted, + executionOrder: typeof cell.data.execution_count === 'number' ? cell.data.execution_count : undefined, + hasExecutionOrder: true, + runState, + runnable: model.isTrusted, + statusMessage, + runStartTime, + lastRunDuration + }; + updateVSCNotebookCellMetadata(notebookCellMetadata, cell); // If not trusted, then clear the output in VSC Cell. @@ -690,12 +692,14 @@ export async function updateVSCNotebookAfterTrustingNotebook( return; } - document.metadata.cellEditable = true; - document.metadata.cellRunnable = true; - document.metadata.editable = true; - document.metadata.runnable = true; - await editor.edit((edit) => { + edit.replaceMetadata({ + ...document.metadata, + cellEditable: true, + cellRunnable: true, + editable: true, + runnable: true + }); document.cells.forEach((cell, index) => { if (cell.cellKind === vscodeNotebookEnums.CellKind.Markdown) { edit.replaceCellMetadata(index, { ...cell.metadata, editable: true }); diff --git a/types/vscode-proposed/index.d.ts b/types/vscode-proposed/index.d.ts index e81ab771f432..d78ec147d08b 100644 --- a/types/vscode-proposed/index.d.ts +++ b/types/vscode-proposed/index.d.ts @@ -100,65 +100,65 @@ export interface NotebookCellMetadata { /** * Controls whether a cell's editor is editable/readonly. */ - editable?: boolean; + readonly editable?: boolean; /** * Controls if the cell is executable. * This metadata is ignored for markdown cell. */ - runnable?: boolean; + readonly runnable?: boolean; /** * Controls if the cell has a margin to support the breakpoint UI. * This metadata is ignored for markdown cell. */ - breakpointMargin?: boolean; + readonly breakpointMargin?: boolean; /** * Whether the [execution order](#NotebookCellMetadata.executionOrder) indicator will be displayed. * Defaults to true. */ - hasExecutionOrder?: boolean; + readonly hasExecutionOrder?: boolean; /** * The order in which this cell was executed. */ - executionOrder?: number; + readonly executionOrder?: number; /** * A status message to be shown in the cell's status bar */ - statusMessage?: string; + readonly statusMessage?: string; /** * The cell's current run state */ - runState?: NotebookCellRunState; + readonly runState?: NotebookCellRunState; /** * If the cell is running, the time at which the cell started running */ - runStartTime?: number; + readonly runStartTime?: number; /** * The total duration of the cell's last run */ - lastRunDuration?: number; + readonly lastRunDuration?: number; /** * Whether a code cell's editor is collapsed */ - inputCollapsed?: boolean; + readonly inputCollapsed?: boolean; /** * Whether a code cell's outputs are collapsed */ - outputCollapsed?: boolean; + readonly outputCollapsed?: boolean; /** * Additional attributes of a cell metadata. */ - custom?: { [key: string]: any }; + readonly custom?: { [key: string]: any }; } export interface NotebookCell { @@ -168,8 +168,8 @@ export interface NotebookCell { readonly cellKind: CellKind; readonly document: TextDocument; readonly language: string; - outputs: CellOutput[]; - metadata: NotebookCellMetadata; + readonly outputs: CellOutput[]; + readonly metadata: NotebookCellMetadata; } export interface NotebookDocumentMetadata { @@ -177,43 +177,43 @@ export interface NotebookDocumentMetadata { * Controls if users can add or delete cells * Defaults to true */ - editable?: boolean; + readonly editable?: boolean; /** * Controls whether the full notebook can be run at once. * Defaults to true */ - runnable?: boolean; + readonly runnable?: boolean; /** * Default value for [cell editable metadata](#NotebookCellMetadata.editable). * Defaults to true. */ - cellEditable?: boolean; + readonly cellEditable?: boolean; /** * Default value for [cell runnable metadata](#NotebookCellMetadata.runnable). * Defaults to true. */ - cellRunnable?: boolean; + readonly cellRunnable?: boolean; /** * Default value for [cell hasExecutionOrder metadata](#NotebookCellMetadata.hasExecutionOrder). * Defaults to true. */ - cellHasExecutionOrder?: boolean; + readonly cellHasExecutionOrder?: boolean; - displayOrder?: GlobPattern[]; + readonly displayOrder?: GlobPattern[]; /** * Additional attributes of the document metadata. */ - custom?: { [key: string]: any }; + readonly custom?: { [key: string]: any }; /** * The document's current run state */ - runState?: NotebookRunState; + readonly runState?: NotebookRunState; } export interface NotebookDocumentContentOptions { @@ -221,13 +221,13 @@ export interface NotebookDocumentContentOptions { * Controls if outputs change will trigger notebook document content change and if it will be used in the diff editor * Default to false. If the content provider doesn't persisit the outputs in the file document, this should be set to true. */ - transientOutputs: boolean; + readonly transientOutputs: boolean; /** * Controls if a meetadata property change will trigger notebook document content change and if it will be used in the diff editor * Default to false. If the content provider doesn't persisit a metadata property in the file document, it should be set to true. */ - transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; + readonly transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; } export interface NotebookDocument { @@ -238,9 +238,9 @@ export interface NotebookDocument { readonly isDirty: boolean; readonly isUntitled: boolean; readonly cells: ReadonlyArray; - readonly contentOptions: NotebookDocumentContentOptions; - languages: string[]; - metadata: NotebookDocumentMetadata; + readonly contentOptions: Readonly; + readonly languages: string[]; + readonly metadata: Readonly; } export interface NotebookConcatTextDocument { @@ -651,7 +651,6 @@ export interface NotebookCellStatusBarItem { dispose(): void; } - export interface NotebookEditorDecorationType { readonly key: string; dispose(): void; diff --git a/types/vscode.proposed.d.ts b/types/vscode.proposed.d.ts index 013502d552c8..f693f822bcc1 100644 --- a/types/vscode.proposed.d.ts +++ b/types/vscode.proposed.d.ts @@ -86,65 +86,65 @@ declare module 'vscode' { /** * Controls whether a cell's editor is editable/readonly. */ - editable?: boolean; + readonly editable?: boolean; /** * Controls if the cell is executable. * This metadata is ignored for markdown cell. */ - runnable?: boolean; + readonly runnable?: boolean; /** * Controls if the cell has a margin to support the breakpoint UI. * This metadata is ignored for markdown cell. */ - breakpointMargin?: boolean; + readonly breakpointMargin?: boolean; /** * Whether the [execution order](#NotebookCellMetadata.executionOrder) indicator will be displayed. * Defaults to true. */ - hasExecutionOrder?: boolean; + readonly hasExecutionOrder?: boolean; /** * The order in which this cell was executed. */ - executionOrder?: number; + readonly executionOrder?: number; /** * A status message to be shown in the cell's status bar */ - statusMessage?: string; + readonly statusMessage?: string; /** * The cell's current run state */ - runState?: NotebookCellRunState; + readonly runState?: NotebookCellRunState; /** * If the cell is running, the time at which the cell started running */ - runStartTime?: number; + readonly runStartTime?: number; /** * The total duration of the cell's last run */ - lastRunDuration?: number; + readonly lastRunDuration?: number; /** * Whether a code cell's editor is collapsed */ - inputCollapsed?: boolean; + readonly inputCollapsed?: boolean; /** * Whether a code cell's outputs are collapsed */ - outputCollapsed?: boolean; + readonly outputCollapsed?: boolean; /** * Additional attributes of a cell metadata. */ - custom?: { [key: string]: any }; + readonly custom?: { [key: string]: any }; } export interface NotebookCell { @@ -154,8 +154,8 @@ declare module 'vscode' { readonly cellKind: CellKind; readonly document: TextDocument; readonly language: string; - outputs: CellOutput[]; - metadata: NotebookCellMetadata; + readonly outputs: CellOutput[]; + readonly metadata: NotebookCellMetadata; } export interface NotebookDocumentMetadata { @@ -163,43 +163,43 @@ declare module 'vscode' { * Controls if users can add or delete cells * Defaults to true */ - editable?: boolean; + readonly editable?: boolean; /** * Controls whether the full notebook can be run at once. * Defaults to true */ - runnable?: boolean; + readonly runnable?: boolean; /** * Default value for [cell editable metadata](#NotebookCellMetadata.editable). * Defaults to true. */ - cellEditable?: boolean; + readonly cellEditable?: boolean; /** * Default value for [cell runnable metadata](#NotebookCellMetadata.runnable). * Defaults to true. */ - cellRunnable?: boolean; + readonly cellRunnable?: boolean; /** * Default value for [cell hasExecutionOrder metadata](#NotebookCellMetadata.hasExecutionOrder). * Defaults to true. */ - cellHasExecutionOrder?: boolean; + readonly cellHasExecutionOrder?: boolean; - displayOrder?: GlobPattern[]; + readonly displayOrder?: GlobPattern[]; /** * Additional attributes of the document metadata. */ - custom?: { [key: string]: any }; + readonly custom?: { [key: string]: any }; /** * The document's current run state */ - runState?: NotebookRunState; + readonly runState?: NotebookRunState; } export interface NotebookDocumentContentOptions { @@ -207,13 +207,13 @@ declare module 'vscode' { * Controls if outputs change will trigger notebook document content change and if it will be used in the diff editor * Default to false. If the content provider doesn't persisit the outputs in the file document, this should be set to true. */ - transientOutputs: boolean; + readonly transientOutputs: boolean; /** * Controls if a meetadata property change will trigger notebook document content change and if it will be used in the diff editor * Default to false. If the content provider doesn't persisit a metadata property in the file document, it should be set to true. */ - transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; + readonly transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; } export interface NotebookDocument { @@ -224,9 +224,9 @@ declare module 'vscode' { readonly isDirty: boolean; readonly isUntitled: boolean; readonly cells: ReadonlyArray; - readonly contentOptions: NotebookDocumentContentOptions; - languages: string[]; - metadata: NotebookDocumentMetadata; + readonly contentOptions: Readonly; + readonly languages: string[]; + readonly metadata: Readonly; } export interface NotebookConcatTextDocument { @@ -637,12 +637,6 @@ declare module 'vscode' { dispose(): void; } - export interface NotebookDecorationRenderOptions { - backgroundColor?: string | ThemeColor; - borderColor?: string | ThemeColor; - top: ThemableDecorationAttachmentRenderOptions; - } - export interface NotebookEditorDecorationType { readonly key: string; dispose(): void; @@ -669,9 +663,6 @@ declare module 'vscode' { provider: NotebookKernelProvider ): Disposable; - export function createNotebookEditorDecorationType( - options: NotebookDecorationRenderOptions - ): NotebookEditorDecorationType; export const onDidOpenNotebookDocument: Event; export const onDidCloseNotebookDocument: Event; export const onDidSaveNotebookDocument: Event; diff --git a/typings/vscode-proposed/index.d.ts b/typings/vscode-proposed/index.d.ts index a96d13f576bf..d78ec147d08b 100644 --- a/typings/vscode-proposed/index.d.ts +++ b/typings/vscode-proposed/index.d.ts @@ -100,65 +100,65 @@ export interface NotebookCellMetadata { /** * Controls whether a cell's editor is editable/readonly. */ - editable?: boolean; + readonly editable?: boolean; /** * Controls if the cell is executable. * This metadata is ignored for markdown cell. */ - runnable?: boolean; + readonly runnable?: boolean; /** * Controls if the cell has a margin to support the breakpoint UI. * This metadata is ignored for markdown cell. */ - breakpointMargin?: boolean; + readonly breakpointMargin?: boolean; /** * Whether the [execution order](#NotebookCellMetadata.executionOrder) indicator will be displayed. * Defaults to true. */ - hasExecutionOrder?: boolean; + readonly hasExecutionOrder?: boolean; /** * The order in which this cell was executed. */ - executionOrder?: number; + readonly executionOrder?: number; /** * A status message to be shown in the cell's status bar */ - statusMessage?: string; + readonly statusMessage?: string; /** * The cell's current run state */ - runState?: NotebookCellRunState; + readonly runState?: NotebookCellRunState; /** * If the cell is running, the time at which the cell started running */ - runStartTime?: number; + readonly runStartTime?: number; /** * The total duration of the cell's last run */ - lastRunDuration?: number; + readonly lastRunDuration?: number; /** * Whether a code cell's editor is collapsed */ - inputCollapsed?: boolean; + readonly inputCollapsed?: boolean; /** * Whether a code cell's outputs are collapsed */ - outputCollapsed?: boolean; + readonly outputCollapsed?: boolean; /** * Additional attributes of a cell metadata. */ - custom?: { [key: string]: any }; + readonly custom?: { [key: string]: any }; } export interface NotebookCell { @@ -168,8 +168,8 @@ export interface NotebookCell { readonly cellKind: CellKind; readonly document: TextDocument; readonly language: string; - outputs: CellOutput[]; - metadata: NotebookCellMetadata; + readonly outputs: CellOutput[]; + readonly metadata: NotebookCellMetadata; } export interface NotebookDocumentMetadata { @@ -177,43 +177,43 @@ export interface NotebookDocumentMetadata { * Controls if users can add or delete cells * Defaults to true */ - editable?: boolean; + readonly editable?: boolean; /** * Controls whether the full notebook can be run at once. * Defaults to true */ - runnable?: boolean; + readonly runnable?: boolean; /** * Default value for [cell editable metadata](#NotebookCellMetadata.editable). * Defaults to true. */ - cellEditable?: boolean; + readonly cellEditable?: boolean; /** * Default value for [cell runnable metadata](#NotebookCellMetadata.runnable). * Defaults to true. */ - cellRunnable?: boolean; + readonly cellRunnable?: boolean; /** * Default value for [cell hasExecutionOrder metadata](#NotebookCellMetadata.hasExecutionOrder). * Defaults to true. */ - cellHasExecutionOrder?: boolean; + readonly cellHasExecutionOrder?: boolean; - displayOrder?: GlobPattern[]; + readonly displayOrder?: GlobPattern[]; /** * Additional attributes of the document metadata. */ - custom?: { [key: string]: any }; + readonly custom?: { [key: string]: any }; /** * The document's current run state */ - runState?: NotebookRunState; + readonly runState?: NotebookRunState; } export interface NotebookDocumentContentOptions { @@ -221,13 +221,13 @@ export interface NotebookDocumentContentOptions { * Controls if outputs change will trigger notebook document content change and if it will be used in the diff editor * Default to false. If the content provider doesn't persisit the outputs in the file document, this should be set to true. */ - transientOutputs: boolean; + readonly transientOutputs: boolean; /** * Controls if a meetadata property change will trigger notebook document content change and if it will be used in the diff editor * Default to false. If the content provider doesn't persisit a metadata property in the file document, it should be set to true. */ - transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; + readonly transientMetadata: { [K in keyof NotebookCellMetadata]?: boolean }; } export interface NotebookDocument { @@ -238,9 +238,9 @@ export interface NotebookDocument { readonly isDirty: boolean; readonly isUntitled: boolean; readonly cells: ReadonlyArray; - readonly contentOptions: NotebookDocumentContentOptions; - languages: string[]; - metadata: NotebookDocumentMetadata; + readonly contentOptions: Readonly; + readonly languages: string[]; + readonly metadata: Readonly; } export interface NotebookConcatTextDocument { From 0f583768711e5b409f8b1a26493310fb4438d488 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 15:16:02 -0700 Subject: [PATCH 14/24] More fixes --- src/client/common/utils/decorators.ts | 4 +- .../jupyter/kernels/cellExecution.ts | 162 ++++++++++-------- .../jupyter/kernels/kernelExecution.ts | 12 +- .../datascience/notebook/helpers/helpers.ts | 23 +-- types/vscode-proposed/index.d.ts | 2 +- 5 files changed, 119 insertions(+), 84 deletions(-) diff --git a/src/client/common/utils/decorators.ts b/src/client/common/utils/decorators.ts index 512f31e65e19..2c9362d329f7 100644 --- a/src/client/common/utils/decorators.ts +++ b/src/client/common/utils/decorators.ts @@ -178,11 +178,11 @@ export function cache(expiryDurationMs: number) { * @param {string} [scopeName] Scope for the error message to be logged along with the error. * @returns void */ -export function swallowExceptions(scopeName: string) { +export function swallowExceptions(scopeName?: string) { // tslint:disable-next-line:no-any no-function-expression return function (_target: any, propertyName: string, descriptor: TypedPropertyDescriptor) { const originalMethod = descriptor.value!; - const errorMessage = `Python Extension (Error in ${scopeName}, method:${propertyName}):`; + const errorMessage = `Python Extension (Error in ${scopeName || propertyName}, method:${propertyName}):`; // tslint:disable-next-line:no-any no-function-expression descriptor.value = function (...args: any[]) { try { diff --git a/src/client/datascience/jupyter/kernels/cellExecution.ts b/src/client/datascience/jupyter/kernels/cellExecution.ts index d7bfe2942846..20625a19649b 100644 --- a/src/client/datascience/jupyter/kernels/cellExecution.ts +++ b/src/client/datascience/jupyter/kernels/cellExecution.ts @@ -12,6 +12,7 @@ import { IApplicationShell, IVSCodeNotebook } from '../../../common/application/ import { traceInfo, traceWarning } from '../../../common/logger'; import { RefBool } from '../../../common/refBool'; import { createDeferred } from '../../../common/utils/async'; +import { swallowExceptions } from '../../../common/utils/decorators'; import { noop } from '../../../common/utils/misc'; import { StopWatch } from '../../../common/utils/stopWatch'; import { sendTelemetryEvent } from '../../../telemetry'; @@ -91,6 +92,10 @@ export class CellExecution { private _completed?: boolean; private readonly initPromise: Promise; + /** + * This is used to chain the updates to the cells. + */ + private previousUpdatedToCellHasCompleted = Promise.resolve(); private constructor( public readonly editor: VSCNotebookEditor, @@ -336,12 +341,18 @@ export class CellExecution { } } - private handleIOPub( + @swallowExceptions() + private async handleIOPub( clearState: RefBool, loggers: INotebookExecutionLogger[], msg: KernelMessage.IIOPubMessage // tslint:disable-next-line: no-any ) { + // Wait for previous cell update to complete. + await this.previousUpdatedToCellHasCompleted.then(noop, noop); + const deferred = createDeferred(); + this.previousUpdatedToCellHasCompleted = this.previousUpdatedToCellHasCompleted.then(() => deferred.promise); + // Let our loggers get a first crack at the message. They may change it loggers.forEach((f) => (msg = f.preHandleIOPub ? f.preHandleIOPub(msg) : msg)); @@ -352,9 +363,9 @@ export class CellExecution { let shouldUpdate = true; try { if (jupyterLab.KernelMessage.isExecuteResultMsg(msg)) { - this.handleExecuteResult(msg as KernelMessage.IExecuteResultMsg, clearState); + await this.handleExecuteResult(msg as KernelMessage.IExecuteResultMsg, clearState); } else if (jupyterLab.KernelMessage.isExecuteInputMsg(msg)) { - this.handleExecuteInput(msg as KernelMessage.IExecuteInputMsg, clearState); + await this.handleExecuteInput(msg as KernelMessage.IExecuteInputMsg, clearState); } else if (jupyterLab.KernelMessage.isStatusMsg(msg)) { // Status is handled by the result promise. While it is running we are active. Otherwise we're stopped. // So ignore status messages. @@ -362,16 +373,16 @@ export class CellExecution { shouldUpdate = false; this.handleStatusMessage(statusMsg, clearState); } else if (jupyterLab.KernelMessage.isStreamMsg(msg)) { - this.handleStreamMesssage(msg as KernelMessage.IStreamMsg, clearState); + await this.handleStreamMessage(msg as KernelMessage.IStreamMsg, clearState); } else if (jupyterLab.KernelMessage.isDisplayDataMsg(msg)) { - this.handleDisplayData(msg as KernelMessage.IDisplayDataMsg, clearState); + 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)) { - this.handleClearOutput(msg as KernelMessage.IClearOutputMsg, clearState); + await this.handleClearOutput(msg as KernelMessage.IClearOutputMsg, clearState); } else if (jupyterLab.KernelMessage.isErrorMsg(msg)) { - this.handleError(msg as KernelMessage.IErrorMsg, clearState); + await this.handleError(msg as KernelMessage.IErrorMsg, clearState); } else if (jupyterLab.KernelMessage.isCommOpenMsg(msg)) { // No new data to update UI, hence do not send updates. shouldUpdate = false; @@ -397,10 +408,12 @@ export class CellExecution { } catch (err) { // If not a restart error, then tell the subscriber this.completedWithErrors(err).then(noop, noop); + } finally { + deferred.resolve(); } } - private addToCellData( + private async addToCellData( output: | nbformat.IUnrecognizedOutput | nbformat.IExecuteResult @@ -411,14 +424,18 @@ export class CellExecution { ) { const converted = cellOutputToVSCCellOutput(output); - // Clear if necessary - if (clearState.value) { - this.cell.outputs = []; - clearState.update(false); - } + await this.editor.edit((edit) => { + let existingOutput = [...this.cell.outputs]; - // Append to the data (we would push here but VS code requires a recreation of the array) - this.cell.outputs = [...this.cell.outputs, converted]; + // Clear if necessary + if (clearState.value) { + existingOutput = []; + clearState.update(false); + } + + // Append to the data (we would push here but VS code requires a recreation of the array) + edit.replaceCellOutput(this.cell.notebook.cells.indexOf(this.cell), existingOutput.concat(converted)); + }); } private handleInputRequest(session: IJupyterSession, msg: KernelMessage.IStdinMessage) { @@ -439,13 +456,13 @@ export class CellExecution { // See this for docs on the messages: // https://jupyter-client.readthedocs.io/en/latest/messaging.html#messaging-in-jupyter - private handleExecuteResult(msg: KernelMessage.IExecuteResultMsg, clearState: RefBool) { + private async handleExecuteResult(msg: KernelMessage.IExecuteResultMsg, clearState: RefBool) { // Escape text output if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) { msg.content.data['text/plain'] = escape(msg.content.data['text/plain'] as string); } - this.addToCellData( + await this.addToCellData( { output_type: 'execute_result', data: msg.content.data, @@ -458,30 +475,32 @@ export class CellExecution { ); } - private handleExecuteReply(msg: KernelMessage.IExecuteReplyMsg, clearState: RefBool) { + private async handleExecuteReply(msg: KernelMessage.IExecuteReplyMsg, clearState: RefBool) { const reply = msg.content as KernelMessage.IExecuteReply; if (reply.payload) { - reply.payload.forEach((o) => { - if (o.data && o.data.hasOwnProperty('text/plain')) { - this.addToCellData( - { - // Mark as stream output so the text is formatted because it likely has ansi codes in it. - output_type: 'stream', - // tslint:disable-next-line: no-any - text: escape((o.data as any)['text/plain'].toString()), - metadata: {}, - execution_count: reply.execution_count - }, - clearState - ); - } - }); + await Promise.all( + reply.payload.map(async (o) => { + if (o.data && o.data.hasOwnProperty('text/plain')) { + await this.addToCellData( + { + // Mark as stream output so the text is formatted because it likely has ansi codes in it. + output_type: 'stream', + // tslint:disable-next-line: no-any + text: escape((o.data as any)['text/plain'].toString()), + metadata: {}, + execution_count: reply.execution_count + }, + clearState + ); + } + }) + ); } } - private handleExecuteInput(msg: KernelMessage.IExecuteInputMsg, _clearState: RefBool) { + private async handleExecuteInput(msg: KernelMessage.IExecuteInputMsg, _clearState: RefBool) { if (msg.content.execution_count) { - updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count); + await updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count); } } @@ -489,34 +508,41 @@ export class CellExecution { traceInfo(`Kernel switching to ${msg.content.execution_state}`); } - private handleStreamMesssage(msg: KernelMessage.IStreamMsg, clearState: RefBool) { - // Clear output if waiting for a clear - if (clearState.value) { - this.cell.outputs = []; - clearState.update(false); - } + private get cellIndex() { + return this.cell.notebook.cells.indexOf(this.cell); + } + private async handleStreamMessage(msg: KernelMessage.IStreamMsg, clearState: RefBool) { + await this.editor.edit((edit) => { + let exitingCellOutput = this.cell.outputs; + // Clear output if waiting for a clear + if (clearState.value) { + exitingCellOutput = []; + clearState.update(false); + } - // Might already have a stream message. If so, just add on to it. - const lastOutput = this.cell.outputs.length > 0 ? this.cell.outputs[this.cell.outputs.length - 1] : undefined; - const existing: CellStreamOutput | undefined = - lastOutput && lastOutput.outputKind === CellOutputKind.Text ? lastOutput : undefined; - if (existing) { - // tslint:disable-next-line:restrict-plus-operands - existing.text = formatStreamText(concatMultilineString(existing.text + escape(msg.content.text))); - this.cell.outputs = [...this.cell.outputs]; // This is necessary to get VS code to update (for now) - } else { - const originalText = formatStreamText(concatMultilineString(escape(msg.content.text))); - // Create a new stream entry - const output: nbformat.IStream = { - output_type: 'stream', - name: msg.content.name, - text: originalText - }; - this.cell.outputs = [...this.cell.outputs, cellOutputToVSCCellOutput(output)]; - } + // Might already have a stream message. If so, just add on to it. + const lastOutput = + exitingCellOutput.length > 0 ? exitingCellOutput[exitingCellOutput.length - 1] : undefined; + const existing: CellStreamOutput | undefined = + lastOutput && lastOutput.outputKind === CellOutputKind.Text ? lastOutput : undefined; + if (existing) { + // tslint:disable-next-line:restrict-plus-operands + existing.text = formatStreamText(concatMultilineString(existing.text + escape(msg.content.text))); + edit.replaceCellOutput(this.cellIndex, [...exitingCellOutput]); // This is necessary to get VS code to update (for now) + } else { + const originalText = formatStreamText(concatMultilineString(escape(msg.content.text))); + // Create a new stream entry + const output: nbformat.IStream = { + output_type: 'stream', + name: msg.content.name, + text: originalText + }; + edit.replaceCellOutput(this.cellIndex, [...exitingCellOutput, cellOutputToVSCCellOutput(output)]); + } + }); } - private handleDisplayData(msg: KernelMessage.IDisplayDataMsg, clearState: RefBool) { + private async handleDisplayData(msg: KernelMessage.IDisplayDataMsg, clearState: RefBool) { // Escape text output if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) { msg.content.data['text/plain'] = escape(msg.content.data['text/plain'] as string); @@ -529,40 +555,40 @@ export class CellExecution { // tslint:disable-next-line: no-any transient: msg.content.transient as any // NOSONAR }; - this.addToCellData(output, clearState); + await this.addToCellData(output, clearState); } - private handleClearOutput(msg: KernelMessage.IClearOutputMsg, clearState: RefBool) { + private async handleClearOutput(msg: KernelMessage.IClearOutputMsg, clearState: RefBool) { // If the message says wait, add every message type to our clear state. This will // make us wait for this type of output before we clear it. if (msg && msg.content.wait) { clearState.update(true); } else { // Clear all outputs and start over again. - this.cell.outputs = []; + await this.editor.edit((edit) => edit.replaceCellOutput(this.cellIndex, [])); } } - private handleError(msg: KernelMessage.IErrorMsg, clearState: RefBool) { + private async handleError(msg: KernelMessage.IErrorMsg, clearState: RefBool) { const output: nbformat.IError = { output_type: 'error', ename: msg.content.ename, evalue: msg.content.evalue, traceback: msg.content.traceback }; - this.addToCellData(output, clearState); + await this.addToCellData(output, clearState); } - private handleReply(clearState: RefBool, msg: KernelMessage.IShellControlMessage) { + private async handleReply(clearState: RefBool, msg: KernelMessage.IShellControlMessage) { // tslint:disable-next-line:no-require-imports const jupyterLab = require('@jupyterlab/services') as typeof import('@jupyterlab/services'); if (jupyterLab.KernelMessage.isExecuteReplyMsg(msg)) { - this.handleExecuteReply(msg, clearState); + await this.handleExecuteReply(msg, clearState); // Set execution count, all messages should have it if ('execution_count' in msg.content && typeof msg.content.execution_count === 'number') { - updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count); + await updateCellExecutionCount(this.editor, this.cell, msg.content.execution_count); } // Send this event. diff --git a/src/client/datascience/jupyter/kernels/kernelExecution.ts b/src/client/datascience/jupyter/kernels/kernelExecution.ts index f5fd0215bb06..cdf53a763b1e 100644 --- a/src/client/datascience/jupyter/kernels/kernelExecution.ts +++ b/src/client/datascience/jupyter/kernels/kernelExecution.ts @@ -85,11 +85,17 @@ export class KernelExecution implements IDisposable { if (this.documentExecutions.has(document)) { return; } + const editor = this.vscNotebook.notebookEditors.find((item) => item.document === document); + if (!editor) { + return; + } const cancelTokenSource = new MultiCancellationTokenSource(); this.documentExecutions.set(document, cancelTokenSource); const kernel = this.getKernel(document); - document.metadata.runState = vscodeNotebookEnums.NotebookRunState.Running; + await editor.edit((edit) => + edit.replaceMetadata({ ...document.metadata, runState: vscodeNotebookEnums.NotebookRunState.Running }) + ); const codeCellsToExecute = document.cells .filter((cell) => cell.cellKind === vscodeNotebookEnums.CellKind.Code) .filter((cell) => cell.document.getText().trim().length > 0) @@ -121,7 +127,9 @@ export class KernelExecution implements IDisposable { } finally { await Promise.all(codeCellsToExecute.map((cell) => cell.cancel())); // Cancel pending cells. this.documentExecutions.delete(document); - document.metadata.runState = vscodeNotebookEnums.NotebookRunState.Idle; + await editor.edit((edit) => + edit.replaceMetadata({ ...document.metadata, runState: vscodeNotebookEnums.NotebookRunState.Idle }) + ); } } diff --git a/src/client/datascience/notebook/helpers/helpers.ts b/src/client/datascience/notebook/helpers/helpers.ts index 5a7c0599c83a..d5d562cba3a8 100644 --- a/src/client/datascience/notebook/helpers/helpers.ts +++ b/src/client/datascience/notebook/helpers/helpers.ts @@ -178,21 +178,23 @@ export function createCellFromVSCNotebookCell(vscCell: NotebookCell, model: INot } /** - * Stores the Jupyter Cell metadata into the VSCode Cells. + * Identifies Jupyter Cell metadata that are to be stored in VSCode Cells. * This is used to facilitate: * 1. When a user copies and pastes a cell, then the corresponding metadata is also copied across. * 2. Diffing (VSC knows about metadata & stuff that contributes changes to a cell). */ -export function updateVSCNotebookCellMetadata(cellMetadata: NotebookCellMetadata, cell: ICell) { - cellMetadata.custom = cellMetadata.custom ?? {}; +export function getCustomNotebookCellMetadata(cell: ICell): Record { // We put this only for VSC to display in diff view. // Else we don't use this. const propertiesToClone = ['metadata', 'attachments']; + // tslint:disable-next-line: no-any + const custom: Record = {}; propertiesToClone.forEach((propertyToClone) => { if (cell.data[propertyToClone]) { - cellMetadata.custom![propertyToClone] = cloneDeep(cell.data[propertyToClone]); + custom[propertyToClone] = cloneDeep(cell.data[propertyToClone]); } }); + return custom; } export function getDefaultCodeLanguage(model: INotebookModel) { @@ -218,9 +220,9 @@ function createVSCNotebookCellDataFromRawCell(model: INotebookModel, cell: ICell editable: model.isTrusted, executionOrder: undefined, hasExecutionOrder: false, - runnable: false + runnable: false, + custom: getCustomNotebookCellMetadata(cell) }; - updateVSCNotebookCellMetadata(notebookCellMetadata, cell); return { cellKind: vscodeNotebookEnums.CellKind.Code, language: 'raw', @@ -245,9 +247,9 @@ function createVSCNotebookCellDataFromMarkdownCell(model: INotebookModel, cell: editable: model.isTrusted, executionOrder: undefined, hasExecutionOrder: false, - runnable: false + runnable: false, + custom: getCustomNotebookCellMetadata(cell) }; - updateVSCNotebookCellMetadata(notebookCellMetadata, cell); return { cellKind: vscodeNotebookEnums.CellKind.Markdown, language: MARKDOWN_LANGUAGE, @@ -301,11 +303,10 @@ function createVSCNotebookCellDataFromCodeCell(model: INotebookModel, cell: ICel runnable: model.isTrusted, statusMessage, runStartTime, - lastRunDuration + lastRunDuration, + custom: getCustomNotebookCellMetadata(cell) }; - updateVSCNotebookCellMetadata(notebookCellMetadata, cell); - // If not trusted, then clear the output in VSC Cell. // At this point we have the original output in the ICell. if (!model.isTrusted) { diff --git a/types/vscode-proposed/index.d.ts b/types/vscode-proposed/index.d.ts index d78ec147d08b..8c4fc94024b8 100644 --- a/types/vscode-proposed/index.d.ts +++ b/types/vscode-proposed/index.d.ts @@ -158,7 +158,7 @@ export interface NotebookCellMetadata { /** * Additional attributes of a cell metadata. */ - readonly custom?: { [key: string]: any }; + custom?: { [key: string]: any }; } export interface NotebookCell { From 72295fb9aadbc58992a0b903293e0331051b0bc3 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 15:27:08 -0700 Subject: [PATCH 15/24] Fixes --- src/client/datascience/jupyter/kernels/kernelExecution.ts | 2 +- src/client/datascience/notebookStorage/vscNotebookModel.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/client/datascience/jupyter/kernels/kernelExecution.ts b/src/client/datascience/jupyter/kernels/kernelExecution.ts index cdf53a763b1e..e4ada2dbdb54 100644 --- a/src/client/datascience/jupyter/kernels/kernelExecution.ts +++ b/src/client/datascience/jupyter/kernels/kernelExecution.ts @@ -207,7 +207,7 @@ export class KernelExecution implements IDisposable { ); // Start execution - cellExecution.start(kernelPromise, this.notebook); + await cellExecution.start(kernelPromise, this.notebook); // The result promise will resolve when complete. try { diff --git a/src/client/datascience/notebookStorage/vscNotebookModel.ts b/src/client/datascience/notebookStorage/vscNotebookModel.ts index cae34b736e51..d30947ebf435 100644 --- a/src/client/datascience/notebookStorage/vscNotebookModel.ts +++ b/src/client/datascience/notebookStorage/vscNotebookModel.ts @@ -6,6 +6,7 @@ import { Memento, Uri } from 'vscode'; import { NotebookDocument } from '../../../../types/vscode-proposed'; import { IVSCodeNotebook } from '../../common/application/types'; import { ICryptoUtils } from '../../common/types'; +import { noop } from '../../common/utils/misc'; import { NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; import { createCellFromVSCNotebookCell, @@ -101,7 +102,7 @@ export class VSCodeNotebookModel extends BaseNotebookModel { if (this.document) { const editor = this.vscodeNotebook?.notebookEditors.find((item) => item.document === this.document); if (editor) { - updateVSCNotebookAfterTrustingNotebook(editor, this.document, this._cells); + updateVSCNotebookAfterTrustingNotebook(editor, this.document, this._cells).then(noop, noop); } // We don't need old cells. this._cells = []; From 4d13df2f341f60a904375c1d8150819c21ad9f6a Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 15:28:41 -0700 Subject: [PATCH 16/24] Test fixes --- .../notebook/contentProvider.unit.test.ts | 20 +++++++++++++++---- .../datascience/notebook/helpers.unit.test.ts | 10 ++++++++-- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/test/datascience/notebook/contentProvider.unit.test.ts b/src/test/datascience/notebook/contentProvider.unit.test.ts index 5ccae9026d79..122200ace71b 100644 --- a/src/test/datascience/notebook/contentProvider.unit.test.ts +++ b/src/test/datascience/notebook/contentProvider.unit.test.ts @@ -84,7 +84,10 @@ suite('DataScience - NativeNotebook ContentProvider', () => { executionOrder: 10, hasExecutionOrder: true, runState: (vscodeNotebookEnums as any).NotebookCellRunState.Success, - runnable: isNotebookTrusted + runnable: isNotebookTrusted, + statusMessage: undefined, + runStartTime: undefined, + lastRunDuration: undefined } }, { @@ -96,7 +99,10 @@ suite('DataScience - NativeNotebook ContentProvider', () => { editable: isNotebookTrusted, executionOrder: undefined, hasExecutionOrder: false, - runnable: false + runnable: false, + statusMessage: undefined, + runStartTime: undefined, + lastRunDuration: undefined } } ]); @@ -163,7 +169,10 @@ suite('DataScience - NativeNotebook ContentProvider', () => { executionOrder: 10, hasExecutionOrder: true, runState: (vscodeNotebookEnums as any).NotebookCellRunState.Success, - runnable: isNotebookTrusted + runnable: isNotebookTrusted, + statusMessage: undefined, + runStartTime: undefined, + lastRunDuration: undefined } }, { @@ -175,7 +184,10 @@ suite('DataScience - NativeNotebook ContentProvider', () => { editable: isNotebookTrusted, executionOrder: undefined, hasExecutionOrder: false, - runnable: false + runnable: false, + statusMessage: undefined, + runStartTime: undefined, + lastRunDuration: undefined } } ]); diff --git a/src/test/datascience/notebook/helpers.unit.test.ts b/src/test/datascience/notebook/helpers.unit.test.ts index 18597c3945c7..a1aeea65d979 100644 --- a/src/test/datascience/notebook/helpers.unit.test.ts +++ b/src/test/datascience/notebook/helpers.unit.test.ts @@ -67,7 +67,10 @@ suite('DataScience - NativeNotebook helpers', () => { executionOrder: 10, hasExecutionOrder: true, runState: vscodeNotebookEnums.NotebookCellRunState.Success, - runnable: true + runnable: true, + statusMessage: undefined, + runStartTime: undefined, + lastRunDuration: undefined } }, { @@ -79,7 +82,10 @@ suite('DataScience - NativeNotebook helpers', () => { editable: true, executionOrder: undefined, hasExecutionOrder: false, - runnable: false + runnable: false, + statusMessage: undefined, + runStartTime: undefined, + lastRunDuration: undefined } } ]); From f0cb569002a554aaa20a7abdb4748482ad019e95 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 16:00:07 -0700 Subject: [PATCH 17/24] Fix tests --- .../notebook/contentProvider.unit.test.ts | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/test/datascience/notebook/contentProvider.unit.test.ts b/src/test/datascience/notebook/contentProvider.unit.test.ts index 122200ace71b..0d3da6c0e13e 100644 --- a/src/test/datascience/notebook/contentProvider.unit.test.ts +++ b/src/test/datascience/notebook/contentProvider.unit.test.ts @@ -83,11 +83,11 @@ suite('DataScience - NativeNotebook ContentProvider', () => { editable: isNotebookTrusted, executionOrder: 10, hasExecutionOrder: true, + lastRunDuration: undefined, + runStartTime: undefined, runState: (vscodeNotebookEnums as any).NotebookCellRunState.Success, runnable: isNotebookTrusted, - statusMessage: undefined, - runStartTime: undefined, - lastRunDuration: undefined + statusMessage: undefined } }, { @@ -99,10 +99,7 @@ suite('DataScience - NativeNotebook ContentProvider', () => { editable: isNotebookTrusted, executionOrder: undefined, hasExecutionOrder: false, - runnable: false, - statusMessage: undefined, - runStartTime: undefined, - lastRunDuration: undefined + runnable: false } } ]); @@ -184,10 +181,7 @@ suite('DataScience - NativeNotebook ContentProvider', () => { editable: isNotebookTrusted, executionOrder: undefined, hasExecutionOrder: false, - runnable: false, - statusMessage: undefined, - runStartTime: undefined, - lastRunDuration: undefined + runnable: false } } ]); From 294e474f61566cf33aa07fc3a4b8f9906cb309ae Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 16:05:46 -0700 Subject: [PATCH 18/24] oops --- src/test/mocks/vsc/extHostedTypes.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/mocks/vsc/extHostedTypes.ts b/src/test/mocks/vsc/extHostedTypes.ts index 9416be3be6b5..fc8f77279b65 100644 --- a/src/test/mocks/vsc/extHostedTypes.ts +++ b/src/test/mocks/vsc/extHostedTypes.ts @@ -547,7 +547,7 @@ export namespace vscMockExtHostedTypes { } export class WorkspaceEdit implements vscode.WorkspaceEdit { - replaceNotebookMetadata(_uri: vscode.Uri, _value: vscode.NotebookDocumentMetadata): void{ + replaceNotebookMetadata(_uri: vscode.Uri, _value: vscode.NotebookDocumentMetadata): void { // } replaceNotebookCells( From 71cfcdb676bdd6d8dd007825fdf8eb868e341cef Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 16:25:10 -0700 Subject: [PATCH 19/24] fixes --- src/test/datascience/notebook/helper.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/datascience/notebook/helper.ts b/src/test/datascience/notebook/helper.ts index fced9bed281f..b6da134941cd 100644 --- a/src/test/datascience/notebook/helper.ts +++ b/src/test/datascience/notebook/helper.ts @@ -80,7 +80,7 @@ export async function insertPythonCell(source: string) { return; } await activeEditor.edit((edit) => - edit.replaceCells(activeEditor.document.cells.length - 1, activeEditor.document.cells.length - 1, [ + edit.replaceCells(activeEditor.document.cells.length, 0, [ { cellKind: vscodeNotebookEnums.CellKind.Code, language: PYTHON_LANGUAGE, From 6ecb5f5fd8f45ab913f9a867630e1fb98cc86eb3 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 17:10:44 -0700 Subject: [PATCH 20/24] Fixes --- .../datascience/jupyter/kernels/cellExecution.ts | 3 +-- .../datascience/notebook/helpers/helpers.ts | 15 +++++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/client/datascience/jupyter/kernels/cellExecution.ts b/src/client/datascience/jupyter/kernels/cellExecution.ts index 20625a19649b..8108d2a295f9 100644 --- a/src/client/datascience/jupyter/kernels/cellExecution.ts +++ b/src/client/datascience/jupyter/kernels/cellExecution.ts @@ -189,8 +189,7 @@ export class CellExecution { await updateCellExecutionTimes(this.editor, this.cell, { startTime: this.cell.metadata.runStartTime, - lastRunDuration: this.stopWatch.elapsedTime, - duration: this.cell.metadata.lastRunDuration + lastRunDuration: this.stopWatch.elapsedTime }); // If there are any errors in the cell, then change status to error. diff --git a/src/client/datascience/notebook/helpers/helpers.ts b/src/client/datascience/notebook/helpers/helpers.ts index d5d562cba3a8..4cb3edcfb135 100644 --- a/src/client/datascience/notebook/helpers/helpers.ts +++ b/src/client/datascience/notebook/helpers/helpers.ts @@ -364,11 +364,12 @@ export async function clearCellForExecution(editor: NotebookEditor, cell: Notebo export async function updateCellExecutionTimes( editor: NotebookEditor, cell: NotebookCell, - times?: { startTime?: number; duration?: number; lastRunDuration?: number } + times?: { startTime?: number; lastRunDuration?: number } ) { const cellIndex = cell.notebook.cells.indexOf(cell); - if (!times || !times.duration || !times.startTime) { + if (!times || !times.lastRunDuration || !times.startTime) { + // Based on feedback from VSC, its best to clone these objects when updating them. const cellMetadata = cloneDeep(cell.metadata); let updated = false; if (cellMetadata.custom?.metadata?.vscode?.start_execution_time) { @@ -380,16 +381,22 @@ export async function updateCellExecutionTimes( updated = true; } if (updated) { - await editor.edit((edit) => edit.replaceCellMetadata(cellIndex, { ...cellMetadata })); + await editor.edit((edit) => + edit.replaceCellMetadata(cellIndex, { + ...cellMetadata + }) + ); } return; } const startTimeISO = new Date(times.startTime).toISOString(); - const endTimeISO = new Date(times.startTime + times.duration).toISOString(); + const endTimeISO = new Date(times.startTime + times.lastRunDuration).toISOString(); + // Based on feedback from VSC, its best to clone these objects when updating them. const customMetadata = cloneDeep(cell.metadata.custom || {}); customMetadata.metadata = customMetadata.metadata || {}; customMetadata.metadata.vscode = customMetadata.metadata.vscode || {}; + // We store it in the metadata so we can display this when user opens a notebook again. customMetadata.metadata.vscode.end_execution_time = endTimeISO; customMetadata.metadata.vscode.start_execution_time = startTimeISO; const lastRunDuration = times.lastRunDuration ?? cell.metadata.lastRunDuration; From 8a46c45bdf6d6f7138b58ed908e82ca36360017b Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 24 Sep 2020 17:30:17 -0700 Subject: [PATCH 21/24] Fix tests --- src/test/datascience/notebook/helpers.unit.test.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/test/datascience/notebook/helpers.unit.test.ts b/src/test/datascience/notebook/helpers.unit.test.ts index a1aeea65d979..f526c43b35e0 100644 --- a/src/test/datascience/notebook/helpers.unit.test.ts +++ b/src/test/datascience/notebook/helpers.unit.test.ts @@ -82,10 +82,7 @@ suite('DataScience - NativeNotebook helpers', () => { editable: true, executionOrder: undefined, hasExecutionOrder: false, - runnable: false, - statusMessage: undefined, - runStartTime: undefined, - lastRunDuration: undefined + runnable: false } } ]); From 2103bf646b770f5c13a5e47031b1d4171a228016 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 25 Sep 2020 11:32:41 -0700 Subject: [PATCH 22/24] Disabled for now --- src/client/datascience/notebook/contentProvider.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/datascience/notebook/contentProvider.ts b/src/client/datascience/notebook/contentProvider.ts index a94bc091a61c..251f54e17861 100644 --- a/src/client/datascience/notebook/contentProvider.ts +++ b/src/client/datascience/notebook/contentProvider.ts @@ -45,8 +45,8 @@ export class NotebookContentProvider implements INotebookContentProvider { @inject(NotebookEditorCompatibilitySupport) private readonly compatibilitySupport: NotebookEditorCompatibilitySupport ) {} - public notifyChangesToDocument(document: NotebookDocument) { - this.notebookChanged.fire({ document }); + public notifyChangesToDocument(_document: NotebookDocument) { + // this.notebookChanged.fire({ document }); } public async resolveNotebook(_document: NotebookDocument, _webview: NotebookCommunication): Promise { // Later From 2212a0e64abcc2f36ef02e8d4e42de9cc14e9f48 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 25 Sep 2020 15:52:03 -0700 Subject: [PATCH 23/24] More fixes --- .vscode/launch.json | 5 +-- .../jupyter/kernels/cellExecution.ts | 25 +++++++++---- .../datascience/notebook/notebookEditor.ts | 2 +- .../notebook/executionService.ds.test.ts | 21 ++--------- src/test/datascience/notebook/helper.ts | 35 ++++++------------- .../notebook/interrupRestart.ds.test.ts | 26 +++++++++----- 6 files changed, 52 insertions(+), 62 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 52819f341bb8..6ab6cd6b334b 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -143,7 +143,7 @@ "--extensionTestsPath=${workspaceFolder}/out/test" ], "env": { - "VSC_PYTHON_CI_TEST_GREP": "Language Server:" + "VSC_PYTHON_CI_TEST_GREP": "Language Server:" }, "stopOnEntry": false, "sourceMaps": true, @@ -164,8 +164,9 @@ "--extensionTestsPath=${workspaceFolder}/out/test" ], "env": { - "VSC_PYTHON_CI_TEST_GREP": "", // Modify this to run a subset of the single workspace tests + "VSC_PYTHON_CI_TEST_GREP": "xxx", // Modify this to run a subset of the single workspace tests "VSC_PYTHON_CI_TEST_INVERT_GREP": "", // Initialize this to invert the grep (exclude tests with value defined in grep). + "CI_PYTHON_PATH": "", // Initialize this to invert the grep (exclude tests with value defined in grep). "VSC_PYTHON_LOAD_EXPERIMENTS_FROM_FILE": "true", "TEST_FILES_SUFFIX": "ds.test" diff --git a/src/client/datascience/jupyter/kernels/cellExecution.ts b/src/client/datascience/jupyter/kernels/cellExecution.ts index 8108d2a295f9..fd75fd8a5096 100644 --- a/src/client/datascience/jupyter/kernels/cellExecution.ts +++ b/src/client/datascience/jupyter/kernels/cellExecution.ts @@ -11,6 +11,7 @@ import { concatMultilineString, formatStreamText } from '../../../../datascience import { IApplicationShell, IVSCodeNotebook } from '../../../common/application/types'; import { traceInfo, traceWarning } from '../../../common/logger'; import { RefBool } from '../../../common/refBool'; +import { IDisposable } from '../../../common/types'; import { createDeferred } from '../../../common/utils/async'; import { swallowExceptions } from '../../../common/utils/decorators'; import { noop } from '../../../common/utils/misc'; @@ -78,6 +79,10 @@ export class CellExecution { return this._completed; } + private get cellIndex() { + return this.cell.notebook.cells.indexOf(this.cell); + } + private static sentExecuteCellTelemetry?: boolean; private readonly oldCellRunState?: NotebookCellRunState; @@ -96,6 +101,7 @@ export class CellExecution { * This is used to chain the updates to the cells. */ private previousUpdatedToCellHasCompleted = Promise.resolve(); + private disposables: IDisposable[] = []; private constructor( public readonly editor: VSCNotebookEditor, @@ -138,10 +144,12 @@ export class CellExecution { // Begin the request that will modify our cell. kernelPromise - .then((_k) => this.execute(notebook.session, notebook.getLoggers())) - .catch((e) => this.completedWithErrors(e)); + .then((kernel) => this.handleKernelRestart(kernel)) + .then(() => this.execute(notebook.session, notebook.getLoggers())) + .catch((e) => this.completedWithErrors(e)) + .finally(() => this.dispose()) + .catch(noop); } - /** * Cancel execution. * If execution has commenced, then interrupt (via cancellation token) else dequeue from execution. @@ -158,6 +166,13 @@ export class CellExecution { await this.dequeue(); } this._result.resolve(this.cell.metadata.runState); + this.dispose(); + } + private dispose() { + this.disposables.forEach((d) => d.dispose()); + } + private handleKernelRestart(kernel: IKernel) { + kernel.onRestarted(async () => this.cancel(), this, this.disposables); } private async completedWithErrors(error: Partial) { @@ -506,10 +521,6 @@ export class CellExecution { private handleStatusMessage(msg: KernelMessage.IStatusMsg, _clearState: RefBool) { traceInfo(`Kernel switching to ${msg.content.execution_state}`); } - - private get cellIndex() { - return this.cell.notebook.cells.indexOf(this.cell); - } private async handleStreamMessage(msg: KernelMessage.IStreamMsg, clearState: RefBool) { await this.editor.edit((edit) => { let exitingCellOutput = this.cell.outputs; diff --git a/src/client/datascience/notebook/notebookEditor.ts b/src/client/datascience/notebook/notebookEditor.ts index 65bfaf7a1b90..4ff1439eef42 100644 --- a/src/client/datascience/notebook/notebookEditor.ts +++ b/src/client/datascience/notebook/notebookEditor.ts @@ -137,7 +137,7 @@ export class NotebookEditor implements INotebookEditor { if (editor) { editor .edit((edit) => - edit.replaceCells(0, this.document.cells.length - 1, [ + edit.replaceCells(0, this.document.cells.length, [ { cellKind: vscodeNotebookEnums.CellKind.Code, language: defaultLanguage, diff --git a/src/test/datascience/notebook/executionService.ds.test.ts b/src/test/datascience/notebook/executionService.ds.test.ts index dea97b8af244..ff7d2c882b49 100644 --- a/src/test/datascience/notebook/executionService.ds.test.ts +++ b/src/test/datascience/notebook/executionService.ds.test.ts @@ -11,7 +11,6 @@ import { CellDisplayOutput, commands } from 'vscode'; import { CellErrorOutput } from '../../../../typings/vscode-proposed'; import { IVSCodeNotebook } from '../../../client/common/application/types'; import { IDisposable } from '../../../client/common/types'; -import { INotebookContentProvider } from '../../../client/datascience/notebook/types'; import { INotebookEditorProvider } from '../../../client/datascience/types'; import { createEventHandler, IExtensionTestApi, sleep, waitForCondition } from '../../common'; import { initialize } from '../../initialize'; @@ -111,22 +110,6 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { ); assert.isUndefined(cells[0].metadata.runState); }); - test('Execute cell should mark a notebook as being dirty', async () => { - await insertPythonCellAndWait('print("Hello World")'); - const contentProvider = api.serviceContainer.get(INotebookContentProvider); - const cell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; - const changedEvent = createEventHandler(contentProvider, 'onDidChangeNotebook', disposables); - - await executeCell(cell); - - // Wait till execution count changes and status is success. - await waitForCondition( - async () => assertHasExecutionCompletedSuccessfully(cell), - 15_000, - 'Cell did not get executed' - ); - assert.ok(changedEvent.fired, 'Notebook should be dirty after executing a cell'); - }); test('Verify Cell output, execution count and status', async () => { await insertPythonCellAndWait('print("Hello World")'); const cell = vscodeNotebook.activeNotebookEditor?.document.cells![0]!; @@ -162,8 +145,8 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { ); // Verify output. - assertHasTextOutputInVSCode(cells[0], 'Foo Bar', 0); - assertHasTextOutputInVSCode(cells[1], 'Hello World', 1); + assertHasTextOutputInVSCode(cells[0], 'Foo Bar'); + assertHasTextOutputInVSCode(cells[1], 'Hello World'); // Verify execution count. assert.ok(cells[0].metadata.executionOrder, 'Execution count should be > 0'); diff --git a/src/test/datascience/notebook/helper.ts b/src/test/datascience/notebook/helper.ts index b6da134941cd..06d08d20b30b 100644 --- a/src/test/datascience/notebook/helper.ts +++ b/src/test/datascience/notebook/helper.ts @@ -51,7 +51,7 @@ export async function insertMarkdownCell(source: string) { return; } await activeEditor.edit((edit) => - edit.replaceCells(activeEditor.document.cells.length - 1, activeEditor.document.cells.length - 1, [ + edit.replaceCells(activeEditor.document.cells.length, 0, [ { cellKind: vscodeNotebookEnums.CellKind.Markdown, language: MARKDOWN_LANGUAGE, @@ -63,24 +63,17 @@ export async function insertMarkdownCell(source: string) { } ]) ); - - await waitForCondition( - async () => - activeEditor?.document.cells[activeEditor.document.cells.length - 1].document.getText().trim() === - source.trim(), - 5_000, - 'Cell not inserted' - ); } -export async function insertPythonCell(source: string) { +export async function insertPythonCell(source: string, index?: number) { const { vscodeNotebook } = await getServices(); const activeEditor = vscodeNotebook.activeNotebookEditor; if (!activeEditor) { assert.fail('No active editor'); return; } + const startNumber = index ?? activeEditor.document.cells.length; await activeEditor.edit((edit) => - edit.replaceCells(activeEditor.document.cells.length, 0, [ + edit.replaceCells(startNumber, 0, [ { cellKind: vscodeNotebookEnums.CellKind.Code, language: PYTHON_LANGUAGE, @@ -92,16 +85,9 @@ export async function insertPythonCell(source: string) { } ]) ); - await waitForCondition( - async () => - activeEditor?.document.cells[activeEditor.document.cells.length - 1].document.getText().trim() === - source.trim(), - 5_000, - 'Cell not inserted' - ); } -export async function insertPythonCellAndWait(source: string) { - await insertPythonCell(source); +export async function insertPythonCellAndWait(source: string, index?: number) { + await insertPythonCell(source, index); } export async function insertMarkdownCellAndWait(source: string) { await insertMarkdownCell(source); @@ -116,7 +102,7 @@ export async function deleteCell(index: number = 0) { assert.fail('No active editor'); return; } - await activeEditor.edit((edit) => edit.replaceCells(index, index, [])); + await activeEditor.edit((edit) => edit.replaceCells(index, 1, [])); } export async function deleteAllCellsAndWait() { const { vscodeNotebook } = await getServices(); @@ -124,9 +110,7 @@ export async function deleteAllCellsAndWait() { if (!activeEditor || activeEditor.document.cells.length === 0) { return; } - await activeEditor.edit((edit) => edit.replaceCells(0, activeEditor.document.cells.length - 1, [])); - // Wait for cell to get deleted. - await waitForCondition(async () => activeEditor.document.cells.length === 0, 1_000, 'Cell not deleted'); + await activeEditor.edit((edit) => edit.replaceCells(0, activeEditor.document.cells.length, [])); } export async function createTemporaryFile(options: { @@ -214,6 +198,7 @@ export async function startJupyter(closeInitialEditor: boolean) { const disposables: IDisposable[] = []; try { await editorProvider.createNew(); + await deleteAllCellsAndWait(); await insertPythonCell('print("Hello World")'); const cell = vscodeNotebook.activeNotebookEditor!.document.cells[0]!; await executeActiveDocument(); @@ -278,7 +263,7 @@ export function assertHasOutputInVSCell(cell: NotebookCell) { export function assertHasOutputInICell(cell: ICell, model: INotebookModel) { assert.ok((cell.data.outputs as nbformat.IOutput[]).length, `No output in ICell ${model.cells.indexOf(cell) + 1}`); } -export function assertHasTextOutputInVSCode(cell: NotebookCell, text: string, index: number, isExactMatch = true) { +export function assertHasTextOutputInVSCode(cell: NotebookCell, text: string, index: number = 0, isExactMatch = true) { const cellOutputs = cell.outputs; assert.ok(cellOutputs, 'No output'); assert.equal(cellOutputs[index].outputKind, vscodeNotebookEnums.CellOutputKind.Rich, 'Incorrect output kind'); diff --git a/src/test/datascience/notebook/interrupRestart.ds.test.ts b/src/test/datascience/notebook/interrupRestart.ds.test.ts index 8c80d6adc5d3..7deae6e8bfd5 100644 --- a/src/test/datascience/notebook/interrupRestart.ds.test.ts +++ b/src/test/datascience/notebook/interrupRestart.ds.test.ts @@ -7,7 +7,7 @@ import { assert } from 'chai'; import * as sinon from 'sinon'; import { commands, NotebookEditor as VSCNotebookEditor } from 'vscode'; import { IApplicationShell, IVSCodeNotebook } from '../../../client/common/application/types'; -import { IDisposable } from '../../../client/common/types'; +import { IConfigurationService, IDataScienceSettings, IDisposable } from '../../../client/common/types'; import { createDeferredFromPromise } from '../../../client/common/utils/async'; import { noop } from '../../../client/common/utils/misc'; import { IKernelProvider } from '../../../client/datascience/jupyter/kernels/types'; @@ -21,7 +21,6 @@ import { canRunTests, closeNotebooks, closeNotebooksAndCleanUpAfterTests, - deleteAllCellsAndWait, executeActiveDocument, insertPythonCellAndWait, startJupyter, @@ -46,6 +45,8 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)', let vscEditor: VSCNotebookEditor; let vscodeNotebook: IVSCodeNotebook; const suiteDisposables: IDisposable[] = []; + let oldAskForRestart: boolean | undefined; + let dsSettings: IDataScienceSettings; suiteSetup(async function () { this.timeout(60_000); api = await initialize(); @@ -58,6 +59,11 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)', editorProvider = api.serviceContainer.get(INotebookEditorProvider); editorProvider = api.serviceContainer.get(INotebookEditorProvider); kernelProvider = api.serviceContainer.get(IKernelProvider); + dsSettings = api.serviceContainer.get(IConfigurationService).getSettings(undefined) + .datascience; + oldAskForRestart = dsSettings.askForKernelRestart; + // Disable the prompt (when attempting to restart kernel). + dsSettings.askForKernelRestart = false; }); setup(async () => { sinon.restore(); @@ -66,13 +72,17 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)', await editorProvider.createNew(); assert.isOk(vscodeNotebook.activeNotebookEditor, 'No active notebook'); vscEditor = vscodeNotebook.activeNotebookEditor!; - await deleteAllCellsAndWait(); }); teardown(() => closeNotebooks(disposables)); - suiteTeardown(() => closeNotebooksAndCleanUpAfterTests(disposables.concat(suiteDisposables))); + suiteTeardown(async () => { + oldAskForRestart = dsSettings.askForKernelRestart; + // Restore. + dsSettings.askForKernelRestart = oldAskForRestart; + await closeNotebooksAndCleanUpAfterTests(disposables.concat(suiteDisposables)); + }); - test('Cancelling token will cancel cell execution', async () => { - await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)'); + test('Cancelling token will cancel cell executionxxx', async () => { + await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)', 0); const cell = vscEditor.document.cells[0]; const appShell = api.serviceContainer.get(IApplicationShell); const showInformationMessage = sinon.stub(appShell, 'showInformationMessage'); @@ -105,8 +115,8 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)', assertVSCCellHasErrors(cell); } }); - test('Restarting kernel will cancel cell execution & we can re-run a cell', async () => { - await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)'); + test('Restarting kernel will cancel cell execution & we can re-run a cellxxx', async () => { + await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)', 0); const cell = vscEditor.document.cells[0]; await executeActiveDocument(); From a19da7c24bfdee2b24f553ecfb9443fb701126f8 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 25 Sep 2020 15:56:08 -0700 Subject: [PATCH 24/24] Misc --- .vscode/launch.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 6ab6cd6b334b..4eab7ac182a7 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -164,7 +164,7 @@ "--extensionTestsPath=${workspaceFolder}/out/test" ], "env": { - "VSC_PYTHON_CI_TEST_GREP": "xxx", // Modify this to run a subset of the single workspace tests + "VSC_PYTHON_CI_TEST_GREP": "", // Modify this to run a subset of the single workspace tests "VSC_PYTHON_CI_TEST_INVERT_GREP": "", // Initialize this to invert the grep (exclude tests with value defined in grep). "CI_PYTHON_PATH": "", // Initialize this to invert the grep (exclude tests with value defined in grep).