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

use redraw handle for debouncing LSP messages #7538

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Jul 4, 2023

See #7373 (comment) for prior discussion

Fixes helix not redrawing after receiving messages from the LSP by replacing the existing (buggy) special-purpose deboucing with using the redraw handle that was added as part of #3890

@pascalkuthe pascalkuthe added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much A-language-server Area: Language server client A-helix-term Area: Helix term improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 4, 2023
@@ -609,12 +603,7 @@ impl Application {
EditorEvent::LanguageServerMessage((id, call)) => {
self.handle_language_server_message(call, id).await;
// limit render calls for fast language server messages
let last = self.editor.language_servers.incoming.is_empty();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context: the is_empty here checks that the set of streams is empty rather than that there is no pending message ready to be handled. Detecting that case is a little involved and not very effective for debouncing (see #7373 (comment))

@@ -1703,7 +1703,7 @@ impl Editor {
_ = self.redraw_handle.0.notified() => {
if !self.needs_redraw{
self.needs_redraw = true;
let timeout = Instant::now() + Duration::from_millis(96);
let timeout = Instant::now() + Duration::from_millis(33);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change here?

Copy link
Member Author

@pascalkuthe pascalkuthe Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only disadvantage of this approach was that the rendering now has a longer render timeout (96ms instead of 16ms) which felt a bit slow. I changed it to 33ms to emulate 30FPS. It doesn't matter too much for diff gutters what that value is since diff gutters only use this as a fallback mechanism but I would prefer to not lower this too much more to avoid adding too many renders.

#7373 (comment)

We were rendering with approximately 60fps in the past (the debouncer wasn't exact but we were using 16 ms to debounce). The redraw handle used to have a longer timeout since its only used as a fallback for the diff gutters (basically if we get here its already slow) but the exact value doesn't matter too much. I lowered it so we at least keep rending with 30 FPS. I didn't want to go quite as low as 16 here, if we really care we could allow different users of the redraw handle to specify different timeouts. That seemed overkill tough but if you think that's worth it, it shouldn't be too hard

@archseer archseer merged commit 618620b into helix-editor:master Jul 7, 2023
6 checks passed
@pascalkuthe pascalkuthe deleted the debounce_lsp_messages branch July 7, 2023 21:50
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements A-language-server Area: Language server client C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants