From fe4f79869741ec3cb68ba90d4ef7653dd3999eb9 Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Fri, 10 Aug 2018 16:08:58 -0700 Subject: [PATCH 1/5] refactor: wip --- src/hoverifier.ts | 60 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/hoverifier.ts b/src/hoverifier.ts index a691464c..522ea89a 100644 --- a/src/hoverifier.ts +++ b/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 + /** * HoverifyOptions that need to be included internally with every event */ export interface EventOptions { resolveContext: ContextResolver + adjustPosition?: PositionAdjuster dom: DOMFunctions } @@ -296,6 +322,23 @@ 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 }, + ...rest, + })) + ) + : of({ adjustPosition, codeView, resolveContext, position, ...rest }) + ), share() ) @@ -305,6 +348,23 @@ 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 }, + ...rest, + })) + ) + : of({ adjustPosition, codeView, resolveContext, position, ...rest }) + ), share() ) From 8fb09445288c32412ace4532c69ebd0dcb59ff20 Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Sun, 12 Aug 2018 16:57:42 -0700 Subject: [PATCH 2/5] feat: wip add position adjuster --- src/hoverifier.ts | 52 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/src/hoverifier.ts b/src/hoverifier.ts index 522ea89a..2399bb76 100644 --- a/src/hoverifier.ts +++ b/src/hoverifier.ts @@ -334,6 +334,7 @@ export const createHoverifier = ({ codeView, resolveContext, position: { ...position, line, character }, + adjustPosition, ...rest, })) ) @@ -360,6 +361,7 @@ export const createHoverifier = ({ codeView, resolveContext, position: { ...position, line, character }, + adjustPosition, ...rest, })) ) @@ -429,11 +431,11 @@ export const createHoverifier = ({ */ const hoverObservables = resolvedPositions.pipe( map(({ position, ...rest }) => { - if (!position) { - return of({ ...rest, hoverOrError: null, part: undefined }) + if (!Position.is(position)) { + return of({ ...rest, hoverOrError: null, position, part: undefined }) } // Fetch the hover for that position - const hoverFetch = fetchHover(position).pipe( + const hoverFetch = fetchHover(position!).pipe( catchError(error => { if (error && error.code === EMODENOTFOUND) { return [null] @@ -452,15 +454,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, ...rest }) => { + if (!HoverMerged.is(hoverOrError) || !hoverOrError.range) { + 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 position = { line: line + 1, character: character + 1 } + + if (!adjustPosition) { + return of({ hoverOrError, position, ...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 @@ -470,12 +503,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 } From 2751517aa2054f8dc3d4bec764dc0f8392ce6764 Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Mon, 13 Aug 2018 08:56:54 -0700 Subject: [PATCH 3/5] feat: wip --- src/hoverifier.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/hoverifier.ts b/src/hoverifier.ts index 2399bb76..9216f16f 100644 --- a/src/hoverifier.ts +++ b/src/hoverifier.ts @@ -470,17 +470,17 @@ export const createHoverifier = ({ hoverObservables .pipe( switchMap(hoverObservable => hoverObservable), - switchMap(({ hoverOrError, ...rest }) => { - if (!HoverMerged.is(hoverOrError) || !hoverOrError.range) { + 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 position = { line: line + 1, character: character + 1 } + const pos = { line: line + 1, character: character + 1, ...position } if (!adjustPosition) { - return of({ hoverOrError, position, ...rest }) + return of({ hoverOrError, position: pos, ...rest }) } return adjustPosition({ From 8d3246bbd8ad1866d6d839abad561ff8b58dfff0 Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Mon, 13 Aug 2018 15:20:58 -0700 Subject: [PATCH 4/5] feat: position adjuster --- src/hoverifier.test.ts | 85 +++++++++++++++++++++++++++++++++++++++++- src/hoverifier.ts | 14 +++++-- 2 files changed, 94 insertions(+), 5 deletions(-) diff --git a/src/hoverifier.test.ts b/src/hoverifier.test.ts index fb8bd547..0a56f6b5 100644 --- a/src/hoverifier.test.ts +++ b/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,81 @@ 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 expecteds', () => { + for (const codeView of testcases) { + const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b)) + + scheduler.run(({ cold, expectObservable }) => { + const adjustmentDirections = new Subject() + + const fetchHover = createStubHoverFetcher({}) + const fetchJumpURL = createStubJumpURLFetcher('def') + + const adjustPosition: PositionAdjuster = ({ direction, position }) => { + adjustmentDirections.next(direction) + + return of(position) + } + + const hoverifier = createHoverifier({ + closeButtonClicks: new Observable(), + goToDefinitionClicks: new Observable(), + 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. + 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) + }) + } + }) }) diff --git a/src/hoverifier.ts b/src/hoverifier.ts index 9216f16f..4467f504 100644 --- a/src/hoverifier.ts +++ b/src/hoverifier.ts @@ -431,11 +431,17 @@ export const createHoverifier = ({ */ const hoverObservables = resolvedPositions.pipe( map(({ position, ...rest }) => { - if (!Position.is(position)) { - return of({ ...rest, hoverOrError: null, position, part: undefined }) + if (!position) { + 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( + const hoverFetch = fetchHover(position).pipe( catchError(error => { if (error && error.code === EMODENOTFOUND) { return [null] @@ -459,7 +465,7 @@ export const createHoverifier = ({ ...rest, position, hoverOrError, - part: position!.part, + part: position.part, })) ) }), From f037f1c6190da94df923ce5d3c4fea48b3259c93 Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Tue, 14 Aug 2018 09:01:27 -0700 Subject: [PATCH 5/5] refactor: comments --- src/hoverifier.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/hoverifier.test.ts b/src/hoverifier.test.ts index 0a56f6b5..315ebaaa 100644 --- a/src/hoverifier.test.ts +++ b/src/hoverifier.test.ts @@ -121,7 +121,7 @@ describe('Hoverifier', () => { * 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 expecteds', () => { + it('PositionAdjuster gets called when expected', () => { for (const codeView of testcases) { const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b)) @@ -171,6 +171,8 @@ describe('Hoverifier', () => { 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: {