GH-886 Sign editor (/sign setline <index> <text> command) #895
Conversation
Took 1 hour 0 minutes
Took 11 minutes
WalkthroughThe pull request introduces a sign editing feature for a Minecraft plugin, enhancing user interaction with signs. Key changes include the addition of a new package dedicated to sign editing functionality, along with command classes such as ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditCommand.java (2)
31-48: Consider extracting the block distance as a constant.The value 5 in
player.getTargetBlock(null, 5)should be defined as a named constant for better maintainability.+ private static final int SIGN_EDIT_DISTANCE = 5; void execute(@Context Player player, @Arg Integer index, @Join String text) { - Block targetBlock = player.getTargetBlock(null, 5); + Block targetBlock = player.getTargetBlock(null, SIGN_EDIT_DISTANCE);
50-58: Add null check for deserialized text.The deserialized text should be checked for null to prevent potential NPE.
- frontSide.setLine(index, this.miniMessage.deserialize(text).toString()); + String deserializedText = this.miniMessage.deserialize(text).toString(); + if (deserializedText != null) { + frontSide.setLine(index, deserializedText); + }eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
926-927: Fix Polish description in English translation file.The description comment is in Polish ("Ta sekcja...") but should be in English for consistency.
- @Description({ " ", "# Ta sekcja odpowiada za wiadomości dotyczące edycji tabliczek"}) + @Description({ " ", "# This section is responsible for sign editing messages"})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
🔇 Additional comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditCommand.java (1)
17-27: Clean command setup with proper dependency injection!eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java (1)
477-483: Well-structured translation interface section!The SignEditSection follows the established pattern and provides all necessary messages.
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (1)
945-954: Polish translations are well-implemented!The messages are properly translated and maintain consistency with the English version.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (2)
935-936: Polish comment in English translation file.The description comment is in Polish. Let's keep it in English for consistency.
- @Description({ " ", "# Ta sekcja odpowiada za wiadomości dotyczące edycji tabliczek"}) + @Description({ " ", "# This section is responsible for sign editing messages"})
941-943: Add descriptions for individual messages.Let's document the placeholders used in these messages to help other developers.
@Contextual public static class SignEditor { + @Description("# Message shown when no sign is found at target location") public Notice noSignFound = Notice.chat("<red>✘ <dark_red>Sign not found!"); + @Description("# Message shown when line index is not between 0-3") public Notice invalidIndex = Notice.chat("<red>✘ <dark_red>Invalid index!"); + @Description({" ", "# {LINE} - Line number (0-3)", "# {TEXT} - New text content"}) public Notice lineSet = Notice.chat("<green>► <white>Line <green>{LINE} <white>set to <green>{TEXT}"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
- eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (17)
🔇 Additional comments (4)
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (3)
957-959: LGTM! Section declaration looks good.The new sign editor section is properly declared and documented.
960-962: LGTM! Annotations are correctly applied.The
@Getterand@Contextualannotations are properly used, consistent with other sections.
963-966: LGTM! Messages are well-structured.The messages follow the plugin's style and include proper placeholders for dynamic content.
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
935-944: LGTM! The implementation follows the established patterns.The SignEditor class is well-structured and consistent with other translation sections in the file.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java (1)
34-34:⚠️ Potential issueAdd null check for target block
Same issue as in
SignEditorCommand- need to check if target block is null.
🧹 Nitpick comments (5)
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
937-938: Consider using English for all comments.The comment is in Polish ("Ta sekcja odpowiada za...") while the rest of the file uses English comments.
- @Description({ " ", "# Ta sekcja odpowiada za wiadomości dotyczące edycji tabliczek"}) + @Description({ " ", "# This section is responsible for sign editor messages"})eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java (1)
32-32: Consider making the range a constantThe block range of 5 would be clearer as a named constant.
+ private static final int SIGN_EDIT_RANGE = 5; + @Execute(name = "setline") void execute(@Context Player player, @Arg int index, @Join String text) { - Block targetBlock = player.getTargetBlock(null, 5); + Block targetBlock = player.getTargetBlock(null, SIGN_EDIT_RANGE);eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java (1)
19-22: Consider merging commands using inheritanceThis command shares a lot of code with
SignEditorCommand. Consider creating a base class to avoid duplication.eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java (2)
8-10: Add a quick comment about fluent accessorsConsider adding a brief comment explaining that this class uses fluent-style getters (like
messageInstance.noSignFound()instead ofmessageInstance.getNoSignFound()).
12-14: Make the message fields finalSince these are constant messages, consider making them final:
- public Notice noSignFound = Notice.chat("<red>✘ <dark_red>Sign not found, please look at the sign!"); - public Notice invalidIndex = Notice.chat("<red>✘ <dark_red>Invalid index!"); - public Notice lineSet = Notice.chat("<green>► <white>Line <green>{LINE} <white>set to <green>{TEXT}"); + public final Notice noSignFound = Notice.chat("<red>✘ <dark_red>Sign not found, please look at the sign!"); + public final Notice invalidIndex = Notice.chat("<red>✘ <dark_red>Invalid index!"); + public final Notice lineSet = Notice.chat("<green>► <white>Line <green>{LINE} <white>set to <green>{TEXT}");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/PLSignEditorMessages.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/SignEditorMessages.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java
- eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (17)
🔇 Additional comments (7)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/PLSignEditorMessages.java (2)
8-11: LGTM! Clean class setup with appropriate annotations.The class is well-structured with Lombok annotations for clean getter generation.
12-14: Messages are clear and user-friendly.The Polish translations are well-formatted with consistent styling using MiniMessage format.
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
5-6: LGTM! Appropriate imports added.The imports for sign editor message classes are correctly placed.
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/SignEditorMessages.java (1)
5-9: Nice and clean interface!The methods are clear and focused on their purpose.
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java (1)
34-34: Consider making the range a constantSame suggestion as in
SignEditorCommand- the block range should be a named constant.eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java (2)
1-7: Looks good!The package structure and imports are clean and well-organized.
11-11: Perfect class naming!Clear implementation of the messages interface with good naming convention.
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java
Outdated
Show resolved
Hide resolved
...alcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java
Outdated
Show resolved
Hide resolved
...alcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/signeditor/messages/PLSignEditorMessages.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java
Show resolved
Hide resolved
...alcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java
Outdated
Show resolved
Hide resolved
followed all instructions fixed all (i think) issues and critical errors now its working :), tested Took 32 minutes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java (2)
52-60: Make line number validation more user-friendlyThe line validation could be clearer by showing the valid range in the error message.
Consider adding the valid range to the placeholder:
- .placeholder("{LINE}", String.valueOf(line)) + .placeholder("{LINE}", String.valueOf(line)) + .placeholder("{MAX_LINES}", String.valueOf(signSide.getLines().length))And update the translation to include the range: "Line {LINE} is invalid. Please use 1-{MAX_LINES}."
43-50: Add distance info to the 'no sign found' messageHelp players understand why they can't edit a sign by mentioning the maximum reach distance.
Consider adding the distance to the error message:
.notice(translation -> translation.signEditor().noSignFound()) + .placeholder("{MAX_DISTANCE}", "5")And update the translation to: "No sign found within {MAX_DISTANCE} blocks."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/PLSignEditorMessages.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/PLSignEditorMessages.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java
🔇 Additional comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java (2)
21-24: Nice work on the command setup!The command structure is clean and well-organized with clear permissions and version requirements.
26-33: Clean dependency setup!Good use of dependency injection for the notice service and message formatting.
No description provided.