-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add item_pre_anvil event #1384
add item_pre_anvil event #1384
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick review without having compiled or executed the code. It looks like you were successful at sticking to the project structure! Thanks for contributing :)
|
||
@Override | ||
public int getRepairCostAmount() { | ||
return ai.getRepairCost(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this should call getRepairCostAmount()
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it should! Updated.
import java.util.List; | ||
|
||
public interface MCPrepareAnvilEvent extends MCInventoryEvent { | ||
@Override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this override doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, nothing. Actually, that entire getInventory()
definition doesn't need to exist. It's defined in MCInventoryEvent
. Removed it.
public String docs() { | ||
return "{}" | ||
+ " Fires when a recipe is formed in an anvil, but the result has not yet been clicked." | ||
+ " { viewers: all human entities looking at this anvil " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would word this as all players viewing this anvil's interface
or something similar that is a bit more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verbage updated.
+ " { viewers: all human entities looking at this anvil " | ||
+ " | first_item: the first item in the anvil" | ||
+ " | second_item: the second item in the anvil" | ||
+ " | result: the result of the anvil" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would word this as:
the first ingredient in the anvil recipy
the second ingredient in the anvil recipy
the result of the anvil recipy
Or something alike. Again a bit more explicit, leaving less room for misinterpretation (what is the result of an anvil?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verbage updated.
|
||
@Override | ||
public Map<String, Mixed> evaluate(BindableEvent event) throws EventException { | ||
if(event instanceof MCPrepareAnvilEvent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java now has a nice language feature that you can use here.
if(event instanceof MCPrepareAnvilEvent) {
MCPrepareAnvilEvent e = (MCPrepareAnvilEvent) event;
is equivalent to:
if(event instanceof MCPrepareAnvilEvent e) {
It's up to you to decide whether or not you want to use that, but it's been used more and more in CH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I've updated it to use the short-hand.
Looks like you matched 'viewers' from item_pre_craft, whereas we should probably get a player for both of these from the InventoryView. I don't think there can be multiple viewers, at least for these events on these GUIs. I would recommend removing 'viewers' and add 'player' for this event. There's one hitch. InventoryView was changed from an abstract class to an interface a couple of weeks ago. Directly using methods from that interface may not work on older versions of the server. Might need reflection. Something like:
|
Let me test the other event to see if reflection is needed here too. [edit] Confirmed. Needs reflection. |
Actually can probably just do e.getViewers.get(0) instead of reflection. That's probably the way to go. |
Pushed a commit. Something like that? |
That looks fine. It looks impossible to have more than one viewer for an anvil. Unlike shared inventory views, the list of tracked viewers of an anvil is created every time a call to open an anvil is used. |
Just noticed you have question marks in the description of the repair costs. Perhaps they were temporary? |
Kind of flying blind with creating a new event. It's been many years since I've written one, in an extension. 😂
Let me know if there's any validation you'd like for me to do on my end.
Example payload (pretty printed):