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

C# signal delegates must be public #6700

Merged

Conversation

paulloz
Copy link
Member

@paulloz paulloz commented Jan 30, 2023

This is the largest doc edit you'll ever see 👀

@YuriSizov YuriSizov added area:manual Issues and PRs related to the Manual/Tutorials section of the documentation bug topic:dotnet labels Jan 30, 2023
@YuriSizov YuriSizov requested a review from raulsntos January 30, 2023 09:42
@raulsntos
Copy link
Member

I think we should actually generate the event with the same accessibility as the delegate, although I'm not sure if there are use cases for non-public signals.

@paulloz
Copy link
Member Author

paulloz commented Jan 30, 2023

Does it make sense, given that whatever the event accessibility is, the signal itself is always publicly accessible through the API?
Edit: To be honest, that's essentially true for all methods and property we pick up with source generation, but IDK why it feels different with signals/events 🤔

@raulsntos
Copy link
Member

Yeah, that's also true for Get/Set/Call/... The way I see those methods they are like reflection, if you use them you are opting out of the type safety and accessibility guarantees. Just like reflection, they are useful in some scenarios (e.g. GDScript interop) but otherwise I wouldn't recommend using them.

Signals are a bit different because we currently don't have a type safe alternative to EmitSignal, but I'd like to add one (see godotengine/godot#68233).

@paulloz
Copy link
Member Author

paulloz commented Jan 30, 2023

Signals are a bit different because we currently don't have a type safe alternative to EmitSignal, but I'd like to add one (see godotengine/godot#68233).

We'd still have to use .Connect for specific cases (e.g. making a connection with ConnectFlags.OneShot).

@raulsntos
Copy link
Member

raulsntos commented Jan 30, 2023

Isn't that the same as await ToSignal? Although that will also work with non-public signals.

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.

Anyway, I'm OK with this change since it's true that the restriction exists right now and can't guarantee it'd be lifted for 4.0.

@mhilbrunner mhilbrunner merged commit f416395 into godotengine:master Feb 1, 2023
@mhilbrunner
Copy link
Member

Thanks!

@paulloz paulloz deleted the csharp/signals-must-be-public branch February 7, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation bug topic:dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants