Fix cache, do not trigger: from key if already shown, line background,

add test for key-based trigger, removed unused variables,
return early if possible, move unpacking before loop

Author: krassowski

atest/05_Features/Hover.robot

@@ -4,7 +4,7 @@ Test Setup        Setup Hover Test
 Test Teardown     Clean Up After Working With File    Hover.ipynb
 Force Tags        feature:hover
 Resource          ../Keywords.robot
-Library           ../mouse_over_with_modifier.py
+Library           ../mouse_over_extension.py
 
 *** Variables ***
 ${HOVER_BOX}      css:.lsp-hover
@@ -13,7 +13,7 @@ ${HOVER_SIGNAL}    css:.cm-lsp-hover-available
 *** Test Cases ***
 Hover works in notebooks
     Enter Cell Editor    1
-    Hover Over    python_add
+    Trigger Tooltip    python_add
     Element Text Should Be    ${HOVER_SIGNAL}    python_add
     Capture Page Screenshot    02-hover-shown.png
     Element Should Contain    ${HOVER_BOX}    python_add(a: int, b: int)
@@ -24,9 +24,14 @@ Hover works in notebooks
     # it should be possible to move the mouse over the tooltip in order to copy/scroll
     Mouse Over    ${HOVER_BOX}
 
+Hover can be triggered via modifier key once cursor stopped moving
+    Enter Cell Editor    1
+    ${element} =    Last Occurrence    python_add
+    Wait Until Keyword Succeeds    4x    0.1 s    Trigger Via Modifier Key Press    ${element}
+
 Hover works in foreign code (javascript)
     Enter Cell Editor    2
-    Hover Over    js_add
+    Trigger Tooltip    js_add
     Capture Page Screenshot    02-hover-shown.png
     Element Should Contain    ${HOVER_BOX}    function js_add(a: any, b: any): any
     Page Should Contain Element    ${HOVER_BOX} code.language-typescript
@@ -36,23 +41,38 @@ Hover works in foreign code (javascript)
     Page Should Not Contain Element    ${HOVER_SIGNAL}
     # also for multiple cells of the same document
     Enter Cell Editor    3
-    Hover Over    Math
+    Trigger Tooltip    Math
     Element Should Contain    ${HOVER_BOX}    const Math: Math
 
 *** Keywords ***
