fix: let repeated notes play and require full test coverage

pom.xml

@@ -81,6 +81,51 @@
                 <artifactId>maven-surefire-plugin</artifactId>
                 <version>3.5.4</version>
             </plugin>
+            <plugin>
+                <groupId>org.jacoco</groupId>
+                <artifactId>jacoco-maven-plugin</artifactId>
+                <version>0.8.15</version>
+                <executions>
+                    <execution>
+                        <id>prepare-agent</id>
+                        <goals>
+                            <goal>prepare-agent</goal>
+                        </goals>
+                    </execution>
+                    <execution>
+                        <id>report</id>
+                        <goals>
+                            <goal>report</goal>
+                        </goals>
+                    </execution>
+                    <execution>
+                        <!-- Every instruction and branch must be exercised by the unit tests. -->
+                        <id>check</id>
+                        <goals>
+                            <goal>check</goal>
+                        </goals>
+                        <configuration>
+                            <rules>
+                                <rule>
+                                    <element>BUNDLE</element>
+                                    <limits>
+                                        <limit>
+                                            <counter>INSTRUCTION</counter>
+                                            <value>COVEREDRATIO</value>
+                                            <minimum>1.0</minimum>
+                                        </limit>
+                                        <limit>
+                                            <counter>BRANCH</counter>
+                                            <value>COVEREDRATIO</value>
+                                            <minimum>1.0</minimum>
+                                        </limit>
+                                    </limits>
+                                </rule>
+                            </rules>
+                        </configuration>
+                    </execution>
+                </executions>
+            </plugin>
             <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-compiler-plugin</artifactId>

src/main/java/net/tfminecraft/musicalinstruments/InstrumentPlugin.java

