Skip to content

fix: keep the seal when a right-click uses a block instead of reading #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
XxFran10xX merged 1 commit into from
Oct 20, 2026
+90 −33
Merged

fix: keep the seal when a right-click uses a block instead of reading #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 23 additions & 19 deletions src/main/java/net/tfminecraft/birdmessenger/letters/LetterListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
});
Expand Down
81 changes: 67 additions & 14 deletions src/test/java/net/tfminecraft/birdmessenger/letters/LetterListenerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -189,7 +191,7 @@
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);
Expand All @@ -214,7 +216,7 @@
@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);
Expand All @@ -235,10 +237,49 @@
}
}

@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<String, Object> before = new HashMap<>(Map.of(
"meta-type", "BOOK_SIGNED", "pages", List.of("original text"),
Expand All @@ -256,7 +297,6 @@
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);
}
Expand All @@ -267,6 +307,8 @@
})) {
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());
Expand All @@ -291,7 +333,7 @@
}

@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);
Expand Down Expand Up @@ -367,7 +409,7 @@
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 {
Expand Down Expand Up @@ -400,7 +442,7 @@
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 {
Expand Down Expand Up @@ -432,24 +474,35 @@
PlayerEditBookEvent event = mock(PlayerEditBookEvent.class);
when(event.getPlayer()).thenReturn(player);
// Paper's packet path uses inventory slot 40, but its event factory emits -1 for offhand.
when(event.getSlot()).thenReturn(slot == 40 ? -1 : slot);

Check warning on line 477 in src/test/java/net/tfminecraft/birdmessenger/letters/LetterListenerTest.java

View workflow job for this annotation

GitHub Actions / build 

getSlot() in org.bukkit.event.player.PlayerEditBookEvent has been deprecated and marked for removal
when(event.isSigning()).thenReturn(signing);
when(event.getNewBookMeta()).thenReturn(edited);
when(inventory.getItem(slot)).thenReturn(original);
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);
Expand Down
Loading