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

Feature request: option to disable inlay hints on lines with an active cursor #5903

Closed
raggi opened this issue Aug 28, 2020 · 15 comments
Closed
Labels
A-inlay-hints inlay/inline hints E-has-instructions Issue has some instructions and pointers to code to get started good first issue S-actionable Someone could pick this issue up and work on it right now

Comments

@raggi
Copy link

raggi commented Aug 28, 2020

Sometimes the inlay hints seem to cause refactoring/change troubles, a particularly common case is when adding or removing a type specifier on a let. I suspect this is in part due to my use of the vim plugin at the same time.

I realized today that a great way around this that would work well for me, would be an option to disable inlay hints on the active cursor line.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Aug 28, 2020

First, we need to add the cursor position to the inlay hints LSP request.
Luckily, for inlay hints we use a custom request, so we can adjust it the way we want to.

RA server counterpart is here: https://github.com/rust-analyzer/rust-analyzer/blob/6826dd044a5a08359197b2a006905eb8518494a2/crates/rust-analyzer/src/lsp_ext.rs#L188
VSCode counterpart is here: https://github.com/rust-analyzer/rust-analyzer/blob/6826dd044a5a08359197b2a006905eb8518494a2/editors/code/src/lsp_ext.ts#L91

Position should be enough, similar to the way it's used here: https://github.com/rust-analyzer/rust-analyzer/blob/6826dd044a5a08359197b2a006905eb8518494a2/crates/rust-analyzer/src/lsp_ext.rs#L62

What's very important here is not to break the backward compatibility: we cannot make this new parameter manadatory from the start, because otherwise other editors that use the same command (emacs, vim, etc.) will break and we don't want this.

Then, we need to process this parameter.
After this line, we have the converted inlay hints with their Position calculated:
https://github.com/rust-analyzer/rust-analyzer/blob/7e012ae8b54705bc254154cdff3a503cce3299a7/crates/rust-analyzer/src/handlers.rs#L1078
Keep in mind that there can be multiple cursors in VSCode placed at once, so we might need to pass multiple positions in the request.

Last thing to do would be to add a config entry for this behavior:
server part: https://github.com/rust-analyzer/rust-analyzer/blob/df54561a689a9eb7b1962b69a397a1221200c116/crates/ide/src/inlay_hints.rs#L14
VSCode client part: https://github.com/rust-analyzer/rust-analyzer/blob/816a39cb54766c1dc8960620142659e716f485dc/editors/code/src/config.ts#L99
VSCode defaults & documentation: https://github.com/rust-analyzer/rust-analyzer/blob/d58a3a277a1778ec33e492e958b52869510c1239/editors/code/package.json#L390

@SomeoneToIgnore SomeoneToIgnore added E-easy E-has-instructions Issue has some instructions and pointers to code to get started good first issue labels Aug 28, 2020
@Veetaha
Copy link
Contributor

Veetaha commented Aug 28, 2020

I think this feature might be less beneficial as it might seem at first glance.
First of all, it means we now need to poll for inlay hints on each cursor move instead of each character typed.
Second, it will be annoying to see inlay hints appear-and-disappear shaking the code around while you are just browsing the editor.
Third, we already have a feature to assign a shortcut for toggling the inlay hints (Rust Analyzer: Toggle inlay hints command), see this PR for more details: #4601

@Veetaha
Copy link
Contributor

Veetaha commented Aug 28, 2020

cc @matklad

@matklad
Copy link
Member

matklad commented Aug 28, 2020

First of all, it means we now need to poll for inlay hints on each cursor move instead of each character typed.

Not really, we always query all inlay hint, but just don't show some of them in the editor. This is purely presentation concern.

I am not sure that "disabling hints on an active line" is the best presentation possible. Some alternatives:

  • freeze the hint on the active line (if all the hints update with 100ms delay, make the active line hints update with 500ms delay)
  • freeze the size of hints on the active line. Change the text of the hints themselves, but pad then with whitespace/truncate to preserve the overall width to reduce jumps.

All three ideas seem non-trivial to implement though, the state management required seems tricky. But I might be wrong about this :)

@Veetaha
Copy link
Contributor

Veetaha commented Aug 28, 2020

(if all the hints update with 100ms delay, make the active line hints update with 500ms delay)

There is no delay in updating the inlay hints, as I said we poll for new inlay hints only when the user types a character or opens a new file.

Not really, we always query all inlay hint, but just don't show some of them in the editor. This is purely presentation concern.

My point was not in the amount of inlay hints we query, but the frequency. If we start tracking the cursor, we will need to reevaluate the inlay hints when the user just moves the cursor (currently when the user just moves the cursor the hints are not refetched).

@matklad
Copy link
Member

matklad commented Aug 28, 2020

There's no need to refetch the hints in either case. The only difference is in how the client presents the hints to the user, the client-server interaction is exactly the same in both cases.

Client stores a full hints model (which depends only on the contents of the file), and then renders the model, completely client-side, into a view (which depends on the model, the current position of cursors, and the set of timers (if we want to have "draw hints on the current line after a second" behavior)). Only model -> view needs to be recalculated on cursor movements, and, as it is purely client-side, it is cheap.

@flodiebold flodiebold added the S-actionable Someone could pick this issue up and work on it right now label Dec 21, 2020
@saiintbrisson
Copy link
Contributor

saiintbrisson commented Mar 9, 2021

I was playing around with the VSCode plugin and came up with this. Should it hide the chaining/params hints too?

MthOKCQ8Po

@SomeoneToIgnore
Copy link
Contributor

Looks nice!

I've imagined this feature removing all hints entirely, the original issue comment seems to mean the same.
Out of curiosity, does it work with multiple lines selected?

@saiintbrisson
Copy link
Contributor

It does! Multiple selections and cursors.
6UFPRelxcA

@SomeoneToIgnore
Copy link
Contributor

@saiintbrisson , those gifs above give an impression of a pretty much ready feature, so if you're able to create the PR out of that, please do it's very welcome.

@saiintbrisson
Copy link
Contributor

saiintbrisson commented Apr 22, 2021

This preview is still a bit buggy, I'll open a PR when I get it fixed. Thank you!

@c02y
Copy link

c02y commented Dec 13, 2021

Any update?

@AuracleTech
Copy link

AuracleTech commented Apr 13, 2022

This doesn't work "rust-analyzer.inlayHints.enable": false
image
Dont mind the transparent theme
image

@Veykril
Copy link
Member

Veykril commented Apr 14, 2022

What do the client logs say? Those log the settings it recognizes and sends to the server. Disabling works fine for me, so something might be overwriting your setting or not causing ti to get to the server.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Apr 14, 2022

Not related to the recent comments, but the issue itself can be closed now IMO: we don't control VSCode hint appearance or behaviour now.

@Veykril Veykril closed this as completed Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inlay-hints inlay/inline hints E-has-instructions Issue has some instructions and pointers to code to get started good first issue S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants