Save the current script when adding a new method via signal connection

[Calinou]

This makes it possible for external editors to pick up the changes. Most modern editors should reload the file automatically, but some older/lightweight editors may ask the user instead (or only warn after trying to save in the external editor).

This closes #41283.

[neikeq]

This does not fully solve the issue (at least for C#). It makes things better, but the external editor could have unsaved changes that would be lost when reloading from disk.
The best would be to communicate with the external editor to tell it to add the method. That's unlikely to be possible with the generic external editor system though, but it's something I plan to do with the C# editors.

[Calinou]

It makes things better, but the external editor could have unsaved changes that would be lost when reloading from disk.

Most editors I know of will generally display a conflict dialog in this case, rather than discarding the changes made in the external editor.

[Flavelius]

This auto-generation/insertion in general is likely a bad idea (possible duplicates, must work for every language and conflicts with IDE's, as mentioned above) and should be changed, a better one could be to just display a selection box of available methods to connect.

[Calinou]

@Flavelius The current "Make Function" feature already checks for existing methods before adding a new one. All we need to do is to make it add the method at the right place in C#.

It's a feature that's often relied upon, so I wouldn't remove it.

[siew24]

May I ask when will this pull request go through?

[Calinou]

Rebased and tested again, it works successfully.

May I ask when will this pull request go through?

Some pull requests take a while to be merged these days, mainly because the focus is on finalizing the essential features for 4.0.

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 3.4.

[geowarin]

This does not solve the problem for me with vscode.
The file opens, the signal is connected in the scene but the code is not there when the external editor opens.

Tested on:

• v4.0.dev.custom_build.482866775 (linux)
• v3.4.beta.custom_build.2660fafcc (win and linux)

[Calinou]

This does not solve the problem for me with vscode.
The file opens, the signal is connected in the scene but the code is not there when the external editor opens.

Can you check if the saved file contains the new code? If so, this means VS Code is not reloading the file correctly.

[geowarin]

Yes, I did :)
Maybe I'm doing it wrong, here is a video:

[Calinou]

@geowarin I don't know why this would be happening. There is no specific way to save a file to tell an editor to reload it. As long as the file is being written to, the editor should notice it on its own.

[geowarin]

Yes, I understand that. But as you can see the file is not written to (I'm doing a cat at the end on the file that is supposed to be modified)

[Calinou]

Yes, I understand that. But as you can see the file is not written to (I'm doing a cat at the end on the file that is supposed to be modified)

Ah, I didn't understand what was going on with the cat command on the screencap. If the file isn't saved, this means the PR isn't working as intended. This is likely because the script editor doesn't open at all when an external editor is configured, and this prevents the saving feature from working.

[geowarin]

Indeed, I don't seem to be hitting my breakpoint on save_current_script() when the external editor option is active.

[pablinos]

I'm using Neovim and can confirm, it's not making any changes to the file with a build of the 3.x branch.

This is likely because the script editor doesn't open at all when an external editor is configured, and this prevents the saving feature from working.

I wondered if this might be the case. Do you think there's something we might be able to do with the LSP to inform the external editor to insert the method?

[Calinou]

I wondered if this might be the case. Do you think there's something we might be able to do with the LSP to inform the external editor to insert the method?

I'm not sure if such a feature exists in the LSP specification.

[pablinos]

Yeah, I'm way out of my depth, to be honest, but perhaps the workspace/applyEdit method would work?

A WorkspaceEdit can contain one or more TextDocumentEdits which seems like the sort of thing we'd need.

[Calinou]

cc @Razoric480

[Razoric480]

We can grab the EditorNode singleton from within GDScriptWorkspace during initialize(), and listen to "script_add_function_request", then use that to send an applyEdit request with the newly formed function. I'll do some testing.

[Razoric480]

Well, after testing, I can note that in Godot 4.0, at least as of last pull, works without any changes to the language server. It's Godot 3.x that doesn't respond - it just opens the editor but doesn't actually do anything.

So I tried with the applyEdit route, but having issues getting applyEdit to do anything. The client receives it but doesn't do anything about it and sends no response. Got it to work.

[Razoric480]

I put a first draft in a branch if you'd like to take a look. Right now it does not support go-to-method, but it will write up a function body for editor signals.

Will not be needed for Godot 4.0, as it seems to do it just fine. Something seems to have gotten lost during the cherry-pick to 3.x.

[pablinos]

Wow, thanks @Razoric480, that's working well for me! ...at least with Neovim's LSP, but I'm guessing that VSCode etc, would work as well.

Will not be needed for Godot 4.0, as it seems to do it just fine.

Is that using this technique of saving the file? If so, would it be better to use the LSP? I saw somewhere that there were concerns the external editor may have unsaved edits, and this applyEdit approach could be a way to get around that.

I think there are other commands to ensure that the server has the most up to date edits, so we'd probably need to rely on that to make sure that the function gets inserted in the right place, or is there a way to say "append to the end"?

[Razoric480]

I tested it with VSCode, so yes, can confirm that it works there too.

It uses the technique of saving the file in Godot 4.0. We wouldn't want the editor to be aware of the language server as then it would introduce cross contamination, which goes against Godot's module system. I haven't really looked all that closely at what Godot does internally - I've just been the LSP guy.

[pablinos]

That's great it's working for VSCode too.

We wouldn't want the editor to be aware of the language server as then it would introduce cross contamination, which goes against Godot's module system

I'm not sure what you mean by that, but then as I said, I'm totally out of my depth and a complete novice! I thought the editor was already aware of the language server (for syntax errors, completions etc), and so it would be fine for the server to listen for events from the client informing it of what has changed - if the client supports that of course!

Doing this via the LSP rather than forcing a save of the file means that things like 'undo' will work better, and we're less likely to stamp on any changes that have been made in the editor. To me it feels like a slicker solution, but I know far less than you about the Godot internals.

I've just been the LSP guy.

Amazing, it works really well! This is a complete sidetrack, but one thing I've noticed is that Neovim is only displaying one error at a time. I get multiple warnings, but only the first error from the start of the script. Is that intentional? I can file a separate issue, if you think it's worth discussing further.

[Calinou]

but one thing I've noticed is that Neovim is only displaying one error at a time. I get multiple warnings, but only the first error from the start of the script. Is that intentional?

The GDScript compiler in 3.x only reports one error, but it'll be able to report multiple errors in 4.0 thanks to a complete rewrite. The editor's LSP server will likely have to be modified to support reporting this to clients in master.

[Razoric480]

Having done some more testing, I've realized what was going on because when I tested in 4.0 again, it stopped working.

If an external editor is not set, then Calinou's script goes off and the new method appears because the file got saved.

If an external editor is set, then nothing happens besides opening your script, and no method is added. Not without a call to the LSP anyway. So applyEdit should be added in regardless. Will finish it up and make a PR.