From d8eb6006eb9b9fc9ff99847e5e79bfc3e5fbf8d6 Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Fri, 11 Feb 2022 14:58:36 -0800 Subject: [PATCH] Put cells in pending mode immediately when the user clicks run. Remove them from the pending state if the controller does not create an execution immediately inside the execute handler. --- .../api/browser/mainThreadNotebookKernels.ts | 1 + .../api/common/extHostNotebookKernels.ts | 2 +- .../api/common/extHostTypeConverters.ts | 14 +++++++ .../executionStatusBarItemController.ts | 2 +- .../browser/notebookExecutionServiceImpl.ts | 17 +++++--- .../notebookExecutionStateServiceImpl.ts | 8 +++- .../browser/view/cellParts/cellContextKeys.ts | 2 +- .../view/cellParts/codeCellExecutionIcon.ts | 2 +- .../contrib/notebook/common/notebookCommon.ts | 2 +- .../common/notebookExecutionStateService.ts | 1 + .../browser/notebookExecutionService.test.ts | 29 +++++++++++++ .../test/browser/testNotebookEditor.ts | 41 +++++++++++++++---- 12 files changed, 101 insertions(+), 20 deletions(-) diff --git a/src/vs/workbench/api/browser/mainThreadNotebookKernels.ts b/src/vs/workbench/api/browser/mainThreadNotebookKernels.ts index 4ea045a447f..dc7b4e941a9 100644 --- a/src/vs/workbench/api/browser/mainThreadNotebookKernels.ts +++ b/src/vs/workbench/api/browser/mainThreadNotebookKernels.ts @@ -248,6 +248,7 @@ export class MainThreadNotebookKernels implements MainThreadNotebookKernelsShape $createExecution(handle: number, controllerId: string, rawUri: UriComponents, cellHandle: number): void { const uri = URI.revive(rawUri); const execution = this._notebookExecutionStateService.createCellExecution(controllerId, uri, cellHandle); + execution.confirm(); this._executions.set(handle, execution); } diff --git a/src/vs/workbench/api/common/extHostNotebookKernels.ts b/src/vs/workbench/api/common/extHostNotebookKernels.ts index 493f691ec34..05550b53475 100644 --- a/src/vs/workbench/api/common/extHostNotebookKernels.ts +++ b/src/vs/workbench/api/common/extHostNotebookKernels.ts @@ -341,7 +341,7 @@ export class ExtHostNotebookKernels implements ExtHostNotebookKernelsShape { if (cell) { this._onDidChangeCellExecutionState.fire({ cell: cell.apiCell, - state: state ?? ExtHostNotebookCellExecutionState.Idle + state: state ? extHostTypeConverters.NotebookCellExecutionState.to(state) : ExtHostNotebookCellExecutionState.Idle }); } } diff --git a/src/vs/workbench/api/common/extHostTypeConverters.ts b/src/vs/workbench/api/common/extHostTypeConverters.ts index 9c2420847da..7ec5aff1000 100644 --- a/src/vs/workbench/api/common/extHostTypeConverters.ts +++ b/src/vs/workbench/api/common/extHostTypeConverters.ts @@ -1506,6 +1506,20 @@ export namespace NotebookCellExecutionSummary { } } +export namespace NotebookCellExecutionState { + export function to(state: notebooks.NotebookCellExecutionState): vscode.NotebookCellExecutionState { + if (state === notebooks.NotebookCellExecutionState.Executing) { + return types.NotebookCellExecutionState.Executing; + } else if (state === notebooks.NotebookCellExecutionState.Pending) { + return types.NotebookCellExecutionState.Pending; + } else if (state === notebooks.NotebookCellExecutionState.Unconfirmed) { + return types.NotebookCellExecutionState.Pending; + } else { + throw new Error(`Unknown state: ${state}`); + } + } +} + export namespace NotebookCellKind { export function from(data: vscode.NotebookCellKind): notebooks.CellKind { switch (data) { diff --git a/src/vs/workbench/contrib/notebook/browser/contrib/cellStatusBar/executionStatusBarItemController.ts b/src/vs/workbench/contrib/notebook/browser/contrib/cellStatusBar/executionStatusBarItemController.ts index e3803c1dce1..82fdd9396e0 100644 --- a/src/vs/workbench/contrib/notebook/browser/contrib/cellStatusBar/executionStatusBarItemController.ts +++ b/src/vs/workbench/contrib/notebook/browser/contrib/cellStatusBar/executionStatusBarItemController.ts @@ -162,7 +162,7 @@ class ExecutionStateCellStatusBarItem extends Disposable { alignment: CellStatusbarAlignment.Left, priority: Number.MAX_SAFE_INTEGER }; - } else if (state === NotebookCellExecutionState.Pending) { + } else if (state === NotebookCellExecutionState.Pending || state === NotebookCellExecutionState.Unconfirmed) { return { text: `$(${pendingStateIcon.id})`, tooltip: localize('notebook.cell.status.pending', "Pending"), diff --git a/src/vs/workbench/contrib/notebook/browser/notebookExecutionServiceImpl.ts b/src/vs/workbench/contrib/notebook/browser/notebookExecutionServiceImpl.ts index 68b00019de6..000c7b3b72c 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookExecutionServiceImpl.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookExecutionServiceImpl.ts @@ -9,7 +9,7 @@ import { ILogService } from 'vs/platform/log/common/log'; import { IWorkspaceTrustRequestService } from 'vs/platform/workspace/common/workspaceTrust'; import { SELECT_KERNEL_ID } from 'vs/workbench/contrib/notebook/browser/controller/coreActions'; import { NotebookCellTextModel } from 'vs/workbench/contrib/notebook/common/model/notebookCellTextModel'; -import { CellKind, INotebookTextModel } from 'vs/workbench/contrib/notebook/common/notebookCommon'; +import { CellKind, INotebookTextModel, NotebookCellExecutionState } from 'vs/workbench/contrib/notebook/common/notebookCommon'; import { INotebookExecutionService } from 'vs/workbench/contrib/notebook/common/notebookExecutionService'; import { INotebookExecutionStateService } from 'vs/workbench/contrib/notebook/common/notebookExecutionStateService'; import { INotebookKernelService } from 'vs/workbench/contrib/notebook/common/notebookKernelService'; @@ -45,7 +45,7 @@ export class NotebookExecutionService implements INotebookExecutionService { return; } - const cellHandles: number[] = []; + const executeCells: NotebookCellTextModel[] = []; for (const cell of cellsArr) { const cellExe = this._notebookExecutionStateService.getCellExecution(cell.uri); if (cell.cellKind !== CellKind.Code || !!cellExe) { @@ -54,12 +54,19 @@ export class NotebookExecutionService implements INotebookExecutionService { if (!kernel.supportedLanguages.includes(cell.language)) { continue; } - cellHandles.push(cell.handle); + executeCells.push(cell); } - if (cellHandles.length > 0) { + if (executeCells.length > 0) { this._notebookKernelService.selectKernelForNotebook(kernel, notebook); - await kernel.executeNotebookCellsRequest(notebook.uri, cellHandles); + + const exes = executeCells.map(c => this._notebookExecutionStateService.createCellExecution(kernel!.id, notebook.uri, c.handle)); + await kernel.executeNotebookCellsRequest(notebook.uri, executeCells.map(c => c.handle)); + const unconfirmed = exes.filter(exe => exe.state === NotebookCellExecutionState.Unconfirmed); + if (unconfirmed.length) { + this._logService.debug(`NotebookExecutionService#executeNotebookCells completing unconfirmed executions ${JSON.stringify(unconfirmed.map(exe => exe.cellHandle))}`); + unconfirmed.forEach(exe => exe.complete({})); + } } } diff --git a/src/vs/workbench/contrib/notebook/browser/notebookExecutionStateServiceImpl.ts b/src/vs/workbench/contrib/notebook/browser/notebookExecutionStateServiceImpl.ts index bf3c36a044e..be0522ab323 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookExecutionStateServiceImpl.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookExecutionStateServiceImpl.ts @@ -260,7 +260,7 @@ class CellExecution extends Disposable implements INotebookCellExecution { private readonly _onDidComplete = this._register(new Emitter()); readonly onDidComplete = this._onDidComplete.event; - private _state: NotebookCellExecutionState = NotebookCellExecutionState.Pending; + private _state: NotebookCellExecutionState = NotebookCellExecutionState.Unconfirmed; get state() { return this._state; } @@ -308,6 +308,12 @@ class CellExecution extends Disposable implements INotebookCellExecution { this._logService.debug(`CellExecution#updateExecution ${this.getCellLog()}, [${updateTypes}]`); } + confirm() { + this._logService.debug(`CellExecution#confirm ${this.getCellLog()}`); + this._state = NotebookCellExecutionState.Pending; + this._onDidUpdate.fire(); + } + update(updates: ICellExecuteUpdate[]): void { this.logUpdates(updates); if (updates.some(u => u.editType === CellExecutionUpdateType.ExecutionState)) { diff --git a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellContextKeys.ts b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellContextKeys.ts index 1db19ff74ae..55f58c4ac72 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellContextKeys.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellContextKeys.ts @@ -136,7 +136,7 @@ export class CellContextKeyManager extends Disposable { } else if (exeState?.state === NotebookCellExecutionState.Executing) { this.cellRunState.set('executing'); this.cellExecuting.set(true); - } else if (exeState?.state === NotebookCellExecutionState.Pending) { + } else if (exeState?.state === NotebookCellExecutionState.Pending || exeState?.state === NotebookCellExecutionState.Unconfirmed) { this.cellRunState.set('pending'); this.cellExecuting.set(true); } else if (internalMetadata.lastRunSuccess === true) { diff --git a/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCellExecutionIcon.ts b/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCellExecutionIcon.ts index 02820d8787a..cd463a01580 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCellExecutionIcon.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCellExecutionIcon.ts @@ -73,7 +73,7 @@ export class CodeCellExecutionIcon extends Disposable { text: `$(${errorStateIcon.id})`, tooltip: localize('notebook.cell.status.failed', "Failed"), }; - } else if (state === NotebookCellExecutionState.Pending) { + } else if (state === NotebookCellExecutionState.Pending || state === NotebookCellExecutionState.Unconfirmed) { return { text: `$(${pendingStateIcon.id})`, tooltip: localize('notebook.cell.status.pending', "Pending"), diff --git a/src/vs/workbench/contrib/notebook/common/notebookCommon.ts b/src/vs/workbench/contrib/notebook/common/notebookCommon.ts index cb3b462bf55..d9d6dca87e1 100644 --- a/src/vs/workbench/contrib/notebook/common/notebookCommon.ts +++ b/src/vs/workbench/contrib/notebook/common/notebookCommon.ts @@ -83,8 +83,8 @@ export enum NotebookRunState { export type NotebookDocumentMetadata = Record; -// Aligns with the vscode.d.ts version export enum NotebookCellExecutionState { + Unconfirmed = 1, Pending = 2, Executing = 3 } diff --git a/src/vs/workbench/contrib/notebook/common/notebookExecutionStateService.ts b/src/vs/workbench/contrib/notebook/common/notebookExecutionStateService.ts index b3146bfcab3..0a9902a5ff4 100644 --- a/src/vs/workbench/contrib/notebook/common/notebookExecutionStateService.ts +++ b/src/vs/workbench/contrib/notebook/common/notebookExecutionStateService.ts @@ -60,6 +60,7 @@ export interface INotebookCellExecution { readonly didPause: boolean; readonly isPaused: boolean; + confirm(): void; update(updates: ICellExecuteUpdate[]): void; complete(complete: ICellExecutionComplete): void; } diff --git a/src/vs/workbench/contrib/notebook/test/browser/notebookExecutionService.test.ts b/src/vs/workbench/contrib/notebook/test/browser/notebookExecutionService.test.ts index 47637282a4e..aebc72563ab 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/notebookExecutionService.test.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/notebookExecutionService.test.ts @@ -19,6 +19,7 @@ import { NotebookKernelService } from 'vs/workbench/contrib/notebook/browser/not import { NotebookViewModel } from 'vs/workbench/contrib/notebook/browser/viewModel/notebookViewModelImpl'; import { NotebookTextModel } from 'vs/workbench/contrib/notebook/common/model/notebookTextModel'; import { CellKind, IOutputDto, NotebookCellMetadata } from 'vs/workbench/contrib/notebook/common/notebookCommon'; +import { INotebookExecutionStateService } from 'vs/workbench/contrib/notebook/common/notebookExecutionStateService'; import { INotebookKernel, INotebookKernelService, ISelectedNotebooksChangeEvent } from 'vs/workbench/contrib/notebook/common/notebookKernelService'; import { INotebookService } from 'vs/workbench/contrib/notebook/common/notebookService'; import { setupInstantiationService, withTestNotebook as _withTestNotebook } from 'vs/workbench/contrib/notebook/test/browser/testNotebookEditor'; @@ -135,6 +136,34 @@ suite('NotebookExecutionService', () => { assert.strictEqual(event.oldKernel, undefined); }); }); + + test('Completes unconfirmed executions', async function () { + + return withTestNotebook([], async viewModel => { + let didExecute = false; + const kernel = new class extends TestNotebookKernel { + constructor() { + super({ languages: ['javascript'] }); + this.id = 'mySpecialId'; + } + + override async executeNotebookCellsRequest() { + didExecute = true; + return; + } + }; + + kernelService.registerKernel(kernel); + const executionService = instantiationService.createInstance(NotebookExecutionService); + const exeStateService = instantiationService.get(INotebookExecutionStateService); + + const cell = insertCellAtIndex(viewModel, 0, 'var c = 3', 'javascript', CellKind.Code, {}, [], true, true); + await executionService.executeNotebookCells(viewModel.notebookDocument, [cell]); + + assert.strictEqual(didExecute, true); + assert.strictEqual(exeStateService.getCellExecution(cell.uri), undefined); + }); + }); }); class TestNotebookKernel implements INotebookKernel { diff --git a/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts b/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts index c82d86ce940..acccef7d0e4 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts @@ -44,7 +44,7 @@ import { CellViewModel, NotebookViewModel } from 'vs/workbench/contrib/notebook/ import { ViewContext } from 'vs/workbench/contrib/notebook/browser/viewModel/viewContext'; import { NotebookCellTextModel } from 'vs/workbench/contrib/notebook/common/model/notebookCellTextModel'; import { NotebookTextModel } from 'vs/workbench/contrib/notebook/common/model/notebookTextModel'; -import { CellKind, CellUri, INotebookDiffEditorModel, INotebookEditorModel, INotebookSearchOptions, IOutputDto, IResolvedNotebookEditorModel, NotebookCellMetadata, SelectionStateType } from 'vs/workbench/contrib/notebook/common/notebookCommon'; +import { CellKind, CellUri, INotebookDiffEditorModel, INotebookEditorModel, INotebookSearchOptions, IOutputDto, IResolvedNotebookEditorModel, NotebookCellExecutionState, NotebookCellMetadata, SelectionStateType } from 'vs/workbench/contrib/notebook/common/notebookCommon'; import { ICellExecuteUpdate, ICellExecutionComplete, ICellExecutionStateChangedEvent, INotebookCellExecution, INotebookExecutionStateService } from 'vs/workbench/contrib/notebook/common/notebookExecutionStateService'; import { NotebookOptions } from 'vs/workbench/contrib/notebook/common/notebookOptions'; import { ICellRange } from 'vs/workbench/contrib/notebook/common/notebookRange'; @@ -52,6 +52,7 @@ import { TextModelResolverService } from 'vs/workbench/services/textmodelResolve import { TestWorkspaceTrustRequestService } from 'vs/workbench/services/workspaces/test/common/testWorkspaceTrustService'; import { TestClipboardService, TestLayoutService } from 'vs/workbench/test/browser/workbenchTestServices'; import { TestStorageService } from 'vs/workbench/test/common/workbenchTestServices'; +import { ResourceMap } from 'vs/base/common/map'; export class TestCell extends NotebookCellTextModel { constructor( @@ -378,9 +379,34 @@ export function valueBytesFromString(value: string): VSBuffer { return VSBuffer.fromString(value); } +class TestCellExecution implements INotebookCellExecution { + constructor( + readonly notebook: URI, + readonly cellHandle: number, + private onComplete: () => void, + ) { } + + readonly state: NotebookCellExecutionState = NotebookCellExecutionState.Unconfirmed; + + readonly didPause: boolean = false; + readonly isPaused: boolean = false; + + confirm(): void { + } + + update(updates: ICellExecuteUpdate[]): void { + } + + complete(complete: ICellExecutionComplete): void { + this.onComplete(); + } +} + class TestNotebookExecutionStateService implements INotebookExecutionStateService { _serviceBrand: undefined; + private _executions = new ResourceMap(); + onDidChangeCellExecution = new Emitter().event; forceCancelNotebookExecutions(notebookUri: URI): void { @@ -391,16 +417,13 @@ class TestNotebookExecutionStateService implements INotebookExecutionStateServic } getCellExecution(cellUri: URI): INotebookCellExecution | undefined { - return undefined; + return this._executions.get(cellUri); } createCellExecution(controllerId: string, notebook: URI, cellHandle: number): INotebookCellExecution { - return undefined!; - } - - updateNotebookCellExecution(notebook: URI, cellHandle: number, updates: ICellExecuteUpdate[]): void { - } - - completeNotebookCellExecution(notebook: URI, cellHandle: number, complete: ICellExecutionComplete): void { + const onComplete = () => this._executions.delete(CellUri.generate(notebook, cellHandle)); + const exe = new TestCellExecution(notebook, cellHandle, onComplete); + this._executions.set(CellUri.generate(notebook, cellHandle), exe); + return exe; } }