Skip to content
This repository was archived by the owner on Nov 25, 2021. It is now read-only.

Commit bb3a5e0

Browse files
sqssqs
committed
fix: navigating with pinned hover unpins
fix sourcegraph/sourcegraph#1465 If the hover is pinned, a positionJump to an invalid position (i.e., to a location with no particular line/character, just a file) was not triggering a recheck of whether the hover should remain pinned. (It should only remain pinned if the jump destination also has a hover.)
1 parent a327422 commit bb3a5e0

File tree

2 files changed

+191
-59
lines changed

2 files changed

+191
-59
lines changed

‎src/hoverifier.test.ts‎

Lines changed: 163 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { isEqual } from 'lodash'
22
import { EMPTY, NEVER, Observable, of, Subject, Subscription } from 'rxjs'
33
import { distinctUntilChanged, filter, map } from 'rxjs/operators'
44
import { TestScheduler } from 'rxjs/testing'
5-
import { Position, Range } from 'vscode-languageserver-types'
5+
import { Range } from 'vscode-languageserver-types'
66

77
import { noop } from 'lodash'
88
import { propertyIsDefined } from './helpers'
@@ -12,6 +12,7 @@ import {
1212
LOADER_DELAY,
1313
MOUSEOVER_DELAY,
1414
PositionAdjuster,
15+
PositionJump,
1516
TOOLTIP_DISPLAY_DELAY,
1617
} from './hoverifier'
1718
import { HoverOverlayProps } from './HoverOverlay'
@@ -50,11 +51,7 @@ describe('Hoverifier', () => {
5051
getReferencesURL: () => null,
5152
})
5253

53-
const positionJumps = new Subject<{
54-
position: Position
55-
codeView: HTMLElement
56-
scrollElement: HTMLElement
57-
}>()
54+
const positionJumps = new Subject<PositionJump>()
5855

5956
const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))
6057

@@ -119,11 +116,7 @@ describe('Hoverifier', () => {
119116
getReferencesURL: () => null,
120117
})
121118

122-
const positionJumps = new Subject<{
123-
position: Position
124-
codeView: HTMLElement
125-
scrollElement: HTMLElement
126-
}>()
119+
const positionJumps = new Subject<PositionJump>()
127120

128121
const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))
129122

@@ -195,6 +188,161 @@ describe('Hoverifier', () => {
195188
}
196189
})
197190

191+
describe('pinning', () => {
192+
it('unpins upon clicking on a different position', () => {
193+
for (const codeView of testcases) {
194+
const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b))
195+
196+
const delayTime = 10
197+
198+
scheduler.run(({ cold, expectObservable }) => {
199+
const hoverifier = createHoverifier({
200+
closeButtonClicks: NEVER,
201+
goToDefinitionClicks: new Observable<MouseEvent>(),
202+
hoverOverlayElements: of(null),
203+
hoverOverlayRerenders: EMPTY,
204+
// Only show on line 24, not line 25 (which is the 2nd click event below).
205+
fetchHover: position =>
206+
position.line === 24 ? createStubHoverFetcher({}, delayTime)(position) : of(null),
207+
fetchJumpURL: position =>
208+
position.line === 24 ? createStubJumpURLFetcher('def url', delayTime)(position) : of(null),
209+
pushHistory: noop,
210+
getReferencesURL: () => null,
211+
})
212+
213+
const positionJumps = new Subject<PositionJump>()
214+
215+
const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))
216+
217+
const subscriptions = new Subscription()
218+
219+
subscriptions.add(hoverifier)
220+
subscriptions.add(
221+
hoverifier.hoverify({
222+
dom: codeView,
223+
positionEvents,
224+
positionJumps,
225+
resolveContext: () => codeView.revSpec,
226+
})
227+
)
228+
229+
const isPinned = hoverifier.hoverStateUpdates.pipe(
230+
map(
231+
hoverState =>
232+
!!hoverState.hoverOverlayProps && !!hoverState.hoverOverlayProps.showCloseButton
233+
),
234+
distinctUntilChanged()
235+
)
236+
237+
const outputDiagram = `${delayTime}ms a-c`
238+
const outputValues: {
239+
[key: string]: boolean
240+
} = {
241+
a: true,
242+
c: false,
243+
}
244+
245+
// Click (to pin) https://sourcegraph.sgdev.org/github.com/gorilla/mux@cb4698366aa625048f3b815af6a0dea8aef9280a/-/blob/mux.go#L24:6
246+
cold(`a`).subscribe(() => {
247+
dispatchMouseEventAtPositionImpure('click', codeView, {
248+
line: 24,
249+
character: 6,
250+
})
251+
})
252+
253+
// Click to another position and ensure the hover is no longer pinned.
254+
cold(`${delayTime}ms --c`).subscribe(() =>
255+
positionJumps.next({
256+
codeView: codeView.codeView,
257+
scrollElement: codeView.container,
258+
position: { line: 1, character: 1 },
259+
})
260+
)
261+
262+
expectObservable(isPinned).toBe(outputDiagram, outputValues)
263+
})
264+
}
265+
})
266+
267+
it('unpins upon navigation to an invalid or undefined position (such as a file with no particular position)', () => {
268+
for (const codeView of testcases) {
269+
const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b))
270+
271+
scheduler.run(({ cold, expectObservable }) => {
272+
const hoverifier = createHoverifier({
273+
closeButtonClicks: NEVER,
274+
goToDefinitionClicks: new Observable<MouseEvent>(),
275+
hoverOverlayElements: of(null),
276+
hoverOverlayRerenders: EMPTY,
277+
// Only show on line 24, not line 25 (which is the 2nd click event below).
278+
fetchHover: position =>
279+
position.line === 24 ? createStubHoverFetcher({})(position) : of(null),
280+
fetchJumpURL: position =>
281+
position.line === 24 ? createStubJumpURLFetcher('def url')(position) : of(null),
282+
pushHistory: noop,
283+
getReferencesURL: () => null,
284+
})
285+
286+
const positionJumps = new Subject<PositionJump>()
287+
288+
const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))
289+
290+
const subscriptions = new Subscription()
291+
292+
subscriptions.add(hoverifier)
293+
subscriptions.add(
294+
hoverifier.hoverify({
295+
dom: codeView,
296+
positionEvents,
297+
positionJumps,
298+
resolveContext: () => codeView.revSpec,
299+
})
300+
)
301+
302+
const isPinned = hoverifier.hoverStateUpdates.pipe(
303+
map(hoverState => {
304+
if (!hoverState.hoverOverlayProps) {
305+
return 'hidden'
306+
}
307+
if (hoverState.hoverOverlayProps.showCloseButton) {
308+
return 'pinned'
309+
}
310+
return 'visible'
311+
}),
312+
distinctUntilChanged()
313+
)
314+
315+
const outputDiagram = `ab`
316+
const outputValues: {
317+
[key: string]: 'hidden' | 'pinned' | 'visible'
318+
} = {
319+
a: 'pinned',
320+
b: 'hidden',
321+
}
322+
323+
// Click (to pin) https://sourcegraph.sgdev.org/github.com/gorilla/mux@cb4698366aa625048f3b815af6a0dea8aef9280a/-/blob/mux.go#L24:6
324+
cold(`a`).subscribe(() =>
325+
dispatchMouseEventAtPositionImpure('click', codeView, {
326+
line: 24,
327+
character: 6,
328+
})
329+
)
330+
331+
// Click to another position and ensure the hover is no longer pinned.
332+
cold(`-b`).subscribe(() =>
333+
positionJumps.next({
334+
codeView: codeView.codeView,
335+
scrollElement: codeView.container,
336+
position: { line: undefined, character: undefined },
337+
})
338+
)
339+
340+
expectObservable(isPinned).toBe(outputDiagram, outputValues)
341+
})
342+
}
343+
})
344+
})
345+
198346
it('emits loading and then state on click events', () => {
199347
for (const codeView of testcases) {
200348
const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b))
@@ -215,11 +363,7 @@ describe('Hoverifier', () => {
215363
getReferencesURL: () => null,
216364
})
217365

218-
const positionJumps = new Subject<{
219-
position: Position
220-
codeView: HTMLElement
221-
scrollElement: HTMLElement
222-
}>()
366+
const positionJumps = new Subject<PositionJump>()
223367

224368
const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))
225369

@@ -297,11 +441,7 @@ describe('Hoverifier', () => {
297441
getReferencesURL: () => null,
298442
})
299443

300-
const positionJumps = new Subject<{
301-
position: Position
302-
codeView: HTMLElement
303-
scrollElement: HTMLElement
304-
}>()
444+
const positionJumps = new Subject<PositionJump>()
305445

306446
const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))
307447

@@ -366,11 +506,7 @@ describe('Hoverifier', () => {
366506
getReferencesURL: () => null,
367507
})
368508

369-
const positionJumps = new Subject<{
370-
position: Position
371-
codeView: HTMLElement
372-
scrollElement: HTMLElement
373-
}>()
509+
const positionJumps = new Subject<PositionJump>()
374510

375511
const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))
376512

@@ -455,11 +591,7 @@ describe('Hoverifier', () => {
455591
getReferencesURL: () => null,
456592
})
457593

458-
const positionJumps = new Subject<{
459-
position: Position
460-
codeView: HTMLElement
461-
scrollElement: HTMLElement
462-
}>()
594+
const positionJumps = new Subject<PositionJump>()
463595

464596
const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))
465597

‎src/hoverifier.ts‎

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import {
2929
import { Key } from 'ts-key-enum'
3030
import { Position, Range } from 'vscode-languageserver-types'
3131
import { asError, ErrorLike, isErrorLike } from './errors'
32-
import { isDefined } from './helpers'
3332
import { overlayUIHasContent, scrollIntoCenterIfNeeded } from './helpers'
3433
import { HoverOverlayProps, isJumpURL } from './HoverOverlay'
3534
import { calculateOverlayPosition } from './overlay_position'
@@ -437,22 +436,23 @@ export function createHoverifier<C extends object>({
437436
// It's important to do this before filtering otherwise navigating from
438437
// a position, to a line-only position, back to the first position would get ignored
439438
distinctUntilChanged((a, b) => isEqual(a, b)),
440-
// Ignore undefined or partial positions (e.g. line only)
441-
filter((jump): jump is typeof jump & { position: Position } => Position.is(jump.position)),
442439
map(({ position, codeView, dom, ...rest }) => {
443-
const cell = dom.getCodeElementFromLineNumber(codeView, position.line, position.part)
444-
if (!cell) {
445-
return undefined
446-
}
447-
const target = findElementWithOffset(cell, position.character)
448-
if (!target) {
449-
console.warn('Could not find target for position in file', position)
450-
return undefined
440+
let cell: HTMLElement | null
441+
let target: HTMLElement | undefined
442+
let part: DiffPart | undefined
443+
if (Position.is(position)) {
444+
cell = dom.getCodeElementFromLineNumber(codeView, position.line, position.part)
445+
if (cell) {
446+
target = findElementWithOffset(cell, position.character)
447+
if (target) {
448+
part = dom.getDiffCodePart && dom.getDiffCodePart(target)
449+
} else {
450+
console.warn('Could not find target for position in file', position)
451+
}
452+
}
451453
}
452-
const part = dom.getDiffCodePart && dom.getDiffCodePart(target)
453454
return { ...rest, eventType: 'jump' as 'jump', target, position: { ...position, part }, codeView, dom }
454-
}),
455-
filter(isDefined)
455+
})
456456
)
457457

458458
// REPOSITIONING
@@ -482,7 +482,7 @@ export function createHoverifier<C extends object>({
482482
})
483483
),
484484
switchMap(({ position, codeView, adjustPosition, resolveContext, ...rest }) => {
485-
if (!position || !adjustPosition) {
485+
if (!position || !Position.is(position) || !adjustPosition) {
486486
return of({ position, codeView, ...rest })
487487
}
488488

@@ -500,19 +500,19 @@ export function createHoverifier<C extends object>({
500500
}))
501501
)
502502
}),
503-
map(({ target, position, codeView, dom, ...rest }) => {
504-
// We should ensure we have the correct dom element to place the overlay above.
505-
// It is possible that tokens span multiple elements meaning that its possible for the
506-
// hover overlay to be placed in the middle of a token.
507-
const token = position ? getTokenAtPosition(codeView, position, dom, position.part) : target
508-
509-
return {
510-
target: token || target,
511-
...rest,
512-
}
513-
}),
514-
map(({ hoverOverlayElement, relativeElement, target }) =>
515-
calculateOverlayPosition({ relativeElement, target, hoverOverlayElement })
503+
map(({ target, position, codeView, dom, ...rest }) => ({
504+
// We should ensure we have the correct dom element to place the overlay above. It is possible
505+
// that tokens span multiple elements meaning that it's possible for the hover overlay to be
506+
// placed in the middle of a token.
507+
target:
508+
position && Position.is(position)
509+
? getTokenAtPosition(codeView, position, dom, position.part)
510+
: target,
511+
...rest,
512+
})),
513+
map(
514+
({ hoverOverlayElement, relativeElement, target }) =>
515+
target ? calculateOverlayPosition({ relativeElement, target, hoverOverlayElement }) : undefined
516516
)
517517
)
518518
.subscribe(hoverOverlayPosition => {

0 commit comments

Comments
 (0)