godot icon indicating copy to clipboard operation
godot copied to clipboard

[C#] Change generated On{SignalName} to EmitSignal{SignalName}

Open TML233 opened this issue 1 year ago • 8 comments

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 avatar Sep 28 '24 13:09 TML233

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.

Delsin-Yu avatar Sep 28 '24 13:09 Delsin-Yu

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.

pmoosi avatar Sep 28 '24 13:09 pmoosi

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

Delsin-Yu avatar Sep 28 '24 13:09 Delsin-Yu

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

Delsin-Yu avatar Sep 28 '24 14:09 Delsin-Yu

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 avatar Sep 28 '24 14:09 TML233

EmitSignal{SignalName} seemed a good choice.

TML233 avatar Sep 29 '24 15:09 TML233

@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 avatar Oct 03 '24 06:10 scgm0

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

scgm0 avatar Oct 04 '24 13:10 scgm0

Thanks!

Repiteo avatar Oct 21 '24 21:10 Repiteo