Add bz_eReloadEvent API event #214
Conversation
There was a problem hiding this comment.
It is not clear to me how/when the worldEventManager should interact here. Some events appear to do normal server-side stuff then dispatch to the event manager to (I guess) allow plugins to have a go. Some of the new calls dispatch first, then do server-side stuff, potentially undoing what a plugin might do.
Seems to me that what is intended to be allowed by plugins ought to be clear in the API. I'm not well versed in the API, but it's confusing to the novice.
| { | ||
| bz_ReloadData_V1 event; | ||
| event.target = "all"; | ||
| worldEventManager.callEvents(bz_eReloadEvent, &event); |
There was a problem hiding this comment.
since each method individually calls load events, is this redundant?
There was a problem hiding this comment.
technically no, since this will send "all" and each call below will send a different string, but if "all" is exploded on the other side then ...
There was a problem hiding this comment.
Yea, I was thinking that if we want to listen to an explicit bz_reloadAll() or /reload all, it'd be useful to know when that happens even though it would trigger subsequent bz_eReloadEvent.
| @@ -2762,13 +2762,20 @@ bool ReloadCommand::operator() (const char *message, | |||
|
|
|||
| logDebugMessage(3,"Command is %s\n", cmd.c_str()); | |||
|
|
|||
| bz_ReloadData_V1 event; | |||
| event.playerID = t; | |||
| event.target = cmd.c_str(); | |||
There was a problem hiding this comment.
does this need validation prior to being shoved down the event manager?
There was a problem hiding this comment.
good call, this would allow for event.target to be "all" at the moment, I can fix that
| @@ -2805,8 +2812,14 @@ bool ReloadCommand::operator() (const char *message, | |||
| } | |||
| else | |||
| { | |||
| // Allow a plug-in to take over | |||
There was a problem hiding this comment.
Assuming the unconditional event manager call is correct, then shouldn't this be moved immediately after it?
I apologize for not being clear on how the event manager interacts; is the point to "do what we do" and allow plugins to extend, or do we allow plugins to override?
|
|
||
| bz_ReloadData_V1 event; | ||
| event.target = "bans"; | ||
| worldEventManager.callEvents(bz_eReloadEvent, &event); |
There was a problem hiding this comment.
as noted below, what's the proper interaction with the event manager? Should the call be before the acl.load() like this, meaning this might potentially undo a plugin action, or should the event manager call be after the defined action? Or should the event be tested to see if it was handled?
|
|
||
| bz_ReloadData_V1 event; | ||
| event.target = "masterbans"; | ||
| worldEventManager.callEvents(bz_eReloadEvent, &event); |
There was a problem hiding this comment.
this method can short-circuit (line 4320 above), meaning the event manager will not dispatch this message. Is that right?
|
@jwmelto since the remaining comments are related, my thinking was that As for the Whether the event should be fired before or after the action, I'm on the fence about. Thoughts? |
|
I don’t know enough to have an informed opinion. As long as the intent is clearly commented and consistent, I defer to those of you who write plugins. Is the distinction between notification and overridable behavior prevalent? Should the event name reflect that, like |
This PR will allow plug-ins to safely and easily hook into
/reloadandbz_reload*()events. This will also allow for custom/reloadsubcommands without needing to overload the/reloadcommand.The reason behind this change is to allow plugins to have custom reload functionality.