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

@@ -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();
 	}
 

src/sql/parts/notebook/models/modelInterfaces.ts

@@ -276,6 +276,11 @@ export interface INotebookModel {
 	 */
 	readonly kernelChanged: Event<nb.IKernelChangedArgs>;
 
+	/**
+	 * Fired on notifications that notebook components should be re-laid out.
+	 */
+	readonly layoutChanged: Event<void>;
+
 	/**
 	 * 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<void>;
+
 	notificationService: INotificationService;
 	connectionService: IConnectionManagementService;
 	capabilitiesService: ICapabilitiesService;

src/sql/parts/notebook/models/notebookModel.ts

@@ -44,6 +44,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
 	private _contextsChangedEmitter = new Emitter<void>();
 	private _contentChangedEmitter = new Emitter<NotebookContentChange>();
 	private _kernelsChangedEmitter = new Emitter<nb.IKernelSpec>();
+	private _layoutChanged = new Emitter<void>();
 	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<INotification>();
+	private _onErrorEmitter = new Emitter<INotification>();
 	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<void> {
+		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<INotification> {
-		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 {

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));

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 {
+	public get notebookInput(): NotebookInput {
-		return this._input;
+		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<void> {
@@ -70,10 +72,11 @@ export class NotebookEditor extends BaseEditor {
 			let container = DOM.$<HTMLElement>('.notebookEditor');
 			container.style.height = '100%';
 			this._notebookContainer = DOM.append(parentElement, container);
-			this.input.container = this._notebookContainer;
+			input.container = this._notebookContainer;
 			return TPromise.wrap<void>(this.bootstrapAngular(input));
 		} else {
-			this._notebookContainer = DOM.append(parentElement, this.input.container);
+			this._notebookContainer = DOM.append(parentElement, input.container);
+			input.doChangeLayout();
 			return TPromise.wrap<void>(null);
 		}
 	}

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<void> = this._register(new Emitter<void>());
 	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<void> {
+		return this._layoutChanged.event;
+	}
+
+	doChangeLayout(): any {
+		this._layoutChanged.fire();
+	}
+
 	public getTypeId(): string {
 		return NotebookInput.ID;
 	}

src/sqltest/parts/notebook/common.ts

@@ -38,7 +38,11 @@ export class NotebookModelStub implements INotebookModel {
 	}
 	get kernelsChanged(): Event<nb.IKernelSpec> {
 		throw new Error('method not implemented.');
-	} get defaultKernel(): nb.IKernelSpec {
+	}
+	get layoutChanged(): Event<void> {
+		throw new Error('method not implemented.');
+	}
+	get defaultKernel(): nb.IKernelSpec {
 		throw new Error('method not implemented.');
 	}
 	get contextsChanged(): Event<void> {

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);