Skip to content

Commit

Permalink
Add ability to sync INotebookModel with cell text edits (#12092)
Browse files Browse the repository at this point in the history
For #10496
  • Loading branch information
DonJayamanne authored Jun 1, 2020
1 parent 722d81b commit feb74dc
Show file tree
Hide file tree
Showing 9 changed files with 234 additions and 32 deletions.
23 changes: 1 addition & 22 deletions src/client/common/application/notebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Licensed under the MIT License.

import { inject, injectable } from 'inversify';
import { Disposable, Event, EventEmitter, GlobPattern, TextDocument, window } from 'vscode';
import { Disposable, Event, EventEmitter, GlobPattern } from 'vscode';
import type {
notebook,
NotebookCellsChangeEvent as VSCNotebookCellsChangeEvent,
Expand Down Expand Up @@ -45,19 +45,6 @@ export class VSCodeNotebook implements IVSCodeNotebook {
if (!this.useProposedApi) {
return;
}
// Temporary, currently VSC API doesn't work well.
// `notebook.activeNotebookEditor` is not reset when opening another file.
if (!this.notebook.activeNotebookEditor) {
return;
}
// If we have a text editor opened and it is not a cell, then we know for certain a notebook is not open.
if (window.activeTextEditor && !this.isCell(window.activeTextEditor.document)) {
return;
}
// Temporary until VSC API stabilizes.
if (Array.isArray(this.notebook.visibleNotebookEditors)) {
return this.notebook.visibleNotebookEditors.find((item) => item.active && item.visible);
}
return this.notebook.activeNotebookEditor;
}
private get notebook() {
Expand Down Expand Up @@ -94,14 +81,6 @@ export class VSCodeNotebook implements IVSCodeNotebook {
): Disposable {
return this.notebook.registerNotebookOutputRenderer(id, outputSelector, renderer);
}
public isCell(textDocument: TextDocument) {
return (
(textDocument.uri.fsPath.toLowerCase().includes('.ipynb') &&
textDocument.uri.query.includes('notebook') &&
textDocument.uri.query.includes('cell')) ||
textDocument.uri.scheme.includes('vscode-notebook-cell')
);
}
private addEventHandlers() {
if (this.addedEventHandlers) {
return;
Expand Down
4 changes: 0 additions & 4 deletions src/client/common/application/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1468,10 +1468,6 @@ export interface IVSCodeNotebook {
>;
readonly notebookEditors: Readonly<NotebookEditor[]>;
readonly activeNotebookEditor: NotebookEditor | undefined;
/**
* Whether the current document is aCell.
*/
isCell(textDocument: TextDocument): boolean;
registerNotebookContentProvider(notebookType: string, provider: NotebookContentProvider): Disposable;

registerNotebookKernel(id: string, selectors: GlobPattern[], kernel: NotebookKernel): Disposable;
Expand Down
3 changes: 2 additions & 1 deletion src/client/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ export const JUPYTER_LANGUAGE = 'jupyter';

export const PYTHON_WARNINGS = 'PYTHONWARNINGS';

export const NotebookCellScheme = 'vscode-notebook-cell';
export const PYTHON = [
{ scheme: 'file', language: PYTHON_LANGUAGE },
{ scheme: 'untitled', language: PYTHON_LANGUAGE },
{ scheme: 'vscode-notebook', language: PYTHON_LANGUAGE },
{ scheme: 'vscode-notebook-cell', language: PYTHON_LANGUAGE }
{ scheme: NotebookCellScheme, language: PYTHON_LANGUAGE }
];
export const PYTHON_ALLFILES = [{ language: PYTHON_LANGUAGE }];

Expand Down
8 changes: 7 additions & 1 deletion src/client/common/utils/misc.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
'use strict';
import type { Uri } from 'vscode';
import type { TextDocument, Uri } from 'vscode';
import { NotebookCellScheme } from '../constants';
import { InterpreterUri } from '../installer/types';
import { IAsyncDisposable, IDisposable, Resource } from '../types';

Expand Down Expand Up @@ -72,3 +73,8 @@ export function isUri(resource?: Uri | any): resource is Uri {
const uri = resource as Uri;
return typeof uri.path === 'string' && typeof uri.scheme === 'string';
}

export function isNotebookCell(documentOrUri: TextDocument | Uri): boolean {
const uri = isUri(documentOrUri) ? documentOrUri : documentOrUri.uri;
return uri.scheme.includes(NotebookCellScheme);
}
114 changes: 114 additions & 0 deletions src/client/datascience/notebook/cellEditSyncService.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { inject, injectable } from 'inversify';
import { TextDocument, TextDocumentChangeEvent } from 'vscode';
import type { NotebookCell, NotebookDocument } from '../../../../typings/vscode-proposed';
import { splitMultilineString } from '../../../datascience-ui/common';
import { IExtensionSingleActivationService } from '../../activation/types';
import { IDocumentManager, IVSCodeNotebook } from '../../common/application/types';
import { NativeNotebook } from '../../common/experiments/groups';
import { IDisposable, IDisposableRegistry, IExperimentsManager } from '../../common/types';
import { isNotebookCell } from '../../common/utils/misc';
import { traceError } from '../../logging';
import { INotebookEditorProvider, INotebookModel } from '../types';

@injectable()
export class CellEditSyncService implements IExtensionSingleActivationService, IDisposable {
private readonly disposables: IDisposable[] = [];
private mappedDocuments = new WeakMap<TextDocument, { cellId: string; model: INotebookModel }>();
constructor(
@inject(IDocumentManager) private readonly documentManager: IDocumentManager,
@inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry,
@inject(IVSCodeNotebook) private readonly vscNotebook: IVSCodeNotebook,
@inject(INotebookEditorProvider) private readonly editorProvider: INotebookEditorProvider,
@inject(IExperimentsManager) private readonly experiment: IExperimentsManager
) {
disposableRegistry.push(this);
}
public dispose() {
while (this.disposables.length) {
this.disposables.pop()?.dispose(); //NOSONAR
}
}
public async activate(): Promise<void> {
if (!this.experiment.inExperiment(NativeNotebook.experiment)) {
return;
}
this.documentManager.onDidChangeTextDocument(this.onDidChangeTextDocument, this, this.disposables);
}

private onDidChangeTextDocument(e: TextDocumentChangeEvent) {
if (!isNotebookCell(e.document)) {
return;
}

const details = this.getEditorsAndCell(e.document);
if (!details) {
return;
}

const cell = details.model.cells.find((item) => item.id === details.cellId);
if (!cell) {
traceError(
`Syncing Cell Editor aborted, Unable to find corresponding ICell for ${e.document.uri.toString()}`,
new Error('ICell not found')
);
return;
}

cell.data.source = splitMultilineString(e.document.getText());
}

private getEditorsAndCell(cellDocument: TextDocument) {
if (this.mappedDocuments.has(cellDocument)) {
return this.mappedDocuments.get(cellDocument)!;
}

let document: NotebookDocument | undefined;
let cell: NotebookCell | undefined;
this.vscNotebook.notebookEditors.find((vscEditor) => {
const found = vscEditor.document.cells.find((item) => item.document === cellDocument);
if (found) {
document = vscEditor.document;
cell = found;
}
return !!found;
});

if (!document) {
traceError(
`Syncing Cell Editor aborted, Unable to find corresponding Notebook for ${cellDocument.uri.toString()}`,
new Error('Unable to find corresponding Notebook')
);
return;
}
if (!cell) {
traceError(
`Syncing Cell Editor aborted, Unable to find corresponding NotebookCell for ${cellDocument.uri.toString()}`,
new Error('Unable to find corresponding NotebookCell')
);
return;
}

// Check if we have an editor associated with this document.
const editor = this.editorProvider.editors.find((item) => item.file.toString() === document?.uri.toString());
if (!editor) {
traceError(
`Syncing Cell Editor aborted, Unable to find corresponding Editor for ${cellDocument.uri.toString()}`,
new Error('Unable to find corresponding Editor')
);
return;
}
if (!editor.model) {
traceError(
`Syncing Cell Editor aborted, Unable to find corresponding INotebookModel for ${cellDocument.uri.toString()}`,
new Error('No INotebookModel in editor')
);
return;
}

this.mappedDocuments.set(cellDocument, { model: editor.model, cellId: cell.metadata.custom!.cellId });
return this.mappedDocuments.get(cellDocument);
}
}
5 changes: 5 additions & 0 deletions src/client/datascience/notebook/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import { IExtensionSingleActivationService } from '../../activation/types';
import { IServiceManager } from '../../ioc/types';
import { CellEditSyncService } from './cellEditSyncService';
import { NotebookContentProvider } from './contentProvider';
import { NotebookExecutionService } from './executionService';
import { NotebookIntegration } from './integration';
Expand All @@ -26,4 +27,8 @@ export function registerTypes(serviceManager: IServiceManager) {
IExtensionSingleActivationService,
NotebookEditorProviderActivation
);
serviceManager.addSingleton<IExtensionSingleActivationService>(
IExtensionSingleActivationService,
CellEditSyncService
);
}
99 changes: 99 additions & 0 deletions src/test/datascience/notebook/cellEditSyncService.ds.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// Licensed under the MIT License.
// Copyright (c) Microsoft Corporation. All rights reserved.

// tslint:disable: no-var-requires no-require-imports no-invalid-this no-any

import * as path from 'path';
import * as sinon from 'sinon';
import { Position, Range, Uri, window } from 'vscode';
import { IVSCodeNotebook } from '../../../client/common/application/types';
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,
insertPythonCellAndWait,
swallowSavingOfNotebooks
} from './helper';

suite('DataScience - VSCode Notebook (Cell Edit Syncing)', function () {
this.timeout(10_000);

const templateIPynb = path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'src', 'test', 'datascience', 'test.ipynb');
let testIPynb: Uri;
let api: IExtensionTestApi;
let editorProvider: INotebookEditorProvider;
let vscNotebook: IVSCodeNotebook;
const disposables: IDisposable[] = [];
suiteSetup(async function () {
this.timeout(10_000);
api = await initialize();
if (!(await canRunTests())) {
return this.skip();
}
editorProvider = api.serviceContainer.get<INotebookEditorProvider>(INotebookEditorProvider);
vscNotebook = api.serviceContainer.get<IVSCodeNotebook>(IVSCodeNotebook);
});
suiteTeardown(() => closeNotebooksAndCleanUpAfterTests(disposables));
[true, false].forEach((isUntitled) => {
suite(isUntitled ? 'Untitled Notebook' : 'Existing Notebook', () => {
let model: INotebookModel;
setup(async () => {
sinon.restore();
await swallowSavingOfNotebooks();

// 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!;
await deleteAllCellsAndWait();
});
teardown(() => closeNotebooksAndCleanUpAfterTests(disposables));

async function assertTextInCell(cell: ICell, text: string) {
await waitForCondition(
async () => (cell.data.source as string[]).join('') === splitMultilineString(text).join(''),
1_000,
`Source; is not ${text}`
);
}
test('Insert and edit cell', async () => {
await insertPythonCellAndWait('HELLO');
const doc = vscNotebook.activeNotebookEditor?.document;
const cellEditor1 = window.visibleTextEditors.find(
(item) => doc?.cells.length && item.document.uri.toString() === doc?.cells[0].uri.toString()
);
await assertTextInCell(model.cells[0], 'HELLO');

// Edit cell.
await new Promise((resolve) =>
cellEditor1?.edit((editor) => {
editor.insert(new Position(0, 5), ' WORLD');
resolve();
})
);

await assertTextInCell(model.cells[0], 'HELLO WORLD');

//Clear cell text.
await new Promise((resolve) =>
cellEditor1?.edit((editor) => {
editor.delete(new Range(0, 0, 0, 'HELLO WORLD'.length));
resolve();
})
);

await assertTextInCell(model.cells[0], '');
});
});
});
});
4 changes: 2 additions & 2 deletions src/test/datascience/notebook/edit.ds.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { splitMultilineString } from '../../../datascience-ui/common';
import { createCodeCell, createMarkdownCell } from '../../../datascience-ui/common/cellFactory';
import { createEventHandler, IExtensionTestApi, TestEventHandler } from '../../common';
import { EXTENSION_ROOT_DIR_FOR_TESTS } from '../../constants';
import { closeActiveWindows, initialize } from '../../initialize';
import { initialize } from '../../initialize';
import {
canRunTests,
closeNotebooksAndCleanUpAfterTests,
Expand Down Expand Up @@ -42,7 +42,7 @@ suite('DataScience - VSCode Notebook (Edit)', function () {
}
editorProvider = api.serviceContainer.get<INotebookEditorProvider>(INotebookEditorProvider);
});
suiteTeardown(closeActiveWindows);
suiteTeardown(() => closeNotebooksAndCleanUpAfterTests(disposables));
[true, false].forEach((isUntitled) => {
suite(isUntitled ? 'Untitled Notebook' : 'Existing Notebook', () => {
let model: INotebookModel;
Expand Down
6 changes: 4 additions & 2 deletions src/test/datascience/notebook/interrupRestart.ds.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,13 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors', functio
await waitForCondition(async () => deferred.completed, 5_000, 'Execution not cancelled');
assertVSCCellIsIdle(cell);
});
test('Cancelling using VSC Command for cell (slow)', async () => {
test('Cancelling using VSC Command for cell (slow)', async function () {
// Fails due to VSC bugs.
return this.skip();
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 commands.executeCommand('notebook.cell.execute');
await commands.executeCommand('notebook.cell.execute', cell);

// Wait for cell to get busy.
await waitForCondition(async () => assertVSCCellIsRunning(cell), 15_000, 'Cell not being executed');
Expand Down

0 comments on commit feb74dc

Please sign in to comment.