Event handling for custom click actions

[TheDeathlyCow]

Implements a basic system for registering event listeners to custom click actions. Events are registered by the action ID, so that mods don't have to parse that out themselves. Also uses an event system rather than a map so that multiple mods can listen to the same click events.

Will add javadoc and tests soon:tm:

[modmuss50]

Needs build fixes and tests.

[TheDeathlyCow]

Ok I think this is ready for review now, feel free to roast. I added some tests but my approach for the configuration phase had to be a little weird because of the client tests. There isnt, to my knowledge, a good way to 'manually' request a dialog during the configuration phase, so a mandatory dialog would show up on world join that would block the E2E client tests from being able to run. To get around this, I implemented a mock show dialog packet that mimics the behavior of the regular ShowDialogS2CPacket. When a client receives this packet, rather than displaying a dialog like usual it will just automatically return the regular custom click event response packet that should get handled by the event receiver (so it will be as if a user pressed the custom click event button). It's a little weird, but it works. The play click event test uses a regular multi option dialog and can be displayed with the command /networktestcommand testdialog.

As far as I can tell, the networking API does not use automated tests, so I didn't implement them here. Though it does not seem really easily possible at this time to do that using existing frameworks (fabric-integration-test-api when?).

[modmuss50]

This is looking really good, I think my main worry is around the confusion of CustomClickEventContext. I had in mind a single event where you get passed the CustomClickEventContext and are forced to handle if its play or configuration in your handler. The fact that this has confused you likely means it will also confuse people trying to use this API. So maybe the approach of 2 events as you have here is better, do let me know what you think, dont take what I say as a rule.

[TheDeathlyCow]

I think having a single event and pattern matching two different parameters depending on where it came from feels like an unusual departure from the usual style of events in Fabric. There are also some differences in how custom dialogs need to be set up if they're used in the configuration phase (ie all references must be in-line, as no registries are available). So there's already gotta be special handling for configuration phase dialogs by the developer.

At the same time, it does feel like a bit of a low level concern for something that should probably be a better high level abstraction. And having two separate events might lead to excessive code duplication for use cases that only care about the payload and not the handler (which evidently seems to be mostly what Mojang had in mind when they created this click event). Maybe we could add an Optional<ServerPlayerEntity> parameter, in addition to the handler, that's only available as a shorthand for when the packet is received in the play phase. This might be a good compromise that doesn't involve needing to do pattern matching (in most cases) or sort out too much low level networking stuff.

I might need to spend a bit more time thinking about it and get some more input lol.

[TheDeathlyCow]

I've decided to merge them into a single event, based on what I said earlier about preventing code duplication. The context instanceof checking is weird, but at least with switch statements its not that bad (see tests). I've also moved the tests into a single package and class, which shows how to do the switch-casing and shows an actual dialog using a reconfiguration command.

Finally, I added a new event that is invoked for any custom click action received - and includes the ID. I'm not sure what the use case is for it exactly, but it's something I could see someone asking for down the line. If you think it's too unnecessary, then I can remove it.

[modmuss50]

What is the example use case for this? Generally it seems like you should only care about your own specific click actions?

[TheDeathlyCow]

I had a usecase where I tracked where a user was in a particular flow using the ID of the event. But that is probably not a good way to handle that, adding to payload is better (eg with the additions field). Will remove.

[modmuss50]

Looking good, just a few minor questions/points.

[modmuss50]

Should the default impl for this throw? Something is wrong if this is called but not overriden.

[TheDeathlyCow]

There are some other methods in the addon that are just declared as abstract but have empty implementations (like handleUnregistration).

[modmuss50]

Why a wrap operation and not just an inject?

[cputnam-a11y]

an Inject would require capturing more locals