Merge pull request #162 from krassowski/fix/completer

krassowski · web-flow · commit f7ddb227d3f2 · 2020-01-19T16:04:57.000Z
Fix various bugs in the completer feature
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,12 @@
     [#159](https://github.com/krassowski/jupyterlab-lsp/pull/159)
     )
 
+- bugfixes
+
+  - fixed various small bugs in the completer (
+    [#162](https://github.com/krassowski/jupyterlab-lsp/pull/162)
+    )
+
 ## `lsp-ws-connection 0.3.1`
 
 - added `sendSaved()` method (textDocument/didSave) (
diff --git a/atest/05_Features/Completion.robot b/atest/05_Features/Completion.robot
@@ -58,11 +58,55 @@ Autocompletes If Only One Option
 User Can Select Lowercase After Starting Uppercase
     [Tags]    language:python
     Setup Notebook    Python    Completion.ipynb
-    Enter Cell Editor    4    line=1
+    # `from time import Tim<tab>` → `from time import time`
+    Wait Until Fully Initialized
+    Enter Cell Editor    5    line=1
     Trigger Completer
     Completer Should Suggest    time
     Press Keys    None    ENTER
-    Wait Until Keyword Succeeds    40x    0.5s    Cell Editor Should Equal    4    from time import time
+    Wait Until Keyword Succeeds    40x    0.5s    Cell Editor Should Equal    5    from time import time
+    [Teardown]    Clean Up After Working With File    Completion.ipynb
+
+Mid Token Completions Do Not Overwrite
+    [Tags]    language:python
+    Setup Notebook    Python    Completion.ipynb
+    # `disp<tab>data` → `display_table<cursor>data`
+    Place Cursor In Cell Editor At    9    line=1    character=4
+    Capture Page Screenshot    01-cursor-placed.png
+    Wait Until Fully Initialized
+    Press Keys    None    TAB
+    Capture Page Screenshot    02-completed.png
+    Wait Until Keyword Succeeds    40x    0.5s    Cell Editor Should Equal    9    display_tabledata
+    # `disp<tab>lay` → `display_table<cursor>`
+    Place Cursor In Cell Editor At    11    line=1    character=4
+    Press Keys    None    TAB
+    Wait Until Keyword Succeeds    40x    0.5s    Cell Editor Should Equal    11    display_table
+    [Teardown]    Clean Up After Working With File    Completion.ipynb
+
+Completion Works For Tokens Separated By Space
+    [Tags]    language:python
+    Setup Notebook    Python    Completion.ipynb
+    # `from statistics <tab>` → `from statistics import<cursor>`
+    Enter Cell Editor    13    line=1
+    Wait Until Fully Initialized
+    Trigger Completer
+    Completer Should Suggest    import
+    Press Keys    None    ENTER
+    Wait Until Keyword Succeeds    40x    0.5s    Cell Editor Should Equal    13    from statistics import
+    [Teardown]    Clean Up After Working With File    Completion.ipynb
+
+Kernel And LSP Completions Merge Prefix Conflicts Are Resolved
+    [Documentation]    Reconciliate Python kernel returning prefixed completions and LSP (pyls) not-prefixed ones
+    [Tags]    language:python
+    # For more details see: https://github.com/krassowski/jupyterlab-lsp/issues/30#issuecomment-576003987
+    # `import os.pat<tab>` → `import os.pathsep`
+    Setup Notebook    Python    Completion.ipynb
+    Enter Cell Editor    15    line=1
+    Wait Until Fully Initialized
+    Trigger Completer
+    Completer Should Suggest    pathsep
+    Select Completer Suggestion    pathsep
+    Wait Until Keyword Succeeds    40x    0.5s    Cell Editor Should Equal    15    import os.pathsep
     [Teardown]    Clean Up After Working With File    Completion.ipynb
 
 Triggers Completer On Dot
@@ -79,14 +123,20 @@ Triggers Completer On Dot
 *** Keywords ***
 Get Cell Editor Content
     [Arguments]    ${cell_nr}
-    ${content}    Execute JavaScript    return document.querySelector('.jp-CodeCell:nth-child(${cell_nr}) .CodeMirror').CodeMirror.getValue()
+    ${content}    Execute JavaScript    return document.querySelector('.jp-Cell:nth-child(${cell_nr}) .CodeMirror').CodeMirror.getValue()
     [Return]    ${content}
 
 Cell Editor Should Equal
     [Arguments]    ${cell}    ${value}
     ${content} =    Get Cell Editor Content    ${cell}
     Should Be Equal    ${content}    ${value}
 
+Select Completer Suggestion
+    [Arguments]    ${text}
+    ${suggestion} =    Set Variable    css:.jp-Completer-item[data-value="${text}"]
+    Mouse Over    ${suggestion}
+    Click Element    ${suggestion} code
+
 Completer Should Suggest
     [Arguments]    ${text}
     Page Should Contain Element    ${COMPLETER_BOX} .jp-Completer-item[data-value="${text}"]
diff --git a/atest/Keywords.robot b/atest/Keywords.robot
@@ -198,8 +198,13 @@ Gently Reset Workspace
 
 Enter Cell Editor
     [Arguments]    ${cell_nr}    ${line}=1
-    Click Element    css:.jp-CodeCell:nth-child(${cell_nr}) .CodeMirror-line:nth-child(${line})
-    Wait Until Page Contains Element    css:.jp-CodeCell:nth-child(${cell_nr}) .CodeMirror-focused
+    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
+
+Place Cursor In Cell Editor At
+    [Arguments]    ${cell_nr}    ${line}    ${character}
+    Enter Cell Editor    ${cell_nr}    ${line}
+    Execute JavaScript    return document.querySelector('.jp-Cell:nth-child(${cell_nr}) .CodeMirror').CodeMirror.setCursor({line: ${line} - 1, ch: ${character}})
 
 Wait Until Fully Initialized
     Wait Until Element Contains    ${STATUSBAR}    Fully initialized    timeout=35s
diff --git a/atest/examples/Completion.ipynb b/atest/examples/Completion.ipynb
@@ -28,6 +28,13 @@
     "list."
    ]
   },
+  {
+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "`from time import Tim<tab>` → `from time import time`"
+   ]
+  },
   {
    "cell_type": "code",
    "execution_count": null,
@@ -36,6 +43,87 @@
    "source": [
     "from time import Tim"
    ]
+  },
+  {
+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "Setup (not a test case)"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "def display_table():\n",
+    "    pass"
+   ]
+  },
+  {
+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "`disp<tab>data` → `display_table<cursor>data`"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "dispdata"
+   ]
+  },
+  {
+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "`disp<tab>lay` → `display_table<cursor>`"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "display"
+   ]
+  },
+  {
+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "`from statistics <tab>` → `from statistics import<cursor>`"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "from statistics "
+   ]
+  },
+  {
+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "`import os.pat<tab>` → select `pathsep` → `import os.pathsep` (tests whether kernel prefixes are removed)"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "import os.pat"
+   ]
   }
  ],
  "metadata": {
diff --git a/packages/jupyterlab-lsp/src/adapters/jupyterlab/components/completion.ts b/packages/jupyterlab-lsp/src/adapters/jupyterlab/components/completion.ts
@@ -111,6 +111,7 @@ export class LSPConnector extends DataConnector<
     const start = editor.getPositionAt(token.offset);
     const end = editor.getPositionAt(token.offset + token.value.length);
 
+    let position_in_token = cursor.column - start.column - 1;
     const typed_character = token.value[cursor.column - start.column - 1];
 
     let start_in_root = this.transform_from_editor_to_root(start);
@@ -150,7 +151,8 @@ export class LSPConnector extends DataConnector<
             virtual_start,
             virtual_end,
             virtual_cursor,
-            document
+            document,
+            position_in_token
           )
         ]).then(([kernel, lsp]) =>
           this.merge_replies(kernel, lsp, this._editor)
@@ -163,7 +165,8 @@ export class LSPConnector extends DataConnector<
         virtual_start,
         virtual_end,
         virtual_cursor,
-        document
+        document,
+        position_in_token
       ).catch(e => {
         console.warn('LSP: hint failed', e);
         return this.fallback_connector.fetch(request);
@@ -180,7 +183,8 @@ export class LSPConnector extends DataConnector<
     start: IVirtualPosition,
     end: IVirtualPosition,
     cursor: IVirtualPosition,
-    document: VirtualDocument
+    document: VirtualDocument,
+    position_in_token: number
   ): Promise<CompletionHandler.IReply> {
     let connection = this._connections.get(document.id_path);
 
@@ -190,7 +194,7 @@ export class LSPConnector extends DataConnector<
     // to the matches...
     // Suggested in https://github.com/jupyterlab/jupyterlab/issues/7044, TODO PR
 
-    console.log(token);
+    console.log('[LSP][Completer] Token:', token);
 
     let completion_items: lsProtocol.CompletionItem[] = [];
     await connection
@@ -208,6 +212,8 @@ export class LSPConnector extends DataConnector<
         completion_items = items || [];
       });
 
