IF icon indicating copy to clipboard operation
IF copied to clipboard

Removed `getInventory().clear()` from onInventoryClose

Open Kyren223 opened this issue 1 year ago • 4 comments

Removed getInventory().clear() as some users don't want this functionality, but there is no way to opt-out of this, so it's better to remove it and give users the option to do it themselves if needed.

Alternative Solution: Add a way to opt-out from this, or only clear the inventory after the callbacks have been called, so if needed, callbacks can use the inventory without it being empty

Kyren223 avatar Jan 29 '24 14:01 Kyren223

Thank you for your pull request. The idea behind this line of code was to ensure items that were in the inventory are not put back into the player's inventory upon closing it, which is what happens for certain types of inventories. Clearing the inventory ensures there's nothing to be put back. Clearly this doesn't work, as other viewers then also get an empty inventory. So, while your remark is correct, we should find an alternative solution to the issue.

stefvanschie avatar Feb 05 '24 19:02 stefvanschie

Thank you for your pull request. The idea behind this line of code was to ensure items that were in the inventory are not put back into the player's inventory upon closing it, which is what happens for certain types of inventories. Clearing the inventory ensures there's nothing to be put back. Clearly this doesn't work, as other viewers then also get an empty inventory. So, while your remark is correct, we should find an alternative solution to the issue.

Yea I agree this is not a great solution, it's just to quickly solve it for those who don't want it. Maybe a better solution you can make is add a method (and a private field flag) to allow for toggling this, so it's on by default, but users can opt-out of it.

What I needed it for specifically is for a "Crafter GUI" where if someone closed it while having items in the GUI, it will give him the items back (only if they are in specific slots) so I added a listener for the close event but the inventory in the event is completely empty so I wasn't able to get the items in those slots. I guess another solution can be to move the line to after running all the listeners, so at least users can get the inventory on close event before it was cleared

Kyren223 avatar Feb 05 '24 21:02 Kyren223

@stefvanschie, is there an update on this? I'm currently trying to use IF to create a navigator UI that updates frequently with server information and is displayed to multiple viewers. However, as soon as one viewer closes the inventory, it's cleared for all others, making IF unsuitable for such a use case. An opt-out would be appropriate, as suggested by @Kyren223.

Galapen avatar May 03 '24 13:05 Galapen

@Galapen a friend recommended me to check out "Triumph Gui", it's a framework similar to this but it doesn't have fancy things like "button" (as far as I know) it's a bit more primitive but it doesn't have the issue, so you can check it out if you want. I personally prefer it bcz I don't need a UI framework, just something to abstract most of the tedious GUI stuff, so it's really nice (still let's u have custom items and attach click listeners to them)

Kyren223 avatar May 05 '24 06:05 Kyren223

@stefvanschie, is there an update on this? I'm currently trying to use IF to create a navigator UI that updates frequently with server information and is displayed to multiple viewers. However, as soon as one viewer closes the inventory, it's cleared for all others, making IF unsuitable for such a use case. An opt-out would be appropriate, as suggested by @Kyren223.

My intent is to make the next version 0.11.0 with breaking changes to address some larger issues, including this one.

stefvanschie avatar Jul 15 '24 19:07 stefvanschie

This has been addressed in version 0.11.0. Due to the large number of surrounding changes, I unfortunately wasn't able to incorporate your pull request directly, apologies for that. Nonetheless, thank you for your pull request.

stefvanschie avatar May 20 '25 19:05 stefvanschie