feat: hold down ctrl/meta to go-to-def on clicks

[dadlerj]

Video of how this works in dev: https://cl.ly/2k1n470y083b

My remaining concerns:

• The action of pressing cmd/ctrl now causes any currently displayed hover to disappear (but you can move off of the symbol, and then back on to bring it back). This can probably be cleaned up.

• My additions to position.ts seem a bit dangerous (adding global hoverTarget and hoverCodeView vars, in particular). Any thoughts on how to improve this? https://github.com/sourcegraph/codeintellify/compare/cmdj2d?expand=1#diff-baad18ed00346ed65d8f93a0f6579b20R33

• Get @francisschmaltz's eyes on styling (see https://github.com/sourcegraph/sourcegraph/pull/12850)

[codecov]

Codecov Report

Merging #42 into master will decrease coverage by 0.38%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
- Coverage   65.24%   64.86%   -0.39%     
==========================================
  Files          15       15              
  Lines         587      609      +22     
  Branches      155      157       +2     
==========================================
+ Hits          383      395      +12     
- Misses        204      214      +10

Impacted Files	Coverage Δ
src/positions.ts	51.72% <30%> (-11.44%)	⬇️
src/hoverifier.ts	62.84% <75%> (+0.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 178b32e...fdbd4c1. Read the comment docs.

[ijsnow]

Did you know about MouseEvent.metaKey or MouseEvent.ctrlKey? metaKey is true if command or the windows key was held down and ctrlKey is true if control was held down during the mouse event. See how we use these to determine whether to open a new tab or not when you click J2D before the jump url is finished loading.

The changes in positions.ts that add keyboard events and track currently hovered tokens seem like they could cause race conditions and are probably unnecessary with these properties :)

[ijsnow]

Why this change?

[dadlerj]

To enable this (can't next an observable): https://github.com/sourcegraph/codeintellify/pull/42/files#diff-8a128e9e8a5a8bb9767f5f5392391217R434

[ijsnow]

👍 btw you can just pass the subject in to subscribe instead of wrapping it in an arrow func and calling next on it

[ijsnow]

Plus if you use the metaKey and ctrlKey properties the changes will be less and you won't be getting the red x of doom from codecov. Bonus points if we add a test for this feature as I think it's do-able

[dadlerj]

So I DO use metaKey and ctrlKey for instances when mouseover events fire and the key was already held down.

The addition of keydown and keyup events (in positions.ts) is required, though, to update the formatting when the user presses the key after they're already hovering. Without those (which was in fact my initial/simple attempt), you had to press the key first and then mouseover, in that specific order, to see the formatting change. :(

[dadlerj]

And yes I absolutely need to add tests!

[ijsnow]

to update the formatting

By this do you mean underlining the token to look like a link?

[dadlerj]

Yep. Otherwise, nothing gets re-rendered on key presses. This might be an acceptable change given how much simpler the code would be, but it would be a bit weird to give the user no indication that their click has a different meaning.

[ijsnow]

Sorry, didn't read the code all that closely once I saw the changes in positions.ts. Should have kept reading and my question would have been answered!

I'd like to get rid of the usage of hoverTarget and hoverCodeView in there. That operator isn't intended to keep that kind of state around and we shouldn't treat that file as a stateful module(with the global variables).

I wonder if we could do something like take the latest PositionEvent and use the position from that to get the token to highlight. If we can do that, I'd say we should move the keyboard event handlers out of the positions file and probably into its own area. That file is actually subject to get removed at any point. We'll eventually remove that and switch to github.com/sourcegraph/event-positions when we have the time.

[francisschmaltz]

The colors clash right now. I'd like to change the light theme link to be #329AF0 or "Dodger Blue" from our style guide. Additionally, if the cmd/ctrl key is pressed and hovered over a link that will go-to-def, I think the tool tip should disappear since it crowds the UI and provides the same features.

[ijsnow]

Additionally, if the cmd/ctrl key is pressed and hovered over a link that will go-to-def, I think the tool tip should disappear since it crowds the UI and provides the same features.

It only adds the go to def support, not the other features. For that reason I think it should stay around. Also, the tooltip is there for the information it provides as well. The tooltips in vscode stay around when you hold down cmd.

[francisschmaltz]

Can we change the context of the tool tip to show the def like vscode does? If we could do this, I think it makes sense to remove the other options from the tool tip as you wouldn't have the cmd key pressed if it goes to def only to do something else. I think they're two distinct interactions that might be worth separating.

[ijsnow]

Can we change the context of the tool tip to show the def like vscode does

So vscode goes from showing a little bit of information to more information when you hold it down. We actually just show all the information we have about the token no matter what. I think this is better in our use case since the main goal is to understand code rather than write code.

So in other words, if the language server returns the definition, we show it already.

[felixfbecker]

I only took a brief look on this PR but I would not want to merge it in the current state. I can take a closer look once I’m back.

Initial thoughts:

• The globals in positions are problematic
• I think this should be possible without keyup/keydown. The effect should come on hover with held-down CMD and the action on click. If for some reason it doesn’t “rerender” (in quotes since we’re using DOM manipulation here, not React) we should fix that
• The public interface should not change (goToDefinitionClicks)
• Subjects can likely be replaced by operators
• @francisschmaltz VS Code I think fetches the line of the definition from the destination contents and shows that. I don’t think that is worth it for us. Personally, I don’t like that vS Code behaviour and don’t understand why they do it - the hover already shows me a superset of the information, the definition is usually cut off. Maybe they implemented it before they implemented hovers.