Skip to content

Commit

Permalink
Fix missing events and hooks from INotebookEditor (microsoft#12570)
Browse files Browse the repository at this point in the history 
* Ensures a number of events have been wired up properly
* Added a number of tests (for these events)
  • Loading branch information
@DonJayamanne
DonJayamanne committed Jun 26, 2020
1 parent c70d8be commit 6a52460
Show file tree
Hide file tree
Showing 10 changed files with 251 additions and 43 deletions.
52 changes: 27 additions & 25 deletions src/client/datascience/interactive-ipynb/nativeEditorStorage.ts
View file
Expand Up @@ -119,7 +119,7 @@ export class NativeEditorNotebookModel implements INotebookModel {

constructor(
isTrusted: boolean,
public useNativeEditorApi: boolean,
public useVSCodeNotebookEditorApi: boolean,
file: Uri,
cells: ICell[],
json: Partial<nbformat.INotebookContent> = {},
Expand Down Expand Up @@ -149,30 +149,40 @@ export class NativeEditorNotebookModel implements INotebookModel {
}

public async applyEdits(edits: readonly NotebookModelChange[]): Promise<void> {
if (this.useVSCodeNotebookEditorApi) {
throw new Error('INotebookModel.applyEdits Not supported when using VSCode Notebooks');
}
edits.forEach((e) => this.update({ ...e, source: 'redo' }));
}
public async undoEdits(edits: readonly NotebookModelChange[]): Promise<void> {
if (this.useVSCodeNotebookEditorApi) {
throw new Error('INotebookModel.applyEdits Not supported when using VSCode Notebooks');
}
edits.forEach((e) => this.update({ ...e, source: 'undo' }));
}

public getContent(): string {
return this.generateNotebookContent();
}

public handleModelChange(change: NotebookModelChange) {
private handleModelChange(change: NotebookModelChange) {
const oldDirty = this.isDirty;
let changed = false;

switch (change.source) {
case 'redo':
case 'user':
changed = this.handleRedo(change);
break;
case 'undo':
changed = this.handleUndo(change);
break;
default:
break;
if (this.useVSCodeNotebookEditorApi) {
changed = true;
} else {
switch (change.source) {
case 'redo':
case 'user':
changed = this.handleRedo(change);
break;
case 'undo':
changed = this.handleUndo(change);
break;
default:
break;
}
}

// Forward onto our listeners if necessary
Expand Down Expand Up @@ -220,19 +230,11 @@ export class NativeEditorNotebookModel implements INotebookModel {
break;
case 'save':
this._state.saveChangeCount = this._state.changeCount;
// Trigger event.
if (this.useNativeEditorApi) {
changed = true;
}
break;
case 'saveAs':
this._state.saveChangeCount = this._state.changeCount;
this._state.changeCount = this._state.saveChangeCount = 0;
this._state.file = change.target;
// Trigger event.
if (this.useNativeEditorApi) {
changed = true;
}
break;
default:
break;
Expand Down Expand Up @@ -376,7 +378,7 @@ export class NativeEditorNotebookModel implements INotebookModel {
}

private clearOutputs(): boolean {
if (this.useNativeEditorApi) {
if (this.useVSCodeNotebookEditorApi) {
// Do not create new cells when using native editor.
// We'll update the cells in place (cuz undo/redo is handled by VS Code).
return true;
Expand Down Expand Up @@ -507,7 +509,7 @@ export function updateModelForUseWithVSCodeNotebook(model: INotebookModel) {
throw new Error('Unsupported NotebookModel');
}
const rawModel = model as NativeEditorNotebookModel;
rawModel.useNativeEditorApi = true;
rawModel.useVSCodeNotebookEditorApi = true;
}

@injectable()
Expand All @@ -530,7 +532,7 @@ export class NativeEditorStorage implements INotebookStorage {
@inject(IMemento) @named(GLOBAL_MEMENTO) private globalStorage: Memento,
@inject(IMemento) @named(WORKSPACE_MEMENTO) private localStorage: Memento,
@inject(ITrustService) private trustService: ITrustService,
@inject(UseVSCodeNotebookEditorApi) private readonly useNativeEditorApi: boolean
@inject(UseVSCodeNotebookEditorApi) private readonly useVSCodeNotebookEditorApi: boolean
) {}
private static isUntitledFile(file: Uri) {
return isUntitledFile(file);
Expand Down Expand Up @@ -730,7 +732,7 @@ export class NativeEditorStorage implements INotebookStorage {
} catch (ex) {
// May not exist at this time. Should always have a single cell though
traceError(`Failed to load notebook file ${file.toString()}`, ex);
return new NativeEditorNotebookModel(true, this.useNativeEditorApi, file, []);
return new NativeEditorNotebookModel(true, this.useVSCodeNotebookEditorApi, file, []);
}
}

Expand Down Expand Up @@ -798,7 +800,7 @@ export class NativeEditorStorage implements INotebookStorage {
: await this.trustService.isNotebookTrusted(file.toString(), contentsToCheck!);
return new NativeEditorNotebookModel(
isTrusted,
this.useNativeEditorApi,
this.useVSCodeNotebookEditorApi,
file,
remapped,
json,
Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/interactive-ipynb/nativeEditorViewTracker.ts
View file
Expand Up @@ -48,7 +48,7 @@ export class NativeEditorViewTracker implements IExtensionSingleActivationServic
const fileKey = editor.file.toString();

// Skip untitled files. They have to be changed first.
if (!list.includes(fileKey) && (!isUntitled(editor.model) || editor.model?.isDirty)) {
if (!list.includes(fileKey) && (!isUntitled(editor.model) || editor.isDirty)) {
this.workspaceMemento.update(MEMENTO_KEY, [...list, fileKey]);
} else if (isUntitled(editor.model) && editor.model) {
editor.model.changed(this.onUntitledChanged.bind(this, editor.file));
Expand Down
17 changes: 15 additions & 2 deletions src/client/datascience/notebook/executionService.ts
View file
Expand Up @@ -19,7 +19,13 @@ import { IServiceContainer } from '../../ioc/types';
import { captureTelemetry, sendTelemetryEvent } from '../../telemetry';
import { Commands, Telemetry, VSCodeNativeTelemetry } from '../constants';
import { INotebookStorageProvider } from '../interactive-ipynb/notebookStorageProvider';
import { IDataScienceErrorHandler, INotebook, INotebookModel, INotebookProvider } from '../types';
import {
IDataScienceErrorHandler,
INotebook,
INotebookEditorProvider,
INotebookModel,
INotebookProvider
} from '../types';
import { findMappedNotebookCellModel } from './helpers/cellMappers';
import {
handleUpdateDisplayDataMessage,
Expand All @@ -30,6 +36,7 @@ import {
updateCellWithErrorStatus
} from './helpers/executionHelpers';
import { getCellStatusMessageBasedOnFirstErrorOutput, updateVSCNotebookCellMetadata } from './helpers/helpers';
import { NotebookEditor } from './notebookEditor';
import { INotebookContentProvider, INotebookExecutionService } from './types';
// tslint:disable-next-line: no-var-requires no-require-imports
const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed');
Expand All @@ -55,7 +62,8 @@ export class NotebookExecutionService implements INotebookExecutionService {
@inject(IServiceContainer) private readonly serviceContainer: IServiceContainer,
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(IDataScienceErrorHandler) private readonly errorHandler: IDataScienceErrorHandler,
@inject(INotebookContentProvider) private readonly contentProvider: INotebookContentProvider
@inject(INotebookContentProvider) private readonly contentProvider: INotebookContentProvider,
@inject(INotebookEditorProvider) private readonly editorProvider: INotebookEditorProvider
) {}
@captureTelemetry(Telemetry.ExecuteNativeCell, undefined, true)
public async executeCell(document: NotebookDocument, cell: NotebookCell, token: CancellationToken): Promise<void> {
Expand Down Expand Up @@ -177,6 +185,10 @@ export class NotebookExecutionService implements INotebookExecutionService {
return;
}

const editor = this.editorProvider.editors.find((e) => e.model === model);
if (!(editor instanceof NotebookEditor)) {
throw new Error('Executing Notebook with another Editor');
}
// If we need to cancel this execution (from our code, due to kernel restarts or similar, then cancel).
const cancelExecution = new CancellationTokenSource();
if (!this.pendingExecutionCancellations.has(document.uri.fsPath)) {
Expand Down Expand Up @@ -222,6 +234,7 @@ export class NotebookExecutionService implements INotebookExecutionService {
let modelClearedEventHandler: IDisposable | undefined;
try {
nb.clear(cell.uri.fsPath); // NOSONAR
editor.notifyExecution(cell.document.getText());
const observable = nb.executeObservable(
cell.document.getText(),
document.fileName,
Expand Down
29 changes: 28 additions & 1 deletion src/client/datascience/notebook/helpers/cellUpdateHelpers.ts
View file
Expand Up @@ -178,6 +178,15 @@ function handleCellMove(change: NotebookCellsChangeEvent, model: INotebookModel)
const indexOfCellToSwap = model.cells.indexOf(cellToSwap);
model.cells[insertChange.start] = cellToSwap;
model.cells[indexOfCellToSwap] = cellToSwapWith;
// Get model to fire events.
model.update({
source: 'user',
kind: 'swap',
firstCellId: cellToSwap.id,
secondCellId: cellToSwapWith.id,
oldDirty: model.isDirty,
newDirty: true
});
}
function handleCellInsertion(change: NotebookCellsChangeEvent, model: INotebookModel) {
assert.equal(change.changes.length, 1, 'When inserting cells we must have only 1 change');
Expand All @@ -186,10 +195,28 @@ function handleCellInsertion(change: NotebookCellsChangeEvent, model: INotebookM
const cell = change.changes[0].items[0];
const newCell = createCellFromVSCNotebookCell(cell, model);
model.cells.splice(insertChange.start, 0, newCell);
// Get model to fire events.
model.update({
source: 'user',
kind: 'insert',
cell: newCell,
index: insertChange.start,
oldDirty: model.isDirty,
newDirty: true
});
}
function handleCellDelete(change: NotebookCellsChangeEvent, model: INotebookModel) {
assert.equal(change.changes.length, 1, 'When deleting cells we must have only 1 change');
const deletionChange = change.changes[0];
assert.equal(deletionChange.deletedCount, 1, 'Deleting more than one cell is not supported');
model.cells.splice(deletionChange.start, 1);
const cellToRemove = model.cells.splice(deletionChange.start, 1);
// Get model to fire events.
model.update({
source: 'user',
kind: 'remove',
cell: cellToRemove[0],
index: deletionChange.start,
oldDirty: model.isDirty,
newDirty: true
});
}
21 changes: 17 additions & 4 deletions src/client/datascience/notebook/notebookEditor.ts
View file
Expand Up @@ -7,7 +7,7 @@ import { CellKind, ConfigurationTarget, Event, EventEmitter, Uri, WebviewPanel }
import type { NotebookDocument } from 'vscode-proposed';
import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../common/application/types';
import { traceError } from '../../common/logger';
import { IConfigurationService } from '../../common/types';
import { IConfigurationService, IDisposableRegistry } from '../../common/types';
import { DataScience } from '../../common/utils/localize';
import { noop } from '../../common/utils/misc';
import { sendTelemetryEvent } from '../../telemetry';
Expand Down Expand Up @@ -46,7 +46,7 @@ export class NotebookEditor implements INotebookEditor {
return this.model.isUntitled;
}
public get isDirty(): boolean {
return this.model.isDirty;
return this.document.isDirty;
}
public get file(): Uri {
return this.model.file;
Expand Down Expand Up @@ -78,9 +78,18 @@ export class NotebookEditor implements INotebookEditor {
private readonly notebookProvider: INotebookProvider,
private readonly statusProvider: IStatusProvider,
private readonly applicationShell: IApplicationShell,
private readonly configurationService: IConfigurationService
private readonly configurationService: IConfigurationService,
disposables: IDisposableRegistry
) {
model.onDidEdit(() => this._modified.fire(this));
disposables.push(model.onDidEdit(() => this._modified.fire(this)));
disposables.push(
model.changed((e) => {
if (e.kind === 'save') {
this._saved.fire(this);
}
})
);
disposables.push(model.onDidDispose(this._closed.fire.bind(this._closed, this)));
}
public async load(_storage: INotebookModel, _webViewPanel?: WebviewPanel): Promise<void> {
// Not used.
Expand Down Expand Up @@ -122,6 +131,10 @@ export class NotebookEditor implements INotebookEditor {
}
});
}
public notifyExecution(code: string) {
this._executed.fire(this);
this.executedCode.fire(code);
}
public async interruptKernel(): Promise<void> {
this.executionService.cancelPendingExecutions(this.document);

Expand Down
10 changes: 7 additions & 3 deletions src/client/datascience/notebook/notebookEditorProvider.ts
View file
Expand Up @@ -69,7 +69,6 @@ export class NotebookEditorProvider implements INotebookEditorProvider {
@inject(INotebookStorageProvider) private readonly storage: INotebookStorageProvider,
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
@inject(INotebookExecutionService) private readonly executionService: INotebookExecutionService,
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
@inject(IStatusProvider) private readonly statusProvider: IStatusProvider,
Expand Down Expand Up @@ -150,6 +149,9 @@ export class NotebookEditorProvider implements INotebookEditorProvider {
}

private closedEditor(editor: INotebookEditor): void {
if (!(editor instanceof NotebookEditor)) {
throw new Error('Executing Notebook with another Editor');
}
this.openedEditors.delete(editor);
this._onDidCloseNotebookEditor.fire(editor);
}
Expand All @@ -166,16 +168,18 @@ export class NotebookEditorProvider implements INotebookEditorProvider {
let editor = this.notebookEditorsByUri.get(uri.toString());
if (!editor) {
const notebookProvider = this.serviceContainer.get<INotebookProvider>(INotebookProvider);
const executionService = this.serviceContainer.get<INotebookExecutionService>(INotebookExecutionService);
editor = new NotebookEditor(
model,
doc,
this.vscodeNotebook,
this.executionService,
executionService,
this.commandManager,
notebookProvider,
this.statusProvider,
this.appShell,
this.configurationService
this.configurationService,
this.disposables
);
this.onEditorOpened(editor);
}
Expand Down

0 comments on commit 6a52460

Please sign in to comment.