GH-1031 2.0 vanish implementation #1048
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update adds a vanish feature for players with new interfaces and service implementations to manage vanish states and metadata. It introduces commands to toggle vanish mode and permission constants for access control. Several event listeners handle player actions like chatting, item use, block breaking/placing, and interactions with containers when vanished. Effects such as god mode, glowing, and night vision are applied or removed on vanish state changes. The system also hides vanished players from tab completions, command suggestions, and player views on join and quit events, replacing previous vanish logic with a modular, event-driven design. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
📦 Development Build ReadyWarning Do not use this build in production. It is for testing purposes only and may be unstable. |
P1otrulla
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (14)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ChatController.java (2)
12-12: Address the TODO commentThe TODO comment suggests incomplete implementation. Please clarify what if-check needs to be added or remove the comment if no longer needed.
23-23: Consider explicit access modifierThe event handler method lacks an explicit access modifier. Consider adding
publicorprivateto make the intent clear.- @EventHandler - void onChat(AsyncPlayerChatEvent event) { + @EventHandler + public void onChat(AsyncPlayerChatEvent event) {eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/PlayerQuitController.java (1)
22-22: Consider explicit access modifierThe event handler method lacks an explicit access modifier. Consider adding
publicorprivatefor clarity.- @EventHandler - void onQuit(PlayerQuitEvent event) { + @EventHandler + public void onQuit(PlayerQuitEvent event) {eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/VanishInvisibleService.java (1)
8-17: Add JavaDoc documentation to clarify method contracts.The interface methods lack documentation. Consider adding JavaDoc to specify behavior, especially for null parameter handling.
+/** + * Service for managing player visibility in vanish mode. + */ public interface VanishInvisibleService { + /** + * Hides the specified player from other players. + * @param player the player to hide + */ void hidePlayer(Player player); + /** + * Shows the specified player to other players. + * @param player the player to show + */ void showPlayer(Player player); + /** + * Hides admin players from the specified player. + * @param player the player who should not see admins + */ void hideAdminForPlayer(Player player); + /** + * Gets the set of currently vanished players. + * @return set of vanished player UUIDs + */ Set<UUID> getVanishedPlayers(); }eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java (1)
7-19: Add JavaDoc documentation for better API clarity.The interface methods need documentation to clarify their contracts and behavior.
+/** + * Service for managing player vanish state. + */ public interface VanishService { + /** + * Enables vanish mode for the specified player. + * @param player the player to enable vanish for + */ void enableVanish(Player player); + /** + * Disables vanish mode for the specified player. + * @param player the player to disable vanish for + */ void disableVanish(Player player); + /** + * Checks if the specified player is vanished. + * @param player the player to check + * @return true if the player is vanished + */ boolean isVanished(Player player); + /** + * Checks if the player with the specified UUID is vanished. + * @param uniqueId the UUID of the player to check + * @return true if the player is vanished + */ boolean isVanished(UUID uniqueId); + /** + * Hides admin players from the specified player. + * @param player the player who should not see admins + */ void hideAdminForPlayer(Player player); }eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/FoodController.java (1)
12-12: Address the TODO comment.The TODO comment about "if-check" needs clarification. What specific check is missing?
Do you want me to help determine what check might be needed here, or should this TODO be resolved?
eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/VanishMetaDataService.java (1)
7-16: Add JavaDoc documentation for API clarity.The interface methods need documentation to specify their behavior and contracts.
+/** + * Service for managing vanish metadata on players. + */ public interface VanishMetaDataService { + /** + * Adds vanish metadata to the specified player. + * @param player the player to add metadata to + */ void addMetadata(Player player); + /** + * Removes vanish metadata from the specified player. + * @param player the player to remove metadata from + */ void removeMetadata(Player player); + /** + * Checks if the player with the specified UUID has vanish metadata. + * @param playerUniqueId the UUID of the player to check + * @return true if the player has vanish metadata + */ boolean hasMetadata(UUID playerUniqueId); + /** + * Checks if the specified player has vanish metadata. + * @param player the player to check + * @return true if the player has vanish metadata + */ boolean hasMetadata(Player player); }eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GodModeController.java (1)
11-11: TODO comment needs attentionThe TODO comment suggests adding an if-check, likely for configuration-based conditional logic. Consider implementing this or removing the comment if it's no longer needed.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/NightVisionController.java (1)
13-13: TODO comment needs attentionSimilar to other controllers, this TODO suggests adding conditional logic, possibly based on configuration settings.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ItemController.java (1)
12-12: Address the TODO comment.The TODO comment about "if-check" needs clarification. What specific if-check needs to be implemented?
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/TabComplateController.java (1)
47-55: Consider optimizing the command filtering logic.The nested loop checking all online players for each command could be slow with many players. Consider caching vanished player names.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GlowingController.java (1)
15-15: Consider adding configuration support.The TODO comment suggests adding configuration support for conditional checks and color customization. This would improve flexibility.
Would you like me to help implement the configuration support mentioned in the TODO comment?
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/OpenSilentController.java (2)
61-61: Remove extra blank line.There's an unnecessary blank line that should be removed for cleaner code.
-
22-22: Consider adding configuration support.The TODO comment suggests adding configuration support. This would allow enabling/disabling this feature per server needs.
Would you like me to help implement the configuration support mentioned in the TODO comment?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/VanishInvisibleService.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/VanishMetaDataService.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/AbstractVanishEvent.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/DisableVanishEvent.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/EnableVanishEvent.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/PlayerArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishConfiguration.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishInvisibleServiceImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishMetaDataServiceImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishPermissionConstant.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java(0 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishServiceImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/BlockController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ChatController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/FoodController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GlowingController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GodModeController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ItemController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/NightVisionController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/OpenSilentController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/PlayerJoinController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/PlayerQuitController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/TabComplateController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholdersSetup.java(2 hunks)
💤 Files with no reviewable changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishService.java
🧰 Additional context used
🧠 Learnings (2)
eternalcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholdersSetup.java (1)
Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalCore#865
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveService.java:0-0
Timestamp: 2024-10-14T20:11:13.610Z
Learning: In the 'EternalCore' project, avoid using null in method signatures in Java code.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishConfiguration.java (2)
Learnt from: CitralFlo
PR: EternalCodeTeam/EternalCore#896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:92-102
Timestamp: 2025-01-28T21:37:36.945Z
Learning: In the EternalCore plugin's private chat feature, PrivateChatToggleState.ON means message blocking is enabled (no messages will be received), while PrivateChatToggleState.OFF means messages are allowed.
Learnt from: CitralFlo
PR: EternalCodeTeam/EternalCore#896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:1-1
Timestamp: 2025-01-30T19:49:57.192Z
Learning: In the EternalCore project's private chat toggle feature, PrivateChatToggleState.ON means messages are blocked, while OFF means messages are allowed.
🧬 Code Graph Analysis (10)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GodModeController.java (4)
eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/DisableVanishEvent.java (1)
DisableVanishEvent(5-10)eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/EnableVanishEvent.java (1)
EnableVanishEvent(5-10)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GlowingController.java (1)
Controller(16-53)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/NightVisionController.java (1)
Controller(12-33)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishCommand.java (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishPermissionConstant.java (1)
VanishPermissionConstant(5-24)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/PlayerQuitController.java (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/PlayerJoinController.java (1)
Controller(13-50)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GlowingController.java (6)
eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/DisableVanishEvent.java (1)
DisableVanishEvent(5-10)eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/EnableVanishEvent.java (1)
EnableVanishEvent(5-10)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ChatController.java (1)
Controller(11-32)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/FoodController.java (1)
Controller(11-32)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GodModeController.java (1)
Controller(10-27)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/NightVisionController.java (1)
Controller(12-33)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/FoodController.java (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/BlockController.java (1)
Controller(12-43)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ChatController.java (1)
Controller(11-32)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ItemController.java (1)
Controller(13-46)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishServiceImpl.java (4)
eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/DisableVanishEvent.java (1)
DisableVanishEvent(5-10)eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/EnableVanishEvent.java (1)
EnableVanishEvent(5-10)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishInvisibleServiceImpl.java (1)
Service(14-68)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishMetaDataServiceImpl.java (1)
Service(13-55)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/BlockController.java (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ChatController.java (1)
Controller(11-32)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/FoodController.java (1)
Controller(11-32)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ItemController.java (1)
Controller(13-46)
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/PlayerArgument.java (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishPermissionConstant.java (1)
VanishPermissionConstant(5-24)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/OpenSilentController.java (8)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/PlayerJoinController.java (1)
Controller(13-50)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/BlockController.java (1)
Controller(12-43)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ChatController.java (1)
Controller(11-32)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/FoodController.java (1)
Controller(11-32)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GodModeController.java (1)
Controller(10-27)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ItemController.java (1)
Controller(13-46)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/NightVisionController.java (1)
Controller(12-33)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/TabComplateController.java (1)
Controller(16-57)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishMetaDataServiceImpl.java (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishInvisibleServiceImpl.java (1)
Service(14-68)eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishServiceImpl.java (1)
Service(12-56)
🔇 Additional comments (42)
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/PlayerArgument.java (1)
72-72: Clean permission constant refactoringThe update from
VANISH_SEE_TABULATION_PERMISSIONtoVanishPermissionConstant.VANISH_SEE_PERMISSIONimproves naming consistency and aligns with the vanish feature overhaul.eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/DisableVanishEvent.java (1)
5-10: Simple and clean event implementationThe event class correctly extends
AbstractVanishEventand follows standard event patterns. The implementation is appropriate for representing vanish disable events.eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/EnableVanishEvent.java (1)
5-10: Consistent event designThe event class mirrors the
DisableVanishEventstructure and correctly extendsAbstractVanishEvent. Good symmetry in the event design.eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ChatController.java (1)
24-31: Clean vanish chat blocking logicThe implementation correctly prevents vanished players from chatting by checking vanish status and canceling the event. The early return pattern keeps the code clean.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/PlayerQuitController.java (1)
25-29: Proper vanish cleanup on quitThe implementation correctly suppresses quit messages for vanished players and cleans up the vanish state. This prevents revealing vanished players through quit messages.
eternalcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholdersSetup.java (2)
21-21: Parameter type updated correctly for new architecture.The change from
VanishServicetoVanishMetaDataServicealigns with the new vanish feature architecture.
25-25: Method call updated consistently with new service.The change from
vanishService.isVanished(onlinePlayer)tovanishMetaDataService.hasMetadata(onlinePlayer)is correct for the new metadata-based vanish system.eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/FoodController.java (1)
22-31: Implementation follows established pattern correctly.The food level change prevention logic is consistent with other vanish controllers and properly handles the event cancellation.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/BlockController.java (3)
12-20: Clean implementation following established patternsThe controller properly uses dependency injection and follows the same structure as other vanish controllers. Good use of the @controller annotation and constructor injection.
22-31: Block breaking prevention logic looks correctThe event handler properly checks vanish status before cancelling the event. The early return pattern keeps the code clean and readable.
33-42: Block placement prevention follows the same patternConsistent implementation with the break handler. Both methods follow the same logical flow which is good for maintainability.
eternalcore-api/src/main/java/com/eternalcode/core/feature/vanish/event/AbstractVanishEvent.java (1)
7-28: Proper Bukkit event implementationThe abstract event class correctly implements the HandlerList pattern and provides a clean foundation for vanish events. The static HandlerList and required methods follow Bukkit conventions perfectly.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GodModeController.java (1)
14-26: God mode toggle implementation is correctThe event handlers properly toggle player invulnerability based on vanish state. The logic is straightforward and matches the pattern used in other controllers.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishConfiguration.java (1)
5-20: Configuration structure looks functionalThe boolean flags provide good coverage of vanish behaviors. The default values seem reasonable for typical vanish functionality.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/NightVisionController.java (2)
16-18: Good potion effect configurationThe night vision effect is properly configured with maximum duration and no visual indicators, which is perfect for a vanish feature.
20-32: Night vision toggle logic is correctThe event handlers properly apply and remove the night vision effect based on vanish state. The implementation follows the established pattern from other controllers.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/ItemController.java (2)
24-34: Item pickup logic looks good.The event handling correctly prevents vanished players from picking up items.
36-45: Item drop logic looks good.The event handling correctly prevents vanished players from dropping items.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishCommand.java (2)
22-32: Command logic looks good.The vanish toggle logic for self is implemented correctly.
34-45: Command logic looks good.The vanish toggle logic for other players is implemented correctly with proper permission checks.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/GlowingController.java (4)
19-25: Well-structured initialization.The constructor properly injects dependencies and initializes the scoreboard reference. The static team name constant is a good practice.
27-33: Good event handling pattern.The enable vanish event handler correctly adds the player to the team and sets glowing state. This follows the same pattern as other vanish controllers.
35-41: Good event handling pattern.The disable vanish event handler correctly removes the player from the team and disables glowing. This properly reverses the enable operation.
43-52: Smart lazy team creation.The borrowTeam method efficiently creates the team only when needed and reuses existing teams. The AQUA color provides good visibility.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishMetaDataServiceImpl.java (5)
16-24: Clean service initialization.The constructor properly initializes dependencies and the metadata key constant is well-defined.
26-29: Proper metadata management.The addMetadata method correctly uses FixedMetadataValue to set the vanish state. This integrates well with Bukkit's metadata system.
31-34: Proper metadata cleanup.The removeMetadata method correctly removes the plugin-specific metadata key.
36-43: Safe offline player handling.The UUID-based metadata check safely handles offline players by returning false. This prevents null pointer exceptions.
45-53: Robust metadata checking.The metadata validation properly iterates through all metadata values and checks for boolean true. This handles edge cases where multiple metadata values might exist.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishServiceImpl.java (5)
15-24: Excellent dependency injection setup.The constructor properly injects all required services and follows dependency injection best practices.
26-32: Well-orchestrated vanish enabling.The enableVanish method properly coordinates invisibility, metadata, and event triggering in the correct order.
34-40: Well-orchestrated vanish disabling.The disableVanish method properly reverses the vanish state by coordinating visibility, metadata, and events.
42-50: Clean delegation pattern.Both isVanished methods properly delegate to the metadata service, maintaining consistent behavior across Player and UUID-based checks.
52-55: Proper delegation to specialized service.The hideAdminForPlayer method correctly delegates to the invisible service, maintaining separation of concerns.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/controller/OpenSilentController.java (5)
25-30: Proper dependency injection.The constructor correctly injects the VanishService dependency following the established pattern.
32-46: Good event filtering.The interaction event handler properly filters for right-click block actions and vanished players only.
48-59: Smart container handling.The code properly identifies container types and handles ender chests specially by using the player's own ender chest inventory.
66-72: Excellent silent viewing implementation.The event cancellation and inventory cloning create a perfect silent viewing experience. The custom title clearly indicates vanish mode.
74-92: Comprehensive container support.The helper methods properly handle all container types including shulker boxes using Tags. The pattern matching is clean and efficient.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishInvisibleServiceImpl.java (3)
17-26: Clean service initialization.The constructor properly initializes dependencies and the vanished players set. Good use of dependency injection.
51-62: Solid admin hiding logic.The hideAdminForPlayer method correctly handles offline players and self-exclusion. The logic is sound and safe.
64-67: Proper encapsulation.The getVanishedPlayers method correctly returns an unmodifiable view of the set, preventing external modification.
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishConfiguration.java
Outdated
Show resolved
Hide resolved
...nalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishPermissionConstant.java
Show resolved
Hide resolved
...core/src/main/java/com/eternalcode/core/feature/vanish/controller/TabComplateController.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/com/eternalcode/core/feature/vanish/controller/TabComplateController.java
Outdated
Show resolved
Hide resolved
...-core/src/main/java/com/eternalcode/core/feature/vanish/controller/PlayerJoinController.java
Outdated
Show resolved
Hide resolved
...-core/src/main/java/com/eternalcode/core/feature/vanish/controller/PlayerJoinController.java
Outdated
Show resolved
Hide resolved
...-core/src/main/java/com/eternalcode/core/feature/vanish/controller/PlayerJoinController.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishCommand.java
Outdated
Show resolved
Hide resolved
...lcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishInvisibleServiceImpl.java
Outdated
Show resolved
Hide resolved
...lcore-core/src/main/java/com/eternalcode/core/feature/vanish/VanishInvisibleServiceImpl.java
Show resolved
Hide resolved
…nish/controller/TabComplateController.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nish/controller/PlayerJoinController.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nish/VanishInvisibleServiceImpl.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
P1otrulla
requested review from
Rollczi,
mydelkonivea,
vLuckyyy,
igoyek,
Jakubk15,
VerniQ and
imDMK
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive vanish system for the EternalCore plugin, allowing players to become invisible to other players with various configuration options and permissions.
- Replaces the simple vanish service with a full-featured implementation including metadata management, invisibility handling, and event-driven architecture
- Adds extensive configuration options for vanish behavior (god mode, night vision, block interactions, etc.)
- Implements event controllers for managing vanish-related game mechanics and player interactions
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| PLTranslation.java / ENTranslation.java | Added vanish message sections for localization support |
| Translation.java | Added vanish messages interface to translation contract |
| PlaceholdersSetup.java | Updated to use new VanishMetaDataService for online player counting |
| VanishMessages.java / PLVanishMessages.java / ENVanishMessages.java | Created message interfaces and implementations for vanish notifications |
| Controller classes | Implemented event handlers for vanish behavior (join/quit, tab complete, interactions, etc.) |
| VanishService implementations | Created service layer with separate concerns for metadata, invisibility, and main vanish logic |
| VanishConfiguration.java | Added comprehensive configuration options for vanish feature |
| VanishCommand.java | Implemented vanish toggle command with self and other player support |
| PluginConfiguration.java | Integrated vanish configuration into main plugin config |
| PlayerArgument.java | Updated permission reference for vanish visibility |
| Event classes | Created vanish enable/disable events for extensibility |
| API interfaces | Defined service contracts in the API module |
| @@ -20,6 +20,8 @@ | |||
| import com.eternalcode.core.feature.teleportrequest.messages.ENTeleportRequestMessages; | |||
| import com.eternalcode.core.feature.time.messages.ENTimeAndWeatherMessages; | |||
| import com.eternalcode.core.feature.troll.elderguardian.messages.ENElderGuardianMessages; | |||
| import com.eternalcode.core.feature.vanish.messages.ENVanishMessages; | |||
| import com.eternalcode.core.feature.vanish.messages.VanishMessages; | |||
There was a problem hiding this comment.
The import for VanishMessages is unused since ENVanishMessages already implements this interface. This import should be removed to avoid unnecessary dependencies.
| import com.eternalcode.core.feature.vanish.messages.VanishMessages; |
Copilot uses AI. Check for mistakes.
| import org.bukkit.event.entity.FoodLevelChangeEvent; | ||
|
|
||
| @Controller | ||
| // TODO: if-check |
There was a problem hiding this comment.
The TODO comment is unclear and should either be removed if the implementation is complete, or expanded to explain what specific if-check is needed.
| // TODO: if-check |
Copilot uses AI. Check for mistakes.
|
|
||
| event.setCancelled(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] There's an extra space before the closing brace. While this is minor, it's inconsistent with the rest of the codebase formatting.
| } | |
| } |
Copilot uses AI. Check for mistakes.
| this.vanishedPlayers.add(player.getUniqueId()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The player UUID is added to vanishedPlayers inside the loop for each online player, which means it could be added multiple times. This should be moved outside the loop or use a Set to prevent duplicates (though Set already prevents duplicates, the logic is confusing).
| this.vanishedPlayers.add(player.getUniqueId()); | |
| } | |
| } | |
| } | |
| } | |
| this.vanishedPlayers.add(player.getUniqueId()); |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| if (!this.config.silentOpen) { | ||
| event.setCancelled(true); |
There was a problem hiding this comment.
When silentOpen is disabled, the event is cancelled but no feedback is given to the player. This could be confusing as the player won't understand why their interaction was blocked.
| event.setCancelled(true); | |
| event.setCancelled(true); | |
| player.sendMessage("You cannot open containers while vanished."); |
Copilot uses AI. Check for mistakes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive implementation of a vanish feature, refactoring the existing service and adding extensive configuration options and event handling. The changes are well-structured, using separate services for invisibility and metadata, and a set of controllers for different aspects of the vanish functionality.
I've found a few issues that should be addressed:
- A critical logic flaw in
TabCompleteControllerthat could hide unrelated commands. - A high-severity bug in
VanishInvisibleServiceImplwhere a player's UUID is added to a set inside a loop. - A missing permission check in
TabCompleteControllerthat affects administrators. - Some minor issues regarding encapsulation and a leftover
TODOcomment.
Overall, this is a solid feature implementation, but the identified issues, particularly the critical and high severity ones, should be fixed before merging.
| void onCommand(PlayerCommandSendEvent event) { | ||
| Collection<String> commands = event.getCommands(); | ||
|
|
||
| commands.removeIf(command -> { | ||
| for (Player target : this.server.getOnlinePlayers()) { | ||
| if (this.vanishService.isVanished(target) && command.contains(target.getName())) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The logic in onCommand(PlayerCommandSendEvent event) is flawed and can lead to incorrect behavior. It attempts to hide commands by checking if the command string contains the name of a vanished player. This can cause unintended side effects. For example, if a vanished player is named "admin", any command containing "admin" (like /adminchat) could be hidden from players. This event is meant for managing command discovery, and using it to filter based on player names in this way is unreliable and dangerous. Remove this event handler. Hiding players from tab-completion of arguments is better handled in onTabComplete, which is already being done.
| } | ||
| if (!online.equals(player)) { | ||
| online.hidePlayer(this.plugin, player); | ||
| this.vanishedPlayers.add(player.getUniqueId()); |
There was a problem hiding this comment.
The player's UUID is added to the vanishedPlayers set inside the for loop. This will cause the player's UUID to be added multiple times if there are multiple players online from whom the target player is hidden. This is inefficient and logically incorrect. The player should be added to the vanishedPlayers set only once when hidePlayer is called. Move this line to before the loop.
@Override
public void hidePlayer(Player player) {
this.vanishedPlayers.add(player.getUniqueId());
for (Player online : this.server.getOnlinePlayers()) {
if (online.hasPermission(VanishPermissionConstant.VANISH_SEE_PERMISSION)) {
continue;
}
if (!online.equals(player)) {
online.hidePlayer(this.plugin, player);
}
}
}| if (!(event.getSender() instanceof Player)) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
The onTabComplete handler removes vanished players from tab completions for everyone, including administrators who have the eternalcore.vanish.see permission, preventing them from tab-completing a vanished player's name. Check if the command sender has the VanishPermissionConstant.VANISH_SEE_PERMISSION permission. If they do, do not modify the completions.
void onTabComplete(TabCompleteEvent event) {
if (!(event.getSender() instanceof Player player)) {
return;
}
if (player.hasPermission(VanishPermissionConstant.VANISH_SEE_PERMISSION)) {
return;
}
List<String> completions = event.getCompletions();| import org.bukkit.event.entity.FoodLevelChangeEvent; | ||
|
|
||
| @Controller | ||
| // TODO: if-check |
There was a problem hiding this comment.
There is a TODO comment left in the code. Resolve TODOs before merging a pull request, or create a ticket to track the work if it's out of scope for the current changes. Either address this TODO or remove it if it's no longer relevant.
| player.setGlowing(false); | ||
| } | ||
|
|
||
| Team borrowTeam() { |
There was a problem hiding this comment.
The borrowTeam() method is only used within this class. Declare it as private to improve encapsulation.
| Team borrowTeam() { | |
| private Team borrowTeam() { |
| player.openInventory(cloneInventory); | ||
| } | ||
|
|
||
| boolean isContainerType(Material type) { |
There was a problem hiding this comment.
This helper method is only used within OpenSilentController. It should be private to improve encapsulation.
| boolean isContainerType(Material type) { | |
| private boolean isContainerType(Material type) { |
| || Tag.SHULKER_BOXES.isTagged(type); | ||
| } | ||
|
|
||
| Inventory getInventory(BlockState state) { |
There was a problem hiding this comment.
This helper method is only used within OpenSilentController. It should be private to improve encapsulation.
| Inventory getInventory(BlockState state) { | |
| private Inventory getInventory(BlockState state) { |
|
|
||
| void showPlayer(Player player); | ||
|
|
||
| void hideVanishedPlayersFrom(Player player); |
There was a problem hiding this comment.
| void hideVanishedPlayersFrom(Player player); | |
| void hideVanishedPlayersFrom(Player observer); |
There was a problem hiding this comment.
This class is public api, right? So please add simple java docs.
package com.eternalcode.core.feature.vanish;
import org.bukkit.entity.Player;
import java.util.Set;
import java.util.UUID;
/**
* Service for managing player visibility in vanish system.
*/
public interface VanishInvisibleService {
/**
* Hides player from other players.
*/
void hidePlayer(Player player);
/**
* Shows previously hidden player to other players.
*/
void showPlayer(Player player);
/**
* Updates vanish visibility for observer - hides all vanished players from them.
*/
void hideVanishedPlayersFrom(Player observer);
/**
* @return UUIDs of currently vanished players
*/
Set<UUID> getVanishedPlayers();
}|
|
||
| import java.util.UUID; | ||
|
|
||
| public interface VanishMetaDataService { |
There was a problem hiding this comment.
Should VanishMetaDataService be exposed in public API?
This interface seems like low-level metadata management that external consumers shouldn't need direct access to. The VanishInvisibleService already provides the main vanish functionality (hidePlayer, showPlayer, etc.).
There was a problem hiding this comment.
I'm unsure which service should be exposed in the API. Let's decide. My first comment was about VanishInvisibleService. Looking at the VanishService code, only VanishService should be publicly exposed.
|
|
||
| void removeMetadata(Player player); | ||
|
|
||
| boolean hasMetadata(UUID playerUniqueId); |
There was a problem hiding this comment.
i'm thinking about switching back to a more traditional approach. playerUuid maybe?
| return; | ||
| } | ||
|
|
||
| event.setCancelled(true); |
There was a problem hiding this comment.
add fallback message "You are on vanish, you cant use chat"
| return; | ||
| } | ||
|
|
||
| event.setCancelled(true); |
There was a problem hiding this comment.
Add fallback messages, it's magically cancelled without any info.
There was a problem hiding this comment.
Missing inventory interaction blocking for silent inventory access. this can cause duplication bugs.
Vanished players can currently modify items in cloned inventory
| this.config = config; | ||
| } | ||
|
|
||
| @EventHandler |
There was a problem hiding this comment.
@EventHandler(priority = EventPriority.MONITOR)
| this.vanishService = vanishService; | ||
| } | ||
|
|
||
| @EventHandler |
|
|
||
| import org.bukkit.entity.Player; | ||
|
|
||
| public class DisableVanishEvent extends AbstractVanishEvent { |
There was a problem hiding this comment.
Shouldn't these events be cancellable?
|
|
||
| import org.bukkit.entity.Player; | ||
|
|
||
| public class EnableVanishEvent extends AbstractVanishEvent { |
There was a problem hiding this comment.
Shouldn't these events be cancellable?
SIGMA VANISH SERVICE