Fix Player#closeInventory implementation and ignore InteractInventoryEvent.Close cancelled result if it was caused by client input

Fixes #1404

Signed-off-by: Walker Crouse <walkercrouse@hotmail.com>

Author: windy1

src/main/java/org/spongepowered/common/event/SpongeCommonEventFactory.java

@@ -39,6 +39,7 @@
 import net.minecraft.entity.player.EntityPlayerMP;
 import net.minecraft.entity.player.InventoryPlayer;
 import net.minecraft.inventory.Container;
+import net.minecraft.inventory.ContainerPlayer;
 import net.minecraft.inventory.IInventory;
 import net.minecraft.inventory.Slot;
 import net.minecraft.item.ItemStack;
@@ -103,8 +104,8 @@
 import org.spongepowered.common.interfaces.world.IMixinWorldServer;
 import org.spongepowered.common.item.inventory.SpongeItemStackSnapshot;
 import org.spongepowered.common.item.inventory.adapter.impl.slots.SlotAdapter;
-import org.spongepowered.common.item.inventory.util.ItemStackUtil;
 import org.spongepowered.common.item.inventory.util.ContainerUtil;
+import org.spongepowered.common.item.inventory.util.ItemStackUtil;
 import org.spongepowered.common.registry.provider.DirectionFacingProvider;
 import org.spongepowered.common.text.SpongeTexts;
 import org.spongepowered.common.util.VecHelper;
@@ -544,28 +545,34 @@ public static boolean callInteractInventoryOpenEvent(Cause cause, EntityPlayerMP
         }
     }
 
-    public static InteractInventoryEvent.Close callInteractInventoryCloseEvent(Cause cause, Container container, EntityPlayerMP player, ItemStackSnapshot lastCursor, ItemStackSnapshot newCursor) {
+    public static InteractInventoryEvent.Close callInteractInventoryCloseEvent(Cause cause, Container container, EntityPlayerMP player, ItemStackSnapshot lastCursor, ItemStackSnapshot newCursor, boolean clientSource) {
         Transaction<ItemStackSnapshot> cursorTransaction = new Transaction<>(lastCursor, newCursor);
-        final InteractInventoryEvent.Close
-                event =
-                SpongeEventFactory.createInteractInventoryEventClose(cause, cursorTransaction, ContainerUtil.fromNative(container));
+        final InteractInventoryEvent.Close event
+                = SpongeEventFactory.createInteractInventoryEventClose(cause, cursorTransaction, ContainerUtil.fromNative(container));
         SpongeImpl.postEvent(event);
         if (event.isCancelled()) {
-            if (container.getSlot(0) != null) {
-                player.openContainer = container;
-                final Slot slot = container.getSlot(0);
-                final String guiId;
-                final IInventory slotInventory = slot.inventory;
-                if (slotInventory instanceof IInteractionObject) {
-                    guiId = ((IInteractionObject) slotInventory).getGuiID();
+            if (clientSource && container.getSlot(0) != null) {
+                if (!(container instanceof ContainerPlayer)) {
+                    // Inventory closed by client, reopen window and send container
+                    player.openContainer = container;
+                    final String guiId;
+                    final Slot slot = container.getSlot(0);
+                    final IInventory slotInventory = slot.inventory;
+                    if (slotInventory instanceof IInteractionObject) {
+                        guiId = ((IInteractionObject) slotInventory).getGuiID();
+                    } else {
+                        guiId = "minecraft:container"; // expected fallback for unknown types
+                    }
+                    slotInventory.openInventory(player);
+                    player.connection.sendPacket(new SPacketOpenWindow(container.windowId, guiId, slotInventory
+                        .getDisplayName(), slotInventory.getSizeInventory()));
+                    // resync data to client
+                    player.sendContainerToPlayer(container);
                 } else {
-                    guiId = "unknown";
+                    // TODO: Maybe print a warning or throw an exception here? The player gui cannot be opened from the
+                    // server so allowing this event to be cancellable when the GUI has been closed already would result
+                    // in opening the wrong GUI window.
                 }
-                slotInventory.openInventory(player);
-                player.connection.sendPacket(new SPacketOpenWindow(container.windowId, guiId, slotInventory
-                        .getDisplayName(), slotInventory.getSizeInventory()));
-                // resync data to client
-                player.sendContainerToPlayer(container);
             }
         } else {
             IMixinContainer mixinContainer = (IMixinContainer) player.openContainer;
@@ -575,7 +582,11 @@ public static InteractInventoryEvent.Close callInteractInventoryCloseEvent(Cause
             if (event.getCursorTransaction().getCustom().isPresent()) {
                 handleCustomCursor(player, event.getCursorTransaction().getFinal());
             }
+            if (!clientSource) {
+                player.closeScreen();
+            }
         }
+
         return event;
     }
 

src/main/java/org/spongepowered/common/event/tracking/phase/packet/PacketFunction.java

@@ -842,7 +842,7 @@ static void processEntities(EntityPlayerMP player, CauseTracker causeTracker, Co
                 .orElseThrow(TrackingUtil.throwWithContext("Expected a cursor item stack, but had nothing!", context));
         ItemStackSnapshot newCursor = ItemStackUtil.snapshotOf(player.inventory.getItemStack());
         final Cause cause = Cause.source(player).build();
-        InteractInventoryEvent.Close event = SpongeCommonEventFactory.callInteractInventoryCloseEvent(cause, container, player, lastCursor, newCursor);
+        InteractInventoryEvent.Close event = SpongeCommonEventFactory.callInteractInventoryCloseEvent(cause, container, player, lastCursor, newCursor, true);
         if (!event.isCancelled()) {
             // Non-merged items
             context.getCapturedItemsSupplier().ifPresentAndNotEmpty(items -> {

src/main/java/org/spongepowered/common/mixin/core/entity/player/MixinEntityPlayerMP.java

@@ -501,7 +501,7 @@ public Optional<Container> openInventory(Inventory inventory, Cause cause) throw
     @Override
     public boolean closeInventory(Cause cause) throws IllegalArgumentException {
         ItemStackSnapshot cursor = ItemStackUtil.snapshotOf(this.inventory.getItemStack());
-        return !SpongeCommonEventFactory.callInteractInventoryCloseEvent(cause, this.openContainer, this$, cursor, cursor).isCancelled();
+        return !SpongeCommonEventFactory.callInteractInventoryCloseEvent(cause, this.openContainer, this.this$, cursor, cursor, false).isCancelled();
     }
 
     @Override