+    let prefix = token.value.slice(0, position_in_token + 1);
+
     let matches: Array<string> = [];
     const types: Array<IItemType> = [];
     let all_non_prefixed = true;
@@ -219,10 +225,22 @@ export class LSPConnector extends DataConnector<
       // kind: 3
       // label: "mean(data)"
       // sortText: "amean"
+
+      // TODO: add support for match.textEdit
       let text = match.insertText ? match.insertText : match.label;
 
-      if (text.toLowerCase().startsWith(token.value.toLowerCase())) {
+      if (text.toLowerCase().startsWith(prefix.toLowerCase())) {
         all_non_prefixed = false;
+        if (prefix !== token.value) {
+          if (text.toLowerCase().startsWith(token.value.toLowerCase())) {
+            // given a completion insert text "display_table" and two test cases:
+            // disp<tab>data →  display_table<cursor>data
+            // disp<tab>lay  →  display_table<cursor>
+            // we have to adjust the prefix for the latter (otherwise we would get display_table<cursor>lay),
+            // as we are constrained NOT to replace after the prefix (which would be "disp" otherwise)
+            prefix = token.value;
+          }
+        }
       }
 
       matches.push(text);
@@ -243,7 +261,7 @@ export class LSPConnector extends DataConnector<
       // text = token.value + text;
       // but it did not work for "from statistics <tab>" and lead to "from statisticsimport" (no space)
       start: token.offset + (all_non_prefixed ? 1 : 0),
-      end: token.offset + token.value.length,
+      end: token.offset + prefix.length,
       matches: matches,
       metadata: {
         _jupyter_types_experimental: types
@@ -266,7 +284,7 @@ export class LSPConnector extends DataConnector<
     } else if (lsp.matches.length === 0) {
       return kernel;
     }
-    console.log('merging LSP and kernel completions:', lsp, kernel);
+    console.log('[LSP][Completer] Merging completions:', lsp, kernel);
 
     // Populate the result with a copy of the lsp matches.
     const matches = lsp.matches.slice();
@@ -285,8 +303,10 @@ export class LSPConnector extends DataConnector<
     if (lsp.start > kernel.start) {
       const cursor = editor.getCursorPosition();
       const line = editor.getLine(cursor.line);
-      prefix = line.substring(cursor.column - 1, cursor.column);
-      console.log('will remove prefix from kernel response:', prefix);
+      prefix = line.substring(kernel.start, lsp.start);
+      console.log('[LSP][Completer] Removing kernel prefix: ', prefix);
+    } else if (lsp.start < kernel.start) {
+      console.warn('[LSP][Completer] Kernel start > LSP start');
     }
 
     let remove_prefix = (value: string) => {
