From bf2555938bdc41f18255e787ab12ed2aa8c0833b Mon Sep 17 00:00:00 2001 From: rebornix Date: Thu, 23 Apr 2020 13:52:34 -0700 Subject: [PATCH] Fix #96007. --- .../src/notebook.test.ts | 17 +++++++------- .../workbench/api/common/extHostNotebook.ts | 20 ++++++++++++---- .../notebook/browser/notebookEditorInput.ts | 4 ++++ .../browser/viewModel/notebookViewModel.ts | 4 +--- .../common/model/notebookTextModel.ts | 23 +++++++++++++++++-- .../contrib/notebook/common/notebookCommon.ts | 16 ++++++++++++- .../api/extHostNotebookConcatDocument.test.ts | 13 ++++++++++- 7 files changed, 76 insertions(+), 21 deletions(-) diff --git a/extensions/vscode-notebook-tests/src/notebook.test.ts b/extensions/vscode-notebook-tests/src/notebook.test.ts index 2c1c96d89fc56..5f5e765144689 100644 --- a/extensions/vscode-notebook-tests/src/notebook.test.ts +++ b/extensions/vscode-notebook-tests/src/notebook.test.ts @@ -100,16 +100,15 @@ suite('notebook workflow', () => { // ---- move up and down ---- // - // await vscode.commands.executeCommand('workbench.notebook.cell.moveDown'); - // await vscode.commands.executeCommand('workbench.notebook.cell.moveDown'); - // activeCell = vscode.notebook.activeNotebookEditor!.selection; - - // assert.equal(vscode.notebook.activeNotebookEditor!.document.cells.indexOf(activeCell!), 2); - // assert.equal(vscode.notebook.activeNotebookEditor!.document.cells[0].source, 'test'); - // assert.equal(vscode.notebook.activeNotebookEditor!.document.cells[1].source, ''); - // assert.equal(vscode.notebook.activeNotebookEditor!.document.cells[2].source, 'test'); - // assert.equal(vscode.notebook.activeNotebookEditor!.document.cells[3].source, ''); + await vscode.commands.executeCommand('workbench.notebook.cell.moveDown'); + await vscode.commands.executeCommand('workbench.notebook.cell.moveDown'); + activeCell = vscode.notebook.activeNotebookEditor!.selection; + assert.equal(vscode.notebook.activeNotebookEditor!.document.cells.indexOf(activeCell!), 2); + assert.equal(vscode.notebook.activeNotebookEditor!.document.cells[0].source, 'test'); + assert.equal(vscode.notebook.activeNotebookEditor!.document.cells[1].source, ''); + assert.equal(vscode.notebook.activeNotebookEditor!.document.cells[2].source, 'test'); + assert.equal(vscode.notebook.activeNotebookEditor!.document.cells[3].source, ''); // ---- ---- // diff --git a/src/vs/workbench/api/common/extHostNotebook.ts b/src/vs/workbench/api/common/extHostNotebook.ts index 5ddeb5d1ef79e..99911194b28cf 100644 --- a/src/vs/workbench/api/common/extHostNotebook.ts +++ b/src/vs/workbench/api/common/extHostNotebook.ts @@ -13,7 +13,7 @@ import { IExtensionDescription } from 'vs/platform/extensions/common/extensions' import { CellKind, CellOutputKind, ExtHostNotebookShape, IMainContext, MainContext, MainThreadNotebookShape, NotebookCellOutputsSplice, MainThreadDocumentsShape, INotebookEditorPropertiesChangeData } from 'vs/workbench/api/common/extHost.protocol'; import { ExtHostCommands } from 'vs/workbench/api/common/extHostCommands'; import { ExtHostDocumentsAndEditors } from 'vs/workbench/api/common/extHostDocumentsAndEditors'; -import { CellEditType, CellUri, diff, ICellEditOperation, ICellInsertEdit, IErrorOutput, INotebookDisplayOrder, INotebookEditData, IOrderedMimeType, IStreamOutput, ITransformedDisplayOutputDto, mimeTypeSupportedByCore, NotebookCellsChangedEvent, NotebookCellsSplice2, sortMimeTypes, ICellDeleteEdit, notebookDocumentMetadataDefaults } from 'vs/workbench/contrib/notebook/common/notebookCommon'; +import { CellEditType, CellUri, diff, ICellEditOperation, ICellInsertEdit, IErrorOutput, INotebookDisplayOrder, INotebookEditData, IOrderedMimeType, IStreamOutput, ITransformedDisplayOutputDto, mimeTypeSupportedByCore, NotebookCellsChangedEvent, NotebookCellsSplice2, sortMimeTypes, ICellDeleteEdit, notebookDocumentMetadataDefaults, NotebookCellsChangeType } from 'vs/workbench/contrib/notebook/common/notebookCommon'; import { Disposable as VSCodeDisposable } from './extHostTypes'; import { CancellationToken } from 'vs/base/common/cancellation'; import { ExtHostDocumentData } from 'vs/workbench/api/common/extHostDocumentData'; @@ -247,7 +247,12 @@ export class ExtHostNotebookDocument extends Disposable implements vscode.Notebo get isDirty() { return false; } accpetModelChanged(event: NotebookCellsChangedEvent) { - this.$spliceNotebookCells(event.changes); + if (event.kind === NotebookCellsChangeType.ModelChange) { + this.$spliceNotebookCells(event.changes); + } else if (event.kind === NotebookCellsChangeType.Move) { + this.$moveCell(event.index, event.newIdx); + } + this._versionId = event.versionId; } @@ -289,6 +294,11 @@ export class ExtHostNotebookDocument extends Disposable implements vscode.Notebo }); } + private $moveCell(index: number, newIdx: number) { + const cells = this.cells.splice(index, 1); + this.cells.splice(newIdx, 0, ...cells); + } + eventuallyUpdateCellOutputs(cell: ExtHostCell, diffs: ISplice[]) { let renderers = new Set(); let outputDtos: NotebookCellOutputsSplice[] = diffs.map(diff => { @@ -600,8 +610,8 @@ export class ExtHostNotebookController implements ExtHostNotebookShape, ExtHostN private readonly _editors = new Map }>(); private readonly _notebookOutputRenderers = new Map(); - private readonly _onDidChangeNotebookDocument = new Emitter<{ document: ExtHostNotebookDocument, changes: NotebookCellsSplice2[] }>(); - readonly onDidChangeNotebookDocument: Event<{ document: ExtHostNotebookDocument, changes: NotebookCellsSplice2[] }> = this._onDidChangeNotebookDocument.event; + private readonly _onDidChangeNotebookDocument = new Emitter<{ document: ExtHostNotebookDocument, changes: NotebookCellsChangedEvent[] }>(); + readonly onDidChangeNotebookDocument: Event<{ document: ExtHostNotebookDocument, changes: NotebookCellsChangedEvent[] }> = this._onDidChangeNotebookDocument.event; private _outputDisplayOrder: INotebookDisplayOrder | undefined; @@ -801,7 +811,7 @@ export class ExtHostNotebookController implements ExtHostNotebookShape, ExtHostN editor.editor.document.accpetModelChanged(event); this._onDidChangeNotebookDocument.fire({ document: editor.editor.document, - changes: event.changes + changes: [event] }); } diff --git a/src/vs/workbench/contrib/notebook/browser/notebookEditorInput.ts b/src/vs/workbench/contrib/notebook/browser/notebookEditorInput.ts index ae20f831ae7f9..9da0738e23e74 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookEditorInput.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookEditorInput.ts @@ -69,6 +69,10 @@ export class NotebookEditorModel extends EditorModel { } } + moveCellToIdx(index: number, newIdx: number) { + this.notebook.moveCellToIdx(index, newIdx); + } + async save(): Promise { if (this._notebook) { this._dirty = false; diff --git a/src/vs/workbench/contrib/notebook/browser/viewModel/notebookViewModel.ts b/src/vs/workbench/contrib/notebook/browser/viewModel/notebookViewModel.ts index 6dba4584473d6..357a99779a1a8 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewModel/notebookViewModel.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewModel/notebookViewModel.ts @@ -664,10 +664,8 @@ export class NotebookViewModel extends Disposable implements EditorFoldingStateD } this.viewCells.splice(index, 1); - this._model.deleteCell(index); - this.viewCells!.splice(newIdx, 0, viewCell); - this._model.insertCell(viewCell.model, newIdx); + this._model.moveCellToIdx(index, newIdx); if (pushedToUndoStack) { this.undoService.pushElement(new MoveCellEdit(this.uri, index, newIdx, { diff --git a/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts b/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts index c702b0dd20b60..c1522c4bb6ddb 100644 --- a/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts +++ b/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts @@ -7,7 +7,7 @@ import { Emitter, Event } from 'vs/base/common/event'; import { Disposable, IDisposable } from 'vs/base/common/lifecycle'; import { URI } from 'vs/base/common/uri'; import { NotebookCellTextModel } from 'vs/workbench/contrib/notebook/common/model/notebookCellTextModel'; -import { INotebookTextModel, NotebookCellOutputsSplice, NotebookCellTextModelSplice, NotebookDocumentMetadata, NotebookCellMetadata, ICellEditOperation, CellEditType, CellUri, ICellInsertEdit, NotebookCellsChangedEvent, CellKind, IOutput, notebookDocumentMetadataDefaults, diff, ICellDeleteEdit } from 'vs/workbench/contrib/notebook/common/notebookCommon'; +import { INotebookTextModel, NotebookCellOutputsSplice, NotebookCellTextModelSplice, NotebookDocumentMetadata, NotebookCellMetadata, ICellEditOperation, CellEditType, CellUri, ICellInsertEdit, NotebookCellsChangedEvent, CellKind, IOutput, notebookDocumentMetadataDefaults, diff, ICellDeleteEdit, NotebookCellsChangeType } from 'vs/workbench/contrib/notebook/common/notebookCommon'; function compareRangesUsingEnds(a: [number, number], b: [number, number]): number { if (a[1] === b[1]) { @@ -192,6 +192,7 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel this._onDidChangeContent.fire(); this._onDidModelChangeProxy.fire({ + kind: NotebookCellsChangeType.ModelChange, versionId: this._versionId, changes: [ [ 0, @@ -228,6 +229,7 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel this._onDidChangeContent.fire(); this._increaseVersionId(); this._onDidModelChangeProxy.fire({ + kind: NotebookCellsChangeType.ModelChange, versionId: this._versionId, changes: [ [ index, @@ -258,7 +260,24 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel this._onDidChangeContent.fire(); this._increaseVersionId(); - this._onDidModelChangeProxy.fire({ versionId: this._versionId, changes: [[index, 1, []]] }); + this._onDidModelChangeProxy.fire({ kind: NotebookCellsChangeType.ModelChange, versionId: this._versionId, changes: [[index, 1, []]] }); + } + + moveCellToIdx(index: number, newIdx: number) { + this.assertIndex(index); + this.assertIndex(newIdx); + + const cells = this.cells.splice(index, 1); + this.cells.splice(newIdx, 0, ...cells); + + this._increaseVersionId(); + this._onDidModelChangeProxy.fire({ kind: NotebookCellsChangeType.Move, versionId: this._versionId, index, newIdx }); + } + + assertIndex(index: number) { + if (index < 0 || index >= this.cells.length) { + throw new Error(`model index out of range ${index}`); + } } // TODO@rebornix should this trigger content change event? diff --git a/src/vs/workbench/contrib/notebook/common/notebookCommon.ts b/src/vs/workbench/contrib/notebook/common/notebookCommon.ts index cdf8e513b160b..8253ce08f344d 100644 --- a/src/vs/workbench/contrib/notebook/common/notebookCommon.ts +++ b/src/vs/workbench/contrib/notebook/common/notebookCommon.ts @@ -215,11 +215,25 @@ export type NotebookCellsSplice2 = [ IMainCellDto[] ]; -export interface NotebookCellsChangedEvent { +export enum NotebookCellsChangeType { + ModelChange = 1, + Move = 2 +} + +export interface NotebookCellsModelChangedEvent { + readonly kind: NotebookCellsChangeType.ModelChange; readonly changes: NotebookCellsSplice2[]; readonly versionId: number; } +export interface NotebookCellsModelMoveEvent { + readonly kind: NotebookCellsChangeType.Move; + readonly index: number; + readonly newIdx: number; + readonly versionId: number; +} + +export type NotebookCellsChangedEvent = NotebookCellsModelChangedEvent | NotebookCellsModelMoveEvent; export enum CellEditType { Insert = 1, Delete = 2 diff --git a/src/vs/workbench/test/browser/api/extHostNotebookConcatDocument.test.ts b/src/vs/workbench/test/browser/api/extHostNotebookConcatDocument.test.ts index c26fdb77f9da9..0b0827adb8aa4 100644 --- a/src/vs/workbench/test/browser/api/extHostNotebookConcatDocument.test.ts +++ b/src/vs/workbench/test/browser/api/extHostNotebookConcatDocument.test.ts @@ -11,7 +11,7 @@ import { NullLogService } from 'vs/platform/log/common/log'; import { ExtHostNotebookConcatDocument } from 'vs/workbench/api/common/extHostNotebookConcatDocument'; import { ExtHostNotebookDocument, ExtHostNotebookController } from 'vs/workbench/api/common/extHostNotebook'; import { URI } from 'vs/base/common/uri'; -import { CellKind, CellUri } from 'vs/workbench/contrib/notebook/common/notebookCommon'; +import { CellKind, CellUri, NotebookCellsChangeType } from 'vs/workbench/contrib/notebook/common/notebookCommon'; import { Position, Location, Range } from 'vs/workbench/api/common/extHostTypes'; import { ExtHostCommands } from 'vs/workbench/api/common/extHostCommands'; import { nullExtensionDescription } from 'vs/workbench/services/extensions/common/extensions'; @@ -51,6 +51,7 @@ suite('NotebookConcatDocument', function () { }); await extHostNotebooks.$resolveNotebook('test', notebookUri); extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: 0, changes: [[0, 0, [{ handle: 0, @@ -104,6 +105,7 @@ suite('NotebookConcatDocument', function () { test('location, position mapping', function () { extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: notebook.versionId + 1, changes: [[0, 0, [{ handle: 1, @@ -142,6 +144,7 @@ suite('NotebookConcatDocument', function () { // UPDATE 1 extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: notebook.versionId + 1, changes: [[0, 0, [{ handle: 1, @@ -163,6 +166,7 @@ suite('NotebookConcatDocument', function () { // UPDATE 2 extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: notebook.versionId + 1, changes: [[1, 0, [{ handle: 2, @@ -185,6 +189,7 @@ suite('NotebookConcatDocument', function () { // UPDATE 3 (remove cell #2 again) extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: notebook.versionId + 1, changes: [[1, 1, []]] }); @@ -202,6 +207,7 @@ suite('NotebookConcatDocument', function () { // UPDATE 1 extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: notebook.versionId + 1, changes: [[0, 0, [{ handle: 1, @@ -264,6 +270,7 @@ suite('NotebookConcatDocument', function () { test('selector', function () { extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: notebook.versionId + 1, changes: [[0, 0, [{ handle: 1, @@ -291,6 +298,7 @@ suite('NotebookConcatDocument', function () { assertLines(barLangDoc, 'barLang-document'); extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: notebook.versionId + 1, changes: [[2, 0, [{ handle: 3, @@ -323,6 +331,7 @@ suite('NotebookConcatDocument', function () { test('offsetAt(position) <-> positionAt(offset)', function () { extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: notebook.versionId + 1, changes: [[0, 0, [{ handle: 1, @@ -373,6 +382,7 @@ suite('NotebookConcatDocument', function () { test('locationAt(position) <-> positionAt(location)', function () { extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: notebook.versionId + 1, changes: [[0, 0, [{ handle: 1, @@ -407,6 +417,7 @@ suite('NotebookConcatDocument', function () { test('getText(range)', function () { extHostNotebooks.$acceptModelChanged(notebookUri, { + kind: NotebookCellsChangeType.ModelChange, versionId: notebook.versionId + 1, changes: [[0, 0, [{ handle: 1,