feat: add optional position adjuster to HoverifierOptions (#38)

This commit adds the option to adjust positions used in codeintellify. This is needed because some code hosts change whitespace to fit their needs in the UI. In order to get correct code intelligence we need to be able to map the position that we found in the DOM to the actual position of that token in it's source.

Author: ijsnow

src/hoverifier.test.ts

@@ -6,7 +6,13 @@ import { Position } from 'vscode-languageserver-types'
 
 import { noop } from 'lodash'
 import { propertyIsDefined } from './helpers'
-import { createHoverifier, LOADER_DELAY, TOOLTIP_DISPLAY_DELAY } from './hoverifier'
+import {
+    AdjustmentDirection,
+    createHoverifier,
+    LOADER_DELAY,
+    PositionAdjuster,
+    TOOLTIP_DISPLAY_DELAY,
+} from './hoverifier'
 import { HoverOverlayProps } from './HoverOverlay'
 import { findPositionsFromEvents } from './positions'
 import { CodeViewProps, DOM } from './testutils/dom'
@@ -108,4 +114,83 @@ describe('Hoverifier', () => {
             })
         }
     })
+
+    /**
+     * This test ensures that the adjustPosition options is being called in the ways we expect. This test is actually not the best way to ensure the feature
+     * works as expected. This is a good example of a bad side effect of how the main `hoverifier.ts` file is too tightly integrated with itself. Ideally, I'd be able to assert
+     * that the effected positions have actually been adjusted as intended but this is impossible with the current implementation. We can assert that the `HoverFetcher` and `JumpURLFetcher`s
+     * have the adjusted positions (AdjustmentDirection.CodeViewToActual). However, we cannot reliably assert that the code "highlighting" the token has the position adjusted (AdjustmentDirection.ActualToCodeView).
+     */
+    it('PositionAdjuster gets called when expected', () => {
+        for (const codeView of testcases) {
+            const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b))
+
+            scheduler.run(({ cold, expectObservable }) => {
+                const adjustmentDirections = new Subject<AdjustmentDirection>()
+
+                const fetchHover = createStubHoverFetcher({})
+                const fetchJumpURL = createStubJumpURLFetcher('def')
+
+                const adjustPosition: PositionAdjuster = ({ direction, position }) => {
+                    adjustmentDirections.next(direction)
+
+                    return of(position)
+                }
+
+                const hoverifier = createHoverifier({
+                    closeButtonClicks: new Observable<MouseEvent>(),
+                    goToDefinitionClicks: new Observable<MouseEvent>(),
+                    hoverOverlayElements: of(null),
+                    hoverOverlayRerenders: EMPTY,
+                    fetchHover,
+                    fetchJumpURL,
+                    pushHistory: noop,
+                    logTelemetryEvent: noop,
+                })
+
+                const positionJumps = new Subject<{
+                    position: Position
+                    codeView: HTMLElement
+                    scrollElement: HTMLElement
+                }>()
+
+                const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))
+
+                const subscriptions = new Subscription()
+
+                subscriptions.add(hoverifier)
+                subscriptions.add(
+                    hoverifier.hoverify({
+                        dom: codeView,
+                        positionEvents,
+                        positionJumps,
+                        adjustPosition,
+                        resolveContext: () => codeView.revSpec,
+                    })
+                )
+
+                const inputDiagram = 'ab'
+                // There is probably a bug in code that is unrelated to this feature that is causing the PositionAdjuster to be called an extra time.
+                // It should look like '-(ba)'. That is, we adjust the position from CodeViewToActual for the LSP fetches and then back from CodeViewToActual
+                // for highlighting the token in the DOM.
+                const outputDiagram = 'a(ba)'
+
+                const outputValues: {
+                    [key: string]: AdjustmentDirection
+                } = {
+                    a: AdjustmentDirection.ActualToCodeView,
+                    b: AdjustmentDirection.CodeViewToActual,
+                }
+
+                cold(inputDiagram).subscribe(() =>
+                    clickPositionImpure(codeView, {
+                        line: 1,
+                        character: 1,
+                    })
+                )
+
+                expectObservable(adjustmentDirections).toBe(outputDiagram, outputValues)
+            })
+        }
+    })
 })

src/hoverifier.ts

@@ -115,11 +115,37 @@ export interface PositionJump {
     scrollElement: HTMLElement
 }
 
+/**
+ * The possible directions to adjust a position in.
+ */
+export enum AdjustmentDirection {
+    /** Adjusting the position from what is found on the page to what it would be in the actual file. */
+    CodeViewToActual,
+    /** Adjusting the position from what is in the actual file to what would be found on the page. */
+    ActualToCodeView,
+}
+
+export interface AdjustPositionProps {
+    /** The code view the token is in. */
+    codeView: HTMLElement
+    /** The position the token is at. */
+    position: HoveredToken & HoveredTokenContext
+    /** The direction the adjustment should go. */
+    direction: AdjustmentDirection
+}
+
+/**
+ * Function to adjust positions coming into and out of hoverifier. It can be used to correct the position used in HoverFetcher and
+ * JumpURLFetcher requests and the position of th etoken to highlight in the code view. This is useful for code hosts that convert whitespace.
+ */
+export type PositionAdjuster = (props: AdjustPositionProps) => Observable<Position>
+
 /**
  * HoverifyOptions that need to be included internally with every event
  */
 export interface EventOptions {
     resolveContext: ContextResolver
+    adjustPosition?: PositionAdjuster
     dom: DOMFunctions
 }
 
