Merge pull request #975 from krassowski/bugfixes

Fix issues identified in 5.0.0b0, prepare 5.0.0b1

Author: krassowski

CHANGELOG.md

@@ -1,5 +1,11 @@
 ## Changelog
 
+### `@jupyter-lsp/jupyterlab-lsp 5.0.0-beta.1`
+
+- fix highlights conflict with selection
+- fix scrolling to diagnostics and diagnostic rendering in windowed notebook
+- suppress kernel completer in transclusions
+
 ### `@jupyter-lsp/jupyterlab-lsp 5.0.0-beta.0`
 
 - fix most regressions caught by tests in alpha

packages/jupyterlab-lsp/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@jupyter-lsp/jupyterlab-lsp",
-  "version": "5.0.0-beta.0",
+  "version": "5.0.0-beta.1",
   "description": "Language Server Protocol integration for JupyterLab",
   "keywords": [
     "jupyter",
@@ -151,7 +151,7 @@
       "@jupyterlab/lsp": {
         "bundled": false,
         "singleton": true,
-        "strictVersion": true
+        "strictVersion": false
       }
     },
     "schemaDir": "schema",

packages/jupyterlab-lsp/src/features/completion/overrides.ts

@@ -8,14 +8,25 @@ import {
   CompletionHandler,
   ICompletionContext
 } from '@jupyterlab/completer';
+import { IDocumentWidget } from '@jupyterlab/docregistry';
+import {
+  ILSPDocumentConnectionManager,
+  IEditorPosition
+} from '@jupyterlab/lsp';
 import { LabIcon } from '@jupyterlab/ui-components';
 
 import { CodeCompletion as LSPCompletionSettings } from '../../_completion';
+import {
+  editorPositionToRootPosition,
+  PositionConverter,
+  documentAtRootPosition
+} from '../../converter';
 import { FeatureSettings } from '../../feature';
 
 interface IOptions {
   settings: FeatureSettings<LSPCompletionSettings>;
   iconsThemeManager: ILSPCompletionThemeManager;
+  connectionManager: ILSPDocumentConnectionManager;
 }
 
 export class EnhancedContextCompleterProvider extends ContextCompleterProvider {
@@ -75,6 +86,56 @@ export class EnhancedKernelCompleterProvider extends KernelCompleterProvider {
     return result;
   }
 
+  async isApplicable(context: ICompletionContext): Promise<boolean> {
+    // Note: this method logs errors instead of throwing to ensure we do not ever
+    // break the upstream kernel completer, even if there is an error elsehwere.
+    const upstream = await super.isApplicable(context);
+
+    if (upstream === false) {
+      return false;
+    }
+
+    const manager = this.options.connectionManager;
+    const widget = context.widget as IDocumentWidget;
+    const adapter = manager.adapters.get(widget.context.path);
+
+    if (!adapter) {
+      return upstream;
+    }
+
+    if (!context.editor) {
+      // TODO: why is editor optional in the first place?
+      console.error('No editor');
+      return upstream;
+    }
+    const editor = context.editor;
+
+    const editorPosition = PositionConverter.ce_to_cm(
+      editor.getCursorPosition()
+    ) as IEditorPosition;
+
+    const block = adapter.editors.find(
+      value => value.ceEditor.getEditor() == editor
+    );
+
+    if (!block) {
+      console.error('Could not get block with editor');
+      return upstream;
+    }
+    const rootPosition = editorPositionToRootPosition(
+      adapter,
+      block.ceEditor,
+      editorPosition
+    );
+
+    if (!rootPosition) {
+      console.error('Could not get root position');
+      return upstream;
+    }
+    const virtualDocument = documentAtRootPosition(adapter, rootPosition);
+    return virtualDocument === adapter.virtualDocument;
+  }
+
   protected iconFor(type: string): LabIcon | undefined {
     const icon = this.options.iconsThemeManager.getIcon(type) as LabIcon | null;
     return icon ? icon : undefined;

packages/jupyterlab-lsp/src/features/diagnostics/diagnostics.ts

@@ -231,7 +231,7 @@ class DiagnosticsPanel {
           console.warn('LPS: diagnostics row not found for jump execute()');
           return;
         }
-        this.widget.content.jumpTo(row);
+        return this.widget.content.jumpTo(row);
       },
       label: this.trans.__('Jump to location'),
       icon: jumpToIcon

packages/jupyterlab-lsp/src/features/diagnostics/feature.ts

@@ -57,6 +57,7 @@ export class DiagnosticsFeature extends Feature implements IDiagnosticsFeature {
   protected settings: IFeatureSettings<LSPDiagnosticsSettings>;
   protected console = new BrowserConsole().scope('Diagnostics');
   private _responseReceived: PromiseDelegate<void> = new PromiseDelegate();
+  private _firstResponseReceived: PromiseDelegate<void> = new PromiseDelegate();
   private _diagnosticsDatabases = new WeakMap<
     WidgetLSPAdapter<any>,
     DiagnosticsDatabase
@@ -96,6 +97,7 @@ export class DiagnosticsFeature extends Feature implements IDiagnosticsFeature {
         return value;
       }
     });
