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

Rewrite progress handling to allow for debouncing messages #571

Merged
merged 6 commits into from
May 9, 2024

Conversation

michaelpj
Copy link
Collaborator

This had to be redone in order to allow us to "wake up" and notice that there are pending messages. I also wrote it so there can be a stateful interface (the ProgressTracker) which I think might make it easier to use in that weird case in ghcide. I haven't exposed that yet, though.

@michaelpj
Copy link
Collaborator Author

@soulomoon what do you think? I think the ProgressTracker interface might be usable for that weird case in ghcide, since it lets you store it and call the update function from various places. The cost is that you have to deal with cancellation etc. yourself, but that seems maybe inevitable.

@michaelpj
Copy link
Collaborator Author

I also need to actually test this with a live HLS and see if I can actually notice the difference!

@michaelpj michaelpj force-pushed the mpj/progress-threads branch from c228abb to afedfb1 Compare May 6, 2024 17:16
This had to be redone in order to allow us to "wake up" and notice that
there are pending messages. I also wrote it so there can be a stateful
interface (the `ProgressTracker`) which I think might make it easier to
use in that weird case in `ghcide`. I haven't exposed that yet, though.
@michaelpj michaelpj force-pushed the mpj/progress-threads branch from afedfb1 to b81f25c Compare May 6, 2024 17:35
Copy link
Collaborator

@soulomoon soulomoon left a comment

Choose a reason for hiding this comment

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

Sounds good, it seems it is exactly the kind of flexibility HLS reports need.
But I am not sure about the global delay.

@@ -138,6 +140,10 @@ data LanguageContextEnv config = LanguageContextEnv
resState :: !(LanguageContextState config)
, resClientCapabilities :: !L.ClientCapabilities
, resRootPath :: !(Maybe FilePath)
, resProgressStartDelay :: Int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this can be set for each progress instead of globally? Since we might want different delay for different progress

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't actually think of a use case for this. I think it should be pretty generic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm inclined to leave it as global for now unless we have a need for it to be different.

@soulomoon
Copy link
Collaborator

soulomoon commented May 6, 2024

I also need to actually test this with a live HLS and see if I can actually notice the difference!

This piece of code in HLS is quite delicate, the previous version did not work well.
Maybe you can pick up my branch and adapt it a bit to see if the new api works ? haskell/haskell-language-server#4206

@michaelpj michaelpj force-pushed the mpj/progress-threads branch 2 times, most recently from 0a6728c to e65065a Compare May 8, 2024 21:42
@michaelpj michaelpj force-pushed the mpj/progress-threads branch from e65065a to 1b9cc1d Compare May 8, 2024 21:43
@michaelpj michaelpj marked this pull request as ready for review May 9, 2024 13:38
@michaelpj
Copy link
Collaborator Author

Turns out the stateful interface wasn't really that helpful and complicated things, I think haskell/haskell-language-server#4218 is actually perfectly fine with the normal interface, arguably even clearer.

@michaelpj michaelpj force-pushed the mpj/progress-threads branch from b05ee42 to e4ab8c5 Compare May 9, 2024 13:51
@michaelpj michaelpj enabled auto-merge (squash) May 9, 2024 13:51
@michaelpj michaelpj force-pushed the mpj/progress-threads branch from 4f63f44 to f45f29c Compare May 9, 2024 13:53
@michaelpj michaelpj merged commit de08abe into master May 9, 2024
14 checks passed
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