From d87e6efe0c4214c492f7792f651c37909894a36d Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 30 Jan 2020 16:37:45 -0800 Subject: [PATCH 01/23] Partially working undo/redo --- .../interactive-ipynb/nativeEditor.ts | 22 +++++++++++++------ .../interactive-ipynb/nativeEditorProvider.ts | 20 ++++++++++++----- .../interactive-ipynb/nativeEditorStorage.ts | 15 +++++++++---- src/client/datascience/types.ts | 11 +++++++--- 4 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/client/datascience/interactive-ipynb/nativeEditor.ts b/src/client/datascience/interactive-ipynb/nativeEditor.ts index a1701d2dbe3b..89e191b2aa0c 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditor.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditor.ts @@ -378,8 +378,11 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { // Filter out sysinfo messages. Don't want to show those const filtered = cells.filter(c => c.data.cell_type !== 'messages'); - // Update these cells in our storage - this.commandManager.executeCommand(Commands.NotebookStorage_ModifyCells, this.file, cells); + // Update these cells in our storage only when cells are finished + const finished = filtered.filter(c => c.state === CellState.finished || c.state === CellState.error); + if (finished.length > 0) { + this.commandManager.executeCommand(Commands.NotebookStorage_ModifyCells, this.file, finished); + } // Tell storage about our notebook object const notebook = this.getNotebook(); @@ -428,17 +431,22 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { // Actually don't close, just let the error bubble out } - private modelChanged(change: INotebookModelChange) { - if (change.isDirty !== undefined) { + private async modelChanged(change: INotebookModelChange) { + if (change.source === 'vscode' && change.newCells !== undefined) { + // VS code is telling us to broadcast this to our UI. Tell the UI to reload the cells + await this.postMessage(InteractiveWindowMessages.LoadAllCells, { cells: change.newCells }); + } + + if (change.newDirty !== undefined) { this.modifiedEvent.fire(); - if (change.model.isDirty) { - return this.postMessage(InteractiveWindowMessages.NotebookDirty); + if (change.newDirty) { + await this.postMessage(InteractiveWindowMessages.NotebookDirty); } else { // Going clean should only happen on a save (for now. Undo might do this too) this.savedEvent.fire(this); // Then tell the UI - return this.postMessage(InteractiveWindowMessages.NotebookClean); + await this.postMessage(InteractiveWindowMessages.NotebookClean); } } } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts index 99ddb356b6ca..45e4f82224aa 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts @@ -77,11 +77,19 @@ export class NativeEditorProvider implements INotebookEditorProvider, WebviewCus public get onEdit(): Event<{ readonly resource: Uri; readonly edit: INotebookEdit }> { return this._editEventEmitter.event; } - public applyEdits(_resource: Uri, _edits: readonly INotebookEdit[]): Thenable { - return Promise.resolve(); + public applyEdits(resource: Uri, edits: readonly INotebookEdit[]): Thenable { + return this.loadModel(resource).then(s => { + if (s) { + edits.forEach(e => s.update(e.newCells, e.newDirty)); + } + }); } - public undoEdits(_resource: Uri, _edits: readonly INotebookEdit[]): Thenable { - return Promise.resolve(); + public undoEdits(resource: Uri, edits: readonly INotebookEdit[]): Thenable { + return this.loadModel(resource).then(s => { + if (s) { + edits.forEach(e => s.update(e.oldCells, e.oldDirty)); + } + }); } public async resolveWebviewEditor(resource: Uri, panel: WebviewPanel) { try { @@ -220,8 +228,8 @@ export class NativeEditorProvider implements INotebookEditorProvider, WebviewCus this.models.set(change.newFile.toString(), promise!); } // If the cells change, tell VS code about it - if (change.newCells && change.isDirty) { - this._editEventEmitter.fire({ resource: file, edit: { contents: change.newCells } }); + if (change.newCells !== undefined && change.oldCells !== undefined && change.newDirty && change.oldDirty !== undefined && change.source !== 'vscode') { + this._editEventEmitter.fire({ resource: file, edit: { newCells: change.newCells, oldCells: change.oldCells, newDirty: change.newDirty, oldDirty: change.oldDirty } }); } } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 0281b1141699..706c1a78be52 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -207,6 +207,10 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID return this; } + public update(cells: ICell[], isDirty: boolean): void { + this.setState({ cells, isDirty, source: 'vscode' }); + } + public save(): Promise { return this.saveAs(this.file); } @@ -408,9 +412,10 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID } as any) as nbformat.ICell; // nyc (code coverage) barfs on this so just trick it. } - private setState(newState: Partial) { + private setState(newState: Partial & { source?: 'vscode' | 'internal' }) { let changed = false; - const change: INotebookModelChange = { model: this }; + const oldDirty = this.isDirty; + const change: INotebookModelChange = { model: this, source: newState.source ? newState.source : 'internal' }; if (newState.file) { change.newFile = newState.file; change.oldFile = this.file; @@ -425,13 +430,15 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID this._state.cells = newState.cells; // Force dirty on a cell change + change.oldDirty = oldDirty; this._state.isDirty = true; - change.isDirty = true; + change.newDirty = true; changed = true; } if (newState.isDirty !== undefined && newState.isDirty !== this._state.isDirty) { // This should happen on save all (to put back the dirty cell change) - change.isDirty = newState.isDirty; + change.newDirty = newState.isDirty; + change.oldDirty = oldDirty; this._state.isDirty = newState.isDirty; changed = true; } diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index 1943107e992a..e87a9faf644b 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -728,17 +728,21 @@ export interface IJupyterInterpreterDependencyManager { } export interface INotebookEdit { - readonly contents: ICell[]; + readonly newCells: ICell[]; + readonly oldCells: ICell[]; + readonly oldDirty: boolean; + readonly newDirty: boolean; } export interface INotebookModelChange { model: INotebookModel; newFile?: Uri; oldFile?: Uri; - isDirty?: boolean; - isUntitled?: boolean; + newDirty?: boolean; + oldDirty?: boolean; newCells?: ICell[]; oldCells?: ICell[]; + source: 'vscode' | 'internal'; } export interface INotebookModel { @@ -749,6 +753,7 @@ export interface INotebookModel { readonly cells: ICell[]; getJson(): Promise>; getContent(cells?: ICell[]): Promise; + update(cells: ICell[], isDirty: boolean): void; } export const INotebookStorage = Symbol('INotebookStorage'); From edc1f36e85584c03dbcd261aeeaf75c61b3e5488 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 31 Jan 2020 16:50:34 -0800 Subject: [PATCH 02/23] Concept created using update message/command --- src/client/common/application/commands.ts | 15 +- src/client/datascience/constants.ts | 11 +- .../intellisense/intellisenseDocument.ts | 112 +++--- .../intellisense/intellisenseProvider.ts | 154 ++++---- .../interactive-common/interactiveBase.ts | 8 - .../interactiveWindowTypes.ts | 132 ++++++- .../interactive-ipynb/nativeEditor.ts | 114 ++---- .../interactive-ipynb/nativeEditorProvider.ts | 48 ++- .../interactive-ipynb/nativeEditorStorage.ts | 360 +++++++++--------- .../interactive-window/interactiveWindow.ts | 17 +- src/client/datascience/types.ts | 23 +- .../history-react/redux/reducers/creation.ts | 21 +- .../interactive-common/mainState.ts | 3 +- .../interactive-common/redux/postOffice.ts | 8 +- .../redux/reducers/helpers.ts | 6 + .../redux/reducers/transfer.ts | 87 ++++- .../interactive-common/redux/store.ts | 1 + .../native-editor/redux/mapping.ts | 3 +- .../native-editor/redux/reducers/creation.ts | 91 ++++- .../native-editor/redux/reducers/execution.ts | 14 +- .../native-editor/redux/reducers/index.ts | 3 +- .../native-editor/redux/reducers/movement.ts | 35 +- .../datascience/intellisense.unit.test.ts | 104 ++++- .../nativeEditorStorage.unit.test.ts | 79 +++- .../datascience/mockCustomEditorService.ts | 7 +- 25 files changed, 918 insertions(+), 538 deletions(-) diff --git a/src/client/common/application/commands.ts b/src/client/common/application/commands.ts index d12d54546284..edbd7680638a 100644 --- a/src/client/common/application/commands.ts +++ b/src/client/common/application/commands.ts @@ -6,10 +6,8 @@ import { CancellationToken, Position, TextDocument, Uri } from 'vscode'; import { Commands as LSCommands } from '../../activation/languageServer/constants'; import { Commands as DSCommands } from '../../datascience/constants'; -import { IEditCell, IInsertCell, ISwapCells } from '../../datascience/interactive-common/interactiveWindowTypes'; -import { LiveKernelModel } from '../../datascience/jupyter/kernels/types'; -import { ICell, IJupyterKernelSpec, INotebook } from '../../datascience/types'; -import { PythonInterpreter } from '../../interpreter/contracts'; +import { NotebookModelChange } from '../../datascience/interactive-common/interactiveWindowTypes'; +import { INotebook } from '../../datascience/types'; import { CommandSource } from '../../testing/common/constants'; import { TestFunction, TestsToRun } from '../../testing/common/types'; import { TestDataItem, TestWorkspaceFolder } from '../../testing/types'; @@ -148,12 +146,5 @@ export interface ICommandNameArgumentTypeMapping extends ICommandNameWithoutArgu [DSCommands.ScrollToCell]: [string, string]; [DSCommands.ViewJupyterOutput]: []; [DSCommands.SwitchJupyterKernel]: [INotebook | undefined]; - [DSCommands.NotebookStorage_DeleteAllCells]: [Uri]; - [DSCommands.NotebookStorage_ModifyCells]: [Uri, ICell[]]; - [DSCommands.NotebookStorage_EditCell]: [Uri, IEditCell]; - [DSCommands.NotebookStorage_InsertCell]: [Uri, IInsertCell]; - [DSCommands.NotebookStorage_RemoveCell]: [Uri, string]; - [DSCommands.NotebookStorage_SwapCells]: [Uri, ISwapCells]; - [DSCommands.NotebookStorage_ClearCellOutputs]: [Uri]; - [DSCommands.NotebookStorage_UpdateVersion]: [Uri, PythonInterpreter | undefined, IJupyterKernelSpec | LiveKernelModel | undefined]; + [DSCommands.NotebookModel_Update]: [Uri, NotebookModelChange]; } diff --git a/src/client/datascience/constants.ts b/src/client/datascience/constants.ts index 774d782567cf..f4633ee2cf14 100644 --- a/src/client/datascience/constants.ts +++ b/src/client/datascience/constants.ts @@ -62,16 +62,7 @@ export namespace Commands { export const ScrollToCell = 'python.datascience.scrolltocell'; export const CreateNewNotebook = 'python.datascience.createnewnotebook'; export const ViewJupyterOutput = 'python.datascience.viewJupyterOutput'; - - // Make sure to put these into the package .json - export const NotebookStorage_DeleteAllCells = 'python.datascience.notebook.deleteall'; - export const NotebookStorage_ModifyCells = 'python.datascience.notebook.modifycells'; - export const NotebookStorage_EditCell = 'python.datascience.notebook.editcell'; - export const NotebookStorage_InsertCell = 'python.datascience.notebook.insertcell'; - export const NotebookStorage_RemoveCell = 'python.datascience.notebook.removecell'; - export const NotebookStorage_SwapCells = 'python.datascience.notebook.swapcells'; - export const NotebookStorage_ClearCellOutputs = 'python.datascience.notebook.clearoutputs'; - export const NotebookStorage_UpdateVersion = 'python.datascience.notebook.updateversion'; + export const NotebookModel_Update = 'python.datascience.notebook.update'; } export namespace CodeLensCommands { diff --git a/src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts b/src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts index 8b5f81b968f2..3bcc02baf0ea 100644 --- a/src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts +++ b/src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts @@ -3,12 +3,12 @@ 'use strict'; import '../../../common/extensions'; -import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api'; import { EndOfLine, Position, Range, TextDocument, TextDocumentContentChangeEvent, TextLine, Uri } from 'vscode'; import * as vscodeLanguageClient from 'vscode-languageclient'; import { PYTHON_LANGUAGE } from '../../../common/constants'; import { Identifiers } from '../../constants'; +import { ICellContentChange } from '../interactiveWindowTypes'; import { DefaultWordPattern, ensureValidWordDefinition, getWordAtText, regExpLeadsToEndlessLoop } from './wordHelper'; class IntellisenseLine implements TextLine { @@ -201,54 +201,57 @@ export class IntellisenseDocument implements TextDocument { } public loadAllCells(cells: { code: string; id: string }[]): TextDocumentContentChangeEvent[] { - let changes: TextDocumentContentChangeEvent[] = []; if (!this.inEditMode) { this.inEditMode = true; - this._version += 1; + return this.reloadCells(cells); + } + return []; + } - // Normalize all of the cells, removing \r and separating each - // with a newline - const normalized = cells.map(c => { - return { - id: c.id, - code: `${c.code.replace(/\r/g, '')}\n` - }; - }); + public reloadCells(cells: { code: string; id: string }[]): TextDocumentContentChangeEvent[] { + this._version += 1; + + // Normalize all of the cells, removing \r and separating each + // with a newline + const normalized = cells.map(c => { + return { + id: c.id, + code: `${c.code.replace(/\r/g, '')}\n` + }; + }); - // Contents are easy, just load all of the code in a row - this._contents = normalized - .map(c => c.code) - .reduce((p, c) => { - return `${p}${c}`; - }); - - // Cell ranges are slightly more complicated - let prev: number = 0; - this._cellRanges = normalized.map(c => { - const result = { - id: c.id, - start: prev, - fullEnd: prev + c.code.length, - currentEnd: prev + c.code.length - }; - prev += c.code.length; - return result; + // Contents are easy, just load all of the code in a row + this._contents = normalized + .map(c => c.code) + .reduce((p, c) => { + return `${p}${c}`; }); - // Then create the lines. - this._lines = this.createLines(); + // Cell ranges are slightly more complicated + let prev: number = 0; + this._cellRanges = normalized.map(c => { + const result = { + id: c.id, + start: prev, + fullEnd: prev + c.code.length, + currentEnd: prev + c.code.length + }; + prev += c.code.length; + return result; + }); - // Return our changes - changes = [ - { - range: this.createSerializableRange(new Position(0, 0), new Position(0, 0)), - rangeOffset: 0, - rangeLength: 0, // Adds are always zero - text: this._contents - } - ]; - } - return changes; + // Then create the lines. + this._lines = this.createLines(); + + // Return our changes + return [ + { + range: this.createSerializableRange(new Position(0, 0), new Position(0, 0)), + rangeOffset: 0, + rangeLength: 0, // Adds are always zero + text: this._contents + } + ]; } public addCell(fullCode: string, currentCode: string, id: string): TextDocumentContentChangeEvent[] { @@ -293,7 +296,24 @@ export class IntellisenseDocument implements TextDocument { ]; } - public insertCell(id: string, code: string, codeCellAbove: string | undefined): TextDocumentContentChangeEvent[] { + public reloadCell(id: string, code: string): TextDocumentContentChangeEvent[] { + this._version += 1; + + // Make sure to put a newline between this code and the next code + const newCode = `${code.replace(/\r/g, '')}\n`; + + // Figure where this goes + const index = this._cellRanges.findIndex(r => r.id === id); + if (index >= 0) { + const start = this.positionAt(this._cellRanges[index].start); + const end = this.positionAt(this._cellRanges[index].currentEnd); + return this.removeRange(newCode, start, end, index); + } + + return []; + } + + public insertCell(id: string, code: string, codeCellAboveOrIndex: string | undefined | number): TextDocumentContentChangeEvent[] { // This should only happen once for each cell. this._version += 1; @@ -301,8 +321,8 @@ export class IntellisenseDocument implements TextDocument { const newCode = `${code.replace(/\r/g, '')}\n`; // Figure where this goes - const aboveIndex = this._cellRanges.findIndex(r => r.id === codeCellAbove); - const insertIndex = aboveIndex + 1; + const aboveIndex = this._cellRanges.findIndex(r => r.id === codeCellAboveOrIndex); + const insertIndex = typeof codeCellAboveOrIndex === 'number' ? codeCellAboveOrIndex : aboveIndex + 1; // Compute where we start from. const fromOffset = insertIndex < this._cellRanges.length ? this._cellRanges[insertIndex].start : this._contents.length; @@ -356,7 +376,7 @@ export class IntellisenseDocument implements TextDocument { return []; } - public edit(editorChanges: monacoEditor.editor.IModelContentChange[], id: string): TextDocumentContentChangeEvent[] { + public editCell(editorChanges: ICellContentChange[], id: string): TextDocumentContentChangeEvent[] { this._version += 1; // Convert the range to local (and remove 1 based) diff --git a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts index 24ab4955477a..909000a12abd 100644 --- a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts +++ b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts @@ -21,13 +21,10 @@ import { noop } from '../../../common/utils/misc'; import { HiddenFileFormatString } from '../../../constants'; import { IInterpreterService, PythonInterpreter } from '../../../interpreter/contracts'; import { sendTelemetryWhenDone } from '../../../telemetry'; -import { Identifiers, Settings, Telemetry } from '../../constants'; -import { IInteractiveWindowListener, IInteractiveWindowProvider, IJupyterExecution, INotebook } from '../../types'; +import { Settings, Telemetry } from '../../constants'; +import { ICell, IInteractiveWindowListener, IInteractiveWindowProvider, IJupyterExecution, INotebook } from '../../types'; import { - IAddCell, ICancelIntellisenseRequest, - IEditCell, - IInsertCell, IInteractiveWindowMapping, ILoadAllCells, INotebookIdentity, @@ -35,9 +32,8 @@ import { IProvideCompletionItemsRequest, IProvideHoverRequest, IProvideSignatureHelpRequest, - IRemoveCell, IResolveCompletionItemRequest, - ISwapCells + NotebookModelChange } from '../interactiveWindowTypes'; import { convertStringsToSuggestions, @@ -109,28 +105,8 @@ export class IntellisenseProvider implements IInteractiveWindowListener { this.dispatchMessage(message, payload, this.handleResolveCompletionItemRequest); break; - case InteractiveWindowMessages.EditCell: - this.dispatchMessage(message, payload, this.editCell); - break; - - case InteractiveWindowMessages.AddCell: - this.dispatchMessage(message, payload, this.addCell); - break; - - case InteractiveWindowMessages.InsertCell: - this.dispatchMessage(message, payload, this.insertCell); - break; - - case InteractiveWindowMessages.RemoveCell: - this.dispatchMessage(message, payload, this.removeCell); - break; - - case InteractiveWindowMessages.SwapCells: - this.dispatchMessage(message, payload, this.swapCells); - break; - - case InteractiveWindowMessages.DeleteAllCells: - this.dispatchMessage(message, payload, this.removeAllCells); + case InteractiveWindowMessages.UpdateModel: + this.dispatchMessage(message, payload, this.update); break; case InteractiveWindowMessages.RestartKernel: @@ -518,63 +494,95 @@ export class IntellisenseProvider implements IInteractiveWindowListener { ); } - private async addCell(request: IAddCell): Promise { - // Save this request file as our potential resource - if (request.cell.file !== Identifiers.EmptyFileName) { - this.potentialResource = Uri.file(request.cell.file); - } - - // Get the document and then pass onto the sub class - const document = await this.getDocument(request.cell.file === Identifiers.EmptyFileName ? undefined : Uri.file(request.cell.file)); - if (document) { - const changes = document.addCell(request.fullText, request.currentText, request.cell.id); - return this.handleChanges(document, changes); + private async update(request: NotebookModelChange): Promise { + // See where this request is coming from + switch (request.source) { + case 'redo': + case 'user': + return this.handleRedo(request); + case 'undo': + return this.handleUndo(request); + default: + break; } } - private async insertCell(request: IInsertCell): Promise { - // Get the document and then pass onto the sub class - const document = await this.getDocument(); - if (document) { - const changes = document.insertCell(request.cell.id, request.code, request.codeCellAboveId); - return this.handleChanges(document, changes); - } + private convertToDocCells(cells: ICell[]): { code: string; id: string }[] { + return cells + .filter(c => c.data.cell_type === 'code') + .map(c => { + return { code: concatMultilineStringInput(c.data.source), id: c.id }; + }); } - private async editCell(request: IEditCell): Promise { - // First get the document + private async handleUndo(request: NotebookModelChange): Promise { const document = await this.getDocument(); - if (document) { - const changes = document.edit(request.changes, request.id); - return this.handleChanges(document, changes); + let changes: TextDocumentContentChangeEvent[] = []; + switch (request.kind) { + case 'clear': + // This one can be ignored, it only clears outputs + break; + case 'edit': + changes = document.reloadCell(request.cell.id, concatMultilineStringInput(request.cell.data.source)); + break; + case 'insert': + changes = document.remove(request.cell.id); + break; + case 'modify': + // This one can be ignored. it's only used for updating cell finished state. + break; + case 'remove': + changes = document.insertCell(request.cell.id, concatMultilineStringInput(request.cell.data.source), request.index); + break; + case 'remove_all': + changes = document.reloadCells(this.convertToDocCells(request.oldCells)); + break; + case 'swap': + changes = document.swap(request.secondCellId, request.firstCellId); + break; + case 'version': + // Also ignored. updates version which we don't keep track of. + break; + default: + break; } - } - private async removeCell(request: IRemoveCell): Promise { - // First get the document - const document = await this.getDocument(); - if (document) { - const changes = document.remove(request.id); - return this.handleChanges(document, changes); - } + return this.handleChanges(document, changes); } - private async swapCells(request: ISwapCells): Promise { - // First get the document + private async handleRedo(request: NotebookModelChange): Promise { const document = await this.getDocument(); - if (document) { - const changes = document.swap(request.firstCellId, request.secondCellId); - return this.handleChanges(document, changes); + let changes: TextDocumentContentChangeEvent[] = []; + switch (request.kind) { + case 'clear': + // This one can be ignored, it only clears outputs + break; + case 'edit': + changes = document.editCell(request.changes, request.cell.id); + break; + case 'insert': + changes = document.insertCell(request.cell.id, concatMultilineStringInput(request.cell.data.source), request.codeCellAboveId); + break; + case 'modify': + // This one can be ignored. it's only used for updating cell finished state. + break; + case 'remove': + changes = document.remove(request.cell.id); + break; + case 'remove_all': + changes = document.removeAll(); + break; + case 'swap': + changes = document.swap(request.firstCellId, request.secondCellId); + break; + case 'version': + // Also ignored. updates version which we don't keep track of. + break; + default: + break; } - } - private async removeAllCells(): Promise { - // First get the document - const document = await this.getDocument(); - if (document) { - const changes = document.removeAll(); - return this.handleChanges(document, changes); - } + return this.handleChanges(document, changes); } private async loadAllCells(payload: ILoadAllCells) { diff --git a/src/client/datascience/interactive-common/interactiveBase.ts b/src/client/datascience/interactive-common/interactiveBase.ts index f365aec71c06..da309b402b2d 100644 --- a/src/client/datascience/interactive-common/interactiveBase.ts +++ b/src/client/datascience/interactive-common/interactiveBase.ts @@ -202,14 +202,6 @@ export abstract class InteractiveBase extends WebViewHost { - super.removeAllCells(); - // Clear our visible cells in our model too. This should cause an update to the model - // that will fire off a changed event - this.commandManager.executeCommand(Commands.NotebookStorage_DeleteAllCells, this.file); - } - protected addSysInfo(_reason: SysInfoReason): Promise { // These are not supported. return Promise.resolve(); @@ -349,7 +311,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { line: 0, state: CellState.error } - ]); + ]).ignoreErrors(); // Tell the other side we restarted the kernel. This will stop all executions this.postMessage(InteractiveWindowMessages.RestartKernel).ignoreErrors(); @@ -374,22 +336,37 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { } } - protected sendCellsToWebView(cells: ICell[]) { + protected async sendCellsToWebView(cells: ICell[]) { // Filter out sysinfo messages. Don't want to show those const filtered = cells.filter(c => c.data.cell_type !== 'messages'); // Update these cells in our storage only when cells are finished - const finished = filtered.filter(c => c.state === CellState.finished || c.state === CellState.error); - if (finished.length > 0) { - this.commandManager.executeCommand(Commands.NotebookStorage_ModifyCells, this.file, finished); + const modified = filtered.filter(c => c.state === CellState.finished || c.state === CellState.error); + const unmodified = this._model?.cells.filter(c => modified.find(m => m.id === c.id)); + if (modified.length > 0 && unmodified && this._model) { + await this.commandManager.executeCommand(Commands.NotebookModel_Update, this.file, { + source: 'user', + kind: 'modify', + newCells: modified, + oldCells: unmodified, + oldDirty: this._model.isDirty, + newDirty: true + }); } // Tell storage about our notebook object const notebook = this.getNotebook(); - if (notebook) { + if (notebook && this._model) { const interpreter = notebook.getMatchingInterpreter(); const kernelSpec = notebook.getKernelSpec(); - this.commandManager.executeCommand(Commands.NotebookStorage_UpdateVersion, this.file, interpreter, kernelSpec); + this.commandManager.executeCommand(Commands.NotebookModel_Update, this.file, { + source: 'user', + kind: 'version', + oldDirty: this._model.isDirty, + newDirty: this._model.isDirty, + interpreter, + kernelSpec + }); } // Send onto the webview. @@ -431,15 +408,16 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { // Actually don't close, just let the error bubble out } - private async modelChanged(change: INotebookModelChange) { - if (change.source === 'vscode' && change.newCells !== undefined) { - // VS code is telling us to broadcast this to our UI. Tell the UI to reload the cells - await this.postMessage(InteractiveWindowMessages.LoadAllCells, { cells: change.newCells }); + private async modelChanged(change: NotebookModelChange) { + if (change.source !== 'user') { + // VS code is telling us to broadcast this to our UI. Tell the UI about the new change + await this.postMessage(InteractiveWindowMessages.UpdateModel, change); } - if (change.newDirty !== undefined) { + // Use the current state of the model to indicate dirty (not the message itself) + if (this._model && change.newDirty !== change.oldDirty) { this.modifiedEvent.fire(); - if (change.newDirty) { + if (this._model.isDirty) { await this.postMessage(InteractiveWindowMessages.NotebookDirty); } else { // Going clean should only happen on a save (for now. Undo might do this too) @@ -451,6 +429,13 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { } } + private updateModel(change: NotebookModelChange) { + // Send to our model using a command. User has done something that changes the model + if (change.source === 'user' && this._model) { + this.commandManager.executeCommand(Commands.NotebookModel_Update, this.file, change); + } + } + private async sendInitialCellsToWebView(cells: ICell[]): Promise { sendTelemetryEvent(Telemetry.CellCount, undefined, { count: cells.length }); return this.postMessage(InteractiveWindowMessages.LoadAllCells, { cells }); @@ -473,23 +458,6 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { } } - private async editCell(request: IEditCell) { - this.commandManager.executeCommand(Commands.NotebookStorage_EditCell, this.file, request); - } - - private async insertCell(request: IInsertCell): Promise { - this.commandManager.executeCommand(Commands.NotebookStorage_InsertCell, this.file, request); - } - - private async removeCell(request: IRemoveCell): Promise { - this.commandManager.executeCommand(Commands.NotebookStorage_RemoveCell, this.file, request.id); - } - - private async swapCells(request: ISwapCells): Promise { - // Swap two cells in our list - this.commandManager.executeCommand(Commands.NotebookStorage_SwapCells, this.file, request); - } - @captureTelemetry(Telemetry.ConvertToPythonFile, undefined, false) private async export(cells: ICell[]): Promise { const status = this.setStatus(localize.DataScience.convertingToPythonFile(), false); @@ -542,8 +510,4 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { sendTelemetryEvent(Telemetry.NotebookOpenTime, this.startupTimer.elapsedTime); } } - - private async clearAllOutputs() { - this.commandManager.executeCommand(Commands.NotebookStorage_ClearCellOutputs, this.file); - } } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts index 45e4f82224aa..2dd2e9a630e4 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts @@ -13,12 +13,13 @@ import * as localize from '../../common/utils/localize'; import { IServiceContainer } from '../../ioc/types'; import { captureTelemetry, sendTelemetryEvent } from '../../telemetry'; import { Identifiers, Settings, Telemetry } from '../constants'; -import { INotebookEdit, INotebookEditor, INotebookEditorProvider, INotebookModel, INotebookModelChange, INotebookServerOptions, INotebookStorage } from '../types'; +import { NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; +import { INotebookEditor, INotebookEditorProvider, INotebookModel, INotebookServerOptions, INotebookStorage } from '../types'; // Class that is registered as the custom editor provider for notebooks. VS code will call into this class when // opening an ipynb file. This class then creates a backing storage, model, and opens a view for the file. @injectable() -export class NativeEditorProvider implements INotebookEditorProvider, WebviewCustomEditorProvider, WebviewCustomEditorEditingDelegate, IAsyncDisposable { +export class NativeEditorProvider implements INotebookEditorProvider, WebviewCustomEditorProvider, WebviewCustomEditorEditingDelegate, IAsyncDisposable { // Note, this constant has to match the value used in the package.json to register the webview custom editor. public static readonly customEditorViewType = 'NativeEditorProvider.ipynb'; public get onDidChangeActiveNotebookEditor(): Event { @@ -29,7 +30,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, WebviewCus } private readonly _onDidChangeActiveNotebookEditor = new EventEmitter(); private readonly _onDidCloseNotebookEditor = new EventEmitter(); - private readonly _editEventEmitter = new EventEmitter<{ readonly resource: Uri; readonly edit: INotebookEdit }>(); + private readonly _editEventEmitter = new EventEmitter<{ readonly resource: Uri; readonly edit: NotebookModelChange }>(); private openedEditors: Set = new Set(); private models = new Map>(); private modelChangedHandlers: Map = new Map(); @@ -74,20 +75,20 @@ export class NativeEditorProvider implements INotebookEditorProvider, WebviewCus } }); } - public get onEdit(): Event<{ readonly resource: Uri; readonly edit: INotebookEdit }> { + public get onEdit(): Event<{ readonly resource: Uri; readonly edit: NotebookModelChange }> { return this._editEventEmitter.event; } - public applyEdits(resource: Uri, edits: readonly INotebookEdit[]): Thenable { - return this.loadModel(resource).then(s => { + public applyEdits(resource: Uri, edits: readonly NotebookModelChange[]): Thenable { + return this.loadStorage(resource).then(s => { if (s) { - edits.forEach(e => s.update(e.newCells, e.newDirty)); + edits.forEach(e => s.update({ ...e, source: 'redo' })); } }); } - public undoEdits(resource: Uri, edits: readonly INotebookEdit[]): Thenable { - return this.loadModel(resource).then(s => { + public undoEdits(resource: Uri, edits: readonly NotebookModelChange[]): Thenable { + return this.loadStorage(resource).then(s => { if (s) { - edits.forEach(e => s.update(e.oldCells, e.oldDirty)); + edits.forEach(e => s.update({ ...e, source: 'undo' })); } }); } @@ -220,16 +221,23 @@ export class NativeEditorProvider implements INotebookEditorProvider, WebviewCus } } - private async modelChanged(file: Uri, change: INotebookModelChange): Promise { - // If the file changes, update our storage - if (change.oldFile && change.newFile && this.models.has(change.oldFile.toString())) { - const promise = this.models.get(change.oldFile.toString()); - this.models.delete(change.oldFile.toString()); - this.models.set(change.newFile.toString(), promise!); - } - // If the cells change, tell VS code about it - if (change.newCells !== undefined && change.oldCells !== undefined && change.newDirty && change.oldDirty !== undefined && change.source !== 'vscode') { - this._editEventEmitter.fire({ resource: file, edit: { newCells: change.newCells, oldCells: change.oldCells, newDirty: change.newDirty, oldDirty: change.oldDirty } }); + private async modelChanged(file: Uri, change: NotebookModelChange): Promise { + // If the cells change because of a UI event, tell VS code about it + if (change.source === 'user') { + // Skip version and file changes. They can't be undone + switch (change.kind) { + case 'version': + break; + case 'file': + // Update our storage + const promise = this.models.get(change.oldFile.toString()); + this.models.delete(change.oldFile.toString()); + this.models.set(change.newFile.toString(), promise!); + break; + default: + this._editEventEmitter.fire({ resource: file, edit: change }); + break; + } } } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 706c1a78be52..8b4c9b40c8fb 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -1,5 +1,4 @@ import { nbformat } from '@jupyterlab/coreutils'; -import * as fastDeepEqual from 'fast-deep-equal'; import { inject, injectable, named } from 'inversify'; import * as path from 'path'; import * as uuid from 'uuid/v4'; @@ -13,10 +12,10 @@ import { GLOBAL_MEMENTO, ICryptoUtils, IDisposable, IDisposableRegistry, IExtens import { noop } from '../../common/utils/misc'; import { PythonInterpreter } from '../../interpreter/contracts'; import { Commands, Identifiers } from '../constants'; -import { IEditCell, IInsertCell, ISwapCells } from '../interactive-common/interactiveWindowTypes'; +import { ICellContentChange, NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; import { InvalidNotebookFileError } from '../jupyter/invalidNotebookFileError'; import { LiveKernelModel } from '../jupyter/kernels/types'; -import { CellState, ICell, IJupyterExecution, IJupyterKernelSpec, INotebookModel, INotebookModelChange, INotebookStorage } from '../types'; +import { CellState, ICell, IJupyterExecution, IJupyterKernelSpec, INotebookModel, INotebookStorage } from '../types'; // tslint:disable-next-line:no-require-imports no-var-requires import detectIndent = require('detect-indent'); @@ -36,7 +35,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID public get isDirty(): boolean { return this._state.isDirty; } - public get changed(): Event { + public get changed(): Event { return this._changedEmitter.event; } public get file(): Uri { @@ -52,7 +51,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID private static signedUpForCommands = false; private static storageMap = new Map(); - private _changedEmitter = new EventEmitter(); + private _changedEmitter = new EventEmitter(); private _state: INativeEditorStorageState = { file: Uri.file(''), isDirty: false, cells: [], notebookJson: {} }; private _loadPromise: Promise | undefined; private indentAmount: string = ' '; @@ -83,165 +82,219 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID return undefined; } - private static async handleEdit(s: NativeEditorStorage, request: IEditCell): Promise { + public dispose(): void { + NativeEditorStorage.storageMap.delete(this.file.toString()); + } + + public async load(file: Uri, possibleContents?: string): Promise { + // Reset the load promise and reload our cells + this._loadPromise = this.loadFromFile(file, possibleContents); + await this._loadPromise; + return this; + } + + public update(change: NotebookModelChange): void { + this.handleModelChange(change); + } + + public save(): Promise { + return this.saveAs(this.file); + } + + public async saveAs(file: Uri): Promise { + const contents = await this.getContent(); + await this.fileSystem.writeFile(file.fsPath, contents, 'utf-8'); + if (this.isDirty || file.fsPath !== this.file.fsPath) { + this.handleModelChange({ source: 'user', kind: 'file', newFile: file, oldFile: this.file, newDirty: false, oldDirty: this.isDirty }); + } + return this; + } + + public async getJson(): Promise> { + await this.ensureNotebookJson(); + return this._state.notebookJson; + } + + public getContent(cells?: ICell[]): Promise { + return this.generateNotebookContent(cells ? cells : this.cells); + } + + private handleModelChange(change: NotebookModelChange) { + const oldDirty = this.isDirty; + + switch (change.source) { + case 'redo': + case 'user': + this.handleRedo(change); + break; + case 'undo': + this.handleUndo(change); + break; + default: + break; + } + + // Forward onto our listeners (modifying dirty) + this._changedEmitter.fire({ ...change, newDirty: this.isDirty, oldDirty }); + } + + private handleRedo(change: NotebookModelChange) { + switch (change.kind) { + case 'clear': + this.clearOutputs(); + break; + case 'edit': + this.editCell(change.changes, change.cell.id); + break; + case 'insert': + this.insertCell(change.cell, change.index); + break; + case 'modify': + this.updateCellState(change.newCells); + break; + case 'remove': + this.removeCell(change.cell); + break; + case 'remove_all': + this.removeAllCells(change.newCellId); + break; + case 'swap': + this.swapCells(change.firstCellId, change.secondCellId); + break; + case 'version': + this.updateVersionInfo(change.interpreter, change.kernelSpec); + break; + default: + break; + } + this._state.isDirty = change.kind !== 'version'; + } + + private handleUndo(change: NotebookModelChange) { + switch (change.kind) { + case 'clear': + this._state.cells = change.oldCells; + break; + case 'edit': + this.replaceCell(change.cell); + break; + case 'insert': + this.removeCell(change.cell); + break; + case 'modify': + this.updateCellState(change.oldCells); + break; + case 'remove': + this.insertCell(change.cell, change.index); + break; + case 'remove_all': + this._state.cells = change.oldCells; + break; + case 'swap': + this.swapCells(change.firstCellId, change.secondCellId); + break; + case 'version': + // Not supporting undo + break; + default: + break; + } + + // Dirty state comes from undo + this._state.isDirty = change.oldDirty; + } + + private removeAllCells(newCellId: string) { + this._state.cells = []; + this._state.cells.push(this.createEmptyCell(newCellId)); + } + + private editCell(changes: ICellContentChange[], id: string) { // Apply the changes to the visible cell list. We won't get an update until // submission otherwise - if (request.changes && request.changes.length) { - const change = request.changes[0]; + if (changes && changes.length) { + const change = changes[0]; const normalized = change.text.replace(/\r/g, ''); // Figure out which cell we're editing. - const index = s.cells.findIndex(c => c.id === request.id); + const index = this.cells.findIndex(c => c.id === id); if (index >= 0) { // This is an actual edit. - const contents = concatMultilineStringInput(s.cells[index].data.source); + const contents = concatMultilineStringInput(this.cells[index].data.source); const before = contents.substr(0, change.rangeOffset); const after = contents.substr(change.rangeOffset + change.rangeLength); const newContents = `${before}${normalized}${after}`; if (contents !== newContents) { - const newCells = [...s.cells]; - const newCell = { ...newCells[index], data: { ...newCells[index].data, source: newContents } }; - newCells[index] = NativeEditorStorage.asCell(newCell); - s.setState({ cells: newCells }); + const newCell = { ...this.cells[index], data: { ...this.cells[index].data, source: newContents } }; + this._state.cells[index] = this.asCell(newCell); } } } } - private static async handleInsert(s: NativeEditorStorage, request: IInsertCell): Promise { - // Insert a cell into our visible list based on the index. They should be in sync - const newCells = [...s.cells]; - newCells.splice(request.index, 0, request.cell); - s.setState({ cells: newCells }); - } - - private static async handleRemoveCell(s: NativeEditorStorage, id: string): Promise { - // Filter our list - const newCells = [...s.cells].filter(v => v.id !== id); - if (newCells.length !== s.cells.length) { - s.setState({ cells: newCells }); - } - } - - private static async handleSwapCells(s: NativeEditorStorage, request: ISwapCells): Promise { - // Swap two cells in our list - const first = s.cells.findIndex(v => v.id === request.firstCellId); - const second = s.cells.findIndex(v => v.id === request.secondCellId); + private swapCells(firstCellId: string, secondCellId: string) { + const first = this.cells.findIndex(v => v.id === firstCellId); + const second = this.cells.findIndex(v => v.id === secondCellId); if (first >= 0 && second >= 0) { - const newCells = [...s.cells]; - const temp = { ...newCells[first] }; - newCells[first] = NativeEditorStorage.asCell(newCells[second]); - newCells[second] = NativeEditorStorage.asCell(temp); - s.setState({ cells: newCells }); + const temp = { ...this.cells[first] }; + this._state.cells[first] = this.asCell(this.cells[second]); + this._state.cells[second] = this.asCell(temp); } } - private static async handleDeleteAllCells(s: NativeEditorStorage): Promise { - if (s.cells.length !== 0) { - s.setState({ cells: [] }); - } + private updateCellState(cells: ICell[]) { + // Update these cells in our list + cells.forEach(c => { + const index = this.cells.findIndex(v => v.id === c.id); + this._state.cells[index] = this.asCell(c); + }); } - private static async handleClearAllOutputs(s: NativeEditorStorage): Promise { - const newCells = s.cells.map(c => { - return NativeEditorStorage.asCell({ ...c, data: { ...c.data, execution_count: null, outputs: [] } }); - }); + private removeCell(cell: ICell) { + this._state.cells = this.cells.filter(c => c.id !== cell.id); + } - // Do our check here to see if any changes happened. We don't want - // to fire an unnecessary change if we can help it. - if (!fastDeepEqual(s.cells, newCells)) { - s.setState({ cells: newCells }); + private replaceCell(cell: ICell) { + const index = this.cells.findIndex(c => c.id === cell.id); + if (index >= 0) { + this._state.cells[index] = cell; } } - private static async handleModifyCells(s: NativeEditorStorage, cells: ICell[]): Promise { - const newCells = [...s.cells]; - // Update these cells in our list - cells.forEach(c => { - const index = newCells.findIndex(v => v.id === c.id); - newCells[index] = NativeEditorStorage.asCell(c); - }); + private clearOutputs() { + this._state.cells = this.cells.map(c => this.asCell({ ...c, data: { ...c.data, execution_count: null, outputs: [] } })); + } - // Indicate dirty - if (!fastDeepEqual(newCells, s.cells)) { - s.setState({ cells: newCells, isDirty: true }); - } + private insertCell(cell: ICell, index: number) { + // Insert a cell into our visible list based on the index. They should be in sync + this._state.cells.splice(index, 0, cell); } - private static async handleUpdateVersionInfo( - s: NativeEditorStorage, - interpreter: PythonInterpreter | undefined, - kernelSpec: IJupyterKernelSpec | LiveKernelModel | undefined - ): Promise { + private updateVersionInfo(interpreter: PythonInterpreter | undefined, kernelSpec: IJupyterKernelSpec | LiveKernelModel | undefined) { // Get our kernel_info and language_info from the current notebook - if (interpreter && interpreter.version && s._state.notebookJson.metadata && s._state.notebookJson.metadata.language_info) { - s._state.notebookJson.metadata.language_info.version = interpreter.version.raw; + if (interpreter && interpreter.version && this._state.notebookJson.metadata && this._state.notebookJson.metadata.language_info) { + this._state.notebookJson.metadata.language_info.version = interpreter.version.raw; } - if (kernelSpec && s._state.notebookJson.metadata && !s._state.notebookJson.metadata.kernelspec) { + if (kernelSpec && this._state.notebookJson.metadata && !this._state.notebookJson.metadata.kernelspec) { // Add a new spec in this case - s._state.notebookJson.metadata.kernelspec = { + this._state.notebookJson.metadata.kernelspec = { name: kernelSpec.name || kernelSpec.display_name || '', display_name: kernelSpec.display_name || kernelSpec.name || '' }; - } else if (kernelSpec && s._state.notebookJson.metadata && s._state.notebookJson.metadata.kernelspec) { + } else if (kernelSpec && this._state.notebookJson.metadata && this._state.notebookJson.metadata.kernelspec) { // Spec exists, just update name and display_name - s._state.notebookJson.metadata.kernelspec.name = kernelSpec.name || kernelSpec.display_name || ''; - s._state.notebookJson.metadata.kernelspec.display_name = kernelSpec.display_name || kernelSpec.name || ''; + this._state.notebookJson.metadata.kernelspec.name = kernelSpec.name || kernelSpec.display_name || ''; + this._state.notebookJson.metadata.kernelspec.display_name = kernelSpec.display_name || kernelSpec.name || ''; } } // tslint:disable-next-line: no-any - private static asCell(cell: any): ICell { + private asCell(cell: any): ICell { + // Works around problems with setting a cell to another one in the nyc compiler. return cell as ICell; } - public dispose(): void { - NativeEditorStorage.storageMap.delete(this.file.toString()); - } - - public async load(file: Uri, possibleContents?: string): Promise { - // Reset the load promise and reload our cells - this._loadPromise = this.loadFromFile(file, possibleContents); - await this._loadPromise; - return this; - } - - public update(cells: ICell[], isDirty: boolean): void { - this.setState({ cells, isDirty, source: 'vscode' }); - } - - public save(): Promise { - return this.saveAs(this.file); - } - - public async saveAs(file: Uri): Promise { - const contents = await this.getContent(); - await this.fileSystem.writeFile(file.fsPath, contents, 'utf-8'); - if (this.isDirty || file.fsPath !== this.file.fsPath) { - this.setState({ isDirty: false, file }); - } - return this; - } - - public async getJson(): Promise> { - await this.ensureNotebookJson(); - return this._state.notebookJson; - } - - public getContent(cells?: ICell[]): Promise { - return this.generateNotebookContent(cells ? cells : this.cells); - } - - // tslint:disable-next-line: no-any - private async commandCallback(handler: (...any: [NativeEditorStorage, ...any[]]) => Promise, resource: Uri) { - const args = Array.prototype.slice.call(arguments).slice(2); - const storage = await NativeEditorStorage.getStorage(resource); - if (storage) { - return handler(storage, ...args); - } - } - private registerCommands(commandManager: ICommandManager, disposableRegistry: IDisposableRegistry): void { NativeEditorStorage.signedUpForCommands = true; disposableRegistry.push({ @@ -250,24 +303,19 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID } }); disposableRegistry.push( - commandManager.registerCommand(Commands.NotebookStorage_ClearCellOutputs, this.commandCallback.bind(undefined, NativeEditorStorage.handleClearAllOutputs)) - ); - disposableRegistry.push( - commandManager.registerCommand(Commands.NotebookStorage_DeleteAllCells, this.commandCallback.bind(undefined, NativeEditorStorage.handleDeleteAllCells)) - ); - disposableRegistry.push(commandManager.registerCommand(Commands.NotebookStorage_EditCell, this.commandCallback.bind(undefined, NativeEditorStorage.handleEdit))); - disposableRegistry.push(commandManager.registerCommand(Commands.NotebookStorage_InsertCell, this.commandCallback.bind(undefined, NativeEditorStorage.handleInsert))); - disposableRegistry.push(commandManager.registerCommand(Commands.NotebookStorage_ModifyCells, this.commandCallback.bind(undefined, NativeEditorStorage.handleModifyCells))); - disposableRegistry.push(commandManager.registerCommand(Commands.NotebookStorage_RemoveCell, this.commandCallback.bind(undefined, NativeEditorStorage.handleRemoveCell))); - disposableRegistry.push(commandManager.registerCommand(Commands.NotebookStorage_SwapCells, this.commandCallback.bind(undefined, NativeEditorStorage.handleSwapCells))); - disposableRegistry.push( - commandManager.registerCommand(Commands.NotebookStorage_UpdateVersion, this.commandCallback.bind(undefined, NativeEditorStorage.handleUpdateVersionInfo)) + commandManager.registerCommand(Commands.NotebookModel_Update, async (resource: Uri, change: NotebookModelChange) => { + // Find the appropriate storage object and call into it + const storage = await NativeEditorStorage.getStorage(resource); + if (storage) { + return storage.handleModelChange(change); + } + }) ); } private async loadFromFile(file: Uri, possibleContents?: string): Promise { // Save file - this.setState({ file }); + this._state.file = file; try { // Attempt to read the contents if a viable file @@ -284,13 +332,13 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID } } catch { // May not exist at this time. Should always have a single cell though - return [this.createEmptyCell()]; + return [this.createEmptyCell(uuid())]; } } - private createEmptyCell() { + private createEmptyCell(id: string) { return { - id: uuid(), + id, line: 0, file: Identifiers.EmptyFileName, state: CellState.finished, @@ -333,12 +381,13 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID // Make sure at least one if (remapped.length === 0) { - remapped.splice(0, 0, this.createEmptyCell()); + remapped.splice(0, 0, this.createEmptyCell(uuid())); forceDirty = true; } // Save as our visible list - this.setState({ cells: remapped, isDirty: forceDirty }); + this._state.cells = remapped; + this._state.isDirty = forceDirty; return this.cells; } @@ -412,41 +461,6 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID } as any) as nbformat.ICell; // nyc (code coverage) barfs on this so just trick it. } - private setState(newState: Partial & { source?: 'vscode' | 'internal' }) { - let changed = false; - const oldDirty = this.isDirty; - const change: INotebookModelChange = { model: this, source: newState.source ? newState.source : 'internal' }; - if (newState.file) { - change.newFile = newState.file; - change.oldFile = this.file; - this._state.file = change.newFile; - NativeEditorStorage.storageMap.delete(this.file.toString()); - NativeEditorStorage.storageMap.set(newState.file.toString(), this); - changed = true; - } - if (newState.cells) { - change.oldCells = this._state.cells; - change.newCells = newState.cells; - this._state.cells = newState.cells; - - // Force dirty on a cell change - change.oldDirty = oldDirty; - this._state.isDirty = true; - change.newDirty = true; - changed = true; - } - if (newState.isDirty !== undefined && newState.isDirty !== this._state.isDirty) { - // This should happen on save all (to put back the dirty cell change) - change.newDirty = newState.isDirty; - change.oldDirty = oldDirty; - this._state.isDirty = newState.isDirty; - changed = true; - } - if (changed) { - this._changedEmitter.fire(change); - } - } - private getStorageKey(): string { return `${KeyPrefix}${this.file.toString()}`; } diff --git a/src/client/datascience/interactive-window/interactiveWindow.ts b/src/client/datascience/interactive-window/interactiveWindow.ts index 911564318af4..5fac9b234bd3 100644 --- a/src/client/datascience/interactive-window/interactiveWindow.ts +++ b/src/client/datascience/interactive-window/interactiveWindow.ts @@ -16,7 +16,7 @@ import { IInterpreterService } from '../../interpreter/contracts'; import { captureTelemetry, sendTelemetryEvent } from '../../telemetry'; import { EditorContexts, Identifiers, Telemetry } from '../constants'; import { InteractiveBase } from '../interactive-common/interactiveBase'; -import { InteractiveWindowMessages, ISubmitNewCell, SysInfoReason } from '../interactive-common/interactiveWindowTypes'; +import { InteractiveWindowMessages, ISubmitNewCell, NotebookModelChange, SysInfoReason } from '../interactive-common/interactiveWindowTypes'; import { ProgressReporter } from '../progress/progressReporter'; import { ICell, @@ -185,6 +185,10 @@ export class InteractiveWindow extends InteractiveBase implements IInteractiveWi this.handleMessage(message, payload, this.handleReturnAllCells); break; + case InteractiveWindowMessages.UpdateModel: + this.handleMessage(message, payload, this.handleModelChange); + break; + default: break; } @@ -339,6 +343,17 @@ export class InteractiveWindow extends InteractiveBase implements IInteractiveWi } } + private handleModelChange(update: NotebookModelChange) { + // Send telemetry for delete and delete all. We don't send telemetry for the other updates yet + if (update.source === 'user') { + if (update.kind === 'remove_all') { + sendTelemetryEvent(Telemetry.DeleteAllCells); + } else if (update.kind === 'remove') { + sendTelemetryEvent(Telemetry.DeleteCell); + } + } + } + // tslint:disable-next-line:no-any private handleReturnAllCells(cells: ICell[]) { // See what we're waiting for. diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index e87a9faf644b..eeb4cbd77a69 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -14,6 +14,7 @@ import { IAsyncDisposable, IDataScienceSettings, IDisposable } from '../common/t import { StopWatch } from '../common/utils/stopWatch'; import { PythonInterpreter } from '../interpreter/contracts'; import { JupyterCommands } from './constants'; +import { NotebookModelChange } from './interactive-common/interactiveWindowTypes'; import { JupyterServerInfo } from './jupyter/jupyterConnection'; import { JupyterInstallError } from './jupyter/jupyterInstallError'; import { JupyterKernelSpec } from './jupyter/kernels/jupyterKernelSpec'; @@ -727,33 +728,14 @@ export interface IJupyterInterpreterDependencyManager { installMissingDependencies(err?: JupyterInstallError): Promise; } -export interface INotebookEdit { - readonly newCells: ICell[]; - readonly oldCells: ICell[]; - readonly oldDirty: boolean; - readonly newDirty: boolean; -} - -export interface INotebookModelChange { - model: INotebookModel; - newFile?: Uri; - oldFile?: Uri; - newDirty?: boolean; - oldDirty?: boolean; - newCells?: ICell[]; - oldCells?: ICell[]; - source: 'vscode' | 'internal'; -} - export interface INotebookModel { readonly file: Uri; readonly isDirty: boolean; readonly isUntitled: boolean; - readonly changed: Event; + readonly changed: Event; readonly cells: ICell[]; getJson(): Promise>; getContent(cells?: ICell[]): Promise; - update(cells: ICell[], isDirty: boolean): void; } export const INotebookStorage = Symbol('INotebookStorage'); @@ -762,4 +744,5 @@ export interface INotebookStorage { load(file: Uri, contents?: string): Promise; save(): Promise; saveAs(file: Uri): Promise; + update(change: NotebookModelChange): void; } diff --git a/src/datascience-ui/history-react/redux/reducers/creation.ts b/src/datascience-ui/history-react/redux/reducers/creation.ts index f06baaf97874..9de129f774a7 100644 --- a/src/datascience-ui/history-react/redux/reducers/creation.ts +++ b/src/datascience-ui/history-react/redux/reducers/creation.ts @@ -86,10 +86,15 @@ export namespace Creation { // We're adding a new cell here. Tell the intellisense engine we have a new cell arg.queueAction( - createPostableAction(InteractiveWindowMessages.AddCell, { + createPostableAction(InteractiveWindowMessages.UpdateModel, { + source: 'user', + kind: 'insert', + oldDirty: arg.prevState.dirty, + newDirty: true, + cell: cellVM.cell, fullText: extractInputText(cellVM, result.settings), currentText: cellVM.inputBlockText, - cell: cellVM.cell + index: result.cellVMs.findIndex(c => c.cell.id === arg.payload.id) - 1 }) ); } @@ -130,8 +135,16 @@ export namespace Creation { const index = arg.prevState.cellVMs.findIndex(c => c.cell.id === arg.payload.cellId); if (index >= 0 && arg.payload.cellId) { // Send messages to other side to indicate the delete - arg.queueAction(createPostableAction(InteractiveWindowMessages.DeleteCell)); - arg.queueAction(createPostableAction(InteractiveWindowMessages.RemoveCell, { id: arg.payload.cellId })); + arg.queueAction( + createPostableAction(InteractiveWindowMessages.UpdateModel, { + source: 'user', + kind: 'remove', + index, + oldDirty: arg.prevState.dirty, + newDirty: true, + cell: arg.prevState.cellVMs[index].cell + }) + ); const newVMs = arg.prevState.cellVMs.filter((_c, i) => i !== index); return { diff --git a/src/datascience-ui/interactive-common/mainState.ts b/src/datascience-ui/interactive-common/mainState.ts index f4e65a80b883..3c4799405794 100644 --- a/src/datascience-ui/interactive-common/mainState.ts +++ b/src/datascience-ui/interactive-common/mainState.ts @@ -59,7 +59,7 @@ export type IMainState = { editorOptions?: monacoEditor.editor.IEditorOptions; currentExecutionCount: number; debugging: boolean; - dirty?: boolean; + dirty: boolean; selectedCellId?: string; focusedCellId?: string; isAtBottom: boolean; @@ -132,6 +132,7 @@ export function generateTestState(filePath: string = '', editable: boolean = fal size: 14, family: "Consolas, 'Courier New', monospace" }, + dirty: false, codeTheme: 'Foo', settings: defaultSettings, activateCount: 0, diff --git a/src/datascience-ui/interactive-common/redux/postOffice.ts b/src/datascience-ui/interactive-common/redux/postOffice.ts index 1a7891eef9e3..1f146b86d26e 100644 --- a/src/datascience-ui/interactive-common/redux/postOffice.ts +++ b/src/datascience-ui/interactive-common/redux/postOffice.ts @@ -58,11 +58,6 @@ export enum IncomingMessageActions { RESOLVECOMPLETIONITEMREQUEST = 'action.resolve_completion_item_request', CANCELRESOLVECOMPLETIONITEMREQUEST = 'action.cancel_completion_items_request', RESOLVECOMPLETIONITEMRESPONSE = 'action.resolve_completion_item_response', - ADDCELL = 'action.add_cell', - EDITCELL = 'action.edit_cell', - REMOVECELL = 'action.remove_cell', - SWAPCELLS = 'action.swap_cells', - INSERTCELL = 'action.insert_cell', LOADONIGASMASSEMBLYREQUEST = 'action.load_onigasm_assembly_request', LOADONIGASMASSEMBLYRESPONSE = 'action.load_onigasm_assembly_response', LOADTMLANGUAGEREQUEST = 'action.load_tmlanguage_request', @@ -93,7 +88,8 @@ export enum IncomingMessageActions { GETMONACOTHEMEREQUEST = 'action.get_monaco_theme_request', GETMONACOTHEMERESPONSE = 'action.get_monaco_theme_response', UPDATEKERNEL = 'action.update_kernel', - LOCINIT = 'action.loc_init' + LOCINIT = 'action.loc_init', + UPDATEMODEL = 'action.update_model' } export const AllowedMessages = [...Object.values(InteractiveWindowMessages), ...Object.values(CssMessages), ...Object.values(SharedMessages)]; diff --git a/src/datascience-ui/interactive-common/redux/reducers/helpers.ts b/src/datascience-ui/interactive-common/redux/reducers/helpers.ts index 8653be19a67c..c0b8291573b0 100644 --- a/src/datascience-ui/interactive-common/redux/reducers/helpers.ts +++ b/src/datascience-ui/interactive-common/redux/reducers/helpers.ts @@ -44,6 +44,12 @@ export namespace Helpers { return cvm as ICellViewModel; } + // This function is because the unit test typescript compiler can't handle ICell.metadata + // tslint:disable-next-line: no-any + export function asCell(cell: any): ICell { + return cell as ICell; + } + export function updateOrAdd(arg: CommonReducerArg, generateVM: (cell: ICell, mainState: IMainState) => ICellViewModel): IMainState { // First compute new execution count. const newExecutionCount = arg.payload.data.execution_count diff --git a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts index f12fd12b8d18..c5d5a0a38fdd 100644 --- a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts +++ b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts @@ -1,8 +1,10 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. 'use strict'; -import { InteractiveWindowMessages } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; +import { ICellContentChange, InteractiveWindowMessages, NotebookModelChange } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; import { CssMessages } from '../../../../client/datascience/messages'; +import { ICell } from '../../../../client/datascience/types'; +import { concatMultilineStringInput } from '../../../common'; import { extractInputText, IMainState } from '../../mainState'; import { createPostableAction } from '../postOffice'; import { Helpers } from './helpers'; @@ -89,9 +91,88 @@ export namespace Transfer { return arg.prevState; } + function postModelUpdate(arg: CommonReducerArg, update: NotebookModelChange) { + arg.queueAction(createPostableAction(InteractiveWindowMessages.UpdateModel, update)); + } + + export function postModelEdit(arg: CommonReducerArg, changes: ICellContentChange[], cell: ICell, newText: string) { + postModelUpdate(arg, { + source: 'user', + kind: 'edit', + newDirty: true, + oldDirty: arg.prevState.dirty, + changes, + cell, + newText + }); + } + + export function postModelInsert(arg: CommonReducerArg, index: number, cell: ICell, codeCellAboveId?: string, fullText?: string, currentText?: string) { + const trueFullText = fullText === undefined ? concatMultilineStringInput(cell.data.source) : fullText; + const trueCurrentText = currentText === undefined ? trueFullText : currentText; + postModelUpdate(arg, { + source: 'user', + kind: 'insert', + newDirty: true, + oldDirty: arg.prevState.dirty, + index, + cell, + codeCellAboveId, + fullText: trueFullText, + currentText: trueCurrentText + }); + } + + export function postModelRemove(arg: CommonReducerArg, index: number, cell: ICell) { + postModelUpdate(arg, { + source: 'user', + kind: 'remove', + oldDirty: arg.prevState.dirty, + newDirty: true, + cell, + index + }); + } + + export function postModelClearOutputs(arg: CommonReducerArg) { + postModelUpdate(arg, { + source: 'user', + kind: 'clear', + oldDirty: arg.prevState.dirty, + newDirty: true, + // tslint:disable-next-line: no-any + oldCells: arg.prevState.cellVMs.map(c => c.cell as any) as ICell[] + }); + } + + export function postModelRemoveAll(arg: CommonReducerArg, newCellId: string) { + postModelUpdate(arg, { + source: 'user', + kind: 'remove_all', + oldDirty: arg.prevState.dirty, + newDirty: true, + // tslint:disable-next-line: no-any + oldCells: arg.prevState.cellVMs.map(c => c.cell as any) as ICell[], + newCellId + }); + } + + export function postModelSwap(arg: CommonReducerArg, firstCellId: string, secondCellId: string) { + postModelUpdate(arg, { + source: 'user', + kind: 'swap', + oldDirty: arg.prevState.dirty, + newDirty: true, + firstCellId, + secondCellId + }); + } + export function editCell(arg: CommonReducerArg): IMainState { - if (arg.payload.cellId) { - arg.queueAction(createPostableAction(InteractiveWindowMessages.EditCell, { changes: arg.payload.changes, id: arg.payload.cellId })); + const cellVM = arg.prevState.cellVMs.find(c => c.cell.id === arg.payload.cellId); + if (cellVM) { + // Tell the underlying model on the extension side + postModelEdit(arg, arg.payload.changes, cellVM.cell, arg.payload.code); // Update the uncomitted text on the cell view model // We keep this saved here so we don't re-render and we put this code into the input / code data diff --git a/src/datascience-ui/interactive-common/redux/store.ts b/src/datascience-ui/interactive-common/redux/store.ts index d83f01a4d5a0..cc7d2f8f021b 100644 --- a/src/datascience-ui/interactive-common/redux/store.ts +++ b/src/datascience-ui/interactive-common/redux/store.ts @@ -35,6 +35,7 @@ function generateDefaultState(skipDefault: boolean, testMode: boolean, baseTheme currentExecutionCount: 0, debugging: false, knownDark: false, + dirty: false, editCellVM: editable ? undefined : createEditableCellVM(0), isAtBottom: true, font: { diff --git a/src/datascience-ui/native-editor/redux/mapping.ts b/src/datascience-ui/native-editor/redux/mapping.ts index f9b33fde953a..bd97a5e49714 100644 --- a/src/datascience-ui/native-editor/redux/mapping.ts +++ b/src/datascience-ui/native-editor/redux/mapping.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. 'use strict'; -import { ILoadAllCells } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; +import { ILoadAllCells, NotebookModelChange } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; import { IGetCssResponse } from '../../../client/datascience/messages'; import { IGetMonacoThemeResponse } from '../../../client/datascience/monacoMessages'; import { ICell } from '../../../client/datascience/types'; @@ -90,4 +90,5 @@ export class INativeEditorActionMapping { public [IncomingMessageActions.GETMONACOTHEMERESPONSE]: NativeEditorReducerFunc; public [IncomingMessageActions.UPDATEKERNEL]: NativeEditorReducerFunc; public [IncomingMessageActions.LOCINIT]: NativeEditorReducerFunc; + public [IncomingMessageActions.UPDATEMODEL]: NativeEditorReducerFunc; } diff --git a/src/datascience-ui/native-editor/redux/reducers/creation.ts b/src/datascience-ui/native-editor/redux/reducers/creation.ts index fad4c7d3c92f..dfb27ff0dc53 100644 --- a/src/datascience-ui/native-editor/redux/reducers/creation.ts +++ b/src/datascience-ui/native-editor/redux/reducers/creation.ts @@ -3,14 +3,17 @@ 'use strict'; import * as uuid from 'uuid/v4'; -import { ILoadAllCells, InteractiveWindowMessages } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; +import { noop } from '../../../../client/common/utils/misc'; +import { ILoadAllCells, NotebookModelChange } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; import { ICell, IDataScienceExtraSettings } from '../../../../client/datascience/types'; import { createCellVM, createEmptyCell, CursorPos, extractInputText, ICellViewModel, IMainState } from '../../../interactive-common/mainState'; -import { createPostableAction } from '../../../interactive-common/redux/postOffice'; import { Helpers } from '../../../interactive-common/redux/reducers/helpers'; +import { Transfer } from '../../../interactive-common/redux/reducers/transfer'; import { IAddCellAction, ICellAction } from '../../../interactive-common/redux/reducers/types'; import { actionCreators } from '../actions'; import { NativeEditorReducerArg } from '../mapping'; +import { Execution } from './execution'; +import { Movement } from './movement'; export namespace Creation { function prepareCellVM(cell: ICell, hasBeenRun: boolean, settings?: IDataScienceExtraSettings): ICellViewModel { @@ -57,9 +60,7 @@ export namespace Creation { }; // Send a messsage that we inserted a cell - arg.queueAction( - createPostableAction(InteractiveWindowMessages.InsertCell, { cell: newVM.cell, index: position, code: '', codeCellAboveId: findFirstCodeCellAbove(newList, position) }) - ); + Transfer.postModelInsert(arg, position, newVM.cell, findFirstCodeCellAbove(newList, position)); // Queue up an action to set focus to the cell we're inserting setTimeout(() => { @@ -92,9 +93,7 @@ export namespace Creation { }; // Send a messsage that we inserted a cell - arg.queueAction( - createPostableAction(InteractiveWindowMessages.InsertCell, { cell: newVM.cell, index, code: '', codeCellAboveId: findFirstCodeCellAbove(newList, position) }) - ); + Transfer.postModelInsert(arg, -1, newVM.cell, findFirstCodeCellAbove(newList, position)); // Queue up an action to set focus to the cell we're inserting setTimeout(() => { @@ -130,9 +129,6 @@ export namespace Creation { } export function deleteAllCells(arg: NativeEditorReducerArg): IMainState { - // Send messages to other side to indicate the deletes - arg.queueAction(createPostableAction(InteractiveWindowMessages.DeleteAllCells)); - // Just leave one single blank empty cell const newVM: ICellViewModel = { cell: createEmptyCell(arg.payload.newCellId, null), @@ -148,7 +144,7 @@ export namespace Creation { scrollCount: 0 }; - arg.queueAction(createPostableAction(InteractiveWindowMessages.InsertCell, { cell: newVM.cell, code: '', index: 0, codeCellAboveId: undefined })); + Transfer.postModelRemoveAll(arg, newVM.cell.id); return { ...arg.prevState, @@ -178,9 +174,8 @@ export namespace Creation { }; // Send messages to other side to indicate the new add - arg.queueAction(createPostableAction(InteractiveWindowMessages.DeleteCell)); - arg.queueAction(createPostableAction(InteractiveWindowMessages.RemoveCell, { id: arg.payload.cellId })); - arg.queueAction(createPostableAction(InteractiveWindowMessages.InsertCell, { cell: newVM.cell, code: '', index: 0, codeCellAboveId: undefined })); + Transfer.postModelRemove(arg, 0, cells[0].cell); + Transfer.postModelInsert(arg, 0, newVM.cell); return { ...arg.prevState, @@ -191,8 +186,7 @@ export namespace Creation { // Otherwise just a straight delete const index = arg.prevState.cellVMs.findIndex(c => c.cell.id === arg.payload.cellId); if (index >= 0) { - arg.queueAction(createPostableAction(InteractiveWindowMessages.DeleteCell)); - arg.queueAction(createPostableAction(InteractiveWindowMessages.RemoveCell, { id: arg.payload.cellId })); + Transfer.postModelRemove(arg, 0, cells[index].cell); // Recompute select/focus if this item has either let newSelection = arg.prevState.selectedCellId; @@ -241,4 +235,67 @@ export namespace Creation { redoStack: [] }; } + + function handleUndoModel(arg: NativeEditorReducerArg): IMainState { + // Disable the queueAction in the arg so that calling other reducers doesn't cause + // messages to be posted back (as were handling a message from the extension here) + const disabledQueueArg = { ...arg, queueAction: noop }; + switch (arg.payload.kind) { + case 'clear': + return loadAllCells({ ...disabledQueueArg, payload: { cells: arg.payload.oldCells } }); + case 'edit': + return updateCell({ ...disabledQueueArg, payload: arg.payload.cell }); + case 'insert': + return deleteCell({ ...disabledQueueArg, payload: { cellId: arg.payload.cell.id } }); + case 'remove': + const cellBelow = arg.prevState.cellVMs.length > arg.payload.index ? arg.prevState.cellVMs[arg.payload.index].cell : undefined; + return insertAbove({ ...disabledQueueArg, payload: { newCellId: arg.payload.cell.id, cellId: cellBelow ? cellBelow.id : undefined } }); + case 'remove_all': + return loadAllCells({ ...disabledQueueArg, payload: { cells: arg.payload.oldCells } }); + case 'swap': + return Movement.swapCells({ ...disabledQueueArg, payload: { firstCellId: arg.payload.secondCellId, secondCellId: arg.payload.firstCellId } }); + default: + // Modify, file, version can all be ignored. + break; + } + + return arg.prevState; + } + + function handleRedoModel(arg: NativeEditorReducerArg): IMainState { + // Disable the queueAction in the arg so that calling other reducers doesn't cause + // messages to be posted back (as were handling a message from the extension here) + const disabledQueueArg = { ...arg, queueAction: noop }; + switch (arg.payload.kind) { + case 'clear': + return Execution.clearAllOutputs(disabledQueueArg); + case 'edit': + return updateCell({ ...disabledQueueArg, payload: Helpers.asCell({ ...arg.payload.cell, data: { ...arg.payload.cell.data, source: arg.payload.newText } }) }); + case 'insert': + return insertAbove({ ...disabledQueueArg, payload: { newCellId: arg.payload.cell.id, cellId: arg.payload.codeCellAboveId } }); + case 'remove': + return deleteCell({ ...disabledQueueArg, payload: { cellId: arg.payload.cell.id } }); + case 'remove_all': + return deleteAllCells({ ...disabledQueueArg, payload: { newCellId: arg.payload.newCellId } }); + case 'swap': + return Movement.swapCells({ ...disabledQueueArg, payload: { firstCellId: arg.payload.secondCellId, secondCellId: arg.payload.firstCellId } }); + default: + // Modify, file, version can all be ignored. + break; + } + + return arg.prevState; + } + + export function handleUpdate(arg: NativeEditorReducerArg): IMainState { + switch (arg.payload.source) { + case 'undo': + return handleUndoModel(arg); + case 'redo': + return handleRedoModel(arg); + default: + break; + } + return arg.prevState; + } } diff --git a/src/datascience-ui/native-editor/redux/reducers/execution.ts b/src/datascience-ui/native-editor/redux/reducers/execution.ts index ed51ae4fffe9..3aa81151f239 100644 --- a/src/datascience-ui/native-editor/redux/reducers/execution.ts +++ b/src/datascience-ui/native-editor/redux/reducers/execution.ts @@ -11,6 +11,7 @@ import { createCellFrom } from '../../../common/cellFactory'; import { CursorPos, ICellViewModel, IMainState } from '../../../interactive-common/mainState'; import { createPostableAction } from '../../../interactive-common/redux/postOffice'; import { Helpers } from '../../../interactive-common/redux/reducers/helpers'; +import { Transfer } from '../../../interactive-common/redux/reducers/transfer'; import { CommonActionType, ICellAction, IChangeCellTypeAction, ICodeAction, IExecuteAction } from '../../../interactive-common/redux/reducers/types'; import { QueueAnotherFunc } from '../../../react-common/reduxUtils'; import { NativeEditorReducerArg } from '../mapping'; @@ -133,7 +134,7 @@ export namespace Execution { return Helpers.asCellViewModel({ ...cellVM, cell: { ...cellVM.cell, data: { ...cellVM.cell.data, outputs: [], execution_count: null } } }); }); - arg.queueAction(createPostableAction(InteractiveWindowMessages.ClearAllOutputs)); + Transfer.postModelClearOutputs(arg); return { ...arg.prevState, @@ -160,16 +161,9 @@ export namespace Execution { // tslint:disable-next-line: no-any cellVMs[index] = newCell as any; // This is because IMessageCell doesn't fit in here. But message cells can't change type if (newType === 'code') { - arg.queueAction( - createPostableAction(InteractiveWindowMessages.InsertCell, { - cell: cellVMs[index].cell, - index, - code: arg.payload.currentCode, - codeCellAboveId: Helpers.firstCodeCellAbove(arg.prevState, current.cell.id) - }) - ); + Transfer.postModelInsert(arg, index, cellVMs[index].cell, Helpers.firstCodeCellAbove(arg.prevState, current.cell.id)); } else { - arg.queueAction(createPostableAction(InteractiveWindowMessages.RemoveCell, { id: current.cell.id })); + Transfer.postModelRemove(arg, index, current.cell); } // When changing a cell type, also give the cell focus. diff --git a/src/datascience-ui/native-editor/redux/reducers/index.ts b/src/datascience-ui/native-editor/redux/reducers/index.ts index 88daa9375dc0..6fb60b0d5e21 100644 --- a/src/datascience-ui/native-editor/redux/reducers/index.ts +++ b/src/datascience-ui/native-editor/redux/reducers/index.ts @@ -76,5 +76,6 @@ export const reducerMap: INativeEditorActionMapping = { [IncomingMessageActions.MONACOREADY]: CommonEffects.monacoReady, [IncomingMessageActions.GETMONACOTHEMERESPONSE]: CommonEffects.monacoThemeChange, [IncomingMessageActions.UPDATEKERNEL]: Kernel.updateStatus, - [IncomingMessageActions.LOCINIT]: CommonEffects.handleLocInit + [IncomingMessageActions.LOCINIT]: CommonEffects.handleLocInit, + [IncomingMessageActions.UPDATEMODEL]: Creation.handleUpdate }; diff --git a/src/datascience-ui/native-editor/redux/reducers/movement.ts b/src/datascience-ui/native-editor/redux/reducers/movement.ts index ca8d51aa6120..7d8ac97ce774 100644 --- a/src/datascience-ui/native-editor/redux/reducers/movement.ts +++ b/src/datascience-ui/native-editor/redux/reducers/movement.ts @@ -1,21 +1,23 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. 'use strict'; -import { InteractiveWindowMessages } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; import { CursorPos, IMainState } from '../../../interactive-common/mainState'; -import { createPostableAction } from '../../../interactive-common/redux/postOffice'; import { Helpers } from '../../../interactive-common/redux/reducers/helpers'; +import { Transfer } from '../../../interactive-common/redux/reducers/transfer'; import { ICellAction, ICodeAction } from '../../../interactive-common/redux/reducers/types'; import { NativeEditorReducerArg } from '../mapping'; import { Effects } from './effects'; export namespace Movement { - export function moveCellUp(arg: NativeEditorReducerArg): IMainState { + export function swapCells(arg: NativeEditorReducerArg<{ firstCellId: string; secondCellId: string }>) { const newVMs = [...arg.prevState.cellVMs]; - const index = newVMs.findIndex(cvm => cvm.cell.id === arg.payload.cellId); - if (index > 0) { - [newVMs[index - 1], newVMs[index]] = [newVMs[index], newVMs[index - 1]]; - arg.queueAction(createPostableAction(InteractiveWindowMessages.SwapCells, { firstCellId: arg.payload.cellId!, secondCellId: newVMs[index].cell.id })); + const first = newVMs.findIndex(cvm => cvm.cell.id === arg.payload.firstCellId); + const second = newVMs.findIndex(cvm => cvm.cell.id === arg.payload.secondCellId); + if (first >= 0 && second >= 0) { + const temp = newVMs[first]; + newVMs[first] = newVMs[second]; + newVMs[second] = temp; + Transfer.postModelSwap(arg, arg.payload.firstCellId, arg.payload.secondCellId); return { ...arg.prevState, cellVMs: newVMs, @@ -26,17 +28,20 @@ export namespace Movement { return arg.prevState; } + export function moveCellUp(arg: NativeEditorReducerArg): IMainState { + const index = arg.prevState.cellVMs.findIndex(cvm => cvm.cell.id === arg.payload.cellId); + if (index > 0 && arg.payload.cellId) { + return swapCells({ ...arg, payload: { firstCellId: arg.prevState.cellVMs[index - 1].cell.id, secondCellId: arg.payload.cellId } }); + } + + return arg.prevState; + } + export function moveCellDown(arg: NativeEditorReducerArg): IMainState { const newVMs = [...arg.prevState.cellVMs]; const index = newVMs.findIndex(cvm => cvm.cell.id === arg.payload.cellId); - if (index < newVMs.length - 1) { - [newVMs[index + 1], newVMs[index]] = [newVMs[index], newVMs[index + 1]]; - arg.queueAction(createPostableAction(InteractiveWindowMessages.SwapCells, { firstCellId: arg.payload.cellId!, secondCellId: newVMs[index].cell.id })); - return { - ...arg.prevState, - cellVMs: newVMs, - undoStack: Helpers.pushStack(arg.prevState.undoStack, arg.prevState.cellVMs) - }; + if (index < newVMs.length - 1 && arg.payload.cellId) { + return swapCells({ ...arg, payload: { firstCellId: arg.payload.cellId, secondCellId: arg.prevState.cellVMs[index + 1].cell.id } }); } return arg.prevState; diff --git a/src/test/datascience/intellisense.unit.test.ts b/src/test/datascience/intellisense.unit.test.ts index 0125aa06ee15..a7bf49ed3bda 100644 --- a/src/test/datascience/intellisense.unit.test.ts +++ b/src/test/datascience/intellisense.unit.test.ts @@ -4,6 +4,7 @@ import { expect } from 'chai'; import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api'; import * as TypeMoq from 'typemoq'; +import * as uuid from 'uuid/v4'; import { IWorkspaceService } from '../../client/common/application/types'; import { PythonSettings } from '../../client/common/configSettings'; @@ -43,6 +44,7 @@ suite('DataScience Intellisense Unit Tests', () => { let fileSystem: TypeMoq.IMock; let jupyterExecution: TypeMoq.IMock; let interactiveWindowProvider: TypeMoq.IMock; + let cells: ICell[] = [createEmptyCell(Identifiers.EditCellId, null)]; const pythonSettings = new (class extends PythonSettings { public fireChangeEvent() { this.changed.fire(); @@ -79,7 +81,20 @@ suite('DataScience Intellisense Unit Tests', () => { } function addCell(code: string, id: string): Promise { - return sendMessage(InteractiveWindowMessages.AddCell, { fullText: code, currentText: code, cell: createEmptyCell(id, null) }); + const cell = createEmptyCell(id, null); + cell.data.source = code; + const result = sendMessage(InteractiveWindowMessages.UpdateModel, { + source: 'user', + kind: 'insert', + oldDirty: false, + newDirty: true, + fullText: code, + currentText: code, + index: cells.length - 1, + cell + }); + cells.splice(cells.length - 1, 0, cell); + return result; } function updateCell(newCode: string, oldCode: string, id: string): Promise { @@ -95,7 +110,15 @@ suite('DataScience Intellisense Unit Tests', () => { rangeLength: oldCode.length, text: newCode }; - return sendMessage(InteractiveWindowMessages.EditCell, { changes: [change], id }); + return sendMessage(InteractiveWindowMessages.UpdateModel, { + source: 'user', + kind: 'edit', + oldDirty: false, + newDirty: true, + changes: [change], + cell: cells.find(c => c.id === id)!, + newText: newCode + }); } function addCode(code: string, line: number, pos: number, offset: number): Promise { @@ -113,7 +136,15 @@ suite('DataScience Intellisense Unit Tests', () => { rangeLength: 0, text: code }; - return sendMessage(InteractiveWindowMessages.EditCell, { changes: [change], id: Identifiers.EditCellId }); + return sendMessage(InteractiveWindowMessages.UpdateModel, { + source: 'user', + kind: 'edit', + oldDirty: false, + newDirty: true, + changes: [change], + cell: cells[cells.length - 1], + newText: code + }); } function removeCode(line: number, startPos: number, endPos: number, length: number): Promise { @@ -131,26 +162,69 @@ suite('DataScience Intellisense Unit Tests', () => { rangeLength: length, text: '' }; - return sendMessage(InteractiveWindowMessages.EditCell, { changes: [change], id: Identifiers.EditCellId }); + return sendMessage(InteractiveWindowMessages.UpdateModel, { + source: 'user', + kind: 'edit', + oldDirty: false, + newDirty: true, + changes: [change], + cell: cells[cells.length - 1], + newText: '' + }); } function removeCell(id: string): Promise { - return sendMessage(InteractiveWindowMessages.RemoveCell, { id }); + return sendMessage(InteractiveWindowMessages.UpdateModel, { + source: 'user', + kind: 'remove', + oldDirty: false, + newDirty: true, + cell: cells.find(c => c.id === id)!, + index: cells.findIndex(c => c.id === id) + }); } function removeAllCells(): Promise { - return sendMessage(InteractiveWindowMessages.DeleteAllCells); + return sendMessage(InteractiveWindowMessages.UpdateModel, { + source: 'user', + kind: 'remove_all', + oldDirty: false, + newDirty: true, + oldCells: cells, + newCellId: uuid() + }); } function swapCells(id1: string, id2: string): Promise { - return sendMessage(InteractiveWindowMessages.SwapCells, { firstCellId: id1, secondCellId: id2 }); + return sendMessage(InteractiveWindowMessages.UpdateModel, { + source: 'user', + kind: 'swap', + oldDirty: false, + newDirty: true, + firstCellId: id1, + secondCellId: id2 + }); } function insertCell(id: string, code: string, codeCellAbove?: string): Promise { - return sendMessage(InteractiveWindowMessages.InsertCell, { cell: createEmptyCell(id, null), index: 0, code, codeCellAboveId: codeCellAbove }); + const cell = createEmptyCell(id, null); + cell.data.source = code; + const index = codeCellAbove ? cells.findIndex(c => c.id === codeCellAbove) : 0; + return sendMessage(InteractiveWindowMessages.UpdateModel, { + source: 'user', + kind: 'insert', + oldDirty: false, + newDirty: true, + codeCellAboveId: codeCellAbove, + cell, + index, + fullText: code, + currentText: code + }); } - function loadAllCells(cells: ICell[]): Promise { + function loadAllCells(allCells: ICell[]): Promise { + cells = allCells; return sendMessage(InteractiveWindowMessages.LoadAllCellsComplete, { cells }); } @@ -306,8 +380,8 @@ suite('DataScience Intellisense Unit Tests', () => { }); test('Load remove and insert', async () => { - const cells = generateTestCells('foo.py', 1); - await loadAllCells(cells); + const test = generateTestCells('foo.py', 1); + await loadAllCells(test); expect(getDocumentContents()).to.be.eq(TestCellContents, 'Load all cells is failing'); await removeAllCells(); expect(getDocumentContents()).to.be.eq('', 'Remove all cells is failing'); @@ -318,8 +392,8 @@ suite('DataScience Intellisense Unit Tests', () => { }); test('Swap cells around', async () => { - const cells = generateTestCells('foo.py', 1); - await loadAllCells(cells); + const test = generateTestCells('foo.py', 1); + await loadAllCells(test); await swapCells('0', '1'); // 2nd cell is markdown expect(getDocumentContents()).to.be.eq(TestCellContents, 'Swap cells should skip swapping on markdown'); await swapCells('0', '2'); @@ -342,8 +416,8 @@ df }); test('Insert and swap', async () => { - const cells = generateTestCells('foo.py', 1); - await loadAllCells(cells); + const test = generateTestCells('foo.py', 1); + await loadAllCells(test); expect(getDocumentContents()).to.be.eq(TestCellContents, 'Load all cells is failing'); await insertCell('6', 'foo'); const afterInsert = `foo diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index 8fa146938ff2..61aef8c2608e 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -22,10 +22,10 @@ import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService, ICryptoUtils, IDisposable, IExtensionContext } from '../../../client/common/types'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { Commands } from '../../../client/datascience/constants'; -import { InteractiveWindowMessages } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; +import { ICellContentChange, InteractiveWindowMessages } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; import { NativeEditorStorage } from '../../../client/datascience/interactive-ipynb/nativeEditorStorage'; import { JupyterExecutionFactory } from '../../../client/datascience/jupyter/jupyterExecutionFactory'; -import { IJupyterExecution, INotebookServerOptions } from '../../../client/datascience/types'; +import { ICell, IJupyterExecution, INotebookServerOptions } from '../../../client/datascience/types'; import { IInterpreterService } from '../../../client/interpreter/contracts'; import { InterpreterService } from '../../../client/interpreter/interpreterService'; import { createEmptyCell } from '../../../datascience-ui/interactive-common/mainState'; @@ -345,13 +345,71 @@ suite('Data Science - Native Editor Storage', () => { disposables.forEach(d => d.dispose()); }); + function insertCell(index: number, code: string) { + return executeCommand(Commands.NotebookModel_Update, baseUri, { + source: 'user', + kind: 'insert', + oldDirty: storage.isDirty, + newDirty: true, + cell: createEmptyCell(code, 1), + index, + fullText: code, + currentText: code + }); + } + + function swapCells(first: string, second: string) { + return executeCommand(Commands.NotebookModel_Update, baseUri, { + source: 'user', + kind: 'swap', + oldDirty: storage.isDirty, + newDirty: true, + firstCellId: first, + secondCellId: second + }); + } + + function editCell(changes: ICellContentChange[], cell: ICell, newCode: string) { + return executeCommand(Commands.NotebookModel_Update, baseUri, { + source: 'user', + kind: 'edit', + oldDirty: storage.isDirty, + newDirty: true, + changes, + cell, + newText: newCode + }); + } + + function removeCell(index: number, cell: ICell) { + return executeCommand(Commands.NotebookModel_Update, baseUri, { + source: 'user', + kind: 'remove', + oldDirty: storage.isDirty, + newDirty: true, + index, + cell + }); + } + + function deleteAllCells() { + return executeCommand(Commands.NotebookModel_Update, baseUri, { + source: 'user', + kind: 'remove_all', + oldDirty: storage.isDirty, + newDirty: true, + oldCells: storage.cells, + newCellId: '1' + }); + } + function executeCommand(command: E, ...rest: U) { return cmdManager.executeCommand(command, ...rest); } test('Create new editor and add some cells', async () => { await storage.load(baseUri); - await executeCommand(Commands.NotebookStorage_InsertCell, baseUri, { index: 0, cell: createEmptyCell('1', 1), code: '1', codeCellAboveId: undefined }); + await insertCell(0, '1'); const cells = storage.cells; expect(cells).to.be.lengthOf(4); expect(storage.isDirty).to.be.equal(true, 'Editor should be dirty'); @@ -360,7 +418,7 @@ suite('Data Science - Native Editor Storage', () => { test('Move cells around', async () => { await storage.load(baseUri); - await executeCommand(Commands.NotebookStorage_SwapCells, baseUri, { firstCellId: 'NotebookImport#0', secondCellId: 'NotebookImport#1' }); + await swapCells('NotebookImport#0', 'NotebookImport#1'); const cells = storage.cells; expect(cells).to.be.lengthOf(3); expect(storage.isDirty).to.be.equal(true, 'Editor should be dirty'); @@ -370,8 +428,8 @@ suite('Data Science - Native Editor Storage', () => { test('Edit/delete cells', async () => { await storage.load(baseUri); expect(storage.isDirty).to.be.equal(false, 'Editor should not be dirty'); - await executeCommand(Commands.NotebookStorage_EditCell, baseUri, { - changes: [ + await editCell( + [ { range: { startLineNumber: 2, @@ -384,18 +442,19 @@ suite('Data Science - Native Editor Storage', () => { text: 'a' } ], - id: 'NotebookImport#1' - }); + storage.cells[1], + 'a' + ); let cells = storage.cells; expect(cells).to.be.lengthOf(3); expect(cells[1].id).to.be.match(/NotebookImport#1/); expect(cells[1].data.source).to.be.equals('b=2\nab'); expect(storage.isDirty).to.be.equal(true, 'Editor should be dirty'); - await executeCommand(Commands.NotebookStorage_RemoveCell, baseUri, 'NotebookImport#0'); + await removeCell(0, cells[0]); cells = storage.cells; expect(cells).to.be.lengthOf(2); expect(cells[0].id).to.be.match(/NotebookImport#1/); - await executeCommand(Commands.NotebookStorage_DeleteAllCells, baseUri); + await deleteAllCells(); cells = storage.cells; expect(cells).to.be.lengthOf(0); }); diff --git a/src/test/datascience/mockCustomEditorService.ts b/src/test/datascience/mockCustomEditorService.ts index bfd4613b8ec7..c6b1897d4d93 100644 --- a/src/test/datascience/mockCustomEditorService.ts +++ b/src/test/datascience/mockCustomEditorService.ts @@ -4,7 +4,8 @@ import { Disposable, Uri, WebviewPanel, WebviewPanelOptions } from 'vscode'; import { ICommandManager, ICustomEditorService, WebviewCustomEditorEditingDelegate, WebviewCustomEditorProvider } from '../../client/common/application/types'; import { IDisposableRegistry } from '../../client/common/types'; import { noop } from '../../client/common/utils/misc'; -import { INotebookEdit, INotebookEditor, INotebookEditorProvider } from '../../client/datascience/types'; +import { NotebookModelChange } from '../../client/datascience/interactive-common/interactiveWindowTypes'; +import { INotebookEditor, INotebookEditorProvider } from '../../client/datascience/types'; export class MockCustomEditorService implements ICustomEditorService { private provider: WebviewCustomEditorProvider | undefined; @@ -43,14 +44,14 @@ export class MockCustomEditorService implements ICustomEditorService { } private onFileSave(file: Uri) { - const nativeProvider = (this.provider as unknown) as WebviewCustomEditorEditingDelegate; + const nativeProvider = (this.provider as unknown) as WebviewCustomEditorEditingDelegate; if (nativeProvider) { nativeProvider.save(file); } } private onFileSaveAs(file: Uri) { - const nativeProvider = (this.provider as unknown) as WebviewCustomEditorEditingDelegate; + const nativeProvider = (this.provider as unknown) as WebviewCustomEditorEditingDelegate; if (nativeProvider) { // Just make up a new URI nativeProvider.saveAs(file, Uri.file('bar.ipynb')); From 1d57e739122615dff047f3474d36b883aec6bf33 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 31 Jan 2020 17:07:32 -0800 Subject: [PATCH 03/23] Put the storage object in the global map --- .../datascience/interactive-ipynb/nativeEditorStorage.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 8b4c9b40c8fb..d4e48e33bbce 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -164,6 +164,11 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID case 'version': this.updateVersionInfo(change.interpreter, change.kernelSpec); break; + case 'file': + this._state.file = change.newFile; + NativeEditorStorage.storageMap.delete(change.oldFile.toString()); + NativeEditorStorage.storageMap.set(change.newFile.toString(), this); + break; default: break; } @@ -316,6 +321,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID private async loadFromFile(file: Uri, possibleContents?: string): Promise { // Save file this._state.file = file; + NativeEditorStorage.storageMap.set(file.toString(), this); try { // Attempt to read the contents if a viable file From 4658b15dc0c65b0494c6d25d1b7f4bf4e1516be5 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 3 Feb 2020 15:08:10 -0800 Subject: [PATCH 04/23] Look for more data during undo/redo --- .../interactiveWindowTypes.ts | 2 + .../interactive-ipynb/nativeEditorStorage.ts | 88 ++++++++++++------- .../history-react/interactiveCell.tsx | 6 +- .../history-react/redux/actions.ts | 11 ++- .../interactive-common/cellInput.tsx | 6 +- .../interactive-common/code.tsx | 6 +- .../interactive-common/editor.tsx | 4 +- .../interactive-common/markdown.tsx | 6 +- .../redux/reducers/transfer.ts | 8 +- .../redux/reducers/types.ts | 2 + .../native-editor/nativeCell.tsx | 15 +++- .../native-editor/redux/actions.ts | 11 ++- .../native-editor/redux/reducers/creation.ts | 3 - .../datascience/intellisense.unit.test.ts | 12 ++- .../nativeEditorStorage.unit.test.ts | 4 +- 15 files changed, 123 insertions(+), 61 deletions(-) diff --git a/src/client/datascience/interactive-common/interactiveWindowTypes.ts b/src/client/datascience/interactive-common/interactiveWindowTypes.ts index fb6fb012d6d4..903de03200d5 100644 --- a/src/client/datascience/interactive-common/interactiveWindowTypes.ts +++ b/src/client/datascience/interactive-common/interactiveWindowTypes.ts @@ -377,6 +377,8 @@ export interface INotebookModelEditChange extends INotebookModelChange { changes: ICellContentChange[]; newText: string; cell: ICell; + isUndo: boolean; + isRedo: boolean; } export interface INotebookModelVersionChange extends INotebookModelChange { diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index d4e48e33bbce..e4822f92bcf3 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -12,7 +12,7 @@ import { GLOBAL_MEMENTO, ICryptoUtils, IDisposable, IDisposableRegistry, IExtens import { noop } from '../../common/utils/misc'; import { PythonInterpreter } from '../../interpreter/contracts'; import { Commands, Identifiers } from '../constants'; -import { ICellContentChange, NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; +import { INotebookModelEditChange, NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; import { InvalidNotebookFileError } from '../jupyter/invalidNotebookFileError'; import { LiveKernelModel } from '../jupyter/kernels/types'; import { CellState, ICell, IJupyterExecution, IJupyterKernelSpec, INotebookModel, INotebookStorage } from '../types'; @@ -121,45 +121,49 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID private handleModelChange(change: NotebookModelChange) { const oldDirty = this.isDirty; + let changed = false; switch (change.source) { case 'redo': case 'user': - this.handleRedo(change); + changed = this.handleRedo(change); break; case 'undo': - this.handleUndo(change); + changed = this.handleUndo(change); break; default: break; } - // Forward onto our listeners (modifying dirty) - this._changedEmitter.fire({ ...change, newDirty: this.isDirty, oldDirty }); + // Forward onto our listeners if necessary + if (changed) { + this._changedEmitter.fire({ ...change, newDirty: this.isDirty, oldDirty }); + } } - private handleRedo(change: NotebookModelChange) { + private handleRedo(change: NotebookModelChange): boolean { + let changed = false; switch (change.kind) { case 'clear': - this.clearOutputs(); + changed = this.clearOutputs(); break; case 'edit': - this.editCell(change.changes, change.cell.id); + changed = this.editCell(change); break; case 'insert': - this.insertCell(change.cell, change.index); + changed = this.insertCell(change.cell, change.index); break; case 'modify': - this.updateCellState(change.newCells); + changed = this.modifyCells(change.newCells); break; case 'remove': - this.removeCell(change.cell); + changed = this.removeCell(change.cell); break; case 'remove_all': - this.removeAllCells(change.newCellId); + changed = this.removeAllCells(change.newCellId); break; case 'swap': - this.swapCells(change.firstCellId, change.secondCellId); + changed = this.swapCells(change.firstCellId, change.secondCellId); break; case 'version': this.updateVersionInfo(change.interpreter, change.kernelSpec); @@ -168,35 +172,42 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID this._state.file = change.newFile; NativeEditorStorage.storageMap.delete(change.oldFile.toString()); NativeEditorStorage.storageMap.set(change.newFile.toString(), this); + changed = change.oldFile.toString() !== change.newFile.toString(); break; default: break; } - this._state.isDirty = change.kind !== 'version'; + if (changed) { + this._state.isDirty = true; + } + return changed; } - private handleUndo(change: NotebookModelChange) { + private handleUndo(change: NotebookModelChange): boolean { + let changed = false; switch (change.kind) { case 'clear': this._state.cells = change.oldCells; + changed = true; break; case 'edit': - this.replaceCell(change.cell); + changed = this.replaceCell(change.cell); break; case 'insert': - this.removeCell(change.cell); + changed = this.removeCell(change.cell); break; case 'modify': - this.updateCellState(change.oldCells); + changed = this.modifyCells(change.oldCells); break; case 'remove': - this.insertCell(change.cell, change.index); + changed = this.insertCell(change.cell, change.index); break; case 'remove_all': this._state.cells = change.oldCells; + changed = true; break; case 'swap': - this.swapCells(change.firstCellId, change.secondCellId); + changed = this.swapCells(change.firstCellId, change.secondCellId); break; case 'version': // Not supporting undo @@ -207,34 +218,39 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID // Dirty state comes from undo this._state.isDirty = change.oldDirty; + return changed; } private removeAllCells(newCellId: string) { this._state.cells = []; this._state.cells.push(this.createEmptyCell(newCellId)); + return true; } - private editCell(changes: ICellContentChange[], id: string) { + private editCell(change: INotebookModelEditChange): boolean { // Apply the changes to the visible cell list. We won't get an update until // submission otherwise - if (changes && changes.length) { - const change = changes[0]; - const normalized = change.text.replace(/\r/g, ''); + if (change.changes && change.changes.length) { + const first = change.changes[0]; + const normalized = first.text.replace(/\r/g, ''); // Figure out which cell we're editing. - const index = this.cells.findIndex(c => c.id === id); + const index = this.cells.findIndex(c => c.id === change.cell.id); if (index >= 0) { // This is an actual edit. const contents = concatMultilineStringInput(this.cells[index].data.source); - const before = contents.substr(0, change.rangeOffset); - const after = contents.substr(change.rangeOffset + change.rangeLength); + const before = contents.substr(0, first.rangeOffset); + const after = contents.substr(first.rangeOffset + first.rangeLength); const newContents = `${before}${normalized}${after}`; if (contents !== newContents) { const newCell = { ...this.cells[index], data: { ...this.cells[index].data, source: newContents } }; this._state.cells[index] = this.asCell(newCell); + return true; } } } + + return false; } private swapCells(firstCellId: string, secondCellId: string) { @@ -244,35 +260,43 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID const temp = { ...this.cells[first] }; this._state.cells[first] = this.asCell(this.cells[second]); this._state.cells[second] = this.asCell(temp); + return true; } + return false; } - private updateCellState(cells: ICell[]) { + private modifyCells(cells: ICell[]): boolean { // Update these cells in our list cells.forEach(c => { const index = this.cells.findIndex(v => v.id === c.id); this._state.cells[index] = this.asCell(c); }); + return true; } - private removeCell(cell: ICell) { + private removeCell(cell: ICell): boolean { this._state.cells = this.cells.filter(c => c.id !== cell.id); + return true; } - private replaceCell(cell: ICell) { + private replaceCell(cell: ICell): boolean { const index = this.cells.findIndex(c => c.id === cell.id); if (index >= 0) { this._state.cells[index] = cell; + return true; } + return false; } - private clearOutputs() { + private clearOutputs(): boolean { this._state.cells = this.cells.map(c => this.asCell({ ...c, data: { ...c.data, execution_count: null, outputs: [] } })); + return true; } - private insertCell(cell: ICell, index: number) { + private insertCell(cell: ICell, index: number): boolean { // Insert a cell into our visible list based on the index. They should be in sync this._state.cells.splice(index, 0, cell); + return true; } private updateVersionInfo(interpreter: PythonInterpreter | undefined, kernelSpec: IJupyterKernelSpec | LiveKernelModel | undefined) { diff --git a/src/datascience-ui/history-react/interactiveCell.tsx b/src/datascience-ui/history-react/interactiveCell.tsx index 8329fcf7867a..847a54982369 100644 --- a/src/datascience-ui/history-react/interactiveCell.tsx +++ b/src/datascience-ui/history-react/interactiveCell.tsx @@ -286,8 +286,8 @@ export class InteractiveCell extends React.Component { return contents || concatMultilineStringInput(this.props.cellVM.cell.data.source); } - private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string) => { - this.props.editCell(cellId, changes, modelId, this.getCurrentCode()); + private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string, isUndo: boolean, isRedo: boolean) => { + this.props.editCell(cellId, changes, modelId, this.getCurrentCode(), isUndo, isRedo); }; private onCodeCreated = (_code: string, _file: string, cellId: string, modelId: string) => { @@ -328,6 +328,8 @@ export class InteractiveCell extends React.Component { this.editCellEscape(e); } else if (e.code === 'Enter' && e.shiftKey) { this.editCellSubmit(e); + } else if (e.code === 'NumpadEnter' && e.shiftKey) { + this.editCellSubmit(e); } }; diff --git a/src/datascience-ui/history-react/redux/actions.ts b/src/datascience-ui/history-react/redux/actions.ts index 94ccc0aa57f9..4c49b0b54e92 100644 --- a/src/datascience-ui/history-react/redux/actions.ts +++ b/src/datascience-ui/history-react/redux/actions.ts @@ -35,9 +35,16 @@ export const actionCreators = { gatherCell: (cellId: string): CommonAction => ({ type: CommonActionType.GATHER_CELL, payload: { cellId } }), clickCell: (cellId: string): CommonAction => ({ type: CommonActionType.CLICK_CELL, payload: { cellId } }), doubleClickCell: (cellId: string): CommonAction => ({ type: CommonActionType.DOUBLE_CLICK_CELL, payload: { cellId } }), - editCell: (cellId: string, changes: monacoEditor.editor.IModelContentChange[], modelId: string, code: string): CommonAction => ({ + editCell: ( + cellId: string, + changes: monacoEditor.editor.IModelContentChange[], + modelId: string, + code: string, + isUndo: boolean, + isRedo: boolean + ): CommonAction => ({ type: CommonActionType.EDIT_CELL, - payload: { cellId, changes, modelId, code } + payload: { cellId, changes, modelId, code, isUndo, isRedo } }), submitInput: (code: string, cellId: string): CommonAction => ({ type: CommonActionType.SUBMIT_INPUT, payload: { code, cellId } }), toggleVariableExplorer: (): CommonAction => ({ type: CommonActionType.TOGGLE_VARIABLE_EXPLORER }), diff --git a/src/datascience-ui/interactive-common/cellInput.tsx b/src/datascience-ui/interactive-common/cellInput.tsx index 5ff000057b6f..5d05e79d7f31 100644 --- a/src/datascience-ui/interactive-common/cellInput.tsx +++ b/src/datascience-ui/interactive-common/cellInput.tsx @@ -26,7 +26,7 @@ interface ICellInputProps { editorMeasureClassName?: string; showLineNumbers?: boolean; font: IFont; - onCodeChange(changes: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string): void; + onCodeChange(changes: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string, isUndo: boolean, isRedo: boolean): void; onCodeCreated(code: string, file: string, cellId: string, modelId: string): void; openLink(uri: monacoEditor.Uri): void; keyDown?(cellId: string, e: IKeyboardEvent): void; @@ -180,8 +180,8 @@ export class CellInput extends React.Component { } }; - private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], modelId: string) => { - this.props.onCodeChange(changes, this.props.cellVM.cell.id, modelId); + private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], modelId: string, isUndo: boolean, isRedo: boolean) => { + this.props.onCodeChange(changes, this.props.cellVM.cell.id, modelId, isUndo, isRedo); }; private onCodeCreated = (code: string, modelId: string) => { diff --git a/src/datascience-ui/interactive-common/code.tsx b/src/datascience-ui/interactive-common/code.tsx index 2159047cb11a..ff6ece96895e 100644 --- a/src/datascience-ui/interactive-common/code.tsx +++ b/src/datascience-ui/interactive-common/code.tsx @@ -27,7 +27,7 @@ export interface ICodeProps { hasFocus: boolean; cursorPos: CursorPos; onCreated(code: string, modelId: string): void; - onChange(changes: monacoEditor.editor.IModelContentChange[], modelId: string): void; + onChange(changes: monacoEditor.editor.IModelContentChange[], modelId: string, isUndo: boolean, isRedo: boolean): void; openLink(uri: monacoEditor.Uri): void; keyDown?(e: IKeyboardEvent): void; focused?(): void; @@ -106,10 +106,10 @@ export class Code extends React.Component { return getLocString('DataScience.inputWatermark', 'Type code here and press shift-enter to run'); }; - private onModelChanged = (changes: monacoEditor.editor.IModelContentChange[], model: monacoEditor.editor.ITextModel) => { + private onModelChanged = (changes: monacoEditor.editor.IModelContentChange[], isUndo: boolean, isRedo: boolean, model: monacoEditor.editor.ITextModel) => { if (!this.props.readOnly && model) { this.setState({ allowWatermark: model.getValueLength() === 0 }); } - this.props.onChange(changes, model.id); + this.props.onChange(changes, model.id, isUndo, isRedo); }; } diff --git a/src/datascience-ui/interactive-common/editor.tsx b/src/datascience-ui/interactive-common/editor.tsx index 1b9b961fe8a2..afc44f0e9d01 100644 --- a/src/datascience-ui/interactive-common/editor.tsx +++ b/src/datascience-ui/interactive-common/editor.tsx @@ -28,7 +28,7 @@ export interface IEditorProps { hasFocus: boolean; cursorPos: CursorPos; onCreated(code: string, modelId: string): void; - onChange(changes: monacoEditor.editor.IModelContentChange[], model: monacoEditor.editor.ITextModel): void; + onChange(changes: monacoEditor.editor.IModelContentChange[], isUndo: boolean, isRedo: boolean, model: monacoEditor.editor.ITextModel): void; openLink(uri: monacoEditor.Uri): void; keyDown?(e: IKeyboardEvent): void; focused?(): void; @@ -184,7 +184,7 @@ export class Editor extends React.Component { private modelChanged = (e: monacoEditor.editor.IModelContentChangedEvent) => { if (this.state.model) { - this.props.onChange(e.changes, this.state.model); + this.props.onChange(e.changes, e.isUndoing, e.isRedoing, this.state.model); } }; diff --git a/src/datascience-ui/interactive-common/markdown.tsx b/src/datascience-ui/interactive-common/markdown.tsx index 1076ae8fa307..42ae8f463a22 100644 --- a/src/datascience-ui/interactive-common/markdown.tsx +++ b/src/datascience-ui/interactive-common/markdown.tsx @@ -22,7 +22,7 @@ export interface IMarkdownProps { hasFocus: boolean; cursorPos: CursorPos; onCreated(code: string, modelId: string): void; - onChange(changes: monacoEditor.editor.IModelContentChange[], modelId: string): void; + onChange(changes: monacoEditor.editor.IModelContentChange[], modelId: string, isUndo: boolean, isRedo: boolean): void; focused?(): void; unfocused?(): void; openLink(uri: monacoEditor.Uri): void; @@ -75,7 +75,7 @@ export class Markdown extends React.Component { } } - private onModelChanged = (changes: monacoEditor.editor.IModelContentChange[], model: monacoEditor.editor.ITextModel) => { - this.props.onChange(changes, model.id); + private onModelChanged = (changes: monacoEditor.editor.IModelContentChange[], isUndo: boolean, isRedo: boolean, model: monacoEditor.editor.ITextModel) => { + this.props.onChange(changes, model.id, isUndo, isRedo); }; } diff --git a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts index c5d5a0a38fdd..b904b50a36dd 100644 --- a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts +++ b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts @@ -95,7 +95,7 @@ export namespace Transfer { arg.queueAction(createPostableAction(InteractiveWindowMessages.UpdateModel, update)); } - export function postModelEdit(arg: CommonReducerArg, changes: ICellContentChange[], cell: ICell, newText: string) { + export function postModelEdit(arg: CommonReducerArg, changes: ICellContentChange[], cell: ICell, newText: string, isUndo: boolean, isRedo: boolean) { postModelUpdate(arg, { source: 'user', kind: 'edit', @@ -103,7 +103,9 @@ export namespace Transfer { oldDirty: arg.prevState.dirty, changes, cell, - newText + newText, + isUndo, + isRedo }); } @@ -172,7 +174,7 @@ export namespace Transfer { const cellVM = arg.prevState.cellVMs.find(c => c.cell.id === arg.payload.cellId); if (cellVM) { // Tell the underlying model on the extension side - postModelEdit(arg, arg.payload.changes, cellVM.cell, arg.payload.code); + postModelEdit(arg, arg.payload.changes, cellVM.cell, arg.payload.code, arg.payload.isUndo, arg.payload.isRedo); // Update the uncomitted text on the cell view model // We keep this saved here so we don't re-render and we put this code into the input / code data diff --git a/src/datascience-ui/interactive-common/redux/reducers/types.ts b/src/datascience-ui/interactive-common/redux/reducers/types.ts index 9a3f8f893190..75a803ff37bc 100644 --- a/src/datascience-ui/interactive-common/redux/reducers/types.ts +++ b/src/datascience-ui/interactive-common/redux/reducers/types.ts @@ -109,6 +109,8 @@ export interface ICodeAction extends ICellAction { export interface IEditCellAction extends ICodeAction { changes: monacoEditor.editor.IModelContentChange[]; modelId: string; + isUndo: boolean; + isRedo: boolean; } // I.e. when using the operation `add`, we need the corresponding `IAddCellAction`. diff --git a/src/datascience-ui/native-editor/nativeCell.tsx b/src/datascience-ui/native-editor/nativeCell.tsx index c3158de886b9..974091434340 100644 --- a/src/datascience-ui/native-editor/nativeCell.tsx +++ b/src/datascience-ui/native-editor/nativeCell.tsx @@ -280,6 +280,7 @@ export class NativeCell extends React.Component { this.props.sendCommand(NativeCommandType.ToggleOutput, 'keyboard'); } break; + case 'NumpadEnter': case 'Enter': if (e.shiftKey) { this.shiftEnterCell(e); @@ -327,6 +328,16 @@ export class NativeCell extends React.Component { } } break; + case 'KeyZ': + if (e.ctrlKey) { + window.console.log('Undoing'); + } + break; + case 'KeyY': + if (e.ctrlKey) { + window.console.log('Redoing'); + } + break; default: break; @@ -613,8 +624,8 @@ export class NativeCell extends React.Component { this.props.unfocusCell(this.cellId, this.getCurrentCode()); }; - private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string) => { - this.props.editCell(cellId, changes, modelId, this.getCurrentCode()); + private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string, isUndo: boolean, isRedo: boolean) => { + this.props.editCell(cellId, changes, modelId, this.getCurrentCode(), isUndo, isRedo); }; private onCodeCreated = (_code: string, _file: string, cellId: string, modelId: string) => { diff --git a/src/datascience-ui/native-editor/redux/actions.ts b/src/datascience-ui/native-editor/redux/actions.ts index ade4d76b5fde..af57903f0b5b 100644 --- a/src/datascience-ui/native-editor/redux/actions.ts +++ b/src/datascience-ui/native-editor/redux/actions.ts @@ -80,9 +80,16 @@ export const actionCreators = { redo: (): CommonAction => ({ type: CommonActionType.REDO }), arrowUp: (cellId: string, code: string): CommonAction => ({ type: CommonActionType.ARROW_UP, payload: { cellId, code } }), arrowDown: (cellId: string, code: string): CommonAction => ({ type: CommonActionType.ARROW_DOWN, payload: { cellId, code } }), - editCell: (cellId: string, changes: monacoEditor.editor.IModelContentChange[], modelId: string, code: string): CommonAction => ({ + editCell: ( + cellId: string, + changes: monacoEditor.editor.IModelContentChange[], + modelId: string, + code: string, + isUndo: boolean, + isRedo: boolean + ): CommonAction => ({ type: CommonActionType.EDIT_CELL, - payload: { cellId, changes, modelId, code } + payload: { cellId, changes, modelId, code, isUndo, isRedo } }), linkClick: (href: string): CommonAction => ({ type: CommonActionType.LINK_CLICK, payload: { href } }), showPlot: (imageHtml: string): CommonAction => ({ type: CommonActionType.SHOW_PLOT, payload: { imageHtml } }), diff --git a/src/datascience-ui/native-editor/redux/reducers/creation.ts b/src/datascience-ui/native-editor/redux/reducers/creation.ts index dfb27ff0dc53..c61e9c87f7d9 100644 --- a/src/datascience-ui/native-editor/redux/reducers/creation.ts +++ b/src/datascience-ui/native-editor/redux/reducers/creation.ts @@ -76,14 +76,11 @@ export namespace Creation { // Find the position where we want to insert let position = arg.prevState.cellVMs.findIndex(c => c.cell.id === arg.payload.cellId); - let index = 0; if (position >= 0) { newList.splice(position + 1, 0, newVM); - index = position + 1; } else { newList.push(newVM); position = newList.length - 2; - index = newList.length - 1; } const result = { diff --git a/src/test/datascience/intellisense.unit.test.ts b/src/test/datascience/intellisense.unit.test.ts index a7bf49ed3bda..0d0c2ff40c60 100644 --- a/src/test/datascience/intellisense.unit.test.ts +++ b/src/test/datascience/intellisense.unit.test.ts @@ -117,7 +117,9 @@ suite('DataScience Intellisense Unit Tests', () => { newDirty: true, changes: [change], cell: cells.find(c => c.id === id)!, - newText: newCode + newText: newCode, + isUndo: false, + isRedo: false }); } @@ -143,7 +145,9 @@ suite('DataScience Intellisense Unit Tests', () => { newDirty: true, changes: [change], cell: cells[cells.length - 1], - newText: code + newText: code, + isUndo: false, + isRedo: false }); } @@ -169,7 +173,9 @@ suite('DataScience Intellisense Unit Tests', () => { newDirty: true, changes: [change], cell: cells[cells.length - 1], - newText: '' + newText: '', + isRedo: false, + isUndo: false }); } diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index 61aef8c2608e..477eaec5ed12 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -377,7 +377,9 @@ suite('Data Science - Native Editor Storage', () => { newDirty: true, changes, cell, - newText: newCode + newText: newCode, + isUndo: false, + isRedo: false }); } From 433c4a9eb3706c095a22087f04e94ed368e104ff Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 3 Feb 2020 16:49:07 -0800 Subject: [PATCH 05/23] New idea with reverse edit and disable undo/redo in monaco --- .../intellisense/intellisenseProvider.ts | 4 +- .../interactiveWindowTypes.ts | 8 +-- .../interactive-ipynb/nativeEditorStorage.ts | 58 +++++++++---------- .../history-react/interactiveCell.tsx | 5 +- .../history-react/redux/actions.ts | 9 ++- .../interactive-common/cellInput.tsx | 9 ++- .../interactive-common/code.tsx | 8 ++- .../interactive-common/editor.tsx | 35 ++++++++++- .../interactive-common/markdown.tsx | 8 ++- .../redux/reducers/transfer.ts | 12 ++-- .../redux/reducers/types.ts | 4 +- .../native-editor/nativeCell.tsx | 5 +- .../native-editor/redux/actions.ts | 9 ++- .../native-editor/redux/reducers/creation.ts | 27 ++++++++- .../datascience/intellisense.unit.test.ts | 24 +++----- .../nativeEditorStorage.unit.test.ts | 10 ++-- 16 files changed, 138 insertions(+), 97 deletions(-) diff --git a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts index 909000a12abd..830309df153f 100644 --- a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts +++ b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts @@ -523,7 +523,7 @@ export class IntellisenseProvider implements IInteractiveWindowListener { // This one can be ignored, it only clears outputs break; case 'edit': - changes = document.reloadCell(request.cell.id, concatMultilineStringInput(request.cell.data.source)); + changes = document.editCell(request.reverse, request.id); break; case 'insert': changes = document.remove(request.cell.id); @@ -558,7 +558,7 @@ export class IntellisenseProvider implements IInteractiveWindowListener { // This one can be ignored, it only clears outputs break; case 'edit': - changes = document.editCell(request.changes, request.cell.id); + changes = document.editCell(request.forward, request.id); break; case 'insert': changes = document.insertCell(request.cell.id, concatMultilineStringInput(request.cell.data.source), request.codeCellAboveId); diff --git a/src/client/datascience/interactive-common/interactiveWindowTypes.ts b/src/client/datascience/interactive-common/interactiveWindowTypes.ts index 903de03200d5..38c26f6c11af 100644 --- a/src/client/datascience/interactive-common/interactiveWindowTypes.ts +++ b/src/client/datascience/interactive-common/interactiveWindowTypes.ts @@ -374,11 +374,9 @@ export interface ICellContentChange { export interface INotebookModelEditChange extends INotebookModelChange { kind: 'edit'; - changes: ICellContentChange[]; - newText: string; - cell: ICell; - isUndo: boolean; - isRedo: boolean; + forward: ICellContentChange[]; + reverse: ICellContentChange[]; + id: string; } export interface INotebookModelVersionChange extends INotebookModelChange { diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index e4822f92bcf3..70944007d281 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -12,7 +12,7 @@ import { GLOBAL_MEMENTO, ICryptoUtils, IDisposable, IDisposableRegistry, IExtens import { noop } from '../../common/utils/misc'; import { PythonInterpreter } from '../../interpreter/contracts'; import { Commands, Identifiers } from '../constants'; -import { INotebookModelEditChange, NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; +import { ICellContentChange, NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; import { InvalidNotebookFileError } from '../jupyter/invalidNotebookFileError'; import { LiveKernelModel } from '../jupyter/kernels/types'; import { CellState, ICell, IJupyterExecution, IJupyterKernelSpec, INotebookModel, INotebookStorage } from '../types'; @@ -148,7 +148,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID changed = this.clearOutputs(); break; case 'edit': - changed = this.editCell(change); + changed = this.editCell(change.forward, change.id); break; case 'insert': changed = this.insertCell(change.cell, change.index); @@ -191,7 +191,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID changed = true; break; case 'edit': - changed = this.replaceCell(change.cell); + changed = this.editCell(change.reverse, change.id); break; case 'insert': changed = this.removeCell(change.cell); @@ -227,28 +227,31 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID return true; } - private editCell(change: INotebookModelEditChange): boolean { - // Apply the changes to the visible cell list. We won't get an update until - // submission otherwise - if (change.changes && change.changes.length) { - const first = change.changes[0]; - const normalized = first.text.replace(/\r/g, ''); - - // Figure out which cell we're editing. - const index = this.cells.findIndex(c => c.id === change.cell.id); - if (index >= 0) { - // This is an actual edit. - const contents = concatMultilineStringInput(this.cells[index].data.source); - const before = contents.substr(0, first.rangeOffset); - const after = contents.substr(first.rangeOffset + first.rangeLength); - const newContents = `${before}${normalized}${after}`; - if (contents !== newContents) { - const newCell = { ...this.cells[index], data: { ...this.cells[index].data, source: newContents } }; - this._state.cells[index] = this.asCell(newCell); - return true; - } + private applyCellContentChange(change: ICellContentChange, id: string): boolean { + const normalized = change.text.replace(/\r/g, ''); + + // Figure out which cell we're editing. + const index = this.cells.findIndex(c => c.id === id); + if (index >= 0) { + // This is an actual edit. + const contents = concatMultilineStringInput(this.cells[index].data.source); + const before = contents.substr(0, change.rangeOffset); + const after = contents.substr(change.rangeOffset + change.rangeLength); + const newContents = `${before}${normalized}${after}`; + if (contents !== newContents) { + const newCell = { ...this.cells[index], data: { ...this.cells[index].data, source: newContents } }; + this._state.cells[index] = this.asCell(newCell); + return true; } } + return false; + } + + private editCell(changes: ICellContentChange[], id: string): boolean { + // Apply the changes to the visible cell list + if (changes && changes.length) { + return changes.map(c => this.applyCellContentChange(c, id)).reduce((p, c) => p || c, false); + } return false; } @@ -279,15 +282,6 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID return true; } - private replaceCell(cell: ICell): boolean { - const index = this.cells.findIndex(c => c.id === cell.id); - if (index >= 0) { - this._state.cells[index] = cell; - return true; - } - return false; - } - private clearOutputs(): boolean { this._state.cells = this.cells.map(c => this.asCell({ ...c, data: { ...c.data, execution_count: null, outputs: [] } })); return true; diff --git a/src/datascience-ui/history-react/interactiveCell.tsx b/src/datascience-ui/history-react/interactiveCell.tsx index 847a54982369..53b13a21bd4b 100644 --- a/src/datascience-ui/history-react/interactiveCell.tsx +++ b/src/datascience-ui/history-react/interactiveCell.tsx @@ -266,6 +266,7 @@ export class InteractiveCell extends React.Component { keyDown={this.isEditCell() ? this.onEditCellKeyDown : undefined} showLineNumbers={this.props.cellVM.showLineNumbers} font={this.props.font} + disableUndoStack={this.props.cellVM.cell.id !== Identifiers.EditCellId} /> ); } @@ -286,8 +287,8 @@ export class InteractiveCell extends React.Component { return contents || concatMultilineStringInput(this.props.cellVM.cell.data.source); } - private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string, isUndo: boolean, isRedo: boolean) => { - this.props.editCell(cellId, changes, modelId, this.getCurrentCode(), isUndo, isRedo); + private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string) => { + this.props.editCell(cellId, modelId, changes, reverse, this.getCurrentCode()); }; private onCodeCreated = (_code: string, _file: string, cellId: string, modelId: string) => { diff --git a/src/datascience-ui/history-react/redux/actions.ts b/src/datascience-ui/history-react/redux/actions.ts index 4c49b0b54e92..acf3bacd8d1a 100644 --- a/src/datascience-ui/history-react/redux/actions.ts +++ b/src/datascience-ui/history-react/redux/actions.ts @@ -37,14 +37,13 @@ export const actionCreators = { doubleClickCell: (cellId: string): CommonAction => ({ type: CommonActionType.DOUBLE_CLICK_CELL, payload: { cellId } }), editCell: ( cellId: string, - changes: monacoEditor.editor.IModelContentChange[], modelId: string, - code: string, - isUndo: boolean, - isRedo: boolean + changes: monacoEditor.editor.IModelContentChange[], + reverse: monacoEditor.editor.IModelContentChange[], + code: string ): CommonAction => ({ type: CommonActionType.EDIT_CELL, - payload: { cellId, changes, modelId, code, isUndo, isRedo } + payload: { cellId, modelId, changes, reverse, id: cellId, code } }), submitInput: (code: string, cellId: string): CommonAction => ({ type: CommonActionType.SUBMIT_INPUT, payload: { code, cellId } }), toggleVariableExplorer: (): CommonAction => ({ type: CommonActionType.TOGGLE_VARIABLE_EXPLORER }), diff --git a/src/datascience-ui/interactive-common/cellInput.tsx b/src/datascience-ui/interactive-common/cellInput.tsx index 5d05e79d7f31..fbff1a95a91e 100644 --- a/src/datascience-ui/interactive-common/cellInput.tsx +++ b/src/datascience-ui/interactive-common/cellInput.tsx @@ -26,7 +26,8 @@ interface ICellInputProps { editorMeasureClassName?: string; showLineNumbers?: boolean; font: IFont; - onCodeChange(changes: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string, isUndo: boolean, isRedo: boolean): void; + disableUndoStack: boolean; + onCodeChange(changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string): void; onCodeCreated(code: string, file: string, cellId: string, modelId: string): void; openLink(uri: monacoEditor.Uri): void; keyDown?(cellId: string, e: IKeyboardEvent): void; @@ -110,6 +111,7 @@ export class CellInput extends React.Component { showLineNumbers={this.props.showLineNumbers} useQuickEdit={this.props.cellVM.useQuickEdit} font={this.props.font} + disableUndoStack={this.props.disableUndoStack} /> ); @@ -142,6 +144,7 @@ export class CellInput extends React.Component { ref={this.markdownRef} useQuickEdit={false} font={this.props.font} + disableUndoStack={this.props.disableUndoStack} /> ); @@ -180,8 +183,8 @@ export class CellInput extends React.Component { } }; - private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], modelId: string, isUndo: boolean, isRedo: boolean) => { - this.props.onCodeChange(changes, this.props.cellVM.cell.id, modelId, isUndo, isRedo); + private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], modelId: string) => { + this.props.onCodeChange(changes, reverse, this.props.cellVM.cell.id, modelId); }; private onCodeCreated = (code: string, modelId: string) => { diff --git a/src/datascience-ui/interactive-common/code.tsx b/src/datascience-ui/interactive-common/code.tsx index ff6ece96895e..45c85c97ba4c 100644 --- a/src/datascience-ui/interactive-common/code.tsx +++ b/src/datascience-ui/interactive-common/code.tsx @@ -26,8 +26,9 @@ export interface ICodeProps { font: IFont; hasFocus: boolean; cursorPos: CursorPos; + disableUndoStack: boolean; onCreated(code: string, modelId: string): void; - onChange(changes: monacoEditor.editor.IModelContentChange[], modelId: string, isUndo: boolean, isRedo: boolean): void; + onChange(changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], modelId: string): void; openLink(uri: monacoEditor.Uri): void; keyDown?(e: IKeyboardEvent): void; focused?(): void; @@ -76,6 +77,7 @@ export class Code extends React.Component { showLineNumbers={this.props.showLineNumbers} useQuickEdit={this.props.useQuickEdit} font={this.props.font} + disableUndoStack={this.props.disableUndoStack} />
{this.getWatermarkString()} @@ -106,10 +108,10 @@ export class Code extends React.Component { return getLocString('DataScience.inputWatermark', 'Type code here and press shift-enter to run'); }; - private onModelChanged = (changes: monacoEditor.editor.IModelContentChange[], isUndo: boolean, isRedo: boolean, model: monacoEditor.editor.ITextModel) => { + private onModelChanged = (changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], model: monacoEditor.editor.ITextModel) => { if (!this.props.readOnly && model) { this.setState({ allowWatermark: model.getValueLength() === 0 }); } - this.props.onChange(changes, model.id, isUndo, isRedo); + this.props.onChange(changes, reverse, model.id); }; } diff --git a/src/datascience-ui/interactive-common/editor.tsx b/src/datascience-ui/interactive-common/editor.tsx index afc44f0e9d01..c92dfd086ea1 100644 --- a/src/datascience-ui/interactive-common/editor.tsx +++ b/src/datascience-ui/interactive-common/editor.tsx @@ -27,8 +27,9 @@ export interface IEditorProps { font: IFont; hasFocus: boolean; cursorPos: CursorPos; + disableUndoStack: boolean; onCreated(code: string, modelId: string): void; - onChange(changes: monacoEditor.editor.IModelContentChange[], isUndo: boolean, isRedo: boolean, model: monacoEditor.editor.ITextModel): void; + onChange(changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], model: monacoEditor.editor.ITextModel): void; openLink(uri: monacoEditor.Uri): void; keyDown?(e: IKeyboardEvent): void; focused?(): void; @@ -165,6 +166,13 @@ export class Editor extends React.Component { const model = editor.getModel(); this.setState({ editor, model: editor.getModel() }); + // Disable undo/redo on the model if asked + // tslint:disable: no-any + if (this.props.disableUndoStack && (model as any).undo && (model as any).redo) { + (model as any).undo = noop; + (model as any).redo = noop; + } + // Listen for model changes this.subscriptions.push(editor.onDidChangeModelContent(this.modelChanged)); @@ -182,9 +190,32 @@ export class Editor extends React.Component { this.subscriptions.push(editor.onDidBlurEditorWidget(this.props.unfocused ? this.props.unfocused : noop)); }; + private generateReverseChange(c: monacoEditor.editor.IModelContentChange): monacoEditor.editor.IModelContentChange { + const oldStart = this.state.model!.getPositionAt(c.rangeOffset); + const oldEnd = this.state.model!.getPositionAt(c.rangeOffset + c.rangeLength); + const oldText = this.state.model!.getValueInRange(c.range); + const oldRange: monacoEditor.IRange = { + startColumn: oldStart.column, + startLineNumber: oldStart.lineNumber, + endColumn: oldEnd.column, + endLineNumber: oldEnd.lineNumber + }; + return { + rangeLength: c.text.length, + rangeOffset: c.rangeOffset, + text: oldText ? oldText : '', + range: oldRange + }; + } + + private generateReverseChanges(c: monacoEditor.editor.IModelContentChangedEvent): monacoEditor.editor.IModelContentChange[] { + // Reverse changes are always in reverse order + return c.changes.map(this.generateReverseChange).reverse(); + } + private modelChanged = (e: monacoEditor.editor.IModelContentChangedEvent) => { if (this.state.model) { - this.props.onChange(e.changes, e.isUndoing, e.isRedoing, this.state.model); + this.props.onChange(e.changes, this.generateReverseChanges(e), this.state.model); } }; diff --git a/src/datascience-ui/interactive-common/markdown.tsx b/src/datascience-ui/interactive-common/markdown.tsx index 42ae8f463a22..d48f0482db6c 100644 --- a/src/datascience-ui/interactive-common/markdown.tsx +++ b/src/datascience-ui/interactive-common/markdown.tsx @@ -21,8 +21,9 @@ export interface IMarkdownProps { font: IFont; hasFocus: boolean; cursorPos: CursorPos; + disableUndoStack: boolean; onCreated(code: string, modelId: string): void; - onChange(changes: monacoEditor.editor.IModelContentChange[], modelId: string, isUndo: boolean, isRedo: boolean): void; + onChange(changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], modelId: string): void; focused?(): void; unfocused?(): void; openLink(uri: monacoEditor.Uri): void; @@ -64,6 +65,7 @@ export class Markdown extends React.Component { showLineNumbers={this.props.showLineNumbers} useQuickEdit={this.props.useQuickEdit} font={this.props.font} + disableUndoStack={this.props.disableUndoStack} />
); @@ -75,7 +77,7 @@ export class Markdown extends React.Component { } } - private onModelChanged = (changes: monacoEditor.editor.IModelContentChange[], isUndo: boolean, isRedo: boolean, model: monacoEditor.editor.ITextModel) => { - this.props.onChange(changes, model.id, isUndo, isRedo); + private onModelChanged = (changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], model: monacoEditor.editor.ITextModel) => { + this.props.onChange(changes, reverse, model.id); }; } diff --git a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts index b904b50a36dd..e80ad4927e11 100644 --- a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts +++ b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts @@ -95,17 +95,15 @@ export namespace Transfer { arg.queueAction(createPostableAction(InteractiveWindowMessages.UpdateModel, update)); } - export function postModelEdit(arg: CommonReducerArg, changes: ICellContentChange[], cell: ICell, newText: string, isUndo: boolean, isRedo: boolean) { + export function postModelEdit(arg: CommonReducerArg, forward: ICellContentChange[], reverse: ICellContentChange[], id: string) { postModelUpdate(arg, { source: 'user', kind: 'edit', newDirty: true, oldDirty: arg.prevState.dirty, - changes, - cell, - newText, - isUndo, - isRedo + forward, + reverse, + id }); } @@ -174,7 +172,7 @@ export namespace Transfer { const cellVM = arg.prevState.cellVMs.find(c => c.cell.id === arg.payload.cellId); if (cellVM) { // Tell the underlying model on the extension side - postModelEdit(arg, arg.payload.changes, cellVM.cell, arg.payload.code, arg.payload.isUndo, arg.payload.isRedo); + postModelEdit(arg, arg.payload.changes, arg.payload.reverse, cellVM.cell.id); // Update the uncomitted text on the cell view model // We keep this saved here so we don't re-render and we put this code into the input / code data diff --git a/src/datascience-ui/interactive-common/redux/reducers/types.ts b/src/datascience-ui/interactive-common/redux/reducers/types.ts index 75a803ff37bc..4ed08e1db6af 100644 --- a/src/datascience-ui/interactive-common/redux/reducers/types.ts +++ b/src/datascience-ui/interactive-common/redux/reducers/types.ts @@ -108,9 +108,9 @@ export interface ICodeAction extends ICellAction { export interface IEditCellAction extends ICodeAction { changes: monacoEditor.editor.IModelContentChange[]; + reverse: monacoEditor.editor.IModelContentChange[]; + id: string; modelId: string; - isUndo: boolean; - isRedo: boolean; } // I.e. when using the operation `add`, we need the corresponding `IAddCellAction`. diff --git a/src/datascience-ui/native-editor/nativeCell.tsx b/src/datascience-ui/native-editor/nativeCell.tsx index 974091434340..1a205bbf774b 100644 --- a/src/datascience-ui/native-editor/nativeCell.tsx +++ b/src/datascience-ui/native-editor/nativeCell.tsx @@ -608,6 +608,7 @@ export class NativeCell extends React.Component { keyDown={this.keyDownInput} showLineNumbers={this.props.cellVM.showLineNumbers} font={this.props.font} + disableUndoStack={true} /> ); @@ -624,8 +625,8 @@ export class NativeCell extends React.Component { this.props.unfocusCell(this.cellId, this.getCurrentCode()); }; - private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string, isUndo: boolean, isRedo: boolean) => { - this.props.editCell(cellId, changes, modelId, this.getCurrentCode(), isUndo, isRedo); + private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string) => { + this.props.editCell(cellId, modelId, changes, reverse, this.getCurrentCode()); }; private onCodeCreated = (_code: string, _file: string, cellId: string, modelId: string) => { diff --git a/src/datascience-ui/native-editor/redux/actions.ts b/src/datascience-ui/native-editor/redux/actions.ts index af57903f0b5b..b6352f72dff2 100644 --- a/src/datascience-ui/native-editor/redux/actions.ts +++ b/src/datascience-ui/native-editor/redux/actions.ts @@ -82,14 +82,13 @@ export const actionCreators = { arrowDown: (cellId: string, code: string): CommonAction => ({ type: CommonActionType.ARROW_DOWN, payload: { cellId, code } }), editCell: ( cellId: string, - changes: monacoEditor.editor.IModelContentChange[], modelId: string, - code: string, - isUndo: boolean, - isRedo: boolean + changes: monacoEditor.editor.IModelContentChange[], + reverse: monacoEditor.editor.IModelContentChange[], + code: string ): CommonAction => ({ type: CommonActionType.EDIT_CELL, - payload: { cellId, changes, modelId, code, isUndo, isRedo } + payload: { cellId, modelId, changes, reverse, id: cellId, code } }), linkClick: (href: string): CommonAction => ({ type: CommonActionType.LINK_CLICK, payload: { href } }), showPlot: (imageHtml: string): CommonAction => ({ type: CommonActionType.SHOW_PLOT, payload: { imageHtml } }), diff --git a/src/datascience-ui/native-editor/redux/reducers/creation.ts b/src/datascience-ui/native-editor/redux/reducers/creation.ts index c61e9c87f7d9..8caa8b372b4b 100644 --- a/src/datascience-ui/native-editor/redux/reducers/creation.ts +++ b/src/datascience-ui/native-editor/redux/reducers/creation.ts @@ -4,7 +4,7 @@ import * as uuid from 'uuid/v4'; import { noop } from '../../../../client/common/utils/misc'; -import { ILoadAllCells, NotebookModelChange } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; +import { ICellContentChange, ILoadAllCells, NotebookModelChange } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; import { ICell, IDataScienceExtraSettings } from '../../../../client/datascience/types'; import { createCellVM, createEmptyCell, CursorPos, extractInputText, ICellViewModel, IMainState } from '../../../interactive-common/mainState'; import { Helpers } from '../../../interactive-common/redux/reducers/helpers'; @@ -152,6 +152,27 @@ export namespace Creation { }; } + export function editCell(arg: NativeEditorReducerArg<{ id: string; changes: ICellContentChange[] }>): IMainState { + const index = arg.prevState.cellVMs.findIndex(c => c.cell.id === arg.payload.id); + if (index) { + const newVM = { ...arg.prevState.cellVMs[index] }; + arg.payload.changes.forEach(c => { + const source = newVM.inputBlockText; + const before = source.slice(0, c.rangeOffset); + const after = source.slice(c.rangeOffset + c.rangeLength); + newVM.inputBlockText = `${before}${c.text}${after}`; + }); + newVM.cell.data.source = newVM.inputBlockText; + const newVMs = [...arg.prevState.cellVMs]; + newVMs[index] = Helpers.asCellViewModel(newVM); + return { + ...arg.prevState, + cellVMs: newVMs + }; + } + return arg.prevState; + } + export function deleteCell(arg: NativeEditorReducerArg): IMainState { const cells = arg.prevState.cellVMs; if (cells.length === 1 && cells[0].cell.id === arg.payload.cellId) { @@ -241,7 +262,7 @@ export namespace Creation { case 'clear': return loadAllCells({ ...disabledQueueArg, payload: { cells: arg.payload.oldCells } }); case 'edit': - return updateCell({ ...disabledQueueArg, payload: arg.payload.cell }); + return editCell({ ...disabledQueueArg, payload: { id: arg.payload.id, changes: arg.payload.reverse } }); case 'insert': return deleteCell({ ...disabledQueueArg, payload: { cellId: arg.payload.cell.id } }); case 'remove': @@ -267,7 +288,7 @@ export namespace Creation { case 'clear': return Execution.clearAllOutputs(disabledQueueArg); case 'edit': - return updateCell({ ...disabledQueueArg, payload: Helpers.asCell({ ...arg.payload.cell, data: { ...arg.payload.cell.data, source: arg.payload.newText } }) }); + return editCell({ ...disabledQueueArg, payload: { id: arg.payload.id, changes: arg.payload.forward } }); case 'insert': return insertAbove({ ...disabledQueueArg, payload: { newCellId: arg.payload.cell.id, cellId: arg.payload.codeCellAboveId } }); case 'remove': diff --git a/src/test/datascience/intellisense.unit.test.ts b/src/test/datascience/intellisense.unit.test.ts index 0d0c2ff40c60..71cd5da7415c 100644 --- a/src/test/datascience/intellisense.unit.test.ts +++ b/src/test/datascience/intellisense.unit.test.ts @@ -115,11 +115,9 @@ suite('DataScience Intellisense Unit Tests', () => { kind: 'edit', oldDirty: false, newDirty: true, - changes: [change], - cell: cells.find(c => c.id === id)!, - newText: newCode, - isUndo: false, - isRedo: false + forward: [change], + reverse: [change], + id }); } @@ -143,11 +141,9 @@ suite('DataScience Intellisense Unit Tests', () => { kind: 'edit', oldDirty: false, newDirty: true, - changes: [change], - cell: cells[cells.length - 1], - newText: code, - isUndo: false, - isRedo: false + forward: [change], + reverse: [change], + id: cells[cells.length - 1].id }); } @@ -171,11 +167,9 @@ suite('DataScience Intellisense Unit Tests', () => { kind: 'edit', oldDirty: false, newDirty: true, - changes: [change], - cell: cells[cells.length - 1], - newText: '', - isRedo: false, - isUndo: false + forward: [change], + reverse: [change], + id: cells[cells.length - 1].id }); } diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index 477eaec5ed12..3f41346ad209 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -369,17 +369,15 @@ suite('Data Science - Native Editor Storage', () => { }); } - function editCell(changes: ICellContentChange[], cell: ICell, newCode: string) { + function editCell(changes: ICellContentChange[], cell: ICell, _newCode: string) { return executeCommand(Commands.NotebookModel_Update, baseUri, { source: 'user', kind: 'edit', oldDirty: storage.isDirty, newDirty: true, - changes, - cell, - newText: newCode, - isUndo: false, - isRedo: false + forward: changes, + reverse: changes, + id: cell.id }); } From aeb79f94fa9e9dfb66eb7005a9366039b585f595 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 4 Feb 2020 14:37:17 -0800 Subject: [PATCH 06/23] Potentially working with tracking edits --- .../intellisense/intellisenseDocument.ts | 4 +- .../interactiveWindowTypes.ts | 25 ++++- .../interactive-ipynb/nativeEditorStorage.ts | 6 +- .../history-react/interactiveCell.tsx | 13 +-- .../history-react/redux/actions.ts | 12 +-- .../interactive-common/cellInput.tsx | 16 +-- .../interactive-common/code.tsx | 17 ++-- .../interactive-common/editor.tsx | 96 +++++------------- .../interactive-common/mainState.ts | 6 +- .../interactive-common/markdown.tsx | 15 +-- .../redux/reducers/transfer.ts | 11 ++- .../redux/reducers/types.ts | 8 +- .../native-editor/nativeCell.tsx | 6 +- .../native-editor/redux/actions.ts | 12 +-- .../native-editor/redux/reducers/creation.ts | 16 +-- .../native-editor/redux/reducers/effects.ts | 4 +- .../react-common/monacoEditor.tsx | 80 ++++++++++++++- .../react-common/monacoHelpers.ts | 98 +++++++++++++++++++ .../datascience/intellisense.unit.test.ts | 27 +++-- .../nativeEditorStorage.unit.test.ts | 10 +- 20 files changed, 322 insertions(+), 160 deletions(-) create mode 100644 src/datascience-ui/react-common/monacoHelpers.ts diff --git a/src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts b/src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts index 3bcc02baf0ea..3df2d3d4d52c 100644 --- a/src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts +++ b/src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts @@ -8,7 +8,7 @@ import * as vscodeLanguageClient from 'vscode-languageclient'; import { PYTHON_LANGUAGE } from '../../../common/constants'; import { Identifiers } from '../../constants'; -import { ICellContentChange } from '../interactiveWindowTypes'; +import { IEditorContentChange } from '../interactiveWindowTypes'; import { DefaultWordPattern, ensureValidWordDefinition, getWordAtText, regExpLeadsToEndlessLoop } from './wordHelper'; class IntellisenseLine implements TextLine { @@ -376,7 +376,7 @@ export class IntellisenseDocument implements TextDocument { return []; } - public editCell(editorChanges: ICellContentChange[], id: string): TextDocumentContentChangeEvent[] { + public editCell(editorChanges: IEditorContentChange[], id: string): TextDocumentContentChangeEvent[] { this._version += 1; // Convert the range to local (and remove 1 based) diff --git a/src/client/datascience/interactive-common/interactiveWindowTypes.ts b/src/client/datascience/interactive-common/interactiveWindowTypes.ts index 38c26f6c11af..6c360e81237a 100644 --- a/src/client/datascience/interactive-common/interactiveWindowTypes.ts +++ b/src/client/datascience/interactive-common/interactiveWindowTypes.ts @@ -334,7 +334,18 @@ export interface INotebookModelInsertChange extends INotebookModelChange { currentText: string; } -export interface IRange { +export interface IEditorPosition { + /** + * line number (starts at 1) + */ + readonly lineNumber: number; + /** + * column (the first character in a line is between column 1 and column 2) + */ + readonly column: number; +} + +export interface IEditorRange { /** * Line number on which the range starts (starts at 1). */ @@ -353,11 +364,11 @@ export interface IRange { readonly endColumn: number; } -export interface ICellContentChange { +export interface IEditorContentChange { /** * The range that got replaced. */ - readonly range: IRange; + readonly range: IEditorRange; /** * The offset of the range that got replaced. */ @@ -370,12 +381,16 @@ export interface ICellContentChange { * The new text for the range. */ readonly text: string; + /** + * The cursor position to be set after the change + */ + readonly position: IEditorPosition; } export interface INotebookModelEditChange extends INotebookModelChange { kind: 'edit'; - forward: ICellContentChange[]; - reverse: ICellContentChange[]; + forward: IEditorContentChange[]; + reverse: IEditorContentChange[]; id: string; } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 70944007d281..18241427e339 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -12,7 +12,7 @@ import { GLOBAL_MEMENTO, ICryptoUtils, IDisposable, IDisposableRegistry, IExtens import { noop } from '../../common/utils/misc'; import { PythonInterpreter } from '../../interpreter/contracts'; import { Commands, Identifiers } from '../constants'; -import { ICellContentChange, NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; +import { IEditorContentChange, NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; import { InvalidNotebookFileError } from '../jupyter/invalidNotebookFileError'; import { LiveKernelModel } from '../jupyter/kernels/types'; import { CellState, ICell, IJupyterExecution, IJupyterKernelSpec, INotebookModel, INotebookStorage } from '../types'; @@ -227,7 +227,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID return true; } - private applyCellContentChange(change: ICellContentChange, id: string): boolean { + private applyCellContentChange(change: IEditorContentChange, id: string): boolean { const normalized = change.text.replace(/\r/g, ''); // Figure out which cell we're editing. @@ -247,7 +247,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID return false; } - private editCell(changes: ICellContentChange[], id: string): boolean { + private editCell(changes: IEditorContentChange[], id: string): boolean { // Apply the changes to the visible cell list if (changes && changes.length) { return changes.map(c => this.applyCellContentChange(c, id)).reduce((p, c) => p || c, false); diff --git a/src/datascience-ui/history-react/interactiveCell.tsx b/src/datascience-ui/history-react/interactiveCell.tsx index 53b13a21bd4b..67d6eeeea974 100644 --- a/src/datascience-ui/history-react/interactiveCell.tsx +++ b/src/datascience-ui/history-react/interactiveCell.tsx @@ -11,7 +11,6 @@ import { connect } from 'react-redux'; import { Identifiers } from '../../client/datascience/constants'; import { CellState, IDataScienceExtraSettings } from '../../client/datascience/types'; -import { concatMultilineStringInput } from '../common'; import { CellInput } from '../interactive-common/cellInput'; import { CellOutput } from '../interactive-common/cellOutput'; import { CollapseButton } from '../interactive-common/collapseButton'; @@ -23,6 +22,7 @@ import { IKeyboardEvent } from '../react-common/event'; import { Image, ImageName } from '../react-common/image'; import { ImageButton } from '../react-common/imageButton'; import { getLocString } from '../react-common/locReactSide'; +import { IMonacoModelContentChangeEvent } from '../react-common/monacoHelpers'; import { actionCreators } from './redux/actions'; interface IInteractiveCellBaseProps { @@ -267,6 +267,7 @@ export class InteractiveCell extends React.Component { showLineNumbers={this.props.cellVM.showLineNumbers} font={this.props.font} disableUndoStack={this.props.cellVM.cell.id !== Identifiers.EditCellId} + codeVersion={this.props.cellVM.codeVersion ? this.props.cellVM.codeVersion : 0} /> ); } @@ -281,14 +282,8 @@ export class InteractiveCell extends React.Component { this.props.unfocus(this.getCell().id); }; - private getCurrentCode(): string { - // Get current monaco code, if not available fallback to cell data source - const contents = this.codeRef.current ? this.codeRef.current.getContents() : undefined; - return contents || concatMultilineStringInput(this.props.cellVM.cell.data.source); - } - - private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string) => { - this.props.editCell(cellId, modelId, changes, reverse, this.getCurrentCode()); + private onCodeChange = (e: IMonacoModelContentChangeEvent) => { + this.props.editCell(this.getCell().id, e); }; private onCodeCreated = (_code: string, _file: string, cellId: string, modelId: string) => { diff --git a/src/datascience-ui/history-react/redux/actions.ts b/src/datascience-ui/history-react/redux/actions.ts index acf3bacd8d1a..023ef74d2051 100644 --- a/src/datascience-ui/history-react/redux/actions.ts +++ b/src/datascience-ui/history-react/redux/actions.ts @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. 'use strict'; -import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api'; import { IRefreshVariablesRequest } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; import { IJupyterVariable, IJupyterVariablesRequest } from '../../../client/datascience/types'; @@ -17,6 +16,7 @@ import { IShowDataViewerAction, IShowPlotAction } from '../../interactive-common/redux/reducers/types'; +import { IMonacoModelContentChangeEvent } from '../../react-common/monacoHelpers'; // See https://react-redux.js.org/using-react-redux/connect-mapdispatch#defining-mapdispatchtoprops-as-an-object export const actionCreators = { @@ -35,15 +35,9 @@ export const actionCreators = { gatherCell: (cellId: string): CommonAction => ({ type: CommonActionType.GATHER_CELL, payload: { cellId } }), clickCell: (cellId: string): CommonAction => ({ type: CommonActionType.CLICK_CELL, payload: { cellId } }), doubleClickCell: (cellId: string): CommonAction => ({ type: CommonActionType.DOUBLE_CLICK_CELL, payload: { cellId } }), - editCell: ( - cellId: string, - modelId: string, - changes: monacoEditor.editor.IModelContentChange[], - reverse: monacoEditor.editor.IModelContentChange[], - code: string - ): CommonAction => ({ + editCell: (cellId: string, e: IMonacoModelContentChangeEvent): CommonAction => ({ type: CommonActionType.EDIT_CELL, - payload: { cellId, modelId, changes, reverse, id: cellId, code } + payload: { cellId, version: e.versionId, modelId: e.model.id, forward: e.forward, reverse: e.reverse, id: cellId, code: e.model.getValue() } }), submitInput: (code: string, cellId: string): CommonAction => ({ type: CommonActionType.SUBMIT_INPUT, payload: { code, cellId } }), toggleVariableExplorer: (): CommonAction => ({ type: CommonActionType.TOGGLE_VARIABLE_EXPLORER }), diff --git a/src/datascience-ui/interactive-common/cellInput.tsx b/src/datascience-ui/interactive-common/cellInput.tsx index fbff1a95a91e..37fa68551bbd 100644 --- a/src/datascience-ui/interactive-common/cellInput.tsx +++ b/src/datascience-ui/interactive-common/cellInput.tsx @@ -9,6 +9,7 @@ import * as React from 'react'; import { concatMultilineStringInput } from '../common'; import { IKeyboardEvent } from '../react-common/event'; +import { IMonacoModelContentChangeEvent } from '../react-common/monacoHelpers'; import { Code } from './code'; import { InputHistory } from './inputHistory'; import { ICellViewModel, IFont } from './mainState'; @@ -17,6 +18,7 @@ import { Markdown } from './markdown'; // tslint:disable-next-line: no-require-importss interface ICellInputProps { cellVM: ICellViewModel; + codeVersion: number; codeTheme: string; testMode?: boolean; history: InputHistory | undefined; @@ -27,7 +29,7 @@ interface ICellInputProps { showLineNumbers?: boolean; font: IFont; disableUndoStack: boolean; - onCodeChange(changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string): void; + onCodeChange(e: IMonacoModelContentChangeEvent): void; onCodeCreated(code: string, file: string, cellId: string, modelId: string): void; openLink(uri: monacoEditor.Uri): void; keyDown?(cellId: string, e: IKeyboardEvent): void; @@ -97,7 +99,7 @@ export class CellInput extends React.Component { readOnly={!this.props.cellVM.editable} showWatermark={this.props.showWatermark} ref={this.codeRef} - onChange={this.onCodeChange} + onChange={this.props.onCodeChange} onCreated={this.onCodeCreated} outermostParentClass="cell-wrapper" monacoTheme={this.props.monacoTheme} @@ -112,6 +114,8 @@ export class CellInput extends React.Component { useQuickEdit={this.props.cellVM.useQuickEdit} font={this.props.font} disableUndoStack={this.props.disableUndoStack} + version={this.props.codeVersion} + previousCode={this.props.cellVM.uncommittedText} /> ); @@ -130,7 +134,7 @@ export class CellInput extends React.Component { markdown={source} codeTheme={this.props.codeTheme} testMode={this.props.testMode ? true : false} - onChange={this.onCodeChange} + onChange={this.props.onCodeChange} onCreated={this.onCodeCreated} outermostParentClass="cell-wrapper" hasFocus={this.props.cellVM.focused} @@ -145,6 +149,8 @@ export class CellInput extends React.Component { useQuickEdit={false} font={this.props.font} disableUndoStack={this.props.disableUndoStack} + version={this.props.codeVersion} + previousMarkdown={this.props.cellVM.uncommittedText} /> ); @@ -183,10 +189,6 @@ export class CellInput extends React.Component { } }; - private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], modelId: string) => { - this.props.onCodeChange(changes, reverse, this.props.cellVM.cell.id, modelId); - }; - private onCodeCreated = (code: string, modelId: string) => { this.props.onCodeCreated(code, this.props.cellVM.cell.file, this.props.cellVM.cell.id, modelId); }; diff --git a/src/datascience-ui/interactive-common/code.tsx b/src/datascience-ui/interactive-common/code.tsx index 45c85c97ba4c..2baab6eba609 100644 --- a/src/datascience-ui/interactive-common/code.tsx +++ b/src/datascience-ui/interactive-common/code.tsx @@ -7,11 +7,14 @@ import * as React from 'react'; import { InputHistory } from '../interactive-common/inputHistory'; import { IKeyboardEvent } from '../react-common/event'; import { getLocString } from '../react-common/locReactSide'; +import { IMonacoModelContentChangeEvent } from '../react-common/monacoHelpers'; import { Editor } from './editor'; import { CursorPos, IFont } from './mainState'; export interface ICodeProps { code: string; + previousCode: string | undefined; + version: number; codeTheme: string; testMode: boolean; readOnly: boolean; @@ -25,10 +28,10 @@ export interface ICodeProps { useQuickEdit?: boolean; font: IFont; hasFocus: boolean; - cursorPos: CursorPos; + cursorPos: CursorPos | monacoEditor.IPosition; disableUndoStack: boolean; onCreated(code: string, modelId: string): void; - onChange(changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], modelId: string): void; + onChange(e: IMonacoModelContentChangeEvent): void; openLink(uri: monacoEditor.Uri): void; keyDown?(e: IKeyboardEvent): void; focused?(): void; @@ -78,6 +81,8 @@ export class Code extends React.Component { useQuickEdit={this.props.useQuickEdit} font={this.props.font} disableUndoStack={this.props.disableUndoStack} + version={this.props.version} + previousContent={this.props.previousCode} />
{this.getWatermarkString()} @@ -108,10 +113,10 @@ export class Code extends React.Component { return getLocString('DataScience.inputWatermark', 'Type code here and press shift-enter to run'); }; - private onModelChanged = (changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], model: monacoEditor.editor.ITextModel) => { - if (!this.props.readOnly && model) { - this.setState({ allowWatermark: model.getValueLength() === 0 }); + private onModelChanged = (e: IMonacoModelContentChangeEvent) => { + if (!this.props.readOnly && e.model) { + this.setState({ allowWatermark: e.model.getValueLength() === 0 }); } - this.props.onChange(changes, reverse, model.id); + this.props.onChange(e); }; } diff --git a/src/datascience-ui/interactive-common/editor.tsx b/src/datascience-ui/interactive-common/editor.tsx index c92dfd086ea1..b85343e526eb 100644 --- a/src/datascience-ui/interactive-common/editor.tsx +++ b/src/datascience-ui/interactive-common/editor.tsx @@ -7,12 +7,15 @@ import * as React from 'react'; import { noop } from '../../client/common/utils/misc'; import { IKeyboardEvent } from '../react-common/event'; import { MonacoEditor } from '../react-common/monacoEditor'; +import { IMonacoModelContentChangeEvent } from '../react-common/monacoHelpers'; import { InputHistory } from './inputHistory'; import { CursorPos, IFont } from './mainState'; // tslint:disable-next-line: import-name export interface IEditorProps { content: string; + previousContent: string | undefined; + version: number; codeTheme: string; readOnly: boolean; testMode: boolean; @@ -26,23 +29,17 @@ export interface IEditorProps { useQuickEdit?: boolean; font: IFont; hasFocus: boolean; - cursorPos: CursorPos; + cursorPos: CursorPos | monacoEditor.IPosition; disableUndoStack: boolean; onCreated(code: string, modelId: string): void; - onChange(changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], model: monacoEditor.editor.ITextModel): void; + onChange(e: IMonacoModelContentChangeEvent): void; openLink(uri: monacoEditor.Uri): void; keyDown?(e: IKeyboardEvent): void; focused?(): void; unfocused?(): void; } -interface IEditorState { - editor: monacoEditor.editor.IStandaloneCodeEditor | undefined; - model: monacoEditor.editor.ITextModel | null; - forceMonaco: boolean; -} - -export class Editor extends React.Component { +export class Editor extends React.Component { private subscriptions: monacoEditor.IDisposable[] = []; private lastCleanVersionId: number = 0; private monacoRef: React.RefObject = React.createRef(); @@ -56,35 +53,27 @@ export class Editor extends React.Component { this.subscriptions.forEach(d => d.dispose()); }; - public componentDidUpdate(prevProps: IEditorProps, prevState: IEditorState) { - if (this.props.hasFocus && (!prevProps.hasFocus || !prevState.editor)) { + public componentDidUpdate(prevProps: IEditorProps) { + if (this.props.hasFocus && !prevProps.hasFocus) { this.giveFocus(this.props.cursorPos); } } public render() { const classes = this.props.readOnly ? 'editor-area' : 'editor-area editor-area-editable'; - const renderEditor = - this.state.forceMonaco || this.props.useQuickEdit === undefined || this.props.useQuickEdit === false ? this.renderMonacoEditor : this.renderQuickEditor; + const renderEditor = this.props.useQuickEdit === undefined || this.props.useQuickEdit === false ? this.renderMonacoEditor : this.renderQuickEditor; return
{renderEditor()}
; } - public giveFocus(cursorPos: CursorPos) { - const readOnly = this.props.readOnly; - if (this.state.editor && !readOnly) { - this.state.editor.focus(); - } - if (this.state.editor && cursorPos !== CursorPos.Current) { - const current = this.state.editor.getPosition(); - const lineNumber = cursorPos === CursorPos.Top ? 1 : this.state.editor.getModel()!.getLineCount(); - const column = current && current.lineNumber === lineNumber ? current.column : 1; - this.state.editor.setPosition({ lineNumber, column }); + public giveFocus(cursorPos: CursorPos | monacoEditor.IPosition) { + if (this.monacoRef.current) { + this.monacoRef.current.giveFocus(cursorPos); } } public getContents(): string { - if (this.state.model) { - return this.state.model.getValue().replace(/\r/g, ''); + if (this.monacoRef.current) { + return this.monacoRef.current.getContents(); } return ''; } @@ -140,13 +129,17 @@ export class Editor extends React.Component { measureWidthClassName={this.props.editorMeasureClassName} testMode={this.props.testMode} value={this.props.content} + previousValue={this.props.previousContent} outermostParentClass={this.props.outermostParentClass} theme={this.props.monacoTheme ? this.props.monacoTheme : 'vs'} language={this.props.language} editorMounted={this.editorDidMount} + modelChanged={this.props.onChange} options={options} + version={this.props.version} openLink={this.props.openLink} ref={this.monacoRef} + cursorPos={this.props.cursorPos} /> ); }; @@ -173,9 +166,6 @@ export class Editor extends React.Component { (model as any).redo = noop; } - // Listen for model changes - this.subscriptions.push(editor.onDidChangeModelContent(this.modelChanged)); - // List for key up/down events if not read only if (!this.props.readOnly) { this.subscriptions.push(editor.onKeyDown(this.onKeyDown)); @@ -190,64 +180,32 @@ export class Editor extends React.Component { this.subscriptions.push(editor.onDidBlurEditorWidget(this.props.unfocused ? this.props.unfocused : noop)); }; - private generateReverseChange(c: monacoEditor.editor.IModelContentChange): monacoEditor.editor.IModelContentChange { - const oldStart = this.state.model!.getPositionAt(c.rangeOffset); - const oldEnd = this.state.model!.getPositionAt(c.rangeOffset + c.rangeLength); - const oldText = this.state.model!.getValueInRange(c.range); - const oldRange: monacoEditor.IRange = { - startColumn: oldStart.column, - startLineNumber: oldStart.lineNumber, - endColumn: oldEnd.column, - endLineNumber: oldEnd.lineNumber - }; - return { - rangeLength: c.text.length, - rangeOffset: c.rangeOffset, - text: oldText ? oldText : '', - range: oldRange - }; - } - - private generateReverseChanges(c: monacoEditor.editor.IModelContentChangedEvent): monacoEditor.editor.IModelContentChange[] { - // Reverse changes are always in reverse order - return c.changes.map(this.generateReverseChange).reverse(); - } - - private modelChanged = (e: monacoEditor.editor.IModelContentChangedEvent) => { - if (this.state.model) { - this.props.onChange(e.changes, this.generateReverseChanges(e), this.state.model); - } - }; - // tslint:disable-next-line: cyclomatic-complexity private onKeyDown = (e: monacoEditor.IKeyboardEvent) => { - if (this.state.editor && this.state.model && this.monacoRef && this.monacoRef.current) { - const cursor = this.state.editor.getPosition(); + if (this.monacoRef.current) { + const cursor = this.monacoRef.current.getPosition(); const currentLine = this.monacoRef.current.getCurrentVisibleLine(); const visibleLineCount = this.monacoRef.current.getVisibleLineCount(); const isSuggesting = this.monacoRef.current.isSuggesting(); const isFirstLine = currentLine === 0; const isLastLine = currentLine === visibleLineCount - 1; - const isDirty = this.state.model!.getVersionId() > this.lastCleanVersionId; + const isDirty = this.monacoRef.current.getVersionId() > this.lastCleanVersionId; // See if we need to use the history or not if (cursor && this.props.history && e.code === 'ArrowUp' && isFirstLine && !isSuggesting) { const currentValue = this.getContents(); const newValue = this.props.history.completeUp(currentValue); if (newValue !== currentValue) { - this.state.model.setValue(newValue); - this.lastCleanVersionId = this.state.model.getVersionId(); - this.state.editor.setPosition({ lineNumber: 1, column: 1 }); + this.monacoRef.current.setValue(newValue, CursorPos.Top); + this.lastCleanVersionId = this.monacoRef.current.getVersionId(); e.stopPropagation(); } } else if (cursor && this.props.history && e.code === 'ArrowDown' && isLastLine && !isSuggesting) { const currentValue = this.getContents(); const newValue = this.props.history.completeDown(currentValue); if (newValue !== currentValue) { - this.state.model.setValue(newValue); - this.lastCleanVersionId = this.state.model.getVersionId(); - const lastLine = this.state.model.getLineCount(); - this.state.editor.setPosition({ lineNumber: lastLine, column: this.state.model.getLineLength(lastLine) + 1 }); + this.monacoRef.current.setValue(newValue, CursorPos.Bottom); + this.lastCleanVersionId = this.monacoRef.current.getVersionId(); e.stopPropagation(); } } else if (this.props.keyDown) { @@ -283,8 +241,8 @@ export class Editor extends React.Component { }; private clear = () => { - if (this.state.editor) { - this.state.editor.setValue(''); + if (this.monacoRef.current) { + this.monacoRef.current.setValue('', CursorPos.Top); } }; } diff --git a/src/datascience-ui/interactive-common/mainState.ts b/src/datascience-ui/interactive-common/mainState.ts index 3c4799405794..7d285d365da8 100644 --- a/src/datascience-ui/interactive-common/mainState.ts +++ b/src/datascience-ui/interactive-common/mainState.ts @@ -10,6 +10,7 @@ import * as path from 'path'; import { IDataScienceSettings } from '../../client/common/types'; import { CellMatcher } from '../../client/datascience/cellMatcher'; import { Identifiers } from '../../client/datascience/constants'; +import { IEditorPosition } from '../../client/datascience/interactive-common/interactiveWindowTypes'; import { CellState, ICell, IDataScienceExtraSettings, IMessageCell } from '../../client/datascience/types'; import { concatMultilineStringInput, splitMultilineString } from '../common'; import { createCodeCell } from '../common/cellFactory'; @@ -35,10 +36,11 @@ export interface ICellViewModel { selected: boolean; focused: boolean; scrollCount: number; - cursorPos: CursorPos; + cursorPos: CursorPos | IEditorPosition; hasBeenRun: boolean; runDuringDebug?: boolean; - uncomittedText?: string; + uncommittedText?: string; + codeVersion?: number; } export type IMainState = { diff --git a/src/datascience-ui/interactive-common/markdown.tsx b/src/datascience-ui/interactive-common/markdown.tsx index d48f0482db6c..d1efae38747c 100644 --- a/src/datascience-ui/interactive-common/markdown.tsx +++ b/src/datascience-ui/interactive-common/markdown.tsx @@ -5,11 +5,14 @@ import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api'; import * as React from 'react'; import { IKeyboardEvent } from '../react-common/event'; +import { IMonacoModelContentChangeEvent } from '../react-common/monacoHelpers'; import { Editor } from './editor'; import { CursorPos, IFont } from './mainState'; export interface IMarkdownProps { markdown: string; + previousMarkdown: string | undefined; + version: number; codeTheme: string; testMode: boolean; monacoTheme: string | undefined; @@ -20,10 +23,10 @@ export interface IMarkdownProps { useQuickEdit?: boolean; font: IFont; hasFocus: boolean; - cursorPos: CursorPos; + cursorPos: CursorPos | monacoEditor.IPosition; disableUndoStack: boolean; onCreated(code: string, modelId: string): void; - onChange(changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], modelId: string): void; + onChange(e: IMonacoModelContentChangeEvent): void; focused?(): void; unfocused?(): void; openLink(uri: monacoEditor.Uri): void; @@ -47,7 +50,7 @@ export class Markdown extends React.Component { readOnly={false} history={undefined} onCreated={this.props.onCreated} - onChange={this.onModelChanged} + onChange={this.props.onChange} testMode={this.props.testMode} content={this.props.markdown} outermostParentClass={this.props.outermostParentClass} @@ -66,6 +69,8 @@ export class Markdown extends React.Component { useQuickEdit={this.props.useQuickEdit} font={this.props.font} disableUndoStack={this.props.disableUndoStack} + version={this.props.version} + previousContent={this.props.previousMarkdown} />
); @@ -76,8 +81,4 @@ export class Markdown extends React.Component { return this.editorRef.current.getContents(); } } - - private onModelChanged = (changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], model: monacoEditor.editor.ITextModel) => { - this.props.onChange(changes, reverse, model.id); - }; } diff --git a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts index e80ad4927e11..c54aa15f0535 100644 --- a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts +++ b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. 'use strict'; -import { ICellContentChange, InteractiveWindowMessages, NotebookModelChange } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; +import { IEditorContentChange, InteractiveWindowMessages, NotebookModelChange } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; import { CssMessages } from '../../../../client/datascience/messages'; import { ICell } from '../../../../client/datascience/types'; import { concatMultilineStringInput } from '../../../common'; @@ -95,7 +95,7 @@ export namespace Transfer { arg.queueAction(createPostableAction(InteractiveWindowMessages.UpdateModel, update)); } - export function postModelEdit(arg: CommonReducerArg, forward: ICellContentChange[], reverse: ICellContentChange[], id: string) { + export function postModelEdit(arg: CommonReducerArg, forward: IEditorContentChange[], reverse: IEditorContentChange[], id: string) { postModelUpdate(arg, { source: 'user', kind: 'edit', @@ -172,9 +172,9 @@ export namespace Transfer { const cellVM = arg.prevState.cellVMs.find(c => c.cell.id === arg.payload.cellId); if (cellVM) { // Tell the underlying model on the extension side - postModelEdit(arg, arg.payload.changes, arg.payload.reverse, cellVM.cell.id); + postModelEdit(arg, arg.payload.forward, arg.payload.reverse, cellVM.cell.id); - // Update the uncomitted text on the cell view model + // Update the uncommitted text on the cell view model // We keep this saved here so we don't re-render and we put this code into the input / code data // when focus is lost const index = arg.prevState.cellVMs.findIndex(c => c.cell.id === arg.payload.cellId); @@ -183,7 +183,8 @@ export namespace Transfer { const current = arg.prevState.cellVMs[index]; const newCell = { ...current, - uncomittedText: arg.payload.code + uncommittedText: arg.payload.code, + codeVersion: arg.payload.version }; // tslint:disable-next-line: no-any diff --git a/src/datascience-ui/interactive-common/redux/reducers/types.ts b/src/datascience-ui/interactive-common/redux/reducers/types.ts index 4ed08e1db6af..a2e926cbaedf 100644 --- a/src/datascience-ui/interactive-common/redux/reducers/types.ts +++ b/src/datascience-ui/interactive-common/redux/reducers/types.ts @@ -1,9 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. 'use strict'; -import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api'; -import { IShowDataViewer, NativeCommandType } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; +import { IEditorContentChange, IShowDataViewer, NativeCommandType } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; import { ActionWithPayload, ReducerArg } from '../../../react-common/reduxUtils'; import { CursorPos, IMainState } from '../../mainState'; @@ -107,10 +106,11 @@ export interface ICodeAction extends ICellAction { } export interface IEditCellAction extends ICodeAction { - changes: monacoEditor.editor.IModelContentChange[]; - reverse: monacoEditor.editor.IModelContentChange[]; + forward: IEditorContentChange[]; + reverse: IEditorContentChange[]; id: string; modelId: string; + version: number; } // I.e. when using the operation `add`, we need the corresponding `IAddCellAction`. diff --git a/src/datascience-ui/native-editor/nativeCell.tsx b/src/datascience-ui/native-editor/nativeCell.tsx index 1a205bbf774b..a0e094589c20 100644 --- a/src/datascience-ui/native-editor/nativeCell.tsx +++ b/src/datascience-ui/native-editor/nativeCell.tsx @@ -24,6 +24,7 @@ import { IKeyboardEvent } from '../react-common/event'; import { Image, ImageName } from '../react-common/image'; import { ImageButton } from '../react-common/imageButton'; import { getLocString } from '../react-common/locReactSide'; +import { IMonacoModelContentChangeEvent } from '../react-common/monacoHelpers'; import { AddCellLine } from './addCellLine'; import { actionCreators } from './redux/actions'; @@ -609,6 +610,7 @@ export class NativeCell extends React.Component { showLineNumbers={this.props.cellVM.showLineNumbers} font={this.props.font} disableUndoStack={true} + codeVersion={this.props.cellVM.codeVersion ? this.props.cellVM.codeVersion : 1} /> ); @@ -625,8 +627,8 @@ export class NativeCell extends React.Component { this.props.unfocusCell(this.cellId, this.getCurrentCode()); }; - private onCodeChange = (changes: monacoEditor.editor.IModelContentChange[], reverse: monacoEditor.editor.IModelContentChange[], cellId: string, modelId: string) => { - this.props.editCell(cellId, modelId, changes, reverse, this.getCurrentCode()); + private onCodeChange = (e: IMonacoModelContentChangeEvent) => { + this.props.editCell(this.getCell().id, e); }; private onCodeCreated = (_code: string, _file: string, cellId: string, modelId: string) => { diff --git a/src/datascience-ui/native-editor/redux/actions.ts b/src/datascience-ui/native-editor/redux/actions.ts index b6352f72dff2..0152dabd8e60 100644 --- a/src/datascience-ui/native-editor/redux/actions.ts +++ b/src/datascience-ui/native-editor/redux/actions.ts @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. 'use strict'; -import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api'; import * as uuid from 'uuid/v4'; import { NativeCommandType } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; import { IJupyterVariable, IJupyterVariablesRequest } from '../../../client/datascience/types'; @@ -23,6 +22,7 @@ import { IShowDataViewerAction, IShowPlotAction } from '../../interactive-common/redux/reducers/types'; +import { IMonacoModelContentChangeEvent } from '../../react-common/monacoHelpers'; // See https://react-redux.js.org/using-react-redux/connect-mapdispatch#defining-mapdispatchtoprops-as-an-object export const actionCreators = { @@ -80,15 +80,9 @@ export const actionCreators = { redo: (): CommonAction => ({ type: CommonActionType.REDO }), arrowUp: (cellId: string, code: string): CommonAction => ({ type: CommonActionType.ARROW_UP, payload: { cellId, code } }), arrowDown: (cellId: string, code: string): CommonAction => ({ type: CommonActionType.ARROW_DOWN, payload: { cellId, code } }), - editCell: ( - cellId: string, - modelId: string, - changes: monacoEditor.editor.IModelContentChange[], - reverse: monacoEditor.editor.IModelContentChange[], - code: string - ): CommonAction => ({ + editCell: (cellId: string, e: IMonacoModelContentChangeEvent): CommonAction => ({ type: CommonActionType.EDIT_CELL, - payload: { cellId, modelId, changes, reverse, id: cellId, code } + payload: { cellId, version: e.versionId, modelId: e.model.id, forward: e.forward, reverse: e.reverse, id: cellId, code: e.model.getValue() } }), linkClick: (href: string): CommonAction => ({ type: CommonActionType.LINK_CLICK, payload: { href } }), showPlot: (imageHtml: string): CommonAction => ({ type: CommonActionType.SHOW_PLOT, payload: { imageHtml } }), diff --git a/src/datascience-ui/native-editor/redux/reducers/creation.ts b/src/datascience-ui/native-editor/redux/reducers/creation.ts index 8caa8b372b4b..d8e1263fd880 100644 --- a/src/datascience-ui/native-editor/redux/reducers/creation.ts +++ b/src/datascience-ui/native-editor/redux/reducers/creation.ts @@ -4,7 +4,7 @@ import * as uuid from 'uuid/v4'; import { noop } from '../../../../client/common/utils/misc'; -import { ICellContentChange, ILoadAllCells, NotebookModelChange } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; +import { IEditorContentChange, ILoadAllCells, NotebookModelChange } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; import { ICell, IDataScienceExtraSettings } from '../../../../client/datascience/types'; import { createCellVM, createEmptyCell, CursorPos, extractInputText, ICellViewModel, IMainState } from '../../../interactive-common/mainState'; import { Helpers } from '../../../interactive-common/redux/reducers/helpers'; @@ -152,17 +152,19 @@ export namespace Creation { }; } - export function editCell(arg: NativeEditorReducerArg<{ id: string; changes: ICellContentChange[] }>): IMainState { + export function applyCellEdit(arg: NativeEditorReducerArg<{ id: string; changes: IEditorContentChange[] }>): IMainState { const index = arg.prevState.cellVMs.findIndex(c => c.cell.id === arg.payload.id); if (index) { const newVM = { ...arg.prevState.cellVMs[index] }; arg.payload.changes.forEach(c => { - const source = newVM.inputBlockText; + const source = newVM.uncommittedText ? newVM.uncommittedText : newVM.inputBlockText; const before = source.slice(0, c.rangeOffset); const after = source.slice(c.rangeOffset + c.rangeLength); - newVM.inputBlockText = `${before}${c.text}${after}`; + newVM.uncommittedText = `${before}${c.text}${after}`; }); - newVM.cell.data.source = newVM.inputBlockText; + newVM.codeVersion = newVM.codeVersion ? newVM.codeVersion + 1 : 1; + newVM.inputBlockText = newVM.cell.data.source = newVM.uncommittedText!; + newVM.cursorPos = arg.payload.changes[0].position; const newVMs = [...arg.prevState.cellVMs]; newVMs[index] = Helpers.asCellViewModel(newVM); return { @@ -262,7 +264,7 @@ export namespace Creation { case 'clear': return loadAllCells({ ...disabledQueueArg, payload: { cells: arg.payload.oldCells } }); case 'edit': - return editCell({ ...disabledQueueArg, payload: { id: arg.payload.id, changes: arg.payload.reverse } }); + return applyCellEdit({ ...disabledQueueArg, payload: { id: arg.payload.id, changes: arg.payload.reverse } }); case 'insert': return deleteCell({ ...disabledQueueArg, payload: { cellId: arg.payload.cell.id } }); case 'remove': @@ -288,7 +290,7 @@ export namespace Creation { case 'clear': return Execution.clearAllOutputs(disabledQueueArg); case 'edit': - return editCell({ ...disabledQueueArg, payload: { id: arg.payload.id, changes: arg.payload.forward } }); + return applyCellEdit({ ...disabledQueueArg, payload: { id: arg.payload.id, changes: arg.payload.forward } }); case 'insert': return insertAbove({ ...disabledQueueArg, payload: { newCellId: arg.payload.cell.id, cellId: arg.payload.codeCellAboveId } }); case 'remove': diff --git a/src/datascience-ui/native-editor/redux/reducers/effects.ts b/src/datascience-ui/native-editor/redux/reducers/effects.ts index 65d35324431a..22de558ca36d 100644 --- a/src/datascience-ui/native-editor/redux/reducers/effects.ts +++ b/src/datascience-ui/native-editor/redux/reducers/effects.ts @@ -24,7 +24,7 @@ export namespace Effects { if (removeFocusIndex >= 0) { const oldFocusCell = prevState.cellVMs[removeFocusIndex]; - const oldCode = oldFocusCell.uncomittedText || oldFocusCell.inputBlockText; + const oldCode = oldFocusCell.uncommittedText || oldFocusCell.inputBlockText; prevState = unfocusCell({ ...arg, prevState, payload: { cellId: prevState.cellVMs[removeFocusIndex].cell.id, code: oldCode } }); prevState = deselectCell({ ...arg, prevState, payload: { cellId: prevState.cellVMs[removeFocusIndex].cell.id } }); } @@ -140,7 +140,7 @@ export namespace Effects { if (removeFocusIndex >= 0) { const oldFocusCell = prevState.cellVMs[removeFocusIndex]; - const oldCode = oldFocusCell.uncomittedText || oldFocusCell.inputBlockText; + const oldCode = oldFocusCell.uncommittedText || oldFocusCell.inputBlockText; prevState = unfocusCell({ ...arg, prevState, payload: { cellId: prevState.cellVMs[removeFocusIndex].cell.id, code: oldCode } }); prevState = deselectCell({ ...arg, prevState, payload: { cellId: prevState.cellVMs[removeFocusIndex].cell.id } }); } diff --git a/src/datascience-ui/react-common/monacoEditor.tsx b/src/datascience-ui/react-common/monacoEditor.tsx index b933362ce3e8..533804a25e0a 100644 --- a/src/datascience-ui/react-common/monacoEditor.tsx +++ b/src/datascience-ui/react-common/monacoEditor.tsx @@ -16,7 +16,9 @@ const debounce = require('lodash/debounce') as typeof import('lodash/debounce'); // tslint:disable-next-line:no-require-imports no-var-requires const throttle = require('lodash/throttle') as typeof import('lodash/throttle'); +import { CursorPos } from '../interactive-common/mainState'; import './monacoEditor.css'; +import { generateChangeEvent, IMonacoModelContentChangeEvent } from './monacoHelpers'; const LINE_HEIGHT = 18; @@ -37,12 +39,16 @@ enum WidgetCSSSelector { export interface IMonacoEditorProps { language: string; value: string; + previousValue: string | undefined; theme?: string; outermostParentClass: string; options: monacoEditor.editor.IEditorConstructionOptions; testMode?: boolean; forceBackground?: string; measureWidthClassName?: string; + version: number; + cursorPos: CursorPos | monacoEditor.IPosition; + modelChanged(e: IMonacoModelContentChangeEvent): void; editorMounted(editor: monacoEditor.editor.IStandaloneCodeEditor): void; openLink(uri: monacoEditor.Uri): void; } @@ -76,6 +82,7 @@ export class MonacoEditor extends React.Component { + // If not skipping notifications, send an event + if (!this.skipNotifications && this.state.model && this.state.editor) { + this.props.modelChanged(generateChangeEvent(e, this.state.editor, this.state.model, this.props.previousValue ? this.props.previousValue : this.props.value)); + } + }; + private getCurrentVisibleLinePosOrIndex(pickResult: (pos: number, index: number) => number): number | undefined { // Convert the current cursor into a top and use that to find which visible // line it is in. diff --git a/src/datascience-ui/react-common/monacoHelpers.ts b/src/datascience-ui/react-common/monacoHelpers.ts new file mode 100644 index 000000000000..e75d7c458337 --- /dev/null +++ b/src/datascience-ui/react-common/monacoHelpers.ts @@ -0,0 +1,98 @@ +import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api'; +import { IEditorContentChange } from '../../client/datascience/interactive-common/interactiveWindowTypes'; + +export interface IMonacoModelContentChangeEvent { + // Changes to apply + readonly forward: IEditorContentChange[]; + // Change to undo the apply + readonly reverse: IEditorContentChange[]; + readonly eol: string; + readonly versionId: number; + readonly isUndoing: boolean; + readonly isRedoing: boolean; + readonly isFlush: boolean; + readonly model: monacoEditor.editor.ITextModel; +} + +function getValueInRange(text: string, r: monacoEditor.IRange): string { + // Compute start and end offset using line and column data + let startOffset = -1; + let endOffset = -1; + let line = 1; + let col = 1; + + // Go forwards through the text searching for matching lines + for (let pos = 0; pos <= text.length && (startOffset < 0 || endOffset < 0); pos += 1) { + if (line === r.startLineNumber && col === r.startColumn) { + startOffset = pos; + } else if (line === r.endLineNumber && col === r.endColumn) { + endOffset = pos; + } + if (pos < text.length) { + if (text[pos] === '\n') { + line += 1; + col = 1; + } else { + col += 1; + } + } + } + + if (startOffset && endOffset > 0) { + return text.slice(startOffset, endOffset); + } + + return ''; +} + +function generateReverseChange(oldModelValue: string, model: monacoEditor.editor.ITextModel, c: monacoEditor.editor.IModelContentChange): monacoEditor.editor.IModelContentChange { + const oldStart = model.getPositionAt(c.rangeOffset); + const oldEnd = model.getPositionAt(c.rangeOffset + c.rangeLength); + const oldText = getValueInRange(oldModelValue, c.range); + const oldRange: monacoEditor.IRange = { + startColumn: oldStart.column, + startLineNumber: oldStart.lineNumber, + endColumn: oldEnd.column, + endLineNumber: oldEnd.lineNumber + }; + return { + rangeLength: c.text.length, + rangeOffset: c.rangeOffset, + text: oldText ? oldText : '', + range: oldRange + }; +} + +export function generateChangeEvent( + ev: monacoEditor.editor.IModelContentChangedEvent, + _e: monacoEditor.editor.ICodeEditor, + m: monacoEditor.editor.ITextModel, + oldText: string +): IMonacoModelContentChangeEvent { + // Figure out the end position from the offset plus the length of the text we added + const currentOffset = ev.changes[ev.changes.length - 1].rangeOffset + ev.changes[ev.changes.length - 1].text.length; + const currentPosition = m.getPositionAt(currentOffset); + + // Create the reverse changes + const reverseChanges = ev.changes.map(generateReverseChange.bind(undefined, oldText, m)).reverse(); + + // Figure out the old position by using the first offset + const oldOffset = ev.changes[0].rangeOffset; + const oldPosition = m.getPositionAt(oldOffset); + + // Combine position and change to create result + return { + forward: ev.changes.map(c => { + return { ...c, position: currentPosition! }; + }), + reverse: reverseChanges.map(r => { + return { ...r, position: oldPosition! }; + }), + eol: ev.eol, + isFlush: ev.isFlush, + isUndoing: ev.isUndoing, + isRedoing: ev.isRedoing, + versionId: m.getVersionId(), + model: m + }; +} diff --git a/src/test/datascience/intellisense.unit.test.ts b/src/test/datascience/intellisense.unit.test.ts index 71cd5da7415c..2e4039df2fd6 100644 --- a/src/test/datascience/intellisense.unit.test.ts +++ b/src/test/datascience/intellisense.unit.test.ts @@ -2,7 +2,6 @@ // Licensed under the MIT License. 'use strict'; import { expect } from 'chai'; -import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api'; import * as TypeMoq from 'typemoq'; import * as uuid from 'uuid/v4'; @@ -12,7 +11,7 @@ import { IFileSystem } from '../../client/common/platform/types'; import { IConfigurationService } from '../../client/common/types'; import { Identifiers } from '../../client/datascience/constants'; import { IntellisenseProvider } from '../../client/datascience/interactive-common/intellisense/intellisenseProvider'; -import { IInteractiveWindowMapping, InteractiveWindowMessages } from '../../client/datascience/interactive-common/interactiveWindowTypes'; +import { IEditorContentChange, IInteractiveWindowMapping, InteractiveWindowMessages } from '../../client/datascience/interactive-common/interactiveWindowTypes'; import { ICell, IInteractiveWindowListener, IInteractiveWindowProvider, IJupyterExecution } from '../../client/datascience/types'; import { IInterpreterService } from '../../client/interpreter/contracts'; import { createEmptyCell, generateTestCells } from '../../datascience-ui/interactive-common/mainState'; @@ -99,7 +98,7 @@ suite('DataScience Intellisense Unit Tests', () => { function updateCell(newCode: string, oldCode: string, id: string): Promise { const oldSplit = oldCode.split('\n'); - const change: monacoEditor.editor.IModelContentChange = { + const change: IEditorContentChange = { range: { startLineNumber: 1, startColumn: 1, @@ -108,7 +107,11 @@ suite('DataScience Intellisense Unit Tests', () => { }, rangeOffset: 0, rangeLength: oldCode.length, - text: newCode + text: newCode, + position: { + column: 1, + lineNumber: 1 + } }; return sendMessage(InteractiveWindowMessages.UpdateModel, { source: 'user', @@ -125,7 +128,7 @@ suite('DataScience Intellisense Unit Tests', () => { if (!line || !pos) { throw new Error('Invalid line or position data'); } - const change: monacoEditor.editor.IModelContentChange = { + const change: IEditorContentChange = { range: { startLineNumber: line, startColumn: pos, @@ -134,7 +137,11 @@ suite('DataScience Intellisense Unit Tests', () => { }, rangeOffset: offset, rangeLength: 0, - text: code + text: code, + position: { + column: 1, + lineNumber: 1 + } }; return sendMessage(InteractiveWindowMessages.UpdateModel, { source: 'user', @@ -151,7 +158,7 @@ suite('DataScience Intellisense Unit Tests', () => { if (!line || !startPos || !endPos) { throw new Error('Invalid line or position data'); } - const change: monacoEditor.editor.IModelContentChange = { + const change: IEditorContentChange = { range: { startLineNumber: line, startColumn: startPos, @@ -160,7 +167,11 @@ suite('DataScience Intellisense Unit Tests', () => { }, rangeOffset: startPos, rangeLength: length, - text: '' + text: '', + position: { + column: 1, + lineNumber: 1 + } }; return sendMessage(InteractiveWindowMessages.UpdateModel, { source: 'user', diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index 3f41346ad209..bf52e066bced 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -22,7 +22,7 @@ import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService, ICryptoUtils, IDisposable, IExtensionContext } from '../../../client/common/types'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { Commands } from '../../../client/datascience/constants'; -import { ICellContentChange, InteractiveWindowMessages } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; +import { IEditorContentChange, InteractiveWindowMessages } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; import { NativeEditorStorage } from '../../../client/datascience/interactive-ipynb/nativeEditorStorage'; import { JupyterExecutionFactory } from '../../../client/datascience/jupyter/jupyterExecutionFactory'; import { ICell, IJupyterExecution, INotebookServerOptions } from '../../../client/datascience/types'; @@ -369,7 +369,7 @@ suite('Data Science - Native Editor Storage', () => { }); } - function editCell(changes: ICellContentChange[], cell: ICell, _newCode: string) { + function editCell(changes: IEditorContentChange[], cell: ICell, _newCode: string) { return executeCommand(Commands.NotebookModel_Update, baseUri, { source: 'user', kind: 'edit', @@ -439,7 +439,11 @@ suite('Data Science - Native Editor Storage', () => { }, rangeOffset: 4, rangeLength: 0, - text: 'a' + text: 'a', + position: { + lineNumber: 1, + column: 1 + } } ], storage.cells[1], From 1fcd271c5b7205d23f2742e3166089281ef7d622 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 4 Feb 2020 14:52:10 -0800 Subject: [PATCH 07/23] Fix completions for interactive --- .../interactive-common/redux/reducers/transfer.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts index c54aa15f0535..ae8c08d51f6b 100644 --- a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts +++ b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts @@ -9,6 +9,7 @@ import { extractInputText, IMainState } from '../../mainState'; import { createPostableAction } from '../postOffice'; import { Helpers } from './helpers'; import { CommonReducerArg, ICellAction, IEditCellAction, ILinkClickAction, ISendCommandAction, IShowDataViewerAction, IShowPlotAction } from './types'; +import { Identifiers } from '../../../../client/datascience/constants'; // These are all reducers that don't actually change state. They merely dispatch a message to the other side. export namespace Transfer { @@ -169,7 +170,7 @@ export namespace Transfer { } export function editCell(arg: CommonReducerArg): IMainState { - const cellVM = arg.prevState.cellVMs.find(c => c.cell.id === arg.payload.cellId); + const cellVM = arg.payload.cellId === Identifiers.EditCellId ? arg.prevState.editCellVM : arg.prevState.cellVMs.find(c => c.cell.id === arg.payload.cellId); if (cellVM) { // Tell the underlying model on the extension side postModelEdit(arg, arg.payload.forward, arg.payload.reverse, cellVM.cell.id); From 43981337acd19995af4143e8f48a8e5324e4340a Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 5 Feb 2020 09:05:19 -0800 Subject: [PATCH 08/23] Fix unit tests --- .vscode/launch.json | 2 +- .../intellisense/intellisenseProvider.ts | 65 ++++--- .../interactiveWindowTypes.ts | 6 + .../history-react/redux/reducers/creation.ts | 5 +- .../redux/reducers/transfer.ts | 11 +- .../native-editor/redux/reducers/creation.ts | 2 +- .../react-common/monacoEditor.tsx | 2 +- .../react-common/monacoHelpers.ts | 25 +-- .../datascience/intellisense.unit.test.ts | 183 +++++++++++++----- .../nativeEditorStorage.unit.test.ts | 2 +- 10 files changed, 198 insertions(+), 105 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index e972d37c7bad..721b8c645555 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -187,7 +187,7 @@ "--ui=tdd", "--recursive", "--colors", - //"--grep", "", + "--grep", "Edit/delete cells", "--timeout=300000" ], "outFiles": [ diff --git a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts index 830309df153f..6c52b6e0daa6 100644 --- a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts +++ b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts @@ -48,6 +48,9 @@ import { IntellisenseDocument } from './intellisenseDocument'; // tslint:disable:no-any @injectable() export class IntellisenseProvider implements IInteractiveWindowListener { + public get postMessage(): Event<{ message: string; payload: any }> { + return this.postEmitter.event; + } private documentPromise: Deferred | undefined; private temporaryFile: TemporaryFile | undefined; private postEmitter: EventEmitter<{ message: string; payload: any }> = new EventEmitter<{ message: string; payload: any }>(); @@ -78,10 +81,6 @@ export class IntellisenseProvider implements IInteractiveWindowListener { } } - public get postMessage(): Event<{ message: string; payload: any }> { - return this.postEmitter.event; - } - public onMessage(message: string, payload?: any) { switch (message) { case InteractiveWindowMessages.CancelCompletionItemsRequest: @@ -126,6 +125,32 @@ export class IntellisenseProvider implements IInteractiveWindowListener { } } + public getDocument(resource?: Uri): Promise { + if (!this.documentPromise) { + this.documentPromise = createDeferred(); + + // Create our dummy document. Compute a file path for it. + if (this.workspaceService.rootPath || resource) { + const dir = resource ? path.dirname(resource.fsPath) : this.workspaceService.rootPath!; + const dummyFilePath = path.join(dir, HiddenFileFormatString.format(uuid().replace(/-/g, ''))); + this.documentPromise.resolve(new IntellisenseDocument(dummyFilePath)); + } else { + this.fileSystem + .createTemporaryFile('.py') + .then(t => { + this.temporaryFile = t; + const dummyFilePath = this.temporaryFile.filePath; + this.documentPromise!.resolve(new IntellisenseDocument(dummyFilePath)); + }) + .catch(e => { + this.documentPromise!.reject(e); + }); + } + } + + return this.documentPromise.promise; + } + protected async getLanguageServer(): Promise { // Resource should be our potential resource if its set. Otherwise workspace root const resource = this.potentialResource || (this.workspaceService.rootPath ? Uri.parse(this.workspaceService.rootPath) : undefined); @@ -169,32 +194,6 @@ export class IntellisenseProvider implements IInteractiveWindowListener { return this.languageServer; } - protected getDocument(resource?: Uri): Promise { - if (!this.documentPromise) { - this.documentPromise = createDeferred(); - - // Create our dummy document. Compute a file path for it. - if (this.workspaceService.rootPath || resource) { - const dir = resource ? path.dirname(resource.fsPath) : this.workspaceService.rootPath!; - const dummyFilePath = path.join(dir, HiddenFileFormatString.format(uuid().replace(/-/g, ''))); - this.documentPromise.resolve(new IntellisenseDocument(dummyFilePath)); - } else { - this.fileSystem - .createTemporaryFile('.py') - .then(t => { - this.temporaryFile = t; - const dummyFilePath = this.temporaryFile.filePath; - this.documentPromise!.resolve(new IntellisenseDocument(dummyFilePath)); - }) - .catch(e => { - this.documentPromise!.reject(e); - }); - } - } - - return this.documentPromise.promise; - } - protected async provideCompletionItems( position: monacoEditor.Position, context: monacoEditor.languages.CompletionContext, @@ -525,6 +524,7 @@ export class IntellisenseProvider implements IInteractiveWindowListener { case 'edit': changes = document.editCell(request.reverse, request.id); break; + case 'add': case 'insert': changes = document.remove(request.cell.id); break; @@ -560,8 +560,11 @@ export class IntellisenseProvider implements IInteractiveWindowListener { case 'edit': changes = document.editCell(request.forward, request.id); break; + case 'add': + changes = document.addCell(request.fullText, request.currentText, request.cell.id); + break; case 'insert': - changes = document.insertCell(request.cell.id, concatMultilineStringInput(request.cell.data.source), request.codeCellAboveId); + changes = document.insertCell(request.cell.id, concatMultilineStringInput(request.cell.data.source), request.codeCellAboveId || request.index); break; case 'modify': // This one can be ignored. it's only used for updating cell finished state. diff --git a/src/client/datascience/interactive-common/interactiveWindowTypes.ts b/src/client/datascience/interactive-common/interactiveWindowTypes.ts index 6c360e81237a..f0b38d24a0de 100644 --- a/src/client/datascience/interactive-common/interactiveWindowTypes.ts +++ b/src/client/datascience/interactive-common/interactiveWindowTypes.ts @@ -330,6 +330,11 @@ export interface INotebookModelInsertChange extends INotebookModelChange { cell: ICell; index: number; codeCellAboveId?: string; +} + +export interface INotebookModelAddChange extends INotebookModelChange { + kind: 'add'; + cell: ICell; fullText: string; currentText: string; } @@ -413,6 +418,7 @@ export type NotebookModelChange = | INotebookModelSwapChange | INotebookModelRemoveChange | INotebookModelInsertChange + | INotebookModelAddChange | INotebookModelEditChange | INotebookModelVersionChange | INotebookModelFileChange; diff --git a/src/datascience-ui/history-react/redux/reducers/creation.ts b/src/datascience-ui/history-react/redux/reducers/creation.ts index 9de129f774a7..90e57b9a30d9 100644 --- a/src/datascience-ui/history-react/redux/reducers/creation.ts +++ b/src/datascience-ui/history-react/redux/reducers/creation.ts @@ -88,13 +88,12 @@ export namespace Creation { arg.queueAction( createPostableAction(InteractiveWindowMessages.UpdateModel, { source: 'user', - kind: 'insert', + kind: 'add', oldDirty: arg.prevState.dirty, newDirty: true, cell: cellVM.cell, fullText: extractInputText(cellVM, result.settings), - currentText: cellVM.inputBlockText, - index: result.cellVMs.findIndex(c => c.cell.id === arg.payload.id) - 1 + currentText: cellVM.inputBlockText }) ); } diff --git a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts index ae8c08d51f6b..78afa6c30572 100644 --- a/src/datascience-ui/interactive-common/redux/reducers/transfer.ts +++ b/src/datascience-ui/interactive-common/redux/reducers/transfer.ts @@ -1,15 +1,14 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. 'use strict'; +import { Identifiers } from '../../../../client/datascience/constants'; import { IEditorContentChange, InteractiveWindowMessages, NotebookModelChange } from '../../../../client/datascience/interactive-common/interactiveWindowTypes'; import { CssMessages } from '../../../../client/datascience/messages'; import { ICell } from '../../../../client/datascience/types'; -import { concatMultilineStringInput } from '../../../common'; import { extractInputText, IMainState } from '../../mainState'; import { createPostableAction } from '../postOffice'; import { Helpers } from './helpers'; import { CommonReducerArg, ICellAction, IEditCellAction, ILinkClickAction, ISendCommandAction, IShowDataViewerAction, IShowPlotAction } from './types'; -import { Identifiers } from '../../../../client/datascience/constants'; // These are all reducers that don't actually change state. They merely dispatch a message to the other side. export namespace Transfer { @@ -108,9 +107,7 @@ export namespace Transfer { }); } - export function postModelInsert(arg: CommonReducerArg, index: number, cell: ICell, codeCellAboveId?: string, fullText?: string, currentText?: string) { - const trueFullText = fullText === undefined ? concatMultilineStringInput(cell.data.source) : fullText; - const trueCurrentText = currentText === undefined ? trueFullText : currentText; + export function postModelInsert(arg: CommonReducerArg, index: number, cell: ICell, codeCellAboveId?: string) { postModelUpdate(arg, { source: 'user', kind: 'insert', @@ -118,9 +115,7 @@ export namespace Transfer { oldDirty: arg.prevState.dirty, index, cell, - codeCellAboveId, - fullText: trueFullText, - currentText: trueCurrentText + codeCellAboveId }); } diff --git a/src/datascience-ui/native-editor/redux/reducers/creation.ts b/src/datascience-ui/native-editor/redux/reducers/creation.ts index d8e1263fd880..965136afb043 100644 --- a/src/datascience-ui/native-editor/redux/reducers/creation.ts +++ b/src/datascience-ui/native-editor/redux/reducers/creation.ts @@ -90,7 +90,7 @@ export namespace Creation { }; // Send a messsage that we inserted a cell - Transfer.postModelInsert(arg, -1, newVM.cell, findFirstCodeCellAbove(newList, position)); + Transfer.postModelInsert(arg, arg.prevState.cellVMs.length, newVM.cell, findFirstCodeCellAbove(newList, position)); // Queue up an action to set focus to the cell we're inserting setTimeout(() => { diff --git a/src/datascience-ui/react-common/monacoEditor.tsx b/src/datascience-ui/react-common/monacoEditor.tsx index 533804a25e0a..91eb10f3be6b 100644 --- a/src/datascience-ui/react-common/monacoEditor.tsx +++ b/src/datascience-ui/react-common/monacoEditor.tsx @@ -420,7 +420,7 @@ export class MonacoEditor extends React.Component { // If not skipping notifications, send an event if (!this.skipNotifications && this.state.model && this.state.editor) { - this.props.modelChanged(generateChangeEvent(e, this.state.editor, this.state.model, this.props.previousValue ? this.props.previousValue : this.props.value)); + this.props.modelChanged(generateChangeEvent(e, this.state.model, this.props.previousValue ? this.props.previousValue : this.props.value)); } }; diff --git a/src/datascience-ui/react-common/monacoHelpers.ts b/src/datascience-ui/react-common/monacoHelpers.ts index e75d7c458337..aae8890ff4b0 100644 --- a/src/datascience-ui/react-common/monacoHelpers.ts +++ b/src/datascience-ui/react-common/monacoHelpers.ts @@ -1,5 +1,13 @@ import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api'; -import { IEditorContentChange } from '../../client/datascience/interactive-common/interactiveWindowTypes'; +import { IEditorContentChange, IEditorPosition, IEditorRange } from '../../client/datascience/interactive-common/interactiveWindowTypes'; + +export interface IMonacoTextModel { + readonly id: string; + getValue(): string; + getValueLength(): number; + getVersionId(): number; + getPositionAt(offset: number): IEditorPosition; +} export interface IMonacoModelContentChangeEvent { // Changes to apply @@ -11,10 +19,10 @@ export interface IMonacoModelContentChangeEvent { readonly isUndoing: boolean; readonly isRedoing: boolean; readonly isFlush: boolean; - readonly model: monacoEditor.editor.ITextModel; + readonly model: IMonacoTextModel; } -function getValueInRange(text: string, r: monacoEditor.IRange): string { +function getValueInRange(text: string, r: IEditorRange): string { // Compute start and end offset using line and column data let startOffset = -1; let endOffset = -1; @@ -38,14 +46,14 @@ function getValueInRange(text: string, r: monacoEditor.IRange): string { } } - if (startOffset && endOffset > 0) { + if (startOffset >= 0 && endOffset >= 0) { return text.slice(startOffset, endOffset); } return ''; } -function generateReverseChange(oldModelValue: string, model: monacoEditor.editor.ITextModel, c: monacoEditor.editor.IModelContentChange): monacoEditor.editor.IModelContentChange { +export function generateReverseChange(oldModelValue: string, model: IMonacoTextModel, c: monacoEditor.editor.IModelContentChange): monacoEditor.editor.IModelContentChange { const oldStart = model.getPositionAt(c.rangeOffset); const oldEnd = model.getPositionAt(c.rangeOffset + c.rangeLength); const oldText = getValueInRange(oldModelValue, c.range); @@ -63,12 +71,7 @@ function generateReverseChange(oldModelValue: string, model: monacoEditor.editor }; } -export function generateChangeEvent( - ev: monacoEditor.editor.IModelContentChangedEvent, - _e: monacoEditor.editor.ICodeEditor, - m: monacoEditor.editor.ITextModel, - oldText: string -): IMonacoModelContentChangeEvent { +export function generateChangeEvent(ev: monacoEditor.editor.IModelContentChangedEvent, m: IMonacoTextModel, oldText: string): IMonacoModelContentChangeEvent { // Figure out the end position from the offset plus the length of the text we added const currentOffset = ev.changes[ev.changes.length - 1].rangeOffset + ev.changes[ev.changes.length - 1].text.length; const currentPosition = m.getPositionAt(currentOffset); diff --git a/src/test/datascience/intellisense.unit.test.ts b/src/test/datascience/intellisense.unit.test.ts index 2e4039df2fd6..20f8473895f0 100644 --- a/src/test/datascience/intellisense.unit.test.ts +++ b/src/test/datascience/intellisense.unit.test.ts @@ -10,11 +10,13 @@ import { PythonSettings } from '../../client/common/configSettings'; import { IFileSystem } from '../../client/common/platform/types'; import { IConfigurationService } from '../../client/common/types'; import { Identifiers } from '../../client/datascience/constants'; +import { IntellisenseDocument } from '../../client/datascience/interactive-common/intellisense/intellisenseDocument'; import { IntellisenseProvider } from '../../client/datascience/interactive-common/intellisense/intellisenseProvider'; import { IEditorContentChange, IInteractiveWindowMapping, InteractiveWindowMessages } from '../../client/datascience/interactive-common/interactiveWindowTypes'; -import { ICell, IInteractiveWindowListener, IInteractiveWindowProvider, IJupyterExecution } from '../../client/datascience/types'; +import { ICell, IInteractiveWindowProvider, IJupyterExecution } from '../../client/datascience/types'; import { IInterpreterService } from '../../client/interpreter/contracts'; import { createEmptyCell, generateTestCells } from '../../datascience-ui/interactive-common/mainState'; +import { generateReverseChange, IMonacoTextModel } from '../../datascience-ui/react-common/monacoHelpers'; import { MockAutoSelectionService } from '../mocks/autoSelector'; import { MockLanguageServerCache } from './mockLanguageServerCache'; @@ -34,8 +36,9 @@ df `; // tslint:disable-next-line: max-func-body-length -suite('DataScience Intellisense Unit Tests', () => { - let intellisenseProvider: IInteractiveWindowListener; +suite('Data Science Intellisense Unit Tests', () => { + let intellisenseProvider: IntellisenseProvider; + let intellisenseDocument: IntellisenseDocument; let interpreterService: TypeMoq.IMock; let languageServerCache: MockLanguageServerCache; let workspaceService: TypeMoq.IMock; @@ -50,7 +53,7 @@ suite('DataScience Intellisense Unit Tests', () => { } })(undefined, new MockAutoSelectionService()); - setup(() => { + setup(async () => { languageServerCache = new MockLanguageServerCache(); interpreterService = TypeMoq.Mock.ofType(); workspaceService = TypeMoq.Mock.ofType(); @@ -71,6 +74,7 @@ suite('DataScience Intellisense Unit Tests', () => { interpreterService.object, languageServerCache ); + intellisenseDocument = await intellisenseProvider.getDocument(); }); function sendMessage(type: T, payload?: M[T]): Promise { @@ -84,19 +88,45 @@ suite('DataScience Intellisense Unit Tests', () => { cell.data.source = code; const result = sendMessage(InteractiveWindowMessages.UpdateModel, { source: 'user', - kind: 'insert', + kind: 'add', oldDirty: false, newDirty: true, fullText: code, currentText: code, - index: cells.length - 1, cell }); cells.splice(cells.length - 1, 0, cell); return result; } - function updateCell(newCode: string, oldCode: string, id: string): Promise { + function generateModel(doc: IntellisenseDocument): IMonacoTextModel { + const code = doc.getText(); + return { + id: '1', + getValue: () => code, + getValueLength: () => code.length, + getVersionId: () => doc.version, + getPositionAt: (o: number) => { + const p = doc.positionAt(o); + return { lineNumber: p.line + 1, column: p.character + 1 }; + } + }; + } + + function sendUpdate(id: string, oldText: string, doc: IntellisenseDocument, change: IEditorContentChange, source: 'user' | 'undo' | 'redo' = 'user') { + const reverse = { ...generateReverseChange(oldText, generateModel(doc), change), position: { lineNumber: 1, column: 1 } }; + return sendMessage(InteractiveWindowMessages.UpdateModel, { + source, + kind: 'edit', + oldDirty: false, + newDirty: true, + forward: [change], + reverse: [reverse], + id + }); + } + + function updateCell(newCode: string, oldCode: string, id: string, source: 'user' | 'undo' | 'redo' = 'user'): Promise { const oldSplit = oldCode.split('\n'); const change: IEditorContentChange = { range: { @@ -113,15 +143,7 @@ suite('DataScience Intellisense Unit Tests', () => { lineNumber: 1 } }; - return sendMessage(InteractiveWindowMessages.UpdateModel, { - source: 'user', - kind: 'edit', - oldDirty: false, - newDirty: true, - forward: [change], - reverse: [change], - id - }); + return sendUpdate(id, oldCode, getDocument(), change, source); } function addCode(code: string, line: number, pos: number, offset: number): Promise { @@ -173,42 +195,44 @@ suite('DataScience Intellisense Unit Tests', () => { lineNumber: 1 } }; - return sendMessage(InteractiveWindowMessages.UpdateModel, { - source: 'user', - kind: 'edit', - oldDirty: false, - newDirty: true, - forward: [change], - reverse: [change], - id: cells[cells.length - 1].id - }); + return sendUpdate(cells[cells.length - 1].id, '', getDocument(), change); } - function removeCell(id: string): Promise { - return sendMessage(InteractiveWindowMessages.UpdateModel, { - source: 'user', - kind: 'remove', - oldDirty: false, - newDirty: true, - cell: cells.find(c => c.id === id)!, - index: cells.findIndex(c => c.id === id) - }); + async function removeCell(cell: ICell | undefined, oldIndex: number = -1, source: 'user' | 'undo' | 'redo' = 'user'): Promise { + if (cell) { + let index = cells.findIndex(c => c.id === cell.id); + if (index < 0) { + index = oldIndex; + } else { + cells.splice(index, 1); + } + await sendMessage(InteractiveWindowMessages.UpdateModel, { + source, + kind: 'remove', + oldDirty: false, + newDirty: true, + cell, + index + }); + return index; + } + return -1; } - function removeAllCells(): Promise { + function removeAllCells(source: 'user' | 'undo' | 'redo' = 'user', oldCells: ICell[] = cells): Promise { return sendMessage(InteractiveWindowMessages.UpdateModel, { - source: 'user', + source, kind: 'remove_all', oldDirty: false, newDirty: true, - oldCells: cells, + oldCells, newCellId: uuid() }); } - function swapCells(id1: string, id2: string): Promise { + function swapCells(id1: string, id2: string, source: 'user' | 'undo' | 'redo' = 'user'): Promise { return sendMessage(InteractiveWindowMessages.UpdateModel, { - source: 'user', + source, kind: 'swap', oldDirty: false, newDirty: true, @@ -217,20 +241,23 @@ suite('DataScience Intellisense Unit Tests', () => { }); } - function insertCell(id: string, code: string, codeCellAbove?: string): Promise { + function insertCell(id: string, code: string, codeCellAbove?: string, source: 'user' | 'undo' | 'redo' = 'user', end?: boolean): Promise { const cell = createEmptyCell(id, null); cell.data.source = code; - const index = codeCellAbove ? cells.findIndex(c => c.id === codeCellAbove) : 0; + const index = codeCellAbove ? cells.findIndex(c => c.id === codeCellAbove) : end ? cells.length : 0; + if (source === 'undo') { + cells = cells.filter(c => c.id !== id); + } else { + cells.splice(index, 0, cell); + } return sendMessage(InteractiveWindowMessages.UpdateModel, { - source: 'user', + source, kind: 'insert', oldDirty: false, newDirty: true, codeCellAboveId: codeCellAbove, cell, - index, - fullText: code, - currentText: code + index }); } @@ -243,6 +270,10 @@ suite('DataScience Intellisense Unit Tests', () => { return languageServerCache.getMockServer().getDocumentContents(); } + function getDocument(): IntellisenseDocument { + return intellisenseDocument; + } + test('Add a single cell', async () => { await addCell('import sys\n\n', '1'); expect(getDocumentContents()).to.be.eq('import sys\n\n\n', 'Document not set'); @@ -363,13 +394,13 @@ suite('DataScience Intellisense Unit Tests', () => { await addCode('y', 1, 2, 1); await addCode('s', 1, 3, 2); expect(getDocumentContents()).to.be.eq('import sys\nsys', 'Document not set after edit'); - await removeCell('1'); + await removeCell(cells.find(c => c.id === '1')); expect(getDocumentContents()).to.be.eq('import sys\nsys', 'Removing a cell broken'); await addCell('import sys', '2'); expect(getDocumentContents()).to.be.eq('import sys\nimport sys\nsys', 'Adding a cell broken'); await addCell('import bar', '3'); expect(getDocumentContents()).to.be.eq('import sys\nimport sys\nimport bar\nsys', 'Adding a cell broken'); - await removeCell('1'); + await removeCell(cells.find(c => c.id === '1')); expect(getDocumentContents()).to.be.eq('import sys\nimport sys\nimport bar\nsys', 'Removing a cell broken'); }); @@ -463,7 +494,7 @@ df df `; expect(getDocumentContents()).to.be.eq(afterInsert2, 'Insert2 cell failed'); - await removeCell('7'); + await removeCell(cells.find(c => c.id === '7')); expect(getDocumentContents()).to.be.eq(afterInsert, 'Remove 2 cell failed'); await swapCells('0', '2'); const afterSwap = `foo @@ -482,4 +513,60 @@ df `; expect(getDocumentContents()).to.be.eq(afterSwap, 'Swap cell failed'); }); + + test('Edit and undo', async () => { + const loadable = [createEmptyCell('0', null), createEmptyCell('1', null)]; + loadable[0].data.source = 'a=1\na'; + loadable[1].data.source = 'b=2\nb'; + await loadAllCells(loadable); + const startContent = `a=1 +a +b=2 +b +`; + expect(getDocumentContents()).to.be.eq(startContent, 'Load all cells is failing'); + await swapCells('0', '1'); + const afterSwap = `b=2 +b +a=1 +a +`; + expect(getDocumentContents()).to.be.eq(afterSwap, 'Swap cell failed'); + await swapCells('0', '1', 'undo'); + expect(getDocumentContents()).to.be.eq(startContent, 'Swap cell undo failed'); + await updateCell('a=4\na', 'a=1\na', '0'); + const afterUpdate = `a=4 +a +b=2 +b +`; + expect(getDocumentContents()).to.be.eq(afterUpdate, 'Edit cell failed'); + await updateCell('a=4\na', 'a=1\na', '0', 'undo'); + expect(getDocumentContents()).to.be.eq(startContent, 'Edit undo cell failed'); + + const afterInsert = `a=1 +a +b=2 +b +c=5 +c +`; + await insertCell('2', 'c=5\nc', undefined, 'user', true); + expect(getDocumentContents()).to.be.eq(afterInsert, 'Insert cell failed'); + await insertCell('2', 'c=5\nc', undefined, 'undo', true); + expect(getDocumentContents()).to.be.eq(startContent, 'Insert cell update failed'); + const oldCells = [...cells]; + await removeAllCells(); + expect(getDocumentContents()).to.be.eq('', 'Remove all failed'); + await removeAllCells('undo', oldCells); + expect(getDocumentContents()).to.be.eq(startContent, 'Remove all undo failed'); + const cell = cells.find(c => c.id === '1'); + const oldIndex = await removeCell(cell); + const afterRemove = `a=1 +a +`; + expect(getDocumentContents()).to.be.eq(afterRemove, 'Remove failed'); + await removeCell(cell, oldIndex, 'undo'); + expect(getDocumentContents()).to.be.eq(startContent, 'Remove undo failed'); + }); }); diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index bf52e066bced..7e22a74bff45 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -460,7 +460,7 @@ suite('Data Science - Native Editor Storage', () => { expect(cells[0].id).to.be.match(/NotebookImport#1/); await deleteAllCells(); cells = storage.cells; - expect(cells).to.be.lengthOf(0); + expect(cells).to.be.lengthOf(1); }); test('Opening file with local storage but no global will still open with old contents', async () => { From 6a3366fe053036e2bd99c7ee2476fe012b531b58 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 5 Feb 2020 10:38:07 -0800 Subject: [PATCH 09/23] First working functional test of undo --- .../interactiveWindowTypes.ts | 4 +- .../interactive-common/redux/store.ts | 5 ++ .../datascience/mockCustomEditorService.ts | 47 ++++++++++++++++++- .../nativeEditor.functional.test.tsx | 31 +++++++++++- src/test/datascience/testHelpers.tsx | 4 ++ 5 files changed, 88 insertions(+), 3 deletions(-) diff --git a/src/client/datascience/interactive-common/interactiveWindowTypes.ts b/src/client/datascience/interactive-common/interactiveWindowTypes.ts index f0b38d24a0de..c5c1e8a1dc04 100644 --- a/src/client/datascience/interactive-common/interactiveWindowTypes.ts +++ b/src/client/datascience/interactive-common/interactiveWindowTypes.ts @@ -86,7 +86,8 @@ export enum InteractiveWindowMessages { SelectKernel = 'select_kernel', UpdateKernel = 'update_kernel', SelectJupyterServer = 'select_jupyter_server', - UpdateModel = 'update_model' + UpdateModel = 'update_model', + ReceivedUpdateModel = 'received_update_model' } export enum NativeCommandType { @@ -502,4 +503,5 @@ export class IInteractiveWindowMapping { public [InteractiveWindowMessages.ClearAllOutputs]: never | undefined; public [InteractiveWindowMessages.UpdateKernel]: IServerState | undefined; public [InteractiveWindowMessages.UpdateModel]: NotebookModelChange; + public [InteractiveWindowMessages.ReceivedUpdateModel]: never | undefined; } diff --git a/src/datascience-ui/interactive-common/redux/store.ts b/src/datascience-ui/interactive-common/redux/store.ts index cc7d2f8f021b..a3a77e41c3f1 100644 --- a/src/datascience-ui/interactive-common/redux/store.ts +++ b/src/datascience-ui/interactive-common/redux/store.ts @@ -142,6 +142,11 @@ function createTestMiddleware(): Redux.Middleware<{}, IStore> { sendMessage(InteractiveWindowMessages.VariablesComplete); } + // Indicate update from extension side + if (action.type && action.type === IncomingMessageActions.UPDATEMODEL) { + sendMessage(InteractiveWindowMessages.ReceivedUpdateModel); + } + // Special case for rendering complete const prevFinished = prevState.main.cellVMs.filter(c => c.cell.state === CellState.finished || c.cell.state === CellState.error).map(c => c.cell.id); const afterFinished = afterState.main.cellVMs.filter(c => c.cell.state === CellState.finished || c.cell.state === CellState.error).map(c => c.cell.id); diff --git a/src/test/datascience/mockCustomEditorService.ts b/src/test/datascience/mockCustomEditorService.ts index c6b1897d4d93..ccccacf0a4f3 100644 --- a/src/test/datascience/mockCustomEditorService.ts +++ b/src/test/datascience/mockCustomEditorService.ts @@ -10,8 +10,10 @@ import { INotebookEditor, INotebookEditorProvider } from '../../client/datascien export class MockCustomEditorService implements ICustomEditorService { private provider: WebviewCustomEditorProvider | undefined; private resolvedList = new Map>(); + private undoStack = new Map(); + private redoStack = new Map(); - constructor(disposableRegistry: IDisposableRegistry, commandManager: ICommandManager) { + constructor(private disposableRegistry: IDisposableRegistry, commandManager: ICommandManager) { disposableRegistry.push(commandManager.registerCommand('workbench.action.files.save', this.onFileSave.bind(this))); disposableRegistry.push(commandManager.registerCommand('workbench.action.files.saveAs', this.onFileSaveAs.bind(this))); } @@ -24,6 +26,11 @@ export class MockCustomEditorService implements ICustomEditorService { // tslint:disable-next-line: no-any ((this.provider as any) as INotebookEditorProvider).onDidCloseNotebookEditor(this.closedEditor.bind(this)); + // Listen for updates so we can keep an undo/redo stack + if (this.provider.editingDelegate) { + this.disposableRegistry.push(this.provider.editingDelegate.onEdit(this.onEditChange.bind(this))); + } + return { dispose: noop }; } public async openEditor(file: Uri): Promise { @@ -43,6 +50,35 @@ export class MockCustomEditorService implements ICustomEditorService { await resolved; } + public undo(file: Uri) { + this.popAndApply(file, this.undoStack, this.redoStack, e => { + const nativeProvider = (this.provider as unknown) as WebviewCustomEditorEditingDelegate; + nativeProvider.undoEdits(file, [e as NotebookModelChange]); + }); + } + + public redo(file: Uri) { + this.popAndApply(file, this.redoStack, this.undoStack, e => { + const nativeProvider = (this.provider as unknown) as WebviewCustomEditorEditingDelegate; + nativeProvider.applyEdits(file, [e as NotebookModelChange]); + }); + } + + private popAndApply(file: Uri, from: Map, to: Map, apply: (element: unknown) => void) { + const key = file.toString(); + const fromStack = from.get(key); + if (fromStack) { + const element = fromStack.pop(); + apply(element); + let toStack = to.get(key); + if (toStack === undefined) { + toStack = []; + to.set(key, toStack); + } + toStack.push(element); + } + } + private onFileSave(file: Uri) { const nativeProvider = (this.provider as unknown) as WebviewCustomEditorEditingDelegate; if (nativeProvider) { @@ -61,4 +97,13 @@ export class MockCustomEditorService implements ICustomEditorService { private closedEditor(editor: INotebookEditor) { this.resolvedList.delete(editor.file.toString()); } + + private onEditChange(e: { readonly resource: Uri; readonly edit: unknown }) { + let stack = this.undoStack.get(e.resource.toString()); + if (stack === undefined) { + stack = []; + this.undoStack.set(e.resource.toString(), stack); + } + stack.push(e.edit); + } } diff --git a/src/test/datascience/nativeEditor.functional.test.tsx b/src/test/datascience/nativeEditor.functional.test.tsx index b5016091fe5a..d67fd4034fd3 100644 --- a/src/test/datascience/nativeEditor.functional.test.tsx +++ b/src/test/datascience/nativeEditor.functional.test.tsx @@ -11,7 +11,7 @@ import * as path from 'path'; import * as sinon from 'sinon'; import * as TypeMoq from 'typemoq'; import { Disposable, TextDocument, TextEditor, Uri } from 'vscode'; -import { IApplicationShell, IDocumentManager } from '../../client/common/application/types'; +import { IApplicationShell, ICustomEditorService, IDocumentManager } from '../../client/common/application/types'; import { IFileSystem } from '../../client/common/platform/types'; import { createDeferred, sleep, waitForPromise } from '../../client/common/utils/async'; import { noop } from '../../client/common/utils/misc'; @@ -30,6 +30,7 @@ import { IMonacoEditorState, MonacoEditor } from '../../datascience-ui/react-com import { waitForCondition } from '../common'; import { createTemporaryFile } from '../utils/fs'; import { DataScienceIocContainer } from './dataScienceIocContainer'; +import { MockCustomEditorService } from './mockCustomEditorService'; import { MockDocumentManager } from './mockDocumentManager'; import { addCell, closeNotebook, createNewEditor, getNativeCellResults, mountNativeWebView, openEditor, runMountedTest, setupWebview } from './nativeEditorTestHelpers'; import { waitForUpdate } from './reactHelpers'; @@ -846,6 +847,34 @@ for _ in range(50): }); }); + suite('Model updates', () => { + setup(async function() { + initIoc(); + // tslint:disable-next-line: no-invalid-this + await setupFunction.call(this); + }); + async function undo(): Promise { + const uri = Uri.file(notebookFile.filePath); + const update = waitForMessage(ioc, InteractiveWindowMessages.ReceivedUpdateModel); + const editorService = ioc.serviceManager.get(ICustomEditorService) as MockCustomEditorService; + editorService.undo(uri); + return update; + } + test('Add a cell and undo', async () => { + addMockData(ioc, 'c=4\nc', '4'); + await addCell(wrapper, ioc, 'c=4\nc', false); + + // Should have 4 cells + assert.equal(wrapper.find('NativeCell').length, 4, 'Cell not added'); + + // Send undo through the custom editor + await undo(); + + // Should have 3 + assert.equal(wrapper.find('NativeCell').length, 3, 'Cell not removed'); + }); + }); + suite('Keyboard Shortcuts', () => { const originalPlatform = window.navigator.platform; Object.defineProperty( diff --git a/src/test/datascience/testHelpers.tsx b/src/test/datascience/testHelpers.tsx index 805e8d2a5745..9eecd2c04e1e 100644 --- a/src/test/datascience/testHelpers.tsx +++ b/src/test/datascience/testHelpers.tsx @@ -91,6 +91,10 @@ export function waitForMessage(ioc: DataScienceIocContainer, message: string, op clearTimeout(timer); } ioc.removeMessageListener(handler); + // Make sure to rerender current state. + if (ioc.wrapper) { + ioc.wrapper.update(); + } promise.resolve(); } }; From 7403fc259385bc85b78cd6b5fb846809603793a1 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 5 Feb 2020 10:48:21 -0800 Subject: [PATCH 10/23] More testing of undo --- .../nativeEditor.functional.test.tsx | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/test/datascience/nativeEditor.functional.test.tsx b/src/test/datascience/nativeEditor.functional.test.tsx index d67fd4034fd3..354bdfe9a478 100644 --- a/src/test/datascience/nativeEditor.functional.test.tsx +++ b/src/test/datascience/nativeEditor.functional.test.tsx @@ -860,6 +860,13 @@ for _ in range(50): editorService.undo(uri); return update; } + async function redo(): Promise { + const uri = Uri.file(notebookFile.filePath); + const update = waitForMessage(ioc, InteractiveWindowMessages.ReceivedUpdateModel); + const editorService = ioc.serviceManager.get(ICustomEditorService) as MockCustomEditorService; + editorService.redo(uri); + return update; + } test('Add a cell and undo', async () => { addMockData(ioc, 'c=4\nc', '4'); await addCell(wrapper, ioc, 'c=4\nc', false); @@ -873,6 +880,52 @@ for _ in range(50): // Should have 3 assert.equal(wrapper.find('NativeCell').length, 3, 'Cell not removed'); }); + test('Edit a cell and undo', async () => { + await addCell(wrapper, ioc, '', false); + + // Should have 4 cells + assert.equal(wrapper.find('NativeCell').length, 4, 'Cell not added'); + + // Change the contents of the cell + const editorEnzyme = getNativeFocusedEditor(wrapper); + + // Type in something with brackets + typeCode(editorEnzyme, 'some more'); + + // Verify cell content + const reactEditor = editorEnzyme!.instance() as MonacoEditor; + const editor = reactEditor.state.editor; + if (editor) { + assert.equal(editor.getModel()!.getValue(), 'some more', 'Text does not match'); + } + + // Add a new cell + await addCell(wrapper, ioc, '', false); + + // Send undo a bunch of times. Should undo the add and the edits + await undo(); + await undo(); + await undo(); + + // Should have four again + assert.equal(wrapper.find('NativeCell').length, 4, 'Cell not removed on undo'); + + // Should have different content + if (editor) { + assert.equal(editor.getModel()!.getValue(), 'some mo', 'Text does not match after undo'); + } + + // Send redo to see if goes back + await redo(); + if (editor) { + assert.equal(editor.getModel()!.getValue(), 'some mor', 'Text does not match'); + } + + // Send redo to see if goes back + await redo(); + await redo(); + assert.equal(wrapper.find('NativeCell').length, 5, 'Cell not readded on redo'); + }); }); suite('Keyboard Shortcuts', () => { From 1714b7f8fb298b49a771c777357913afc65ad957 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 5 Feb 2020 11:04:40 -0800 Subject: [PATCH 11/23] More tests --- .../native-editor/nativeCell.tsx | 24 ---------- .../native-editor/nativeEditor.tsx | 14 ------ .../nativeEditor.functional.test.tsx | 46 +++++++++++++++++++ 3 files changed, 46 insertions(+), 38 deletions(-) diff --git a/src/datascience-ui/native-editor/nativeCell.tsx b/src/datascience-ui/native-editor/nativeCell.tsx index a0e094589c20..2c1c44dbf2a7 100644 --- a/src/datascience-ui/native-editor/nativeCell.tsx +++ b/src/datascience-ui/native-editor/nativeCell.tsx @@ -315,30 +315,6 @@ export class NativeCell extends React.Component { this.props.sendCommand(NativeCommandType.InsertBelow, 'keyboard'); } break; - case 'z': - case 'Z': - if (!this.isFocused()) { - if (e.shiftKey && !e.ctrlKey && !e.altKey) { - e.stopPropagation(); - this.props.redo(); - this.props.sendCommand(NativeCommandType.Redo, 'keyboard'); - } else if (!e.shiftKey && !e.altKey && !e.ctrlKey) { - e.stopPropagation(); - this.props.undo(); - this.props.sendCommand(NativeCommandType.Undo, 'keyboard'); - } - } - break; - case 'KeyZ': - if (e.ctrlKey) { - window.console.log('Undoing'); - } - break; - case 'KeyY': - if (e.ctrlKey) { - window.console.log('Redoing'); - } - break; default: break; diff --git a/src/datascience-ui/native-editor/nativeEditor.tsx b/src/datascience-ui/native-editor/nativeEditor.tsx index d099dc1136e1..b3233701c94c 100644 --- a/src/datascience-ui/native-editor/nativeEditor.tsx +++ b/src/datascience-ui/native-editor/nativeEditor.tsx @@ -313,20 +313,6 @@ export class NativeEditor extends React.Component { } break; } - case 'z': - case 'Z': - if (this.props.focusedCellId === undefined) { - if (event.shiftKey && !event.ctrlKey && !event.altKey) { - event.stopPropagation(); - this.props.redo(); - this.props.sendCommand(NativeCommandType.Redo, 'keyboard'); - } else if (!event.shiftKey && !event.altKey && !event.ctrlKey) { - event.stopPropagation(); - this.props.undo(); - this.props.sendCommand(NativeCommandType.Undo, 'keyboard'); - } - } - break; default: break; } diff --git a/src/test/datascience/nativeEditor.functional.test.tsx b/src/test/datascience/nativeEditor.functional.test.tsx index 354bdfe9a478..2f78b208e0b7 100644 --- a/src/test/datascience/nativeEditor.functional.test.tsx +++ b/src/test/datascience/nativeEditor.functional.test.tsx @@ -926,6 +926,52 @@ for _ in range(50): await redo(); assert.equal(wrapper.find('NativeCell').length, 5, 'Cell not readded on redo'); }); + test('Remove, move, and undo', async () => { + await addCell(wrapper, ioc, '', false); + + // Should have 4 cells + assert.equal(wrapper.find('NativeCell').length, 4, 'Cell not added'); + + // Delete the cell + let cell = getLastOutputCell(wrapper, 'NativeCell'); + let imageButtons = cell.find(ImageButton); + assert.equal(imageButtons.length, 6, 'Cell buttons not found'); + const deleteButton = imageButtons.at(5); + await getNativeCellResults(ioc, wrapper, async () => { + deleteButton.simulate('click'); + return Promise.resolve(); + }); + // Should have 3 cells + assert.equal(wrapper.find('NativeCell').length, 3, 'Cell not deleted'); + + // Undo the delete + await undo(); + + // Should have 4 cells again + assert.equal(wrapper.find('NativeCell').length, 4, 'Cell delete not undone'); + + // Redo the delete + await redo(); + + // Should have 3 cells again + assert.equal(wrapper.find('NativeCell').length, 3, 'Cell delete not redone'); + + // Move some cells around + cell = getLastOutputCell(wrapper, 'NativeCell'); + imageButtons = cell.find(ImageButton); + assert.equal(imageButtons.length, 6, 'Cell buttons not found'); + const moveUpButton = imageButtons.at(0); + await getNativeCellResults(ioc, wrapper, async () => { + moveUpButton.simulate('click'); + return Promise.resolve(); + }); + + let foundCell = getOutputCell(wrapper, 'NativeCell', 2)?.instance() as NativeCell; + assert.equal(foundCell.props.cellVM.cell.id, 'NotebookImport#1', 'Cell did not move'); + await undo(); + foundCell = getOutputCell(wrapper, 'NativeCell', 2)?.instance() as NativeCell; + assert.equal(foundCell.props.cellVM.cell.id, 'NotebookImport#2', 'Cell did not move back'); + }); }); suite('Keyboard Shortcuts', () => { From 3a9b8859c1306f5e95979dc1f0363514bbc7da1a Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 5 Feb 2020 11:22:01 -0800 Subject: [PATCH 12/23] Close suggestions on undo --- src/datascience-ui/react-common/monacoEditor.tsx | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/datascience-ui/react-common/monacoEditor.tsx b/src/datascience-ui/react-common/monacoEditor.tsx index 91eb10f3be6b..a8194fa869bc 100644 --- a/src/datascience-ui/react-common/monacoEditor.tsx +++ b/src/datascience-ui/react-common/monacoEditor.tsx @@ -392,6 +392,14 @@ export class MonacoEditor extends React.Component Date: Wed, 5 Feb 2020 13:48:58 -0800 Subject: [PATCH 13/23] Fix other functional tests --- .../interactive-ipynb/nativeEditor.ts | 8 - .../interactive-ipynb/nativeEditorStorage.ts | 7 +- src/client/datascience/types.ts | 1 - .../nativeEditor.functional.test.tsx | 189 ++---------------- 4 files changed, 20 insertions(+), 185 deletions(-) diff --git a/src/client/datascience/interactive-ipynb/nativeEditor.ts b/src/client/datascience/interactive-ipynb/nativeEditor.ts index f7eaac3bb710..96cc2c4aa40c 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditor.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditor.ts @@ -54,7 +54,6 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { private closedEvent: EventEmitter = new EventEmitter(); private executedEvent: EventEmitter = new EventEmitter(); private modifiedEvent: EventEmitter = new EventEmitter(); - private savedEvent: EventEmitter = new EventEmitter(); private loadedPromise: Deferred = createDeferred(); private startupTimer: StopWatch = new StopWatch(); private loadedAllCells: boolean = false; @@ -178,10 +177,6 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { return this.modifiedEvent.event; } - public get saved(): Event { - return this.savedEvent.event; - } - public get isDirty(): boolean { return this._model ? this._model.isDirty : false; } @@ -420,9 +415,6 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { if (this._model.isDirty) { await this.postMessage(InteractiveWindowMessages.NotebookDirty); } else { - // Going clean should only happen on a save (for now. Undo might do this too) - this.savedEvent.fire(this); - // Then tell the UI await this.postMessage(InteractiveWindowMessages.NotebookClean); } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 18241427e339..56a97b1ae453 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -170,10 +170,13 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID break; case 'file': this._state.file = change.newFile; + this._state.isDirty = false; NativeEditorStorage.storageMap.delete(change.oldFile.toString()); NativeEditorStorage.storageMap.set(change.newFile.toString(), this); - changed = change.oldFile.toString() !== change.newFile.toString(); - break; + + // Special case for file, don't set dirty (as we're saving), but still + // indicate changed. + return true; default: break; } diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index eeb4cbd77a69..84302b1dd566 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -321,7 +321,6 @@ export interface INotebookEditor extends IInteractiveBase { readonly closed: Event; readonly executed: Event; readonly modified: Event; - readonly saved: Event; /** * Is this notebook representing an untitled file which has never been saved yet. */ diff --git a/src/test/datascience/nativeEditor.functional.test.tsx b/src/test/datascience/nativeEditor.functional.test.tsx index 2f78b208e0b7..a9c2cfbb150b 100644 --- a/src/test/datascience/nativeEditor.functional.test.tsx +++ b/src/test/datascience/nativeEditor.functional.test.tsx @@ -2,7 +2,7 @@ // Licensed under the MIT License. 'use strict'; import { nbformat } from '@jupyterlab/coreutils'; -import { assert, expect, use } from 'chai'; +import { assert, use } from 'chai'; import * as chaiAsPromised from 'chai-as-promised'; import { ReactWrapper } from 'enzyme'; import { EventEmitter } from 'events'; @@ -27,7 +27,6 @@ import { NativeEditor } from '../../datascience-ui/native-editor/nativeEditor'; import { IKeyboardEvent } from '../../datascience-ui/react-common/event'; import { ImageButton } from '../../datascience-ui/react-common/imageButton'; import { IMonacoEditorState, MonacoEditor } from '../../datascience-ui/react-common/monacoEditor'; -import { waitForCondition } from '../common'; import { createTemporaryFile } from '../utils/fs'; import { DataScienceIocContainer } from './dataScienceIocContainer'; import { MockCustomEditorService } from './mockCustomEditorService'; @@ -384,17 +383,14 @@ for _ in range(50): ioc.serviceManager.rebindInstance(IApplicationShell, appShell.object); // Make sure to create the interactive window after the rebind or it gets the wrong application shell. - const saveCalled = createDeferred(); - const editor = await createNewEditor(ioc); - editor.saved(() => { - saveCalled.resolve(true); - }); + await createNewEditor(ioc); await addCell(wrapper, ioc, 'a=1\na'); // Export should cause exportCalled to change to true const saveButton = findButton(wrapper, NativeEditor, 8); + const saved = waitForMessage(ioc, InteractiveWindowMessages.NotebookClean); await waitForMessageResponse(ioc, () => saveButton!.simulate('click')); - await waitForPromise(saveCalled.promise, 5_000); + await saved; // Click export and wait for a document to change const activeTextEditorChange = createDeferred(); @@ -1366,155 +1362,6 @@ for _ in range(50): const monacoEditor = editor!.instance() as MonacoEditor; assert.equal('foo', monacoEditor.state.editor!.getValue(), 'Changing cell type lost input'); }); - - test("Test undo using the key 'z'", async () => { - clickCell(0); - - // Add, then undo, keep doing at least 3 times and confirm it works as expected. - for (let i = 0; i < 3; i += 1) { - // Add a new cell - let update = waitForMessage(ioc, InteractiveWindowMessages.FocusedCellEditor); - simulateKeyPressOnCell(0, { code: 'a' }); - await update; - - // Wait a bit for the time out to try and set focus a second time (this will be - // fixed when we switch to redux) - await sleep(100); - - // There should be 4 cells and first cell is focused. - assert.equal(isCellSelected(wrapper, 'NativeCell', 0), false); - assert.equal(isCellSelected(wrapper, 'NativeCell', 1), false); - assert.equal(isCellFocused(wrapper, 'NativeCell', 0), true); - assert.equal(isCellFocused(wrapper, 'NativeCell', 1), false); - assert.equal(wrapper.find('NativeCell').length, 4); - - // Unfocus the cell - update = waitForUpdate(wrapper, NativeEditor, 1); - simulateKeyPressOnCell(0, { code: 'Escape' }); - await update; - assert.equal(isCellSelected(wrapper, 'NativeCell', 0), true); - - // Press 'ctrl+z'. This should do nothing - update = waitForUpdate(wrapper, NativeEditor, 1); - simulateKeyPressOnCell(0, { code: 'z', ctrlKey: true }); - await waitForPromise(update, 100); - - // There should be 4 cells and first cell is selected. - assert.equal(isCellSelected(wrapper, 'NativeCell', 0), true); - assert.equal(isCellSelected(wrapper, 'NativeCell', 1), false); - assert.equal(isCellFocused(wrapper, 'NativeCell', 0), false); - assert.equal(isCellFocused(wrapper, 'NativeCell', 1), false); - assert.equal(wrapper.find('NativeCell').length, 4); - - // Press 'z' to undo. - update = waitForUpdate(wrapper, NativeEditor, 1); - simulateKeyPressOnCell(0, { code: 'z' }); - await update; - - // There should be 3 cells and first cell is selected & nothing focused. - assert.equal(isCellSelected(wrapper, 'NativeCell', 0), true); - assert.equal(isCellSelected(wrapper, 'NativeCell', 1), false); - assert.equal(wrapper.find('NativeCell').length, 3); - - // Press 'shift+z' to redo - update = waitForUpdate(wrapper, NativeEditor, 1); - simulateKeyPressOnCell(0, { code: 'z', shiftKey: true }); - await update; - - // There should be 4 cells and first cell is selected. - assert.equal(isCellSelected(wrapper, 'NativeCell', 0), true); - assert.equal(isCellSelected(wrapper, 'NativeCell', 1), false); - assert.equal(isCellFocused(wrapper, 'NativeCell', 0), false); - assert.equal(isCellFocused(wrapper, 'NativeCell', 1), false); - assert.equal(wrapper.find('NativeCell').length, 4); - - // Press 'z' to undo. - update = waitForUpdate(wrapper, NativeEditor, 1); - simulateKeyPressOnCell(0, { code: 'z' }); - await update; - - // There should be 3 cells and first cell is selected & nothing focused. - assert.equal(isCellSelected(wrapper, 'NativeCell', 0), true); - assert.equal(isCellSelected(wrapper, 'NativeCell', 1), false); - assert.equal(wrapper.find('NativeCell').length, 3); - } - }); - - test("Test save using the key 'ctrl+s' on Windows", async () => { - (window.navigator as any).platform = 'Win'; - clickCell(0); - - await addCell(wrapper, ioc, 'a=1\na', true); - - const notebookProvider = ioc.get(INotebookEditorProvider); - const editor = notebookProvider.editors[0]; - assert.ok(editor, 'No editor when saving'); - const savedPromise = createDeferred(); - editor.saved(() => savedPromise.resolve()); - - simulateKeyPressOnCell(1, { code: 's', ctrlKey: true }); - - await waitForCondition(() => savedPromise.promise.then(() => true).catch(() => false), 1_000, 'Timedout'); - - assert.ok(!editor!.isDirty, 'Editor should not be dirty after saving'); - }); - - test("Test save using the key 'ctrl+s' on Mac", async () => { - (window.navigator as any).platform = 'Mac'; - clickCell(0); - - await addCell(wrapper, ioc, 'a=1\na', true); - - const notebookProvider = ioc.get(INotebookEditorProvider); - const editor = notebookProvider.editors[0]; - assert.ok(editor, 'No editor when saving'); - const savedPromise = createDeferred(); - editor.saved(() => savedPromise.resolve()); - - simulateKeyPressOnCell(1, { code: 's', ctrlKey: true }); - - await expect(waitForCondition(() => savedPromise.promise.then(() => true).catch(() => false), 1_000, 'Timedout')).to.eventually.be.rejected; - assert.ok(editor!.isDirty, 'Editor be dirty as nothing got saved'); - }); - - test("Test save using the key 'cmd+s' on a Mac", async () => { - (window.navigator as any).platform = 'Mac'; - - clickCell(0); - - await addCell(wrapper, ioc, 'a=1\na', true); - - const notebookProvider = ioc.get(INotebookEditorProvider); - const editor = notebookProvider.editors[0]; - assert.ok(editor, 'No editor when saving'); - const savedPromise = createDeferred(); - editor.saved(() => savedPromise.resolve()); - - simulateKeyPressOnCell(1, { code: 's', metaKey: true }); - - await waitForCondition(() => savedPromise.promise.then(() => true).catch(() => false), 1_000, 'Timedout'); - - assert.ok(!editor!.isDirty, 'Editor should not be dirty after saving'); - }); - test("Test save using the key 'cmd+s' on a Windows", async () => { - (window.navigator as any).platform = 'Win'; - - clickCell(0); - - await addCell(wrapper, ioc, 'a=1\na', true); - - const notebookProvider = ioc.get(INotebookEditorProvider); - const editor = notebookProvider.editors[0]; - assert.ok(editor, 'No editor when saving'); - const savedPromise = createDeferred(); - editor.saved(() => savedPromise.resolve()); - - // CMD+s won't work on Windows. - simulateKeyPressOnCell(1, { code: 's', metaKey: true }); - - await expect(waitForCondition(() => savedPromise.promise.then(() => true).catch(() => false), 1_000, 'Timedout')).to.eventually.be.rejected; - assert.ok(editor!.isDirty, 'Editor be dirty as nothing got saved'); - }); }); suite('Update Metadata', () => { @@ -1597,8 +1444,6 @@ for _ in range(50): const notebookProvider = ioc.get(INotebookEditorProvider); const editor = notebookProvider.editors[0]; assert.ok(editor, 'No editor when saving'); - const savedPromise = createDeferred(); - const disposeSaved = editor.saved(() => savedPromise.resolve()); // add cells, run them and save await addCell(wrapper, ioc, 'a=1\na'); @@ -1607,10 +1452,10 @@ for _ in range(50): await waitForMessageResponse(ioc, () => runAllButton!.simulate('click')); await threeCellsUpdated; - simulateKeyPressOnCell(1, { code: 's', ctrlKey: true }); - - await savedPromise.promise; - disposeSaved.dispose(); + const saveButton = findButton(wrapper, NativeEditor, 8); + const saved = waitForMessage(ioc, InteractiveWindowMessages.NotebookClean); + await waitForMessageResponse(ioc, () => saveButton!.simulate('click')); + await saved; // the file has output and execution count const fileContent = await fs.readFile(notebookFile.filePath, 'utf8'); @@ -1690,30 +1535,26 @@ for _ in range(50): const notebookProvider = ioc.get(INotebookEditorProvider); const editor = notebookProvider.editors[0]; assert.ok(editor, 'No editor when saving'); - let savedPromise = createDeferred(); - let disp = editor.saved(() => savedPromise.resolve()); - // add cells, run them and save await addCell(wrapper, ioc, 'a=1\na'); const runAllButton = findButton(wrapper, NativeEditor, 0); await waitForMessageResponse(ioc, () => runAllButton!.simulate('click')); - simulateKeyPressOnCell(1, { code: 's', ctrlKey: true }); - await savedPromise.promise; - disp.dispose(); + const saveButton = findButton(wrapper, NativeEditor, 8); + let saved = waitForMessage(ioc, InteractiveWindowMessages.NotebookClean); + await waitForMessageResponse(ioc, () => saveButton!.simulate('click')); + await saved; // the file has output and execution count const fileContent = await fs.readFile(notebookFile.filePath, 'utf8'); - savedPromise = createDeferred(); - disp = editor.saved(() => savedPromise.resolve()); - // press clear all outputs, and save const clearAllOutputButton = findButton(wrapper, NativeEditor, 6); await waitForMessageResponse(ioc, () => clearAllOutputButton!.simulate('click')); - simulateKeyPressOnCell(1, { code: 's', ctrlKey: true }); - await savedPromise.promise; + saved = waitForMessage(ioc, InteractiveWindowMessages.NotebookClean); + await waitForMessageResponse(ioc, () => saveButton!.simulate('click')); + await saved; // the file now shouldn't have outputs or execution count const newFileContent = await fs.readFile(notebookFile.filePath, 'utf8'); From 679f3a4a55facc03b55412244be9a3ccb8dfb384 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 6 Feb 2020 08:30:34 -0800 Subject: [PATCH 14/23] Fix undo misses --- src/client/common/application/commands.ts | 2 - src/client/datascience/constants.ts | 1 - .../interactive-ipynb/nativeEditor.ts | 10 +-- .../interactive-ipynb/nativeEditorProvider.ts | 4 +- .../interactive-ipynb/nativeEditorStorage.ts | 62 +++---------------- src/client/datascience/types.ts | 2 +- .../nativeEditorStorage.unit.test.ts | 37 ++++------- 7 files changed, 28 insertions(+), 90 deletions(-) diff --git a/src/client/common/application/commands.ts b/src/client/common/application/commands.ts index edbd7680638a..a34a714c217c 100644 --- a/src/client/common/application/commands.ts +++ b/src/client/common/application/commands.ts @@ -6,7 +6,6 @@ import { CancellationToken, Position, TextDocument, Uri } from 'vscode'; import { Commands as LSCommands } from '../../activation/languageServer/constants'; import { Commands as DSCommands } from '../../datascience/constants'; -import { NotebookModelChange } from '../../datascience/interactive-common/interactiveWindowTypes'; import { INotebook } from '../../datascience/types'; import { CommandSource } from '../../testing/common/constants'; import { TestFunction, TestsToRun } from '../../testing/common/types'; @@ -146,5 +145,4 @@ export interface ICommandNameArgumentTypeMapping extends ICommandNameWithoutArgu [DSCommands.ScrollToCell]: [string, string]; [DSCommands.ViewJupyterOutput]: []; [DSCommands.SwitchJupyterKernel]: [INotebook | undefined]; - [DSCommands.NotebookModel_Update]: [Uri, NotebookModelChange]; } diff --git a/src/client/datascience/constants.ts b/src/client/datascience/constants.ts index f4633ee2cf14..c8a03ab4a8b9 100644 --- a/src/client/datascience/constants.ts +++ b/src/client/datascience/constants.ts @@ -62,7 +62,6 @@ export namespace Commands { export const ScrollToCell = 'python.datascience.scrolltocell'; export const CreateNewNotebook = 'python.datascience.createnewnotebook'; export const ViewJupyterOutput = 'python.datascience.viewJupyterOutput'; - export const NotebookModel_Update = 'python.datascience.notebook.update'; } export namespace CodeLensCommands { diff --git a/src/client/datascience/interactive-ipynb/nativeEditor.ts b/src/client/datascience/interactive-ipynb/nativeEditor.ts index 96cc2c4aa40c..eaecf1c01b46 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditor.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditor.ts @@ -19,7 +19,7 @@ import { StopWatch } from '../../common/utils/stopWatch'; import { EXTENSION_ROOT_DIR } from '../../constants'; import { IInterpreterService } from '../../interpreter/contracts'; import { captureTelemetry, sendTelemetryEvent } from '../../telemetry'; -import { Commands, EditorContexts, Identifiers, NativeKeyboardCommandTelemetryLookup, NativeMouseCommandTelemetryLookup, Telemetry } from '../constants'; +import { EditorContexts, Identifiers, NativeKeyboardCommandTelemetryLookup, NativeMouseCommandTelemetryLookup, Telemetry } from '../constants'; import { InteractiveBase } from '../interactive-common/interactiveBase'; import { INativeCommand, InteractiveWindowMessages, ISaveAll, ISubmitNewCell, NotebookModelChange, SysInfoReason } from '../interactive-common/interactiveWindowTypes'; import { ProgressReporter } from '../progress/progressReporter'; @@ -339,7 +339,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { const modified = filtered.filter(c => c.state === CellState.finished || c.state === CellState.error); const unmodified = this._model?.cells.filter(c => modified.find(m => m.id === c.id)); if (modified.length > 0 && unmodified && this._model) { - await this.commandManager.executeCommand(Commands.NotebookModel_Update, this.file, { + this._model.update({ source: 'user', kind: 'modify', newCells: modified, @@ -354,7 +354,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { if (notebook && this._model) { const interpreter = notebook.getMatchingInterpreter(); const kernelSpec = notebook.getKernelSpec(); - this.commandManager.executeCommand(Commands.NotebookModel_Update, this.file, { + this._model.update({ source: 'user', kind: 'version', oldDirty: this._model.isDirty, @@ -424,7 +424,9 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { private updateModel(change: NotebookModelChange) { // Send to our model using a command. User has done something that changes the model if (change.source === 'user' && this._model) { - this.commandManager.executeCommand(Commands.NotebookModel_Update, this.file, change); + // Note, originally this was posted with a command but sometimes had problems + // with commands being handled out of order. + this._model.update(change); } } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts index 2dd2e9a630e4..78796b9471b9 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts @@ -79,14 +79,14 @@ export class NativeEditorProvider implements INotebookEditorProvider, WebviewCus return this._editEventEmitter.event; } public applyEdits(resource: Uri, edits: readonly NotebookModelChange[]): Thenable { - return this.loadStorage(resource).then(s => { + return this.loadModel(resource).then(s => { if (s) { edits.forEach(e => s.update({ ...e, source: 'redo' })); } }); } public undoEdits(resource: Uri, edits: readonly NotebookModelChange[]): Thenable { - return this.loadStorage(resource).then(s => { + return this.loadModel(resource).then(s => { if (s) { edits.forEach(e => s.update({ ...e, source: 'undo' })); } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 56a97b1ae453..4133a2f2c7c1 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -5,13 +5,12 @@ import * as uuid from 'uuid/v4'; import { Event, EventEmitter, Memento, Uri } from 'vscode'; import { concatMultilineStringInput, splitMultilineString } from '../../../datascience-ui/common'; import { createCodeCell } from '../../../datascience-ui/common/cellFactory'; -import { ICommandManager } from '../../common/application/types'; import { traceError } from '../../common/logger'; import { IFileSystem } from '../../common/platform/types'; -import { GLOBAL_MEMENTO, ICryptoUtils, IDisposable, IDisposableRegistry, IExtensionContext, IMemento, WORKSPACE_MEMENTO } from '../../common/types'; +import { GLOBAL_MEMENTO, ICryptoUtils, IExtensionContext, IMemento, WORKSPACE_MEMENTO } from '../../common/types'; import { noop } from '../../common/utils/misc'; import { PythonInterpreter } from '../../interpreter/contracts'; -import { Commands, Identifiers } from '../constants'; +import { Identifiers } from '../constants'; import { IEditorContentChange, NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; import { InvalidNotebookFileError } from '../jupyter/invalidNotebookFileError'; import { LiveKernelModel } from '../jupyter/kernels/types'; @@ -31,7 +30,7 @@ interface INativeEditorStorageState { } @injectable() -export class NativeEditorStorage implements INotebookModel, INotebookStorage, IDisposable { +export class NativeEditorStorage implements INotebookModel, INotebookStorage { public get isDirty(): boolean { return this._state.isDirty; } @@ -48,43 +47,19 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID public get cells(): ICell[] { return this._state.cells; } - private static signedUpForCommands = false; - - private static storageMap = new Map(); private _changedEmitter = new EventEmitter(); private _state: INativeEditorStorageState = { file: Uri.file(''), isDirty: false, cells: [], notebookJson: {} }; private _loadPromise: Promise | undefined; private indentAmount: string = ' '; constructor( - @inject(IDisposableRegistry) disposables: IDisposableRegistry, @inject(IJupyterExecution) private jupyterExecution: IJupyterExecution, @inject(IFileSystem) private fileSystem: IFileSystem, @inject(ICryptoUtils) private crypto: ICryptoUtils, @inject(IExtensionContext) private context: IExtensionContext, @inject(IMemento) @named(GLOBAL_MEMENTO) private globalStorage: Memento, - @inject(IMemento) @named(WORKSPACE_MEMENTO) private localStorage: Memento, - @inject(ICommandManager) cmdManager: ICommandManager - ) { - // Sign up for commands if this is the first storage created. - if (!NativeEditorStorage.signedUpForCommands) { - this.registerCommands(cmdManager, disposables); - } - disposables.push(this); - } - - private static async getStorage(resource: Uri): Promise { - const storage = NativeEditorStorage.storageMap.get(resource.toString()); - if (storage && storage._loadPromise) { - await storage._loadPromise; - return storage; - } - return undefined; - } - - public dispose(): void { - NativeEditorStorage.storageMap.delete(this.file.toString()); - } + @inject(IMemento) @named(WORKSPACE_MEMENTO) private localStorage: Memento + ) {} public async load(file: Uri, possibleContents?: string): Promise { // Reset the load promise and reload our cells @@ -171,8 +146,6 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID case 'file': this._state.file = change.newFile; this._state.isDirty = false; - NativeEditorStorage.storageMap.delete(change.oldFile.toString()); - NativeEditorStorage.storageMap.set(change.newFile.toString(), this); // Special case for file, don't set dirty (as we're saving), but still // indicate changed. @@ -194,7 +167,8 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID changed = true; break; case 'edit': - changed = this.editCell(change.reverse, change.id); + this.editCell(change.reverse, change.id); + changed = true; break; case 'insert': changed = this.removeCell(change.cell); @@ -212,9 +186,6 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID case 'swap': changed = this.swapCells(change.firstCellId, change.secondCellId); break; - case 'version': - // Not supporting undo - break; default: break; } @@ -321,28 +292,9 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage, ID return cell as ICell; } - private registerCommands(commandManager: ICommandManager, disposableRegistry: IDisposableRegistry): void { - NativeEditorStorage.signedUpForCommands = true; - disposableRegistry.push({ - dispose: () => { - NativeEditorStorage.signedUpForCommands = false; - } - }); - disposableRegistry.push( - commandManager.registerCommand(Commands.NotebookModel_Update, async (resource: Uri, change: NotebookModelChange) => { - // Find the appropriate storage object and call into it - const storage = await NativeEditorStorage.getStorage(resource); - if (storage) { - return storage.handleModelChange(change); - } - }) - ); - } - private async loadFromFile(file: Uri, possibleContents?: string): Promise { // Save file this._state.file = file; - NativeEditorStorage.storageMap.set(file.toString(), this); try { // Attempt to read the contents if a viable file diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index 84302b1dd566..8526cda143c2 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -735,6 +735,7 @@ export interface INotebookModel { readonly cells: ICell[]; getJson(): Promise>; getContent(cells?: ICell[]): Promise; + update(change: NotebookModelChange): void; } export const INotebookStorage = Symbol('INotebookStorage'); @@ -743,5 +744,4 @@ export interface INotebookStorage { load(file: Uri, contents?: string): Promise; save(): Promise; saveAs(file: Uri): Promise; - update(change: NotebookModelChange): void; } diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index 7e22a74bff45..3c872df967dc 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -9,7 +9,6 @@ import { Matcher } from 'ts-mockito/lib/matcher/type/Matcher'; import * as typemoq from 'typemoq'; import { ConfigurationChangeEvent, ConfigurationTarget, EventEmitter, TextEditor, Uri, WorkspaceConfiguration } from 'vscode'; -import { ICommandNameArgumentTypeMapping } from '../../../client/common/application/commands'; import { DocumentManager } from '../../../client/common/application/documentManager'; import { IDocumentManager, IWebPanelMessageListener, IWebPanelProvider, IWorkspaceService } from '../../../client/common/application/types'; import { WebPanel } from '../../../client/common/application/webPanels/webPanel'; @@ -21,7 +20,6 @@ import { CryptoUtils } from '../../../client/common/crypto'; import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService, ICryptoUtils, IDisposable, IExtensionContext } from '../../../client/common/types'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; -import { Commands } from '../../../client/datascience/constants'; import { IEditorContentChange, InteractiveWindowMessages } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; import { NativeEditorStorage } from '../../../client/datascience/interactive-ipynb/nativeEditorStorage'; import { JupyterExecutionFactory } from '../../../client/datascience/jupyter/jupyterExecutionFactory'; @@ -30,7 +28,6 @@ import { IInterpreterService } from '../../../client/interpreter/contracts'; import { InterpreterService } from '../../../client/interpreter/interpreterService'; import { createEmptyCell } from '../../../datascience-ui/interactive-common/mainState'; import { MockMemento } from '../../mocks/mementos'; -import { MockCommandManager } from '../mockCommandManager'; // tslint:disable: no-any chai-vague-errors no-unused-expression class MockWorkspaceConfiguration implements WorkspaceConfiguration { @@ -67,7 +64,6 @@ suite('Data Science - Native Editor Storage', () => { let configService: IConfigurationService; let fileSystem: typemoq.IMock; let docManager: IDocumentManager; - let cmdManager: MockCommandManager; let interpreterService: IInterpreterService; let webPanelProvider: IWebPanelProvider; let executionProvider: IJupyterExecution; @@ -261,7 +257,6 @@ suite('Data Science - Native Editor Storage', () => { configService = mock(ConfigurationService); fileSystem = typemoq.Mock.ofType(); docManager = mock(DocumentManager); - cmdManager = new MockCommandManager(); workspace = mock(WorkspaceService); interpreterService = mock(InterpreterService); webPanelProvider = mock(WebPanelProvider); @@ -328,14 +323,12 @@ suite('Data Science - Native Editor Storage', () => { }); storage = new NativeEditorStorage( - disposables, instance(executionProvider), fileSystem.object, // Use typemoq so can save values in returns instance(crypto), context.object, globalMemento, - localMemento, - cmdManager + localMemento ); }); @@ -346,20 +339,18 @@ suite('Data Science - Native Editor Storage', () => { }); function insertCell(index: number, code: string) { - return executeCommand(Commands.NotebookModel_Update, baseUri, { + return storage.update({ source: 'user', kind: 'insert', oldDirty: storage.isDirty, newDirty: true, cell: createEmptyCell(code, 1), - index, - fullText: code, - currentText: code + index }); } function swapCells(first: string, second: string) { - return executeCommand(Commands.NotebookModel_Update, baseUri, { + return storage.update({ source: 'user', kind: 'swap', oldDirty: storage.isDirty, @@ -370,7 +361,7 @@ suite('Data Science - Native Editor Storage', () => { } function editCell(changes: IEditorContentChange[], cell: ICell, _newCode: string) { - return executeCommand(Commands.NotebookModel_Update, baseUri, { + return storage.update({ source: 'user', kind: 'edit', oldDirty: storage.isDirty, @@ -382,7 +373,7 @@ suite('Data Science - Native Editor Storage', () => { } function removeCell(index: number, cell: ICell) { - return executeCommand(Commands.NotebookModel_Update, baseUri, { + return storage.update({ source: 'user', kind: 'remove', oldDirty: storage.isDirty, @@ -393,7 +384,7 @@ suite('Data Science - Native Editor Storage', () => { } function deleteAllCells() { - return executeCommand(Commands.NotebookModel_Update, baseUri, { + return storage.update({ source: 'user', kind: 'remove_all', oldDirty: storage.isDirty, @@ -403,13 +394,9 @@ suite('Data Science - Native Editor Storage', () => { }); } - function executeCommand(command: E, ...rest: U) { - return cmdManager.executeCommand(command, ...rest); - } - test('Create new editor and add some cells', async () => { await storage.load(baseUri); - await insertCell(0, '1'); + insertCell(0, '1'); const cells = storage.cells; expect(cells).to.be.lengthOf(4); expect(storage.isDirty).to.be.equal(true, 'Editor should be dirty'); @@ -418,7 +405,7 @@ suite('Data Science - Native Editor Storage', () => { test('Move cells around', async () => { await storage.load(baseUri); - await swapCells('NotebookImport#0', 'NotebookImport#1'); + swapCells('NotebookImport#0', 'NotebookImport#1'); const cells = storage.cells; expect(cells).to.be.lengthOf(3); expect(storage.isDirty).to.be.equal(true, 'Editor should be dirty'); @@ -428,7 +415,7 @@ suite('Data Science - Native Editor Storage', () => { test('Edit/delete cells', async () => { await storage.load(baseUri); expect(storage.isDirty).to.be.equal(false, 'Editor should not be dirty'); - await editCell( + editCell( [ { range: { @@ -454,11 +441,11 @@ suite('Data Science - Native Editor Storage', () => { expect(cells[1].id).to.be.match(/NotebookImport#1/); expect(cells[1].data.source).to.be.equals('b=2\nab'); expect(storage.isDirty).to.be.equal(true, 'Editor should be dirty'); - await removeCell(0, cells[0]); + removeCell(0, cells[0]); cells = storage.cells; expect(cells).to.be.lengthOf(2); expect(cells[0].id).to.be.match(/NotebookImport#1/); - await deleteAllCells(); + deleteAllCells(); cells = storage.cells; expect(cells).to.be.lengthOf(1); }); From 21d0ee904f1b1b96aee2b3ac62f0114562a6a527 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 6 Feb 2020 08:45:20 -0800 Subject: [PATCH 15/23] Fix focus changes on undo --- .../native-editor/redux/reducers/creation.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/datascience-ui/native-editor/redux/reducers/creation.ts b/src/datascience-ui/native-editor/redux/reducers/creation.ts index 965136afb043..040cc0d78d65 100644 --- a/src/datascience-ui/native-editor/redux/reducers/creation.ts +++ b/src/datascience-ui/native-editor/redux/reducers/creation.ts @@ -12,6 +12,7 @@ import { Transfer } from '../../../interactive-common/redux/reducers/transfer'; import { IAddCellAction, ICellAction } from '../../../interactive-common/redux/reducers/types'; import { actionCreators } from '../actions'; import { NativeEditorReducerArg } from '../mapping'; +import { Effects } from './effects'; import { Execution } from './execution'; import { Movement } from './movement'; @@ -167,10 +168,8 @@ export namespace Creation { newVM.cursorPos = arg.payload.changes[0].position; const newVMs = [...arg.prevState.cellVMs]; newVMs[index] = Helpers.asCellViewModel(newVM); - return { - ...arg.prevState, - cellVMs: newVMs - }; + // When editing, make sure we focus the edited cell (otherwise undo looks weird because it undoes a non focused cell) + return Effects.focusCell({ ...arg, prevState: { ...arg.prevState, cellVMs: newVMs }, payload: { cursorPos: CursorPos.Current, cellId: arg.payload.id } }); } return arg.prevState; } From 58e6f676028992a5eadfa5278d16f47d4ef4f361 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 6 Feb 2020 09:23:33 -0800 Subject: [PATCH 16/23] Fixup after merge --- .../datascience/interactive-common/synchronization.ts | 9 +++------ src/datascience-ui/history-react/redux/reducers/index.ts | 2 +- src/datascience-ui/interactive-common/redux/store.ts | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/client/datascience/interactive-common/synchronization.ts b/src/client/datascience/interactive-common/synchronization.ts index 54c0decb7007..63223f977843 100644 --- a/src/client/datascience/interactive-common/synchronization.ts +++ b/src/client/datascience/interactive-common/synchronization.ts @@ -35,6 +35,7 @@ const messageWithMessageTypes: MessageMapping & Messa [CommonActionType.ARROW_UP]: MessageType.syncWithLiveShare, [CommonActionType.CHANGE_CELL_TYPE]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, [CommonActionType.CLICK_CELL]: MessageType.syncWithLiveShare, + [CommonActionType.DELETE_CELL]: MessageType.syncWithLiveShare, [CommonActionType.CODE_CREATED]: MessageType.noIdea, [CommonActionType.COPY_CELL_CODE]: MessageType.other, [CommonActionType.EDITOR_LOADED]: MessageType.other, @@ -73,7 +74,6 @@ const messageWithMessageTypes: MessageMapping & Messa // Types from InteractiveWindowMessages [InteractiveWindowMessages.Activate]: MessageType.other, - [InteractiveWindowMessages.AddCell]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, [InteractiveWindowMessages.AddedSysInfo]: MessageType.other, [InteractiveWindowMessages.CancelCompletionItemsRequest]: MessageType.other, [InteractiveWindowMessages.CancelHoverRequest]: MessageType.other, @@ -83,9 +83,7 @@ const messageWithMessageTypes: MessageMapping & Messa [InteractiveWindowMessages.CollapseAll]: MessageType.syncWithLiveShare, [InteractiveWindowMessages.CopyCodeCell]: MessageType.other, [InteractiveWindowMessages.DeleteAllCells]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, - [InteractiveWindowMessages.DeleteCell]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, [InteractiveWindowMessages.DoSave]: MessageType.other, - [InteractiveWindowMessages.EditCell]: MessageType.other, [InteractiveWindowMessages.ExecutionRendered]: MessageType.other, [InteractiveWindowMessages.ExpandAll]: MessageType.syncWithLiveShare, [InteractiveWindowMessages.Export]: MessageType.other, @@ -97,7 +95,6 @@ const messageWithMessageTypes: MessageMapping & Messa [InteractiveWindowMessages.GetVariablesResponse]: MessageType.other, [InteractiveWindowMessages.GotoCodeCell]: MessageType.syncWithLiveShare, [InteractiveWindowMessages.GotoCodeCell]: MessageType.syncWithLiveShare, - [InteractiveWindowMessages.InsertCell]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, [InteractiveWindowMessages.Interrupt]: MessageType.other, [InteractiveWindowMessages.LoadAllCells]: MessageType.other, [InteractiveWindowMessages.LoadAllCellsComplete]: MessageType.other, @@ -124,8 +121,8 @@ const messageWithMessageTypes: MessageMapping & Messa [InteractiveWindowMessages.ReExecuteCell]: MessageType.other, [InteractiveWindowMessages.Redo]: MessageType.other, [InteractiveWindowMessages.RemoteAddCode]: MessageType.other, + [InteractiveWindowMessages.ReceivedUpdateModel]: MessageType.other, [InteractiveWindowMessages.RemoteReexecuteCode]: MessageType.other, - [InteractiveWindowMessages.RemoveCell]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, [InteractiveWindowMessages.ResolveCompletionItemRequest]: MessageType.other, [InteractiveWindowMessages.ResolveCompletionItemResponse]: MessageType.other, [InteractiveWindowMessages.RestartKernel]: MessageType.other, @@ -146,11 +143,11 @@ const messageWithMessageTypes: MessageMapping & Messa [InteractiveWindowMessages.StopDebugging]: MessageType.other, [InteractiveWindowMessages.StopProgress]: MessageType.other, [InteractiveWindowMessages.SubmitNewCell]: MessageType.other, - [InteractiveWindowMessages.SwapCells]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, [InteractiveWindowMessages.Sync]: MessageType.other, [InteractiveWindowMessages.Undo]: MessageType.other, [InteractiveWindowMessages.UnfocusedCellEditor]: MessageType.syncWithLiveShare, [InteractiveWindowMessages.UpdateCell]: MessageType.other, + [InteractiveWindowMessages.UpdateModel]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, [InteractiveWindowMessages.UpdateKernel]: MessageType.other, [InteractiveWindowMessages.VariableExplorerToggle]: MessageType.other, [InteractiveWindowMessages.VariablesComplete]: MessageType.other, diff --git a/src/datascience-ui/history-react/redux/reducers/index.ts b/src/datascience-ui/history-react/redux/reducers/index.ts index d82b8b200a12..f1c53b4b03d5 100644 --- a/src/datascience-ui/history-react/redux/reducers/index.ts +++ b/src/datascience-ui/history-react/redux/reducers/index.ts @@ -22,7 +22,7 @@ export const reducerMap: Partial = { [CommonActionType.EXPORT]: Transfer.exportCells, [CommonActionType.SAVE]: Transfer.save, [CommonActionType.SHOW_DATA_VIEWER]: Transfer.showDataViewer, - [InteractiveWindowMessages.DeleteCell]: Creation.deleteCell, + [CommonActionType.DELETE_CELL]: Creation.deleteCell, [InteractiveWindowMessages.ShowPlot]: Transfer.showPlot, [CommonActionType.LINK_CLICK]: Transfer.linkClick, [CommonActionType.GOTO_CELL]: Transfer.gotoCell, diff --git a/src/datascience-ui/interactive-common/redux/store.ts b/src/datascience-ui/interactive-common/redux/store.ts index b28b65997e3b..25a9633be08b 100644 --- a/src/datascience-ui/interactive-common/redux/store.ts +++ b/src/datascience-ui/interactive-common/redux/store.ts @@ -154,7 +154,7 @@ function createTestMiddleware(): Redux.Middleware<{}, IStore> { } // Indicate update from extension side - if (action.type && action.type === IncomingMessageActions.UPDATEMODEL) { + if (action.type && action.type === InteractiveWindowMessages.UpdateModel) { sendMessage(InteractiveWindowMessages.ReceivedUpdateModel); } From d53f44c2fa4ce203690baef2d02d59a1953501ef Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 6 Feb 2020 09:54:09 -0800 Subject: [PATCH 17/23] Add news entry --- news/1 Enhancements/9821.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/1 Enhancements/9821.md diff --git a/news/1 Enhancements/9821.md b/news/1 Enhancements/9821.md new file mode 100644 index 000000000000..0e7967ac65ba --- /dev/null +++ b/news/1 Enhancements/9821.md @@ -0,0 +1 @@ +Add undo/redo support to notebooks. \ No newline at end of file From 9d62143295a3d1874ac7b0abb7b093c94fe0536f Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 6 Feb 2020 10:07:34 -0800 Subject: [PATCH 18/23] Put back launch.json --- .vscode/launch.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 721b8c645555..e972d37c7bad 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -187,7 +187,7 @@ "--ui=tdd", "--recursive", "--colors", - "--grep", "Edit/delete cells", + //"--grep", "", "--timeout=300000" ], "outFiles": [ From 5b87c06e3c9d1d571661a94316b78a985c8b987d Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 6 Feb 2020 10:10:19 -0800 Subject: [PATCH 19/23] Get rid of asyncness of sendCellsToWebView --- src/client/datascience/interactive-ipynb/nativeEditor.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/datascience/interactive-ipynb/nativeEditor.ts b/src/client/datascience/interactive-ipynb/nativeEditor.ts index eaecf1c01b46..53aa4114f8c1 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditor.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditor.ts @@ -306,7 +306,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { line: 0, state: CellState.error } - ]).ignoreErrors(); + ]); // Tell the other side we restarted the kernel. This will stop all executions this.postMessage(InteractiveWindowMessages.RestartKernel).ignoreErrors(); @@ -331,7 +331,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { } } - protected async sendCellsToWebView(cells: ICell[]) { + protected sendCellsToWebView(cells: ICell[]) { // Filter out sysinfo messages. Don't want to show those const filtered = cells.filter(c => c.data.cell_type !== 'messages'); From 367e476b398e8a440b3f198c74b951cbe9163585 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 7 Feb 2020 10:09:05 -0800 Subject: [PATCH 20/23] Add a comment about why we remove \r --- src/datascience-ui/react-common/monacoEditor.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/datascience-ui/react-common/monacoEditor.tsx b/src/datascience-ui/react-common/monacoEditor.tsx index a8194fa869bc..fed0756b0bd0 100644 --- a/src/datascience-ui/react-common/monacoEditor.tsx +++ b/src/datascience-ui/react-common/monacoEditor.tsx @@ -373,6 +373,8 @@ export class MonacoEditor extends React.Component Date: Fri, 7 Feb 2020 10:10:40 -0800 Subject: [PATCH 21/23] Prevent swapCells from doing anything if same cell --- src/datascience-ui/native-editor/redux/reducers/movement.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/datascience-ui/native-editor/redux/reducers/movement.ts b/src/datascience-ui/native-editor/redux/reducers/movement.ts index d1d518af7f0f..94492d2faf1d 100644 --- a/src/datascience-ui/native-editor/redux/reducers/movement.ts +++ b/src/datascience-ui/native-editor/redux/reducers/movement.ts @@ -13,7 +13,7 @@ export namespace Movement { const newVMs = [...arg.prevState.cellVMs]; const first = newVMs.findIndex(cvm => cvm.cell.id === arg.payload.data.firstCellId); const second = newVMs.findIndex(cvm => cvm.cell.id === arg.payload.data.secondCellId); - if (first >= 0 && second >= 0) { + if (first >= 0 && second >= 0 && first !== second) { const temp = newVMs[first]; newVMs[first] = newVMs[second]; newVMs[second] = temp; From 902a1674aba44b84457fa997628639c0e88b20e1 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 7 Feb 2020 10:31:07 -0800 Subject: [PATCH 22/23] Fix changed event for clear --- .../datascience/interactive-ipynb/nativeEditorStorage.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 4133a2f2c7c1..106cca801770 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -1,4 +1,5 @@ import { nbformat } from '@jupyterlab/coreutils'; +import * as fastDeepEqual from 'fast-deep-equal'; import { inject, injectable, named } from 'inversify'; import * as path from 'path'; import * as uuid from 'uuid/v4'; @@ -163,8 +164,8 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { let changed = false; switch (change.kind) { case 'clear': + changed = !fastDeepEqual(this._state.cells, change.oldCells); this._state.cells = change.oldCells; - changed = true; break; case 'edit': this.editCell(change.reverse, change.id); @@ -257,8 +258,10 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { } private clearOutputs(): boolean { - this._state.cells = this.cells.map(c => this.asCell({ ...c, data: { ...c.data, execution_count: null, outputs: [] } })); - return true; + const newCells = this.cells.map(c => this.asCell({ ...c, data: { ...c.data, execution_count: null, outputs: [] } })); + const result = !fastDeepEqual(newCells, this.cells); + this._state.cells = newCells; + return result; } private insertCell(cell: ICell, index: number): boolean { From 146c76b293c1a13a6dd2a1f66d97128b026caab6 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 7 Feb 2020 13:47:54 -0800 Subject: [PATCH 23/23] Actually remove all state from editor class. Try to fix undo on save --- .../interactive-ipynb/nativeEditorStorage.ts | 59 ++++++++++--------- .../interactive-common/editor.tsx | 29 +-------- 2 files changed, 32 insertions(+), 56 deletions(-) diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 106cca801770..bc34569ae631 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -26,14 +26,14 @@ const NotebookTransferKey = 'notebook-transfered'; interface INativeEditorStorageState { file: Uri; cells: ICell[]; - isDirty: boolean; + changeCountSinceSave: number; notebookJson: Partial; } @injectable() export class NativeEditorStorage implements INotebookModel, INotebookStorage { public get isDirty(): boolean { - return this._state.isDirty; + return this._state.changeCountSinceSave > 0; } public get changed(): Event { return this._changedEmitter.event; @@ -49,8 +49,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { return this._state.cells; } private _changedEmitter = new EventEmitter(); - private _state: INativeEditorStorageState = { file: Uri.file(''), isDirty: false, cells: [], notebookJson: {} }; - private _loadPromise: Promise | undefined; + private _state: INativeEditorStorageState = { file: Uri.file(''), changeCountSinceSave: 0, cells: [], notebookJson: {} }; private indentAmount: string = ' '; constructor( @@ -63,9 +62,8 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { ) {} public async load(file: Uri, possibleContents?: string): Promise { - // Reset the load promise and reload our cells - this._loadPromise = this.loadFromFile(file, possibleContents); - await this._loadPromise; + // Reload our cells + await this.loadFromFile(file, possibleContents); return this; } @@ -112,7 +110,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { } // Forward onto our listeners if necessary - if (changed) { + if (changed || this.isDirty !== oldDirty) { this._changedEmitter.fire({ ...change, newDirty: this.isDirty, oldDirty }); } } @@ -145,18 +143,20 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { this.updateVersionInfo(change.interpreter, change.kernelSpec); break; case 'file': + changed = !this.fileSystem.arePathsSame(this._state.file.fsPath, change.newFile.fsPath); this._state.file = change.newFile; - this._state.isDirty = false; - - // Special case for file, don't set dirty (as we're saving), but still - // indicate changed. - return true; + this._state.changeCountSinceSave = 0; + break; default: break; } - if (changed) { - this._state.isDirty = true; + + // Dirty state comes from undo. At least VS code will track it that way. However + // skip version and file changes as we don't forward those to VS code + if (change.kind !== 'file' && change.kind !== 'version') { + this._state.changeCountSinceSave += 1; } + return changed; } @@ -191,8 +191,11 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { break; } - // Dirty state comes from undo - this._state.isDirty = change.oldDirty; + // Dirty state comes from undo. At least VS code will track it that way. + // Note unlike redo, 'file' and 'version' are not possible on undo as + // we don't send them to VS code. + this._state.changeCountSinceSave -= 1; + return changed; } @@ -234,7 +237,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { private swapCells(firstCellId: string, secondCellId: string) { const first = this.cells.findIndex(v => v.id === firstCellId); const second = this.cells.findIndex(v => v.id === secondCellId); - if (first >= 0 && second >= 0) { + if (first >= 0 && second >= 0 && first !== second) { const temp = { ...this.cells[first] }; this._state.cells[first] = this.asCell(this.cells[second]); this._state.cells[second] = this.asCell(temp); @@ -253,8 +256,12 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { } private removeCell(cell: ICell): boolean { - this._state.cells = this.cells.filter(c => c.id !== cell.id); - return true; + const index = this.cells.findIndex(c => c.id === cell.id); + if (index >= 0) { + this._state.cells.splice(index, 1); + return true; + } + return false; } private clearOutputs(): boolean { @@ -295,7 +302,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { return cell as ICell; } - private async loadFromFile(file: Uri, possibleContents?: string): Promise { + private async loadFromFile(file: Uri, possibleContents?: string) { // Save file this._state.file = file; @@ -307,10 +314,10 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { const dirtyContents = await this.getStoredContents(); if (dirtyContents) { // This means we're dirty. Indicate dirty and load from this content - return this.loadContents(dirtyContents, true); + this.loadContents(dirtyContents); } else { // Load without setting dirty - return this.loadContents(contents, false); + this.loadContents(contents); } } catch { // May not exist at this time. Should always have a single cell though @@ -328,7 +335,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { }; } - private async loadContents(contents: string | undefined, forceDirty: boolean): Promise { + private loadContents(contents: string | undefined) { // tslint:disable-next-line: no-any const json = contents ? (JSON.parse(contents) as Partial) : undefined; @@ -364,14 +371,10 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { // Make sure at least one if (remapped.length === 0) { remapped.splice(0, 0, this.createEmptyCell(uuid())); - forceDirty = true; } // Save as our visible list this._state.cells = remapped; - this._state.isDirty = forceDirty; - - return this.cells; } private async extractPythonMainVersion(notebookData: Partial): Promise { diff --git a/src/datascience-ui/interactive-common/editor.tsx b/src/datascience-ui/interactive-common/editor.tsx index b85343e526eb..bf13ed3a488e 100644 --- a/src/datascience-ui/interactive-common/editor.tsx +++ b/src/datascience-ui/interactive-common/editor.tsx @@ -46,7 +46,6 @@ export class Editor extends React.Component { constructor(prop: IEditorProps) { super(prop); - this.state = { editor: undefined, model: null, forceMonaco: false }; } public componentWillUnmount = () => { @@ -61,7 +60,7 @@ export class Editor extends React.Component { public render() { const classes = this.props.readOnly ? 'editor-area' : 'editor-area editor-area-editable'; - const renderEditor = this.props.useQuickEdit === undefined || this.props.useQuickEdit === false ? this.renderMonacoEditor : this.renderQuickEditor; + const renderEditor = this.renderMonacoEditor; return
{renderEditor()}
; } @@ -78,20 +77,6 @@ export class Editor extends React.Component { return ''; } - private renderQuickEditor = (): JSX.Element => { - const readOnly = this.props.readOnly; - return ( -