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

make inlay hints typing debounce max delay configurable #145377

Closed
jhgg opened this issue Mar 18, 2022 · 19 comments
Closed

make inlay hints typing debounce max delay configurable #145377

jhgg opened this issue Mar 18, 2022 · 19 comments
Assignees
Labels
feature-request Request for new features or functionality inlay-hints on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded

Comments

@jhgg
Copy link
Contributor

jhgg commented Mar 18, 2022

recently, rust-analyzer switched to using vs code's inlay hints provider. however, due to:

const delay = Math.max(scheduler.delay, 1250);
there is a delay for inlay hints when typing.

However, I think that this actually makes the experience feel much worse if your language server is able to compute inlay hints quite quickly, and indeed users of the extension have noted this fact as well!

I have only noticed that after I took the screenshot, I also couldn't reproduce whatever glitch that was. Maybe it's worth noting though, that at least in my case I have noticed that the inlay hints take longer now to render in my VsCode for whatever reason.

rust-lang/rust-analyzer#11733 (comment)

This feature requests proposes to make the hard-coded 1250 be configurable via user settings.

@lnicola
Copy link
Contributor

lnicola commented Mar 18, 2022

So the reasoning here is to make them bounce less while editing the code. But if you make a change then move the caret, the hint for the original line still gets delayed.

Also, should that say Math.min? No, it seems intentional.

@jrieken jrieken added feature-request Request for new features or functionality inlay-hints labels Mar 18, 2022
@jrieken jrieken added this to the Backlog milestone Mar 18, 2022
@ejpcmac
Copy link

ejpcmac commented Mar 21, 2022

Why would you even want a delay at all for type hints for instance? It’s annoying to have the impression the editor is not responsive like it used to be.

@lnicola
Copy link
Contributor

lnicola commented Mar 21, 2022

@ejpcmac #133730 (comment)

@ejpcmac
Copy link

ejpcmac commented Mar 22, 2022

@ejpcmac #133730 (comment)

I don’t feel this is bad, even, this gives the feeling that the editor is really responsive. This is approximately what we had in rust-analyzer before they switched to the native inlay implementation, and I quite liked it. You don’t have to wait to see your changes. It seems not to be the taste of everyone though, so maybe we should need an option for this. However, what would be the default? Do the majority of users prefer reactivity or seeing the hints with a delay? How would the minority even be aware of such parameter to speed-up or slow-down their editor?

@lnicola
Copy link
Contributor

lnicola commented Mar 22, 2022

I don't know. It looks a bit bad in that recording, but I used rust-analyzer before and the inlay hints never bothered me. I think you get used to it very quickly. Just so it's clear, I'm not arguing in favour of delaying updates like this, I just linked to the original issue what prompted it.

before they switched

Yeah, umm... 😅.

@ejpcmac
Copy link

ejpcmac commented Mar 22, 2022

before they switched

Yeah, umm... sweat_smile.

Oh, you’re a rust-analyzer contributor :) I’m not, hence the “they”. Hope it was not understood as a critique: it’s not. It’s a good thing to use the standard APIs, it just has currently some side effects in term of UX.

@hammypants

This comment was marked as spam.

@cutsoy
Copy link

cutsoy commented Oct 8, 2022

I'm working on a LSP that's not rust-analyzer and I also notice the delay. In my case, the hints are very fast to compute (less than 3ms) and the hints are always shown at the end of a line. E.g.:

const example = true        # Bool

Therefore, it wouldn't cause any editor unresponsiveness nor the bouncing layout that's mentioned earlier.

On the other hand, the current approach (waiting 1250ms) does cause a jittery layout when inserting a newline. In my example above, the inlay hint floats onto the wrong line while typing for 1250ms:

const example = true        
const newline = fal# Bool

In my opinion, this behavior is much less user friendly than updating the inlay hints in real time.

As a temporary work around, I tried sending a "workspace/inlayHint/refresh" request but that is either a no-op or suffers the same debounce as mentioned before.

Pragmatically, I see three solutions:

  1. Remove the debounce delay all-together (and leave it up to the language server to introduce a delay if deemed necessary).
  2. Make it possible for extensions to customize their own debounce delay.
  3. Skip the debounce delay when the LSP sends an inlay hint refresh request.

@Veykril Veykril mentioned this issue Nov 8, 2022
5 tasks
@jhgg
Copy link
Contributor Author

jhgg commented Dec 29, 2022

Hey, @jrieken - was wondering when you would have a chance to look at this. Happy to update my original PR that implements this feature, or take it in an entirely different direction. Just need some feedback here. Thank you!

@jrieken
Copy link
Member

