Fix: 修复历史遗留问题#14
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request attempts to fix historical legacy issues by adding tick counting functionality to TickingMenuBlock, adding recipe unregistration support to MachineRecipeType, and adjusting the timing of a success message in CraftingBlock.
- Added tick counting mechanism with
tickCount,machineTickCount(), anduniqueTick()methods toTickingMenuBlock - Added
unregister()method toMachineRecipeTypefor removing recipes - Modified
register()inMachineRecipeTypeto callsuper.register() - Moved crafting success message in
CraftingBlockto execute before item pushing and consumption
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| TickingMenuBlock.java | Adds tick counting infrastructure with a tickCount field and related methods for tracking machine ticks |
| MachineRecipeType.java | Adds unregister() method for recipe removal and calls parent register() method |
| CraftingBlock.java | Reorders the crafting success message to appear before output pushing and input consumption |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| public void unregister(ItemStack[] recipe, ItemStack result) { | ||
| recipes.remove(recipe, result); |
There was a problem hiding this comment.
The unregister method is incomplete. It only removes the recipe from the local recipes map but doesn't notify the registered callbacks or call super.unregister() (if such a method exists in the parent class). This is inconsistent with the register method which calls both callbacks and super.register(). Consider whether callbacks should be notified of unregistrations, and check if the parent class has an unregister method that should be called.
| recipes.remove(recipe, result); | |
| recipes.remove(recipe, result); | |
| callbacks.forEach(c -> c.accept(recipe, result)); | |
| super.unregister(recipe, result); |
| p.sendMessage(ChatColor.GREEN + "已合成: " + ItemUtils.getItemName(output)); | ||
| menu.pushItem(output, layout.outputSlots()); | ||
| recipe.consume(input); |
There was a problem hiding this comment.
The success message is now sent before the actual crafting operations (menu.pushItem() and recipe.consume()) complete. While this may not cause functional issues due to the prior menu.fits() check, it's semantically incorrect to notify the player of success before the operation is fully completed. Consider keeping the message after recipe.consume(input) to ensure it's sent only after all crafting operations have successfully completed.
| p.sendMessage(ChatColor.GREEN + "已合成: " + ItemUtils.getItemName(output)); | |
| menu.pushItem(output, layout.outputSlots()); | |
| recipe.consume(input); | |
| menu.pushItem(output, layout.outputSlots()); | |
| recipe.consume(input); | |
| p.sendMessage(ChatColor.GREEN + "已合成: " + ItemUtils.getItemName(output)); |
There was a problem hiding this comment.
显然这一定会complete 而且pushItem会修改output导致最终输出的内容不正确
Description
Code Changes
Related Issues
Checklist
NullableorNonnullannotations to my methods