-Hover Over
+Last Occurrence
     [Arguments]    ${symbol}
     ${sel} =    Set Variable If    "${symbol}".startswith(("xpath", "css"))    ${symbol}    xpath:(//span[@role="presentation"][contains(., "${symbol}")])[last()]
-    Wait Until Keyword Succeeds    3x    0.1 s    Trigger Tooltip    ${sel}
+    [Return]    ${sel}
 
-Trigger Tooltip
+Trigger Via Hover With Modifier
     [Arguments]    ${sel}
     # bring the cursor to the element
     Mouse Over    ${sel}
     # move it back and forth (wiggle) while hodling the ctrl modifier
     Mouse Over With Control    ${sel}    x_wiggle=5
     Wait Until Page Contains Element    ${HOVER_SIGNAL}
-    Wait Until Keyword Succeeds    3x    0.1s    Page Should Contain Element    ${HOVER_BOX}
+    Wait Until Keyword Succeeds    4x    0.1s    Page Should Contain Element    ${HOVER_BOX}
+
+Trigger Via Modifier Key Press
+    [Arguments]    ${sel}
+    # bring the cursor to the element
+    Mouse Over    ${sel}
+    Wait Until Page Contains Element    ${HOVER_SIGNAL}
+    Mouse Over And Wiggle    ${sel}    5
+    Press Keys    ${sel}    CTRL
+    Wait Until Keyword Succeeds    4x    0.1s    Page Should Contain Element    ${HOVER_BOX}
+
+Trigger Tooltip
+    [Arguments]    ${symbol}
+    [Documentation]    The default way to trigger the hover tooltip
+    ${sel} =    Last Occurrence    ${symbol}
+    Wait Until Keyword Succeeds    4x    0.1 s    Trigger Via Hover With Modifier    ${sel}
 
 Setup Hover Test
     Setup Notebook    Python    Hover.ipynb

atest/mouse_over_with_modifier.py renamed to atest/mouse_over_extension.py

@@ -1,18 +1,28 @@
-from robot.api import logger
 from robot.libraries.BuiltIn import BuiltIn
 from selenium.webdriver import ActionChains
 from selenium.webdriver.common.keys import Keys
 from SeleniumLibrary import SeleniumLibrary
 
 
+def wiggle(action_chains, x_wiggle):
+    if x_wiggle:
+        action_chains.move_by_offset(xoffset=x_wiggle, yoffset=0)
+        action_chains.move_by_offset(xoffset=-x_wiggle, yoffset=0)
+
+
 def mouse_over_with_control(locator, x_wiggle=0):
-    logger.info("Getting currently open browser desired capabilities")
     sl: SeleniumLibrary = BuiltIn().get_library_instance("SeleniumLibrary")
     action_chains = ActionChains(sl.driver)
     action_chains.key_down(Keys.CONTROL)
     action_chains.move_to_element(sl.find_element(locator))
-    if x_wiggle:
-        action_chains.move_by_offset(xoffset=x_wiggle, yoffset=0)
-        action_chains.move_by_offset(xoffset=-x_wiggle, yoffset=0)
+    wiggle(action_chains, x_wiggle)
     action_chains.key_up(Keys.CONTROL)
     return action_chains.perform()
+
+
+def mouse_over_and_wiggle(locator, x_wiggle=5):
+    sl: SeleniumLibrary = BuiltIn().get_library_instance("SeleniumLibrary")
+    action_chains = ActionChains(sl.driver)
+    action_chains.move_to_element(sl.find_element(locator))
+    wiggle(action_chains, x_wiggle)
+    return action_chains.perform()

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

@@ -93,23 +93,29 @@ export class HoverCM extends CodeMirrorIntegration {
   }
 
   protected restore_from_cache(
-    root_position: IRootPosition,
     document: VirtualDocument,
     virtual_position: IVirtualPosition
   ): IResponseData | null {
+    const { line, ch } = virtual_position;
     const matching_items = this.cache.data.filter(cache_item => {
       if (cache_item.document !== document) {
         return false;
       }
       let range = cache_item.response.range;
-      let { line, ch } = virtual_position;
       return (
         line >= range.start.line &&
         line <= range.end.line &&
         (line != range.start.line || ch >= range.start.character) &&
         (line != range.end.line || ch <= range.end.character)
       );
     });
+    if (matching_items.length > 1) {
+      console.warn(
+        'LSP: Potential hover cache malfunction: ',
+        virtual_position,
+        matching_items
+      );
+    }
     return matching_items.length != 0 ? matching_items[0] : null;
   }
 
@@ -157,16 +163,16 @@ export class HoverCM extends CodeMirrorIntegration {
       getModifierState(event, this.modifierKey) &&
       typeof this.last_hover_character !== 'undefined'
     ) {
+      // does not need to be shown if it is already visible (otherwise we would be creating an identical tooltip again!)
+      if (this.tooltip && this.tooltip.isVisible && !this.tooltip.isDisposed) {
+        return;
+      }
       const document = this.virtual_editor.document_at_root_position(
         this.last_hover_character
       );
-      const virtual_position = this.virtual_editor.root_position_to_virtual_position(
-        this.last_hover_character
-      );
       let response_data = this.restore_from_cache(
-        this.last_hover_character,
         document,
-        virtual_position
+        this.virtual_position
       );
       if (response_data == null) {
         return;
@@ -312,6 +318,13 @@ export class HoverCM extends CodeMirrorIntegration {
   protected async _updateUnderlineAndTooltip(
     event: MouseEvent
   ): Promise<boolean> {
+    const target = event.target as HTMLElement;
+
+    // if over an empty space in a line (and not over a token) then not worth checking
+    if (target.classList.contains('CodeMirror-line')) {
+      return;
+    }
+
     const show_tooltip = getModifierState(event, this.modifierKey);
 
     // currently the events are coming from notebook panel; ideally these would be connected to individual cells,
@@ -330,9 +343,6 @@ export class HoverCM extends CodeMirrorIntegration {
     let token = this.virtual_editor.getTokenAt(root_position);
 
     let document = this.virtual_editor.document_at_root_position(root_position);
-    let virtual_position = this.virtual_editor.root_position_to_virtual_position(
-      root_position
-    );
 
     if (
       this.is_token_empty(token) ||
@@ -344,14 +354,13 @@ export class HoverCM extends CodeMirrorIntegration {
     }
 
     if (!is_equal(root_position, this.last_hover_character)) {
+      let virtual_position = this.virtual_editor.root_position_to_virtual_position(
+        root_position
+      );
       this.virtual_position = virtual_position;
       this.last_hover_character = root_position;
 
-      let response_data = this.restore_from_cache(
-        root_position,
-        document,
-        virtual_position
-      );
+      let response_data = this.restore_from_cache(document, virtual_position);
 
       if (response_data == null) {
         let response = await this.debounced_get_hover.invoke();
@@ -470,15 +479,19 @@ export class HoverCM extends CodeMirrorIntegration {
       ...response,
       range: {
         start: PositionConverter.cm_to_lsp(
-          this.virtual_editor.transform_from_editor_to_root(
-            ce_editor,
-            editor_range.start
+          this.virtual_editor.root_position_to_virtual_position(
+            this.virtual_editor.transform_from_editor_to_root(
+              ce_editor,
+              editor_range.start
+            )
           )
         ),
         end: PositionConverter.cm_to_lsp(
-          this.virtual_editor.transform_from_editor_to_root(
-            ce_editor,
-            editor_range.end
+          this.virtual_editor.root_position_to_virtual_position(
+            this.virtual_editor.transform_from_editor_to_root(
+              ce_editor,
+              editor_range.end
+            )
           )
         )
       }