Sponge: Return value of Player.openInventory might "lie"

[Xemiru]

I am currently running

• SpongeForge version: 1.12.2-2705-7.1.10-BETA-3206
• Java version: 1.8.0_144
• Operating System: Windows 7

Issue Description
_This pertains specifically to Player#openInventory's implementation in SpongeCommon, but could be applied to any other inventory actions that delay themselves a similar way._

When detecting inventory event sensitivity, a call to Player#openInventory will try to delay itself to the next tick possible. If this happens inside a response to an inventory event (for example, the ClickInventoryEvent), the delay happens and the inventory isn't actually opened at the callsite and thus the returned Optional<Container> of the method isn't necessarily what's expected.

This poses an issue in the example use case of inventory menus, as without a way to change the bottom inventory that a Container uses without one's own implementation of a container (not recommended), a menu that wants to use both the top inventory and the bottom inventory needs to know when the inventory is opened through an InteractInventoryEvent.Open event. The callsite uses the optional return value to determine whether or not the inventory was actually opened and responds accordingly to set up for the open event. Under the assumption that an empty result means the inventory wasn't opened at all, the open event handler will miss tracking the player opening that inventory (and thus storing, then updating the bottom half of the menu) leaving a player in an inventory with unintended effects (for example, if this happens during a response to ClickInventoryEvent) _since the inventory is opened anyway on the next tick without necessarily notifying the plugin specifically._

The proposed solution is to throw an exception instead of having Sponge automagically delay the action itself. This allows the plugin to catch the exception and choose what to do in this scenario, including rescheduling the inventory opening so as to make sure it can always properly respond.

You can see this issue in action here in the provided snippet. This is not the optimal way to do an inventory menu implementation, and is purely to demonstrate the issue.

InventoryIssues.java / JAR file
(JAR file suspiciously big? This file does also have the code for testing #1985 with the @Plugin annotation commented out.)

• Open the menu with /openinv.
• Ensure the bottom slots and the stick in the top slots are denied changes.
• Click the Golden Sword to cause Sponge to reopen the menu with a delay, due to the call happening inside a ClickInventoryEvent.
• Line 105 is executed (it shouldn't get executed if the inventory gets opened).
• All of the menu is suddenly modifiable. This is a problem.

[SimonFlash]

An exception doesn't really solve your issue. You need to change your calls to openMenu during your click event (or for that matter any inventory event and related method) so that it's delayed through a Task accordingly.

[Xemiru]

I did say it wasn't the optimal way.

I'm moreso poking at the fact that Sponge doing the delay on its own opens up the potential for issues that can be avoided by instead giving more strict control to the plugin.

The project that encountered this issue already completely resolved it, along with various other issues related to the general unreliability of inventory open/close events in their current state by just choosing to ignore them entirely.

edit: Ignoring the events and relying on the scheduler proved unreliable, and I instead went back to the events but simply performed the setup through the open event instead and checking if the event is relevant.

[Faithcaio]

Exception when opening/closing during an Inventory Event will come in API8.