+
     const connectionManager = options.connectionManager;
     // https://github.com/jupyterlab/jupyterlab/issues/14783
     options.shell.currentChanged.connect(shell => {
@@ -143,14 +145,30 @@ export class DiagnosticsFeature extends Feature implements IDiagnosticsFeature {
           // few seconds to trigger timeout). Because `response.version` is
           // optional it would require further testing.
 
-          await adapter.updateFinished;
-          await this._responseReceived.promise;
+          if (view.state.field(this._invalidationCounter) == 0) {
+            // If we are displaying the editor for the first time,
+            // e.g. after scrolling down in windowed notebook,
+            // do not wait for next update, show what we already know.
+
+            // TODO: this still fails when scrolling down fast and then
+            // scrolling up to the skipped cells because the state invlidation
+            // counter kicks in but diagnostics does not get rendered yet before
+            // we leave..
+            await this._firstResponseReceived.promise;
+          } else {
+            await adapter.updateFinished;
+            await this._responseReceived.promise;
+          }
 
           const database = this.getDiagnosticsDB(adapter);
 
           for (const editorDiagnostics of database.values()) {
             for (const editorDiagnostic of editorDiagnostics) {
-              if (editorDiagnostic.editor.editor !== view) {
+              const editor = editorDiagnostic.editorAccessor.getEditor() as
+                | CodeMirrorEditor
+                | undefined;
+
+              if (editor?.editor !== view) {
                 continue;
               }
               const diagnostic = editorDiagnostic.diagnostic;
@@ -165,7 +183,7 @@ export class DiagnosticsFeature extends Feature implements IDiagnosticsFeature {
                 editorDiagnostic.range.end
               );
 
-              const from = editorDiagnostic.editor.getOffsetAt(
+              const from = editor.getOffsetAt(
                 start.line >= lines
                   ? {
                       line: Math.min(start.line, lines),
@@ -174,7 +192,7 @@ export class DiagnosticsFeature extends Feature implements IDiagnosticsFeature {
                   : start
               );
               // TODO: this is wrong; there is however an issue if this is not applied
-              const to = editorDiagnostic.editor.getOffsetAt(
+              const to = editor.getOffsetAt(
                 end.line >= lines
                   ? {
                       line: Math.min(end.line, lines),
@@ -451,7 +469,6 @@ export class DiagnosticsFeature extends Feature implements IDiagnosticsFeature {
         }
 
         const editorAccessor = document.getEditorAtVirtualLine(start);
-        const editor = editorAccessor.getEditor()!;
 
         const startInEditor = document.transformVirtualToEditor(start);
         let endInEditor: IEditorPosition | null;
@@ -483,7 +500,7 @@ export class DiagnosticsFeature extends Feature implements IDiagnosticsFeature {
         for (let diagnostic of diagnostics) {
           diagnosticsList.push({
             diagnostic,
-            editor: editor as CodeMirrorEditor,
+            editorAccessor: editorAccessor,
             range: {
               start: startInEditor,
               end: endInEditor
@@ -497,10 +514,12 @@ export class DiagnosticsFeature extends Feature implements IDiagnosticsFeature {
 
     const previousList = diagnosticsDB.get(document);
     const editorsWhichHadDiagnostics = new Set(
-      previousList?.map(d => d.editor)
+      previousList?.map(d => d.editorAccessor.getEditor())
     );
 
-    const editorsWithDiagnostics = new Set(diagnosticsList?.map(d => d.editor));
+    const editorsWithDiagnostics = new Set(
+      diagnosticsList?.map(d => d.editorAccessor.getEditor())
+    );
     diagnosticsDB.set(document, diagnosticsList);
 
     // Refresh editors with diagnostics; this is needed because linter's
@@ -548,6 +567,7 @@ export class DiagnosticsFeature extends Feature implements IDiagnosticsFeature {
       this.setDiagnostics(response, document, adapter);
       const done = new Promise<void>(resolve => {
         setTimeout(() => {
+          this._firstResponseReceived.resolve();
           this._responseReceived.resolve();
           this._responseReceived = new PromiseDelegate();
           resolve();

packages/jupyterlab-lsp/src/features/diagnostics/listing.tsx

@@ -286,7 +286,7 @@ export class DiagnosticsListing extends VDomRenderer<DiagnosticsListing.Model> {
           let cellNumber: number | null = null;
           if (adapter.hasMultipleEditors) {
             const cellIndex = adapter.editors.findIndex(
-              value => value.ceEditor.getEditor() == diagnosticData.editor
+              value => value.ceEditor == diagnosticData.editorAccessor
             );
             cellNumber = cellIndex + 1;
           }
@@ -330,7 +330,7 @@ export class DiagnosticsListing extends VDomRenderer<DiagnosticsListing.Model> {
           key={row.key}
           data-key={row.key}
           onClick={() => {
-            this.jumpTo(row);
+            return this.jumpTo(row);
           }}
         >
           {cells}
@@ -360,12 +360,10 @@ export class DiagnosticsListing extends VDomRenderer<DiagnosticsListing.Model> {
     return this._diagnostics.get(key);
   }
 
-  jumpTo(row: IDiagnosticsRow) {
-    const cmEditor = row.data.editor;
-    cmEditor.setCursorPosition(
-      PositionConverter.cm_to_ce(row.data.range.start)
-    );
-    cmEditor.focus();
+  async jumpTo(row: IDiagnosticsRow): Promise<void> {
+    const editor = await row.data.editorAccessor.reveal();
+    editor.setCursorPosition(PositionConverter.cm_to_ce(row.data.range.start));
+    editor.focus();
   }
 }
 

packages/jupyterlab-lsp/src/features/diagnostics/tokens.ts

@@ -1,5 +1,4 @@
-import { CodeMirrorEditor } from '@jupyterlab/codemirror';
-import { IEditorPosition, WidgetLSPAdapter } from '@jupyterlab/lsp';
+import { IEditorPosition, WidgetLSPAdapter, Document } from '@jupyterlab/lsp';
 import { Token } from '@lumino/coreutils';
 import * as lsProtocol from 'vscode-languageserver-protocol';
 
@@ -11,7 +10,7 @@ import { PLUGIN_ID } from '../../tokens';
  */
 export interface IEditorDiagnostic {
   diagnostic: lsProtocol.Diagnostic;
-  editor: CodeMirrorEditor;
+  editorAccessor: Document.IEditor;
   range: {
     start: IEditorPosition;
     end: IEditorPosition;

packages/jupyterlab-lsp/src/features/rename.ts

@@ -170,7 +170,7 @@ function guessFailureReason(
         let start = diagnostic.range.start;
         if (adapter.hasMultipleEditors) {
           let editorIndex = adapter.editors.findIndex(
-            e => e.ceEditor.getEditor() === diagnostic.editor
+            e => e.ceEditor === diagnostic.editorAccessor
           );
           let cellNumber = editorIndex === -1 ? '(?)' : editorIndex + 1;
           return trans.__(

packages/jupyterlab-lsp/style/highlight.css

@@ -31,7 +31,7 @@
 .cm-lsp-highlight-Read {
   /* highlight on places where variable is accessed, e.g. `print(var)` */
   background-color: var(--jp-editor-mirror-lsp-highlight-background-color);
-  outline: 1px dashed var(--jp-editor-mirror-lsp-highlight-border-color);
+  outline: 1px solid var(--jp-editor-mirror-lsp-highlight-background-color);
 }
 
 .cm-lsp-highlight-Write {

packages/jupyterlab-lsp/style/variables/base.css

@@ -1,6 +1,10 @@
 :root {
   /* highlight */
-  --jp-editor-mirror-lsp-highlight-background-color: var(--jp-layout-color2);
+  --jp-editor-mirror-lsp-highlight-background-color: color-mix(
+    in srgb,
+    var(--jp-layout-color3) 25%,
+    transparent
+  );
   --jp-editor-mirror-lsp-highlight-border-color: var(--jp-layout-color3);
   /* diagnostics */
   --jp-editor-mirror-lsp-diagnostic-decoration-style: dashed;