Fix connection manager loosing track of notebooks

which were not correctly scoped by path by switching away from id_path which was refactored to not contain the actual file name to uri which does contain the file name and path Add missing wait Co-authored-by: krassowski <[email protected]>
jupyter-lsp · Jan 20, 2021 · 60c9525 · 60c9525
1 parent a86b20b
commit 60c9525
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 25 deletions.
diff --git a/atest/04_Interface/Statusbar.robot b/atest/04_Interface/Statusbar.robot
@@ -19,6 +19,19 @@ Statusbar Popup Opens
     Element Should Contain    ${POPOVER}    initialized
     [Teardown]    Clean Up After Working With File    Python.ipynb
 
+Status Changes Between Notebooks
+    Setup Notebook    Python    Python.ipynb
+    Wait Until Fully Initialized
+    Lab Command    New Notebook
+    Wait For Dialog
+    # Kernel selection dialog shows up, accept Python as default kernel
+    Accept Default Dialog Option
+    Element Should Contain    ${STATUSBAR}    Waiting...
+    Wait Until Fully Initialized
+    Switch To Tab    Python.ipynb
+    Wait Until Fully Initialized
+    [Teardown]    Clean Up After Working With File    Python.ipynb
+
 Status Changes Correctly Between Editors
     Prepare File for Editing    Python    status    example.py
     Wait Until Fully Initialized

diff --git a/packages/jupyterlab-lsp/src/adapters/adapter.ts b/packages/jupyterlab-lsp/src/adapters/adapter.ts
@@ -413,7 +413,7 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
 
     // TODO only send the difference, using connection.sendSelectiveChange()
     let connection = this.connection_manager.connections.get(
-      virtual_document.id_path
+      virtual_document.uri
     );
     let adapter = this.adapters.get(virtual_document.id_path);
 
@@ -596,7 +596,7 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
     );
     return {
       document,
-      connection: this.connection_manager.connections.get(document.id_path),
+      connection: this.connection_manager.connections.get(document.uri),
       virtual_position,
       root_position,
       features: this.get_features(document),

diff --git a/packages/jupyterlab-lsp/src/components/statusbar.tsx b/packages/jupyterlab-lsp/src/components/statusbar.tsx
@@ -128,7 +128,7 @@ class LSPPopup extends VDomRenderer<LSPStatus.Model> {
         // TODO: add a config buttons next to the language header
         let list = documents.map((document, i) => {
           let connection = this.model.connection_manager.connections.get(
-            document.id_path
+            document.uri
           );
 
           let status = '';
@@ -457,8 +457,8 @@ export namespace LSPStatus {
       // detected documents with LSP servers available
       let documents_with_servers = new Set<VirtualDocument>();
 
-      detected_documents.forEach((document, id_path) => {
-        let connection = this._connection_manager.connections.get(id_path);
+      detected_documents.forEach((document, uri) => {
+        let connection = this._connection_manager.connections.get(uri);
         if (!connection) {
           absent_documents.add(document);
           return;
@@ -572,7 +572,7 @@ export namespace LSPStatus {
 
     change_adapter(adapter: WidgetAdapter<IDocumentWidget> | null) {
       if (this._adapter != null) {
-        this._adapter.status_message.changed.connect(this._onChange);
+        this._adapter.status_message.changed.disconnect(this._onChange);
       }
 
       if (adapter != null) {
@@ -586,6 +586,9 @@ export namespace LSPStatus {
       return this._connection_manager;
     }
 
+    /**
+     * Note: it is ever only set once, as connection_manager is a singleton.
+     */
     set connection_manager(connection_manager) {
       if (this._connection_manager != null) {
         this._connection_manager.connected.disconnect(this._onChange);

diff --git a/packages/jupyterlab-lsp/src/connection_manager.ts b/packages/jupyterlab-lsp/src/connection_manager.ts
@@ -32,12 +32,13 @@ export interface ISocketConnectionOptions {
 }
 
 /**
- * Each Widget with a document (whether file or a notebook) has its own DocumentConnectionManager
- * (see JupyterLabWidgetAdapter), keeping the virtual document spaces separate if a file is opened twice.
+ * Each Widget with a document (whether file or a notebook) has the same DocumentConnectionManager
+ * (see JupyterLabWidgetAdapter). Using id_path instead of uri led to documents being overwritten
+ * as two identical id_paths could be created for two different notebooks.
  */
 export class DocumentConnectionManager {
-  connections: Map<VirtualDocument.id_path, LSPConnection>;
-  documents: Map<VirtualDocument.id_path, VirtualDocument>;
+  connections: Map<VirtualDocument.uri, LSPConnection>;
+  documents: Map<VirtualDocument.uri, VirtualDocument>;
   initialized: Signal<DocumentConnectionManager, IDocumentConnectionData>;
   connected: Signal<DocumentConnectionManager, IDocumentConnectionData>;
   /**
@@ -53,7 +54,7 @@ export class DocumentConnectionManager {
   closed: Signal<DocumentConnectionManager, IDocumentConnectionData>;
   documents_changed: Signal<
     DocumentConnectionManager,
-    Map<VirtualDocument.id_path, VirtualDocument>
+    Map<VirtualDocument.uri, VirtualDocument>
   >;
   language_server_manager: ILanguageServerManager;
   initial_configurations: TLanguageServerConfigurations;
@@ -83,7 +84,7 @@ export class DocumentConnectionManager {
       this
     );
 
-    this.documents.set(virtual_document.id_path, virtual_document);
+    this.documents.set(virtual_document.uri, virtual_document);
     this.documents_changed.emit(this.documents);
   }
 
@@ -98,7 +99,7 @@ export class DocumentConnectionManager {
       this
     );
 
-    this.documents.delete(virtual_document.id_path);
+    this.documents.delete(virtual_document.uri);
     for (const foreign of virtual_document.foreign_documents.values()) {
       this.disconnect_document_signals(foreign, false);
     }
@@ -111,7 +112,7 @@ export class DocumentConnectionManager {
   on_foreign_document_opened(_host: VirtualDocument, context: IForeignContext) {
     console.log(
       'LSP: ConnectionManager received foreign document: ',
-      context.foreign_document.id_path
+      context.foreign_document.uri
     );
   }
 
@@ -149,7 +150,7 @@ export class DocumentConnectionManager {
 
     // if connecting for the first time, all documents subsequent documents will
     // be re-opened and synced
-    this.connections.set(virtual_document.id_path, connection);
+    this.connections.set(virtual_document.uri, connection);
 
     return connection;
   }
@@ -188,12 +189,12 @@ export class DocumentConnectionManager {
       if (error.message.indexOf('code = 1005') !== -1) {
         console.warn(`LSP: Connection failed for ${connection}`);
         this.forEachDocumentOfConnection(connection, virtual_document => {
-          console.warn('LSP: disconnecting ' + virtual_document.id_path);
+          console.warn('LSP: disconnecting ' + virtual_document.uri);
           this.closed.emit({ connection, virtual_document });
           this.ignored_languages.add(virtual_document.language);
 
           console.warn(
-            `Cancelling further attempts to connect ${virtual_document.id_path} and other documents for this language (no support from the server)`
+            `Cancelling further attempts to connect ${virtual_document.uri} and other documents for this language (no support from the server)`
           );
         });
       } else if (error.message.indexOf('code = 1006') !== -1) {
@@ -230,13 +231,13 @@ export class DocumentConnectionManager {
     callback: (virtual_document: VirtualDocument) => void
   ) {
     for (const [
-      virtual_document_id_path,
+      virtual_document_uri,
       a_connection
     ] of this.connections.entries()) {
       if (connection !== a_connection) {
         continue;
       }
-      callback(this.documents.get(virtual_document_id_path));
+      callback(this.documents.get(virtual_document_uri));
     }
   }
 
@@ -287,20 +288,20 @@ export class DocumentConnectionManager {
       try {
         await until_ready(() => connection.isReady, 200, 200);
       } catch {
-        console.warn(`LSP: Connect timed out for ${virtual_document.id_path}`);
+        console.warn(`LSP: Connect timed out for ${virtual_document.uri}`);
         return;
       }
     }
 
-    console.log('LSP:', document_path, virtual_document.id_path, 'connected.');
+    console.log('LSP:', document_path, virtual_document.uri, 'connected.');
 
     this.connected.emit({ connection, virtual_document });
 
     return connection;
   }
 
   public unregister_document(virtual_document: VirtualDocument) {
-    this.connections.delete(virtual_document.id_path);
+    this.connections.delete(virtual_document.uri);
     this.documents_changed.emit(this.documents);
   }
 }

diff --git a/packages/jupyterlab-lsp/src/features/completion/completion_handler.ts b/packages/jupyterlab-lsp/src/features/completion/completion_handler.ts
@@ -40,7 +40,7 @@ export class LSPConnector
   implements CompletionHandler.ICompletionItemsConnector {
   isDisposed = false;
   private _editor: CodeEditor.IEditor;
-  private _connections: Map<VirtualDocument.id_path, LSPConnection>;
+  private _connections: Map<VirtualDocument.uri, LSPConnection>;
   private _context_connector: ContextConnector;
   private _kernel_connector: KernelConnector;
   private _kernel_and_context_connector: CompletionConnector;
@@ -220,7 +220,7 @@ export class LSPConnector
     document: VirtualDocument,
     position_in_token: number
   ): Promise<CompletionHandler.ICompletionItemsReply> {
-    let connection = this._connections.get(document.id_path);
+    let connection = this._connections.get(document.uri);
 
     console.log('[LSP][Completer] Fetching and Transforming');
     console.log('[LSP][Completer] Token:', token, start, end);

diff --git a/packages/jupyterlab-lsp/src/virtual/document.ts b/packages/jupyterlab-lsp/src/virtual/document.ts
@@ -734,7 +734,7 @@ export class VirtualDocument {
     return this.parent.id_path + '-' + this.virtual_id;
   }
 
-  get uri(): string {
+  get uri(): VirtualDocument.uri {
     const encodedPath = encodeURI(this.path);
     if (!this.parent) {
       return encodedPath;
@@ -816,6 +816,10 @@ export namespace VirtualDocument {
    * for documents which should be interpreted as one when stretched across cells.
    */
   export type virtual_id = string;
+  /**
+   * Identifier composed of the file path and id_path.
+   */
+  export type uri = string;
 }
 
 export function collect_documents(