@@ -22,25 +22,21 @@ public class InstrumentPlugin extends JavaPlugin {
 
     private static final int BSTATS_PLUGIN_ID = 33322;
 
-    private static InstrumentPlugin instance;
-    private ItemResolver itemResolver;
     private InstrumentManager manager;
 
     // Play counts since the last bStats submission.
-    // Written from the main thread (listener), read and reset from the bStats submit thread every 30 minutes.
+    // Written by the listener and drained when bStats collects chart data every 30 minutes.
+    // bStats collects on the main thread, but its Folia path collects on its own thread, so keep these atomic.
     private final Map<String, AtomicInteger> playCounts = new ConcurrentHashMap<>();
     private final AtomicInteger totalPlays = new AtomicInteger();
 
     @Override
     public void onEnable() {
-        instance = this;
         getLogger().info("MusicalInstruments is enabled!");
 
         saveDefaultConfig();
 
-        itemResolver = new ItemResolver(getLogger());
-
-        manager = new InstrumentManager(this, itemResolver);
+        manager = new InstrumentManager(this, new ItemResolver(getLogger()));
 
         // Resolve instrument templates on the first tick, after every plugin
         // (MMOItems, ItemsAdder, Nexo) has finished enabling and registered its items.
@@ -87,13 +83,4 @@ public void recordInstrumentPlay(String instrument) {
         playCounts.computeIfAbsent(instrument, k -> new AtomicInteger()).incrementAndGet();
         totalPlays.incrementAndGet();
     }
-
-    @Override
-    public void onDisable() {
-        getLogger().info("MusicalInstruments is disabled!");
-    }
-
-    public static InstrumentPlugin getInstance() { return instance; }
-    public ItemResolver getItemResolver() { return itemResolver; }
-    public InstrumentManager getManager() { return manager; }
 }

src/main/java/net/tfminecraft/musicalinstruments/commands/InstrumentCommand.java

@@ -140,15 +140,18 @@ private void handleGive(Player player, String[] args) {
             return;
         }
 
-        String instrument = args[1].toLowerCase();
-        ItemStack item = manager.getInstrumentItem(instrument);
+        // Config keys keep their case, and tab completion suggests them as written.
+        String instrument = manager.findInstrument(args[1]);
 
-        if (item == null) {
-            player.sendMessage("§cUnknown instrument: §e" + instrument);
+        if (instrument == null) {
+            player.sendMessage("§cUnknown instrument: §e" + args[1]);
             return;
         }
 
-        player.getInventory().addItem(item);
+        // Drop whatever does not fit, like vanilla /give.
+        for (ItemStack leftover : player.getInventory().addItem(manager.getInstrumentItem(instrument)).values()) {
+            player.getWorld().dropItemNaturally(player.getLocation(), leftover);
+        }
         player.sendMessage("§aYou received: §e" + instrument);
     }
 

src/main/java/net/tfminecraft/musicalinstruments/items/ItemResolver.java

@@ -10,6 +10,7 @@
 
 import java.lang.reflect.Method;
 import java.util.HashMap;
+import java.util.Locale;
 import java.util.Map;
 import java.util.logging.Logger;
 
@@ -76,7 +77,8 @@ private ItemStack resolveVanilla(String path) {
             return null;
         }
 
-        Material material = Material.matchMaterial(parts[1].toUpperCase());
+        // matchMaterial only strips a lower-case "minecraft:" prefix.
+        Material material = Material.matchMaterial(parts[1].toLowerCase(Locale.ROOT));
         if (material == null) {
             logger.warning("Unknown material '" + parts[1] + "' in item path '" + path + "'.");
             return null;
@@ -89,9 +91,10 @@ private ItemStack resolveVanilla(String path) {
     // Keep the existing legacy text representation, formatting, and exact-string comparisons.
     @SuppressWarnings("deprecation")
     private ItemStack resolveModeled(String path) {
+        // resolve() only routes paths starting with "modeled(", so the bracket is present.
         int open = path.indexOf('(');
         int close = path.lastIndexOf(')');
-        if (open < 0 || close < open) {
+        if (close < open) {
             logger.warning("Malformed modeled item path '" + path + "'. Expected modeled(type=..;name=..;model=..).");
             return null;
         }
@@ -104,7 +107,7 @@ private ItemStack resolveModeled(String path) {
             }
         }
 
-        Material material = Material.matchMaterial(attributes.getOrDefault("type", "DIRT").toUpperCase());
+        Material material = Material.matchMaterial(attributes.getOrDefault("type", "dirt").toLowerCase(Locale.ROOT));
         if (material == null) {
             logger.warning("Invalid material type in modeled item '" + path + "'.");
             return null;

src/main/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListener.java

@@ -7,48 +7,37 @@
 import org.bukkit.event.EventHandler;
 import org.bukkit.event.Listener;
 import org.bukkit.event.player.PlayerItemHeldEvent;
-import org.bukkit.event.player.PlayerQuitEvent;
-import org.bukkit.scheduler.BukkitRunnable;
 import net.tfminecraft.musicalinstruments.InstrumentPlugin;
 import net.tfminecraft.musicalinstruments.events.InstrumentPlayEvent;
 import net.tfminecraft.musicalinstruments.managers.InstrumentManager;
 
-import java.util.HashMap;
-import java.util.Map;
-import java.util.UUID;
-
 // ====================================
 // Handles instrument-related events.
 // Responsible for playing sounds when players change hotbar slots while holding an instrument.
 // ====================================
 public class InstrumentListener implements Listener {
 
+    // Hotbar slot 9, which the player returns to after each note.
+    private static final int RESET_SLOT = 8;
+
     private final InstrumentPlugin plugin;
     private final InstrumentManager manager;
-    private final Map<UUID, Integer> instrumentTasks;
 
     public InstrumentListener(InstrumentPlugin plugin, InstrumentManager manager) {
         this.plugin = plugin;
         this.manager = manager;
-        this.instrumentTasks = new HashMap<>();
     }
 
-    @EventHandler
-    public void onPlayerQuit(PlayerQuitEvent event) {
-        stopInstrumentDisplay(event.getPlayer());
-    }
-
-    @EventHandler
+    @EventHandler(ignoreCancelled = true)
     public void onPlayerHotbarChange(PlayerItemHeldEvent event) {
         Player player = event.getPlayer();
         String instrument = manager.getInstrument(player.getInventory().getItemInOffHand());
 
-        if (instrument == null) {
+        // Pressing the reset slot again sends nothing, so it cannot play a note.
+        if (instrument == null || event.getNewSlot() == RESET_SLOT) {
             return;
         }
 
-        startInstrumentDisplay(player, instrument);
-
         // Get hotbar slot (1-9)
         int newSlot = event.getNewSlot() + 1;
 
@@ -86,44 +75,10 @@ public void onPlayerHotbarChange(PlayerItemHeldEvent event) {
             1.0
         );
 
-        // Switch back to 9th hotbar slot after playing (so we can use the same note multiple times)
-        player.getInventory().setHeldItemSlot(8);
-    }
-
-    // ====================================
-    // Check which instrument is being held by the player.
-    // ====================================
-    private void startInstrumentDisplay(Player player, String instrument) {
-        if (instrumentTasks.containsKey(player.getUniqueId())) {
-            return;
-        }
-
-        BukkitRunnable task = new BukkitRunnable() {
-            @Override
-            public void run() {
-                if (!player.isOnline()) {
-                    stopInstrumentDisplay(player);
-                    return;
-                }
-                String currentInstrument = manager.getInstrument(player.getInventory().getItemInOffHand());
-                if (!instrument.equals(currentInstrument)) {
-                    stopInstrumentDisplay(player);
-                }
-            }
-        };
-
-        int taskId = task.runTaskTimer(plugin, 0L, 20L).getTaskId();
-        instrumentTasks.put(player.getUniqueId(), taskId);
-    }
-
-    // ====================================
-    // Stops monitoring if the player is holding an instrument.
-    // ====================================
-    private void stopInstrumentDisplay(Player player)
-    {
-        Integer taskId = instrumentTasks.remove(player.getUniqueId());
-        if (taskId != null) {
-            Bukkit.getScheduler().cancelTask(taskId);
-        }
+        // Switch back to 9th hotbar slot after playing (so we can use the same note multiple times).
+        // The event must be cancelled too: otherwise the server applies the pressed slot after this
+        // handler, while the client stays on slot 9, and Paper then ignores the next press of that key.
+        player.getInventory().setHeldItemSlot(RESET_SLOT);
+        event.setCancelled(true);
     }
 }

src/main/java/net/tfminecraft/musicalinstruments/managers/InstrumentManager.java

@@ -51,10 +51,16 @@ public void loadTemplates() {
                     plugin.getLogger().warning("Could not resolve item '" + configPath + "' for instrument '" + instrument + "'.");
                     continue;
                 }
+                // Air in the off-hand never counts as an instrument, so it could not be played.
+                if (template.getType().isAir()) {
+                    plugin.getLogger().warning("Item '" + configPath + "' for instrument '" + instrument + "' is air.");
+                    continue;
+                }
 
                 ItemStack cosmeticFree = withoutCosmetics(template);
                 templates.put(instrument, template);
                 cosmeticFreeTemplates.put(instrument, cosmeticFree);
+                warnAboutResetSlot(instrument);
             } catch (Exception e) {
                 plugin.getLogger().warning("Failed to load instrument '" + instrument + "': " + e.getMessage());
             }
@@ -92,14 +98,23 @@ public String getInstrument(ItemStack item) {
         return match;
     }
 
+    // Playing a note returns the player to slot 9, so notes mapped there cannot be played.
+    private void warnAboutResetSlot(String instrument) {
+        for (String key : new String[] {"9", "9+sneak"}) {
+            if (plugin.getConfig().contains(instrument + ".hotbar-sounds." + key)) {
+                plugin.getLogger().warning("Instrument '" + instrument + "' maps hotbar-sounds." + key
+                        + ", but slot 9 is where the hotbar resets after each note, so it is ignored.");
+            }
+        }
+    }
+
+    // Only called with non-air items, which always have item meta.
     private static ItemStack withoutCosmetics(ItemStack item) {
         ItemStack copy = item.clone();
         ItemMeta meta = copy.getItemMeta();
-        if (meta != null) {
-            meta.displayName(null);
-            meta.lore(null);
-            copy.setItemMeta(meta);
-        }
+        meta.displayName(null);
+        meta.lore(null);
+        copy.setItemMeta(meta);
         return copy;
     }
 
@@ -116,6 +131,19 @@ public String getSoundKey(String instrument, int slot, boolean sneaking)
     public double getPitch(String instrument){ return plugin.getConfig().getDouble(instrument + ".hotbar-sounds.pitch", 1.0); }
     public Set<String> getAllInstruments() { return Collections.unmodifiableSet(templates.keySet()); }
 
+    // Finds a loaded instrument by name, preferring an exact match over a case-insensitive one.
+    public String findInstrument(String name) {
+        if (templates.containsKey(name)) {
+            return name;
+        }
+        for (String instrument : templates.keySet()) {
+            if (instrument.equalsIgnoreCase(name)) {
+                return instrument;
+            }
+        }
+        return null;
+    }
+
     // Gets a copy of an instrument's cached item template.
     public ItemStack getInstrumentItem(String instrument) {
         ItemStack template = templates.get(instrument);

src/main/java/net/tfminecraft/musicalinstruments/util/LegacyModelData.java

@@ -8,26 +8,10 @@
 public final class LegacyModelData {
     private LegacyModelData() {}
 
-    public static boolean has(ItemMeta meta) {
-        return !meta.getCustomModelDataComponent().getFloats().isEmpty();
-    }
-
-    public static int get(ItemMeta meta) {
-        List<Float> floats = meta.getCustomModelDataComponent().getFloats();
-        if (floats.isEmpty()) {
-            throw new IllegalStateException("We don't have CustomModelData! Check hasCustomModelData first!");
-        }
-        return floats.get(0).intValue();
-    }
-
-    public static void set(ItemMeta meta, Integer value) {
-        if (value == null) {
-            meta.setCustomModelDataComponent(null);
-            return;
-        }
+    public static void set(ItemMeta meta, int value) {
         CustomModelDataComponent component = meta.getCustomModelDataComponent();
         // The former integer setter replaced the entire component, not just its first float.
-        component.setFloats(List.of(value.floatValue()));
+        component.setFloats(List.of((float) value));
         component.setFlags(List.of());
         component.setStrings(List.of());
         component.setColors(List.of());

src/test/java/com/nexomc/nexo/api/NexoItems.java

@@ -0,0 +1,15 @@
+package com.nexomc.nexo.api;
+
+import java.util.function.Function;
+
+/** Stands in for the Nexo item lookup that ItemResolver reaches through reflection. */
+public final class NexoItems {
+
+    public static Function<String, Object> lookup = id -> null;
+
+    private NexoItems() {}
+
+    public static Object itemFromId(String id) {
+        return lookup.apply(id);
+    }
+}

src/test/java/dev/lone/itemsadder/api/CustomStack.java

@@ -0,0 +1,15 @@
+package dev.lone.itemsadder.api;
+
+import java.util.function.Function;
+
+/** Stands in for the ItemsAdder item lookup that ItemResolver reaches through reflection. */
+public final class CustomStack {
+
+    public static Function<String, Object> lookup = id -> null;
+
+    private CustomStack() {}
+
+    public static Object getInstance(String id) {
+        return lookup.apply(id);
+    }
+}