@@ -296,6 +322,24 @@ export const createHoverifier = ({
         debounceTime(50),
         // Do not consider mouseovers while overlay is pinned
         filter(() => !container.values.hoverOverlayIsFixed),
+        switchMap(
+            ({ adjustPosition, codeView, resolveContext, position, ...rest }) =>
+                adjustPosition && position
+                    ? adjustPosition({
+                          codeView,
+                          position: { ...position, ...resolveContext(position) },
+                          direction: AdjustmentDirection.CodeViewToActual,
+                      }).pipe(
+                          map(({ line, character }) => ({
+                              codeView,
+                              resolveContext,
+                              position: { ...position, line, character },
+                              adjustPosition,
+                              ...rest,
+                          }))
+                      )
+                    : of({ adjustPosition, codeView, resolveContext, position, ...rest })
+        ),
         share()
     )
 
@@ -305,6 +349,24 @@ export const createHoverifier = ({
             target: event.target as HTMLElement,
             ...rest,
         })),
+        switchMap(
+            ({ adjustPosition, codeView, resolveContext, position, ...rest }) =>
+                adjustPosition && position
+                    ? adjustPosition({
+                          codeView,
+                          position: { ...position, ...resolveContext(position) },
+                          direction: AdjustmentDirection.CodeViewToActual,
+                      }).pipe(
+                          map(({ line, character }) => ({
+                              codeView,
+                              resolveContext,
+                              position: { ...position, line, character },
+                              adjustPosition,
+                              ...rest,
+                          }))
+                      )
+                    : of({ adjustPosition, codeView, resolveContext, position, ...rest })
+        ),
         share()
     )
 
@@ -370,7 +432,13 @@ export const createHoverifier = ({
     const hoverObservables = resolvedPositions.pipe(
         map(({ position, ...rest }) => {
             if (!position) {
-                return of({ ...rest, hoverOrError: null, part: undefined })
+                return of({
+                    // Typescript seems to give up on type inference if we don't explicitely declare the types here.
+                    hoverOrError: null as 'loading' | HoverMerged | Error | null | undefined,
+                    position: undefined as (HoveredToken & HoveredTokenContext) | undefined,
+                    part: undefined,
+                    ...rest,
+                })
             }
             // Fetch the hover for that position
             const hoverFetch = fetchHover(position).pipe(
@@ -392,15 +460,46 @@ export const createHoverifier = ({
                     takeUntil(hoverFetch)
                 ),
                 hoverFetch
-            ).pipe(map(hoverOrError => ({ ...rest, hoverOrError, part: position.part })))
+            ).pipe(
+                map(hoverOrError => ({
+                    ...rest,
+                    position,
+                    hoverOrError,
+                    part: position.part,
+                }))
+            )
         }),
         share()
     )
     // Highlight the hover range returned by the language server
     subscription.add(
         hoverObservables
-            .pipe(switchMap(hoverObservable => hoverObservable))
-            .subscribe(({ hoverOrError, codeView, dom, part }) => {
+            .pipe(
+                switchMap(hoverObservable => hoverObservable),
+                switchMap(({ hoverOrError, position, adjustPosition, ...rest }) => {
+                    if (!HoverMerged.is(hoverOrError) || !hoverOrError.range || !position) {
+                        return of({ hoverOrError, position: undefined as Position | undefined, ...rest })
+                    }
+
+                    // LSP is 0-indexed, the code here is currently 1-indexed
+                    const { line, character } = hoverOrError.range.start
+                    const pos = { line: line + 1, character: character + 1, ...position }
+
+                    if (!adjustPosition) {
+                        return of({ hoverOrError, position: pos, ...rest })
+                    }
+
+                    return adjustPosition({
+                        codeView: rest.codeView,
+                        direction: AdjustmentDirection.ActualToCodeView,
+                        position: {
+                            ...position,
+                            part: rest.part,
+                        },
+                    }).pipe(map(position => ({ position, hoverOrError, ...rest })))
+                })
+            )
+            .subscribe(({ hoverOrError, position, codeView, dom, part }) => {
                 container.update({
                     hoverOrError,
                     // Reset the hover position, it's gonna be repositioned after the hover was rendered
@@ -410,12 +509,11 @@ export const createHoverifier = ({
                 if (currentHighlighted) {
                     currentHighlighted.classList.remove('selection-highlight')
                 }
-                if (!HoverMerged.is(hoverOrError) || !hoverOrError.range) {
+                if (!position) {
                     return
                 }
-                // LSP is 0-indexed, the code here is currently 1-indexed
-                const { line, character } = hoverOrError.range.start
-                const token = getTokenAtPosition(codeView, { line: line + 1, character: character + 1 }, dom, part)
+
+                const token = getTokenAtPosition(codeView, position, dom, part)
                 if (!token) {
                     return
                 }