From 0aa71b5237ee8255b5c37f58ee392469a97714bc Mon Sep 17 00:00:00 2001 From: Kevin Cunnane Date: Tue, 12 Feb 2019 14:26:22 -0800 Subject: [PATCH] Fix kernel name check bug, double-event hooking, and other Notebook issues (#4016) * Fix error where kernel name was compared to itself. This doesn't break anything right now since we happen to have special handling of Python3, and for other kernels they share the same set of supported providers (which is what the check is used for). However it needs fixing for next release. * Fix console error due to queryTextEditor trying to access model before it's ready * Fix issues where notebook model hooked to session events multiple times * Removed calls that weren't needed such as loadActiveContexts (if undefined, does nothing) and passing connection to initialize method --- .../parts/modelComponents/queryTextEditor.ts | 4 ++++ .../parts/notebook/models/modelInterfaces.ts | 2 +- .../parts/notebook/models/notebookModel.ts | 23 ++++++++----------- .../notebook/model/notebookModel.test.ts | 4 +++- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/sql/parts/modelComponents/queryTextEditor.ts b/src/sql/parts/modelComponents/queryTextEditor.ts index 9f878d24bd..abff4ecd90 100644 --- a/src/sql/parts/modelComponents/queryTextEditor.ts +++ b/src/sql/parts/modelComponents/queryTextEditor.ts @@ -131,6 +131,10 @@ export class QueryTextEditor extends BaseTextEditor { this._scrollbarHeight = this._config.editor.viewInfo.scrollbar.horizontalScrollbarSize; } let editorWidgetModel = editorWidget.getModel(); + if (!editorWidgetModel) { + // Not ready yet + return; + } let lineCount = editorWidgetModel.getLineCount(); // Need to also keep track of lines that wrap; if we just keep into account line count, then the editor's height would not be // tall enough and we would need to show a scrollbar. Unfortunately, it looks like there isn't any metadata saved in a ICodeEditor diff --git a/src/sql/parts/notebook/models/modelInterfaces.ts b/src/sql/parts/notebook/models/modelInterfaces.ts index 2931470699..105bbad69e 100644 --- a/src/sql/parts/notebook/models/modelInterfaces.ts +++ b/src/sql/parts/notebook/models/modelInterfaces.ts @@ -135,7 +135,7 @@ export interface IClientSession extends IDisposable { * This will optionally start a session if the kernel preferences * indicate this is desired */ - initialize(connection?: IConnectionProfile): Promise; + initialize(): Promise; /** * Change the current kernel associated with the document. diff --git a/src/sql/parts/notebook/models/notebookModel.ts b/src/sql/parts/notebook/models/notebookModel.ts index efc672eefd..d9dd868cd1 100644 --- a/src/sql/parts/notebook/models/notebookModel.ts +++ b/src/sql/parts/notebook/models/notebookModel.ts @@ -391,15 +391,14 @@ export class NotebookModel extends Disposable implements INotebookModel { this._activeConnection = undefined; } - clientSession.initialize(this._activeConnection); + clientSession.initialize(); this._sessionLoadFinished = clientSession.ready.then(async () => { if (clientSession.isInErrorState) { this.setErrorState(clientSession.errorMessage); } else { this._onClientSessionReady.fire(clientSession); // Once session is loaded, can use the session manager to retrieve useful info - this.loadKernelInfo(); - await this.loadActiveContexts(undefined); + this.loadKernelInfo(clientSession); } }); }); @@ -488,11 +487,12 @@ export class NotebookModel extends Disposable implements INotebookModel { } } - private loadKernelInfo(): void { - this._clientSessions.forEach(clientSession => { - clientSession.onKernelChanging(async (e) => { - await this.loadActiveContexts(e); - }); + private loadKernelInfo(clientSession: IClientSession): void { + clientSession.onKernelChanging(async (e) => { + await this.loadActiveContexts(e); + }); + clientSession.statusChanged(async (session) => { + this._kernelsChangedEmitter.fire(session.kernel); }); if (!this.notebookManager) { return; @@ -503,11 +503,6 @@ export class NotebookModel extends Disposable implements INotebookModel { if (!this._defaultKernel) { this._defaultKernel = NotebookContexts.getDefaultKernel(sessionManager.specs, this.connectionProfile, this._savedKernelInfo); } - this._clientSessions.forEach(clientSession => { - clientSession.statusChanged(async (session) => { - this._kernelsChangedEmitter.fire(session.kernel); - }); - }); let spec = this.getKernelSpecFromDisplayName(this._defaultKernel.display_name); if (spec) { this._defaultKernel = spec; @@ -551,7 +546,7 @@ export class NotebookModel extends Disposable implements INotebookModel { if (!specs || !specs.kernels) { return kernel.name; } - let newKernel = this.notebookManager.sessionManager.specs.kernels.find(kernel => kernel.name === kernel.name); + let newKernel = this.notebookManager.sessionManager.specs.kernels.find(k => k.name === kernel.name); let newKernelDisplayName; if (newKernel) { newKernelDisplayName = newKernel.display_name; diff --git a/src/sqltest/parts/notebook/model/notebookModel.test.ts b/src/sqltest/parts/notebook/model/notebookModel.test.ts index b9c1c0c4f0..1bdf8449db 100644 --- a/src/sqltest/parts/notebook/model/notebookModel.test.ts +++ b/src/sqltest/parts/notebook/model/notebookModel.test.ts @@ -101,7 +101,7 @@ describe('notebook model', function(): void { capabilitiesService: capabilitiesService.object }; mockClientSession = TypeMoq.Mock.ofType(ClientSession, undefined, defaultModelOptions); - mockClientSession.setup(c => c.initialize(TypeMoq.It.isAny())).returns(() => { + mockClientSession.setup(c => c.initialize()).returns(() => { return Promise.resolve(); }); mockClientSession.setup(c => c.ready).returns(() => sessionReady.promise); @@ -203,10 +203,12 @@ describe('notebook model', function(): void { mockContentManager.setup(c => c.getNotebookContents(TypeMoq.It.isAny())).returns(() => Promise.resolve(expectedNotebookContentOneCell)); notebookManagers[0].contentManager = mockContentManager.object; let kernelChangedEmitter: Emitter = new Emitter(); + let statusChangedEmitter: Emitter = new Emitter(); mockClientSession.setup(c => c.isInErrorState).returns(() => false); mockClientSession.setup(c => c.isReady).returns(() => true); mockClientSession.setup(c => c.kernelChanged).returns(() => kernelChangedEmitter.event); + mockClientSession.setup(c => c.statusChanged).returns(() => statusChangedEmitter.event); queryConnectionService.setup(c => c.getActiveConnections(TypeMoq.It.isAny())).returns(() => null);