Allow Message Events depend on mod load order
kevinthegreat1 opened this issue ยท 13 comments
Events such as ClientReceiveMessageEvents.ALLOW_GAME
and ServerMessageEvents.ALLOW_CHAT_MESSAGE
return immediately after a listener returns false
. The remaining listeners do not get called, and the order of the listeners depends on the mod load order.
This is an issue if two mods need to read and block the same message. Only one mod will be able to read the message. Since the first mod blocks the message, the event returns, and the second mod never sees the message.
Proposed change would be to call all listeners regardless of cancellation status.
If two mods want to block the same message, why does it matter if the second never gets to read the message?
It wanted to block the message anyway, and the message has been blocked.
If one listener blocks the message, the message will still be blocked, just that all listeners will still get a chance to read the message. Blocking is the same as old behavior.
If two mods want to block the same message, why does it matter if the second never gets to read the message? It wanted to block the message anyway, and the message has been blocked.
I am specifying situations where the mod needs the messages to work such as notifying the mod of something happening.
Yeah, also we don't want to add a breaking change just for this.
This would be a very minor breaking change. The only difference is that listeners will be called after the message in canceled. The message will still be canceled if one listener cancels.
Mods already should not depend on mod load order, so no mod should be depending on another mod filtering out messages for them.
This would only impact mods that register two or more listeners to the same event and the second listener won't work without the first listener filtering out some messages, which I imagine is a very strange design.
@kevinthegreat1 Hm, how would that work then? If your mod approves something but another mod does not, should the mod "notify"?
@kevinthegreat1 Hmm, so do each listener check independently or should they have some knowledge on whether another mod blocked it, is what I'm saying.
I think either way would be fine.
The API is designed under the principle that, once a message is blocked, it should operate as if it was never sent (at least from mod's view). I don't know under what circumstance you need to know all the received messages, including ones that users don't see.
My use case is that the server sends information via chat and mods need that information. As a quality-of-life feature, mods also hide it from the user, since it's not needed by the user.
I understand that short circuit can be useful to reduce workloads but I think mods are more likely to do that in one listener and not register multiple listeners and depend on fabric api short circuit.
I can't use listener ordering since no matter what, only one listener would be able to read the message. I know that I could mixin, but I thought this is a very minor change with little drawbacks.
@kevinthegreat1 Hmm, so do each listener check independently or should they have some knowledge on whether another mod blocked it, is what I'm saying.
I expect that some mods have checks (like DB query) that benefit from having less workload.
The API is designed under the principle that, once a message is blocked, it should operate as if it was never sent (at least from mod's view). I don't know under what circumstance you need to know all the received messages, including ones that users don't see.
That said, you're free to Mixin yourself, or use the listener ordering in a creative way.
I would need to register two listeners, one to ALLOW
and one to CANCELED
to solve the problem I'm facing. The same logic would have to run twice, which I argue is worse and more wasteful than running all listeners. Again, I am proposing a small change with very limited breaking impact.
Also, server message events do not have CANCELED
events.
That seems fine to me, I assume most of the code can be shared using helper functions. Breaking changes are a no-go.