fix: stack CustomFishing fish and keep food stacking consistent

README.md

@@ -11,7 +11,7 @@ Cooking makes food an interactive part of everyday life. Players grow and gather
 - **Serving and sharing** — plate dishes, ladle soup into bowls, and carve foods into portions for the table.
 - **Quality and freshness** — ingredient quality and composition influence finished dishes, while aging and food tags carry preparation history through the kitchen.
 - **Farming and husbandry** — crop fertility and harvest quality sit alongside animal care, breeding, genetics, and produce collection.
-- **Fish and seafood** — turn supported catches into cooking ingredients while preserving catch size.
+- **Fish and seafood** — turn supported catches into cooking ingredients that keep their catch size; fish of one kind and quality stack whatever their size.
 - **Character nutrition** — meals feed a character's food reserve and influence diet quality, with variety rewarding a broader selection of ingredients.
 - **MMOItems crafting stations** — the `cooking{item="seafood(type=seafood_whole;origin=Salmon)",amount=64,display="Salmon"}` ingredient takes Cooking food, plus the plain items Cooking converts on pickup (a fresh catch counts as the salmon it becomes). `item` is a `c.` path without the prefix and must name a food `type`; quality and tags filters work too. Cooking claims those items before MMOItems' `vanilla{}` type, so use `cooking{}` for salmon, wheat, sugar, beef and the other converted foods. Cancelling a queued craft refunds the least the line accepts: its lowest quality, aged out unless the line pins that tag.
 

src/main/java/net/tfminecraft/cooking/fishing/LegacyFishScan.java

@@ -1,30 +1,50 @@
 package net.tfminecraft.cooking.fishing;
 
 import org.bukkit.Material;
+import org.bukkit.Tag;
 import org.bukkit.entity.Player;
 import org.bukkit.inventory.Inventory;
 import org.bukkit.inventory.ItemStack;
+import org.bukkit.persistence.PersistentDataContainer;
+import org.bukkit.persistence.PersistentDataType;
 
-import net.tfminecraft.cooking.item.FoodItem;
+import net.tfminecraft.cooking.manager.TagManager;
+import net.tfminecraft.cooking.utils.Keys;
 import net.tfminecraft.tlibs.itemscan.ItemScanHandler;
 
+/** Converts leftover CustomFishing fish, and clears CustomFishing data that older versions left on whole fish. */
 public final class LegacyFishScan implements ItemScanHandler {
 
     @Override
     public boolean matches(ItemStack stack) {
-        return stack != null
-                && stack.getType() == Material.COD
-                && FoodItem.fromItem(stack) == null;
+        if (stack == null || !Tag.ITEMS_FISHES.isTagged(stack.getType())) {
+            return false;
+        }
+        PersistentDataContainer data = stack.getItemMeta().getPersistentDataContainer();
+        return isFood(data) ? SeafoodWholeItems.carriesCatchData(data) : stack.getType() == Material.COD;
     }
 
     @Override
     public void update(Player player, Inventory inventory, int slot, ItemStack stack) {
         if (inventory == null || slot < 0) {
             return;
         }
-        ItemStack built = LegacyFishConversion.convert(player, stack);
+        ItemStack built;
+        if (!isFood(stack.getItemMeta().getPersistentDataContainer())) {
+            built = LegacyFishConversion.convert(player, stack);
+        } else if (TagManager.isHeldSlot(player, inventory, slot)) {
+            // Like TagManager, leave a held fish alone so eating or using it isn't interrupted.
+            return;
+        } else {
+            // Earlier handlers in this scan (TagManager) may have rewritten the slot, so clean what is there now.
+            built = SeafoodWholeItems.withoutCatchData(inventory.getItem(slot));
+        }
         if (built != null) {
             inventory.setItem(slot, built);
         }
     }
+
+    private static boolean isFood(PersistentDataContainer data) {
+        return data.has(Keys.FOOD_ID, PersistentDataType.STRING);
+    }
 }

src/main/java/net/tfminecraft/cooking/fishing/SeafoodWholeItems.java

@@ -1,11 +1,15 @@
 package net.tfminecraft.cooking.fishing;
 
 import org.bukkit.inventory.ItemStack;
+import org.bukkit.inventory.meta.ItemMeta;
+import org.bukkit.persistence.PersistentDataContainer;
+import org.bukkit.persistence.PersistentDataType;
 
 import net.tfminecraft.cooking.item.FoodItem;
 import net.tfminecraft.cooking.item.IngredientLineage;
 import net.tfminecraft.cooking.loader.FoodLoader;
 import net.tfminecraft.cooking.utils.ItemBuilder;
+import net.tfminecraft.cooking.utils.Keys;
 
 public final class SeafoodWholeItems {
     private SeafoodWholeItems() {}
@@ -46,11 +50,58 @@ public static ItemStack build(ItemStack caught, VanillaFish fish, int quality) {
                 quality);
     }
 
+    /**
+     * True for a whole fish built by an older version on the caught CustomFishing item itself, so
+     * it still carries that item's per-catch data.
+     */
+    static boolean carriesCatchData(PersistentDataContainer data) {
+        return CustomFishingCatalog.WHOLE_TYPE.equals(data.get(Keys.FOOD_ID, PersistentDataType.STRING))
+                && data.has(Keys.CUSTOM_FISHING_ID, PersistentDataType.STRING)
+                && !data.has(Keys.CLEAN_CATCH, PersistentDataType.BYTE);
+    }
+
+    /** The whole fish with its Cooking data and look, but nothing else left over from the catch. */
+    static ItemStack withoutCatchData(ItemStack whole) {
+        // Lore as components: ItemRef.mergeMeta's legacy text round trip can lose formatting.
+        ItemStack clean = lookOf(whole);
+        ItemMeta from = whole.getItemMeta();
+        ItemMeta to = clean.getItemMeta();
+        to.lore(from.lore());
+        from.getPersistentDataContainer().copyTo(to.getPersistentDataContainer(), true);
+        to.getPersistentDataContainer().set(Keys.CLEAN_CATCH, PersistentDataType.BYTE, (byte) 1);
+        clean.setItemMeta(to);
+        return clean;
+    }
+
+    /**
+     * The caught item's name and model on a fresh stack. CustomFishing tags every catch with its own
+     * exact size, so a whole fish built on the caught item itself never stacks with another.
+     */
+    static ItemStack lookOf(ItemStack caught) {
+        ItemStack look = new ItemStack(caught.getType(), caught.getAmount());
+        if (caught.hasItemMeta()) look.setItemMeta(copyLook(caught.getItemMeta(), look.getItemMeta()));
+        return look;
+    }
+
+    static ItemMeta copyLook(ItemMeta from, ItemMeta to) {
+        if (from.hasDisplayName()) to.displayName(from.displayName());
+        if (from.hasItemName()) to.itemName(from.itemName());
+        if (from.hasCustomModelDataComponent()) to.setCustomModelDataComponent(from.getCustomModelDataComponent());
+        if (from.hasItemModel()) to.setItemModel(from.getItemModel());
+        return to;
+    }
+
     private static FoodItem templateOrNull() {
         return FoodLoader.getByString(CustomFishingCatalog.WHOLE_TYPE);
     }
 
     private static ItemStack build(ItemStack caught, FoodItem item, int quality) {
-        return ItemBuilder.buildSingleWithQuality(item, caught, quality);
+        ItemStack whole = ItemBuilder.buildSingleWithQuality(item, lookOf(caught), quality);
+        if (whole != null) {
+            ItemMeta meta = whole.getItemMeta();
+            meta.getPersistentDataContainer().set(Keys.CLEAN_CATCH, PersistentDataType.BYTE, (byte) 1);
+            whole.setItemMeta(meta);
+        }
+        return whole;
     }
 }

src/main/java/net/tfminecraft/cooking/manager/ConversionManager.java

@@ -46,15 +46,23 @@ public void onClick(InventoryClickEvent event) {
                 || PlayerActionContext.capture(player) == null) return;
         Inventory top = event.getView().getTopInventory();
         if (!stackingInventory(top)) return;
-        if (event.getSlotType() != InventoryType.SlotType.CONTAINER
-                && event.getSlotType() != InventoryType.SlotType.QUICKBAR) return;
+        InventoryType.SlotType slotType = event.getSlotType();
+        // A double-click may start in the crafting grid; collectFood never takes the result.
+        boolean gridCollect = slotType == InventoryType.SlotType.CRAFTING
+                && event.getAction() == InventoryAction.COLLECT_TO_CURSOR;
+        if (slotType != InventoryType.SlotType.CONTAINER
+                && slotType != InventoryType.SlotType.QUICKBAR && !gridCollect) return;
         Inventory clicked = event.getClickedInventory();
         if (clicked == null) return;
 
         if (event.getAction() == InventoryAction.MOVE_TO_OTHER_INVENTORY && event.isShiftClick()) {
             shiftFood(event, top, clicked);
             return;
         }
+        if (event.getAction() == InventoryAction.COLLECT_TO_CURSOR) {
+            collectFood(event, top);
+            return;
+        }
         if (event.getClick() != ClickType.LEFT && event.getClick() != ClickType.RIGHT) return;
         ItemStack cursor = event.getCursor();
         ItemStack current = event.getCurrentItem();
@@ -127,6 +135,58 @@ private static void shiftFood(InventoryClickEvent event, Inventory top, Inventor
         event.setCurrentItem(remainder(item, remaining));
     }
 
+    private record Slot(Inventory inventory, int index) {
+        ItemStack item() {
+            return inventory.getItem(index);
+        }
+    }
+
+    /**
+     * Double-click gathers only exact copies, so food that aged apart would stay behind. When some
+     * did, gather it the way vanilla does: the container or crafting grid first, then storage, the
+     * hotbar and the off-hand, part stacks before full ones. Otherwise vanilla gathers as usual.
+     */
+    private static void collectFood(InventoryClickEvent event, Inventory top) {
+        ItemStack cursor = event.getCursor();
+        if (FoodItem.fromItem(cursor) == null) return;
+        Inventory bottom = event.getView().getBottomInventory();
+        boolean crafting = top.getType() == InventoryType.CRAFTING;
+        List<Slot> order = new ArrayList<>();
+        // The crafting result (slot 0) is never gathered.
+        for (int i = crafting ? 1 : 0; i < top.getSize(); i++) order.add(new Slot(top, i));
+        // The player view then lists the armour, head to feet.
+        if (crafting) for (int i = 39; i >= 36; i--) order.add(new Slot(bottom, i));
+        for (int i = 9; i < 36; i++) order.add(new Slot(bottom, i));
+        for (int i = 0; i < 9; i++) order.add(new Slot(bottom, i));
+        if (crafting) order.add(new Slot(bottom, 40));
+
+        List<Slot> food = new ArrayList<>();
+        boolean agedApart = false;
+        for (Slot slot : order) {
+            ItemStack item = slot.item();
+            if (!stackableFood(cursor, item)) continue;
+            food.add(slot);
+            agedApart |= !item.isSimilar(cursor);
+        }
+        if (!agedApart) return;
+        event.setCancelled(true);
+
+        ItemStack gathered = cursor;
+        int limit = Math.min(cursor.getMaxStackSize(), bottom.getMaxStackSize());
+        for (boolean fullStacks : new boolean[] {false, true}) {
+            for (Slot slot : food) {
+                ItemStack item = slot.item();
+                int room = limit - gathered.getAmount();
+                // A slot emptied by the first pass is null in the second.
+                if (room <= 0 || item == null || (item.getAmount() >= item.getMaxStackSize()) != fullStacks) continue;
+                int amount = Math.min(room, item.getAmount());
+                gathered = mergedFood(gathered, item, amount);
+                slot.inventory().setItem(slot.index(), remainder(item, item.getAmount() - amount));
+            }
+        }
+        event.getView().setCursor(gathered);
+    }
+
     private static boolean stackableFood(ItemStack a, ItemStack b) {
         if (a == null || b == null || a.isEmpty() || b.isEmpty() || a.getType() != b.getType()) return false;
         FoodItem foodA = FoodItem.fromItem(a);
@@ -137,7 +197,8 @@ private static boolean stackableFood(ItemStack a, ItemStack b) {
         return InventoryAdder.equalsFood(foodA, foodB) && sameApartFromAging(a, b);
     }
 
-    private static ItemStack mergedFood(ItemStack current, ItemStack incoming, int amount) {
+    /** current with amount more of incoming, keeping the more aged copy's data. */
+    public static ItemStack mergedFood(ItemStack current, ItemStack incoming, int amount) {
         ItemStack merged = BundleFoodStacker.moreAged(current, incoming).clone();
         merged.setAmount(current.getAmount() + amount);
         return merged;
@@ -278,6 +339,7 @@ public void onOpen(InventoryOpenEvent e) {
         InventoryHolder holder = top.getHolder();
         if (holder != null &&
             !(holder instanceof org.bukkit.block.BlockState) &&
+            !(holder instanceof org.bukkit.block.DoubleChest) &&
             !(holder instanceof org.bukkit.entity.Entity) &&
             !(holder instanceof Player)) {
             return;
@@ -323,7 +385,8 @@ public void onOpen(InventoryOpenEvent e) {
     /**
      * True when two food stacks differ only in how far they have aged, so one can be copied over
      * the other to let them stack. equalsFood matches by kind and tag step alone, so without this
-     * a carved roast, a sausage chain or a big catch would be replaced by a fresh copy of another.
+     * a carved roast or a sausage chain would be replaced by a fresh copy of another. Catch size
+     * changes nothing once a fish is caught, so fish of one kind and quality stack whatever their size.
      */
     public static boolean sameApartFromAging(ItemStack a, ItemStack b) {
         return withoutAging(a).isSimilar(withoutAging(b));
@@ -335,12 +398,15 @@ private static ItemStack withoutAging(ItemStack item) {
         ItemMeta meta = copy.getItemMeta();
         if (meta == null) return copy;
         var pdc = meta.getPersistentDataContainer();
-        // The clock, the progress within each tag step, and the lore written from them. Food lore
+        // The clock, the progress within each tag step, catch size, the clean-catch marker (fish
+        // built from plain vanilla fish never got it), and the lore written from them. Food lore
         // is always rebuilt whole from the item's data (ItemBuilder.stamp), which is compared.
         pdc.remove(Keys.LAST_UPDATE);
         pdc.remove(Keys.AGE_REMAINDER);
         pdc.remove(Keys.TAGS);
         pdc.remove(Keys.LORE_INDEX_MAP);
+        pdc.remove(Keys.CATCH_SIZE_CM);
+        pdc.remove(Keys.CLEAN_CATCH);
         meta.lore(null);
         copy.setItemMeta(meta);
         return copy;

src/main/java/net/tfminecraft/cooking/manager/TagManager.java

@@ -49,7 +49,7 @@ public void update(Player player, Inventory inventory, int slot, ItemStack stack
         }
     }
 
-    static boolean isHeldSlot(Player player, Inventory inventory, int slot) {
+    public static boolean isHeldSlot(Player player, Inventory inventory, int slot) {
         if (player == null || !(inventory instanceof PlayerInventory playerInventory)) {
             return false;
         }

src/main/java/net/tfminecraft/cooking/utils/InventoryAdder.java

@@ -26,7 +26,8 @@ public static ItemStack addItem(Player p, ItemStack in) {
             if (space <= 0) continue;
 
             int add = Math.min(space, amount);
-            cur.setAmount(cur.getAmount() + add);
+            // Keep the older copy's data, so adding food never makes the stack fresher.
+            inv.setItem(slot, ConversionManager.mergedFood(cur, in, add));
             amount -= add;
 
             if (amount <= 0) return null;

src/main/java/net/tfminecraft/cooking/utils/Keys.java

@@ -76,4 +76,8 @@ public class Keys {
 
     public static final NamespacedKey CUSTOM_FISHING_ID =
             new NamespacedKey(Cooking.plugin, "custom_fishing_id");
+
+    /** On whole fish built without the caught item's CustomFishing data. */
+    public static final NamespacedKey CLEAN_CATCH =
+            new NamespacedKey(Cooking.plugin, "clean_catch");
 }

src/test/java/net/tfminecraft/cooking/fishing/FishingRuntimeCoverageTest.java

@@ -43,6 +43,7 @@ public class FishingRuntimeCoverageTest {
     PluginManager plugins;
     Plugin customFishing;
     MockedStatic<Bukkit> bukkit;
+    MockedStatic<SeafoodWholeItems> wholeItems;
     boolean oldRegistered;
     CatchMapping tuna = new CatchMapping("tuna", "Tuna", "fish");
 
@@ -55,6 +56,9 @@ public class FishingRuntimeCoverageTest {
         Server server=mock(Server.class); when(server.getPluginManager()).thenReturn(plugins);
         when(Cooking.plugin.getServer()).thenReturn(server); when(Cooking.plugin.getLogger()).thenReturn(Logger.getLogger("fishing-test"));
         bukkit=mockStatic(Bukkit.class,CALLS_REAL_METHODS); bukkit.when(Bukkit::getPluginManager).thenReturn(plugins);
+        // MockBukkit has no custom model data or item model components, so keep the fresh look as it is.
+        wholeItems=mockStatic(SeafoodWholeItems.class,CALLS_REAL_METHODS);
+        wholeItems.when(()->SeafoodWholeItems.copyLook(any(),any())).thenAnswer(inv->inv.getArgument(1));
         CustomFishingCatalog.replace(Map.of("tuna",tuna),Map.of("rod",new RodBand(3,3)),new RodBand(2,2),Map.of(),Map.of(),Map.of(17,"tuna"),45);
         QualityConfig.apply(1,5,null,null); PermissionEffectsConfig.apply(List.of(),List.of());
         env.originQuality.when(()->net.tfminecraft.cooking.quality.OriginQualityResolver.applyPickupPermissions(any(),anyInt()))
@@ -64,7 +68,7 @@ public class FishingRuntimeCoverageTest {
                 .thenAnswer(inv->env.stack(inv.getArgument(0),Material.COD,1));
     }
     @AfterEach void cleanup() throws Exception {
-        bukkit.close(); env.close();
+        wholeItems.close(); bukkit.close(); env.close();
         var field=CustomFishingBridge.class.getDeclaredField("registered"); field.setAccessible(true); field.setBoolean(null,oldRegistered);
         CustomFishingCatalog.load(new File("src/main/resources/custom-fishing.yml"));
     }
@@ -144,6 +148,11 @@ public class FishingRuntimeCoverageTest {
             ItemStack converted=LegacyFishConversion.convert(env.player,external);
             assertEquals(7,converted.getAmount()); assertEquals(45,env.resolve(converted).getCatchSizeCm());
             LegacyFishScan scan=new LegacyFishScan(); assertFalse(scan.matches(null)); assertFalse(scan.matches(new ItemStack(Material.SALMON)));
+            // The workshop resolves food by its own key; the scan reads the real food id.
+            ItemMeta convertedMeta=converted.getItemMeta();
+            convertedMeta.getPersistentDataContainer().set(net.tfminecraft.cooking.utils.Keys.FOOD_ID,
+                    org.bukkit.persistence.PersistentDataType.STRING,CustomFishingCatalog.WHOLE_TYPE);
+            converted.setItemMeta(convertedMeta);
             assertTrue(scan.matches(external)); assertFalse(scan.matches(converted));
             scan.update(env.player,null,0,external); scan.update(env.player,env.player.getInventory(),-1,external);
             scan.update(env.player,env.player.getInventory(),4,external); assertEquals(7,env.player.getInventory().getItem(4).getAmount());