BookTake.jar
BookTakePlugin.java
[SPIGOT-4889] Taking a book from a lectern should call an event Created: 07/May/19 Updated: 11/May/19 Resolved: 11/May/19 |
|
| Status: | Resolved |
| Project: | Spigot |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | New Feature | Priority: | Minor |
| Reporter: | Brokkonaut | Assignee: | Unassigned |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | 1.14, Inventory, Lectern, Spigot | ||
| Attachments: |
BookTake.jar
BookTakePlugin.java
|
| Version: | CraftBukkit version git-Bukkit-d203283 (MC: 1.14) |
| Guidelines Read: | Yes |
| Description |
|
When a player tries to take a book from a lectern an event should be called.
I think the best matching event is the InventoryClickEvent with a new InventoryAction MOVE_TO_OTHER_INVENTORY_OR_DROP - Because unlike other inventory actions this action will drop the stack if there is no free space in the players inventory.
I will create a PR for this.
Attached is an example plugin that does some debug output for the InventoryClickEvent and allows to cancel it. To toggle the cancelling, execute /togglebooktake |
| Comments |
| Comment by blablubbabc [ 10/May/19 ] |
|
Looks fine to me. |
| Comment by md_5 [ 10/May/19 ] |
|
Seems ok to me, any other thoughts blablubbabc? |
| Comment by Brokkonaut [ 10/May/19 ] |
|
Ok, I changed it to PlayerTakeLecternBookEvent - I think that's most useful variant. |
| Comment by blablubbabc [ 10/May/19 ] |
|
> I'm not completely confident about the package (event.inventory, event.player or event.block would all match in some way) and about the class name. The name might be misleading, since it's not the lectern doing anything on its own. Two possible solutions might be:
|
| Comment by Brokkonaut [ 10/May/19 ] |
|
I changed the API to a seperate event: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/pull-requests/433/diff Is it ok, or should I change something? I'm not completely confident about the package (event.inventory, event.player or event.block would all match in some way) and about the class name. |
| Comment by md_5 [ 08/May/19 ] |
|
Uh is it really the right event though? Unless I'm missing something about how to use a lectern, they don't present as anything like a regular inventory. They're closer to the book GUI than anything else.
There's no actual clicking in the inventory going on. There will never be eg a cursor, stuff like "Gets the ItemStack currently in the clicked slot." is meaningless since there is no real "clicked slot" (though the event kind of fakes that out because a lectern is an inventory. |
| Comment by Brokkonaut [ 07/May/19 ] |
|
I know its a different packet on the network level, but it requires an open inventory (the lectern inventory) and has a behaviour very similar to clicking an item in that open inventory (moving an item there). so i think it matches this event quite well. |
| Comment by blablubbabc [ 07/May/19 ] |
|
I don't think the InventoryClickEvent is fitting for this: InventoryClickEvent is corresponding to minecraft's PacketPlayInWindowClick (player clicking an inventory slot), while the lectern (currently) uses the PacketPlayInEnchantItem packet for the different GUI actions / buttons. A new separate event might make more sense. Maybe 'PlayerPickupLecternBookEvent', since there is no other generic event for this kind of pickup-item-from-block action yet? |
| Comment by Brokkonaut [ 07/May/19 ] |
|
PRs: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/pull-requests/433/overview https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/pull-requests/554/overview |
), "EnchantItemEvent", or "CraftItemEvent". Since the event involves a player interacting with an open lectern inventory, it might make sense to let it extend the InventoryInteractEvent (just like the mentioned events extend inventory events as well). It would then also be more clear that the event fits into the event.inventory package, just like you have already placed it. I think this would match the event quite well.