Player: Window system now only allows 1 window at a time

src/pocketmine/Player.php

@@ -115,6 +115,7 @@ use pocketmine\world\particle\PunchBlockParticle;
 use pocketmine\world\Position;
 use pocketmine\world\World;
 use function abs;
+use function array_search;
 use function assert;
 use function ceil;
 use function count;
@@ -182,12 +183,13 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 	/** @var PlayerInfo */
 	protected $playerInfo;
 
+	/** @var int */
 	protected $lastInventoryNetworkId = 2;
-	/** @var int[] object ID => network ID */
+	/** @var Inventory[] network ID => inventory */
-	protected $inventoryNetworkIdMap = [];
+	protected $networkIdToInventoryMap = [];
+	/** @var Inventory|null */
+	protected $currentWindow = null;
 	/** @var Inventory[] */
-	protected $windowIndex = [];
-	/** @var bool[] */
 	protected $permanentWindows = [];
 	/** @var PlayerCursorInventory */
 	protected $cursorInventory;
@@ -2424,13 +2426,11 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 		$this->usedChunks = [];
 		$this->loadQueue = [];
 
-		$this->removeAllWindows();
+		$this->removeCurrentWindow();
-		foreach($this->permanentWindows as $networkId => $bool){
+		foreach($this->permanentWindows as $objectId => $inventory){
-			$this->closeInventoryInternal($this->windowIndex[$networkId], true);
+			$this->closeInventoryInternal($inventory, true);
 		}
-		assert(empty($this->windowIndex) && empty($this->inventoryNetworkIdMap));
+		assert(empty($this->networkIdToInventoryMap));
-		$this->inventoryNetworkIdMap = [];
-		$this->windowIndex = [];
 
 		$this->perm->clearPermissions();
 
@@ -2657,7 +2657,7 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 	public function teleport(Vector3 $pos, ?float $yaw = null, ?float $pitch = null) : bool{
 		if(parent::teleport($pos, $yaw, $pitch)){
 
-			$this->removeAllWindows();
+			$this->removeCurrentWindow();
 
 			$this->sendPosition($this, $this->yaw, $this->pitch, MovePlayerPacket::MODE_TELEPORT);
 			$this->broadcastMovement(true);
@@ -2741,14 +2741,10 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 	 * @return bool
 	 */
 	public function doCloseWindow(int $windowId) : bool{
-		if($windowId === 0){
-			return false;
-		}
-
 		$this->doCloseInventory();
 
-		if(isset($this->windowIndex[$windowId])){
+		if($windowId === $this->lastInventoryNetworkId and $this->currentWindow !== null){
-			$this->removeWindow($this->windowIndex[$windowId]);
+			$this->removeCurrentWindow();
 			return true;
 		}
 		if($windowId === 255){
@@ -2756,18 +2752,62 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 			return true;
 		}
 
+		$this->logger->debug("Attempted to close inventory with network ID $windowId, but current is $this->lastInventoryNetworkId");
 		return false;
 	}
 
+	/**
+	 * Returns the inventory the player is currently viewing. This might be a chest, furnace, or any other container.
+	 *
+	 * @return Inventory|null
+	 */
+	public function getCurrentWindow() : ?Inventory{
+		return $this->currentWindow;
+	}
+
+	/**
+	 * Opens an inventory window to the player. Returns if it was successful.
+	 *
+	 * @param Inventory $inventory
+	 *
+	 * @return bool
+	 */
+	public function setCurrentWindow(Inventory $inventory) : bool{
+		if($inventory === $this->currentWindow){
+			return true;
+		}
+		$ev = new InventoryOpenEvent($inventory, $this);
+		$ev->call();
+		if($ev->isCancelled()){
+			return false;
+		}
+
+		//TODO: client side race condition here makes the opening work incorrectly
+		$this->removeCurrentWindow();
+
+		$networkId = $this->lastInventoryNetworkId = max(ContainerIds::FIRST, ($this->lastInventoryNetworkId + 1) % ContainerIds::LAST);
+
+		$this->openInventoryInternal($inventory, $networkId, false);
+		$this->currentWindow = $inventory;
+		return true;
+	}
+
+	public function removeCurrentWindow() : void{
+		if($this->currentWindow !== null){
+			(new InventoryCloseEvent($this->craftingGrid, $this))->call();
+			$this->closeInventoryInternal($this->currentWindow, false);
+		}
+	}
+
 	/**
 	 * Returns the window ID which the inventory has for this player, or -1 if the window is not open to the player.
 	 *
 	 * @param Inventory $inventory
 	 *
-	 * @return int
+	 * @return int|null
 	 */
-	public function getWindowId(Inventory $inventory) : int{
+	public function getWindowId(Inventory $inventory) : ?int{
-		return $this->inventoryNetworkIdMap[spl_object_id($inventory)] ?? ContainerIds::NONE;
+		return ($id = array_search($inventory, $this->networkIdToInventoryMap, true)) !== false ? $id : null;
 	}
 
 	/**
@@ -2779,104 +2819,48 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 	 * @return Inventory|null
 	 */
 	public function getWindow(int $windowId) : ?Inventory{
-		return $this->windowIndex[$windowId] ?? null;
+		return $this->networkIdToInventoryMap[$windowId] ?? null;
 	}
 
 	protected function openInventoryInternal(Inventory $inventory, int $networkId, bool $permanent) : void{
-		$this->windowIndex[$networkId] = $inventory;
+		$this->logger->debug("Opening inventory " . get_class($inventory) . "#" . spl_object_id($inventory) . " with network ID $networkId");
-		$this->inventoryNetworkIdMap[spl_object_id($inventory)] = $networkId;
+		$this->networkIdToInventoryMap[$networkId] = $inventory;
 		$inventory->onOpen($this);
 		if($permanent){
-			$this->logger->debug("Opening permanent inventory " . get_class($inventory) . " with network ID $networkId");
+			$this->permanentWindows[spl_object_id($inventory)] = $inventory;
-			$this->permanentWindows[$networkId] = true;
 		}
 	}
 
 	protected function closeInventoryInternal(Inventory $inventory, bool $force) : bool{
+		$this->logger->debug("Closing inventory " . get_class($inventory) . "#" . spl_object_id($inventory));
 		$objectId = spl_object_id($inventory);
-		$networkId = $this->inventoryNetworkIdMap[$objectId] ?? null;
+		if($inventory === $this->currentWindow){
-		if($networkId !== null){
+			$this->currentWindow = null;
-			if(isset($this->permanentWindows[$networkId])){
+		}elseif(isset($this->permanentWindows[$objectId])){
 			if(!$force){
 				throw new \InvalidArgumentException("Cannot remove fixed window " . get_class($inventory) . " from " . $this->getName());
 			}
-				$this->logger->debug("Closing permanent inventory " . get_class($inventory) . " with network ID $networkId");
+			unset($this->permanentWindows[$objectId]);
-			}
+		}else{
-
-			$inventory->onClose($this);
-			unset($this->inventoryNetworkIdMap[$objectId], $this->windowIndex[$networkId], $this->permanentWindows[$networkId]);
-			return true;
-		}
 			return false;
 		}
 
-	/**
+		$inventory->onClose($this);
-	 * Opens an inventory window to the player. Returns the ID of the created window, or the existing window ID if the
+		$networkId = $this->getWindowId($inventory);
-	 * player is already viewing the specified inventory.
+		assert($networkId !== null);
-	 *
+		unset($this->networkIdToInventoryMap[$networkId]);
-	 * @param Inventory $inventory
+		return true;
-	 *
-	 * @return int
-	 *
-	 * @throws \InvalidArgumentException if a forceID which is already in use is specified
-	 * @throws \InvalidStateException if trying to add a window without forceID when no slots are free
-	 */
-	public function addWindow(Inventory $inventory) : int{
-		if(($id = $this->getWindowId($inventory)) !== ContainerIds::NONE){
-			return $id;
-		}
-
-		$networkId = $this->lastInventoryNetworkId;
-		do{
-			$networkId = max(ContainerIds::FIRST, ($networkId + 1) % ContainerIds::LAST);
-			if($networkId === $this->lastInventoryNetworkId){ //wraparound, no free slots
-				throw new \InvalidStateException("No free window IDs found");
-			}
-		}while(isset($this->windowIndex[$networkId]));
-		$this->lastInventoryNetworkId = $networkId;
-
-		$ev = new InventoryOpenEvent($inventory, $this);
-		$ev->call();
-		if($ev->isCancelled()){
-			return -1;
-		}
-
-		$this->openInventoryInternal($inventory, $networkId, false);
-		return $networkId;
-	}
-
-	/**
-	 * Removes an inventory window from the player.
-	 *
-	 * @param Inventory $inventory
-	 *
-	 * @throws \InvalidArgumentException if trying to remove a fixed inventory window
-	 */
-	public function removeWindow(Inventory $inventory){
-		if($this->closeInventoryInternal($inventory, false)){
-			(new InventoryCloseEvent($inventory, $this))->call();
-		}
-	}
-
-	/**
-	 * Removes all inventory windows from the player. This WILL NOT remove permanent inventories such as the player's
-	 * own inventory.
-	 */
-	public function removeAllWindows(){
-		foreach($this->windowIndex as $networkId => $window){
-			if(isset($this->permanentWindows[$networkId])){
-				continue;
-			}
-
-			$this->removeWindow($window);
-		}
 	}
 
 	/**
 	 * @return Inventory[]
 	 */
 	public function getAllWindows() : array{
-		return $this->windowIndex;
+		$windows = $this->permanentWindows;
+		if($this->currentWindow !== null){
+			$windows[] = $this->currentWindow;
+		}
+		return $windows;
 	}
 
 	public function setMetadata(string $metadataKey, MetadataValue $newMetadataValue) : void{

src/pocketmine/block/Anvil.php

@@ -64,7 +64,7 @@ class Anvil extends Transparent implements Fallable{
 
 	public function onInteract(Item $item, int $face, Vector3 $clickVector, ?Player $player = null) : bool{
 		if($player instanceof Player){
-			$player->addWindow(new AnvilInventory($this));
+			$player->setCurrentWindow(new AnvilInventory($this));
 		}
 
 		return true;

src/pocketmine/block/BrewingStand.php

@@ -62,7 +62,7 @@ class BrewingStand extends Transparent{
 		if($player instanceof Player){
 			$stand = $this->getWorld()->getTile($this);
 			if($stand instanceof TileBrewingStand and $stand->canOpenWith($item->getCustomName())){
-				$player->addWindow($stand->getInventory());
+				$player->setCurrentWindow($stand->getInventory());
 			}
 		}
 

src/pocketmine/block/Chest.php

@@ -99,7 +99,7 @@ class Chest extends Transparent{
 					return true;
 				}
 
-				$player->addWindow($chest->getInventory());
+				$player->setCurrentWindow($chest->getInventory());
 			}
 		}
 

src/pocketmine/block/EnchantingTable.php

@@ -45,7 +45,7 @@ class EnchantingTable extends Transparent{
 		if($player instanceof Player){
 			//TODO lock
 
-			$player->addWindow(new EnchantInventory($this));
+			$player->setCurrentWindow(new EnchantInventory($this));
 		}
 
 		return true;

src/pocketmine/block/EnderChest.php

@@ -76,7 +76,7 @@ class EnderChest extends Transparent{
 			$enderChest = $this->getWorld()->getTile($this);
 			if($enderChest instanceof TileEnderChest and $this->getSide(Facing::UP)->isTransparent()){
 				$player->getEnderChestInventory()->setHolderPosition($enderChest);
-				$player->addWindow($player->getEnderChestInventory());
+				$player->setCurrentWindow($player->getEnderChestInventory());
 			}
 		}
 

src/pocketmine/block/Furnace.php

@@ -92,7 +92,7 @@ class Furnace extends Solid{
 		if($player instanceof Player){
 			$furnace = $this->getWorld()->getTile($this);
 			if($furnace instanceof TileFurnace and $furnace->canOpenWith($item->getCustomName())){
-				$player->addWindow($furnace->getInventory());
+				$player->setCurrentWindow($furnace->getInventory());
 			}
 		}
 

src/pocketmine/block/Hopper.php

@@ -78,7 +78,7 @@ class Hopper extends Transparent{
 		if($player !== null){
 			$tile = $this->world->getTile($this);
 			if($tile instanceof TileHopper){ //TODO: find a way to have inventories open on click without this boilerplate in every block
-				$player->addWindow($tile->getInventory());
+				$player->setCurrentWindow($tile->getInventory());
 			}
 			return true;
 		}

src/pocketmine/inventory/BaseInventory.php

@@ -342,7 +342,9 @@ abstract class BaseInventory implements Inventory{
 	 */
 	public function removeAllViewers() : void{
 		foreach($this->viewers as $hash => $viewer){
-			$viewer->removeWindow($this);
+			if($viewer->getCurrentWindow() === $this){ //this might not be the case for the player's own inventory
+				$viewer->removeCurrentWindow();
+			}
 			unset($this->viewers[$hash]);
 		}
 	}

src/pocketmine/network/mcpe/NetworkSession.php

@@ -753,14 +753,14 @@ class NetworkSession{
 
 	public function syncInventorySlot(Inventory $inventory, int $slot) : void{
 		$windowId = $this->player->getWindowId($inventory);
-		if($windowId !== ContainerIds::NONE){
+		if($windowId !== null){
 			$this->sendDataPacket(InventorySlotPacket::create($windowId, $slot, $inventory->getItem($slot)));
 		}
 	}
 
 	public function syncInventoryContents(Inventory $inventory) : void{
 		$windowId = $this->player->getWindowId($inventory);
-		if($windowId !== ContainerIds::NONE){
+		if($windowId !== null){
 			$this->sendDataPacket(InventoryContentPacket::create($windowId, $inventory->getContents(true)));
 		}
 	}
@@ -773,7 +773,7 @@ class NetworkSession{
 
 	public function syncInventoryData(Inventory $inventory, int $propertyId, int $value) : void{
 		$windowId = $this->player->getWindowId($inventory);
-		if($windowId !== ContainerIds::NONE){
+		if($windowId !== null){
 			$this->sendDataPacket(ContainerSetDataPacket::create($windowId, $propertyId, $value));
 		}
 	}