From d80af21b470ecf2d6b4192d65ca89b4db67bffc3 Mon Sep 17 00:00:00 2001 From: Daniel Walsh Date: Sat, 13 Jan 2024 04:38:06 +0000 Subject: [PATCH] Fixes #4087 When a block is broken we don't want to allow users to still use the inventory. This allows for them to pull items out along with blocks being dropped (if the block implements that). This fix is in 2 parts, the main part is just preventing the inventory from being opened if the block is queued for deletion. The second smaller part is just closing the inventory for all viewers on break. We would do this during cleanup but that was a tick later, we want it done in this tick and to prevent opening before the cleaning up is ran. --- .../listeners/BlockListener.java | 9 ++ .../SlimefunItemInteractListener.java | 7 + .../interfaces/InventoryBlock.java | 6 +- .../TestSlimefunItemInteractListener.java | 137 ++++++++++++++++++ .../slimefun4/test/TestUtilities.java | 32 ++++ 5 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 src/test/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/TestSlimefunItemInteractListener.java diff --git a/src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BlockListener.java b/src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BlockListener.java index 5fdc720076..1d7a75df25 100644 --- a/src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BlockListener.java +++ b/src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BlockListener.java @@ -18,6 +18,7 @@ import org.bukkit.block.BlockState; import org.bukkit.block.data.BlockData; import org.bukkit.enchantments.Enchantment; +import org.bukkit.entity.HumanEntity; import org.bukkit.entity.Player; import org.bukkit.event.EventHandler; import org.bukkit.event.EventPriority; @@ -220,6 +221,14 @@ private void callBlockHandler(BlockBreakEvent e, ItemStack item, List } drops.addAll(sfItem.getDrops()); + // Partial fix for #4087 - We don't want the inventory to be usable post break, close it for anyone still inside + // The main fix is in SlimefunItemInteractListener preventing opening to begin with + // Close the inventory for all viewers of this block + // TODO(future): Remove this check when MockBukkit supports viewers + if (!Slimefun.instance().isUnitTest()) { + BlockStorage.getInventory(e.getBlock()).toInventory().getViewers().forEach(HumanEntity::closeInventory); + } + // Remove the block data BlockStorage.clearBlockInfo(e.getBlock()); } } diff --git a/src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/SlimefunItemInteractListener.java b/src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/SlimefunItemInteractListener.java index a9931b2cc6..195f6ac0c5 100644 --- a/src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/SlimefunItemInteractListener.java +++ b/src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/SlimefunItemInteractListener.java @@ -57,6 +57,13 @@ public void onRightClick(PlayerInteractEvent e) { return; } + // Fixes #4087 - Prevents players from interacting with a block that is about to be deleted + // We especially don't want to open inventories as that can cause duplication + if (Slimefun.getTickerTask().isDeletedSoon(e.getClickedBlock().getLocation())) { + e.setCancelled(true); + return; + } + // Fire our custom Event PlayerRightClickEvent event = new PlayerRightClickEvent(e); Bukkit.getPluginManager().callEvent(event); diff --git a/src/main/java/me/mrCookieSlime/Slimefun/Objects/SlimefunItem/interfaces/InventoryBlock.java b/src/main/java/me/mrCookieSlime/Slimefun/Objects/SlimefunItem/interfaces/InventoryBlock.java index 199b60caeb..9c368d67ad 100644 --- a/src/main/java/me/mrCookieSlime/Slimefun/Objects/SlimefunItem/interfaces/InventoryBlock.java +++ b/src/main/java/me/mrCookieSlime/Slimefun/Objects/SlimefunItem/interfaces/InventoryBlock.java @@ -64,7 +64,11 @@ public boolean canOpen(Block b, Player p) { if (p.hasPermission("slimefun.inventory.bypass")) { return true; } else { - return item.canUse(p, false) && Slimefun.getProtectionManager().hasPermission(p, b.getLocation(), Interaction.INTERACT_BLOCK); + return item.canUse(p, false) && ( + // Protection manager doesn't exist in unit tests + Slimefun.instance().isUnitTest() + || Slimefun.getProtectionManager().hasPermission(p, b.getLocation(), Interaction.INTERACT_BLOCK) + ); } } }; diff --git a/src/test/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/TestSlimefunItemInteractListener.java b/src/test/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/TestSlimefunItemInteractListener.java new file mode 100644 index 0000000000..e403243393 --- /dev/null +++ b/src/test/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/TestSlimefunItemInteractListener.java @@ -0,0 +1,137 @@ +package io.github.thebusybiscuit.slimefun4.implementation.listeners; + +import io.github.thebusybiscuit.slimefun4.api.events.SlimefunBlockBreakEvent; +import org.bukkit.World; +import org.bukkit.block.Block; +import org.bukkit.block.BlockFace; +import org.bukkit.entity.Player; +import org.bukkit.event.Event.Result; +import org.bukkit.event.block.Action; +import org.bukkit.event.block.BlockBreakEvent; +import org.bukkit.event.player.PlayerInteractEvent; +import org.bukkit.inventory.EquipmentSlot; +import org.bukkit.inventory.ItemStack; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import io.github.thebusybiscuit.slimefun4.api.events.PlayerRightClickEvent; +import io.github.thebusybiscuit.slimefun4.api.items.SlimefunItem; +import io.github.thebusybiscuit.slimefun4.api.recipes.RecipeType; +import io.github.thebusybiscuit.slimefun4.implementation.Slimefun; +import io.github.thebusybiscuit.slimefun4.implementation.SlimefunItems; +import io.github.thebusybiscuit.slimefun4.implementation.items.electric.machines.ElectricFurnace; +import io.github.thebusybiscuit.slimefun4.test.TestUtilities; +import me.mrCookieSlime.Slimefun.api.BlockStorage; +import me.mrCookieSlime.Slimefun.api.inventory.BlockMenuPreset; +import be.seeseemelk.mockbukkit.MockBukkit; +import be.seeseemelk.mockbukkit.ServerMock; + +class TestSlimefunItemInteractListener { + + private static ServerMock server; + private static Slimefun plugin; + private static SlimefunItem slimefunItem; + + @BeforeAll + public static void load() { + server = MockBukkit.mock(); + plugin = MockBukkit.load(Slimefun.class); + + // Register block listener (for place + break) and our interact listener for inventory handling + new BlockListener(plugin); + new SlimefunItemInteractListener(plugin); + + // Enable tickers so the electric furnace can be registered + Slimefun.getCfg().setValue("URID.enable-tickers", true); + + slimefunItem = new ElectricFurnace( + TestUtilities.getItemGroup(plugin, "test"), SlimefunItems.ELECTRIC_FURNACE, RecipeType.NULL, new ItemStack[]{} + ) + .setCapacity(100) + .setEnergyConsumption(10) + .setProcessingSpeed(1); + slimefunItem.register(plugin); + } + + @AfterAll + public static void unload() { + MockBukkit.unmock(); + } + + @AfterEach + public void afterEach() { + server.getPluginManager().clearEvents(); + } + + // Test for dupe bug - issue #4087 + @Test + void testCannotOpenInvOfBrokenBlock() { + // Place down an electric furnace + Player player = server.addPlayer(); + ItemStack itemStack = slimefunItem.getItem(); + player.getInventory().setItemInMainHand(itemStack); + + // Create a world and place the block + World world = TestUtilities.createWorld(server); + Block block = TestUtilities.placeSlimefunBlock(server, itemStack, world, player); + + // Right click on the block + PlayerInteractEvent playerInteractEvent = new PlayerInteractEvent( + player, Action.RIGHT_CLICK_BLOCK, itemStack, block, BlockFace.UP, EquipmentSlot.HAND + ); + + server.getPluginManager().callEvent(playerInteractEvent); + server.getPluginManager().assertEventFired(PlayerInteractEvent.class, e -> { + // We cancel the event on inventory open + Assertions.assertSame(e.useInteractedBlock(), Result.DENY); + return true; + }); + + // Assert our right click event fired and the block usage was not denied + server.getPluginManager().assertEventFired(PlayerRightClickEvent.class, e -> { + Assertions.assertNotSame(e.useBlock(), Result.DENY); + return true; + }); + + // Assert we do have an inventory which would be opened + // TODO: Create an event for open inventory so this isn't guess work + Assertions.assertTrue(BlockMenuPreset.isInventory(slimefunItem.getId())); + Assertions.assertTrue(BlockStorage.getStorage(block.getWorld()).hasInventory(block.getLocation())); + // TODO(future): Check viewers - MockBukkit does not implement this today + + // Break the block + BlockBreakEvent blockBreakEvent = new BlockBreakEvent(block, player); + server.getPluginManager().callEvent(blockBreakEvent); + server.getPluginManager().assertEventFired(SlimefunBlockBreakEvent.class, e -> { + Assertions.assertEquals(slimefunItem.getId(), e.getSlimefunItem().getId()); + return true; + }); + + // Assert the block is queued for removal + Assertions.assertTrue(Slimefun.getTickerTask().isDeletedSoon(block.getLocation())); + + // Clear event queue since we'll be running duplicate events + server.getPluginManager().clearEvents(); + + // Right click on the block again now that it's broken + PlayerInteractEvent secondPlayerInteractEvent = new PlayerInteractEvent( + player, Action.RIGHT_CLICK_BLOCK, itemStack, block, BlockFace.UP, EquipmentSlot.HAND + ); + + server.getPluginManager().callEvent(secondPlayerInteractEvent); + server.getPluginManager().assertEventFired(PlayerInteractEvent.class, e -> { + // We cancelled the event due to the block being removed + Assertions.assertSame(e.useInteractedBlock(), Result.DENY); + return true; + }); + + // Assert our right click event was not fired due to the block being broken + Assertions.assertThrows( + AssertionError.class, + () -> server.getPluginManager().assertEventFired(PlayerRightClickEvent.class, e -> true) + ); + } +} diff --git a/src/test/java/io/github/thebusybiscuit/slimefun4/test/TestUtilities.java b/src/test/java/io/github/thebusybiscuit/slimefun4/test/TestUtilities.java index 56f517da20..d5f5b134c4 100644 --- a/src/test/java/io/github/thebusybiscuit/slimefun4/test/TestUtilities.java +++ b/src/test/java/io/github/thebusybiscuit/slimefun4/test/TestUtilities.java @@ -10,17 +10,26 @@ import javax.annotation.Nonnull; import javax.annotation.ParametersAreNonnullByDefault; +import org.bukkit.Location; import org.bukkit.Material; import org.bukkit.NamespacedKey; import org.bukkit.OfflinePlayer; +import org.bukkit.World; +import org.bukkit.block.Block; +import org.bukkit.entity.Player; +import org.bukkit.event.block.BlockPlaceEvent; import org.bukkit.event.inventory.InventoryType; +import org.bukkit.inventory.EquipmentSlot; import org.bukkit.inventory.Inventory; import org.bukkit.inventory.ItemStack; import org.bukkit.plugin.Plugin; import org.junit.jupiter.api.Assertions; import org.mockito.Mockito; +import be.seeseemelk.mockbukkit.ServerMock; +import be.seeseemelk.mockbukkit.block.BlockMock; import io.github.bakedlibs.dough.items.CustomItemStack; +import io.github.thebusybiscuit.slimefun4.api.events.SlimefunBlockPlaceEvent; import io.github.thebusybiscuit.slimefun4.api.items.ItemGroup; import io.github.thebusybiscuit.slimefun4.api.items.SlimefunItem; import io.github.thebusybiscuit.slimefun4.api.player.PlayerProfile; @@ -28,6 +37,7 @@ import io.github.thebusybiscuit.slimefun4.implementation.Slimefun; import io.github.thebusybiscuit.slimefun4.implementation.items.VanillaItem; import io.github.thebusybiscuit.slimefun4.test.mocks.MockSlimefunItem; +import me.mrCookieSlime.Slimefun.api.BlockStorage; public final class TestUtilities { @@ -89,4 +99,26 @@ private TestUtilities() {} public static @Nonnull int randomInt(int upperBound) { return random.nextInt(upperBound); } + + public static World createWorld(ServerMock server) { + World world = server.addSimpleWorld("world_" + randomInt()); + Slimefun.getRegistry().getWorlds().put(world.getName(), new BlockStorage(world)); + return world; + } + + public static Block placeSlimefunBlock(ServerMock server, ItemStack item, World world, Player player) { + int x = TestUtilities.randomInt(); + int z = TestUtilities.randomInt(); + Block block = new BlockMock(item.getType(), new Location(world, x, 0, z)); + Block blockAgainst = new BlockMock(Material.GRASS, new Location(world, x, 1, z)); + + BlockPlaceEvent blockPlaceEvent = new BlockPlaceEvent( + block, block.getState(), blockAgainst, item, player, true, EquipmentSlot.HAND + ); + + server.getPluginManager().callEvent(blockPlaceEvent); + server.getPluginManager().assertEventFired(SlimefunBlockPlaceEvent.class, e -> true); + + return block; + } }