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

Implement applyEdit in LSP for signal connecting #50378

Merged
merged 1 commit into from
Sep 25, 2021

Conversation

Razoric480
Copy link
Contributor

@Razoric480 Razoric480 commented Jul 11, 2021

As per the discussion in #42416, when an external text editor is set, the Connect Dialog fails to actually enact any change into the script. The workaround currently is to disable external editor, connect your signals, then re-enable it (or work in an external editor even with it turned off.)

This PR implements the workspace/applyEdit server request, which can cause a connected client to insert or replace text - in this case, a blank method stub. By connecting to the EditorNode, we can detect the "script_add_function_request" signal and pass the new function along to the client.

## Known Issues

Currently, once the script has been saved, its changes are not detected by Godot, so the "Go to method" menu item fails, and it will try to add duplicate functions if it already exists. This is fixed by the as yet unmerged #48615, which forces a resync by the server whenever a script is saved.

That PR has been merged in, so it's no longer an issue - so long as you save your script.

@Razoric480 Razoric480 requested a review from a team as a code owner July 11, 2021 20:15
@Arrow-x
Copy link

Arrow-x commented Sep 25, 2021

could this be cherry-picked to 3.x?

@Razoric480
Copy link
Contributor Author

I have a 3.x version already coded up, but want to wait for this to get approved before I push that one.

@akien-mga akien-mga merged commit 14dcb97 into godotengine:master Sep 25, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

This failed building after merge, seems like WorkspaceEdit is now defined twice: https://github.com/godotengine/godot/runs/3709439712?check_suite_focus=true

@Razoric480
Copy link
Contributor Author

That's embarrassing. I've implemented a fix and made sure it compiled successfully on latest. What's the protocol, should I open a new emergency PR with it?

@akien-mga
Copy link
Member

Yes, please do.

And no worry, the issue seems to stem from your previous implementation in #48615 which was merged in August, so was not available yet in this July PR. The lengthy PR review process is to blame :)

@Razoric480
Copy link
Contributor Author

Done and done at #53070 , once it passes automated checks.

@Razoric480 Razoric480 deleted the apply-edit-40 branch September 25, 2021 21:43
@PascalCzempiel
Copy link

I am on Version 3.4.4, and I still have this issue. I still need to do the "disable external editor" workaround. Did I misunderstand that this was supposed to be already fixed for 3.x as well? Or do I need to wait for version 4?

@Razoric480
Copy link
Contributor Author

This is fixed in 3.x. I just tested it just now and it popped a new function.
image
image
image

I am unsure why it wouldn't work.

@PascalCzempiel
Copy link

PascalCzempiel commented Apr 8, 2022

I am using C# and Visual Studio Code, but from reading the issues I was under the impression, that this fix should work for that language as well. Am I mistaken in the first place, or should I document my exact situation with screenshots?

@Razoric480
Copy link
Contributor Author

The language server is part of the gdscript module. It would need to be extended our refactored to support other languages.

@PascalCzempiel
Copy link

Ok, so, the bug is NOT fixed for C# yet, got it. Thanks for the quick answer though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants