Fix state not being updated after notebook rename/restart

krassowski · krassowski · commit 13366c183fe3 · 2021-01-30T19:55:14.000Z
diff --git a/atest/03_Notebook.robot b/atest/03_Notebook.robot
@@ -66,8 +66,41 @@ Foreign Extractors
 Code Overrides
     ${file} =    Set Variable    Code overrides.ipynb
     Setup Notebook    Python    ${file}
-    ${virtual_path} =    Set Variable    ${VIRTUALDOCS DIR}${/}Code overrides.ipynb
+    ${virtual_path} =    Set Variable    ${VIRTUALDOCS DIR}${/}${file}
     Wait Until Created    ${virtual_path}
     Wait Until Keyword Succeeds    10x    1s    File Should Not Be Empty    ${virtual_path}
     ${document} =    Get File    ${virtual_path}
     Should Be Equal    ${document}    get_ipython().run_line_magic("ls", "")\n\n\nget_ipython().run_line_magic("pip", " freeze")\n
+    [Teardown]    Clean Up After Working With File    Code overrides.ipynb
+
+Adding Text To Cells Is Reflected In Virtual Document
+    ${file} =    Set Variable    Empty.ipynb
+    Setup Notebook    Python    ${file}
+    ${virtual_path} =    Set Variable    ${VIRTUALDOCS DIR}${/}${file}
+    Wait Until Created    ${virtual_path}
+    Enter Cell Editor    1
+    Press Keys    None    cell_1
+    Lab Command    Insert Cell Below
+    Enter Cell Editor    2
+    Press Keys    None    cell_2
+    Wait Until Keyword Succeeds    3x    1s    File Content Should Be Equal    ${virtual_path}    cell_1\n\n\ncell_2\n
+    [Teardown]    Clean Up After Working With File    Empty.ipynb
+
+Adding Text To Cells After Kernel Restart
+    ${file} =    Set Variable    Empty.ipynb
+    Setup Notebook    Python    ${file}
+    ${virtual_path} =    Set Variable    ${VIRTUALDOCS DIR}${/}${file}
+    Wait Until Created    ${virtual_path}
+    Enter Cell Editor    1
+    Lab Command    Insert Cell Below
+    Enter Cell Editor    2    line=1
+    Press Keys    None    text
+    Wait Until Keyword Succeeds    3x    1s    File Content Should Be Equal    ${virtual_path}    \n\n\ntext\n
+    [Teardown]    Clean Up After Working With File    Empty.ipynb
+
+*** Keywords ***
+File Content Should Be Equal
+    [Arguments]    ${file}    ${text}
+    Wait Until Keyword Succeeds    5x    1s    File Should Not Be Empty    ${file}
+    ${document} =    Get File    ${file}
+    Should Be Equal    ${document}    ${text}
diff --git a/atest/04_Interface/Statusbar.robot b/atest/04_Interface/Statusbar.robot
@@ -22,10 +22,7 @@ Statusbar Popup Opens
 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
+    Open New Notebook
     Element Should Contain    ${STATUSBAR}    Waiting...
     Wait Until Fully Initialized
     Switch To Tab    Python.ipynb
diff --git a/atest/Keywords.robot b/atest/Keywords.robot
@@ -413,3 +413,9 @@ Measure Cursor Position
 Switch To Tab
     [Arguments]    ${file}
     Click Element    ${JLAB XP DOCK TAB}\[contains(., '${file}')]
+
+Open New Notebook
+    Lab Command    New Notebook
+    Wait For Dialog
+    # Kernel selection dialog shows up, accept Python as default kernel
+    Accept Default Dialog Option
diff --git a/packages/jupyterlab-lsp/src/adapters/adapter.ts b/packages/jupyterlab-lsp/src/adapters/adapter.ts
@@ -221,6 +221,14 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
       this
     );
 
+    // pretend that all editors were removed to trigger the disconnection of even handlers
+    // they will be connected again on new connection
+    for (let editor of this.editors) {
+      this.editorRemoved.emit({
+        editor: editor
+      });
+    }
+
     for (let adapter of this.adapters.values()) {
       adapter.dispose();
     }
@@ -392,6 +400,7 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
       return;
     }
     this.virtual_editor = virtual_editor;
