Skip to content
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

Make ItemInfoEvent cancellable (to cancel /iteminfo command) #349

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

LogGits
Copy link

@LogGits LogGits commented Aug 3, 2020

Making this because I need to cancel /iteminfo depending on if the item has specific properties (e.g. custom nbt data)

Haven't been able to test but because this is so straight forward and small I don't see any way for it to cause any issues.

@Phoenix616
Copy link
Member

Doesn't this result in the item meta information getting sent before the ID messages? For this to work properly without changing the existing output you would need to move the item ID messages to their own listeners too.

@LogGits
Copy link
Author

LogGits commented Aug 3, 2020

Will look into that. So just to be clear, you're saying that both trys are required to be successful before it sends the ItemInfoEvent and if so, why is that?

Do you think we could get away with some sort of cancellable PreItemInfoEvent?

@Phoenix616
Copy link
Member

An additionally event is not necessary, simply make the command fully event driven by moving the two hard coded message sends into their own event handlers (e.g. in the ItemInfoListener class) on a lower priority than the existing meta ones and make sure that all ItemInfoEvent handlers ignore cancelled events as well.

@LogGits
Copy link
Author

LogGits commented Aug 3, 2020

Sure I can do that, do you mind if i add a feedback boolean to the event to prevent sending the feedback message? I want to have a custom message when a player tries to /iteminfo specific items.

@Phoenix616
Copy link
Member

Wouldn't cancelling already do that shop of removing the message?

@LogGits
Copy link
Author

LogGits commented Aug 3, 2020

Fair point, just gotta ignorecancelled real quick and then it should be ready for review.
EDIT: Should be ready for review

Copy link
Member

@Phoenix616 Phoenix616 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall besides the return and empty check as well as the formatting issues.

@LogGits
Copy link
Author

LogGits commented Aug 3, 2020

Will fix this tomorrow, my ide auto formats on saving.

@LogGits LogGits requested a review from Phoenix616 August 6, 2020 07:02
ItemInfoEvent event = new ItemInfoEvent(sender, item);
ChestShop.callEvent(event);

return true;
return !MaterialUtil.isEmpty(item);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure any other event listener doesn't throw an error when the item is null/empty? (As you moved the check from before calling the event to after it and don't check that again anywhere)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants