From 6f6634858380de7488fa7419746937c65e46f02c Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Tue, 20 Oct 2026 10:19:47 +0200 Subject: [PATCH] fix: keep the seal when a right-click uses a block instead of reading Right-clicking a barrel, door, lever or other usable block while holding a sealed letter broke the seal even though the book never opened. Break it only when vanilla counts the written book as used, which happens right after the reading screen opens and on no other click. Co-Authored-By: Claude Opus 5.5 --- .../birdmessenger/letters/LetterListener.java | 42 +++++----- .../letters/LetterListenerTest.java | 81 +++++++++++++++---- 2 files changed, 90 insertions(+), 33 deletions(-) diff --git a/src/main/java/net/tfminecraft/birdmessenger/letters/LetterListener.java b/src/main/java/net/tfminecraft/birdmessenger/letters/LetterListener.java index fc6196c..cd7445a 100644 --- a/src/main/java/net/tfminecraft/birdmessenger/letters/LetterListener.java +++ b/src/main/java/net/tfminecraft/birdmessenger/letters/LetterListener.java @@ -4,15 +4,16 @@ import org.bukkit.Bukkit; import org.bukkit.Material; -import org.bukkit.block.Block; +import org.bukkit.Statistic; import org.bukkit.entity.Player; import org.bukkit.event.EventHandler; +import org.bukkit.event.EventPriority; import org.bukkit.event.Listener; -import org.bukkit.event.block.Action; import org.bukkit.event.player.PlayerEditBookEvent; -import org.bukkit.event.player.PlayerInteractEvent; +import org.bukkit.event.player.PlayerStatisticIncrementEvent; import org.bukkit.inventory.EquipmentSlot; import org.bukkit.inventory.ItemStack; +import org.bukkit.inventory.PlayerInventory; import org.bukkit.inventory.meta.BookMeta; import net.tfminecraft.birdmessenger.BirdMessenger; @@ -78,23 +79,26 @@ public void onBookSign(PlayerEditBookEvent event) { }); } - @EventHandler - public void onBookOpen(PlayerInteractEvent event) { - Action action = event.getAction(); - if (action != Action.RIGHT_CLICK_AIR && action != Action.RIGHT_CLICK_BLOCK) return; - Block clicked = event.getClickedBlock(); - if (clicked != null && (clicked.getType() == Material.CHISELED_BOOKSHELF || clicked.getType() == Material.LECTERN)) return; - // No guard on which hand was used - off-hand letters open too - but the swap below - // must go back into that same hand, or the other hand's item gets overwritten. - EquipmentSlot hand = event.getHand(); - if (hand != EquipmentSlot.HAND && hand != EquipmentSlot.OFF_HAND) return; - ItemStack item = event.getItem(); + // Not ignoreCancelled: a cancelled count still means the book opened. + @EventHandler(priority = EventPriority.MONITOR) + public void onBookOpen(PlayerStatisticIncrementEvent event) { + // Vanilla counts a written book as used right after it opens the reading screen, + // and nowhere else. Clicks that open a barrel, use a door, place the book on a + // lectern or are denied by another plugin never reach this, so the seal holds. + if (event.getStatistic() != Statistic.USE_ITEM || event.getMaterial() != Material.WRITTEN_BOOK) return; + Player player = event.getPlayer(); + PlayerInventory inventory = player.getInventory(); + // The client only tries the off hand when the main hand item does nothing, and a + // written book always opens, so a main-hand written book is the one being read. + // The swap below must go back into that same hand, or the other hand's item gets overwritten. + ItemStack mainHand = inventory.getItemInMainHand(); + EquipmentSlot hand = mainHand.getType() == Material.WRITTEN_BOOK ? EquipmentSlot.HAND : EquipmentSlot.OFF_HAND; + ItemStack item = hand == EquipmentSlot.HAND ? mainHand : inventory.getItemInOffHand(); if (!items.isSealedLetter(item)) return; ItemStack original = item.clone(); BookMeta current = (BookMeta) item.getItemMeta(); if (current == null) return; - Player player = event.getPlayer(); CharacterSession session = Characters.currentSession(player).orElse(null); if (session == null && Characters.available()) return; ItemStack opened = items.createOpenedLetter(current); @@ -103,16 +107,16 @@ public void onBookOpen(PlayerInteractEvent event) { sendMessage(player, LetterConfig.openFailedMessage); return; } - // Not cancelled - the book still opens for reading, so swap the item on the next tick. + // The book is already open for reading, so swap the item on the next tick. Bukkit.getScheduler().runTask(plugin, () -> { if (!isCurrent(player, session)) return; ItemStack held = hand == EquipmentSlot.OFF_HAND - ? player.getInventory().getItemInOffHand() : player.getInventory().getItemInMainHand(); + ? inventory.getItemInOffHand() : inventory.getItemInMainHand(); if (!unchangedOnOpen(original, held)) return; if (hand == EquipmentSlot.OFF_HAND) { - player.getInventory().setItemInOffHand(opened); + inventory.setItemInOffHand(opened); } else { - player.getInventory().setItemInMainHand(opened); + inventory.setItemInMainHand(opened); } sendMessage(player, LetterConfig.openedMessage); }); diff --git a/src/test/java/net/tfminecraft/birdmessenger/letters/LetterListenerTest.java b/src/test/java/net/tfminecraft/birdmessenger/letters/LetterListenerTest.java index b154873..005e8c5 100644 --- a/src/test/java/net/tfminecraft/birdmessenger/letters/LetterListenerTest.java +++ b/src/test/java/net/tfminecraft/birdmessenger/letters/LetterListenerTest.java @@ -13,10 +13,12 @@ import org.bukkit.Bukkit; import org.bukkit.Material; +import org.bukkit.Statistic; import org.bukkit.entity.Player; -import org.bukkit.event.block.Action; +import org.bukkit.event.EventHandler; +import org.bukkit.event.EventPriority; import org.bukkit.event.player.PlayerEditBookEvent; -import org.bukkit.event.player.PlayerInteractEvent; +import org.bukkit.event.player.PlayerStatisticIncrementEvent; import org.bukkit.inventory.EquipmentSlot; import org.bukkit.inventory.ItemStack; import org.bukkit.inventory.PlayerInventory; @@ -189,7 +191,7 @@ class LetterListenerTest { for (EquipmentSlot hand : List.of(EquipmentSlot.HAND, EquipmentSlot.OFF_HAND)) { for (String change : List.of("replaced", "mutated", "amount", "offline")) { setup(); - PlayerInteractEvent event = openEvent(hand); + PlayerStatisticIncrementEvent event = openEvent(hand); try (var bukkit = mockBukkit(); var factories = mockConstruction(LetterItems.class, (items, context) -> { when(items.isSealedLetter(original)).thenReturn(true); @@ -214,7 +216,7 @@ class LetterListenerTest { @Test void openingStillReplacesOnlyTheRecordedHand() { for (EquipmentSlot hand : List.of(EquipmentSlot.HAND, EquipmentSlot.OFF_HAND)) { setup(); - PlayerInteractEvent event = openEvent(hand); + PlayerStatisticIncrementEvent event = openEvent(hand); try (var bukkit = mockBukkit(); var factories = mockConstruction(LetterItems.class, (items, context) -> { when(items.isSealedLetter(original)).thenReturn(true); @@ -235,10 +237,49 @@ class LetterListenerTest { } } + @Test void onlyAnOpenedWrittenBookBreaksTheSeal() { + // A click that opens a barrel, uses a door or fills a lectern never counts the book as used. + for (var counted : List.of( + Map.entry(Statistic.DROP, Material.WRITTEN_BOOK), + Map.entry(Statistic.PICKUP, Material.WRITTEN_BOOK), + Map.entry(Statistic.USE_ITEM, Material.WRITABLE_BOOK), + Map.entry(Statistic.USE_ITEM, Material.BARREL))) { + setup(); + PlayerStatisticIncrementEvent event = statisticEvent(counted.getKey(), counted.getValue(), EquipmentSlot.HAND); + try (var bukkit = mockBukkit(); var factories = successfulFactories()) { + new LetterListener(plugin).onBookOpen(event); + verifyNoInteractions(factories.constructed().getFirst()); + assertTrue(pending.isEmpty()); + verifyNoWrites(); + } + } + } + + @Test void aMainHandWrittenBookIsTheOneReadNotAnOffHandLetter() { + PlayerStatisticIncrementEvent event = openEvent(EquipmentSlot.OFF_HAND); + ItemStack book = stackOf(Material.WRITTEN_BOOK); + held(EquipmentSlot.HAND, book); + try (var bukkit = mockBukkit(); var factories = successfulFactories()) { + new LetterListener(plugin).onBookOpen(event); + LetterItems factory = factories.constructed().getFirst(); + verify(factory).isSealedLetter(book); + verify(factory, never()).isSealedLetter(original); + assertTrue(pending.isEmpty()); + verifyNoWrites(); + } + } + + @Test void openingListensLastAndAlsoToCancelledCounts() throws NoSuchMethodException { + EventHandler handler = LetterListener.class + .getMethod("onBookOpen", PlayerStatisticIncrementEvent.class).getAnnotation(EventHandler.class); + assertEquals(EventPriority.MONITOR, handler.priority()); + assertFalse(handler.ignoreCancelled()); + } + @Test void openingAllowsOnlyTheVanillaResolvedFlagToChange() { for (String change : List.of("resolved", "pages", "pdc", "name", "type", "amount", "already-resolved")) { setup(); - PlayerInteractEvent event = openEvent(EquipmentSlot.HAND); + PlayerStatisticIncrementEvent event = openEvent(EquipmentSlot.HAND); BookMeta beforeMeta = mock(BookMeta.class); Map before = new HashMap<>(Map.of( "meta-type", "BOOK_SIGNED", "pages", List.of("original text"), @@ -256,7 +297,6 @@ class LetterListenerTest { case "pages" -> after.put("pages", List.of("other letter")); case "pdc" -> after.put("PublicBukkitValues", Map.of("tfmccore:sealed_letter", 2)); case "name" -> after.put("display-name", "Another name"); - case "type" -> when(original.getType()).thenReturn(Material.WRITABLE_BOOK); case "amount" -> when(original.getAmount()).thenReturn(2); case "already-resolved" -> before.put("resolved", true); } @@ -267,6 +307,8 @@ class LetterListenerTest { })) { bukkit.when(Bukkit::getScheduler).thenReturn(scheduler); new LetterListener(plugin).onBookOpen(event); + // The held type decides which hand was read, so it may only change after the open. + if (change.equals("type")) when(original.getType()).thenReturn(Material.WRITABLE_BOOK); pending.getLast().run(); if (change.equals("resolved")) verify(inventory).setItemInMainHand(replacement); else verify(inventory, never()).setItemInMainHand(any()); @@ -291,7 +333,7 @@ class LetterListenerTest { } @Test void failedOpeningKeepsTheOriginalSealAndReportsTheFailure() { - PlayerInteractEvent event = openEvent(EquipmentSlot.OFF_HAND); + PlayerStatisticIncrementEvent event = openEvent(EquipmentSlot.OFF_HAND); try (var bukkit = mockBukkit(); var factories = mockConstruction(LetterItems.class, (items, context) -> { when(items.isSealedLetter(original)).thenReturn(true); @@ -367,7 +409,7 @@ void pendingCharactersCannotRewriteLettersAreToldWhyAndMayStillReadThem(String o try (var bukkit = mockBukkit(); var factories = successfulFactories()) { LetterListener listener = new LetterListener(plugin); if (operation.equals("open")) { - PlayerInteractEvent event = openEvent(EquipmentSlot.HAND); + PlayerStatisticIncrementEvent event = openEvent(EquipmentSlot.HAND); listener.onBookOpen(event); verify(event, never()).setCancelled(anyBoolean()); } else { @@ -400,7 +442,7 @@ private org.mockito.MockedConstruction successfulFactories() { private void schedule(String operation) { LetterListener listener = new LetterListener(plugin); if (operation.equals("open")) { - PlayerInteractEvent event = openEvent(EquipmentSlot.HAND); + PlayerStatisticIncrementEvent event = openEvent(EquipmentSlot.HAND); listener.onBookOpen(event); verify(event, never()).setCancelled(anyBoolean()); } else { @@ -439,17 +481,28 @@ private PlayerEditBookEvent editEvent(int slot, boolean signing) { return event; } - private PlayerInteractEvent openEvent(EquipmentSlot hand) { - PlayerInteractEvent event = mock(PlayerInteractEvent.class); - when(event.getAction()).thenReturn(Action.RIGHT_CLICK_AIR); + private PlayerStatisticIncrementEvent openEvent(EquipmentSlot hand) { + return statisticEvent(Statistic.USE_ITEM, Material.WRITTEN_BOOK, hand); + } + + private PlayerStatisticIncrementEvent statisticEvent(Statistic statistic, Material material, EquipmentSlot hand) { + PlayerStatisticIncrementEvent event = mock(PlayerStatisticIncrementEvent.class); + when(event.getStatistic()).thenReturn(statistic); + when(event.getMaterial()).thenReturn(material); when(event.getPlayer()).thenReturn(player); - when(event.getHand()).thenReturn(hand); - when(event.getItem()).thenReturn(original); + when(original.getType()).thenReturn(Material.WRITTEN_BOOK); when(original.getItemMeta()).thenReturn(edited); held(hand, original); + if (hand == EquipmentSlot.OFF_HAND) held(EquipmentSlot.HAND, stackOf(Material.AIR)); return event; } + private static ItemStack stackOf(Material type) { + ItemStack stack = mock(ItemStack.class); + when(stack.getType()).thenReturn(type); + return stack; + } + private void held(EquipmentSlot hand, ItemStack stack) { if (hand == EquipmentSlot.HAND) when(inventory.getItemInMainHand()).thenReturn(stack); else when(inventory.getItemInOffHand()).thenReturn(stack);