-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
Add server broadcast event #5617
base: dev/feature
Are you sure you want to change the base?
Conversation
Apart from built-in advertisements like Serverpro which will trigger this event, there’s no other way that this event will be triggered within the Skript environment as EffBroadcast does not use Server#broadcast(java.lang.String,java.lang.String), as shown here. |
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.
Nice PR ⚡
src/main/java/ch/njol/skript/expressions/ExprChatRecipients.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprChatRecipients.java
Outdated
Show resolved
Hide resolved
I wonder why Skript doesn't use https://hub.spigotmc.org/javadocs/spigot/org/bukkit/Server.html#broadcastMessage(java.lang.String) here? |
Likely because EffBroadcast also supports sending broadcasts to a specific world, which means special handling would be required to use broadcastMessage (and at the time it was added there was probably no incentive to do so, as sending the messages to each sender individually achieved the same effect) |
EffBroadcast could call the event manually. it would have to strip the formatting for the event, but thats probably fine. |
EffBroadcast could use Server#broadcast as it supports components now. |
That doesn't work for specific worlds, though, which would mean we'd still need to trigger the event manually. Maybe we'd be better off just calling the event manually in any case? |
Co-authored-by: Ayham Al Ali <[email protected]>
Co-authored-by: Ayham Al Ali <[email protected]>
src/main/java/ch/njol/skript/expressions/ExprChatRecipients.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Ayham Al Ali <[email protected]>
Co-authored-by: Ayham Al Ali <[email protected]>
Co-authored-by: Ayham Al Ali <[email protected]>
bd134d0
to
3f08853
Compare
@@ -113,8 +110,28 @@ public void change(Event event, @Nullable Object[] delta, ChangeMode mode) { | |||
case REMOVE_ALL: |
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.
I know that this is the current behavior, but why does REMOVE_ALL
as RESET
and DELETE
? remove all {_x} from the chat recipients
shouldn't just remove all the recipients. I don't believe it should be supported here at all anyway, as there can only be one of a recipient, not more.
Also shouldn't resetting the recipients add all players back in, rather than clearing them? Because that's what I interpret from the RESET
change mode - to reset the value to how it originally is; in this case all the players
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.
Sorry, why did you mark this as resolved?
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.
Not resolved
src/main/java/ch/njol/skript/expressions/ExprChatRecipients.java
Outdated
Show resolved
Hide resolved
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.
Looks good to me!
Co-authored-by: sovdee <[email protected]>
src/main/java/ch/njol/skript/expressions/ExprChatRecipients.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprChatRecipients.java
Outdated
Show resolved
Hide resolved
if (event instanceof AsyncPlayerChatEvent) { | ||
return ((AsyncPlayerChatEvent) event).getRecipients().toArray(new Player[0]); | ||
} else if (event instanceof BroadcastMessageEvent) { | ||
return ((BroadcastMessageEvent) event).getRecipients().toArray(new CommandSender[0]); | ||
} |
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 use instanceof patterns since we now support Java 17
@@ -113,8 +110,28 @@ public void change(Event event, @Nullable Object[] delta, ChangeMode mode) { | |||
case REMOVE_ALL: |
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.
Sorry, why did you mark this as resolved?
if (event instanceof AsyncPlayerChatEvent) { | ||
((AsyncPlayerChatEvent) event).getRecipients().clear(); | ||
} else { | ||
((BroadcastMessageEvent) event).getRecipients().clear(); | ||
} |
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 use instanceof pattern
src/main/java/ch/njol/skript/expressions/ExprChatRecipients.java
Outdated
Show resolved
Hide resolved
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.
Looks good, a couple of minor formatting bits :)
src/main/java/ch/njol/skript/expressions/ExprChatRecipients.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprChatRecipients.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Moderocky <[email protected]>
Co-authored-by: _tud <[email protected]>
Co-authored-by: Moderocky <[email protected]>
Co-authored-by: _tud <[email protected]>
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.
Looking better.
Description
Add server broadcast event
Target Minecraft Versions: none
Requirements: none
Related Issues: #1385 (closing as complete), #4185 (non-closing)