Skip to content

Implemented BlockFailedDispenseEvent#3205

Closed
LB45440078L wants to merge 2 commits into
PaperMC:masterfrom
LB45440078L:master
Closed

Implemented BlockFailedDispenseEvent#3205
LB45440078L wants to merge 2 commits into
PaperMC:masterfrom
LB45440078L:master

Conversation

@LB45440078L
Copy link
Copy Markdown
Contributor

This event should be fired when a block such as a Dispenser or a Dropper tries to dispense an item but it was empty. As KennyTV told me before, and as it also seems a good idea, this event should not be cancellable. The only reason someone could want is as cancellable would be to 'cancel' the 'click' noise produced by the empty dispenser/dropper being triggered.

@Trigary
Copy link
Copy Markdown
Contributor

Trigary commented Apr 22, 2020

I think he meant that it shouldn't be cancellable because you didn't do stuff differently based on whether it was cancelled or not. I personally believe that this event should be cancellable and not make the click sound if it is indeed cancelled, as you said.

@@ -1,5 +1,6 @@
package net.minecraft.server;

+import com.destroystokyo.paper.event.block.BlockFailedDispenseEvent;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You inline the Block import, but not this one. Same in the other file.

Comment thread Spigot-API-Patches/0199-Implemented-BlockFailedDispenseEvent.patch Outdated
Comment thread Spigot-API-Patches/0199-Implemented-BlockFailedDispenseEvent.patch Outdated
…atch

Co-Authored-By: MiniDigger <admin@minidigger.me>
@theminecoder
Copy link
Copy Markdown
Contributor

A cancelable event implies that the entire event can be reverted, which cannot apply to an event that is marking a failure for something to happen.

Making an event like this cancelable to handle playing a sound is like saying you can make the player quit event cancelable to handle the quit message sending.

I personally would prefer that we keep to the standard and have this event be non cancelable and use a field to handle playing of the sound (or even make it so we can change the sound, if it makes it more justifiable)

@Trigary
Copy link
Copy Markdown
Contributor

Trigary commented Apr 22, 2020

By not letting a sound be played you effectively stop this event from doing anything. What is that if not cancelling the event? While I believe we should make the event cancellable instead of adding a method, I don't really care as long as we do get a way to stop the sound from playing.

@EbonJaeger
Copy link
Copy Markdown

The event is for when something has already failed... Being able to cancel it makes no sense. If you want to stop the sound from playing, doing it when something is being dispensed seems like the more logical place to do that, not after it's already happened.

@aikar
Copy link
Copy Markdown
Member

aikar commented Apr 22, 2020

we have another event that lets you control the effect as a result of something else, and we used a separate boolean/setter for it.

That would be more clear to have event.setPlayEffect(false); and no cancellation state on the event.

@Proximyst Proximyst added status: rebase required type: feature Request for a new Feature. labels Aug 24, 2020
@LB45440078L LB45440078L requested a review from a team as a code owner August 25, 2020 02:40
Copy link
Copy Markdown
Member

@electronicboy electronicboy left a comment

Choose a reason for hiding this comment

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

Provisionally approved, but, would be nice to see the playSound suggestion on here implemented

@MiniDigger
Copy link
Copy Markdown
Member

reopened in #4820 since this is bork

@MiniDigger MiniDigger closed this Nov 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants