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#] Change generated On{SignalName} to EmitSignal{SignalName} #97588

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

TML233
Copy link
Contributor

@TML233 TML233 commented Sep 28, 2024

Change the generated On{SignalName}, which is introduced in #68233 , to EmitSignal{SignalName}
Editor already generates _on_{signal_name} for signal callbacks.
Having On{SignalName} introduces confusion and will likely break user code, since Godot users are already familiar with on signal name for signal callback functions.

Using EmitSignal{SignalName} conforms to Godot's name of raising signals emit_signal.
The terms and names are consistent between defining signals (using [Signal] attribute) and emitting signals (using EmitSignalName methods). Also it kinda looks great in the intellisense list, like:

  • EmitSignal (Godot's original emit_signal)
  • EmitSignalPressed
  • EmitSignalChanged

@TML233 TML233 requested a review from a team as a code owner September 28, 2024 13:04
@Delsin-Yu
Copy link
Contributor

Although I'm not entirely sure of the Emit{SignalName} naming scheme, but I do agree that we should not use the On{SignalName}, as I mentioned in the original PR that On{SignalName} is used for referring Callee, not the Caller in guidelines.
Reference: .Net Guidlines.

@pmoosi
Copy link

pmoosi commented Sep 28, 2024

According to the guidelines it should be On.... It's confusingly worded there, but on the events page is an example.

However, I think it's still worth considering if all the C# event guidelines should be followed as strictly. For me signals are not exactly the same as C# events. So using 'emit' instead of 'on' might be more in line with the Godot naming.

@Delsin-Yu
Copy link
Contributor

Oh... emit collides with Resource::emit_changed, so that's blocked.

@TML233 TML233 force-pushed the generated-raise-signal branch from e46bddf to fa48ed9 Compare September 28, 2024 14:10
@TML233 TML233 changed the title [C#] Change generated OnSignalName to EmitSignalName [C#] Change generated On{SignalName} to EmitSignal{SignalName} Sep 28, 2024
@Delsin-Yu
Copy link
Contributor

Or we just tread that emit_signal as a special, already implemented case and do not generate the corresponding wrapper method of it.

@TML233
Copy link
Contributor Author

TML233 commented Sep 28, 2024

Or we just tread that emit_signal as a special, already implemented case and do not generate the corresponding wrapper method of it.

I don't think it's appropriate. Resource::emit_changed has extra logic to notify ResourceLoader. We need to generate another method to just do emit_signal("changed")

@TML233
Copy link
Contributor Author

TML233 commented Sep 29, 2024

EmitSignal{SignalName} seemed a good choice.

@scgm0
Copy link
Contributor

scgm0 commented Oct 3, 2024

@raulsntos Can you check this PR? EmitSignal{SignalName} is more suitable for me than On{SignalName}, because I already use On{SignalName} as the signal listening method in my personal project

@scgm0
Copy link
Contributor

scgm0 commented Oct 4, 2024

This PR could resolve the compatibility issue mentioned in #97781, I honestly don't know why no one has reviewed this PR yet

@raulsntos raulsntos linked an issue Oct 4, 2024 that may be closed by this pull request
@paulloz paulloz modified the milestones: 4.x, 4.4 Oct 16, 2024
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.

We have discussed this and we think the On{SignalName} conflicts too much with existing projects. As others have mentioned in this thread, our own documentation recommends naming signal callbacks with the On{SignalName} pattern, so the chances to conflict are too high.

Renaming to EmitSignal{SignalName} is a lot less disruptive. We think we should avoid breaking existing user projects unnecessarily, so we're approving this change. Thanks for the contribution!

@Repiteo Repiteo merged commit cb3c01a into godotengine:master Oct 21, 2024
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 21, 2024

Thanks!

@TML233 TML233 deleted the generated-raise-signal branch October 27, 2024 03:42
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.

PR #68233 Broken project compatibility
8 participants