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

LSP: Update doc paths renamed by TextDocumentEdit #8924

Closed

Conversation

the-mikedavis
Copy link
Member

A language server might tell us to rename a file via a TextDocumentEdit when renaming a module for example. Previously if you had a buffer open for that file it would retain the old filename. With this change, the path for that document is updated if the file rename succeeds.

The actual change is quite small - we just need to do some shuffling of how the &mut Editor is passed around to satisfy the borrow checker.

Noticed this while looking into #8923

A language server might tell us to rename a file via a TextDocumentEdit
when renaming a module for example. Previously if you had a buffer open
for that file it would retain the old filename. With this change, the
path for that document is updated if the file rename succeeds.
@the-mikedavis the-mikedavis added C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 27, 2023
@pascalkuthe
Copy link
Member

I have the exact same patch lying around s locally (that I was still testing). I noticed some related RA bugs while working on this that I got fixed upstream first.

We also need to trigger file watcher notifications here which I also added.

@the-mikedavis
Copy link
Member Author

Ah ok, I will wait for your patch then!

@the-mikedavis the-mikedavis deleted the lsp-workspace-command-rename-set-doc-path branch November 27, 2023 16:46
@pascalkuthe
Copy link
Member

I am still not quite sure if just renaming internally is enough. We may have to send a close and open notification for the old and new path too (that is why I hadn't submitted yet). I doesn't seem to work as well as I would like yet and I suspect its related to that

@the-mikedavis
Copy link
Member Author

I'm also curious what we should do about unsaved modifications. Document::set_path is nice for that since it will preserve everything but maybe if there are unsaved modifications we should just open the newly renamed file as a new buffer and keep the old doc around in scratch?

@pascalkuthe
Copy link
Member

pascalkuthe commented Nov 27, 2023

I think setting the path is fine/exactly the right thing to do. It's pretty annoying renaming a module and having 20 buffers left open in incorrect paths (I ran into this often while refactoring which is why I was looking into this in the first place, I just closed and reopened helix because it was too annoying). I am mostly unsure if we need to communicate set_path to the LSP somehow

@pascalkuthe
Copy link
Member

(I think we would need to call text_document_did_close(old_path) text_document_did_open(new_path) basically)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client C-enhancement Category: Improvements 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.

2 participants