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

Disable signal callback generation in C# #87952

Merged

Conversation

paulloz
Copy link
Member

@paulloz paulloz commented Feb 4, 2024

A while ago, we talked about disabling the signal callback generation altogether for C# until we have a better implementation. Reasons are, it currently generates the method outside the class (creates compilation error, obviously), and the behaviour is weirdly inconsistent depending on how you have external editors configured.

To prevent confusion like "the dialog didn't do the thing", I added a small message in the connection window, depending on the language of the script. Not sure if this is wanted, let me know (also, the message itself probably kinda sucks, feel free to suggest something better).

godot windows editor dev x86_64 mono_HnMDcxVcl2

Bugsquad edit:

@paulloz paulloz force-pushed the dotnet/byebye-signal-callback-generation branch 2 times, most recently from bd1dab1 to 4f95e07 Compare February 4, 2024 20:21
@paulloz paulloz requested a review from a team as a code owner February 4, 2024 20:21
Copy link
Contributor

@zaevi zaevi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me👍

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM code wise

editor/connections_dialog.cpp Outdated Show resolved Hide resolved
@paulloz paulloz force-pushed the dotnet/byebye-signal-callback-generation branch from 4f95e07 to 551c1c0 Compare February 7, 2024 17:53
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until we find a better workaround for this (such as displaying the code you should copy in a read-only TextEdit), this is probably for the better.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel a bit hesitant about adding new API to ScriptLanguage. I don't know if there are Script languages other than C# that would make use of this, adding this just for C# knowing that C# will eventually move away from the ScriptLanguage API feels wrong.

But if the area maintainers are fine with this, then I do think this is an improvement over the current state for C#.

modules/mono/csharp_script.cpp Outdated Show resolved Hide resolved
@paulloz
Copy link
Member Author

paulloz commented Feb 7, 2024

I feel a bit hesitant about adding new API to ScriptLanguage. I don't know if there are Script languages other than C# that would make use of this, adding this just for C# knowing that C# will eventually move away from the ScriptLanguage API feels wrong.

I had a similar feeling. We could simply always return an empty string and not display anything to the user. It'd be a bit worse UX, but still better than the current state, IMHO. In that case, I'd still modify the add_callback logic to not overwrite the file if make_function returns a string of length zero...

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to add this method. It does feel very specific, but find_function and make_function do too and they're already part of the API. Having a boolean check to control the user of those two methods doesn't seem too far fetched.

@paulloz paulloz force-pushed the dotnet/byebye-signal-callback-generation branch from 551c1c0 to 9fa2355 Compare February 14, 2024 12:26
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@akien-mga akien-mga merged commit 09df8f4 into godotengine:master Feb 15, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@dayo05
Copy link

dayo05 commented Jun 11, 2024

I have a question about this: is this breaks custom signal registration by code as introduced on docs?

@paulloz
Copy link
Member Author

paulloz commented Jun 11, 2024

I have a question about this: is this breaks custom signal registration by code as introduced on docs?

Hello! No, this only blocks the editor to write the empty method in your .cs file when you connect a signal to an inexistent method from Godot's UI.

@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release enhancement topic:dotnet topic:editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New signal methods in C# are declared outside of its class, making the script invalid
7 participants