Skip to content

Remove the concept of read-only orders #2189

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

Merged
merged 2 commits into from
Jul 31, 2020

Conversation

ImMorpheus
Copy link
Contributor

As discussed on discord, read-only orders can't be enforced. This PR removes the concept from the jd.

I think there's value in cancellation allowed (prism-like plugins) in the sense of an event guaranteed not to be cancelled (POST is the last order, wheter the event has been cancelled or not. That's not gonna change after that) so I'm leaving the tab in the jd for discussion.

Close SpongePowered/Sponge#1213 (if we decide to remove both)

@ImMorpheus ImMorpheus added system: event api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023) labels Jul 24, 2020
@dualspiral
Copy link
Contributor

I think there's value in cancellation allowed (prism-like plugins) in the sense of an event guaranteed not to be cancelled

I was having a (very) brief discussion with @gabizou about that, many events have a Post event that is effectively an unmodifiable event. In the case of the phase tracker and block capturing, it would be better to use such events because these occur when everything is said and done, rather than being in a position to allow the action associated with the event to go ahead. So I'd rather ditch the idea of "unmodifiable" cancellation altogether, instead, where it's needed, create Post events that plugins can listen to and know they are read only, uncancellable views of what's happened.

Preventing cancellation won't stop other things being modifed in an event, meaning that the event would still be useless to logging plugins - another reason for a proper read only event.

We have some post events, but there will be some that are missing of course - if there are any that should be there that aren't, I think that's the discussion we really need to have with interested members of the community.

@ImMorpheus
Copy link
Contributor Author

ImMorpheus commented Jul 27, 2020

Preventing cancellation won't stop other things being modifed in an event.

My thought was that this was an edge case, however upon close inspection that's not the case at all. Basically every subclass of AffectEntityEvent, AffectItemStackEvent (inventory events and the likes), etc. can be modified (filter transaction) but not cancelled.

That being said, I've removed the "cancellation allowed" part. There's not much value to it as it is.

@gabizou
Copy link
Member

gabizou commented Jul 28, 2020

The thing is, there's still side effectual modifications that are effectively "permitted". The nature of how some events and listener-based side effects happen though is that the order of operations will be that an event listener (like prism) will get an event from a listener performing side effects as a result of the event that listener received an event, and then the original event, so like this:

Place Wool Block
 |---PluginA Listener
 |       |---Place torches around wool block
 |       |        |---PluginA Listener ignores (because of causes)
 |       |        |---PluginB's Listener gets called
 |       |________|
 |---PluginB's Listener
END

So while we've removed order, we still have a side-effectual understanding that even if the original Place Wool Block was cancelled, the plugin's side effects may not have been cancelled, and therefor still played out., and PluginB will recognize PluginA's changes before the original intention of the Place Wool Block.

@dualspiral
Copy link
Contributor

So while we've removed order

We haven't at all. This is just about removing javadocs that say something that have never been enforced, and the discussion was about the enforcement of cancellation status.

My point was that monitoring plugins want to know what's absolutely happened, not what's about to happen, and a Post event is the way to go. As we control when the post events fire, we can just have one that groups together all the changes made, all the way down the tree for the root cause of the changes after all the side effects play out. Everything you've said there enforces the need for the Post event to remain!

@gabizou
Copy link
Member

gabizou commented Jul 30, 2020

So would it make sense then to change up the ChangeBlockEvent hierarchy to where we have ChangeBlockEvent { Place, Destroy, Modify, Decay, All } Pre, Post? to where we can effectively play a Pre for the first proposed transaction (if it's a child or the start of a chain) and then the Post being for the entire chain with all the children and listener side-effects? I can imagine having to capture listener side-effects being the trouble since that'd effectively mean selective "recording"/"lifting" of transactions from a child PhaseContext..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023) system: event
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants