Paper icon indicating copy to clipboard operation
Paper copied to clipboard

Fix cancelling PlayerDropItemEvent for carried items

Open Machine-Maker opened this issue 3 years ago • 10 comments

Fixes https://github.com/PaperMC/Paper/issues/7726

Now if the PlayerDropItemEvent is cancelled and the drop is trigged by clicking outside a menu, the item remains on the cursor instead of going back into the inventory or being deleted if the inventory is full.

Machine-Maker avatar Apr 10 '22 18:04 Machine-Maker

There needs to be a consensus on what happens if the player closes the inventory with the item in their cursor and the drop item event is cancelled (and the inventory is full). Thoughts on what should happen?

Machine-Maker avatar Apr 10 '22 21:04 Machine-Maker

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jun 10 '22 17:06 stale[bot]

This issue has been automatically closed because it has not had activity in a long time. If the issue still applies to the most recent supported version, please open a new issue referencing this original issue.

stale[bot] avatar Jun 18 '22 16:06 stale[bot]

Yeah, I made some changes cause I spotted a bug in what I was doing before, but I didn't test those. I guess I broke it more.

Machine-Maker avatar Jul 20 '22 17:07 Machine-Maker

Ok, fixed that issue. Also I changed what the cursor is set to if the event is cancelled. It gets the itemstack from the item drop. So if an event handler changes the Item entity and then cancels the event it uses that itemstack in the item entity. Is that right? I don't know if we do that elsewhere, take an action based on the event if its cancelled.

Machine-Maker avatar Jul 20 '22 21:07 Machine-Maker

Since I came across this issue again, how about being able to provide a consumer for the itemstack, that will only accept the item if it has no other place to go. That would allow for plugin authors to handle the "lost" item as they wish. For example adding it to a "stash" of items, which may be used in other places across the server to prevent item loss in the event of any unexpected errors.

aerulion avatar Apr 11 '23 19:04 aerulion

I like this idea. But what should the default behavior be? Just delete the item?

Machine-Maker avatar Apr 11 '23 20:04 Machine-Maker

I guess so, maybe add a warning to the javadoc that one has to be cautious when cancelling the event and may need to provide further handling to prevent item loss? (And that is "known" behavior anyways right now)

aerulion avatar Apr 11 '23 21:04 aerulion

Ok, added that overflow consumer. Tested it as well, setting the consumer to drop the itemstack in the world anyways. By default, it just sets the count of the stack to 0 to "delete" the itemstack.

Machine-Maker avatar Apr 16 '23 00:04 Machine-Maker

Any chance this can get merged at some point? Adding an overflow consumer seems like the best option imo.

bridgelol avatar Apr 09 '24 22:04 bridgelol