Merge pull request #421 from krassowski/fix-editor-removal

Refactor how the notebook changes are picked up, events bound and unbound

Author: krassowski

CHANGELOG.md

@@ -16,8 +16,12 @@
 
   - handles characters that need escaping (spaces, non-ASCII characters) more
     robustly in files and folder names ([#403])
+  - moving cells now triggers the document update immediately leading to immediate diagnostics update ([#421])
+  - changing cell type to `raw` or `markdown` and then back to `code` properly unbinds/binds event handlers and updates document ([#421])
+  - pasted cells are added to the LSP document immediately, without the need for the user to enter them ([#421])
 
 [#403]: https://github.com/krassowski/jupyterlab-lsp/issues/403
+[#421]: https://github.com/krassowski/jupyterlab-lsp/issues/421
 
 ### `@krassowski/jupyterlab_go_to_definition 2.0.0` (???)
 

atest/03_Notebook.robot

@@ -11,6 +11,39 @@ Python
     Capture Page Screenshot    01-python.png
     [Teardown]    Clean Up After Working With File    Python.ipynb
 
+Conversion Of Cell Types
+    [Setup]    Setup Notebook    Python    Python.ipynb
+    ${lsp_entry} =    Set Variable    Show diagnostics panel
+    # initial (code) cell
+    Open Context Menu Over Cell Editor    1
+    Capture Page Screenshot    01-initial-code-cell.png
+    Context Menu Should Contain    ${lsp_entry}
+    Close Context Menu
+    # raw cell
+    Lab Command    Change to Raw Cell Type
+    Open Context Menu Over Cell Editor    1
+    Capture Page Screenshot    02-as-raw-cell.png
+    Context Menu Should Not Contain    ${lsp_entry}
+    Close Context Menu
+    # code cell again
+    Lab Command    Change to Code Cell Type
+    Open Context Menu Over Cell Editor    1
+    Capture Page Screenshot    03-as-code-cell-again.png
+    Context Menu Should Contain    ${lsp_entry}
+    Close Context Menu
+    [Teardown]    Clean Up After Working With File    Python.ipynb
+
+Moving Cells Around
+    [Setup]    Setup Notebook    Python    Python.ipynb
+    ${diagnostic} =    Set Variable    undefined name 'test' (pyflakes)
+    Enter Cell Editor    1
+    Lab Command    Move Cells Down
+    Wait Until Page Contains Element    css:.cm-lsp-diagnostic[title="${diagnostic}"]    timeout=35s
+    Enter Cell Editor    1
+    Lab Command    Move Cells Down
+    Wait Until Page Does Not Contain Element    css:.cm-lsp-diagnostic[title="${diagnostic}"]    timeout=35s
+    [Teardown]    Clean Up After Working With File    Python.ipynb
+
 Foreign Extractors
     ${file} =    Set Variable    Foreign extractors.ipynb
     Configure JupyterLab Plugin

atest/Keywords.robot

@@ -279,6 +279,11 @@ Enter Cell Editor
     Click Element    css:.jp-Cell:nth-child(${cell_nr}) .CodeMirror-line:nth-child(${line})
     Wait Until Page Contains Element    css:.jp-Cell:nth-child(${cell_nr}) .CodeMirror-focused
 
+Open Context Menu Over Cell Editor
+    [Arguments]    ${cell_nr}    ${line}=1
+    Enter Cell Editor    ${cell_nr}    line=${line}
+    Open Context Menu Over    css:.jp-Cell:nth-child(${cell_nr}) .CodeMirror-line:nth-child(${line})
+
 Place Cursor In Cell Editor At
     [Arguments]    ${cell_nr}    ${line}    ${character}
     Enter Cell Editor    ${cell_nr}    ${line}
@@ -301,6 +306,19 @@ Open Context Menu Over
     Wait Until Keyword Succeeds    10 x    0.1 s    Mouse Over    ${sel}
     Wait Until Keyword Succeeds    10 x    0.1 s    Open Context Menu    ${sel}
 
+Context Menu Should Contain
+    [Arguments]    ${label}    ${timeout}=10s
+    ${entry}    Set Variable    xpath://div[contains(@class, 'lm-Menu-itemLabel')][contains(text(), '${label}')]
+    Wait Until Page Contains Element    ${entry}    timeout=${timeout}
+
+Context Menu Should Not Contain
+    [Arguments]    ${label}    ${timeout}=10s
+    ${entry}    Set Variable    xpath://div[contains(@class, 'lm-Menu-itemLabel')][contains(text(), '${label}')]
+    Wait Until Page Does Not Contain Element    ${entry}    timeout=${timeout}
+
+Close Context Menu
+    Press Keys    None    ESCAPE
+
 Prepare File for Editing
     [Arguments]    ${Language}    ${Screenshots}    ${file}
     Set Tags    language:${Language.lower()}

packages/jupyterlab-lsp/src/adapters/adapter.ts

@@ -96,6 +96,8 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
   protected app: JupyterFrontEnd;
 
   public activeEditorChanged: Signal<WidgetAdapter<T>, IEditorChangedData>;
+  public editorAdded: Signal<WidgetAdapter<T>, IEditorChangedData>;
+  public editorRemoved: Signal<WidgetAdapter<T>, IEditorChangedData>;
   public update_finished: Promise<void>;
 
   /**
@@ -117,6 +119,8 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
     this.connection_manager = extension.connection_manager;
     this.adapterConnected = new Signal(this);
     this.activeEditorChanged = new Signal(this);
+    this.editorRemoved = new Signal(this);
+    this.editorAdded = new Signal(this);
     this.adapters = new Map();
     this.status_message = new StatusMessage();
     this.isConnected = false;

packages/jupyterlab-lsp/src/adapters/notebook/notebook.ts

@@ -1,7 +1,7 @@
 import { WidgetAdapter } from '../adapter';
 import { Notebook, NotebookPanel } from '@jupyterlab/notebook';
 import { until_ready } from '../../utils';
-import { Cell } from '@jupyterlab/cells';
+import { Cell, ICellModel } from '@jupyterlab/cells';
 import * as nbformat from '@jupyterlab/nbformat';
 import ILanguageInfoMetadata = nbformat.ILanguageInfoMetadata;
 import { Session } from '@jupyterlab/services';
@@ -17,13 +17,16 @@ import { VirtualDocument } from '../../virtual/document';
 export class NotebookAdapter extends WidgetAdapter<NotebookPanel> {
   editor: Notebook;
   private ce_editor_to_cell: Map<IEditor, Cell>;
+  private known_editors_ids: Set<string>;
 
   private _language_info: ILanguageInfoMetadata;
+  private type: nbformat.CellType = 'code';
 
   constructor(extension: LSPExtension, editor_widget: NotebookPanel) {
     super(extension, editor_widget);
     this.ce_editor_to_cell = new Map();
     this.editor = editor_widget.content;
+    this.known_editors_ids = new Set();
     this.init_once_ready().catch(console.warn);
   }
 
@@ -136,6 +139,83 @@ 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]);
+            }
+          }
+          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 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
+        });
+      }
+    });
   }
 
   get editors(): CodeEditor.IEditor[] {
@@ -178,13 +258,29 @@ export class NotebookAdapter extends WidgetAdapter<NotebookPanel> {
   }
 
   private activeCellChanged(notebook: Notebook, cell: Cell) {
+    if (cell.model.type !== this.type) {
+      return;
+    }
+    if (!this.known_editors_ids.has(cell.editor.uuid)) {
+      this.known_editors_ids.add(cell.editor.uuid);
+      this.editorAdded.emit({
+        editor: cell.editor
+      });
+    }
     this.activeEditorChanged.emit({
       editor: cell.editor
     });
   }
 
   context_from_active_document(): ICommandContext | null {
     let cell = this.widget.content.activeCell;
+    if (cell.model.type !== this.type) {
+      // context will be sought on all cells to verify if the context menu should be visible,
+      // thus it is ok to just return null; it seems to stem from the implementation detail
+      // upstream, i.e. the markdown cells appear to be created by transforming the code cells
+      // but do not quote me on that.
+      return null;
+    }
     let editor = cell.editor;
     let ce_cursor = editor.getCursorPosition();
     let cm_cursor = PositionConverter.ce_to_cm(ce_cursor) as IEditorPosition;

packages/jupyterlab-lsp/src/virtual/codemirror_editor.ts

@@ -280,9 +280,15 @@ export class CodeMirrorVirtualEditor
     };
     this._event_wrappers.set([eventName, handler], wrapped_handler);
 
-    this.forEveryBlockEditor(cm_editor => {
-      cm_editor.on(eventName, wrapped_handler);
-    });
+    this.forEveryBlockEditor(
+      cm_editor => {
+        cm_editor.on(eventName, wrapped_handler);
+      },
+      true,
+      cm_editor => {
+        cm_editor.off(eventName, wrapped_handler);
+      }
+    );
   }
 
   off(eventName: string, handler: CodeMirrorHandler, ...args: any[]): void {
@@ -496,28 +502,25 @@ export class CodeMirrorVirtualEditor
     return 0;
   }
 
-  addEventListener(type: string, listener: EventListenerOrEventListenerObject) {
-    this.forEveryBlockEditor(cm_editor => {
-      cm_editor.getWrapperElement().addEventListener(type, listener);
-    });
-  }
-
   forEveryBlockEditor(
     callback: (cm_editor: CodeMirror.Editor) => any,
-    monitor_for_new_blocks = true
+    monitor_for_new_blocks = true,
+    on_editor_removed_callback: (cm_editor: CodeMirror.Editor) => any = null
   ) {
     const editors_with_handlers = new Set<CodeMirror.Editor>();
 
-    // TODO... the need of iterating over all editors is universal. How does the virtual
-    //  editor gets knowledge of the editor instances? From the adapter obviously.
+    // TODO... the need of iterating over all editors is universal - so this could be
+    //  generalised to the VirtualEditor rather than live in CodeMirrorVirtualEditor;
+    //  How would the VirtualEditor get knowledge of the editor instances?
+    //  From the adapter (obviously).
     for (let editor of this.adapter.editors) {
       let cm_editor = (editor as CodeMirrorEditor).editor;
       editors_with_handlers.add(cm_editor);
       callback(cm_editor);
     }
     if (monitor_for_new_blocks) {
-      this.adapter.activeEditorChanged.connect(
-        (adapter, data: IEditorChangedData) => {
+      this.adapter.editorAdded.connect(
+        (adapter: WidgetAdapter<IDocumentWidget>, data: IEditorChangedData) => {
           let { editor } = data;
           if (editor == null) {
             return;
@@ -528,6 +531,16 @@ export class CodeMirrorVirtualEditor
           }
         }
       );
+      this.adapter.editorRemoved.connect(
+        (adapter, data: IEditorChangedData) => {
+          let { editor } = data;
+          if (editor == null) {
+            return;
+          }
+          let cm_editor = (editor as CodeMirrorEditor).editor;
+          on_editor_removed_callback(cm_editor);
+        }
+      );
     }
   }