Seal up main inventory open/close logic inside InventoryManager where it belongs

src/network/mcpe/InventoryManager.php

@@ -67,8 +67,7 @@ class InventoryManager{
 	//these IDs are used for 1.16 to restore 1.14ish crafting & inventory behaviour; since they don't seem to have any
 	//effect on the behaviour of inventory transactions I don't currently plan to integrate these into the main system.
 	private const RESERVED_WINDOW_ID_RANGE_START = ContainerIds::LAST - 10;
-	private const RESERVED_WINDOW_ID_RANGE_END = ContainerIds::LAST;
+	private const HARDCODED_INVENTORY_WINDOW_ID = self::RESERVED_WINDOW_ID_RANGE_START + 2;
-	public const HARDCODED_INVENTORY_WINDOW_ID = self::RESERVED_WINDOW_ID_RANGE_START + 2;
 
 	/** @var Player */
 	private $player;
@@ -80,6 +79,15 @@ class InventoryManager{
 	/** @var int */
 	private $lastInventoryNetworkId = ContainerIds::FIRST;
 
+	/**
+	 * TODO: HACK! This tracks GUIs for inventories that the server considers "always open" so that the client can't
+	 * open them twice. (1.16 hack)
+	 * @var true[]
+	 * @phpstan-var array<int, true>
+	 * @internal
+	 */
+	protected $openHardcodedWindows = [];
+
 	/**
 	 * @var Item[][]
 	 * @phpstan-var array<int, array<int, Item>>
@@ -186,6 +194,21 @@ class InventoryManager{
 		return null;
 	}
 
+	public function onClientOpenMainInventory() : void{
+		$id = self::HARDCODED_INVENTORY_WINDOW_ID;
+		if(!isset($this->openHardcodedWindows[$id])){
+			//TODO: HACK! this restores 1.14ish behaviour, but this should be able to be listened to and
+			//controlled by plugins. However, the player is always a subscriber to their own inventory so it
+			//doesn't integrate well with the regular container system right now.
+			$this->openHardcodedWindows[$id] = true;
+			$this->session->sendDataPacket(ContainerOpenPacket::entityInv(
+				InventoryManager::HARDCODED_INVENTORY_WINDOW_ID,
+				WindowTypes::INVENTORY,
+				$this->player->getId()
+			));
+		}
+	}
+
 	public function onCurrentWindowRemove() : void{
 		if(isset($this->windowMap[$this->lastInventoryNetworkId])){
 			$this->remove($this->lastInventoryNetworkId);
@@ -194,16 +217,18 @@ class InventoryManager{
 	}
 
 	public function onClientRemoveWindow(int $id) : void{
-		if($id >= self::RESERVED_WINDOW_ID_RANGE_START && $id <= self::RESERVED_WINDOW_ID_RANGE_END){
+		if(isset($this->openHardcodedWindows[$id])){
-			//TODO: HACK! crafting grid & main inventory currently use these fake IDs
+			unset($this->openHardcodedWindows[$id]);
-			return;
+		}elseif($id === $this->lastInventoryNetworkId){
-		}
-		if($id === $this->lastInventoryNetworkId){
 			$this->remove($id);
 			$this->player->removeCurrentWindow();
 		}else{
 			$this->session->getLogger()->debug("Attempted to close inventory with network ID $id, but current is $this->lastInventoryNetworkId");
 		}
+
+		//Always send this, even if no window matches. If we told the client to close a window, it will behave as if it
+		//initiated the close and expect an ack.
+		$this->session->sendDataPacket(ContainerClosePacket::create($id, false));
 	}
 
 	public function syncSlot(Inventory $inventory, int $slot) : void{

src/network/mcpe/handler/InGamePacketHandler.php

@@ -56,7 +56,6 @@ use pocketmine\network\mcpe\protocol\BossEventPacket;
 use pocketmine\network\mcpe\protocol\CommandBlockUpdatePacket;
 use pocketmine\network\mcpe\protocol\CommandRequestPacket;
 use pocketmine\network\mcpe\protocol\ContainerClosePacket;
-use pocketmine\network\mcpe\protocol\ContainerOpenPacket;
 use pocketmine\network\mcpe\protocol\CraftingEventPacket;
 use pocketmine\network\mcpe\protocol\EmotePacket;
 use pocketmine\network\mcpe\protocol\InteractPacket;
@@ -92,12 +91,10 @@ use pocketmine\network\mcpe\protocol\types\inventory\ReleaseItemTransactionData;
 use pocketmine\network\mcpe\protocol\types\inventory\UIInventorySlotOffset;
 use pocketmine\network\mcpe\protocol\types\inventory\UseItemOnEntityTransactionData;
 use pocketmine\network\mcpe\protocol\types\inventory\UseItemTransactionData;
-use pocketmine\network\mcpe\protocol\types\inventory\WindowTypes;
 use pocketmine\network\mcpe\protocol\types\PlayerAction;
 use pocketmine\network\PacketHandlingException;
 use pocketmine\player\Player;
 use pocketmine\utils\AssumptionFailedError;
-use function array_key_exists;
 use function array_push;
 use function base64_encode;
 use function count;
@@ -137,15 +134,6 @@ class InGamePacketHandler extends PacketHandler{
 	/** @var bool */
 	public $forceMoveSync = false;
 
-	/**
-	 * TODO: HACK! This tracks GUIs for inventories that the server considers "always open" so that the client can't
-	 * open them twice. (1.16 hack)
-	 * @var true[]
-	 * @phpstan-var array<int, true>
-	 * @internal
-	 */
-	protected $openHardcodedWindows = [];
-
 	private InventoryManager $inventoryManager;
 
 	public function __construct(Player $player, NetworkSession $session, InventoryManager $inventoryManager){
@@ -488,19 +476,8 @@ class InGamePacketHandler extends PacketHandler{
 		if($target === null){
 			return false;
 		}
-		if(
+		if($packet->action === InteractPacket::ACTION_OPEN_INVENTORY && $target === $this->player){
-			$packet->action === InteractPacket::ACTION_OPEN_INVENTORY && $target === $this->player &&
+			$this->inventoryManager->onClientOpenMainInventory();
-			!array_key_exists($windowId = InventoryManager::HARDCODED_INVENTORY_WINDOW_ID, $this->openHardcodedWindows)
-		){
-			//TODO: HACK! this restores 1.14ish behaviour, but this should be able to be listened to and
-			//controlled by plugins. However, the player is always a subscriber to their own inventory so it
-			//doesn't integrate well with the regular container system right now.
-			$this->openHardcodedWindows[$windowId] = true;
-			$this->session->sendDataPacket(ContainerOpenPacket::entityInv(
-				InventoryManager::HARDCODED_INVENTORY_WINDOW_ID,
-				WindowTypes::INVENTORY,
-				$this->player->getId()
-			));
 			return true;
 		}
 		return false; //TODO
@@ -595,13 +572,7 @@ class InGamePacketHandler extends PacketHandler{
 	public function handleContainerClose(ContainerClosePacket $packet) : bool{
 		$this->player->doCloseInventory();
 
-		if(array_key_exists($packet->windowId, $this->openHardcodedWindows)){
-			unset($this->openHardcodedWindows[$packet->windowId]);
-		}else{
 		$this->inventoryManager->onClientRemoveWindow($packet->windowId);
-		}
-
-		$this->session->sendDataPacket(ContainerClosePacket::create($packet->windowId, false));
 		return true;
 	}