-
Notifications
You must be signed in to change notification settings - Fork 2.8k
LSP: propagate willRenameFiles workspace edits during file rename (Java refactoring groundwork) #16848
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
base: master
Are you sure you want to change the base?
LSP: propagate willRenameFiles workspace edits during file rename (Java refactoring groundwork) #16848
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.
Hey @AtharvGhumtane, thanks for the contribution.
As far as I can tell from the description, this does not actually fix #16748, right? Please update the description to no longer read "Fixes", because that would close the issue as soon as this PR is merged.
Aside from that, I'm a bit hesitant to review this PR since it's not clear whether this is the correct way forward. It makes sense to update the $onWillRunFileOperation to collect the edits, but the other changes - especially to the LanguagesExt - seem rather experimental. Looking at VSCode's interface, it also doesn't seem like they handle this feature through the Languages proxy.
If possible, I would kindly request from you to finish the feature implementation to verify that this is actually going to help fix the issue.
Also, please sign the Eclipse Contributor Agreement (ECA) with the same email that was used to create the commit. Otherwise, we won't be able to accept your changes. Thank you!
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.
Suggestion: We don't use yarn anymore in this project. Please remove this.
| if (renameEdits && renameEdits.length > 0) { | ||
| const mergedEdit = this.mergeWorkspaceEdits(renameEdits); | ||
| console.log('[LanguagesExt] Merged workspace edit:', mergedEdit); | ||
| return typeConverter.fromWorkspaceEdit(mergedEdit); | ||
| } |
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.
Suggestion: This merging is happening in all branches, can we remove the redundancy?
| }; | ||
| } | ||
|
|
||
| async $onWillRunFileOperation(operation: FileOperation, target: UriComponents, source: UriComponents | undefined, timeout: number, token: CancellationToken): Promise<any> { |
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.
Suggestion: Can we improve the typing of the return promise? Now that we return actual values here, using any is quite unsafe.
|
Thanks for the detailed review and for pointing this out. You’re right — this PR does not fully fix #16748. It only addresses the root cause by ensuring willRenameFiles workspace edits are propagated correctly through the plugin-ext pipeline. I will update the description to remove “Fixes #16748” and clarify that this is a foundational step toward a full solution. I’ll also:
And I’ll continue working on applying the returned edits so the refactoring actually takes effect end-to-end. Additionally, I’d really appreciate some guidance on the intended architecture for this feature:
I’d like to align the next steps with Theia’s intended design rather than guessing. Thanks again for the guidance. |
Whereever it makes sense - when possible it's best to keep them local to the plugin process, but that's not always possible. I would recommend to take a look at how VS Code is solving this issue.
It would be best to keep things similar to VS Code's design, as it makes future work easier.
Whatever it actually returns. Before your changes, it always returned |
|
Thanks a lot for the guidance, this is very helpful. I’ll take a closer look at how VS Code handles From an initial look, it seems VS Code keeps most of the edit handling within the extension host / workspace edit infrastructure rather than routing it through the Languages proxy, so I’ll re-evaluate my current approach in that direction. Regarding ownership: I’ll investigate whether the edit application should live in the plugin-ext filesystem event service, the languages main thread side, or a dedicated workspace edit application component similar to VS Code’s implementation. For the return type of I’ll push an update once I’ve aligned the implementation with VS Code’s model. Thanks again for the review and direction. |
Summary
This PR addresses the root cause of Java refactoring edits being lost during file rename/move operations in Theia.
Specifically, it ensures that
workspace/willRenameFilesedits produced by language servers are properly propagated through the plugin-ext pipeline instead of being discarded.Note: This PR does not fully fix #16748. It provides the missing infrastructure needed for a complete solution, but does not yet apply the returned edits to the workspace.
Problem
When renaming or moving Java files in Theia:
willRenameFilesevent is correctly triggeredEvidence from logs:
[LanguagesMain] File rename detected!
[LanguagesExt] willRenameFiles called: { files: [...] }
[LanguagesExt] Workspace edits returned (array): []
Root Cause
The plugin-ext file operation handler collected workspace edits from
_fireWillEvent(...)but did not return them back to the main thread. As a result, the propagated result was always an empty array.What this PR changes
$onWillRunFileOperationwillRenameFileshandlers are propagated correctlyyarn.lockTesting
Manual testing performed:
Limitations / Follow-ups
This PR does not yet:
These are required to fully resolve #16748 and will be addressed in follow-up work.
Related
Progress toward: #16748