Skip to content

Commit aba3a8e

Browse files
rebornixrebornix
authored
Merge pull request microsoft#100545 from microsoft/rebornix/separate-textmodel-selection
Notebook: separate selections and text model
2 parents 88ab7a8 + 8a08b75 commit aba3a8e

23 files changed

Lines changed: 634 additions & 420 deletions

‎extensions/vscode-notebook-tests/src/notebook.test.ts‎

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,32 @@ async function splitEditor() {
5656
await once;
5757
}
5858

59-
suite('API tests', () => {
59+
suite('Notebook API tests', () => {
60+
// test.only('crash', async function () {
61+
// for (let i = 0; i < 200; i++) {
62+
// let resource = vscode.Uri.file(join(vscode.workspace.rootPath || '', './first.vsctestnb'));
63+
// await vscode.commands.executeCommand('vscode.openWith', resource, 'notebookCoreTest');
64+
// await vscode.commands.executeCommand('workbench.action.revertAndCloseActiveEditor');
65+
66+
// resource = vscode.Uri.file(join(vscode.workspace.rootPath || '', './empty.vsctestnb'));
67+
// await vscode.commands.executeCommand('vscode.openWith', resource, 'notebookCoreTest');
68+
// await vscode.commands.executeCommand('workbench.action.revertAndCloseActiveEditor');
69+
// }
70+
// });
71+
72+
// test.only('crash', async function () {
73+
// for (let i = 0; i < 200; i++) {
74+
// let resource = vscode.Uri.file(join(vscode.workspace.rootPath || '', './first.vsctestnb'));
75+
// await vscode.commands.executeCommand('vscode.openWith', resource, 'notebookCoreTest');
76+
// await vscode.commands.executeCommand('workbench.action.files.save');
77+
// await vscode.commands.executeCommand('workbench.action.closeAllEditors');
78+
// resource = vscode.Uri.file(join(vscode.workspace.rootPath || '', './empty.vsctestnb'));
79+
// await vscode.commands.executeCommand('vscode.openWith', resource, 'notebookCoreTest');
80+
// await vscode.commands.executeCommand('workbench.action.files.save');
81+
// await vscode.commands.executeCommand('workbench.action.closeAllEditors');
82+
// }
83+
// });
84+
6085
test('document open/close event', async function () {
6186
const resource = vscode.Uri.file(join(vscode.workspace.rootPath || '', './first.vsctestnb'));
6287
const firstDocumentOpen = getEventOncePromise(vscode.notebook.onDidOpenNotebookDocument);
@@ -222,6 +247,13 @@ suite('API tests', () => {
222247

223248
await vscode.commands.executeCommand('workbench.action.files.save');
224249
await vscode.commands.executeCommand('workbench.action.closeAllEditors');
250+
251+
await vscode.commands.executeCommand('vscode.openWith', resource, 'notebookCoreTest');
252+
const firstEditor = vscode.notebook.activeNotebookEditor;
253+
assert.equal(firstEditor?.document.cells.length, 1);
254+
255+
await vscode.commands.executeCommand('workbench.action.files.save');
256+
await vscode.commands.executeCommand('workbench.action.closeAllEditors');
225257
});
226258

227259
test('notebook editor active/visible', async function () {
@@ -290,7 +322,7 @@ suite('API tests', () => {
290322
assert.equal(cellChangeEventRet.changes[0].items[0], vscode.notebook.activeNotebookEditor!.document.cells[1]);
291323

292324
await vscode.commands.executeCommand('workbench.action.files.save');
293-
await vscode.commands.executeCommand('workbench.action.closeActiveEditor');
325+
await vscode.commands.executeCommand('workbench.action.closeAllEditors');
294326
});
295327

296328
test('initialzation should not emit cell change events.', async function () {
@@ -766,6 +798,9 @@ suite('metadata', () => {
766798
assert.equal(vscode.notebook.activeNotebookEditor!.document.metadata.custom!['testMetadata'] as boolean, false);
767799
assert.equal(vscode.notebook.activeNotebookEditor!.selection?.metadata.custom!['testCellMetadata'] as number, 123);
768800
assert.equal(vscode.notebook.activeNotebookEditor!.selection?.language, 'typescript');
801+
802+
await vscode.commands.executeCommand('workbench.action.files.saveAll');
803+
await vscode.commands.executeCommand('workbench.action.closeAllEditors');
769804
});
770805

771806

@@ -781,6 +816,9 @@ suite('metadata', () => {
781816
const activeCell = vscode.notebook.activeNotebookEditor!.selection;
782817
assert.equal(vscode.notebook.activeNotebookEditor!.document.cells.indexOf(activeCell!), 1);
783818
assert.equal(activeCell?.metadata.custom!['testCellMetadata'] as number, 123);
819+
820+
await vscode.commands.executeCommand('workbench.action.files.saveAll');
821+
await vscode.commands.executeCommand('workbench.action.closeAllEditors');
784822
});
785823
});
786824

@@ -808,7 +846,7 @@ suite('regression', () => {
808846
await vscode.commands.executeCommand('vscode.openWith', resource, 'default');
809847
assert.equal(vscode.window.activeTextEditor?.document.uri.path, resource.path);
810848

811-
await vscode.commands.executeCommand('workbench.action.files.saveAll');
849+
await vscode.commands.executeCommand('workbench.action.revertAndCloseActiveEditor');
812850
await vscode.commands.executeCommand('workbench.action.closeAllEditors');
813851
});
814852

@@ -824,7 +862,7 @@ suite('regression', () => {
824862
assert.notEqual(vscode.notebook.activeNotebookEditor, undefined, 'notebook first');
825863
assert.notEqual(vscode.window.activeTextEditor, undefined);
826864

827-
// await vscode.commands.executeCommand('workbench.action.files.saveAll');
865+
await vscode.commands.executeCommand('workbench.action.revertAndCloseActiveEditor');
828866
await vscode.commands.executeCommand('workbench.action.closeAllEditors');
829867
});
830868

‎src/vs/vscode.proposed.d.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1762,7 +1762,7 @@ declare module 'vscode' {
17621762
resolveNotebook(document: NotebookDocument, webview: NotebookCommunication): Promise<void>;
17631763
saveNotebook(document: NotebookDocument, cancellation: CancellationToken): Promise<void>;
17641764
saveNotebookAs(targetResource: Uri, document: NotebookDocument, cancellation: CancellationToken): Promise<void>;
1765-
readonly onDidChangeNotebook: Event<NotebookDocumentContentChangeEvent>;
1765+
readonly onDidChangeNotebook: Event<NotebookDocumentContentChangeEvent | NotebookDocumentEditEvent>;
17661766
backupNotebook(document: NotebookDocument, context: NotebookDocumentBackupContext, cancellation: CancellationToken): Promise<NotebookDocumentBackup>;
17671767

17681768
kernel?: NotebookKernel;

‎src/vs/workbench/api/browser/mainThreadNotebook.ts‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import * as nls from 'vs/nls';
7+
import * as DOM from 'vs/base/browser/dom';
78
import { extHostNamedCustomer } from 'vs/workbench/api/common/extHostCustomers';
89
import { MainContext, MainThreadNotebookShape, NotebookExtensionDescription, IExtHostContext, ExtHostNotebookShape, ExtHostContext, INotebookDocumentsAndEditorsDelta, INotebookModelAddedData } from '../common/extHost.protocol';
910
import { Disposable, IDisposable, combinedDisposable } from 'vs/base/common/lifecycle';
@@ -40,7 +41,7 @@ export class MainThreadNotebookDocument extends Disposable {
4041
) {
4142
super();
4243

43-
this._textModel = new NotebookTextModel(handle, viewType, supportBackup, uri);
44+
this._textModel = new NotebookTextModel(handle, viewType, supportBackup, uri, undoRedoService);
4445
this._register(this._textModel.onDidModelChangeProxy(e => {
4546
this._proxy.$acceptModelChanged(this.uri, e);
4647
this._proxy.$acceptEditorPropertiesChanged(uri, { selections: { selections: this._textModel.selections }, metadata: null });
@@ -51,9 +52,18 @@ export class MainThreadNotebookDocument extends Disposable {
5152
}));
5253
}
5354

54-
async applyEdit(modelVersionId: number, edits: ICellEditOperation[], emitToExtHost: boolean): Promise<boolean> {
55+
async applyEdit(modelVersionId: number, edits: ICellEditOperation[], emitToExtHost: boolean, synchronous: boolean): Promise<boolean> {
5556
await this.notebookService.transformEditsOutputs(this.textModel, edits);
56-
return this._textModel.$applyEdit(modelVersionId, edits);
57+
if (synchronous) {
58+
return this._textModel.$applyEdit(modelVersionId, edits, emitToExtHost, synchronous);
59+
} else {
60+
return new Promise(resolve => {
61+
this._register(DOM.scheduleAtNextAnimationFrame(() => {
62+
const ret = this._textModel.$applyEdit(modelVersionId, edits, emitToExtHost, true);
63+
resolve(ret);
64+
}));
65+
});
66+
}
5767
}
5868

5969
async spliceNotebookCellOutputs(cellHandle: number, splices: NotebookCellOutputsSplice[]) {
@@ -528,7 +538,7 @@ export class MainThreadNotebookController implements IMainNotebookController {
528538
await mainthreadNotebook.applyEdit(mainthreadNotebook.textModel.versionId, [
529539
{ editType: CellEditType.Delete, count: mainthreadNotebook.textModel.cells.length, index: 0 },
530540
{ editType: CellEditType.Insert, index: 0, cells: data.cells }
531-
], true);
541+
], true, false);
532542
}
533543
return mainthreadNotebook.textModel;
534544
}
@@ -548,7 +558,7 @@ export class MainThreadNotebookController implements IMainNotebookController {
548558
index: 0,
549559
cells: backup.cells || []
550560
}
551-
], false);
561+
], false, true);
552562

553563
// create document in ext host with cells data
554564
await this._mainThreadNotebook.addNotebookDocument({
@@ -625,7 +635,7 @@ export class MainThreadNotebookController implements IMainNotebookController {
625635
let mainthreadNotebook = this._mapping.get(URI.from(resource).toString());
626636

627637
if (mainthreadNotebook) {
628-
return await mainthreadNotebook.applyEdit(modelVersionId, edits, true);
638+
return await mainthreadNotebook.applyEdit(modelVersionId, edits, true, true);
629639
}
630640

631641
return false;

‎src/vs/workbench/contrib/notebook/browser/contrib/coreActions.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -674,7 +674,7 @@ async function moveCell(context: INotebookCellActionContext, direction: 'up' | '
674674

675675
if (result) {
676676
// move cell command only works when the cell container has focus
677-
await context.notebookEditor.focusNotebookCell(context.cell, 'container');
677+
await context.notebookEditor.focusNotebookCell(result, 'container');
678678
}
679679
}
680680

‎src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -210,17 +210,17 @@ export interface INotebookEditor extends IEditor {
210210
/**
211211
* Move a cell up one spot
212212
*/
213-
moveCellUp(cell: ICellViewModel): Promise<boolean>;
213+
moveCellUp(cell: ICellViewModel): Promise<ICellViewModel | null>;
214214

215215
/**
216216
* Move a cell down one spot
217217
*/
218-
moveCellDown(cell: ICellViewModel): Promise<boolean>;
218+
moveCellDown(cell: ICellViewModel): Promise<ICellViewModel | null>;
219219

220220
/**
221221
* Move a cell above or below another cell
222222
*/
223-
moveCell(cell: ICellViewModel, relativeToCell: ICellViewModel, direction: 'above' | 'below'): Promise<boolean>;
223+
moveCell(cell: ICellViewModel, relativeToCell: ICellViewModel, direction: 'above' | 'below'): Promise<ICellViewModel | null>;
224224

225225
/**
226226
* Focus the container of a cell (the monaco editor inside is not focused).
@@ -371,6 +371,7 @@ export interface INotebookEditor extends IEditor {
371371
}
372372

373373
export interface INotebookCellList {
374+
isDisposed: boolean
374375
readonly contextKeyService: IContextKeyService;
375376
elementAt(position: number): ICellViewModel | undefined;
376377
elementHeight(element: ICellViewModel): number;

‎src/vs/workbench/contrib/notebook/browser/notebookEditor.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import * as DOM from 'vs/base/browser/dom';
77
import { CancellationToken } from 'vs/base/common/cancellation';
88
import { Emitter, Event } from 'vs/base/common/event';
9-
import { MutableDisposable, DisposableStore } from 'vs/base/common/lifecycle';
9+
import { DisposableStore } from 'vs/base/common/lifecycle';
1010
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
1111
import { IStorageService } from 'vs/platform/storage/common/storage';
1212
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
@@ -30,7 +30,7 @@ export class NotebookEditor extends BaseEditor {
3030
static readonly ID: string = 'workbench.editor.notebook';
3131

3232
private readonly _editorMemento: IEditorMemento<INotebookEditorViewState>;
33-
private readonly _groupListener = this._register(new MutableDisposable());
33+
private readonly _groupListener = this._register(new DisposableStore());
3434
private readonly _widgetDisposableStore: DisposableStore = new DisposableStore();
3535
private _widget: IBorrowValue<NotebookEditorWidget> = { value: undefined };
3636
private _rootElement!: HTMLElement;
@@ -49,13 +49,13 @@ export class NotebookEditor extends BaseEditor {
4949
@IInstantiationService private readonly instantiationService: IInstantiationService,
5050
@IStorageService storageService: IStorageService,
5151
@IEditorService private readonly _editorService: IEditorService,
52-
@IEditorGroupsService editorGroupService: IEditorGroupsService,
52+
@IEditorGroupsService private readonly _editorGroupService: IEditorGroupsService,
5353
@IEditorDropService private readonly _editorDropService: IEditorDropService,
5454
@INotificationService private readonly _notificationService: INotificationService,
5555
@INotebookEditorWidgetService private readonly _notebookWidgetService: INotebookEditorWidgetService,
5656
) {
5757
super(NotebookEditor.ID, telemetryService, themeService, storageService);
58-
this._editorMemento = this.getEditorMemento<INotebookEditorViewState>(editorGroupService, NOTEBOOK_EDITOR_VIEW_STATE_PREFERENCE_KEY);
58+
this._editorMemento = this.getEditorMemento<INotebookEditorViewState>(_editorGroupService, NOTEBOOK_EDITOR_VIEW_STATE_PREFERENCE_KEY);
5959
}
6060

6161
set viewModel(newModel: NotebookViewModel | undefined) {
@@ -100,7 +100,14 @@ export class NotebookEditor extends BaseEditor {
100100

101101
setEditorVisible(visible: boolean, group: IEditorGroup | undefined): void {
102102
super.setEditorVisible(visible, group);
103-
this._groupListener.value = group?.onWillCloseEditor(e => this._saveEditorViewState(e.editor));
103+
if (group) {
104+
this._groupListener.add(group.onWillCloseEditor(e => this._saveEditorViewState(e.editor)));
105+
this._groupListener.add(group.onDidGroupChange(() => {
106+
if (this._editorGroupService.activeGroup !== group) {
107+
this._widget?.value?.updateEditorFocus();
108+
}
109+
}));
110+
}
104111

105112
if (!visible) {
106113
this._saveEditorViewState(this.input);
@@ -197,6 +204,10 @@ export class NotebookEditor extends BaseEditor {
197204

198205
private _saveEditorViewState(input: IEditorInput | undefined): void {
199206
if (this.group && this._widget.value && input instanceof NotebookEditorInput) {
207+
if (this._widget.value.isDisposed) {
208+
return;
209+
}
210+
200211
const state = this._widget.value.getEditorViewState();
201212
this._editorMemento.saveEditorState(this.group, input.resource, state);
202213
}

0 commit comments

Comments
 (0)