implementing player text input via AnvilUI; #196
Conversation
implementing server name and server host name changing via AnvilUI
WalkthroughThis pull request introduces enhancements to the server configuration and user interface system, focusing on in-game settings management through an anvil-based interface. It includes new classes such as Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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: 4
🧹 Nitpick comments (9)
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (2)
20-22: Potential collision and confusion with random coordinates.
Generating random coordinates (up to 1000) for the anvil location in a live environment can produce colliding interfaces or unexpected behavior if multiple instances use the same coordinates. Consider a more robust approach to ensure unique or intentionally placed coordinates.
47-49: Clarify whygetInventory()returnsnull.
ReturningnullfromgetInventory()might cause unexpected behavior in external plugins or code that rely onInventoryHolder. Consider returning a valid inventory or leaving a comment to explain the design choice.src/main/java/de/j/deathMinigames/listeners/AnvilListener.java (1)
105-112: Ensure consistent user experience when finalizing anvil output.
Using a single static display name for the output may confuse players if multiple anvil interactions occur simultaneously. Ensure that the output item is reset or context-specific for each relevant scenario.src/main/java/de/j/stationofdoom/listener/PlayerJoin.java (1)
74-83: DRY (Don’t Repeat Yourself) suggestion for null-check block.
Lines 74–83 and 85–93 share very similar code structures, differing only in whether the “hosted by” text is included. Extracting this into a helper method and passing in a boolean or the host string would improve maintainability.src/main/java/de/j/stationofdoom/util/Tablist.java (1)
20-21: Possible typo in variable naming.
Consider renaminghostetBytohostedByfor clarity and consistency with standard English.- private static String hostetBy = null; + private static String hostedBy = null;src/main/java/de/j/deathMinigames/main/Config.java (3)
120-128: Possible typo in 'HostetBy'.
The code checks and sets the Tablist configuration. Great approach!
However, the key"Tablist.HostetBy"may be a misspelling of"HostedBy", which could cause confusion.- if(config.contains("Tablist.HostetBy")) { - Tablist.setHostetBy(config.getString("Tablist.HostetBy")); + if(config.contains("Tablist.HostedBy")) { + Tablist.setHostetBy(config.getString("Tablist.HostedBy"));
337-346: Return an empty string instead ofnullto avoid confusion.
Returningnullcould cause unexpectedNullPointerExceptionin consumers ofgetServerName(). Consider providing a safe fallback.- return null; + return "";
355-363: Return a fallback to avoidnulland fix spelling.
LikegetServerName(), returningnullmay introduce edge-case errors.
Consider:-public String getHostetBy() { +public String getHostedBy() { if(Main.getPlugin().getConfig().contains("Tablist.HostetBy")) { return Main.getPlugin().getConfig().getString("Tablist.HostetBy"); } else { Main.getMainLogger().warning("HostetBy not found in config!"); - return null; + return ""; } }src/main/java/de/j/deathMinigames/listeners/InventoryListener.java (1)
253-258: New menu slots for server customization.
Cases 4 and 5 correctly branch to newly introduced anvil-based UIs. Ensure there is a suitable “back button” or user feedback once inputs are confirmed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/de/j/deathMinigames/dmUtil/DmUtil.java(1 hunks)src/main/java/de/j/deathMinigames/dmUtil/TestCMD.java(2 hunks)src/main/java/de/j/deathMinigames/listeners/AnvilListener.java(1 hunks)src/main/java/de/j/deathMinigames/listeners/InventoryListener.java(3 hunks)src/main/java/de/j/deathMinigames/main/Config.java(3 hunks)src/main/java/de/j/deathMinigames/settings/AnvilUI.java(1 hunks)src/main/java/de/j/deathMinigames/settings/GUI.java(0 hunks)src/main/java/de/j/deathMinigames/settings/MainMenu.java(4 hunks)src/main/java/de/j/stationofdoom/listener/PlayerJoin.java(2 hunks)src/main/java/de/j/stationofdoom/main/Main.java(1 hunks)src/main/java/de/j/stationofdoom/util/Tablist.java(3 hunks)src/main/resources/translations.json(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/j/deathMinigames/settings/GUI.java
🧰 Additional context used
📓 Learnings (2)
src/main/java/de/j/deathMinigames/listeners/InventoryListener.java (2)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#176
File: src/main/java/de/j/deathMinigames/listeners/InventoryListener.java:219-254
Timestamp: 2024-11-12T07:49:07.001Z
Learning: In `InventoryListener`, the `reloadInventory` method is overloaded intentionally with two variants: one for reloading the entire inventory and another for reloading only a single slot, to optimize efficiency by avoiding unnecessary full inventory reloads.
Learnt from: JonasFranke
PR: AtomPilz-DevTeam/StationofDoomPlugin#176
File: src/main/java/de/j/deathMinigames/listeners/InventoryListener.java:68-77
Timestamp: 2024-11-12T07:49:07.001Z
Learning: In the context of `InventoryClickEvent` in Paper, slot IDs are never negative, so it's acceptable to rely on this and omit checks for `slot >= 0`.
src/main/java/de/j/deathMinigames/main/Config.java (1)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#176
File: src/main/java/de/j/deathMinigames/listeners/JoinListener.java:28-32
Timestamp: 2024-11-12T07:49:07.001Z
Learning: In this project, concurrent writes to different places in the configuration are acceptable, and thread safety concerns with methods like `addNewPlayer` and `checkIfPlayerInFile` are not considered an issue due to their usage patterns.
🔇 Additional comments (21)
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (1)
40-44: Handle potential concurrency or repeated openings.
When multiple players open the anvil simultaneously or open the anvil quickly in succession at the same location, there's a chance of race conditions in populating items. Ensure that your plugin logic gracefully handles concurrent anvil openings and that overwriting item slots does not conflict with other interactions.src/main/java/de/j/deathMinigames/dmUtil/DmUtil.java (1)
5-14: No concerns with added imports.
The new imports for text components, materials, and inventories seem unused in the current diff but might support future enhancements. No issues identified so far.src/main/java/de/j/deathMinigames/listeners/AnvilListener.java (1)
48-48: Validate rename text.
Ensuring user input is sanitized or length-limited might be necessary to prevent potential client or server issues (e.g., extremely long messages in the tab list). Consider adding length checks or sanitization before assigninghostNameorserverName.src/main/java/de/j/stationofdoom/listener/PlayerJoin.java (2)
15-15: Import for Placeholder usage looks good.
This import correctly introduces the MiniMessage placeholder functionality you need for dynamic tab list updates.
84-93: Consistent tab list update with host name.
The logic to show host info is appropriate. However, as covered in the previous comment, consider reducing duplication by factoring common code into a shared method.src/main/java/de/j/stationofdoom/main/Main.java (1)
118-118: Registration of the AnvilListener is correct.
Ensures that anvil-related events are handled properly. No issues found.src/main/java/de/j/stationofdoom/util/Tablist.java (2)
3-3: New import for Config usage is valid.
Brings the configuration handling into this utility.
164-186: Evaluate duplication in getters and setters.
While the new setters and getters properly synchronize state with the config, bothserverNameandhostetByshare duplicate patterns. You might unify them into a single helper to reduce repetition. Also, ensure these config updates cannot trigger concurrency issues if called from async contexts.✅ Verification successful
Consider extracting config sync logic to reduce duplication
The setters are only called from synchronous contexts, so concurrency is not a concern. However, you can still improve the code by extracting the config synchronization logic into a helper method to reduce duplication:
private static void syncConfigValue(String newValue, String currentValue, Consumer<String> configSetter) { if (currentValue == null || !currentValue.equals(newValue)) { configSetter.accept(newValue); } }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking possible concurrency usage of setServerName() or setHostetBy() from async contexts. rg -A 2 "Tablist\.set(ServerName|HostetBy)"Length of output: 1614
src/main/java/de/j/deathMinigames/settings/MainMenu.java (6)
28-31: Enum updates seem appropriate.
AddingSET_HOSTandSET_SERVER_NAMEtoInventoryMenusaligns with the new features for changing server information.
33-35: Introduction of AnvilUIs for server name and host name.
This new enum is a solid approach for categorizing anvil interactions.
78-80: Getter for the “Set Host” AnvilUI.
Ensures you can retrieve the correct UI instance from anywhere.
82-84: Getter for the “Set Server Name” AnvilUI.
This complements the corresponding host setter method nicely.
95-96: Static AnvilUI instances for setting host and server name.
Straightforward approach to store these enumerated UI instances.
128-129: New clickable menu items for “SetHost” and “SetServerName”.
These properly integrate with anvil-based settings. Remember to confirm the accompanyingInventoryListenerandAnvilListenerlogic.✅ Verification successful
Verification successful: Menu items are properly integrated
Both "SetHost" and "SetServerName" items have complete implementations in the InventoryListener and AnvilListener, with proper anvil-based settings integration and data flow.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Listing relevant lines in InventoryListener to ensure the new items are handled rg -A 5 "SetHost|SetServerName"Length of output: 5750
src/main/java/de/j/deathMinigames/main/Config.java (1)
4-5: Looks fine.
These import statements properly integrate the newPlayerJoinandTablistclasses.src/main/java/de/j/deathMinigames/listeners/InventoryListener.java (4)
3-3: Approved import addition.
This import aligns with the newly introduced code referencingDmUtil.
6-6: Looks good.
All newly imported classes (AnvilUI,InventoryMenus, etc.) are relevant to the expanded UI functionalities.
18-19: InventoryType and PrepareAnvilEvent usage.
No issues. Their usage is consistent with upcoming anvil-based logic.
26-27: New imports
The addition ofItemMetaandAnvilViewsuggests more granular control over item naming.src/main/resources/translations.json (2)
84-85: New"anvilOutput"key for German.
The wording looks good. It aligns with the new anvil-based workflow.
170-171: New"anvilOutput"key for English.
The translation string is clear and consistent with the German entry.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (3)
40-49: Add documentation and improve error handling.The UI interaction methods need documentation and better error handling:
- Methods lack JavaDoc
- Returning null from getInventory should be documented
- No error handling for inventory operations
Apply this diff:
+ /** + * Opens the anvil UI for the specified player and adds the initial item. + * @param playerToShowTheInvTo the player to show the UI to + * @throws IllegalStateException if the inventory cannot be opened + */ public void showInventory(Player playerToShowTheInvTo) { if(playerToShowTheInvTo == null) return; - playerToShowTheInvTo.openAnvil(loc, true); - playerToShowTheInvTo.getOpenInventory().getTopInventory().addItem(firstSlot); + try { + playerToShowTheInvTo.openAnvil(loc, true); + playerToShowTheInvTo.getOpenInventory().getTopInventory().addItem(firstSlot); + } catch (Exception e) { + throw new IllegalStateException("Failed to open anvil UI", e); + } } + /** + * Returns null as the inventory is managed through the anvil interface. + * @return null + */ @Override public @NotNull Inventory getInventory() { return null; }
51-53: Improve location comparison method.The location comparison method needs improvements:
- Method name should follow Java conventions (e.g.,
matchesLocation)- Missing documentation explaining Y coordinate omission
- No null check for input parameter
Apply this diff:
- public boolean compareLocIDTo(Location loc) { - return loc.getBlockX() == this.loc.getBlockX() && loc.getBlockZ() == this.loc.getBlockZ(); + /** + * Checks if the given location matches this UI's location. + * Only X and Z coordinates are compared as Y coordinate is not relevant for UI identification. + * @param location the location to compare + * @return true if the locations match, false otherwise + * @throws IllegalArgumentException if location is null + */ + public boolean matchesLocation(Location location) { + if (location == null) { + throw new IllegalArgumentException("Location cannot be null"); + } + return location.getBlockX() == this.loc.getBlockX() && + location.getBlockZ() == this.loc.getBlockZ(); }
19-19: Consider making the class final.Since this class manages a specific UI implementation and isn't designed for inheritance, consider making it final to prevent potential misuse.
Apply this diff:
-public class AnvilUI implements InventoryHolder { +public final class AnvilUI implements InventoryHolder {
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (1)
22-25: Improve field declarations.
- Mark
locasfinalsince it's only set once increateUniqueLocation().- Mark
playerasprivateto maintain encapsulation.- private Location loc = null; + private final Location loc = null; - Player player = null; + private Player player = null;src/main/java/de/j/deathMinigames/listeners/AnvilListener.java (1)
31-59: Improve error handling and logging.
- The logging messages could be more descriptive.
getViewers().getFirst()could throwNoSuchElementExceptionif the list is empty.@EventHandler public void onAnvilPrepare(PrepareAnvilEvent event) { - Player player = (Player) event.getViewers().getFirst(); + if (event.getViewers().isEmpty()) { + Main.getMainLogger().warning("Anvil prepare event has no viewers"); + return; + } + Player player = (Player) event.getViewers().getFirst(); if(player == null) { - Main.getMainLogger().info("Anvil prepare event has no player"); + Main.getMainLogger().warning("First viewer in anvil prepare event is not a player"); return; }src/main/java/de/j/deathMinigames/settings/MainMenu.java (2)
34-37: Improve enum documentation.
- Convert inline comment to proper Javadoc.
- Consider renaming
DEFAULTto something more descriptive likeNO_NAME.public enum AnvilUIs { SET_SERVER_NAME, SET_HOST_NAME, - DEFAULT // usage when the input slot item should have no name + /** + * Used when the input slot item should have no name. + */ + NO_NAME }
112-116: Improve method name and logging.
- Rename method to better describe its purpose (e.g.,
updateAnvilUITranslations).- Use debug level for logging as this is diagnostic information.
- private void tellCurrentPlayerToUIs(Player player) { + private void updateAnvilUITranslations(Player player) { setHost.setPlayerForTranslations(player); setServerName.setPlayerForTranslations(player); - Main.getMainLogger().info("Told UIs current player: " + player.getName()); + Main.getMainLogger().debug("Updated AnvilUI translations for player: " + player.getName()); }src/main/java/de/j/deathMinigames/main/Config.java (1)
338-363: Improve error handling and reduce code duplication.The getter methods have similar logic that could be extracted, and their warning messages could be more descriptive.
+ private String getConfigValue(String key, String description) { + if(Main.getPlugin().getConfig().contains(key)) { + return Main.getPlugin().getConfig().getString(key); + } + Main.getMainLogger().warning(description + " not found in config at " + key); + return null; + } + public String getServerName() { - if(Main.getPlugin().getConfig().contains("Tablist.ServerName")) { - return Main.getPlugin().getConfig().getString("Tablist.ServerName"); - } - else { - Main.getMainLogger().warning("ServerName not found in config!"); - return null; - } + return getConfigValue("Tablist.ServerName", "Server name"); } - public String getHostetBy() { - if(Main.getPlugin().getConfig().contains("Tablist.HostetBy")) { - return Main.getPlugin().getConfig().getString("Tablist.HostetBy"); - } - else { - Main.getMainLogger().warning("HostetBy not found in config!"); - return null; - } + public String getHostedBy() { + return getConfigValue("Tablist.HostedBy", "Host name"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/de/j/deathMinigames/dmUtil/TestCMD.java(0 hunks)src/main/java/de/j/deathMinigames/listeners/AnvilListener.java(1 hunks)src/main/java/de/j/deathMinigames/main/Config.java(3 hunks)src/main/java/de/j/deathMinigames/settings/AnvilUI.java(1 hunks)src/main/java/de/j/deathMinigames/settings/MainMenu.java(6 hunks)src/main/java/de/j/stationofdoom/main/Main.java(1 hunks)src/main/resources/translations.json(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/j/deathMinigames/dmUtil/TestCMD.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/translations.json
- src/main/java/de/j/stationofdoom/main/Main.java
🧰 Additional context used
📓 Learnings (3)
src/main/java/de/j/deathMinigames/listeners/AnvilListener.java (1)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/listeners/AnvilListener.java:27-0
Timestamp: 2025-01-06T19:05:17.436Z
Learning: In the `AnvilListener` class (src/main/java/de/j/deathMinigames/listeners/AnvilListener.java), concurrency concerns for storing the `serverName` or `hostName` instance fields are considered non-problematic because only operators can access the relevant menus. Overwriting these fields does not impact the experience negatively.
src/main/java/de/j/deathMinigames/main/Config.java (1)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#176
File: src/main/java/de/j/deathMinigames/listeners/JoinListener.java:28-32
Timestamp: 2024-11-12T07:49:07.001Z
Learning: In this project, concurrent writes to different places in the configuration are acceptable, and thread safety concerns with methods like `addNewPlayer` and `checkIfPlayerInFile` are not considered an issue due to their usage patterns.
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (1)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/settings/AnvilUI.java:23-38
Timestamp: 2025-01-06T19:14:21.326Z
Learning: The translation utility in this project requires a player instance. Therefore, the `AnvilUI` constructor cannot rely on translations because no player is available at that stage.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (2)
21-25: Consider making fields final where possible.The fields
loc,input, andtitlecould be marked as final since they are only set once in the constructor. This would make the class more thread-safe and communicate intent better.- private Location loc = null; - private ItemStack input = new ItemStack(Material.RED_CONCRETE); - Player player = null; - private MainMenu.AnvilUIs title; + private final Location loc; + private final ItemStack input = new ItemStack(Material.RED_CONCRETE); + private Player player = null; + private final MainMenu.AnvilUIs title;
59-61: Add Y-coordinate comparison for complete location matching.The
compareLocIDTomethod only compares X and Z coordinates. Consider including Y coordinate for complete location matching.public boolean compareLocIDTo(Location loc) { - return loc.getBlockX() == this.loc.getBlockX() && loc.getBlockZ() == this.loc.getBlockZ(); + return loc.getBlockX() == this.loc.getBlockX() && + loc.getBlockY() == this.loc.getBlockY() && + loc.getBlockZ() == this.loc.getBlockZ(); }src/main/java/de/j/deathMinigames/listeners/AnvilListener.java (1)
26-30: Consider using final fields and builder pattern.The
TranslationFactoryinstance could be final, and the naming fields could be encapsulated better.- private String serverName; - private String hostName; - private TranslationFactory tf = new TranslationFactory(); + private final TranslationFactory tf = new TranslationFactory(); + private volatile String serverName; + private volatile String hostName;src/main/java/de/j/deathMinigames/settings/MainMenu.java (1)
130-131: Consider adding slot index validation.While the implementation follows the existing pattern, consider adding validation to ensure slot indices 4 and 5 don't exceed the inventory size (currently set to 9 in the constructor).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/de/j/deathMinigames/listeners/AnvilListener.java(1 hunks)src/main/java/de/j/deathMinigames/main/Config.java(3 hunks)src/main/java/de/j/deathMinigames/settings/AnvilUI.java(1 hunks)src/main/java/de/j/deathMinigames/settings/MainMenu.java(5 hunks)src/main/java/de/j/stationofdoom/util/Tablist.java(3 hunks)
🧰 Additional context used
📓 Learnings (3)
src/main/java/de/j/deathMinigames/listeners/AnvilListener.java (1)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/listeners/AnvilListener.java:27-0
Timestamp: 2025-01-06T19:05:17.436Z
Learning: In the `AnvilListener` class (src/main/java/de/j/deathMinigames/listeners/AnvilListener.java), concurrency concerns for storing the `serverName` or `hostName` instance fields are considered non-problematic because only operators can access the relevant menus. Overwriting these fields does not impact the experience negatively.
src/main/java/de/j/deathMinigames/main/Config.java (1)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/main/Config.java:327-353
Timestamp: 2025-01-08T10:50:07.451Z
Learning: In the Config class, the `setServerName` and `setHostetBy` methods have intentionally different implementations to handle their specific cases separately, despite similar looking functionality. The `setServerName` method explicitly handles the cases of non-existent key and different value separately, while `setHostetBy` combines these conditions.
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (1)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/settings/AnvilUI.java:23-38
Timestamp: 2025-01-06T19:14:21.326Z
Learning: The translation utility in this project requires a player instance. Therefore, the `AnvilUI` constructor cannot rely on translations because no player is available at that stage.
🔇 Additional comments (6)
src/main/java/de/j/deathMinigames/settings/MainMenu.java (3)
29-32: LGTM! Well-structured enum additions.The new enum constants and AnvilUIs enum are well-organized and follow consistent naming conventions.
Also applies to: 34-38
80-86: LGTM! Consistent getter implementations.The new getters maintain consistency with existing patterns in the class, including proper synchronization.
97-98: LGTM! Proper field declarations.The static final fields are correctly initialized with their respective AnvilUI enum constants.
src/main/java/de/j/deathMinigames/main/Config.java (3)
324-343: LGTM! Implementation aligns with requirements.The separate handling of non-existent key and different value cases in setServerName is intentional, as learned from previous discussions.
345-360:⚠️ Potential issueFix typo in host-related method names and keys.
The "HostetBy" typo appears consistently in method names and configuration keys. This should be corrected to "HostedBy" throughout the codebase.
- public synchronized void setHostedBy(String serverName) { - if(!Main.getPlugin().getConfig().contains("Tablist.HostetBy") || !Main.getPlugin().getConfig().getString("Tablist.HostetBy").equals(serverName)) { - Main.getPlugin().getConfig().set("Tablist.HostetBy", serverName); + public synchronized void setHostedBy(String hostName) { + if(!Main.getPlugin().getConfig().contains("Tablist.HostedBy") || !Main.getPlugin().getConfig().getString("Tablist.HostedBy").equals(hostName)) { + Main.getPlugin().getConfig().set("Tablist.HostedBy", hostName); Main.getPlugin().saveConfig(); } } public String getHostedBy() { - if(Main.getPlugin().getConfig().contains("Tablist.HostetBy")) { - return Main.getPlugin().getConfig().getString("Tablist.HostetBy"); + if(Main.getPlugin().getConfig().contains("Tablist.HostedBy")) { + return Main.getPlugin().getConfig().getString("Tablist.HostedBy"); } else { - Main.getMainLogger().warning("HostetBy not found in config!"); + Main.getMainLogger().warning("HostedBy not found in config!"); return null; } }Likely invalid or redundant comment.
117-125:⚠️ Potential issueFix typo in configuration key name.
The key "HostetBy" appears to be a typo of "HostedBy". This should be consistent with the method names in the class.
- if(config.contains("Tablist.HostetBy")) { - Tablist.setHostetBy(config.getString("Tablist.HostetBy")); + if(config.contains("Tablist.HostedBy")) { + Tablist.setHostedBy(config.getString("Tablist.HostedBy"));Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (3)
19-23: Improve field declarations.Consider the following improvements:
- Make fields final where possible
- Make
playerfield private- Initialize
locin declaration sincecreateUniqueLocation()is called in constructorpublic class AnvilUI implements InventoryHolder { - private Location loc = null; - private ItemStack input = new ItemStack(Material.RED_CONCRETE); - Player player = null; - private MainMenu.AnvilUIs title; + private Location loc; + private final ItemStack input = new ItemStack(Material.RED_CONCRETE); + private Player player; + private final MainMenu.AnvilUIs title;
39-42: Document why getInventory returns null.The implementation of
getInventory()returns null, which is unexpected for an InventoryHolder. Add documentation explaining this design decision.@Override +/** + * Returns null as the inventory is managed through the anvil interface + * rather than a traditional inventory. + */ public @NotNull Inventory getInventory() { return null; }
57-59: Document Y-coordinate exclusion in location comparison.The method only compares X and Z coordinates. Add documentation to explain this design decision.
+/** + * Compares the X and Z coordinates of the given location with this UI's location. + * Y coordinate is intentionally ignored for the comparison. + * + * @param loc The location to compare with + * @return true if the X and Z coordinates match + */ public boolean compareLocIDTo(Location loc) { return loc.getBlockX() == this.loc.getBlockX() && loc.getBlockZ() == this.loc.getBlockZ(); }src/main/java/de/j/stationofdoom/listener/PlayerJoin.java (1)
73-92: Reduce code duplication in tablist update logic.The code blocks for both conditions contain significant duplication, particularly in the TPS and ping display logic.
+private Component createCommonFooter(Player player, int ping, int phase) { + return Component.text(String.format("\nTPS: %s; %s; %s", + (int) Main.getPlugin().getServer().getTPS()[0], + (int) Main.getPlugin().getServer().getTPS()[1], + (int) Main.getPlugin().getServer().getTPS()[2]), NamedTextColor.LIGHT_PURPLE) + .append(Component.text("\n Ping: ") + .append(Component.text(String.valueOf(ping)) + .color(ping > 30 ? NamedTextColor.RED : NamedTextColor.GREEN))) + .append(Component.text("\n") + .append(tablist.getTimeComponent(player))); +} if(Tablist.getHostedBy() == null) { tablist.tabTPS(player, mm.deserialize(" <dark_blue><1></dark_blue> <newline><newline>", Placeholder.component("1", Component.text(Tablist.getServerName()))), - mm.deserialize("<newline> <red>Plugin by </red><rainbow:!" + (phase.get() + 2) + ">LuckyProgrammer</rainbow>") - .append(Component.text(String.format("\nTPS: %s; %s; %s", - (int) Main.getPlugin().getServer().getTPS()[0], - (int) Main.getPlugin().getServer().getTPS()[1], - (int) Main.getPlugin().getServer().getTPS()[2]), NamedTextColor.LIGHT_PURPLE)) - .append(Component.text("\n Ping: ") - .append(Component.text(String.valueOf(ping)) - .color(ping > 30 ? NamedTextColor.RED : NamedTextColor.GREEN))) - .append(Component.text("\n") - .append(tablist.getTimeComponent(player)))); + mm.deserialize("<newline> <red>Plugin by </red><rainbow:!" + (phase.get() + 2) + ">LuckyProgrammer</rainbow>") + .append(createCommonFooter(player, ping, phase.get()))); } else { tablist.tabTPS(player, mm.deserialize(" <dark_blue><1></dark_blue> <newline><newline>", Placeholder.component("1", Component.text(Tablist.getServerName()))), - mm.deserialize("<newline><newline> <red>Hosted by </red><rainbow:" + phase + "><2></rainbow> <newline> <red>Plugin by </red><rainbow:!" + (phase.get() + 2) + ">LuckyProgrammer</rainbow>", - Placeholder.component("2", Component.text(Tablist.getHostedBy()))) - .append(Component.text(String.format("\nTPS: %s; %s; %s", - (int) Main.getPlugin().getServer().getTPS()[0], - (int) Main.getPlugin().getServer().getTPS()[1], - (int) Main.getPlugin().getServer().getTPS()[2]), NamedTextColor.LIGHT_PURPLE)) - .append(Component.text("\n Ping: ") - .append(Component.text(String.valueOf(ping)) - .color(ping > 30 ? NamedTextColor.RED : NamedTextColor.GREEN))) - .append(Component.text("\n") - .append(tablist.getTimeComponent(player)))); + mm.deserialize("<newline><newline> <red>Hosted by </red><rainbow:" + phase + "><2></rainbow> <newline> <red>Plugin by </red><rainbow:!" + (phase.get() + 2) + ">LuckyProgrammer</rainbow>", + Placeholder.component("2", Component.text(Tablist.getHostedBy()))) + .append(createCommonFooter(player, ping, phase.get()))); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/de/j/deathMinigames/listeners/AnvilListener.java(1 hunks)src/main/java/de/j/deathMinigames/main/Config.java(3 hunks)src/main/java/de/j/deathMinigames/settings/AnvilUI.java(1 hunks)src/main/java/de/j/stationofdoom/listener/PlayerJoin.java(2 hunks)src/main/java/de/j/stationofdoom/util/Tablist.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/j/deathMinigames/listeners/AnvilListener.java
🧰 Additional context used
📓 Learnings (2)
src/main/java/de/j/deathMinigames/main/Config.java (1)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/main/Config.java:327-353
Timestamp: 2025-01-08T10:50:07.451Z
Learning: In the Config class, the `setServerName` and `setHostetBy` methods have intentionally different implementations to handle their specific cases separately, despite similar looking functionality. The `setServerName` method explicitly handles the cases of non-existent key and different value separately, while `setHostetBy` combines these conditions.
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (4)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/settings/AnvilUI.java:23-38
Timestamp: 2025-01-06T19:14:21.326Z
Learning: The translation utility in this project requires a player instance. Therefore, the `AnvilUI` constructor cannot rely on translations because no player is available at that stage.
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/settings/AnvilUI.java:63-83
Timestamp: 2025-01-08T14:40:26.768Z
Learning: In Bukkit/Spigot, when creating a new ItemStack with `new ItemStack(Material)`, the ItemMeta obtained via `getItemMeta()` will never be null as it always has a default ItemMeta.
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/settings/AnvilUI.java:63-83
Timestamp: 2025-01-08T14:40:26.767Z
Learning: In Bukkit/Spigot plugins, a newly created ItemStack with `new ItemStack(Material)` always has a default ItemMeta, making null checks on `getItemMeta()` unnecessary for fresh items. The only exception is when dealing with specific meta types like SkullMeta where additional validation might be needed.
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#176
File: src/main/java/de/j/deathMinigames/settings/MainMenu.java:95-106
Timestamp: 2024-11-14T16:53:06.950Z
Learning: In the Bukkit API for Minecraft plugin development, when an `ItemStack` is created and `getItemMeta()` is called on it, the returned `ItemMeta` is guaranteed to be non-null. Therefore, null checks for `ItemMeta` are unnecessary in this context.
🔇 Additional comments (4)
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (2)
25-29: LGTM! Constructor follows good practices.The constructor properly validates input and initializes required fields.
31-37:⚠️ Potential issueFix potential race condition in showInventory.
The method assigns to the class field
playerand then uses it insetInputMeta(), which could cause issues in a concurrent environment.public void showInventory(Player playerToShowTheInvTo) { if(playerToShowTheInvTo == null) return; + setInputMeta(playerToShowTheInvTo); this.player = playerToShowTheInvTo; playerToShowTheInvTo.openAnvil(loc, true); - setInputMeta(); playerToShowTheInvTo.getOpenInventory().getTopInventory().setItem(0, input); }Likely invalid or redundant comment.
src/main/java/de/j/stationofdoom/util/Tablist.java (2)
20-21: LGTM! Fields are properly marked volatile.The static fields are correctly marked as volatile for thread safety.
177-186: 🛠️ Refactor suggestionImprove error handling in setHostedBy.
Instead of logging a warning and returning, throw an exception for null input to maintain consistency with other methods.
public static void setHostedBy(String hostedBy) { if(hostedBy == null) { - Main.getMainLogger().warning("Hostet by is null"); - return; + throw new IllegalArgumentException("Host name cannot be null"); } Tablist.hostedBy = hostedBy; if(Config.getInstance().getHostedBy() == null || !Config.getInstance().getHostedBy().equals(hostedBy)) { Config.getInstance().setHostedBy(hostedBy); } }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (3)
20-23: Improve field encapsulation and initialization.
- The
playerfield should be private to maintain encapsulation.- Consider initializing the
locfield in the constructor rather than setting it to null.- private Location loc = null; + private final Location loc; private ItemStack input = new ItemStack(Material.RED_CONCRETE); - Player player = null; + private Player player = null; private MainMenu.AnvilUIs title;
39-42: Document null return behavior.Add Javadoc explaining why getInventory returns null, as this might be unexpected for implementers.
@Override + /** + * Returns null as the inventory is managed through the anvil interface. + * @return null + */ public @NotNull Inventory getInventory() { return null; }
57-59: Enhance location comparison robustness.The method should validate the input parameter and consider Y coordinate if relevant.
public boolean compareLocIDTo(Location loc) { + if(loc == null) return false; return loc.getBlockX() == this.loc.getBlockX() && loc.getBlockZ() == this.loc.getBlockZ(); }src/main/java/de/j/stationofdoom/util/Tablist.java (1)
165-167: Add null check in getServerName.Consider providing a default value when serverName is null to prevent NPEs.
public static String getServerName() { + if (serverName == null) { + return Bukkit.getServer().getName(); + } return serverName; }src/main/java/de/j/deathMinigames/main/Config.java (1)
345-350: Fix misleading parameter name.The parameter name 'serverName' should be 'hostedBy' to match its purpose.
-public synchronized void setHostedBy(String serverName) { - if(!Main.getPlugin().getConfig().contains("Tablist.HostedBy") || !Main.getPlugin().getConfig().getString("Tablist.HostedBy").equals(serverName)) { - Main.getPlugin().getConfig().set("Tablist.HostedBy", serverName); +public synchronized void setHostedBy(String hostedBy) { + if(!Main.getPlugin().getConfig().contains("Tablist.HostedBy") || !Main.getPlugin().getConfig().getString("Tablist.HostedBy").equals(hostedBy)) { + Main.getPlugin().getConfig().set("Tablist.HostedBy", hostedBy); Main.getPlugin().saveConfig(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/j/deathMinigames/listeners/AnvilListener.java(1 hunks)src/main/java/de/j/deathMinigames/main/Config.java(3 hunks)src/main/java/de/j/deathMinigames/settings/AnvilUI.java(1 hunks)src/main/java/de/j/stationofdoom/util/Tablist.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/j/deathMinigames/listeners/AnvilListener.java
🧰 Additional context used
📓 Learnings (2)
src/main/java/de/j/deathMinigames/main/Config.java (1)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/main/Config.java:327-353
Timestamp: 2025-01-08T10:50:07.451Z
Learning: In the Config class, the `setServerName` and `setHostetBy` methods have intentionally different implementations to handle their specific cases separately, despite similar looking functionality. The `setServerName` method explicitly handles the cases of non-existent key and different value separately, while `setHostetBy` combines these conditions.
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (4)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/settings/AnvilUI.java:23-38
Timestamp: 2025-01-06T19:14:21.326Z
Learning: The translation utility in this project requires a player instance. Therefore, the `AnvilUI` constructor cannot rely on translations because no player is available at that stage.
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/settings/AnvilUI.java:63-83
Timestamp: 2025-01-08T14:40:26.768Z
Learning: In Bukkit/Spigot, when creating a new ItemStack with `new ItemStack(Material)`, the ItemMeta obtained via `getItemMeta()` will never be null as it always has a default ItemMeta.
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/settings/AnvilUI.java:63-83
Timestamp: 2025-01-08T14:40:26.767Z
Learning: In Bukkit/Spigot plugins, a newly created ItemStack with `new ItemStack(Material)` always has a default ItemMeta, making null checks on `getItemMeta()` unnecessary for fresh items. The only exception is when dealing with specific meta types like SkullMeta where additional validation might be needed.
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#176
File: src/main/java/de/j/deathMinigames/settings/MainMenu.java:95-106
Timestamp: 2024-11-14T16:53:06.950Z
Learning: In the Bukkit API for Minecraft plugin development, when an `ItemStack` is created and `getItemMeta()` is called on it, the returned `ItemMeta` is guaranteed to be non-null. Therefore, null checks for `ItemMeta` are unnecessary in this context.
🔇 Additional comments (10)
src/main/java/de/j/deathMinigames/settings/AnvilUI.java (3)
25-29: LGTM! Constructor implementation is robust.The constructor properly validates input and initializes fields.
61-81: LGTM! Input meta handling is well-implemented.The method correctly handles translations and follows the project's requirements regarding player context for translations.
31-37: 🛠️ Refactor suggestionImprove thread safety in showInventory method.
The method assigns to the class field
playerand then uses the parameter. This could cause issues in a concurrent environment.public void showInventory(Player playerToShowTheInvTo) { if(playerToShowTheInvTo == null) return; + synchronized(this) { this.player = playerToShowTheInvTo; + } playerToShowTheInvTo.openAnvil(loc, true); setInputMeta(playerToShowTheInvTo); playerToShowTheInvTo.getOpenInventory().getTopInventory().setItem(0, input); }Likely invalid or redundant comment.
src/main/java/de/j/stationofdoom/util/Tablist.java (4)
20-21: LGTM! Thread-safe field declarations.The static fields are properly marked as volatile to ensure thread safety.
169-175: Add null validation in setServerName.The method should validate the serverName parameter before setting it.
177-186: LGTM! Proper null handling and synchronization.The method correctly handles null values and synchronizes with the Config class.
188-190: LGTM! Consistent with setHostedBy.The getter aligns with setHostedBy's null handling approach.
src/main/java/de/j/deathMinigames/main/Config.java (3)
117-125: LGTM! Thorough null checking.The code properly validates both the configuration keys and their values before loading.
335-343: LGTM! Good error handling.The method properly handles missing configuration and logs warnings.
352-360: LGTM! Consistent implementation.The method follows the same pattern as getServerName with proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/de/j/deathMinigames/main/Config.java (2)
117-117: Align comment indentation with code.The comment should be aligned with the code block it describes.
- //Tablist + //Tablist
349-354: Add null check for consistency in error handling.While the different implementation structure is intentional, adding a null check would improve consistency in error handling across the class.
public synchronized void setHostedBy(String serverName) { + if(serverName == null) { + Main.getMainLogger().warning("HostedBy is null!"); + return; + } if(!Main.getPlugin().getConfig().contains("Tablist.HostedBy") || !Main.getPlugin().getConfig().getString("Tablist.HostedBy").equals(serverName)) { Main.getPlugin().getConfig().set("Tablist.HostedBy", serverName); Main.getPlugin().saveConfig(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/j/deathMinigames/main/Config.java(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/main/java/de/j/deathMinigames/main/Config.java (1)
Learnt from: PrinzPizza42
PR: AtomPilz-DevTeam/StationofDoomPlugin#196
File: src/main/java/de/j/deathMinigames/main/Config.java:327-353
Timestamp: 2025-01-08T10:50:07.451Z
Learning: In the Config class, the `setServerName` and `setHostetBy` methods have intentionally different implementations to handle their specific cases separately, despite similar looking functionality. The `setServerName` method explicitly handles the cases of non-existent key and different value separately, while `setHostetBy` combines these conditions.
🔇 Additional comments (3)
src/main/java/de/j/deathMinigames/main/Config.java (3)
3-3: LGTM!The import for Tablist utility is correctly placed and necessary for the new functionality.
117-125: LGTM! Robust configuration handling.The implementation correctly handles the tablist configuration with proper existence and null checks.
324-347: LGTM! Robust implementation with proper null handling.The implementation is thread-safe and includes proper null checks and error logging. The separate handling of non-existent key and value mismatch cases is intentional and well-structured.
implementing server name and server host name changing via AnvilUI
Describe your changes
This PR introduces a system to get String and int input from a player through a custom AnvilUI. Using this, two menus got added to the settings menu to change the name of the server and the host seen when pressing tab in game.
closes #150
...
Notes for testing your change
...
Summary by CodeRabbit
New Features
Bug Fixes
Chores