From e11767f84e408e7cb95ef3971cf17eaa70345449 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Sat, 26 Sep 2026 09:43:01 +0000 Subject: [PATCH 1/2] fix: let repeated notes play and require full test coverage Paper applies the pressed hotbar slot after PlayerItemHeldEvent handlers run, so resetting to slot 9 left the server on the pressed slot and Paper then ignored the next press of the same key. Cancel the slot change after the reset so the server and client both stay on slot 9. Also skip slot changes other plugins cancel, reject instrument items that resolve to air, accept config case and drop leftovers in /instruments give, and resolve minecraft:-prefixed materials. Remove the per-player task that never displayed anything, and accessors and model-data helpers nothing calls. JaCoCo now fails verify below 100% instruction and branch coverage. Co-Authored-By: Claude Opus 5.5 (1M context) --- pom.xml | 45 ++++ .../musicalinstruments/InstrumentPlugin.java | 19 +- .../commands/InstrumentCommand.java | 13 +- .../items/ItemResolver.java | 9 +- .../listeners/InstrumentListener.java | 59 +---- .../managers/InstrumentManager.java | 27 +- .../util/LegacyModelData.java | 20 +- .../java/com/nexomc/nexo/api/NexoItems.java | 15 ++ .../dev/lone/itemsadder/api/CustomStack.java | 15 ++ .../java/net/Indyuce/mmoitems/MMOItems.java | 23 ++ .../InstrumentPluginTest.java | 126 ++++++++++ .../commands/InstrumentCommandTest.java | 220 +++++++++++++++++ .../items/ItemResolverTest.java | 233 ++++++++++++++++++ .../listeners/InstrumentListenerTest.java | 176 +++++++++++++ .../managers/InstrumentManagerTest.java | 53 +++- .../util/LegacyModelDataTest.java | 28 +++ 16 files changed, 979 insertions(+), 102 deletions(-) create mode 100644 src/test/java/com/nexomc/nexo/api/NexoItems.java create mode 100644 src/test/java/dev/lone/itemsadder/api/CustomStack.java create mode 100644 src/test/java/net/Indyuce/mmoitems/MMOItems.java create mode 100644 src/test/java/net/tfminecraft/musicalinstruments/InstrumentPluginTest.java create mode 100644 src/test/java/net/tfminecraft/musicalinstruments/commands/InstrumentCommandTest.java create mode 100644 src/test/java/net/tfminecraft/musicalinstruments/items/ItemResolverTest.java create mode 100644 src/test/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListenerTest.java create mode 100644 src/test/java/net/tfminecraft/musicalinstruments/util/LegacyModelDataTest.java diff --git a/pom.xml b/pom.xml index 9608990..955e016 100644 --- a/pom.xml +++ b/pom.xml @@ -81,6 +81,51 @@ maven-surefire-plugin 3.5.4 + + org.jacoco + jacoco-maven-plugin + 0.8.15 + + + prepare-agent + + prepare-agent + + + + report + + report + + + + + check + + check + + + + + BUNDLE + + + INSTRUCTION + COVEREDRATIO + 1.0 + + + BRANCH + COVEREDRATIO + 1.0 + + + + + + + + org.apache.maven.plugins maven-compiler-plugin diff --git a/src/main/java/net/tfminecraft/musicalinstruments/InstrumentPlugin.java b/src/main/java/net/tfminecraft/musicalinstruments/InstrumentPlugin.java index 3476f7f..84154a7 100644 --- a/src/main/java/net/tfminecraft/musicalinstruments/InstrumentPlugin.java +++ b/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 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; } } diff --git a/src/main/java/net/tfminecraft/musicalinstruments/commands/InstrumentCommand.java b/src/main/java/net/tfminecraft/musicalinstruments/commands/InstrumentCommand.java index 9cd4d2c..6329cab 100644 --- a/src/main/java/net/tfminecraft/musicalinstruments/commands/InstrumentCommand.java +++ b/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); } diff --git a/src/main/java/net/tfminecraft/musicalinstruments/items/ItemResolver.java b/src/main/java/net/tfminecraft/musicalinstruments/items/ItemResolver.java index 153c6d8..a2a55a9 100644 --- a/src/main/java/net/tfminecraft/musicalinstruments/items/ItemResolver.java +++ b/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; diff --git a/src/main/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListener.java b/src/main/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListener.java index 45d43ef..0bf3242 100644 --- a/src/main/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListener.java +++ b/src/main/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListener.java @@ -7,16 +7,10 @@ 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. @@ -25,20 +19,13 @@ public class InstrumentListener implements Listener { private final InstrumentPlugin plugin; private final InstrumentManager manager; - private final Map 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()); @@ -47,8 +34,6 @@ public void onPlayerHotbarChange(PlayerItemHeldEvent event) { return; } - startInstrumentDisplay(player, instrument); - // Get hotbar slot (1-9) int newSlot = event.getNewSlot() + 1; @@ -86,44 +71,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) + // 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(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); - } + event.setCancelled(true); } } diff --git a/src/main/java/net/tfminecraft/musicalinstruments/managers/InstrumentManager.java b/src/main/java/net/tfminecraft/musicalinstruments/managers/InstrumentManager.java index 674e1b4..361468d 100644 --- a/src/main/java/net/tfminecraft/musicalinstruments/managers/InstrumentManager.java +++ b/src/main/java/net/tfminecraft/musicalinstruments/managers/InstrumentManager.java @@ -51,6 +51,11 @@ 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); @@ -92,14 +97,13 @@ public String getInstrument(ItemStack item) { return match; } + // 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 +120,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 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); diff --git a/src/main/java/net/tfminecraft/musicalinstruments/util/LegacyModelData.java b/src/main/java/net/tfminecraft/musicalinstruments/util/LegacyModelData.java index e002dbb..ba21fc4 100644 --- a/src/main/java/net/tfminecraft/musicalinstruments/util/LegacyModelData.java +++ b/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 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()); diff --git a/src/test/java/com/nexomc/nexo/api/NexoItems.java b/src/test/java/com/nexomc/nexo/api/NexoItems.java new file mode 100644 index 0000000..b75d4ae --- /dev/null +++ b/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 lookup = id -> null; + + private NexoItems() {} + + public static Object itemFromId(String id) { + return lookup.apply(id); + } +} diff --git a/src/test/java/dev/lone/itemsadder/api/CustomStack.java b/src/test/java/dev/lone/itemsadder/api/CustomStack.java new file mode 100644 index 0000000..84e7dd3 --- /dev/null +++ b/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 lookup = id -> null; + + private CustomStack() {} + + public static Object getInstance(String id) { + return lookup.apply(id); + } +} diff --git a/src/test/java/net/Indyuce/mmoitems/MMOItems.java b/src/test/java/net/Indyuce/mmoitems/MMOItems.java new file mode 100644 index 0000000..ca7a73c --- /dev/null +++ b/src/test/java/net/Indyuce/mmoitems/MMOItems.java @@ -0,0 +1,23 @@ +package net.Indyuce.mmoitems; + +/** Stands in for the MMOItems entry point that ItemResolver reaches through reflection. */ +public final class MMOItems { + + public static MMOItems plugin; + + private final Object types; + private final Object items; + + public MMOItems(Object types, Object items) { + this.types = types; + this.items = items; + } + + public Object getTypes() { + return types; + } + + public Object getItems() { + return items; + } +} diff --git a/src/test/java/net/tfminecraft/musicalinstruments/InstrumentPluginTest.java b/src/test/java/net/tfminecraft/musicalinstruments/InstrumentPluginTest.java new file mode 100644 index 0000000..fad8f2e --- /dev/null +++ b/src/test/java/net/tfminecraft/musicalinstruments/InstrumentPluginTest.java @@ -0,0 +1,126 @@ +package net.tfminecraft.musicalinstruments; + +import org.bstats.bukkit.Metrics; +import org.bstats.charts.AdvancedPie; +import org.bstats.charts.CustomChart; +import org.bstats.charts.SimplePie; +import org.bstats.charts.SingleLineChart; +import org.bstats.json.JsonObjectBuilder; +import org.bukkit.Material; +import org.bukkit.SoundCategory; +import org.bukkit.event.player.PlayerItemHeldEvent; +import org.bukkit.inventory.ItemStack; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockbukkit.mockbukkit.MockBukkit; +import org.mockbukkit.mockbukkit.ServerMock; +import org.mockbukkit.mockbukkit.command.ConsoleCommandSenderMock; +import org.mockbukkit.mockbukkit.entity.PlayerMock; +import org.mockbukkit.mockbukkit.sound.AudioExperience; +import org.mockito.ArgumentCaptor; +import org.mockito.MockedConstruction; + +import java.util.ArrayList; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +class InstrumentPluginTest { + private ServerMock server; + private InstrumentPlugin plugin; + private List charts; + + @BeforeEach + void setUp() { + server = MockBukkit.mock(); + // bStats must not start its submission thread or reject the unrelocated test classes. + try (MockedConstruction metrics = mockConstruction(Metrics.class)) { + plugin = MockBukkit.load(InstrumentPlugin.class); + ArgumentCaptor captor = ArgumentCaptor.forClass(CustomChart.class); + verify(metrics.constructed().getFirst(), times(3)).addCustomChart(captor.capture()); + charts = captor.getAllValues(); + } + } + + @AfterEach + void tearDown() { + MockBukkit.unmock(); + } + + // Returns the chart's submission, or null when bStats would skip it. + private String submit(Class type) { + CustomChart chart = charts.stream().filter(type::isInstance).findFirst().orElseThrow(); + JsonObjectBuilder.JsonObject json = chart.getRequestJsonObject((message, error) -> fail(error), true); + return json == null ? null : json.toString(); + } + + // Runs /instruments list from the console and returns what it printed. + private List list() { + ConsoleCommandSenderMock console = (ConsoleCommandSenderMock) server.getConsoleSender(); + assertTrue(server.dispatchCommand(console, "instruments list")); + List messages = new ArrayList<>(); + for (String message = console.nextMessage(); message != null; message = console.nextMessage()) { + messages.add(message); + } + return messages; + } + + @Test + void savesTheBundledConfig() { + assertTrue(plugin.getDataFolder().toPath().resolve("config.yml").toFile().isFile()); + assertEquals("m.instruments.lute", plugin.getConfig().getString("lute.item")); + } + + @Test + void loadsInstrumentsOnTheFirstTick() { + // The bundled instruments need MMOItems, which is not installed, so point one at a vanilla item. + plugin.getConfig().set("lute.item", "v.note_block"); + assertEquals(List.of("§cNo instruments are loaded."), list()); + + server.getScheduler().performOneTick(); + + assertEquals(List.of("§aLoaded instruments (§61§a):", "§elute"), list()); + assertEquals("{\"chartId\":\"instruments_loaded\",\"data\":{\"value\":\"1\"}}", submit(SimplePie.class)); + } + + @Test + void playsBundledNotesAndCountsThem() { + plugin.getConfig().set("lute.item", "v.note_block"); + server.getScheduler().performOneTick(); + PlayerMock player = server.addPlayer(); + player.getInventory().setItemInOffHand(new ItemStack(Material.NOTE_BLOCK)); + + server.getPluginManager().callEvent(new PlayerItemHeldEvent(player, 8, 0)); + player.setSneaking(true); + server.getPluginManager().callEvent(new PlayerItemHeldEvent(player, 8, 0)); + + List sounds = player.getHeardSounds(); + assertEquals("instruments.lute_1c_single", sounds.get(0).getSound()); + assertEquals("instruments.lute_1c_chord", sounds.get(1).getSound()); + assertEquals(SoundCategory.RECORDS, sounds.get(0).getCategory()); + assertEquals(4.0f, sounds.get(0).getVolume()); + assertEquals("{\"chartId\":\"notes_played\",\"data\":{\"value\":2}}", submit(SingleLineChart.class)); + assertEquals("{\"chartId\":\"instrument_usage\",\"data\":{\"values\":{\"lute\":2}}}", submit(AdvancedPie.class)); + } + + @Test + void reportsPlaysPerSubmissionInterval() { + plugin.recordInstrumentPlay("lute"); + plugin.recordInstrumentPlay("lute"); + plugin.recordInstrumentPlay("flute"); + + assertEquals("{\"chartId\":\"notes_played\",\"data\":{\"value\":3}}", submit(SingleLineChart.class)); + String usage = submit(AdvancedPie.class); + assertTrue(usage.contains("\"lute\":2"), usage); + assertTrue(usage.contains("\"flute\":1"), usage); + + // Both charts drain their counters, so an idle interval submits nothing. + assertNull(submit(SingleLineChart.class)); + assertNull(submit(AdvancedPie.class)); + + plugin.recordInstrumentPlay("flute"); + assertEquals("{\"chartId\":\"instrument_usage\",\"data\":{\"values\":{\"flute\":1}}}", submit(AdvancedPie.class)); + } +} diff --git a/src/test/java/net/tfminecraft/musicalinstruments/commands/InstrumentCommandTest.java b/src/test/java/net/tfminecraft/musicalinstruments/commands/InstrumentCommandTest.java new file mode 100644 index 0000000..2f0bb11 --- /dev/null +++ b/src/test/java/net/tfminecraft/musicalinstruments/commands/InstrumentCommandTest.java @@ -0,0 +1,220 @@ +package net.tfminecraft.musicalinstruments.commands; + +import net.tfminecraft.musicalinstruments.InstrumentPlugin; +import net.tfminecraft.musicalinstruments.managers.InstrumentManager; +import org.bukkit.Material; +import org.bukkit.command.Command; +import org.bukkit.command.CommandSender; +import org.bukkit.entity.Item; +import org.bukkit.inventory.ItemStack; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockbukkit.mockbukkit.MockBukkit; +import org.mockbukkit.mockbukkit.ServerMock; +import org.mockbukkit.mockbukkit.command.ConsoleCommandSenderMock; +import org.mockbukkit.mockbukkit.entity.PlayerMock; + +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +class InstrumentCommandTest { + private static final String USAGE = "§cUsage: /instruments "; + private static final String NO_PERMISSION = "§cYou don't have permission to do that!"; + private static final String PLAYERS_ONLY = "§cOnly players can use this command!"; + + private InstrumentPlugin plugin; + private InstrumentManager manager; + private InstrumentCommand handler; + private Command command; + private PlayerMock player; + private PlayerMock operator; + private ConsoleCommandSenderMock console; + + @BeforeEach + void setUp() { + ServerMock server = MockBukkit.mock(); + plugin = mock(InstrumentPlugin.class); + manager = mock(InstrumentManager.class); + handler = new InstrumentCommand(plugin, manager); + command = mock(Command.class); + player = server.addPlayer(); + operator = server.addPlayer(); + operator.setOp(true); + console = (ConsoleCommandSenderMock) server.getConsoleSender(); + } + + @AfterEach + void tearDown() { + MockBukkit.unmock(); + } + + private void run(CommandSender sender, String... args) { + assertTrue(handler.onCommand(sender, command, "instruments", args)); + } + + private List complete(CommandSender sender, String... args) { + return handler.onTabComplete(sender, command, "instruments", args); + } + + private void loadInstruments(String... instruments) { + Set loaded = new LinkedHashSet<>(List.of(instruments)); + when(manager.getAllInstruments()).thenReturn(loaded); + } + + @Test + void showsUsageWithoutAKnownSubcommand() { + run(operator); + assertEquals(USAGE, operator.nextMessage()); + run(operator, "tune"); + assertEquals(USAGE, operator.nextMessage()); + assertNull(operator.nextMessage()); + } + + @Test + void everySubcommandChecksPermission() { + for (String subcommand : List.of("keybinds", "list", "give", "reload")) { + run(player, subcommand); + assertEquals(NO_PERMISSION, player.nextMessage()); + } + assertNull(player.nextMessage()); + verifyNoInteractions(plugin, manager); + } + + @Test + void playerOnlySubcommandsRejectTheConsole() { + run(console, "keybinds"); + assertEquals(PLAYERS_ONLY, console.nextMessage()); + run(console, "give", "lute"); + assertEquals(PLAYERS_ONLY, console.nextMessage()); + assertNull(console.nextMessage()); + verifyNoInteractions(manager); + } + + @Test + void keybindsRequireAnOffHandInstrument() { + run(operator, "keybinds"); + assertEquals("§cYou must be holding an instrument in your off-hand!", operator.nextMessage()); + assertNull(operator.nextMessage()); + } + + @Test + void keybindsSendEachConfiguredLine() { + ItemStack lute = new ItemStack(Material.PAPER); + operator.getInventory().setItemInOffHand(lute); + when(manager.getInstrument(lute)).thenReturn("lute"); + when(manager.getKeybindMessage("lute")).thenReturn("§aUse keys 1-8\n§e1-[C] 2-[D]\n\n"); + + run(operator, "KEYBINDS"); + + assertEquals("§aUse keys 1-8", operator.nextMessage()); + assertEquals("§e1-[C] 2-[D]", operator.nextMessage()); + assertNull(operator.nextMessage()); + } + + @Test + void keybindsFallBackWhenNoMessageIsConfigured() { + when(manager.getInstrument(any())).thenReturn("lute"); + + run(operator, "keybinds"); + + assertEquals("§aYour instrument keybinds were not defined in the config.", operator.nextMessage()); + assertNull(operator.nextMessage()); + } + + @Test + void listsLoadedInstruments() { + loadInstruments(); + run(console, "list"); + assertEquals("§cNo instruments are loaded.", console.nextMessage()); + + loadInstruments("lute", "flute"); + run(console, "list"); + assertEquals("§aLoaded instruments (§62§a):", console.nextMessage()); + assertEquals("§elute§7, §eflute", console.nextMessage()); + assertNull(console.nextMessage()); + } + + @Test + void giveRequiresAnInstrumentName() { + run(operator, "give"); + assertEquals("§cUsage: /instruments give ", operator.nextMessage()); + assertNull(operator.nextMessage()); + } + + @Test + void giveRejectsUnknownInstruments() { + run(operator, "give", "Harp"); + assertEquals("§cUnknown instrument: §eHarp", operator.nextMessage()); + assertNull(operator.nextMessage()); + verify(manager, never()).getInstrumentItem(any()); + } + + @Test + void giveAddsTheInstrumentToTheInventory() { + ItemStack lyre = new ItemStack(Material.PAPER); + when(manager.findInstrument("lyre")).thenReturn("Lyre"); + when(manager.getInstrumentItem("Lyre")).thenReturn(lyre); + + run(operator, "give", "lyre"); + + assertTrue(operator.getInventory().containsAtLeast(lyre, 1)); + assertTrue(operator.getWorld().getEntitiesByClass(Item.class).isEmpty()); + assertEquals("§aYou received: §eLyre", operator.nextMessage()); + assertNull(operator.nextMessage()); + } + + @Test + void giveDropsTheInstrumentWhenTheInventoryIsFull() { + ItemStack lute = new ItemStack(Material.PAPER); + when(manager.findInstrument("lute")).thenReturn("lute"); + when(manager.getInstrumentItem("lute")).thenReturn(lute); + // MockBukkit also fills armour and off-hand slots when adding items. + for (int slot = 0; slot < operator.getInventory().getSize(); slot++) { + operator.getInventory().setItem(slot, new ItemStack(Material.DIRT, 64)); + } + + run(operator, "give", "lute"); + + Item dropped = operator.getWorld().getEntitiesByClass(Item.class).iterator().next(); + assertTrue(dropped.getItemStack().isSimilar(lute)); + assertEquals("§aYou received: §elute", operator.nextMessage()); + } + + @Test + void reloadReloadsConfigAndTemplates() { + loadInstruments("lute"); + + run(console, "reload"); + + var order = inOrder(plugin, manager); + order.verify(plugin).reloadConfig(); + order.verify(manager).loadTemplates(); + assertEquals("§aConfig reloaded. §e1 §ainstrument(s) loaded.", console.nextMessage()); + assertNull(console.nextMessage()); + } + + @Test + void completesPermittedSubcommands() { + assertEquals(List.of(), complete(player, "")); + assertEquals(List.of("keybinds", "list", "give", "reload"), complete(operator, "")); + assertEquals(List.of("reload"), complete(operator, "R")); + assertEquals(List.of(), complete(operator, "x")); + } + + @Test + void completesInstrumentNamesForGive() { + // Suggestions keep the config's case; give accepts them through findInstrument. + loadInstruments("lute", "flute", "Lyre"); + + assertEquals(List.of("lute", "Lyre"), complete(operator, "GIVE", "l")); + assertEquals(List.of(), complete(player, "give", "")); + assertEquals(List.of(), complete(operator, "list", "")); + assertEquals(List.of(), complete(operator, "give", "lute", "")); + } +} diff --git a/src/test/java/net/tfminecraft/musicalinstruments/items/ItemResolverTest.java b/src/test/java/net/tfminecraft/musicalinstruments/items/ItemResolverTest.java new file mode 100644 index 0000000..c9c4957 --- /dev/null +++ b/src/test/java/net/tfminecraft/musicalinstruments/items/ItemResolverTest.java @@ -0,0 +1,233 @@ +package net.tfminecraft.musicalinstruments.items; + +import com.nexomc.nexo.api.NexoItems; +import dev.lone.itemsadder.api.CustomStack; +import net.Indyuce.mmoitems.MMOItems; +import net.tfminecraft.musicalinstruments.util.LegacyModelData; +import org.bukkit.Material; +import org.bukkit.inventory.ItemStack; +import org.bukkit.inventory.meta.ItemMeta; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockbukkit.mockbukkit.MockBukkit; +import org.mockbukkit.mockbukkit.ServerMock; +import org.mockbukkit.mockbukkit.plugin.PluginMock; +import org.mockito.MockedStatic; + +import java.util.Map; +import java.util.logging.Logger; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +// The modeled(...) format keeps legacy display names. +@SuppressWarnings("deprecation") +class ItemResolverTest { + + // Reflection targets shaped like the provider APIs. + public record Types(Map byId) { + public Object get(String id) { return byId.get(id); } + } + + public record Items(Map byId) { + public Object getMMOItem(Object type, String id) { return byId.get(type + ":" + id); } + } + + public record Template(Object builder) { + public Object newBuilder() { return builder; } + } + + public record Builder(Object built) { + public Object build() { return built; } + } + + public record Stack(Object item) { + public Object getItemStack() { return item; } + } + + // Has a build method, but not the no-argument one the resolver calls. + public record AmountBuilder() { + public Object build(int amount) { return new ItemStack(Material.STICK, amount); } + } + + private ServerMock server; + private Logger logger; + private ItemResolver resolver; + + @BeforeEach + void setUp() { + server = MockBukkit.mock(); + logger = mock(Logger.class); + resolver = new ItemResolver(logger); + } + + @AfterEach + void tearDown() { + MMOItems.plugin = null; + CustomStack.lookup = id -> null; + NexoItems.lookup = id -> null; + MockBukkit.unmock(); + } + + private void assertUnresolved(String path, String warning) { + assertNull(resolver.resolve(path)); + verify(logger).warning(contains(warning)); + } + + @Test + void ignoresMissingPaths() { + assertNull(resolver.resolve(null)); + assertNull(resolver.resolve("")); + assertNull(resolver.resolve(" ")); + verifyNoInteractions(logger); + } + + @Test + void rejectsUnknownPrefixes() { + assertUnresolved("x.stone", "Unknown item path prefix in 'x.stone'"); + } + + @Test + void resolvesVanillaItems() { + ItemStack item = resolver.resolve(" V.iron_ingot "); + assertEquals(Material.IRON_INGOT, item.getType()); + assertEquals(1, item.getAmount()); + assertEquals(Material.STICK, resolver.resolve("v.minecraft:stick").getType()); + assertEquals(Material.STICK, resolver.resolve("v.MINECRAFT:STICK").getType()); + } + + @Test + void rejectsMalformedVanillaItems() { + assertUnresolved("v", "Malformed vanilla item path 'v'"); + assertUnresolved("v.not_a_material", "Unknown material 'not_a_material'"); + } + + @Test + void resolvesModeledItems() { + try (MockedStatic modelData = mockStatic(LegacyModelData.class)) { + ItemStack item = resolver.resolve("MODELED(type=minecraft:PAPER; name = &6Flute ;model=1001;ignored)"); + assertEquals(Material.PAPER, item.getType()); + assertEquals("§6Flute", item.getItemMeta().getDisplayName()); + modelData.verify(() -> LegacyModelData.set(any(ItemMeta.class), eq(1001))); + } + verifyNoInteractions(logger); + } + + @Test + void modeledItemsDefaultToUnnamedDirt() { + try (MockedStatic modelData = mockStatic(LegacyModelData.class)) { + ItemStack item = resolver.resolve("modeled()"); + assertEquals(Material.DIRT, item.getType()); + assertFalse(item.getItemMeta().hasDisplayName()); + modelData.verifyNoInteractions(); + } + } + + @Test + void modeledItemsKeepNameWhenModelIsInvalid() { + ItemStack item = resolver.resolve("modeled(type=stick;name=Reed;model=abc)"); + assertEquals(Material.STICK, item.getType()); + assertEquals("Reed", item.getItemMeta().getDisplayName()); + verify(logger).warning(contains("Invalid model data")); + } + + @Test + void modeledItemsWithoutMetaSkipAttributes() { + ItemStack item = resolver.resolve("modeled(type=air;name=Nothing)"); + assertEquals(Material.AIR, item.getType()); + assertNull(item.getItemMeta()); + } + + @Test + void rejectsMalformedModeledItems() { + assertUnresolved("modeled(type=paper", "Malformed modeled item path"); + assertUnresolved("modeled(type=not_a_material)", "Invalid material type in modeled item"); + } + + @Test + void providerItemsRequireAnInstalledEnabledPlugin() { + assertUnresolved("m.instruments.lute", "requires MMOItems"); + assertUnresolved("ia.tfmc:lute", "requires ItemsAdder"); + assertUnresolved("nx.lute", "requires Nexo"); + + PluginMock mmoItems = MockBukkit.createMockPlugin("MMOItems"); + server.getPluginManager().disablePlugin(mmoItems); + assertNull(resolver.resolve("m.instruments.lute")); + verify(logger, times(2)).warning(contains("requires MMOItems")); + } + + @Test + void rejectsMalformedProviderPaths() { + assertUnresolved("m.instruments", "Malformed MMOItems path"); + assertUnresolved("ia", "Malformed ItemsAdder path"); + assertUnresolved("nx", "Malformed Nexo path"); + } + + @Test + void resolvesMmoItems() { + MockBukkit.createMockPlugin("MMOItems"); + ItemStack lute = new ItemStack(Material.PAPER); + MMOItems.plugin = new MMOItems( + new Types(Map.of("INSTRUMENTS", "instrument-type")), + new Items(Map.of( + "instrument-type:LUTE", new Template(new Builder(lute)), + "instrument-type:UNBUILT", new Template(null), + "instrument-type:TEXT", new Template(new Builder("not an item")), + "instrument-type:AMOUNT", new Template(new AmountBuilder()), + "instrument-type:PLAIN", "no builder method"))); + + assertSame(lute, resolver.resolve("m.instruments.lute")); + assertUnresolved("m.drums.snare", "Unknown MMOItems type 'drums'"); + assertUnresolved("m.instruments.harp", "Unknown MMOItems item"); + assertUnresolved("m.instruments.unbuilt", "MMOItems returned no item for 'm.instruments.unbuilt'"); + assertUnresolved("m.instruments.text", "MMOItems returned no item for 'm.instruments.text'"); + assertUnresolved("m.instruments.amount", "Failed to read MMOItems item 'm.instruments.amount'"); + assertUnresolved("m.instruments.plain", "Failed to read MMOItems item 'm.instruments.plain'"); + } + + @Test + void reportsMmoItemsRuntimeFailures() { + MockBukkit.createMockPlugin("MMOItems"); + assertUnresolved("m.instruments.lute", "Failed to read MMOItems item"); + } + + @Test + void resolvesItemsAdderItemsAsSingleItems() { + MockBukkit.createMockPlugin("ItemsAdder"); + ItemStack lute = new ItemStack(Material.PAPER, 5); + CustomStack.lookup = id -> switch (id) { + case "tfmc:lute" -> new Stack(lute); + case "tfmc:text" -> new Stack("not an item"); + case "tfmc:broken" -> throw new IllegalStateException("registry reloading"); + default -> null; + }; + + ItemStack item = resolver.resolve("ia.tfmc:lute"); + assertSame(lute, item); + assertEquals(1, item.getAmount()); + assertUnresolved("ia.tfmc:harp", "Unknown ItemsAdder item"); + assertUnresolved("ia.tfmc:text", "ItemsAdder returned no item"); + assertUnresolved("ia.tfmc:broken", "Failed to read ItemsAdder item"); + } + + @Test + void resolvesNexoItemsAsSingleItems() { + MockBukkit.createMockPlugin("Nexo"); + ItemStack lute = new ItemStack(Material.PAPER, 3); + NexoItems.lookup = id -> switch (id) { + case "lute" -> new Builder(lute); + case "text" -> new Builder("not an item"); + case "amount" -> new AmountBuilder(); + default -> null; + }; + + ItemStack item = resolver.resolve("nx.lute"); + assertSame(lute, item); + assertEquals(1, item.getAmount()); + assertUnresolved("nx.harp", "Unknown Nexo item"); + assertUnresolved("nx.text", "Nexo returned no item"); + assertUnresolved("nx.amount", "Failed to read Nexo item"); + } +} diff --git a/src/test/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListenerTest.java b/src/test/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListenerTest.java new file mode 100644 index 0000000..2c0c23d --- /dev/null +++ b/src/test/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListenerTest.java @@ -0,0 +1,176 @@ +package net.tfminecraft.musicalinstruments.listeners; + +import net.tfminecraft.musicalinstruments.InstrumentPlugin; +import net.tfminecraft.musicalinstruments.events.InstrumentPlayEvent; +import net.tfminecraft.musicalinstruments.managers.InstrumentManager; +import org.bukkit.Location; +import org.bukkit.Particle; +import org.bukkit.SoundCategory; +import org.bukkit.event.EventHandler; +import org.bukkit.event.Listener; +import org.bukkit.event.player.PlayerItemHeldEvent; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockbukkit.mockbukkit.MockBukkit; +import org.mockbukkit.mockbukkit.ServerMock; +import org.mockbukkit.mockbukkit.entity.PlayerMock; +import org.mockbukkit.mockbukkit.sound.AudioExperience; +import org.mockbukkit.mockbukkit.util.SpawnedParticle; +import org.mockbukkit.mockbukkit.world.WorldMock; + +import java.util.ArrayList; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +class InstrumentListenerTest { + private ServerMock server; + private InstrumentPlugin plugin; + private InstrumentManager manager; + private PlayerMock player; + private final List played = new ArrayList<>(); + + // Listens the way other plugins (such as ActivityTF) consume notes. + public class PlayListener implements Listener { + @EventHandler + public void onInstrumentPlay(InstrumentPlayEvent event) { + played.add(event); + } + } + + @BeforeEach + void setUp() { + server = MockBukkit.mock(); + plugin = mock(InstrumentPlugin.class); + manager = mock(InstrumentManager.class); + server.getPluginManager().registerEvents(new InstrumentListener(plugin, manager), MockBukkit.createMockPlugin()); + server.getPluginManager().registerEvents(new PlayListener(), MockBukkit.createMockPlugin("ActivityTF")); + player = server.addPlayer(); + // Start on slot 5, which no test presses. + player.getInventory().setHeldItemSlot(4); + } + + @AfterEach + void tearDown() { + MockBukkit.unmock(); + } + + private void holdLute() { + when(manager.getInstrument(any())).thenReturn("lute"); + } + + // Presses a hotbar key the way Paper 1.21.10 handles it (ServerGamePacketListenerImpl.handleSetCarriedItem): + // pressing the selected slot fires nothing, and an uncancelled event then selects the pressed slot. + private PlayerItemHeldEvent pressSlot(int slot) { + int selected = player.getInventory().getHeldItemSlot(); + if (slot - 1 == selected) { + return null; + } + PlayerItemHeldEvent event = new PlayerItemHeldEvent(player, selected, slot - 1); + server.getPluginManager().callEvent(event); + if (!event.isCancelled()) { + player.getInventory().setHeldItemSlot(slot - 1); + } + return event; + } + + @Test + void ignoresSlotChangesWithoutAnInstrument() { + PlayerItemHeldEvent event = pressSlot(1); + + verify(manager).getInstrument(any()); + verifyNoMoreInteractions(manager); + assertFalse(event.isCancelled()); + assertEquals(0, player.getInventory().getHeldItemSlot()); + assertTrue(player.getHeardSounds().isEmpty()); + assertTrue(played.isEmpty()); + verifyNoInteractions(plugin); + } + + @Test + void playsTheNoteForTheSelectedSlot() { + holdLute(); + when(manager.getSoundKey("lute", 2, false)).thenReturn("instruments.lute_2d_single"); + when(manager.getVolume("lute")).thenReturn(4.0); + when(manager.getPitch("lute")).thenReturn(0.5); + Location location = player.getLocation(); + + PlayerItemHeldEvent event = pressSlot(2); + + AudioExperience sound = player.getHeardSounds().getFirst(); + assertEquals("instruments.lute_2d_single", sound.getSound()); + assertEquals(SoundCategory.RECORDS, sound.getCategory()); + assertEquals(location, sound.getLocation()); + assertEquals(4.0f, sound.getVolume()); + assertEquals(0.5f, sound.getPitch()); + + SpawnedParticle particle = ((WorldMock) player.getWorld()).getSpawnedParticles().getFirst(); + assertEquals(Particle.NOTE, particle.particle()); + assertEquals(location.getY() + 2.0, particle.y()); + assertEquals(1, particle.count()); + + verify(plugin).recordInstrumentPlay("lute"); + InstrumentPlayEvent note = played.getFirst(); + assertSame(player, note.getPlayer()); + assertEquals("lute", note.getInstrument()); + assertEquals("instruments.lute_2d_single", note.getSoundKey()); + + // Cancelling keeps the server on slot 9 too, instead of applying the pressed slot afterwards. + assertTrue(event.isCancelled()); + assertEquals(8, player.getInventory().getHeldItemSlot()); + } + + @Test + void repeatsTheSameNote() { + holdLute(); + when(manager.getSoundKey("lute", 1, false)).thenReturn("instruments.lute_1c_single"); + + pressSlot(1); + pressSlot(1); + pressSlot(1); + + assertEquals(3, player.getHeardSounds().size()); + assertEquals(3, played.size()); + verify(plugin, times(3)).recordInstrumentPlay("lute"); + } + + @Test + void sneakingSelectsTheAlternateNote() { + holdLute(); + player.setSneaking(true); + when(manager.getSoundKey("lute", 3, true)).thenReturn("instruments.lute_3e_chord"); + + pressSlot(3); + + assertEquals("instruments.lute_3e_chord", player.getHeardSounds().getFirst().getSound()); + } + + @Test + void unmappedSlotsChangeSlotNormally() { + holdLute(); + + PlayerItemHeldEvent event = pressSlot(9); + + verify(manager).getSoundKey("lute", 9, false); + assertFalse(event.isCancelled()); + assertEquals(8, player.getInventory().getHeldItemSlot()); + assertTrue(player.getHeardSounds().isEmpty()); + verifyNoInteractions(plugin); + } + + @Test + void ignoresSlotChangesCancelledByOtherPlugins() { + holdLute(); + when(manager.getSoundKey(any(), anyInt(), anyBoolean())).thenReturn("instruments.lute_1c_single"); + PlayerItemHeldEvent event = new PlayerItemHeldEvent(player, 0, 1); + event.setCancelled(true); + + server.getPluginManager().callEvent(event); + + verifyNoInteractions(manager, plugin); + assertTrue(player.getHeardSounds().isEmpty()); + } +} diff --git a/src/test/java/net/tfminecraft/musicalinstruments/managers/InstrumentManagerTest.java b/src/test/java/net/tfminecraft/musicalinstruments/managers/InstrumentManagerTest.java index 2759b81..a137857 100644 --- a/src/test/java/net/tfminecraft/musicalinstruments/managers/InstrumentManagerTest.java +++ b/src/test/java/net/tfminecraft/musicalinstruments/managers/InstrumentManagerTest.java @@ -26,6 +26,7 @@ class InstrumentManagerTest { private final YamlConfiguration config = new YamlConfiguration(); private InstrumentManager manager; private ItemResolver resolver; + private Logger logger; private ItemStack lute; @BeforeEach @@ -33,7 +34,8 @@ void setUp() { MockBukkit.mock(); InstrumentPlugin plugin = mock(InstrumentPlugin.class); when(plugin.getConfig()).thenReturn(config); - when(plugin.getLogger()).thenReturn(Logger.getLogger("InstrumentManagerTest")); + logger = mock(Logger.class); + when(plugin.getLogger()).thenReturn(logger); resolver = mock(ItemResolver.class); manager = new InstrumentManager(plugin, resolver); lute = new ItemStack(Material.PAPER); @@ -126,4 +128,53 @@ void reloadRemovesOldTemplates() { assertEquals("flute", manager.getInstrument(new ItemStack(Material.STICK))); assertNull(manager.getInstrumentItem("lute")); } + + @Test + void skipsInstrumentsThatCannotBeLoaded() { + config.set("drum.keybind-message", "no item"); + config.set("harp.item", "m.instruments.harp"); + config.set("horn.item", "nx.horn"); + when(resolver.resolve("nx.horn")).thenThrow(new IllegalStateException("registry reloading")); + config.set("rest.item", "v.air"); + when(resolver.resolve("v.air")).thenReturn(new ItemStack(Material.AIR)); + manager.loadTemplates(); + assertEquals(List.of("lute"), List.copyOf(manager.getAllInstruments())); + verify(logger).warning("Instrument 'drum' has no 'item' defined in config."); + verify(logger).warning("Could not resolve item 'm.instruments.harp' for instrument 'harp'."); + verify(logger).warning("Failed to load instrument 'horn': registry reloading"); + verify(logger).warning("Item 'v.air' for instrument 'rest' is air."); + assertThrows(UnsupportedOperationException.class, () -> manager.getAllInstruments().clear()); + } + + @Test + void findsInstrumentsIgnoringCase() { + config.set("Lyre.item", "v.STICK"); + config.set("LUTE.item", "v.STICK"); + when(resolver.resolve("v.STICK")).thenReturn(new ItemStack(Material.STICK)); + manager.loadTemplates(); + assertEquals("Lyre", manager.findInstrument("lyre")); + assertEquals("Lyre", manager.findInstrument("LYRE")); + // An exact match beats an earlier case-insensitive one. + assertEquals("LUTE", manager.findInstrument("LUTE")); + assertEquals("lute", manager.findInstrument("Lute")); + assertNull(manager.findInstrument("harp")); + } + + @Test + void readsNoteSettingsFromConfig() { + config.set("lute.keybind-message", "1-[C]"); + config.set("lute.hotbar-sounds.1", "instruments.lute_1c_single"); + config.set("lute.hotbar-sounds.1+sneak", "instruments.lute_1c_chord"); + config.set("lute.hotbar-sounds.volume", 4.0); + config.set("lute.hotbar-sounds.pitch", 0.5); + assertEquals("1-[C]", manager.getKeybindMessage("lute")); + assertEquals("instruments.lute_1c_single", manager.getSoundKey("lute", 1, false)); + assertEquals("instruments.lute_1c_chord", manager.getSoundKey("lute", 1, true)); + assertNull(manager.getSoundKey("lute", 2, false)); + assertEquals(4.0, manager.getVolume("lute")); + assertEquals(0.5, manager.getPitch("lute")); + assertEquals(1.0, manager.getVolume("flute")); + assertEquals(1.0, manager.getPitch("flute")); + assertNull(manager.getKeybindMessage("flute")); + } } diff --git a/src/test/java/net/tfminecraft/musicalinstruments/util/LegacyModelDataTest.java b/src/test/java/net/tfminecraft/musicalinstruments/util/LegacyModelDataTest.java new file mode 100644 index 0000000..3a9cd73 --- /dev/null +++ b/src/test/java/net/tfminecraft/musicalinstruments/util/LegacyModelDataTest.java @@ -0,0 +1,28 @@ +package net.tfminecraft.musicalinstruments.util; + +import org.bukkit.inventory.meta.ItemMeta; +import org.bukkit.inventory.meta.components.CustomModelDataComponent; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.mockito.Mockito.*; + +// MockBukkit 4.95 does not implement model data components, so they are mocked here. +class LegacyModelDataTest { + + @Test + void settingAModelReplacesTheWholeComponent() { + ItemMeta meta = mock(ItemMeta.class); + CustomModelDataComponent component = mock(CustomModelDataComponent.class); + when(meta.getCustomModelDataComponent()).thenReturn(component); + + LegacyModelData.set(meta, 1002); + + verify(component).setFloats(List.of(1002.0f)); + verify(component).setFlags(List.of()); + verify(component).setStrings(List.of()); + verify(component).setColors(List.of()); + verify(meta).setCustomModelDataComponent(component); + } +} From 711ceabe60f366fca397466245096ec4bf5cbc32 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Sat, 26 Sep 2026 09:49:45 +0000 Subject: [PATCH 2/2] fix: ignore notes mapped to the reset slot Playing a note returns the player to slot 9, and pressing the selected slot sends no event, so a note mapped to slot 9 could not be played. Ignore slot 9 in the listener and warn at load when a config maps it. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../listeners/InstrumentListener.java | 8 ++++++-- .../managers/InstrumentManager.java | 11 +++++++++++ .../listeners/InstrumentListenerTest.java | 17 +++++++++++++++-- .../managers/InstrumentManagerTest.java | 14 ++++++++++++++ 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/main/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListener.java b/src/main/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListener.java index 0bf3242..28fbbd3 100644 --- a/src/main/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListener.java +++ b/src/main/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListener.java @@ -17,6 +17,9 @@ // ==================================== 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; @@ -30,7 +33,8 @@ 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; } @@ -74,7 +78,7 @@ public void onPlayerHotbarChange(PlayerItemHeldEvent event) { // 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(8); + player.getInventory().setHeldItemSlot(RESET_SLOT); event.setCancelled(true); } } diff --git a/src/main/java/net/tfminecraft/musicalinstruments/managers/InstrumentManager.java b/src/main/java/net/tfminecraft/musicalinstruments/managers/InstrumentManager.java index 361468d..bbc4317 100644 --- a/src/main/java/net/tfminecraft/musicalinstruments/managers/InstrumentManager.java +++ b/src/main/java/net/tfminecraft/musicalinstruments/managers/InstrumentManager.java @@ -60,6 +60,7 @@ public void loadTemplates() { 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()); } @@ -97,6 +98,16 @@ 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(); diff --git a/src/test/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListenerTest.java b/src/test/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListenerTest.java index 2c0c23d..121b764 100644 --- a/src/test/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListenerTest.java +++ b/src/test/java/net/tfminecraft/musicalinstruments/listeners/InstrumentListenerTest.java @@ -152,13 +152,26 @@ void sneakingSelectsTheAlternateNote() { void unmappedSlotsChangeSlotNormally() { holdLute(); + PlayerItemHeldEvent event = pressSlot(7); + + verify(manager).getSoundKey("lute", 7, false); + assertFalse(event.isCancelled()); + assertEquals(6, player.getInventory().getHeldItemSlot()); + assertTrue(player.getHeardSounds().isEmpty()); + verifyNoInteractions(plugin); + } + + @Test + void theResetSlotNeverPlaysANote() { + holdLute(); + when(manager.getSoundKey(any(), anyInt(), anyBoolean())).thenReturn("instruments.lute_9c_single"); + PlayerItemHeldEvent event = pressSlot(9); - verify(manager).getSoundKey("lute", 9, false); + verify(manager, never()).getSoundKey(any(), anyInt(), anyBoolean()); assertFalse(event.isCancelled()); assertEquals(8, player.getInventory().getHeldItemSlot()); assertTrue(player.getHeardSounds().isEmpty()); - verifyNoInteractions(plugin); } @Test diff --git a/src/test/java/net/tfminecraft/musicalinstruments/managers/InstrumentManagerTest.java b/src/test/java/net/tfminecraft/musicalinstruments/managers/InstrumentManagerTest.java index a137857..0274409 100644 --- a/src/test/java/net/tfminecraft/musicalinstruments/managers/InstrumentManagerTest.java +++ b/src/test/java/net/tfminecraft/musicalinstruments/managers/InstrumentManagerTest.java @@ -146,6 +146,20 @@ void skipsInstrumentsThatCannotBeLoaded() { assertThrows(UnsupportedOperationException.class, () -> manager.getAllInstruments().clear()); } + @Test + void warnsAboutNotesOnTheResetSlot() { + config.set("lute.hotbar-sounds.8", "instruments.lute_8c_single"); + config.set("flute.item", "v.STICK"); + config.set("flute.hotbar-sounds.9", "instruments.flute_9c_single"); + config.set("flute.hotbar-sounds.9+sneak", "instruments.flute_18c_single"); + when(resolver.resolve("v.STICK")).thenReturn(new ItemStack(Material.STICK)); + manager.loadTemplates(); + verify(logger).warning(contains("'flute' maps hotbar-sounds.9,")); + verify(logger).warning(contains("'flute' maps hotbar-sounds.9+sneak,")); + verify(logger, never()).warning(contains("'lute' maps")); + assertEquals(List.of("lute", "flute"), List.copyOf(manager.getAllInstruments())); + } + @Test void findsInstrumentsIgnoringCase() { config.set("Lyre.item", "v.STICK");