Add ScreenEvents.Open

[Fuzss]

This adds a simple callback that runs inside of MinecraftClient::setScreen, allowing for the replacement of the new screen.

This allows for a couple of things:

• Keeping the old screen, preventing the new screen from opening
• Doing some setup on the newly constructed screen instance before it is set (like something that should not run in init)
• Injecting a custom replacement screen for the new screen, useful for something like Better Statistics Screen

Implementation details:
The old screen has already been removed by calling Screen#removed(), and the new screen will always be initialized via Screen#init(MinecraftClient, int, int).
I see no issue with that though, even when keeping the old screen.

This includes a test mod.

[modmuss50]

Id add this to the existing ScreenEvents class as OPEN

[Fuzss]

Also I'd like to have something like an Optional for the return value, so that only when that is present the implementation actually sets the screen.
I find that way more intuitive from just looking at the callback signature than having to figure out from the Javadoc if I need to return one of the input parameters or null for nothing to happen, e.g. when I only want to change something about the screen state without any replacement.

The current implementation follows the design established by other events, so that's why it's like that for now.

[modmuss50]

Im not sure about this, as the existing screen before/after init events provide a lot. As far as I can tell out of your use cases the only one that isnt possible now is Keeping the old screen, preventing the new screen from opening.

Id argue that this would be better done by stopping it at the source (e.g preventing the click) rarther than this late on.

[modmuss50]

We had a similar pattern with canceling the chat events, where mods would not get notified of chat messages if a previous one had already cancled it. Is there a similar concern here? Does the event also fire again for the replaced screen?

[Fuzss]

Yes, that's how it is supposed to be here; first come first serve. That's how most Fabric events work that have a return value. After all there is no concept of receiving "cancelled" events (which is essentially what this is) like there is in NeoForge.

If you prefer we can wrap the newScreen parameter in a MutableObject, so the event is able to run for all listeners and those receive modifications done by others. I do think this is very niche though and do think the current implementation is enough.

[modmuss50]

Some of our events have a canceled counterpart that are invoked when the main one has been cancled. Im not sure how useful that is here.

Will the event be fired again for the new screen or not? I think it would make sense to?

[cputnam-a11y]

Im not sure about this, as the existing screen before/after init events provide a lot. As far as I can tell out of your use cases the only one that isnt possible now is Keeping the old screen, preventing the new screen from opening.

Id argue that this would be better done by stopping it at the source (e.g preventing the click) rarther than this late on.

Is it worth deferring the call to remove until after the event?
Edit: looking at the code, I think it might be, although we would need some java doc or something instructing modders that they can't cancel in-between the event and the remove call without cleaning up the remove call.
Edit 2: it doesn't work because remove is overriden

[cputnam-a11y]

the more I think about this, the more it feels like two separate events.

[Fuzss]

Im not sure about this, as the existing screen before/after init events provide a lot. As far as I can tell out of your use cases the only one that isnt possible now is Keeping the old screen, preventing the new screen from opening.

Id argue that this would be better done by stopping it at the source (e.g preventing the click) rarther than this late on.

The idea is to have every screen that opens go through this, no matter where it opens from. Intercepting at the source is not always possible and far more complicated compared to how simple this callback and its implementation are.

You are right for init though, it is possible to replace a new screen with that like vanilla does when opening the creative mode screen. This is not as clean though and has no way of preventing the original new screen from initializing which could be an issue.

[cputnam-a11y]

the more I think about this, the more it feels like two separate events.

I think there should probably be an earlier (keep the old screen, prevent the new screen from open event) and then make this one a (replace the currently opening screen, where you can pass null to prevent it from opening)

[modmuss50]

Im not sure about this, as the existing screen before/after init events provide a lot. As far as I can tell out of your use cases the only one that isnt possible now is Keeping the old screen, preventing the new screen from opening.
Id argue that this would be better done by stopping it at the source (e.g preventing the click) rarther than this late on.

The idea is to have every screen that opens go through this, no matter where it opens from. Intercepting at the source is not always possible and far more complicated compared to how simple this callback and its implementation are.

You are right for init though, it is possible to replace a new screen with that like vanilla does when opening the creative mode screen. This is not as clean though and has no way of preventing the original new screen from initializing which could be an issue.

Im not sure that opening a screen from within init is really that bad, would a more specific event such as ALLOW_OPEN be better? You could still open a new screen from within the event if you want to replace it. To me the current event overlaps too much with the existing init events?

[Fuzss]

Something along the lines of ALLOW_OPEN is what I had originally; the problem with that is that Screen::removed is called before the new screen is fully computed (vanilla has some extra logic after that for showing the death or title screen).

So implementing that would require delaying the Screen::removed call, or computing the new screen before that. Here is my original Mixin from that taking the second approach: https://github.com/Fuzss/puzzleslib/blob/main/1.20.1/Fabric/src/main/java/fuzs/puzzleslib/mixin/client/MinecraftFabricMixin.java

Regarding using init events; I do have a use case that doesn't really work for: https://github.com/Fuzss/deathfinder/blob/57acee09fa6c0933aebad44aa614463fe37b100e/1.21.5/Common/src/main/java/fuzs/deathfinder/client/handler/DeathScreenHandler.java#L37
The player position must be captured once, when the player has died. It may change afterward, as the dead player entity can still move in the world, but inventory contents are dropped at the original location, which is what I intend on capturing there.

[cputnam-a11y]

I had an idea for actually deferring that removed call without copying vanilla code, but unfortunately it requires mass mixin.

[cputnam-a11y]

I do think the cleanest thing would be two events. I'll sketch it out and show what I think it would look like later