+    this.connect_contentChanged_signal();
   }
 
   /**
@@ -538,7 +547,7 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
    * (range) updates this can be still implemented by comparison of before/after states of the
    * virtual documents, which is even more resilient and -obviously - editor-independent.
    */
-  connect_contentChanged_signal() {
+  private connect_contentChanged_signal() {
     this.widget.context.model.contentChanged.connect(
       this.onContentChanged,
       this
diff --git a/packages/jupyterlab-lsp/src/adapters/file_editor/file_editor.ts b/packages/jupyterlab-lsp/src/adapters/file_editor/file_editor.ts
@@ -46,7 +46,6 @@ export class FileEditorAdapter extends WidgetAdapter<
     this.editor = editor_widget.content;
 
     this.init_virtual();
-    this.connect_contentChanged_signal();
 
     this.console.log('file ready for connection:', this.path);
 
diff --git a/packages/jupyterlab-lsp/src/adapters/notebook/notebook.ts b/packages/jupyterlab-lsp/src/adapters/notebook/notebook.ts
@@ -13,6 +13,10 @@ import { ICommandContext } from '../../command_manager';
 import { CodeEditor } from '@jupyterlab/codeeditor';
 import IEditor = CodeEditor.IEditor;
 import { VirtualDocument } from '../../virtual/document';
+import {
+  IObservableUndoableList,
+  IObservableList
+} from '@jupyterlab/observables';
 
 export class NotebookAdapter extends WidgetAdapter<NotebookPanel> {
   editor: Notebook;
@@ -125,7 +129,6 @@ export class NotebookAdapter extends WidgetAdapter<NotebookPanel> {
     this.console.log(this.document_path, 'ready for connection');
 
     this.init_virtual();
-    this.connect_contentChanged_signal();
 
     // connect the document, but do not open it as the adapter will handle this
     // after registering all features
@@ -139,83 +142,98 @@ export class NotebookAdapter extends WidgetAdapter<NotebookPanel> {
     );
 
     this.widget.content.activeCellChanged.connect(this.activeCellChanged, this);
-    this.widget.model.cells.changed.connect(async (cells, change) => {
-      let cellsAdded: ICellModel[] = [];
-      let cellsRemoved: ICellModel[] = [];
-      const type = this.type;
-
-      if (change.type === 'set') {
-        // handling of conversions is important, because the editors get re-used and their handlers inherited,
-        // so we need to clear our handlers from editors of e.g. markdown cells which previously were code cells.
-        let convertedToMarkdownOrRaw = [];
-        let convertedToCode = [];
-
-        if (change.newValues.length === change.oldValues.length) {
-          // during conversion the cells should not get deleted nor added
-          for (let i = 0; i < change.newValues.length; i++) {
-            if (
-              change.oldValues[i].type === type &&
-              change.newValues[i].type !== type
-            ) {
-              convertedToMarkdownOrRaw.push(change.newValues[i]);
-            } else if (
-              change.oldValues[i].type !== type &&
-              change.newValues[i].type === type
-            ) {
-              convertedToCode.push(change.newValues[i]);
-            }
+    this.widget.model.cells.changed.connect(this.handle_cell_change, this);
+    this.editor.modelChanged.connect(notebook => {
+      // note: this should not usually happen;
+      // there is no default action that would trigger this,
+      // its just a failsafe in case if another extension decides
+      // to swap the notebook model
+      this.console.warn(
+        'Model changed, connecting cell change handler; this is not something we were expecting'
+      );
+      notebook.model.cells.changed.connect(this.handle_cell_change, this);
+    });
+  }
+
+  async handle_cell_change(
+    cells: IObservableUndoableList<ICellModel>,
+    change: IObservableList.IChangedArgs<ICellModel>
+  ) {
+    let cellsAdded: ICellModel[] = [];
+    let cellsRemoved: ICellModel[] = [];
+    const type = this.type;
+
+    if (change.type === 'set') {
+      // handling of conversions is important, because the editors get re-used and their handlers inherited,
+      // so we need to clear our handlers from editors of e.g. markdown cells which previously were code cells.
+      let convertedToMarkdownOrRaw = [];
+      let convertedToCode = [];
+
+      if (change.newValues.length === change.oldValues.length) {
+        // during conversion the cells should not get deleted nor added
+        for (let i = 0; i < change.newValues.length; i++) {
+          if (
+            change.oldValues[i].type === type &&
+            change.newValues[i].type !== type
+          ) {
+            convertedToMarkdownOrRaw.push(change.newValues[i]);
+          } else if (
+            change.oldValues[i].type !== type &&
+            change.newValues[i].type === type
+          ) {
+            convertedToCode.push(change.newValues[i]);
           }
-          cellsAdded = convertedToCode;
-          cellsRemoved = convertedToMarkdownOrRaw;
         }
-      } else if (change.type == 'add') {
-        cellsAdded = change.newValues.filter(
-          cellModel => cellModel.type === type
-        );
-      }
-      // note: editorRemoved is not emitted for removal of cells by change of type 'remove' (but only during cell type conversion)
-      // because there is no easy way to get the widget associated with the removed cell(s) - because it is no
-      // longer in the notebook widget list! It would need to be tracked on our side, but it is not necessary
-      // as (except for a tiny memory leak) it should not impact the functionality in any way
-
-      if (
-        cellsRemoved.length ||
-        cellsAdded.length ||
-        change.type === 'move' ||
-        change.type === 'remove'
-      ) {
-        // in contrast to the file editor document which can be only changed by the modification of the editor content,
-        // the notebook document cna also get modified by a change in the number or arrangement of editors themselves;
-        // for this reason each change has to trigger documents update (so that LSP mirror is in sync).
-        await this.update_documents();
+        cellsAdded = convertedToCode;
+        cellsRemoved = convertedToMarkdownOrRaw;
       }
+    } else if (change.type == 'add') {
+      cellsAdded = change.newValues.filter(
+        cellModel => cellModel.type === type
+      );
+    }
+    // note: editorRemoved is not emitted for removal of cells by change of type 'remove' (but only during cell type conversion)
+    // because there is no easy way to get the widget associated with the removed cell(s) - because it is no
+    // longer in the notebook widget list! It would need to be tracked on our side, but it is not necessary
+    // as (except for a tiny memory leak) it should not impact the functionality in any way
+
+    if (
+      cellsRemoved.length ||
+      cellsAdded.length ||
+      change.type === 'move' ||
+      change.type === 'remove'
+    ) {
+      // in contrast to the file editor document which can be only changed by the modification of the editor content,
+      // the notebook document cna also get modified by a change in the number or arrangement of editors themselves;
+      // for this reason each change has to trigger documents update (so that LSP mirror is in sync).
+      await this.update_documents();
+    }
 
-      for (let cellModel of cellsRemoved) {
-        let cellWidget = this.widget.content.widgets.find(
-          cell => cell.model.id === cellModel.id
-        );
-        this.known_editors_ids.delete(cellWidget.editor.uuid);
-
-        // for practical purposes this editor got removed from our consideration;
-        // it might seem that we should instead look for the editor indicated by
-        // the oldValues[i] cellModel, but this one got already transferred to the
-        // markdown cell in newValues[i]
-        this.editorRemoved.emit({
-          editor: cellWidget.editor
-        });
-      }
+    for (let cellModel of cellsRemoved) {
+      let cellWidget = this.widget.content.widgets.find(
+        cell => cell.model.id === cellModel.id
+      );
+      this.known_editors_ids.delete(cellWidget.editor.uuid);
+
+      // for practical purposes this editor got removed from our consideration;
+      // it might seem that we should instead look for the editor indicated by
+      // the oldValues[i] cellModel, but this one got already transferred to the
+      // markdown cell in newValues[i]
+      this.editorRemoved.emit({
+        editor: cellWidget.editor
+      });
+    }
 
-      for (let cellModel of cellsAdded) {
-        let cellWidget = this.widget.content.widgets.find(
-          cell => cell.model.id === cellModel.id
-        );
-        this.known_editors_ids.add(cellWidget.editor.uuid);
+    for (let cellModel of cellsAdded) {
+      let cellWidget = this.widget.content.widgets.find(
+        cell => cell.model.id === cellModel.id
+      );
+      this.known_editors_ids.add(cellWidget.editor.uuid);
 
-        this.editorAdded.emit({
-          editor: cellWidget.editor
-        });
-      }
-    });
+      this.editorAdded.emit({
+        editor: cellWidget.editor
+      });
+    }
   }
 
   get editors(): CodeEditor.IEditor[] {
diff --git a/packages/jupyterlab-lsp/src/virtual/codemirror_editor.ts b/packages/jupyterlab-lsp/src/virtual/codemirror_editor.ts
