From 5132e620453296be8da6ce3554eb0d541e60379a Mon Sep 17 00:00:00 2001 From: Kevin Cunnane Date: Fri, 1 Feb 2019 10:11:45 -0800 Subject: [PATCH] Fix #3734 Notebook cells are shown empty some times even when there is content (#3878) - Editor layout gets called sometimes when other events happen (and Notebook isn't visible) - Add in a layout call on re-setting input so the cell is updated. This fixes the problem by laying out once the UI is visible again. Note: long term, should really be destroying the UI (while preserving the model), then restoring it including scroll selection etc. and hooking back up to the model. That is... much more work, but something we'll need long term to avoid issues where we have many Notebooks open at once. Not in scope for this PR --- src/sql/parts/notebook/cellViews/code.component.ts | 1 + src/sql/parts/notebook/models/modelInterfaces.ts | 7 +++++++ src/sql/parts/notebook/models/notebookModel.ts | 14 +++++++++++--- src/sql/parts/notebook/notebook.component.ts | 1 + src/sql/parts/notebook/notebookEditor.ts | 13 ++++++++----- src/sql/parts/notebook/notebookInput.ts | 11 +++++++++-- src/sqltest/parts/notebook/common.ts | 6 +++++- .../parts/notebook/model/notebookModel.test.ts | 1 + 8 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/sql/parts/notebook/cellViews/code.component.ts b/src/sql/parts/notebook/cellViews/code.component.ts index b8421e40b5..887f2c11be 100644 --- a/src/sql/parts/notebook/cellViews/code.component.ts +++ b/src/sql/parts/notebook/cellViews/code.component.ts @@ -142,6 +142,7 @@ export class CodeComponent extends AngularDisposable implements OnInit, OnChange this.cellModel.source = this._editorModel.getValue(); this.onContentChanged.emit(); })); + this._register(this.model.layoutChanged(this.layout, this)); this.layout(); } diff --git a/src/sql/parts/notebook/models/modelInterfaces.ts b/src/sql/parts/notebook/models/modelInterfaces.ts index 07c2a9cb4e..a543f5aac6 100644 --- a/src/sql/parts/notebook/models/modelInterfaces.ts +++ b/src/sql/parts/notebook/models/modelInterfaces.ts @@ -276,6 +276,11 @@ export interface INotebookModel { */ readonly kernelChanged: Event; + /** + * Fired on notifications that notebook components should be re-laid out. + */ + readonly layoutChanged: Event; + /** * Event fired on first initialization of the kernels and * on subsequent change events @@ -443,6 +448,8 @@ export interface INotebookModelOptions { standardKernels: IStandardKernelWithProvider[]; defaultKernel: nb.IKernelSpec; + layoutChanged: Event; + notificationService: INotificationService; connectionService: IConnectionManagementService; capabilitiesService: ICapabilitiesService; diff --git a/src/sql/parts/notebook/models/notebookModel.ts b/src/sql/parts/notebook/models/notebookModel.ts index 2b721f2dfc..c9d5367021 100644 --- a/src/sql/parts/notebook/models/notebookModel.ts +++ b/src/sql/parts/notebook/models/notebookModel.ts @@ -44,6 +44,7 @@ export class NotebookModel extends Disposable implements INotebookModel { private _contextsChangedEmitter = new Emitter(); private _contentChangedEmitter = new Emitter(); private _kernelsChangedEmitter = new Emitter(); + private _layoutChanged = new Emitter(); private _inErrorState: boolean = false; private _clientSessions: IClientSession[] = []; private _activeClientSession: IClientSession; @@ -55,7 +56,7 @@ export class NotebookModel extends Disposable implements INotebookModel { private _cells: ICellModel[]; private _defaultLanguageInfo: nb.ILanguageInfo; - private onErrorEmitter = new Emitter(); + private _onErrorEmitter = new Emitter(); private _savedKernelInfo: nb.IKernelInfo; private readonly _nbformat: number = nbversion.MAJOR_VERSION; private readonly _nbformatMinor: number = nbversion.MINOR_VERSION; @@ -81,6 +82,9 @@ export class NotebookModel extends Disposable implements INotebookModel { this._kernelDisplayNameToConnectionProviderIds.set(kernel.name, kernel.connectionProviderIds); this._kernelDisplayNameToNotebookProviderIds.set(kernel.name, kernel.notebookProvider); }); + if (this.notebookOptions.layoutChanged) { + this.notebookOptions.layoutChanged(() => this._layoutChanged.fire()); + } this._defaultKernel = notebookOptions.defaultKernel; } @@ -139,6 +143,10 @@ export class NotebookModel extends Disposable implements INotebookModel { return this._kernelsChangedEmitter.event; } + public get layoutChanged(): Event { + return this._layoutChanged.event; + } + public get defaultKernel(): nb.IKernelSpec { return this._defaultKernel; } @@ -178,7 +186,7 @@ export class NotebookModel extends Disposable implements INotebookModel { } public get onError(): Event { - return this.onErrorEmitter.event; + return this._onErrorEmitter.event; } public get trustedMode(): boolean { @@ -348,7 +356,7 @@ export class NotebookModel extends Disposable implements INotebookModel { } private notifyError(error: string): void { - this.onErrorEmitter.fire({ message: error, severity: Severity.Error }); + this._onErrorEmitter.fire({ message: error, severity: Severity.Error }); } public backgroundStartSession(): void { diff --git a/src/sql/parts/notebook/notebook.component.ts b/src/sql/parts/notebook/notebook.component.ts index 95b218f3f0..9763b8a3e9 100644 --- a/src/sql/parts/notebook/notebook.component.ts +++ b/src/sql/parts/notebook/notebook.component.ts @@ -250,6 +250,7 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe standardKernels: this._notebookParams.input.standardKernels, providerId: notebookUtils.sqlNotebooksEnabled(this.contextKeyService) ? 'sql' : 'jupyter', // this is tricky; really should also depend on the connection profile defaultKernel: this._notebookParams.input.defaultKernel, + layoutChanged: this._notebookParams.input.layoutChanged, capabilitiesService: this.capabilitiesService }, false, this.profile); model.onError((errInfo: INotification) => this.handleModelError(errInfo)); diff --git a/src/sql/parts/notebook/notebookEditor.ts b/src/sql/parts/notebook/notebookEditor.ts index 13c0cc04f9..906966598f 100644 --- a/src/sql/parts/notebook/notebookEditor.ts +++ b/src/sql/parts/notebook/notebookEditor.ts @@ -22,7 +22,6 @@ export class NotebookEditor extends BaseEditor { public static ID: string = 'workbench.editor.notebookEditor'; private _notebookContainer: HTMLElement; - protected _input: NotebookInput; constructor( @ITelemetryService telemetryService: ITelemetryService, @@ -32,8 +31,8 @@ export class NotebookEditor extends BaseEditor { super(NotebookEditor.ID, telemetryService, themeService); } - public get input(): NotebookInput { - return this._input; + public get notebookInput(): NotebookInput { + return this.input as NotebookInput; } /** @@ -53,6 +52,9 @@ export class NotebookEditor extends BaseEditor { * To be called when the container of this editor changes size. */ public layout(dimension: DOM.Dimension): void { + if (this.notebookInput) { + this.notebookInput.doChangeLayout(); + } } public setInput(input: NotebookInput, options: EditorOptions): TPromise { @@ -70,10 +72,11 @@ export class NotebookEditor extends BaseEditor { let container = DOM.$('.notebookEditor'); container.style.height = '100%'; this._notebookContainer = DOM.append(parentElement, container); - this.input.container = this._notebookContainer; + input.container = this._notebookContainer; return TPromise.wrap(this.bootstrapAngular(input)); } else { - this._notebookContainer = DOM.append(parentElement, this.input.container); + this._notebookContainer = DOM.append(parentElement, input.container); + input.doChangeLayout(); return TPromise.wrap(null); } } diff --git a/src/sql/parts/notebook/notebookInput.ts b/src/sql/parts/notebook/notebookInput.ts index d35b5457e4..03ddcfe051 100644 --- a/src/sql/parts/notebook/notebookInput.ts +++ b/src/sql/parts/notebook/notebookInput.ts @@ -104,13 +104,12 @@ export class NotebookInputModel extends EditorModel { export class NotebookInput extends EditorInput { - public static ID: string = 'workbench.editorinputs.notebookInput'; public hasBootstrapped = false; // Holds the HTML content for the editor when the editor discards this input and loads another private _parentContainer: HTMLElement; - + private readonly _layoutChanged: Emitter = this._register(new Emitter()); constructor(private _title: string, private _model: NotebookInputModel, @INotebookService private notebookService: INotebookService, @@ -140,6 +139,14 @@ export class NotebookInput extends EditorInput { return this._model.defaultKernel; } + get layoutChanged(): Event { + return this._layoutChanged.event; + } + + doChangeLayout(): any { + this._layoutChanged.fire(); + } + public getTypeId(): string { return NotebookInput.ID; } diff --git a/src/sqltest/parts/notebook/common.ts b/src/sqltest/parts/notebook/common.ts index be189ab7fe..640cf0f206 100644 --- a/src/sqltest/parts/notebook/common.ts +++ b/src/sqltest/parts/notebook/common.ts @@ -38,7 +38,11 @@ export class NotebookModelStub implements INotebookModel { } get kernelsChanged(): Event { throw new Error('method not implemented.'); - } get defaultKernel(): nb.IKernelSpec { + } + get layoutChanged(): Event { + throw new Error('method not implemented.'); + } + get defaultKernel(): nb.IKernelSpec { throw new Error('method not implemented.'); } get contextsChanged(): Event { diff --git a/src/sqltest/parts/notebook/model/notebookModel.test.ts b/src/sqltest/parts/notebook/model/notebookModel.test.ts index 4efd49beeb..b9c1c0c4f0 100644 --- a/src/sqltest/parts/notebook/model/notebookModel.test.ts +++ b/src/sqltest/parts/notebook/model/notebookModel.test.ts @@ -97,6 +97,7 @@ describe('notebook model', function(): void { providerId: 'SQL', standardKernels: [{ name: 'SQL', connectionProviderIds: ['MSSQL'], notebookProvider: 'sql' }], defaultKernel: undefined, + layoutChanged: undefined, capabilitiesService: capabilitiesService.object }; mockClientSession = TypeMoq.Mock.ofType(ClientSession, undefined, defaultModelOptions);