TShock icon indicating copy to clipboard operation
TShock copied to clipboard

Event Handler Normalization

Open QuiCM opened this issue 6 years ago • 6 comments

We have a multitude of different events, and a multitude of ways to hook onto them. Some things provide base dotnet events:

event EventHandler E; 
E += MyHandler;

Some things use a home-grown hook handler:

HandlerList<EventArgs> E;
E += MyHandler;
//or
E.Register(MyHandler);

Some things use TSAPI hook handling:

HandlerCollection<EventArgs> E;
E.Register(TerrariaPlugin, MyHandler);

Some things use OTAPI hook handling:

OTAPI.Hooks.Net.GetData = MyHandler;

We really need to decide which we like best and normalize on it.

QuiCM avatar Dec 21 '17 03:12 QuiCM

I think there's something to be said for the fact that standard C# events don't support the concept of priorities. Register with a method gives us that (unless someone can figure out a way to make an annotation do that?). However += might be good to have in the event that you don't really care for priority (things like Bukkit's Monitor priority are good for that).

hakusaro avatar Dec 21 '17 04:12 hakusaro

I support @up, we should have everything using .Register to support priorities because they're just required for some types of plugins like rate limiters or anticheats. Perhaps also a global class that has allows registering eventhandler classes? (another nod towards Bukkit)

bartico6 avatar Dec 22 '17 14:12 bartico6

@QuiCM for what it's worth, I would support this being merged with reasons for handling too, but I can see that being out of scope.

hakusaro avatar Dec 23 '17 04:12 hakusaro

From #1570: Events should be able to do something like Handle("Reason for preventing event propagation");

QuiCM avatar Dec 24 '17 01:12 QuiCM

Since hooking a boolean being changed isn't feasible in my opinion, I'd say that pointing at which plugin "dun did it" should be enough for now. Redoing how event handling works will break every single plugin in existence and that's bad.
The idea works on paper but in practice it'll be a pain to refactor and work with (and let's face it a lot of people are sitting at 4.3.25 (including me) because 4.3.26 is breaking enough, can we please not make it even worse for 4.3.27 or so?)
I think an event refactor is necessary, but I also think adding reasons to everything requires a lot of "re-understanding" the code you'll have to edit to get it working and that's unnecessary effort. Unless reasons are optional, then whatever :P

bartico6 avatar Feb 27 '18 19:02 bartico6

No longer WIP. As per above, this should probably be reserved for a new major version or similar.

QuiCM avatar Oct 31 '18 22:10 QuiCM