-
-
Notifications
You must be signed in to change notification settings - Fork 432
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
Added a method to get the value of whether an event's broadcasting di… #946
Conversation
This information is already included in the callback arguments - what are you doing that needs this info from the event handle? |
There is one problem: in another plugin, the notification for the event is disabled, but in another plugin, in the same event, it returns a different value when checked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - that is a pretty annoying limitation of the param on the pre-hook, and something we'll need to document.
The core of this is fairly good, but I've left a couple of small inlines for changes before we can get this merged.
core/smn_events.cpp
Outdated
@@ -466,6 +483,7 @@ REGISTER_NATIVES(gameEventNatives) | |||
{"Event.GetInt", sm_GetEventInt}, | |||
{"Event.GetFloat", sm_GetEventFloat}, | |||
{"Event.GetString", sm_GetEventString}, | |||
{"Event.BroadcastDisabled.get", sm_GetEventBroadcast}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this down under Event.BroadcastDisabled.set
please. It's more readable to keep the properties together.
plugins/include/events.inc
Outdated
* @return The boolean value of the whether an event's broadcasting disabled. | ||
* @error Invalid Handle. | ||
*/ | ||
native bool GetEventBroadcast(Handle event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to avoid adding any more "legacy" API where possible, so could you remove this one and keep only the MethodMap property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed all comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Headline anything you'd like to add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! |
…sabled or not