From 9aa8fa51292cd455f900ec75124b05c3806f26d1 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Sun, 27 Sep 2026 17:15:38 +0000 Subject: [PATCH 1/4] fix: open the mail picker before skin lookups finish The picker waited for RPCharacters to fetch every uncached character skin from ProvinceSystem, one request at a time, before it opened. On Main this can take minutes. Players who retried in the meantime lost their first letter, and each extra refresh later opened a picker over the current one, which cancelled the session so the next click (such as Next) closed the menu. Open the picker straight away with the cached skins and redraw the heads when the refresh finishes. Only open a picker while a letter is waiting and none is already showing. Return any waiting letter before a new one replaces it, and make the delayed close after placing a letter close only the letter GUI. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../birdmessenger/BirdMessenger.java | 39 ++++- .../birdmessenger/gui/CharacterPickerGui.java | 7 +- .../birdmessenger/listener/GuiListener.java | 9 +- .../birdmessenger/PickerOpenTest.java | 156 ++++++++++++++++++ 4 files changed, 201 insertions(+), 10 deletions(-) create mode 100644 src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java diff --git a/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java b/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java index a171867..3ebf1b1 100644 --- a/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java +++ b/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java @@ -15,6 +15,7 @@ import net.tfminecraft.birdmessenger.mail.MailService; import net.tfminecraft.birdmessenger.mail.MailStore; import net.tfminecraft.birdmessenger.session.SelectedTarget; +import net.tfminecraft.birdmessenger.session.SendSession; import net.tfminecraft.birdmessenger.session.SendSessionManager; import net.tfminecraft.birdmessenger.letters.LetterFeature; @@ -107,18 +108,18 @@ public void openPicker(Player player) { if (player == null || !player.isOnline()) { return; } + SendSession session = sessions.get(player.getUniqueId()); + if (session == null || session.getLetter() == null || session.isConfirmed()) { + return; + } + if (player.getOpenInventory().getTopInventory().getHolder() instanceof CharacterPickerGui) { + return; + } if (!Bukkit.getPluginManager().isPluginEnabled("RPCharacters")) { sessions.returnLetter(player, false); player.sendMessage(config.msgRpcMissing()); return; } - RPCharacters.refreshMailTargetTexturesAsync(() -> openPickerAfterTextures(player)); - } - - private void openPickerAfterTextures(Player player) { - if (player == null || !player.isOnline()) { - return; - } List targets = CharacterPickerGui.loadTargets(); if (targets.isEmpty()) { sessions.returnLetter(player, false); @@ -131,7 +132,29 @@ private void openPickerAfterTextures(Player player) { player.sendMessage(config.msgCannotSendToSelf()); return; } + CharacterPickerGui picker = new CharacterPickerGui(this, targets); player.sendMessage(config.msgPickerSelect()); - player.openInventory(new CharacterPickerGui(this, targets).getInventory()); + player.openInventory(picker.getInventory()); + // Skin lookups can take minutes, so fill in missing heads after the picker is open. + RPCharacters.refreshMailTargetTexturesAsync(() -> refreshPickerHeads(player, picker)); + } + + private void refreshPickerHeads(Player player, CharacterPickerGui picker) { + if (!player.isOnline() + || player.getOpenInventory().getTopInventory().getHolder() != picker) { + return; + } + SendSession session = sessions.get(player.getUniqueId()); + if (session == null) { + return; + } + List targets = CharacterPickerGui.loadTargets(); + targets.removeIf(t -> player.getUniqueId().equals(t.getOwnerUuid())); + if (targets.isEmpty()) { + return; + } + picker.setTargets(targets); + session.setPickerPage(Math.min(session.getPickerPage(), picker.maxPage())); + CharacterPickerGui.applyPage(session, picker); } } diff --git a/src/main/java/net/tfminecraft/birdmessenger/gui/CharacterPickerGui.java b/src/main/java/net/tfminecraft/birdmessenger/gui/CharacterPickerGui.java index 96450c4..bba48d9 100644 --- a/src/main/java/net/tfminecraft/birdmessenger/gui/CharacterPickerGui.java +++ b/src/main/java/net/tfminecraft/birdmessenger/gui/CharacterPickerGui.java @@ -31,7 +31,7 @@ public final class CharacterPickerGui implements InventoryHolder { public static final int SLOT_NEXT = 53; private final BirdMessenger plugin; - private final List targets; + private List targets; private final Inventory inventory; // Keep the existing legacy text representation, formatting, and exact-string comparisons. @@ -47,6 +47,11 @@ public List targets() { return targets; } + /** Swap in a reloaded target list, for example once missing skins arrive. */ + public void setTargets(List targets) { + this.targets = targets; + } + // Keep the existing legacy text representation, formatting, and exact-string comparisons. @SuppressWarnings("deprecation") public void render(int page, SelectedTarget selected) { diff --git a/src/main/java/net/tfminecraft/birdmessenger/listener/GuiListener.java b/src/main/java/net/tfminecraft/birdmessenger/listener/GuiListener.java index fe971fa..c04551c 100644 --- a/src/main/java/net/tfminecraft/birdmessenger/listener/GuiListener.java +++ b/src/main/java/net/tfminecraft/birdmessenger/listener/GuiListener.java @@ -82,6 +82,8 @@ public void onClose(InventoryCloseEvent event) { return; } event.getInventory().setItem(LetterGui.LETTER_SLOT, null); + // A second letter must not overwrite one that is still waiting for a recipient. + plugin.sessions().returnLetter(player, false); SendSession session = plugin.sessions().getOrCreate(player.getUniqueId()); session.setLetter(placed.clone()); session.setSelected(null); @@ -114,7 +116,12 @@ private void handleLetterClick(InventoryClickEvent event, Player player) { return; } if (LetterItems.isLetter(plugin.config(), cursor)) { - Bukkit.getScheduler().runTaskLater(plugin, () -> player.closeInventory(), 3L); + Bukkit.getScheduler().runTaskLater(plugin, () -> { + // The player may already have closed this GUI and moved on to the picker. + if (player.getOpenInventory().getTopInventory().getHolder() instanceof LetterGui) { + player.closeInventory(); + } + }, 3L); } return; } diff --git a/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java b/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java new file mode 100644 index 0000000..c0072d9 --- /dev/null +++ b/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java @@ -0,0 +1,156 @@ +package net.tfminecraft.birdmessenger; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.List; +import java.util.UUID; + +import org.bukkit.Bukkit; +import org.bukkit.configuration.file.YamlConfiguration; +import org.bukkit.entity.Player; +import org.bukkit.inventory.Inventory; +import org.bukkit.inventory.InventoryHolder; +import org.bukkit.inventory.InventoryView; +import org.bukkit.inventory.ItemStack; +import org.bukkit.plugin.PluginManager; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.MockedConstruction; +import org.mockito.MockedStatic; + +import net.tfminecraft.birdmessenger.gui.CharacterPickerGui; +import net.tfminecraft.birdmessenger.session.SendSession; +import net.tfminecraft.birdmessenger.session.SendSessionManager; +import net.tfminecraft.rpcharacters.RPCharacters; +import net.tfminecraft.rpcharacters.api.CharacterSkull; +import net.tfminecraft.rpcharacters.mail.CharacterMailTarget; + +class PickerOpenTest { + private final UUID self = UUID.randomUUID(); + private final UUID other = UUID.randomUUID(); + private final List refreshCallbacks = new ArrayList<>(); + private final List directory = new ArrayList<>(); + private BirdMessenger plugin; + private SendSessionManager sessions; + private Player player; + private InventoryHolder openHolder; + private int opens; + + @BeforeEach void setup() throws Exception { + refreshCallbacks.clear(); + directory.clear(); + opens = 0; + openHolder = null; + plugin = mock(BirdMessenger.class, CALLS_REAL_METHODS); + doReturn(new YamlConfiguration()).when(plugin).getConfig(); + sessions = new SendSessionManager(plugin); + setField("config", new BirdConfig(plugin)); + setField("sessions", sessions); + player = mock(Player.class); + when(player.getUniqueId()).thenReturn(self); + when(player.isOnline()).thenReturn(true); + InventoryView view = mock(InventoryView.class); + Inventory top = mock(Inventory.class); + when(player.getOpenInventory()).thenReturn(view); + when(view.getTopInventory()).thenReturn(top); + when(top.getHolder()).thenAnswer(call -> openHolder); + when(player.openInventory(any(Inventory.class))).thenAnswer(call -> { + opens++; + openHolder = ((Inventory) call.getArgument(0)).getHolder(); + return view; + }); + } + + @Test void pickerOpensBeforeSkinRefreshFinishesAndUpdatesAfterwards() { + directory.add(target(other, "a", "Alice")); + pendingLetter(); + try (var mocks = new Mocks()) { + plugin.openPicker(player); + assertEquals(1, opens, "picker must not wait for skin lookups"); + CharacterPickerGui picker = assertInstanceOf(CharacterPickerGui.class, openHolder); + assertEquals(1, picker.targets().size()); + assertEquals(1, refreshCallbacks.size()); + + directory.add(target(other, "b", "Bob")); + refreshCallbacks.getFirst().run(); + assertEquals(1, opens, "refresh re-renders the open picker instead of reopening it"); + assertEquals(2, picker.targets().size()); + } + } + + @Test void refreshIgnoresPickerThePlayerHasClosed() { + directory.add(target(other, "a", "Alice")); + pendingLetter(); + try (var mocks = new Mocks()) { + plugin.openPicker(player); + CharacterPickerGui picker = (CharacterPickerGui) openHolder; + openHolder = null; + directory.add(target(other, "b", "Bob")); + refreshCallbacks.getFirst().run(); + assertEquals(1, picker.targets().size()); + } + } + + @Test void doesNotOpenWithoutPendingLetterOrOverAnOpenPicker() { + directory.add(target(other, "a", "Alice")); + try (var mocks = new Mocks()) { + plugin.openPicker(player); + assertEquals(0, opens); + + pendingLetter(); + plugin.openPicker(player); + plugin.openPicker(player); + assertEquals(1, opens); + } + } + + private void pendingLetter() { + SendSession session = sessions.getOrCreate(self); + session.setLetter(mock(ItemStack.class)); + } + + private CharacterMailTarget target(UUID owner, String id, String name) { + return new CharacterMailTarget(owner, id, name, name, null, null, null, null); + } + + private void setField(String name, Object value) throws Exception { + Field field = BirdMessenger.class.getDeclaredField(name); + field.setAccessible(true); + field.set(plugin, value); + } + + private final class Mocks implements AutoCloseable { + private final MockedStatic bukkit = mockStatic(Bukkit.class); + private final MockedStatic rpc = mockStatic(RPCharacters.class); + private final MockedStatic skulls = mockStatic(CharacterSkull.class); + private final MockedConstruction items = mockConstruction(ItemStack.class); + + Mocks() { + PluginManager manager = mock(PluginManager.class); + when(manager.isPluginEnabled("RPCharacters")).thenReturn(true); + bukkit.when(Bukkit::getPluginManager).thenReturn(manager); + bukkit.when(() -> Bukkit.createInventory(any(InventoryHolder.class), anyInt(), anyString())) + .thenAnswer(call -> { + Inventory inventory = mock(Inventory.class); + InventoryHolder holder = call.getArgument(0); + when(inventory.getHolder()).thenReturn(holder); + return inventory; + }); + rpc.when(RPCharacters::listMailTargets).thenAnswer(call -> new ArrayList<>(directory)); + rpc.when(() -> RPCharacters.refreshMailTargetTexturesAsync(any())) + .thenAnswer(call -> refreshCallbacks.add(call.getArgument(0))); + skulls.when(() -> CharacterSkull.fromTextures(any(), any())) + .thenAnswer(call -> mock(ItemStack.class)); + } + + @Override public void close() { + items.close(); + skulls.close(); + rpc.close(); + bukkit.close(); + } + } +} From 1d958807e69fec0b89c3048384013167d2788346 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Sun, 27 Sep 2026 18:08:00 +0000 Subject: [PATCH 2/4] fix: close only the letter GUI that scheduled the close Compare the open inventory with the one that scheduled the delayed close, so a new letter GUI opened in the meantime stays open. Drop a picker selection whose recipient disappears when the heads refresh. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../birdmessenger/BirdMessenger.java | 10 ++++++++++ .../birdmessenger/listener/GuiListener.java | 4 ++-- .../birdmessenger/PickerOpenTest.java | 20 +++++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java b/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java index 3ebf1b1..8515869 100644 --- a/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java +++ b/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java @@ -20,6 +20,7 @@ import net.tfminecraft.birdmessenger.letters.LetterFeature; import java.util.List; +import java.util.Objects; public final class BirdMessenger extends JavaPlugin { @@ -154,6 +155,15 @@ private void refreshPickerHeads(Player player, CharacterPickerGui picker) { return; } picker.setTargets(targets); + SelectedTarget selected = session.getSelected(); + if (selected != null) { + // Drop a selection whose recipient is no longer listed. + session.setSelected(targets.stream() + .filter(t -> selected.getCharacterId().equals(t.getCharacterId()) + && Objects.equals(selected.getOwnerUuid(), t.getOwnerUuid())) + .findFirst() + .orElse(null)); + } session.setPickerPage(Math.min(session.getPickerPage(), picker.maxPage())); CharacterPickerGui.applyPage(session, picker); } diff --git a/src/main/java/net/tfminecraft/birdmessenger/listener/GuiListener.java b/src/main/java/net/tfminecraft/birdmessenger/listener/GuiListener.java index c04551c..f23e3a0 100644 --- a/src/main/java/net/tfminecraft/birdmessenger/listener/GuiListener.java +++ b/src/main/java/net/tfminecraft/birdmessenger/listener/GuiListener.java @@ -117,8 +117,8 @@ private void handleLetterClick(InventoryClickEvent event, Player player) { } if (LetterItems.isLetter(plugin.config(), cursor)) { Bukkit.getScheduler().runTaskLater(plugin, () -> { - // The player may already have closed this GUI and moved on to the picker. - if (player.getOpenInventory().getTopInventory().getHolder() instanceof LetterGui) { + // The player may already have closed this GUI and moved on to another one. + if (player.getOpenInventory().getTopInventory() == top) { player.closeInventory(); } }, 3L); diff --git a/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java b/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java index c0072d9..a1e7daf 100644 --- a/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java +++ b/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java @@ -81,6 +81,26 @@ class PickerOpenTest { } } + @Test void refreshDropsASelectedRecipientWhoIsNoLongerListed() { + CharacterMailTarget alice = target(other, "a", "Alice"); + CharacterMailTarget bob = target(other, "b", "Bob"); + directory.add(alice); + directory.add(bob); + pendingLetter(); + try (var mocks = new Mocks()) { + plugin.openPicker(player); + CharacterPickerGui picker = (CharacterPickerGui) openHolder; + SendSession session = sessions.get(self); + session.setSelected(picker.targets().get(1)); + refreshCallbacks.getFirst().run(); + assertEquals("b", session.getSelected().getCharacterId(), "a listed selection is kept"); + + directory.remove(bob); + refreshCallbacks.getFirst().run(); + assertNull(session.getSelected()); + } + } + @Test void refreshIgnoresPickerThePlayerHasClosed() { directory.add(target(other, "a", "Alice")); pendingLetter(); From 9848cfb2aba3ebdd6996c4f84fe8056523456e96 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Sun, 27 Sep 2026 18:14:55 +0000 Subject: [PATCH 3/4] fix: redraw the picker when every recipient disappears Co-Authored-By: Claude Opus 5.5 (1M context) --- .../java/net/tfminecraft/birdmessenger/BirdMessenger.java | 3 --- .../java/net/tfminecraft/birdmessenger/PickerOpenTest.java | 6 ++++++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java b/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java index 8515869..a90feb8 100644 --- a/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java +++ b/src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java @@ -151,9 +151,6 @@ private void refreshPickerHeads(Player player, CharacterPickerGui picker) { } List targets = CharacterPickerGui.loadTargets(); targets.removeIf(t -> player.getUniqueId().equals(t.getOwnerUuid())); - if (targets.isEmpty()) { - return; - } picker.setTargets(targets); SelectedTarget selected = session.getSelected(); if (selected != null) { diff --git a/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java b/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java index a1e7daf..b649bd3 100644 --- a/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java +++ b/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java @@ -98,6 +98,12 @@ class PickerOpenTest { directory.remove(bob); refreshCallbacks.getFirst().run(); assertNull(session.getSelected()); + + session.setSelected(picker.targets().get(0)); + directory.clear(); + refreshCallbacks.getFirst().run(); + assertTrue(picker.targets().isEmpty(), "an emptied list is redrawn, not left stale"); + assertNull(session.getSelected()); } } From c401ffff9d7d2cf4c006f6ae16fda98b3f646501 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Sun, 27 Sep 2026 18:21:15 +0000 Subject: [PATCH 4/4] test: check an emptied picker is redrawn Co-Authored-By: Claude Opus 5.5 (1M context) --- .../java/net/tfminecraft/birdmessenger/PickerOpenTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java b/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java index b649bd3..af7fe86 100644 --- a/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java +++ b/src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java @@ -101,8 +101,12 @@ class PickerOpenTest { session.setSelected(picker.targets().get(0)); directory.clear(); + clearInvocations(picker.getInventory()); refreshCallbacks.getFirst().run(); assertTrue(picker.targets().isEmpty(), "an emptied list is redrawn, not left stale"); + verify(picker.getInventory()).clear(); + verify(picker.getInventory()).setItem(eq(CharacterPickerGui.SLOT_CANCEL), any()); + verify(picker.getInventory()).setItem(eq(CharacterPickerGui.SLOT_CONFIRM), any()); assertNull(session.getSelected()); } }