-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add initial support for LSP DidChangeWatchedFiles #7665
Add initial support for LSP DidChangeWatchedFiles #7665
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really promising, thanks for taking a stab at this! I will do a more thorough review later. One thing that I noticed immidietly us that I would prefer the filer handlr to be moved to helix-lsp
. You can store the handle in the registry (that's stored on the editor so you can still keep most of the rest the same)
Also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some minor technical commints the rest looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few style nits and a question but overall I think this looks good, I like the idea 👍
@the-mikedavis thanks for the review! I think I've addressed all feedback, but let me know if there's anything else you'd like me to change 🙂 |
* Add initial support for LSP DidChangeWatchedFiles * Move file event Handler to helix-lsp * Simplify file event handling * Refactor file event handling * Block on future within LSP file event handler * Fully qualify uses of the file_event::Handler type * Rename ops field to options * Revert newline removal from helix-view/Cargo.toml * Ensure file event Handler is cleaned up when lsp client is shutdown
* Add initial support for LSP DidChangeWatchedFiles * Move file event Handler to helix-lsp * Simplify file event handling * Refactor file event handling * Block on future within LSP file event handler * Fully qualify uses of the file_event::Handler type * Rename ops field to options * Revert newline removal from helix-view/Cargo.toml * Ensure file event Handler is cleaned up when lsp client is shutdown
Context: #1125 (comment)
This PR implements initial support for LSP DidChangeWatchedFiles by forwarding a notification for all file writes or reloads to any LSP that has registered with a matching glob. This work will enable LSPs to respond accordingly to changes made to any metadata files (e.g.
Cargo.toml
) that they have registered for notifications on. For now it can only forward notifications for changes that occur within Helix, however this code can continue to be used when a proper file watcher is added at some point in the future.For the implementation, a new file event
Handler
struct spawns a dedicated tokio task to listen for any registrations/unregistrations by LSPs as well as any file change events. Since theEditor
struct owns thehelix-lsp::Registry
instance, I saw a few potential designs for theHandler
to be able to access LSP clients. Ultimately, I chose to pass theHandler
aWeak
reference to the LSP clients as they register for file watching. This is done so that theHandler
can have direct access to the client without preventing the client from being dropped if it is closed and isn't properly notified. Alternatives could include synchronizing references to clients between theEditor
andHandler
, synchronizing access to the LSP clients, or notifying theEditor
of file change notifications that need to be sent to LSPs via a new channel. My opinion is that all of those alternatives seem more complex or bug-prone.There are certainly optimizations that can be made (e.g. batching file changes) but I think those can be added in future PRs, as necessary.
I've tested these changes successfully with the rust-analyzer and gopls LSPs on both macos and Linux. Changes to the
Cargo.toml
of a crate are immediately recognized by rust-analyzer, instead of having to restart the LSP.Thanks!