Skip to content
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

Avoid creating a multitoken when finding definitions #3125

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Dec 12, 2023

This will avoid showing a query-server popup on hovers. When someone does CTRL+hover on source code in a database archive, the definitions provider is triggered. The first time this is run, there will be a query that is invoked in order to find definitions. This can be a long process.

Previously, this query brought up a popup window that could be cancelled. However, this is confusing users since the query is automatically cancelled when the mouse hovers away from the element.

The downside of this PR is that even when find definitions is explicitly invoked, (using F12), there will still be no hover.

I think this is an improvement, but I am happy to discuss if others disagree.

Replace this with a description of the changes your pull request makes.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [n/a] [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@aeisenberg aeisenberg requested a review from a team as a code owner December 12, 2023 00:32
@robertbrignull
Copy link
Contributor

I think this is an improvement, but I am happy to discuss if others disagree.

I don't have strong opinions on what the behaviour should. I'm very happy to trust your judgement as you use this particular feature a lot more and are closer to the users. I think the code changes look fine.

@aeisenberg
Copy link
Contributor Author

Thanks. I'll add a changelog note and then merge. I'm assuming the two failing jobs are flakes.

This will avoid showing a query-server popup on hovers. When someone
does CTRL+hover on source code in a database archive, the definitions
provider is triggered. The first time this is run, there will be a query that
is invoked in order to find definitions. This can be a long process.

Previously, this query brought up a popup window that could be cancelled.
However, this is confusing users since the query is automatically cancelled
when the mouse hovers away from the element.

The downside of this PR is that even when find definitions is explicitly invoked,
(using F12), there will still be no hover.

I think this is an improvement, but I am happy to discuss if others disagree.
@aeisenberg aeisenberg merged commit e15f153 into main Dec 13, 2023
29 checks passed
@aeisenberg aeisenberg deleted the aeisenberg/no-multi-token branch December 13, 2023 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants