From 6200a61382b053bffbbc3124ed119cef85d48625 Mon Sep 17 00:00:00 2001 From: Daniel Grajeda Date: Fri, 22 Oct 2021 15:50:21 -0600 Subject: [PATCH] Notebook Views initialization fix (#17109) Separate the Views load from the initialization. This way we can load previously created views, and only add the new views data to the document when needed. For now, this happens only when a view is created. --- .../browser/notebookEditor.component.ts | 2 +- .../test/browser/notebookViewModel.test.ts | 2 +- .../browser/notebookViewsExtension.test.ts | 13 ++++ .../browser/models/notebookExtension.ts | 19 +++-- .../notebookViews/notebookViewModel.ts | 6 +- .../notebookViews/notebookViewsExtension.ts | 71 +++++++++++++------ 6 files changed, 80 insertions(+), 33 deletions(-) diff --git a/src/sql/workbench/contrib/notebook/browser/notebookEditor.component.ts b/src/sql/workbench/contrib/notebook/browser/notebookEditor.component.ts index 8cf8c90512..15a5327705 100644 --- a/src/sql/workbench/contrib/notebook/browser/notebookEditor.component.ts +++ b/src/sql/workbench/contrib/notebook/browser/notebookEditor.component.ts @@ -46,7 +46,7 @@ export class NotebookEditorComponent extends AngularDisposable { public views: NotebookViewsExtension; public activeView: INotebookView; public viewMode: ViewMode; - public ViewMode = ViewMode; + public ViewMode = ViewMode; //For use of the enum in the template constructor( @Inject(ILogService) private readonly logService: ILogService, diff --git a/src/sql/workbench/contrib/notebook/test/browser/notebookViewModel.test.ts b/src/sql/workbench/contrib/notebook/test/browser/notebookViewModel.test.ts index 9eb229415b..1f40f6eb92 100644 --- a/src/sql/workbench/contrib/notebook/test/browser/notebookViewModel.test.ts +++ b/src/sql/workbench/contrib/notebook/test/browser/notebookViewModel.test.ts @@ -208,7 +208,7 @@ suite('NotebookViewModel', function (): void { viewModel.initialize(); let cell = viewModel.cells[0]; - let cellMeta = notebookViews.getCellMetadata(cell); + let cellMeta = notebookViews.getExtensionCellMetadata(cell); assert(!isUndefinedOrNull(cellMeta.views.find(v => v.guid === viewModel.guid))); assert.deepStrictEqual(viewModel.getCellMetadata(cell), cellMeta.views.find(v => v.guid === viewModel.guid)); diff --git a/src/sql/workbench/contrib/notebook/test/browser/notebookViewsExtension.test.ts b/src/sql/workbench/contrib/notebook/test/browser/notebookViewsExtension.test.ts index 38d3864833..d9f6ac696f 100644 --- a/src/sql/workbench/contrib/notebook/test/browser/notebookViewsExtension.test.ts +++ b/src/sql/workbench/contrib/notebook/test/browser/notebookViewsExtension.test.ts @@ -76,6 +76,19 @@ suite('NotebookViews', function (): void { notebookViews = await initializeExtension(); }); + test('should not modify the notebook document until a view is created', async () => { + //Create some content + notebookViews.notebook.addCell(CellTypes.Code, 0); + const cell = notebookViews.notebook.cells[0]; + + assert.strictEqual(notebookViews.getExtensionMetadata(), undefined); + assert.strictEqual(notebookViews.getExtensionCellMetadata(cell), undefined); + + //Check that the view is created + notebookViews.createNewView(defaultViewName); + assert.notStrictEqual(notebookViews.getExtensionMetadata(), undefined); + }); + test('create new view', async function (): Promise { assert.strictEqual(notebookViews.getViews().length, 0, 'notebook should not initially generate any views'); diff --git a/src/sql/workbench/services/notebook/browser/models/notebookExtension.ts b/src/sql/workbench/services/notebook/browser/models/notebookExtension.ts index 5c30ae475a..c8ef461edf 100644 --- a/src/sql/workbench/services/notebook/browser/models/notebookExtension.ts +++ b/src/sql/workbench/services/notebook/browser/models/notebookExtension.ts @@ -9,27 +9,36 @@ import { deepClone } from 'vs/base/common/objects'; export class NotebookExtension { readonly version = 1; - readonly extensionName = 'azuredatastudio'; readonly extensionNamespace = 'extensions'; - public getNotebookMetadata(notebook: INotebookModel): TNotebookMeta { + private _extensionName: string; + + public constructor(extensionName: string) { + this._extensionName = extensionName; + } + + public get extensionName(): string { + return this._extensionName; + } + + public getExtensionMetadata(notebook: INotebookModel): TNotebookMeta { const metadata = notebook.getMetaValue(this.extensionNamespace) || {}; return metadata[this.extensionName] as TNotebookMeta; } - public setNotebookMetadata(notebook: INotebookModel, metadata: TNotebookMeta) { + public setExtensionMetadata(notebook: INotebookModel, metadata: TNotebookMeta) { const meta = {}; meta[this.extensionName] = metadata; notebook.setMetaValue(this.extensionNamespace, meta); notebook.serializationStateChanged(NotebookChangeType.MetadataChanged); } - public getCellMetadata(cell: ICellModel): TCellMeta { + public getExtensionCellMetadata(cell: ICellModel): TCellMeta { const namespaceMeta = cell.metadata[this.extensionNamespace] || {}; return namespaceMeta[this.extensionName] as TCellMeta; } - public setCellMetadata(cell: ICellModel, metadata: TCellMeta) { + public setExtensionCellMetadata(cell: ICellModel, metadata: TCellMeta) { const meta = {}; meta[this.extensionName] = metadata; cell.metadata[this.extensionNamespace] = meta; diff --git a/src/sql/workbench/services/notebook/browser/notebookViews/notebookViewModel.ts b/src/sql/workbench/services/notebook/browser/notebookViews/notebookViewModel.ts index b7cb649afe..fe191e89d1 100644 --- a/src/sql/workbench/services/notebook/browser/notebookViews/notebookViewModel.ts +++ b/src/sql/workbench/services/notebook/browser/notebookViews/notebookViewModel.ts @@ -51,11 +51,11 @@ export class NotebookViewModel implements INotebookView { } protected initializeCell(cell: ICellModel, idx: number) { - let meta = this._notebookViews.getCellMetadata(cell); + let meta = this._notebookViews.getExtensionCellMetadata(cell); if (!meta) { this._notebookViews.initializeCell(cell); - meta = this._notebookViews.getCellMetadata(cell); + meta = this._notebookViews.getExtensionCellMetadata(cell); } // Ensure that we are not duplicting view entries in cell metadata @@ -91,7 +91,7 @@ export class NotebookViewModel implements INotebookView { } public getCellMetadata(cell: ICellModel): INotebookViewCell { - const meta = this._notebookViews.getCellMetadata(cell); + const meta = this._notebookViews.getExtensionCellMetadata(cell); return meta?.views?.find(view => view.guid === this.guid); } diff --git a/src/sql/workbench/services/notebook/browser/notebookViews/notebookViewsExtension.ts b/src/sql/workbench/services/notebook/browser/notebookViews/notebookViewsExtension.ts index 53aed35b8b..b188626d3e 100644 --- a/src/sql/workbench/services/notebook/browser/notebookViews/notebookViewsExtension.ts +++ b/src/sql/workbench/services/notebook/browser/notebookViews/notebookViewsExtension.ts @@ -13,30 +13,40 @@ import { INotebookView, INotebookViewCell, INotebookViewCellMetadata, INotebookV export class NotebookViewsExtension extends NotebookExtension implements INotebookViews { static readonly defaultViewName = localize('notebookView.untitledView', "Untitled View"); + static readonly extension = 'notebookviews'; readonly maxNameIterationAttempts = 100; - readonly extension = 'azuredatastudio'; override readonly version = 1; - protected _metadata: INotebookViewMetadata; + protected _metadata: INotebookViewMetadata | undefined; + private _initialized: boolean = false; private _onViewDeleted = new Emitter(); private _onActiveViewChanged = new Emitter(); constructor(protected _notebook: INotebookModel) { - super(); - this.loadOrInitialize(); + super(NotebookViewsExtension.extension); + this.load(); } - public loadOrInitialize() { - this._metadata = this.getNotebookMetadata(this._notebook); + public load(): void { + this._metadata = this.getExtensionMetadata(); + + if (this._metadata) { + this._metadata.views = this._metadata.views.map(view => NotebookViewModel.load(view.guid, this)); + this._initialized = true; + } + } + + public initialize() { + this._metadata = this.getExtensionMetadata(); if (!this._metadata) { this.initializeNotebook(); this.initializeCells(); this.commit(); - } else { - this._metadata.views = this._metadata.views.map(view => NotebookViewModel.load(view.guid, this)); } + + this._initialized = true; } protected initializeNotebook() { @@ -59,12 +69,17 @@ export class NotebookViewsExtension extends NotebookExtension view.guid === guid); - if (viewToRemove !== -1) { - let removedView = this._metadata.views.splice(viewToRemove, 1); + let viewToRemove = this._metadata?.views.findIndex(view => view.guid === guid); + if (viewToRemove >= 0) { + let removedView = this._metadata?.views.splice(viewToRemove, 1); // Remove view data for each cell if (removedView.length === 1) { this._notebook?.cells.forEach((cell) => { - let meta = this.getCellMetadata(cell); + let meta = this.getExtensionCellMetadata(cell); meta.views = meta.views.filter(x => x.guid !== removedView[0].guid); - this.setCellMetadata(cell, meta); + this.setExtensionCellMetadata(cell, meta); }); } } - if (guid === this._metadata.activeView) { + if (guid === this._metadata?.activeView) { this._metadata.activeView = undefined; } @@ -113,13 +128,13 @@ export class NotebookViewsExtension extends NotebookExtension view.guid === currentView.guid); if (viewToUpdate >= 0) { cellMetadata.views[viewToUpdate] = override ? cellData : { ...cellMetadata.views[viewToUpdate], ...cellData }; - this.setCellMetadata(cell, cellMetadata); + this.setExtensionCellMetadata(cell, cellMetadata); } } } @@ -129,7 +144,7 @@ export class NotebookViewsExtension extends NotebookExtension this.getCellMetadata(cell)); + return this._notebook.cells.map(cell => this.getExtensionCellMetadata(cell)); + } + + public override getExtensionMetadata(): INotebookViewMetadata { + return super.getExtensionMetadata(this._notebook); } public getActiveView(): INotebookView { - return this.getViews().find(view => view.guid === this._metadata.activeView); + return this.getViews().find(view => view.guid === this._metadata?.activeView); } public setActiveView(view: INotebookView) { - this._metadata.activeView = view.guid; - this._onActiveViewChanged.fire(); + if (this._metadata) { + this._metadata.activeView = view.guid; + this._onActiveViewChanged.fire(); + } } public commit() { this._metadata = Object.assign({}, this._metadata); - this.setNotebookMetadata(this._notebook, this._metadata); + this.setExtensionMetadata(this._notebook, this._metadata); } public viewNameIsTaken(name: string): boolean { @@ -165,4 +186,8 @@ export class NotebookViewsExtension extends NotebookExtension { return this._onActiveViewChanged.event; } + + public get initialized(): boolean { + return this._initialized; + } }