GH-891 Fix removing catboy using butcher, remove catboy on server shutdown, add catboy config. #891
Conversation
…add catboy config. Add catboy persistance.
WalkthroughThe pull request introduces a comprehensive refactoring of the mob management and catboy features within the EternalCore project. The changes primarily involve reorganizing several classes from the A new event system for entity removal has been implemented with the The modifications aim to improve code organization, provide more flexibility in mob handling, and create a more robust implementation of the catboy feature within the plugin's architecture. 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 (
|
vLuckyyy
changed the title
vLuckyyy
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: 0
🔭 Outside diff range comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyServiceImpl.java (1)
Line range hint
55-61: Clean up persistent data when unmarking catboyDon't forget to remove the persistent data when unmarking a catboy to prevent data leftovers.
player.getPassengers().forEach(entity -> { + if (entity instanceof Cat cat) { + cat.getPersistentDataContainer().remove(this.catBoyPersistentDataKey.getCatboyDataKey()); + } entity.remove(); }); player.getPassengers().clear(); player.setWalkSpeed(DEFAULT_WALK_SPEED);
🧹 Nitpick comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatBoyPersistentDataKey.java (1)
18-24: Remove unnecessary null checkSince CATBOY_DATA_KEY is final and always initialized in the constructor, the null check isn't needed. We can simplify this method.
public NamespacedKey getCatboyDataKey() { - if (this.CATBOY_DATA_KEY == null) { - throw new IllegalStateException("Catboy data key is not initialized"); - } - return this.CATBOY_DATA_KEY; }eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyServiceImpl.java (1)
49-50: Move "catboy" string to a constantThe string "catboy" used in the persistent data should be defined as a constant at the class level for better maintainability.
+ private static final String CATBOY_TAG = "catboy"; PersistentDataContainer persistentDataContainer = entity.getPersistentDataContainer(); - persistentDataContainer.set(this.catBoyPersistentDataKey.getCatboyDataKey(), PersistentDataType.STRING, "catboy"); + persistentDataContainer.set(this.catBoyPersistentDataKey.getCatboyDataKey(), PersistentDataType.STRING, CATBOY_TAG);eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyController.java (1)
115-124: Consider bulk operation for shutdown cleanupWhile the current implementation works, consider adding a bulk operation method to
CatboyServicefor cleaning up multiple players at once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
eternalcore-api/src/main/java/com/eternalcode/core/feature/butcher/ButcherEntityRemoveEvent.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/ButcherArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/ButcherCommand.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/MobEntity.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/MobEntityArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/MobFilter.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/MobType.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatBoyPersistentDataKey.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatBoySettings.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyController.java(4 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyServiceImpl.java(3 hunks)
✅ Files skipped from review due to trivial changes (5)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/MobFilter.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/MobType.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/MobEntity.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/ButcherArgument.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/MobEntityArgument.java
🔇 Additional comments (9)
eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyServiceImpl.java (1)
43-43: Consider the impact of disabling AIDisabling the cat's AI completely might affect other features. Consider if partial AI limitations would work better.
eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/ButcherCommand.java (2)
28-28: Good addition of event handling.Including
EventCallerenhances the command's flexibility.Also applies to: 31-31, 33-33
64-70: Nice implementation of cancellable events.Using
ButcherEntityRemoveEventallows for controlled mob removal.eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatBoySettings.java (1)
1-7: Interface looks good.
CatBoySettingsclearly defines the catboy walk speed setting.eternalcore-api/src/main/java/com/eternalcode/core/feature/butcher/ButcherEntityRemoveEvent.java (1)
1-41: Event class is well-structured.
ButcherEntityRemoveEventcorrectly extendsEventand implementsCancellable.eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyController.java (3)
35-44: Nice constructor update!The new parameters help manage catboy persistence and server interactions.
75-77: Clean scheduler usage!The delayed catboy restoration after teleport looks good.
102-113: Good protection for catboy entities!The event handler properly prevents catboy cats from being removed by the butcher feature.
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (1)
425-441: Great configuration setup!The catboy walk speed configuration is well-documented and uses a reasonable default value (0.4F vs Minecraft's default 0.2F).
| @@ -85,4 +99,27 @@ void onHit(EntityDamageByEntityEvent event) { | |||
| } | |||
| } | |||
|
|
|||
| @EventHandler(ignoreCancelled = true, priority = EventPriority.MONITOR) | |||
There was a problem hiding this comment.
raczej jak modyfikujesz eventy to się zaleca: @EventHandler(priority = EventPriority.HIGH)
...ore/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatBoyPersistentDataKey.java
Outdated
Show resolved
Hide resolved
* Add repository for language. Remove user settings. * Simplify API
...ore/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (13)
eternalcore-core/src/main/java/com/eternalcode/core/user/UserClientBukkitSettings.java (1)
23-26: Consider adding a brief comment.A quick note about what this method does would help future developers.
+ /** + * Checks if the player is currently online. + * @return true if the player is online, false otherwise + */ @Override public boolean isOnline() { return this.getPlayer().isPresent(); }eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/HomeArgument.java (1)
38-38: Constructor changes look good, but let's verify the parameter order.The parameter order in the constructor (ViewerService first) doesn't match the initialization order (ViewerService last). While this works fine, it might be clearer to keep them consistent.
Consider reordering the parameters to match the initialization order:
@Inject HomeArgument( - ViewerService viewerService, TranslationManager translationManager, HomeService homeService, NoticeService noticeService, + ViewerService viewerService )Also applies to: 41-41
eternalcore-api/src/main/java/com/eternalcode/core/feature/language/Language.java (1)
37-47: Consider renaming isEquals to matchesThe method checks for language matching rather than equality. The name 'matches' would better reflect what it does.
- public boolean isEquals(Language other) { + public boolean matches(Language other) {eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/enchant/EnchantArgument.java (1)
Line range hint
18-18: Consider making enchantment levels configurableInstead of hardcoding the suggestion levels (1-5), maybe we could make this configurable? This would make it easier to adjust without changing code. What do you think? 🤔
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/AbstractViewerArgument.java (1)
23-26: Consider extracting the Player check to a helper method.The instanceof check could be clearer as a separate method like
isPlayerSender(). This would make the code easier to read and maintain.Here's a simple way to do it:
+ private boolean isPlayerSender(CommandSender sender) { + return sender instanceof Player; + } + @Override protected ParseResult<T> parse(Invocation<CommandSender> invocation, Argument<T> context, String argument) { - if (invocation.sender() instanceof Player player) { + if (isPlayerSender(invocation.sender())) { + Player player = (Player) invocation.sender();eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyServiceImpl.java (1)
27-38: Good job on dependency injection!The new fields and constructor parameters make the code more configurable. Consider adding null checks for the new parameters.
CatboyServiceImpl( EventCaller eventCaller, CatBoyPersistentDataKey catBoyPersistentDataKey, CatBoySettings catBoySettings ) { + if (eventCaller == null || catBoyPersistentDataKey == null || catBoySettings == null) { + throw new IllegalArgumentException("Dependencies cannot be null"); + } this.eventCaller = eventCaller; this.catBoyPersistentDataKey = catBoyPersistentDataKey; this.catBoySettings = catBoySettings; }eternalcore-core/src/main/java/com/eternalcode/core/translation/TranslationManager.java (1)
30-31: Consider making the constructor publicMaking the constructor public allows
TranslationManagerto be created from other packages if needed.Apply this diff:
- TranslationManager(LanguageService languageService, Translation defaultTranslation) { + public TranslationManager(LanguageService languageService, Translation defaultTranslation) {eternalcore-api/src/main/java/com/eternalcode/core/feature/language/LanguageProvider.java (1)
7-7: Rename parameter for clarity.Consider renaming
UUID playertoUUID playerIdfor better understanding.eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageRepository.java (1)
9-13: Rename parameter for clarity.Consider changing
UUID playertoUUID playerIdfor clearer understanding.eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageLoadController.java (2)
21-24: Consider adding error handling for language loading.The language loading might fail, and it would be good to handle any errors gracefully.
@EventHandler(ignoreCancelled = true, priority = EventPriority.MONITOR) void onJoin(AsyncPlayerPreLoginEvent event) { - this.languageService.loadLanguage(event.getUniqueId()); + try { + this.languageService.loadLanguage(event.getUniqueId()); + } catch (Exception e) { + // Log error but don't prevent join + e.printStackTrace(); + } }
26-29: Make event priorities consistent.The quit event should use the same MONITOR priority as the join event to maintain consistency.
-@EventHandler +@EventHandler(priority = EventPriority.MONITOR) void onQuit(PlayerQuitEvent event) { this.languageService.unloadLanguage(event.getPlayer().getUniqueId()); }eternalcore-api/src/main/java/com/eternalcode/core/feature/language/LanguageService.java (1)
12-18: Consider renaming parameterplayertoplayerIdUsing
playerIdinstead ofplayerfor theUUIDparameter can make it clearer that it's a unique identifier.eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageServiceImpl.java (1)
75-78: Simplify theunloadLanguagemethodYou can simplify
unloadLanguageby removing the unnecessary database call and just removing the player from the cache.Here's the updated method:
- CompletableFuture<Void> unloadLanguage(UUID uniqueId) { - return languageRepository.findLanguage(uniqueId) - .thenAccept(language -> cachedLanguages.remove(uniqueId)); - } + CompletableFuture<Void> unloadLanguage(UUID uniqueId) { + cachedLanguages.remove(uniqueId); + return CompletableFuture.completedFuture(null); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
eternalcore-api/src/main/java/com/eternalcode/core/feature/language/Language.java(3 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/language/LanguageProvider.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/language/LanguageService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/AbstractViewerArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/EnchantmentArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/GameModeArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/NoticeTypeArgument.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/bridge/litecommand/argument/UserArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkKickController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AftPlaceholderSetup.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/ButcherArgument.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/MobEntityArgument.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyServiceImpl.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/container/DisposalCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/enchant/EnchantArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/lore/ItemLoreArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/speed/SpeedArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/speed/SpeedTypeArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/time/TimeArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/fullserverbypass/FullServerBypassController.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/home/HomePlaceholderSetup.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/HomeArgument.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/language/BukkitLanguageProvider.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageInventory.java(6 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageLoadController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageRepository.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageRepositoryImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageServiceImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageSettings.java(0 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/language/config/LanguageConfiguration.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrequest/RequesterArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpInventory.java(5 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpArgument.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpCommand.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/notice/NoticeService.java(5 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/TranslationManager.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/TranslationManagerSetup.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/user/User.java(0 hunks)eternalcore-core/src/main/java/com/eternalcode/core/user/UserClientBukkitSettings.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/user/UserClientNoneSettings.java(0 hunks)eternalcore-core/src/main/java/com/eternalcode/core/user/UserClientSettings.java(0 hunks)eternalcore-core/src/main/java/com/eternalcode/core/user/UserSettings.java(0 hunks)eternalcore-core/src/main/java/com/eternalcode/core/user/UserSettingsImpl.java(0 hunks)eternalcore-core/src/main/java/com/eternalcode/core/viewer/BukkitViewerImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/viewer/BukkitViewerProvider.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/viewer/Viewer.java(0 hunks)
💤 Files with no reviewable changes (7)
- eternalcore-core/src/main/java/com/eternalcode/core/viewer/Viewer.java
- eternalcore-core/src/main/java/com/eternalcode/core/user/UserClientNoneSettings.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageSettings.java
- eternalcore-core/src/main/java/com/eternalcode/core/user/UserSettings.java
- eternalcore-core/src/main/java/com/eternalcode/core/user/UserClientSettings.java
- eternalcore-core/src/main/java/com/eternalcode/core/user/UserSettingsImpl.java
- eternalcore-core/src/main/java/com/eternalcode/core/user/User.java
✅ Files skipped from review due to trivial changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/language/config/LanguageConfiguration.java
🚧 Files skipped from review as they are similar to previous changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/MobEntityArgument.java
🔇 Additional comments (57)
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/NoticeTypeArgument.java (1)
21-22: Nice cleanup!The simplified constructor makes the code cleaner while keeping all the functionality intact.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpArgument.java (1)
34-34: Nice cleanup! 👍The simplified constructor looks good. Removing unused dependencies makes the code cleaner and easier to maintain.
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/lore/ItemLoreArgument.java (1)
24-25: Nice cleanup! 👍The constructor simplification looks good - removing ViewerService makes the code cleaner and matches similar changes across other argument classes.
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/GameModeArgument.java (2)
25-26: Looks good! Clean constructor update.The removal of ViewerService simplifies the dependencies while maintaining the core functionality.
Line range hint
1-47: Overall implementation is solid.The class continues to handle gamemode parsing and suggestions effectively, with clean separation of concerns.
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/speed/SpeedTypeArgument.java (1)
21-22: Nice cleanup! 👍The constructor looks cleaner now with just the essential TranslationManager dependency. This matches the pattern used in other similar classes.
eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AftPlaceholderSetup.java (2)
22-24: Nice cleanup of the constructor!The simplified constructor makes the code cleaner by removing unnecessary dependencies.
34-36: Good job on the placeholder formatting!The code looks clean and straightforward. The translation handling is well-implemented.
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/container/DisposalCommand.java (2)
28-33: Nice job simplifying the constructor!The removal of UserManager makes the code cleaner and easier to maintain.
Line range hint
38-46: Good improvement in translation handling!Getting translations directly with the player's UUID is simpler and more straightforward than going through UserManager.
eternalcore-core/src/main/java/com/eternalcode/core/feature/home/HomePlaceholderSetup.java (1)
72-72: Nice update to use UUID!The change to use
user.getUniqueId()looks good and matches similar updates in other files.eternalcore-core/src/main/java/com/eternalcode/core/viewer/BukkitViewerImpl.java (3)
10-10: LGTM! Clean CONSOLE instance initialization.The simplified CONSOLE instance looks good with the language parameter removed.
15-18: LGTM! Nice and clean constructor.The constructor is now simpler and focused on its core responsibility.
24-25: LGTM! Clear factory method implementation.The player factory method is well-implemented and consistent with the constructor changes.
eternalcore-core/src/main/java/com/eternalcode/core/user/UserClientBukkitSettings.java (1)
23-26: Nice implementation!The
isOnline()method is simple and effective, using the existing caching mechanism.eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/command/WarpCommand.java (2)
47-47: Nice simplification of the method signature!Removing the User parameter makes the code cleaner while keeping all the functionality intact.
69-69: Clean update to match the new WarpInventory design!The simplified inventory opening call looks good, now that language handling is managed by WarpInventory itself.
eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/HomeArgument.java (3)
29-29: Nice addition of the ViewerService field!The new field follows the same pattern as other services in the class.
Line range hint
45-74: The parse method looks good!The ViewerService is used correctly to get the viewer for the command sender.
Line range hint
76-85: The suggest method looks good too!Clean and straightforward use of ViewerService to fetch suggestions.
eternalcore-api/src/main/java/com/eternalcode/core/feature/language/Language.java (3)
17-17: Nice choice using Set for aliases!Using LinkedHashSet is perfect here - it prevents duplicate aliases while keeping their order.
Also applies to: 21-21
28-29: Good job maintaining immutability!The unmodifiableSet wrapper keeps the aliases safe from external modifications.
77-77: Good update to use Locale.of!Using the factory method is the modern way to create Locale instances.
eternalcore-core/src/main/java/com/eternalcode/core/feature/fullserverbypass/FullServerBypassController.java (3)
50-50: Nice method rename! 👍The new name
getServerFullMessageis clearer and follows common Java naming patterns.Also applies to: 57-57
69-69: Good switch to UUID! 🎯Using
user.getUniqueId()instead of the User object directly is more reliable for user identification.
Line range hint
59-65: Great error handling! 🛡️Nice job using Optional and providing default messages when a user isn't found.
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/EnchantmentArgument.java (1)
23-24: Looks good! Nice cleanup.The constructor change is clean and straightforward. Good job removing the unused ViewerService parameter! 👍
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/enchant/EnchantArgument.java (1)
24-25: Looking good! Nice and clean.The constructor change matches the cleanup done in other files. Keep up the good work! 👍
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/AbstractViewerArgument.java (2)
17-19: Nice simplification of the constructor!Removing the ViewerService dependency makes the code cleaner and easier to maintain.
28-30: Good fallback handling!Nice job providing a default language fallback for non-player senders.
eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkKickController.java (1)
63-63: Looking good! Nice consistency update.The change to use UUID instead of User object matches similar updates across the codebase.
Let's double-check other translation usage patterns:
✅ Verification successful
Looking good! The translation retrieval is spot-on.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other translation message retrieval patterns rg "translationManager\.getMessages\(" -A 1Length of output: 3919
eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageCommand.java (1)
25-26: Let's verify the language inventory changes.The simplified command looks good, but let's make sure the language inventory is ready for this change.
✅ Verification successful
All good! The language inventory is ready for this change.
The inventory properly handles the simplified command and takes care of language loading behind the scenes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if LanguageInventory.open() method has been updated ast-grep --pattern 'class LanguageInventory { $$$ open(Player $_) { $$$ } $$$ }'Length of output: 94
Script:
#!/bin/bash # Find LanguageInventory file fd -e java "LanguageInventory" # If found, let's see its content fd -e java "LanguageInventory" -x cat {}Length of output: 6441
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/UserArgument.java (1)
25-26: LGTM! Clean constructor refactoring.The removal of ViewerService simplifies the dependencies while maintaining functionality.
Let's make sure this change is consistent across the codebase:
✅ Verification successful
The ViewerService removal from UserArgument is consistent with the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if ViewerService is still being passed to AbstractViewerArgument ast-grep --pattern 'class $_ extends AbstractViewerArgument<$_> { $$$ $_($_, ViewerService $_, $_) { $$$ } }'Length of output: 121
Script:
#!/bin/bash # Find all classes extending AbstractViewerArgument ast-grep --pattern 'class $_ extends AbstractViewerArgument<$_>' # Search for ViewerService mentions near AbstractViewerArgument rg "ViewerService" -B 2 -A 2Length of output: 18508
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpInventory.java (1)
41-41: Nice work on the LanguageService integration!The field declaration and constructor injection are clean and follow the existing patterns.
Also applies to: 52-52, 61-61
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/speed/SpeedArgument.java (2)
Line range hint
27-50: Clean implementation! 🌟The parsing and suggestion logic is well-structured and handles edge cases nicely.
23-24: Nice cleanup! 👍The constructor is now simpler and cleaner.
Let's make sure this change fits with other similar updates:
eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyServiceImpl.java (2)
14-18: Nice work on the imports and constants!The DEFAULT_WALK_SPEED constant matches Minecraft's standard player speed, which is great for consistency.
Also applies to: 23-23
49-49: Don't forget about cleanup!While saving the catboy state is great, we should remove this data when unmarking a player as catboy.
Could you update the
unmarkAsCatboymethod to remove the persistent data? This would prevent any leftover data.Also applies to: 53-56
eternalcore-core/src/main/java/com/eternalcode/core/translation/TranslationManagerSetup.java (3)
4-4: Looking good! 👍The new import fits right in with the other language-related imports.
16-16: Nice update to the method! 👍The new parameter makes sense as part of moving to UUID-based language management.
25-25: Let's double-check for leftover code! 🔍The change looks good, but we should make sure there aren't any old language retrieval methods we can clean up.
✅ Verification successful
All clear! No cleanup needed ✨
The language-related code is well-organized and there are no old methods to clean up.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old Player-based language retrieval methods that might need cleanup rg -l "getPlayerLanguage|getLanguage.*Player" --type javaLength of output: 59
Script:
#!/bin/bash # Search for language-related methods in translation package fd . -e java -x rg -l "language|Language" {} # Also check for any translation-related classes ast-grep --pattern 'class $_ { $$$ get.*Language($$$) { $$$ } $$$ }'Length of output: 2741
eternalcore-core/src/main/java/com/eternalcode/core/viewer/BukkitViewerProvider.java (3)
84-84: Check consistency of language parameter removal.The default language parameter has been removed here too. This change looks good, assuming it's part of the planned language handling refactoring.
68-68: Confirm user language settings migration.The user's language settings are no longer passed to the viewer. Let's verify where language preferences are now handled.
✅ Verification successful
Language settings are properly handled by LanguageService
The language settings have been migrated from viewer level to a centralized LanguageService, which manages user language preferences through UUID lookups. This is a better approach that separates concerns and improves maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for new language handling implementation rg -A 3 "getLanguage|LanguageService"Length of output: 18349
63-63: Verify language handling after parameter removal.The language parameter has been removed from the fallback viewer creation. Let's make sure this aligns with the new language handling approach.
✅ Verification successful
Language parameter removal is correct ✅
The changes align perfectly with the new implementation. The viewer creation now uses a simpler, language-independent approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if BukkitViewerImpl still has any language-related parameters ast-grep --pattern 'class BukkitViewerImpl { $$$ player($$$) { $$$ } }'Length of output: 83
Script:
#!/bin/bash # Find BukkitViewerImpl file fd BukkitViewerImpl # Once found, examine its content rg -A 5 "class BukkitViewerImpl" --type javaLength of output: 870
Script:
#!/bin/bash # Check the player factory method implementation rg -B 2 -A 10 "static.*player.*\(" eternalcore-core/src/main/java/com/eternalcode/core/viewer/BukkitViewerImpl.javaLength of output: 355
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/PlayerArgument.java (1)
31-31: LGTM! Clean removal of ViewerService dependency.The constructor change looks good and aligns with the broader refactoring effort.
Let's verify the parent class compatibility:
✅ Verification successful
All good! The parent class matches perfectly
The constructor change in PlayerArgument is spot-on with what the parent class expects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if AbstractViewerArgument has been updated to handle the single parameter ast-grep --pattern 'class AbstractViewerArgument { $$$ AbstractViewerArgument(TranslationManager $_) { $$$ } $$$ }'Length of output: 129
Script:
#!/bin/bash # First, let's find the import statement for the parent class rg "import.*AbstractViewerArgument" -l # Then find any class that matches this name ast-grep --pattern 'class AbstractViewerArgument' # As a backup, let's also check the PlayerArgument file to see the extends clause cat eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/PlayerArgument.javaLength of output: 3911
Script:
#!/bin/bash # Look for AbstractViewerArgument in the same directory cat eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/AbstractViewerArgument.java # Backup: search for any file containing the class definition rg "class AbstractViewerArgument" -A 5Length of output: 2561
eternalcore-core/src/main/java/com/eternalcode/core/notice/NoticeService.java (4)
4-5: LGTM!The new imports are needed for the language handling changes.
43-43: LGTM!The new language service field is properly defined and follows the class's style.
54-54: LGTM!The constructor changes properly handle the new language service dependency.
Also applies to: 64-64
90-96: Consider adding error handling and performance optimization.The new implementation could benefit from:
- Error handling for potential language service failures
- Checking if getLanguageNow() is a blocking call that might affect performance
Let's check how getLanguageNow is implemented:
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveArgument.java (1)
24-25: Nice cleanup! 👍The constructor is now simpler and only keeps what it needs.
Let's make sure this change fits with the rest of the codebase:
✅ Verification successful
Looks good! The ViewerService removal is safe here 👍
Each argument class can choose whether it needs ViewerService or not. Some classes still use it, but GiveArgument doesn't need it anymore.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if ViewerService is still used by any other argument classes rg -l "ViewerService.*AbstractViewerArgument"Length of output: 47
Script:
#!/bin/bash # First, find all files that extend or use AbstractViewerArgument rg -l "AbstractViewerArgument" # Then find ViewerService usage in the codebase rg -l "ViewerService"Length of output: 2720
Script:
#!/bin/bash # Check constructors in overlapping files echo "=== ButcherArgument.java ===" rg "class ButcherArgument" -A 5 eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/ButcherArgument.java echo "=== HomeArgument.java ===" rg "class HomeArgument" -A 5 eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/HomeArgument.java # Also check the base class constructor echo "=== AbstractViewerArgument.java ===" rg "class AbstractViewerArgument" -A 5 eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/AbstractViewerArgument.javaLength of output: 1343
eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/ButcherArgument.java (4)
1-7: Nice package organization!Moving this to the butcher package makes the code structure clearer.
28-28: Good addition of ViewerService field!The field is properly marked as final and follows the same pattern as other service fields.
Line range hint
39-40: Good use of ViewerService!Getting the viewer through ViewerService is the right approach.
31-35: Check other places using this constructorThe parameter order has changed. Let's make sure this doesn't break anything.
✅ Verification successful
Parameter reordering is safe ✅
The constructor changes won't cause any issues since the class is managed by the command framework and isn't manually created anywhere in the code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other places creating ButcherArgument rg "new ButcherArgument\(" -A 2Length of output: 33
Script:
#!/bin/bash # Look for Spring/DI configuration mentioning ButcherArgument rg "ButcherArgument" -l # Check if it's registered as a component or command ast-grep --pattern 'class ButcherArgument { $$$ }'Length of output: 253
Script:
#!/bin/bash # Check how ButcherArgument is used in ButcherCommand cat eternalcore-core/src/main/java/com/eternalcode/core/feature/butcher/ButcherCommand.javaLength of output: 3676
eternalcore-core/src/main/java/com/eternalcode/core/translation/TranslationManager.java (2)
25-26: Good addition of LanguageService!Adding
languageServiceimproves how translations are managed.
39-41: Ensure null handling in language retrievalWhen fetching the language, it's good to handle cases where it might be null to prevent any issues.
eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageRepositoryImpl.java (1)
61-61: Verify the usage ofLocale.of()Please check if
Locale.of(language)is correct. You might need to useLocale.forLanguageTag(language)instead.Run this script to search for
Locale.ofusage:✅ Verification successful
Locale.of()usage is correctThe current implementation is the right choice for handling standard locale codes from Bukkit. No changes needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of Locale.of in the codebase rg 'Locale\.of\('Length of output: 443
Script:
#!/bin/bash # Check Language class implementation and any forLanguageTag usage ast-grep --pattern 'class Language { $$$ }' # Also search for forLanguageTag usage rg 'forLanguageTag' # Let's see what format the language strings are in rg -B2 -A2 'Locale\.of\('Length of output: 1853
| public void openInventory(Player player) { | ||
| this.languageService.getLanguage(player.getUniqueId()).whenComplete((language, throwable) -> { | ||
| if (language == null) { | ||
| language = Language.DEFAULT; | ||
| } | ||
|
|
||
| this.createInventory(player, language) | ||
| .open(player); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Let's make the async handling more robust.
A few things to consider:
- Make sure we're on the main thread when opening the inventory
- Add error handling for the async operation
- Check if the player is still online
Here's a safer version:
public void openInventory(Player player) {
this.languageService.getLanguage(player.getUniqueId()).whenComplete((language, throwable) -> {
+ if (throwable != null) {
+ throwable.printStackTrace();
+ return;
+ }
+
+ if (!player.isOnline()) {
+ return;
+ }
+
if (language == null) {
language = Language.DEFAULT;
}
- this.createInventory(player, language)
- .open(player);
+ Language finalLanguage = language;
+ this.server.getScheduler().runTask(this.plugin, () ->
+ this.createInventory(player, finalLanguage).open(player)
+ );
});
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -48,7 +63,7 @@ public void unmarkAsCatboy(Player player) { | |||
| this.catboys.remove(player.getUniqueId()); | |||
| player.getPassengers().forEach(entity -> entity.remove()); | |||
| player.getPassengers().clear(); | |||
| player.setWalkSpeed(0.2F); | |||
| player.setWalkSpeed(DEFAULT_WALK_SPEED); | |||
There was a problem hiding this comment.
Watch out for memory leaks!
The walk speed reset looks good, but we should also clean up the persistent data when removing a catboy.
public void unmarkAsCatboy(Player player) {
this.catboys.remove(player.getUniqueId());
- player.getPassengers().forEach(entity -> entity.remove());
+ player.getPassengers().forEach(entity -> {
+ if (entity instanceof Cat cat) {
+ cat.getPersistentDataContainer().remove(this.catBoyPersistentDataKey.getCatboyDataKey());
+ }
+ entity.remove();
+ });
player.getPassengers().clear();
player.setWalkSpeed(DEFAULT_WALK_SPEED);
this.eventCaller.callEvent(new CatboySwitchEvent(player, false));
}Committable suggestion skipped: line range outside the PR's diff.
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/time/TimeArgument.java
Outdated
Show resolved
Hide resolved
...lcore-core/src/main/java/com/eternalcode/core/feature/teleportrequest/RequesterArgument.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageInventory.java
Outdated
Show resolved
Hide resolved
...nalcore-core/src/main/java/com/eternalcode/core/feature/language/BukkitLanguageProvider.java
Outdated
Show resolved
Hide resolved
...nalcore-core/src/main/java/com/eternalcode/core/feature/language/BukkitLanguageProvider.java
Outdated
Show resolved
Hide resolved
# Conflicts: # eternalcore-api/src/main/java/com/eternalcode/core/feature/language/LanguageService.java # eternalcore-core/src/main/java/com/eternalcode/core/feature/home/HomePlaceholderSetup.java # eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageInventory.java # eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageRepositoryImpl.java # eternalcore-core/src/main/java/com/eternalcode/core/feature/language/LanguageServiceImpl.java # eternalcore-core/src/main/java/com/eternalcode/core/notice/NoticeService.java # eternalcore-core/src/main/java/com/eternalcode/core/translation/TranslationManagerSetup.java
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatBoyEntityService.java (2)
23-34: Consider documenting entity configuration choicesThe cat entity is set as invulnerable and has AI disabled. Let's add a comment explaining why these settings are necessary.
Cat createCatboyEntity(Player player, Cat.Type type) { + // Configure cat entity: + // - Invulnerable to prevent accidental damage + // - AI disabled to prevent wandering Cat cat = (Cat) player.getWorld().spawnEntity(player.getLocation(), EntityType.CAT); cat.setInvulnerable(true); cat.setOwner(player); cat.setAI(false); cat.setCatType(type);
36-38: Add method to clean up persistent dataWe should add a method to remove the persistent data when a catboy is removed.
boolean isCatboy(Cat cat) { return cat.getPersistentDataContainer().has(catboyNamespacedKey, PersistentDataType.BOOLEAN); } + +void removeCatboyData(Cat cat) { + cat.getPersistentDataContainer().remove(catboyNamespacedKey); +}eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyController.java (1)
113-122: Optimize server shutdown cleanupThe shutdown cleanup looks good but could be more efficient by using streams.
@Subscribe void onServerShutdown(EternalShutdownEvent event) { - for (Player onlinePlayer : this.server.getOnlinePlayers()) { - boolean catboy = this.catboyService.isCatboy(onlinePlayer.getUniqueId()); - - if (catboy) { - this.catboyService.unmarkAsCatboy(onlinePlayer); - } - } + this.server.getOnlinePlayers().stream() + .filter(player -> this.catboyService.isCatboy(player.getUniqueId())) + .forEach(this.catboyService::unmarkAsCatboy); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatBoyEntityService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyController.java(4 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyServiceImpl.java(2 hunks)
🔇 Additional comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyServiceImpl.java (1)
Line range hint
51-56: Clean up persistent data when unmarking catboyWe should clean up the persistent data when removing a catboy entity.
public void unmarkAsCatboy(Player player) { this.catboys.remove(player.getUniqueId()); - player.getPassengers().forEach(entity -> entity.remove()); + player.getPassengers().forEach(entity -> { + if (entity instanceof Cat cat && this.catBoyEntityService.isCatboy(cat)) { + this.catBoyEntityService.removeCatboyData(cat); + cat.remove(); + } + }); player.getPassengers().clear(); player.setWalkSpeed(DEFAULT_WALK_SPEED);eternalcore-core/src/main/java/com/eternalcode/core/feature/catboy/CatboyController.java (1)
102-111: 🛠️ Refactor suggestionUse HIGH priority for butcher event
As suggested in past reviews, we should use HIGH priority for event handlers that modify events.
-@EventHandler(ignoreCancelled = true, priority = EventPriority.MONITOR) +@EventHandler(ignoreCancelled = true, priority = EventPriority.HIGH) void onEntityRemoveFromWorld(ButcherEntityRemoveEvent event) {Likely invalid or redundant comment.
No description provided.