-
-
Notifications
You must be signed in to change notification settings - Fork 20
Notice`s package-private #1197
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?
Notice`s package-private #1197
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 primarily refactors message classes to use package-private visibility for Notice
fields, improving encapsulation. It also consolidates sound-playing logic for join, quit, and chat events into their respective controllers, which is a good structural improvement.
However, I've found a critical issue in BroadcastCommand
where the executeTitle
method has been refactored in a way that breaks its functionality by not replacing placeholders and introducing confusing behavior. I've also spotted a minor typo in the gradlew
script. My detailed feedback and suggestions are in the comments below.
this.noticeService.create() | ||
.notice(translation -> Notice.title(raw ? " " : translation.broadcast().messageFormat(), text, | ||
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.
The new implementation of executeTitle
seems to have a few issues:
- Placeholder Not Replaced: The title is set from
translation.broadcast().messageFormat()
when not in raw mode. This format string likely contains placeholders like{BROADCAST}
which are not being replaced. This will result in players seeing the literal placeholder string. - Unclear Functionality: The command has been changed to broadcast a "combined title message", and
executeSubtitle
has been removed. However, the implementation always uses the inputtext
as the subtitle, and the title is either a space or the unformatted format string. This is a confusing and likely incorrect behavior. It's also a breaking change that isn't documented in the PR description.
If the intention is for this command to now broadcast a subtitle (as executeSubtitle
was removed), you could reuse the sendBroadcast
helper method which correctly handles placeholder replacement.
Here is a suggested implementation that would make /broadcast title
behave like the old /broadcast subtitle
:
this.noticeService.create() | |
.notice(translation -> Notice.title(raw ? " " : translation.broadcast().messageFormat(), text, | |
this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut())) | |
.onlinePlayers() | |
.send(); | |
this.sendBroadcast( | |
formatted -> Notice.title("", formatted, this.settings.titleFadeIn(), this.settings.titleStay(), this.settings.titleFadeOut()), | |
text, | |
raw | |
); |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Make Notice`s package-private