Skip to content

Commit

Permalink
Model is in charge of updates to model & cells (#12640)
Browse files Browse the repository at this point in the history 
For #10496
Part 2 of #12604
Move code related to updates to INotebookModel into the corresponding INotebookModel class.
  • Loading branch information
@DonJayamanne
DonJayamanne committed Jun 29, 2020
1 parent 9891723 commit 538d913
Show file tree
Hide file tree
Showing 7 changed files with 187 additions and 164 deletions.
21 changes: 8 additions & 13 deletions src/client/datascience/notebook/cellEditSyncService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,19 @@
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 { traceError } from '../../common/logger';
import { IDisposable, IDisposableRegistry } from '../../common/types';
import { isNotebookCell } from '../../common/utils/misc';
import { INotebookEditorProvider, INotebookModel } from '../types';
import { VSCodeNotebookModel } from '../notebookStorage/vscNotebookModel';
import { INotebookEditorProvider } from '../types';
import { getOriginalCellId } from './helpers/cellMappers';

@injectable()
export class CellEditSyncService implements IExtensionSingleActivationService, IDisposable {
private readonly disposables: IDisposable[] = [];
private mappedDocuments = new WeakMap<TextDocument, { cellId: string; model: INotebookModel }>();
private mappedDocuments = new WeakMap<TextDocument, { cellId: string; model: VSCodeNotebookModel }>();
private nonJupyterNotebookDocuments = new WeakSet<TextDocument>();
constructor(
@inject(IDocumentManager) private readonly documentManager: IDocumentManager,
Expand Down Expand Up @@ -45,16 +45,7 @@ export class CellEditSyncService implements IExtensionSingleActivationService, I
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());
details.model.updateCellSource(details.cellId, e.document.getText());
}

private getEditorsAndCell(cellDocument: TextDocument) {
Expand Down Expand Up @@ -105,6 +96,10 @@ export class CellEditSyncService implements IExtensionSingleActivationService, I
return;
}

if (!(editor.model instanceof VSCodeNotebookModel)) {
throw new Error('Notebook Model is not of type VSCodeNotebookModel');
}

this.mappedDocuments.set(cellDocument, { model: editor.model, cellId: getOriginalCellId(cell)! });
return this.mappedDocuments.get(cellDocument);
}
Expand Down
33 changes: 17 additions & 16 deletions src/client/datascience/notebook/executionService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,8 @@ 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,
INotebookEditorProvider,
INotebookModel,
INotebookProvider
} from '../types';
import { VSCodeNotebookModel } from '../notebookStorage/vscNotebookModel';
import { IDataScienceErrorHandler, INotebook, INotebookEditorProvider, INotebookProvider } from '../types';
import { findMappedNotebookCellModel } from './helpers/cellMappers';
import {
handleUpdateDisplayDataMessage,
Expand Down Expand Up @@ -145,7 +140,9 @@ export class NotebookExecutionService implements INotebookExecutionService {
public cancelPendingExecutions(document: NotebookDocument): void {
this.pendingExecutionCancellations.get(document.uri.fsPath)?.forEach((cancellation) => cancellation.cancel()); // NOSONAR
}
private async getNotebookAndModel(document: NotebookDocument): Promise<{ model: INotebookModel; nb: INotebook }> {
private async getNotebookAndModel(
document: NotebookDocument
): Promise<{ model: VSCodeNotebookModel; nb: INotebook }> {
const model = await this.notebookStorage.load(document.uri, undefined, undefined, true);
const nb = await this.notebookProvider.getOrCreateNotebook({
identity: document.uri,
Expand All @@ -157,6 +154,9 @@ export class NotebookExecutionService implements INotebookExecutionService {
if (!nb) {
throw new Error('Unable to get Notebook object to run cell');
}
if (!(model instanceof VSCodeNotebookModel)) {
throw new Error('Notebook Model is not of type VSCodeNotebookModel');
}
return { model, nb };
}
private sendPerceivedCellExecute(runningStopWatch: StopWatch) {
Expand All @@ -170,7 +170,7 @@ export class NotebookExecutionService implements INotebookExecutionService {
}

private async executeIndividualCell(
notebookAndModel: Promise<{ model: INotebookModel; nb: INotebook }>,
notebookAndModel: Promise<{ model: VSCodeNotebookModel; nb: INotebook }>,
document: NotebookDocument,
cell: NotebookCell,
token: CancellationToken,
Expand Down Expand Up @@ -267,12 +267,12 @@ export class NotebookExecutionService implements INotebookExecutionService {
typeof cells[0].data.execution_count === 'number'
) {
const executionCount = cells[0].data.execution_count as number;
if (updateCellExecutionCount(cell, notebookCellModel, executionCount)) {
if (updateCellExecutionCount(model, cell, notebookCellModel, executionCount)) {
this.contentProvider.notifyChangesToDocument(document);
}
}

if (updateCellOutput(cell, notebookCellModel, rawCellOutput)) {
if (updateCellOutput(model, cell, notebookCellModel, rawCellOutput)) {
this.contentProvider.notifyChangesToDocument(document);
}
},
Expand All @@ -290,9 +290,10 @@ export class NotebookExecutionService implements INotebookExecutionService {
cell.metadata.statusMessage = '';

// Update metadata in our model.
const notebookCellModel = findMappedNotebookCellModel(cell, model.cells);
const cellModel = findMappedNotebookCellModel(cell, model.cells);
updateCellExecutionTimes(
notebookCellModel,
model,
cellModel,
cell.metadata.runStartTime,
cell.metadata.lastRunDuration
);
Expand All @@ -302,11 +303,11 @@ export class NotebookExecutionService implements INotebookExecutionService {
cell.metadata.runState = vscodeNotebookEnums.NotebookCellRunState.Error;
cell.metadata.statusMessage = getCellStatusMessageBasedOnFirstErrorOutput(
// tslint:disable-next-line: no-any
notebookCellModel.data.outputs as any
cellModel.data.outputs as any
);
}

updateVSCNotebookCellMetadata(cell.metadata, notebookCellModel);
updateVSCNotebookCellMetadata(cell.metadata, cellModel);
this.contentProvider.notifyChangesToDocument(document);
deferred.resolve(cell.metadata.runState);
}
Expand All @@ -332,7 +333,7 @@ export class NotebookExecutionService implements INotebookExecutionService {
/**
* Ensure we handle display data messages that can result in updates to other cells.
*/
private handleDisplayDataMessages(model: INotebookModel, document: NotebookDocument, nb?: INotebook) {
private handleDisplayDataMessages(model: VSCodeNotebookModel, document: NotebookDocument, nb?: INotebook) {
if (nb && !this.registeredIOPubListeners.has(nb)) {
this.registeredIOPubListeners.add(nb);
//tslint:disable-next-line:no-require-imports
Expand Down
123 changes: 18 additions & 105 deletions src/client/datascience/notebook/helpers/cellUpdateHelpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,18 @@
*/

import * as assert from 'assert';
import { createCellFrom } from '../../../../datascience-ui/common/cellFactory';
import {
NotebookCellLanguageChangeEvent,
NotebookCellOutputsChangeEvent,
NotebookCellsChangeEvent
} from '../../../common/application/types';
import { MARKDOWN_LANGUAGE } from '../../../common/constants';
import { traceError } from '../../../common/logger';
import { sendTelemetryEvent } from '../../../telemetry';
import { VSCodeNativeTelemetry } from '../../constants';
import { INotebookModel } from '../../types';
import { VSCodeNotebookModel } from '../../notebookStorage/vscNotebookModel';
import { findMappedNotebookCellModel } from './cellMappers';
import {
createCellFromVSCNotebookCell,
createVSCCellOutputsFromOutputs,
updateVSCNotebookCellMetadata
} from './helpers';
import { createCellFromVSCNotebookCell, updateVSCNotebookCellMetadata } from './helpers';
// tslint:disable-next-line: no-var-requires no-require-imports
const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed');

/**
* If a VS Code cell changes, then ensure we update the corresponding cell in our INotebookModel.
Expand All @@ -37,13 +30,17 @@ const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed'
*/
export function updateCellModelWithChangesToVSCCell(
change: NotebookCellsChangeEvent | NotebookCellOutputsChangeEvent | NotebookCellLanguageChangeEvent,
model: INotebookModel
model: VSCodeNotebookModel
): boolean | undefined | void {
switch (change.type) {
case 'changeCellOutputs':
return clearCellOutput(change, model);
case 'changeCellLanguage':
return changeCellLanguage(change, model);
// VSC Fires this event only when changing code cells from one language to another.
// If you change markdown to code &/or vice versa, thats treated as a cell being deleted and added.
// In the case of Jupyter cells, we don't care of the language changes from python to csharp.
// Why? Because today its not possible, hence there's nothing we need to do for now.
return false;
case 'changeCells':
return handleChangesToCells(change, model);
default:
Expand All @@ -58,7 +55,7 @@ export function updateCellModelWithChangesToVSCCell(
* However we are interested in cell output being cleared (when user clears output).
* @returns {boolean} Return `true` if NotebookDocument was updated/edited.
*/
function clearCellOutput(change: NotebookCellOutputsChangeEvent, model: INotebookModel): boolean {
function clearCellOutput(change: NotebookCellOutputsChangeEvent, model: VSCodeNotebookModel): boolean {
if (!change.cells.every((cell) => cell.outputs.length === 0)) {
return false;
}
Expand All @@ -73,65 +70,14 @@ function clearCellOutput(change: NotebookCellOutputsChangeEvent, model: INoteboo
// If a cell has been cleared, then clear the corresponding ICell (cell in INotebookModel).
change.cells.forEach((vscCell) => {
const cell = findMappedNotebookCellModel(vscCell, model.cells);
// tslint:disable-next-line: no-console
console.log(cell);
if (vscCell.cellKind === vscodeNotebookEnums.CellKind.Code) {
cell.data.execution_count = null;
}
if (cell.data.metadata.vscode) {
cell.data.metadata.vscode.start_execution_time = undefined;
cell.data.metadata.vscode.end_execution_time = undefined;
}
cell.data.outputs = [];
model.clearCellOutput(cell);
updateVSCNotebookCellMetadata(vscCell.metadata, cell);
model.update({
source: 'user',
kind: 'clear',
oldDirty: model.isDirty,
newDirty: true,
oldCells: [cell]
});
});

return true;
}

/**
* VS Code doesn't seem to fire this when changing between markdown & code.
* Its only fired when changing the language from python to csharp.
* https://github.com/microsoft/vscode/issues/100042
*/
function changeCellLanguage(change: NotebookCellLanguageChangeEvent, model: INotebookModel) {
const cellModel = findMappedNotebookCellModel(change.cell, model.cells);
if (
(change.cell.cellKind === vscodeNotebookEnums.CellKind.Markdown && cellModel.data.cell_type === 'markdown') ||
(change.cell.cellKind === vscodeNotebookEnums.CellKind.Code && cellModel.data.cell_type === 'code')
) {
// This is when user changes from python to csharp or similar.
return;
}
// Here we have changed from a code cell to markdown or vice versa.
const cellData = createCellFrom(cellModel.data, change.language === MARKDOWN_LANGUAGE ? 'markdown' : 'code');
// tslint:disable-next-line: no-any
change.cell.outputs = createVSCCellOutputsFromOutputs(cellData.outputs as any);
change.cell.metadata.executionOrder = undefined;
change.cell.metadata.hasExecutionOrder = change.language !== MARKDOWN_LANGUAGE; // Do not check for Python, to support other languages
change.cell.metadata.runnable = change.language !== MARKDOWN_LANGUAGE; // Do not check for Python, to support other languages

// Create a new cell & replace old one.
const oldCellIndex = model.cells.indexOf(cellModel);
// tslint:disable-next-line: no-suspicious-comment
// TODO: CHANGE.
// tslint:disable-next-line: no-any
(model.cells as any)[oldCellIndex] = createCellFromVSCNotebookCell(change.cell, model);
sendTelemetryEvent(
change.cell.cellKind === vscodeNotebookEnums.CellKind.Markdown
? VSCodeNativeTelemetry.ChangeToMarkdown
: VSCodeNativeTelemetry.ChangeToCode
);
}

function handleChangesToCells(change: NotebookCellsChangeEvent, model: INotebookModel) {
function handleChangesToCells(change: NotebookCellsChangeEvent, model: VSCodeNotebookModel) {
if (isCellMoveChange(change)) {
handleCellMove(change, model);
sendTelemetryEvent(VSCodeNativeTelemetry.MoveCell);
Expand Down Expand Up @@ -171,59 +117,26 @@ function isCellInsertion(change: NotebookCellsChangeEvent) {
return change.changes.length === 1 && change.changes[0].deletedCount === 0 && change.changes[0].items.length > 0;
}

function handleCellMove(change: NotebookCellsChangeEvent, model: INotebookModel) {
function handleCellMove(change: NotebookCellsChangeEvent, model: VSCodeNotebookModel) {
assert.equal(change.changes.length, 2, 'When moving cells we must have only 2 changes');
const [, insertChange] = change.changes;
const cellToSwap = findMappedNotebookCellModel(insertChange.items[0]!, model.cells);
const cellToSwapWith = model.cells[insertChange.start];
assert.notEqual(cellToSwap, cellToSwapWith, 'Cannot swap cell with the same cell');

const indexOfCellToSwap = model.cells.indexOf(cellToSwap);
// tslint:disable-next-line: no-any
(model.cells as any)[insertChange.start] = cellToSwap;
// tslint:disable-next-line: no-any
(model.cells as any)[indexOfCellToSwap] = cellToSwapWith;
// Get model to fire events.
model.update({
source: 'user',
kind: 'swap',
firstCellId: cellToSwap.id,
secondCellId: cellToSwapWith.id,
oldDirty: model.isDirty,
newDirty: true
});
model.swapCells(cellToSwap, cellToSwapWith);
}
function handleCellInsertion(change: NotebookCellsChangeEvent, model: INotebookModel) {
function handleCellInsertion(change: NotebookCellsChangeEvent, model: VSCodeNotebookModel) {
assert.equal(change.changes.length, 1, 'When inserting cells we must have only 1 change');
assert.equal(change.changes[0].items.length, 1, 'Insertion of more than 1 cell is not supported');
const insertChange = change.changes[0];
const cell = change.changes[0].items[0];
const newCell = createCellFromVSCNotebookCell(cell, model);
// tslint:disable-next-line: no-any
(model.cells as any).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
});
model.addCell(newCell, insertChange.start);
}
function handleCellDelete(change: NotebookCellsChangeEvent, model: INotebookModel) {
function handleCellDelete(change: NotebookCellsChangeEvent, model: VSCodeNotebookModel) {
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');
// tslint:disable-next-line: no-any
const cellToRemove = (model.cells as any).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
});
const cellToRemove = model.cells[deletionChange.start];
model.deleteCell(cellToRemove);
}
Loading

0 comments on commit 538d913

Please sign in to comment.