fix: open the mail picker before skin lookups finish

src/main/java/net/tfminecraft/birdmessenger/BirdMessenger.java

@@ -15,10 +15,12 @@
 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;
 
 import java.util.List;
+import java.util.Objects;
 
 public final class BirdMessenger extends JavaPlugin {
 
@@ -107,18 +109,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<SelectedTarget> targets = CharacterPickerGui.loadTargets();
 		if (targets.isEmpty()) {
 			sessions.returnLetter(player, false);
@@ -131,7 +133,35 @@ 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<SelectedTarget> targets = CharacterPickerGui.loadTargets();
+		targets.removeIf(t -> player.getUniqueId().equals(t.getOwnerUuid()));
+		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);
 	}
 }

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<SelectedTarget> targets;
+	private List<SelectedTarget> targets;
 	private final Inventory inventory;
 
 	// Keep the existing legacy text representation, formatting, and exact-string comparisons.
@@ -47,6 +47,11 @@ public List<SelectedTarget> targets() {
 		return targets;
 	}
 
+	/** Swap in a reloaded target list, for example once missing skins arrive. */
+	public void setTargets(List<SelectedTarget> targets) {
+		this.targets = targets;
+	}
+
 	// Keep the existing legacy text representation, formatting, and exact-string comparisons.
 	@SuppressWarnings("deprecation")
 	public void render(int page, SelectedTarget selected) {

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 another one.
+					if (player.getOpenInventory().getTopInventory() == top) {
+						player.closeInventory();
+					}
+				}, 3L);
 			}
 			return;
 		}

src/test/java/net/tfminecraft/birdmessenger/PickerOpenTest.java

@@ -0,0 +1,186 @@
+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<Runnable> refreshCallbacks = new ArrayList<>();
+    private final List<CharacterMailTarget> 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 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());
+
+            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());
+        }
+    }
+
+    @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> bukkit = mockStatic(Bukkit.class);
+        private final MockedStatic<RPCharacters> rpc = mockStatic(RPCharacters.class);
+        private final MockedStatic<CharacterSkull> skulls = mockStatic(CharacterSkull.class);
+        private final MockedConstruction<ItemStack> 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();
+        }
+    }
+}