Skip to content
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

Open
wants to merge 21 commits into
base: dev/feature
Choose a base branch
from
Open

Conversation

TheLimeGlass
Copy link
Collaborator

@TheLimeGlass TheLimeGlass commented Apr 19, 2023

Description

Add server broadcast event


Target Minecraft Versions: none
Requirements: none
Related Issues: #1385 (closing as complete), #4185 (non-closing)

@TheLimeGlass TheLimeGlass added the feature Pull request adding a new feature. label Apr 19, 2023
@TheLimeGlass TheLimeGlass mentioned this pull request Apr 19, 2023
59 tasks
@NotSoDelayed
Copy link
Contributor

NotSoDelayed commented Apr 19, 2023

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.

Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR ⚡

@AyhamAl-Ali
Copy link
Member

AyhamAl-Ali commented Apr 19, 2023

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.

I wonder why Skript doesn't use https://hub.spigotmc.org/javadocs/spigot/org/bukkit/Server.html#broadcastMessage(java.lang.String) here?

@bluelhf
Copy link
Contributor

bluelhf commented Apr 19, 2023

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.

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)

@Pikachu920
Copy link
Member

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.

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.

@TheLimeGlass
Copy link
Collaborator Author

EffBroadcast could use Server#broadcast as it supports components now.

@bluelhf
Copy link
Contributor

bluelhf commented Apr 20, 2023

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?

@TheLimeGlass TheLimeGlass requested a review from AyhamAl-Ali July 30, 2023 21:10
@Moderocky Moderocky force-pushed the master branch 3 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@Moderocky Moderocky changed the base branch from master to dev/feature September 18, 2023 10:37
@TheLimeGlass TheLimeGlass requested a review from sovdeeth October 17, 2023 08:15
@@ -113,8 +110,28 @@ public void change(Event event, @Nullable Object[] delta, ChangeMode mode) {
case REMOVE_ALL:
Copy link
Member

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

Copy link
Member

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?

Copy link
Member

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/ExprMessage.java Outdated Show resolved Hide resolved
Copy link
Member

@sovdeeth sovdeeth left a 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!

@sovdeeth sovdeeth added the 2.8 Targeting a 2.8.X version release label Dec 30, 2023
@sovdeeth sovdeeth added 2.9 Targeting a 2.9.X version release and removed 2.8 Targeting a 2.8.X version release labels Jan 5, 2024
@sovdeeth sovdeeth removed the 2.9 Targeting a 2.9.X version release label Jul 1, 2024
src/main/java/ch/njol/skript/events/SimpleEvents.java Outdated Show resolved Hide resolved
Comment on lines 64 to 68
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]);
}
Copy link
Member

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:
Copy link
Member

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?

Comment on lines +113 to +117
if (event instanceof AsyncPlayerChatEvent) {
((AsyncPlayerChatEvent) event).getRecipients().clear();
} else {
((BroadcastMessageEvent) event).getRecipients().clear();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use instanceof pattern

Copy link
Member

@Moderocky Moderocky left a 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 :)

Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants