-
-
Notifications
You must be signed in to change notification settings - Fork 20
GH-1193 Simplified command (/broadcast) #1193
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
base: master
Are you sure you want to change the base?
GH-1193 Simplified command (/broadcast) #1193
Conversation
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.
Code Review
This pull request simplifies the /broadcast
command by consolidating the title and subtitle functionalities. My review identifies a potential bug where a format placeholder might be displayed literally to users and also suggests an improvement for code clarity by renaming a parameter. I've provided a single comprehensive code suggestion that addresses both points for a more robust and readable implementation.
void executeTitle(@Flag("-raw") boolean raw, @Join String subtitle) { | ||
|
||
@Execute(name = "subtitle") | ||
@DescriptionDocs(description = "Broadcasts a SUBTITLE message to all players.", arguments = "[-raw] <text>") | ||
void executeSubtitle(@Flag("-raw") boolean raw, @Join String subtitle) { | ||
this.sendBroadcast(formatted -> Notice.title("", formatted, this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut()), subtitle, raw); | ||
this.noticeService.create() | ||
.notice(translation -> Notice.title(raw ? " " : translation.broadcast().messageFormat(), subtitle, | ||
this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut())) | ||
.onlinePlayers() | ||
.send(); | ||
} |
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 have a couple of suggestions to improve this method:
- Parameter Naming: The parameter name
subtitle
is a bit confusing withinexecuteTitle
. For consistency with other command methods in this class (likeexecuteChat
) and for better readability, renaming it totext
would be more intuitive. - Placeholder Handling: The
translation.broadcast().messageFormat()
likely contains a{BROADCAST}
placeholder, based on its usage in other commands. In the current implementation, this placeholder would be displayed literally as part of the title. The intention seems to be to use the format as a prefix for the title, so the placeholder should be removed.
Here is a suggested implementation that incorporates these points:
void executeTitle(@Flag("-raw") boolean raw, @Join String subtitle) { | |
@Execute(name = "subtitle") | |
@DescriptionDocs(description = "Broadcasts a SUBTITLE message to all players.", arguments = "[-raw] <text>") | |
void executeSubtitle(@Flag("-raw") boolean raw, @Join String subtitle) { | |
this.sendBroadcast(formatted -> Notice.title("", formatted, this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut()), subtitle, raw); | |
this.noticeService.create() | |
.notice(translation -> Notice.title(raw ? " " : translation.broadcast().messageFormat(), subtitle, | |
this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut())) | |
.onlinePlayers() | |
.send(); | |
} | |
void executeTitle(@Flag("-raw") boolean raw, @Join String text) { | |
this.noticeService.create() | |
.notice(translation -> { | |
String title = raw ? " " : translation.broadcast().messageFormat().replace("{BROADCAST}", "").trim(); | |
return Notice.title(title, text, | |
this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut()); | |
}) | |
.onlinePlayers() | |
.send(); | |
} |
I simplified the
/broadcast
command by removing the subtitle argument and changing the title argument to a combined title (TITLE + SUBTITLE).