jrieken commented Jan 3, 2023

or take it in an entirely different direction.

That. I have added the artificial delay because rapid updates as type were perceived as very nervous, flickery and annoying. I believe that's because your cursor jumps (by more than what you type) as you type. This is esp weird when the cursor moves left as you type.

The large delay is a workaround and meant to catch typing-pauses. I don't think delaying (at whatever rate) is the best solution. A better solution that comes to my mind is to update inlay hints immediately but exempt the lines at which you have typed. Or apply the new inlay hints but don't change the widths of them for the lines at which you have typed (this only works when no new inlays are added)

@bjorn3
Copy link

bjorn3 commented Jan 3, 2023

Excempting the line on which you are typing wouldn't be an improvement IMHO as the main annoyance with the delays is that inlay hints are shown at locations where they shouldn't be, like in the middle of an identifier or when deleting text several inlay hints directly next to each other, due to them not being updated yet. This is happens precisely where you changed code.

@jrieken
Copy link
Member

jrieken commented Jan 3, 2023

There is two things here. synchronous updates and async updates. The former are done by the editor, as you type, and aren't affected by the delay or extension's performance. @bjorn3 The cases you describe, like deleting a block of text, should be covered by that and I consider mis-renderings a bug separate from the/this issue of "badly timed async updates". So, please file a separate issue with a small code sample and edit sequence that shows bad renderings as type. Thanks

@jhgg
Copy link
Contributor Author

jhgg commented Jan 3, 2023

I actually find the delay more annoying than flicker. Inlay hints being more responsive lets me see type information on code I'm typing sooner, which usually means I'm getting it right.

I very much preferred the zero-delay, show it as soon as the language server updates, which is why I added it as an option, because imo, it is a preference, and some people might prefer one thing or another.

It is notably one of the top complaints from when rust-analyzer switched to the built in LSP implementation.

@jhgg
Copy link
Contributor Author

jhgg commented Jan 3, 2023

Screen.Recording.2023-01-03.at.12.24.14.PM.mov

Here's an example, where the delay works against you. I'm writing code, and unable to see the types of the arguments until I literally stop typing and have to wait.

I do agree, there are those who would find the delay desirable, but there are those who find the delay to be annoying. I do not think that a good tool forces you to slow down and interrupt your thought process/flow to wait on an artificial delay for them to see information that will help them write the rest of their code.

I do not think there is a right or wrong here, which is why I think that an option that allows a user to configure the delay to their tastes is the best solution for all.

@jhgg
Copy link
Contributor Author

jhgg commented Jan 4, 2023

Would this be considered a misrendering bug?

Screen.Recording.2023-01-03.at.5.07.36.PM.mov

@jrieken jrieken added this to the September 2024 milestone Sep 2, 2024
@jrieken jrieken closed this as completed Sep 2, 2024
@jrieken
Copy link
Member

jrieken commented Sep 2, 2024

Fixed via #227377 which isn't a configurable delay but updates as type with fixed lengths.

Screen.Recording.2024-09-02.at.15.38.40.mov

@jrieken jrieken added the verification-needed Verification of issue is requested label Sep 23, 2024
@jrieken
Copy link
Member

jrieken commented Sep 23, 2024

Verify that inlay hints update as you type but make sure the width of those of the current line only changes after a short typing pause

@bpasero
Copy link
Member

bpasero commented Sep 25, 2024

This works for me when removing characters, but not when adding, example:

import * as path from 'path';

import { runTests } from '@vscode/test-electron';

async function main() {
	try {
		// The folder containing the Extension Manifest package.json
		// Passed to `--extensionDevelopmentPath`
		const extensionDevelopmentPath = path.resolve(__dirname, '../../');

		// The path to test runner
		// Passed to --extensionTestsPath
		const extensionTestsPath = path.resolve(__dirname, './suite/index');

		// Download VS Code, unzip it and run the integration test
		await runTests({ extensionDevelopmentPath, extensionTestsPath });

		let b = { aaaaaaaaaaaaaaaaaaa: true, b: false };
	} catch (err) {
		console.error('Failed to run tests');
		process.exit(1);
	}
}

main();

Keep on typing in the line let b = { aaaaaaaaaaaaaaaaaaa: true, b: false };

Image

@jrieken
Copy link
Member

jrieken commented Sep 25, 2024

Thanks @bpasero. Extracted that into #229677 - the general case should work but you hit an issue when trimming because of length would also happen

@connor4312 connor4312 added the verified Verification succeeded label Sep 25, 2024
@jrieken jrieken added the on-release-notes Issue/pull request mentioned in release notes label Sep 30, 2024
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Oct 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality inlay-hints on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants