New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hover actions from extensions, better handling of multiple def providers #1313

Merged
merged 1 commit into from Dec 31, 2018

Conversation

Projects
None yet
3 participants
@sqs
Copy link
Member

sqs commented Dec 10, 2018

The goal of this PR is to make basic-code-intel an awesome experience when you need go-to-definition, hovers, and find-references when browsing code in any language with no configuration. That means that the initial experience of site admins and users will be much better, because they get to see code intelligence with much less setup required.

This incorporates the codeintellify PR sourcegraph/codeintellify#70. That PR makes it so that codeintellify (which implements our hover tooltip) supports custom actions, instead of hardcoding "Go to definition" and "Find references".

This commit makes the web app (and TODO soon the browser extension) make use of that, so that:

  1. The "Go to definition" and "Find references" actions work in the same way in the web app and browser extension as all other action buttons. This means those actions are no longer hard-coded in a separate library.
  2. These buttons are selectively displayed in a smarter way based on (e.g.) whether there are any registered definition or reference providers for the current document. This fixes the issue where both buttons were always shown if there was a hover, even if the client app knew they would not return anything.
  3. The buttons can do smarter things, like opening up the definition panel if there are multiple definitions (instead of just jumping to the 1st definition).
  4. Extensions can add their own actions to the hover, such as "Find implementations", "Show history" (eg when a func, or calls to a func, were added/removed in Git history), "Show authors", "Show users", "Show stack traces", etc.

(I initially began implementing this so that basic-code-intel could add "Go to definition (fuzzy)" to the hover using (4), but I think that the improvements in (2) and (3) actually make that unnecessary for basic-code-intel. (It's still important to prioritize precise definitions in the panel; that will come soon.) See sourcegraph/sourcegraph-basic-code-intel#9 for the basic-code-intel changes that make use of this new behavior. (Note that there are no new extension APIs added in this PR, just improved handling of what already exists.))

TODOs before merging:

  • Update the browser extension impl code to use this
  • Add more tests to actions.test.ts
  • Fix issue where the input rev is discarded and the absolute commit ID SHA is used
  • Add back logTelemetryEvent for code intel action logging - make HoverOverlay record it

@sqs sqs force-pushed the hover-actions branch 3 times, most recently from 856cc5b to 63c8d9e Dec 10, 2018

@sqs sqs force-pushed the hover-actions branch from 63c8d9e to 2b2c7ca Dec 10, 2018

@sqs sqs requested a review from ijsnow as a code owner Dec 10, 2018

@sqs sqs force-pushed the hover-actions branch from 2b2c7ca to 41efcff Dec 11, 2018

@sqs sqs force-pushed the hover-actions branch 2 times, most recently from 97b1540 to c3537a2 Dec 12, 2018

@sqs sqs force-pushed the hover-actions branch 7 times, most recently from 9d5d08e to a852296 Dec 14, 2018

sqs added a commit that referenced this pull request Dec 16, 2018

support typed property values in Context (#1459)
This allows the context (which is used for `when` expressions and other expressions in an extension manifest) to contain properties whose value is a TypeScript type other than the primitive types (or Context itself). This is useful when, for example, you have a `TextDocumentPositionParams` value that is the value of a context property; using `Context<TextDocumentPositionParams>` makes it typecheck.

There is no user-facing behavior change. This is just a change to types, not impl code.

Part of #1313 but useful by itself.

@sqs sqs force-pushed the hover-actions branch 5 times, most recently from 3e5d928 to 8de1a70 Dec 18, 2018

@sqs sqs force-pushed the hover-actions branch from 8de1a70 to bef7930 Dec 22, 2018

@sqs sqs requested a review from dadlerj as a code owner Dec 22, 2018

eventAction === 'GoToDefClicked' ||
eventAction === 'goToDefinition' ||
eventAction === 'goToDefinition.preloaded' ||
eventAction === 'findReferences'

This comment has been minimized.

@dadlerj

dadlerj Dec 22, 2018

Member

If you prefer to change naming scheme, can we do it more comprehensively in a separate step, and stick with the old ones for now? Don't want this to look like a list that any ext authors can add to.

Perhaps we add a new category in the future for non-Sourcegraph extension usage tracking/displaying.

This comment has been minimized.

@sqs

sqs Dec 22, 2018

Author Member

It’s going to start tracking all actions run by extensions btw.

But these actions added here are builtin and not from extensions. For marking them actions as code intel, I either need to change the action ID in hover/actions.ts or here (and for technical reasons we need to distinguish between the preloaded j2d and non-preloaded, so we’d need add another entry here anyway). Ok with you now?

This comment has been minimized.

@dadlerj

dadlerj Dec 22, 2018

Member

K, I'd love to understand (and discuss) the full vision for telemetry from extensions in the future. Would like to avoid some of VS Code's mistakes.

For now this is fine of course. Can we delete the pre-existing predefined event names (like "GoToDefClicked") in this PR though if codeintellify is already updated?

This comment has been minimized.

@sqs

sqs Dec 23, 2018

Author Member

Yep, will delete the preexisting event names.

Check out https://github.com/sourcegraph/sourcegraph/pull/1559/files#diff-34372db774cd5ac796ed38d6db567d70 for what I was thinking around extension telemetry and leave your thoughts there. 👍

@sqs sqs force-pushed the hover-actions branch 7 times, most recently from 94f2643 to fa9db3f Dec 23, 2018

@sqs sqs changed the title DONOTREVIEW WIP: Hover actions from extensions Hover actions from extensions Dec 23, 2018

@sqs sqs changed the title Hover actions from extensions Hover actions from extensions, better handling of multiple def providers Dec 23, 2018

@sqs sqs force-pushed the hover-actions branch from fa9db3f to 29c6204 Dec 23, 2018

@sqs sqs referenced this pull request Dec 23, 2018

Closed

Update dependency @sourcegraph/codeintellify to v6 #1565

0 of 1 task complete

@sqs sqs force-pushed the hover-actions branch from 29c6204 to 5d0e56c Dec 23, 2018

@sqs sqs force-pushed the hover-actions branch 3 times, most recently from 4322dd5 to f0915f1 Dec 31, 2018

extension hover actions + implement "Go to definition" and "Find refe…
…rences"

The goal of this PR is to make [basic-code-intel](sourcegraph/sourcegraph-basic-code-intel#9) an awesome experience when you need go-to-definition, hovers, and find-references when browsing code in any language with no configuration. That means that the initial experience of site admins and users will be much better, because they get to see code intelligence with *much* less setup required.

**Depends on** the codeintellify PR sourcegraph/codeintellify#70. That PR makes it so that codeintellify (which implements our hover tooltip) supports custom actions, instead of hardcoding "Go to definition" and "Find references".

This commit makes the web app and browser extensin make use of that, so that:

1. The "Go to definition" and "Find references" actions work in the same way in the web app and browser extension as all other action buttons. This means those actions are no longer hard-coded in a separate library.
2. These buttons are selectively displayed in a smarter way based on (e.g.) whether there are any registered definition or reference providers for the current document. This fixes the issue where both buttons were always shown if there was a hover, even if the client app knew they would not return anything.
3. The buttons can do smarter things, like opening up the definition panel if there are multiple definitions (instead of just jumping to the 1st definition).
4. Extensions can add their own actions to the hover, such as "Find implementations", "Show history" (eg when a func, or calls to a func, were added/removed in Git history), "Show authors", "Show users", "Show stack traces", etc.

(I initially began implementing this so that basic-code-intel could add "Go to definition (fuzzy)" to the hover using (4), but I think that the improvements in (2) and (3) actually make that unnecessary for basic-code-intel. (It's still important to prioritize precise definitions in the panel; that will come soon.) See sourcegraph/sourcegraph-basic-code-intel#9 for the basic-code-intel changes that make use of this new behavior. (Note that there are *no* new extension APIs added in this PR, just improved handling of what already exists.))

@sqs sqs force-pushed the hover-actions branch from f0915f1 to 4f64986 Dec 31, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 31, 2018

Codecov Report

Merging #1313 into master will increase coverage by 0.4%.
The diff coverage is 89.48%.

Impacted Files Coverage Δ
shared/src/api/client/services/registry.ts 100% <ø> (ø) ⬆️
shared/src/actions/ActionItem.tsx 81.92% <ø> (ø) ⬆️
shared/src/api/protocol/contribution.ts 100% <100%> (ø) ⬆️
shared/src/hover/helpers.ts 60.86% <60.86%> (ø)
shared/src/hover/HoverOverlay.tsx 75.86% <75.86%> (ø)
shared/src/hover/actions.ts 91.89% <91.89%> (ø)
shared/src/hover/HoverOverlay.test.tsx 96.15% <96.15%> (ø)
shared/src/hover/actions.test.ts 96.96% <96.96%> (ø)
... and 4 more

@sqs sqs merged commit bd0ab33 into master Dec 31, 2018

1 check passed

buildkite/sourcegraph Build #25972 passed (6 minutes, 15 seconds)
Details

@sqs sqs deleted the hover-actions branch Dec 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment