As proposed in #157, I've done some refactoring to implement the idea.
Already tested with my own plugin, but more test would be ideal.
So why this work?
Lets go back to the Java basics.
We know that we can't compare two String just by using == operator.
But why is that so? because == checks if two instances are exactly the same, not if they are 'equal.' That's why we need to compare two Strings like "abc".equals("abc") as when we use the quote to create String literal, it instantiate new String even though the value is the same. So "abc" and "abc" are 'equal' yet not the same instance.
Well, then why is that even relevant for the PR?
Because when Bukkit#createInventory() method is called, Bukkit API instantiate Inventory for us. And throughout all the complicated Event handlings, this specific Inventory 'instance' does not change.
For example, if we listen to InventoryClickEvent, InventoryOpenEvent, and InventoryCloseEvent, the Inventory instance returned by getInventory() would be the exact same Inventory instance we've created by Bukkit#createInventory().
In current implementation, we map SmartInventory with Player instance, yet in this PR, we will map SmartInvetory to Inventory instance directly.
But mapping to Player instance works; why change?
1. It's more straight forward.
Mapping to Player works, definitely. We can think it as we are keeping tracking of which SmartInventory a Player is currently dealing with
However, things gets really ugly when handling transition between two GUIs. While mapping Player to SmartInventory, it's done by un-registering the previous SmartInventory (which is named oldInv) and then registering the new Inventory to open.
But as we already know, SmartInventory is per Player instance, so at this point, listening to InventoryOpenEvent or InventoryCloseEvent is very complicated process. Before un-registering the previous SmartInventory, it manually fires InventoryCloseEvent, and if the listener of InventoryCloseEvent is opening another SmartInventory, it will close the Inventory that the Player is currently dealing with, so it fires InventoryCloseEvent, and so on and so on.
I'm still getting confused what's going on while writing this PR. The point is, it doesn't have to be this complicate!
Now, using implementation in this PR, each Inventory maps one SmartInventory, which is responsible for the Inventory, so InventoryCloseEvent, InventoryOpenEvent, or whatsoever Inventory event is no longer bounded to any other Inventory. Even opening another SmartInventory inside the InventoryCloseEvent is safe, since the new Inventory instance is not the same instance as the Inventory instance the Player is currently dealing with.
2. It has complete control over any Inventory
when creating a GUI library, cancelling the click event appropriately is very important.
Failing to do so may cause non-GUI Inventory to be not responsive, or even worse, if it fails to cancel the click event of GUI Inventory, players may pick up items and thus duplicate items!
Because now we track Inventory by its instance, as long as the instance exist in our inventories Map, we can simply cancel the event and rest assured. Because until InventoryCloseEvent is called and the instance is removed from the inventories, event will be cancelled no matter what. When Inventory dies, SmartInventory also dies with it.
Then you may ask, "what if InventoryCloseEvent is not called"? That's not going to happen in any case. Even when player lost connection before closing an Inventory, InventoryCloseEvent will be called before PlayerQuitEvent is fired. (Refer to the link)
One downside is that in old versions (such as 1.5.2), Inventory instance change throughout the Inventory events (for example, Inventory instance in InventoryOpenEvent and InventoryClickEvent differ), so it no longer can track which Inventory instance is GUI.
At least tested and confirmed working in 1.16.3, yet I think that this is highly likely not an issue if the server is 1.7 and above.
However, more test will be always better.
My job is done here since I use only latest version (1.16.3 atm), so it's up to you to merge it or not!
As proposed in #157, I've done some refactoring to implement the idea.
Already tested with my own plugin, but more test would be ideal.
So why this work?
Lets go back to the Java basics.
We know that we can't compare two String just by using == operator.
But why is that so? because == checks if two instances are exactly the same, not if they are 'equal.' That's why we need to compare two Strings like
"abc".equals("abc")
as when we use the quote to create String literal, it instantiate new String even though the value is the same. So "abc" and "abc" are 'equal' yet not the same instance.Well, then why is that even relevant for the PR?
Because when Bukkit#createInventory() method is called, Bukkit API instantiate Inventory for us. And throughout all the complicated Event handlings, this specific Inventory 'instance' does not change.
For example, if we listen to InventoryClickEvent, InventoryOpenEvent, and InventoryCloseEvent, the Inventory instance returned by getInventory() would be the exact same Inventory instance we've created by Bukkit#createInventory().
In current implementation, we map SmartInventory with Player instance, yet in this PR, we will map SmartInvetory to Inventory instance directly.
But mapping to Player instance works; why change?
1. It's more straight forward.
Mapping to Player works, definitely. We can think it as we are keeping tracking of
which SmartInventory a Player is currently dealing with
However, things gets really ugly when handling transition between two GUIs. While mapping Player to SmartInventory, it's done by un-registering the previous SmartInventory (which is named oldInv) and then registering the new Inventory to open.
But as we already know, SmartInventory is per Player instance, so at this point, listening to InventoryOpenEvent or InventoryCloseEvent is very complicated process. Before un-registering the previous SmartInventory, it manually fires InventoryCloseEvent, and if the listener of InventoryCloseEvent is opening another SmartInventory, it will close the Inventory that the Player is currently dealing with, so it fires InventoryCloseEvent, and so on and so on.
I'm still getting confused what's going on while writing this PR. The point is, it doesn't have to be this complicate!
Now, using implementation in this PR, each Inventory maps one SmartInventory, which is responsible for the Inventory, so InventoryCloseEvent, InventoryOpenEvent, or whatsoever Inventory event is no longer bounded to any other Inventory. Even opening another SmartInventory inside the InventoryCloseEvent is safe, since the new Inventory instance is not the same instance as the Inventory instance the Player is currently dealing with.
2. It has complete control over any Inventory
when creating a GUI library, cancelling the click event appropriately is very important.
Failing to do so may cause non-GUI Inventory to be not responsive, or even worse, if it fails to cancel the click event of GUI Inventory, players may pick up items and thus duplicate items!
Because now we track Inventory by its instance, as long as the instance exist in our
inventories
Map, we can simply cancel the event and rest assured. Because until InventoryCloseEvent is called and the instance is removed from theinventories,
event will be cancelled no matter what. When Inventory dies, SmartInventory also dies with it.Then you may ask, "what if InventoryCloseEvent is not called"? That's not going to happen in any case. Even when player lost connection before closing an Inventory, InventoryCloseEvent will be called before PlayerQuitEvent is fired. (Refer to the link)
One downside is that in old versions (such as 1.5.2), Inventory instance change throughout the Inventory events (for example, Inventory instance in InventoryOpenEvent and InventoryClickEvent differ), so it no longer can track which Inventory instance is GUI.
At least tested and confirmed working in 1.16.3, yet I think that this is highly likely not an issue if the server is 1.7 and above.
However, more test will be always better.
My job is done here since I use only latest version (1.16.3 atm), so it's up to you to merge it or not!